diff --git a/src/vs/platform/agentHost/node/agentHostGitService.ts b/src/vs/platform/agentHost/node/agentHostGitService.ts index 4ea7d0d900b..f53c0e83840 100644 --- a/src/vs/platform/agentHost/node/agentHostGitService.ts +++ b/src/vs/platform/agentHost/node/agentHostGitService.ts @@ -10,6 +10,7 @@ import { generateUuid } from '../../../base/common/uuid.js'; import { INativeEnvironmentService } from '../../environment/common/environment.js'; import { IFileService } from '../../files/common/files.js'; import { createDecorator } from '../../instantiation/common/instantiation.js'; +import { ILogService } from '../../log/common/log.js'; import { FileEditKind, type ISessionFileDiff, type ISessionGitState } from '../common/state/sessionState.js'; import { buildGitBlobUri } from './gitDiffContent.js'; @@ -179,6 +180,7 @@ export class AgentHostGitService implements IAgentHostGitService { constructor( @IFileService private readonly _fileService: IFileService, @INativeEnvironmentService private readonly _environmentService: INativeEnvironmentService, + @ILogService private readonly _logService: ILogService, ) { } async isInsideWorkTree(workingDirectory: URI): Promise { @@ -250,15 +252,15 @@ export class AgentHostGitService implements IAgentHostGitService { // tracking from the start point (e.g. when starting from // 'origin/main', without --no-track git would set the new branch's // upstream to origin/main, which would mis-attribute pushes/pulls). - await this._runGit(repositoryRoot, ['worktree', 'add', '--no-track', '-b', branchName, worktree.fsPath, resolvedStartPoint], { timeout: 30_000, throwOnError: true }); + await this._runGit(repositoryRoot, ['worktree', 'add', '--no-track', '-b', branchName, worktree.fsPath, resolvedStartPoint], { timeout: 60_000, throwOnError: true }); } async addExistingWorktree(repositoryRoot: URI, worktree: URI, branchName: string): Promise { - await this._runGit(repositoryRoot, ['worktree', 'add', worktree.fsPath, branchName], { timeout: 30_000, throwOnError: true }); + await this._runGit(repositoryRoot, ['worktree', 'add', worktree.fsPath, branchName], { timeout: 60_000, throwOnError: true }); } async removeWorktree(repositoryRoot: URI, worktree: URI): Promise { - await this._runGit(repositoryRoot, ['worktree', 'remove', '--force', worktree.fsPath], { timeout: 30_000, throwOnError: true }); + await this._runGit(repositoryRoot, ['worktree', 'remove', '--force', worktree.fsPath], { timeout: 60_000, throwOnError: true }); } async branchExists(repositoryRoot: URI, branchName: string): Promise { @@ -542,13 +544,25 @@ export class AgentHostGitService implements IAgentHostGitService { private _runGit(workingDirectory: URI, args: readonly string[], options?: { readonly timeout?: number; readonly throwOnError?: boolean; readonly env?: Record; readonly maxBuffer?: number }): Promise { return new Promise((resolve, reject) => { const env = options?.env ? { ...process.env, ...options.env } : undefined; + const timeoutMs = options?.timeout ?? 5000; + // Use our own timer rather than execFile's `timeout` option so + // we can definitively flag the timeout case in the error + // message — execFile only surfaces signal/killed, which can + // also mean the process was killed for other reasons. + let didTimeOut = false; // Default maxBuffer is 32MB — Node's default is ~1MB, which is // easy to exceed for diff output in large repos. Exceeding it // causes execFile to error and we'd silently drop the diff. - cp.execFile('git', [...args], { cwd: workingDirectory.fsPath, timeout: options?.timeout ?? 5000, env, maxBuffer: options?.maxBuffer ?? 32 * 1024 * 1024 }, (error, stdout, stderr) => { + const child = cp.execFile('git', [...args], { cwd: workingDirectory.fsPath, env, maxBuffer: options?.maxBuffer ?? 32 * 1024 * 1024 }, (error, stdout, stderr) => { if (error) { + // stderr is summarized in the thrown error message to keep + // it readable; log the full unmodified output here so the + // raw progress/diagnostic text is still available. + if (stderr) { + this._logService.warn(`[agentHostGitService] git ${args.join(' ')} failed; full stderr:\n${stderr}`); + } if (options?.throwOnError) { - reject(new Error(stderr || error.message)); + reject(new Error(formatGitError(args, timeoutMs, didTimeOut, error, stderr), { cause: error })); return; } resolve(undefined); @@ -556,10 +570,62 @@ export class AgentHostGitService implements IAgentHostGitService { } resolve(stdout); }); + const timer = setTimeout(() => { + didTimeOut = true; + child.kill(); + }, timeoutMs); + child.on('exit', () => clearTimeout(timer)); }); } } +/** + * Builds a diagnostic error message for a failed `git` invocation that + * preserves the reason (timeout / signal / exit code) instead of just + * surfacing whatever happened to be on stderr. When `git` is killed by + * the timeout, stderr often contains only progress output (e.g. + * `Updating files: 0% (149/14834)`), so without the timeout indicator + * the bubbled-up error is misleading. + * + * Exported for tests. + */ +export function formatGitError(args: readonly string[], timeoutMs: number, didTimeOut: boolean, error: cp.ExecFileException, stderr: string): string { + const subcommand = args[0] ?? '(unknown)'; + let reason: string; + if (didTimeOut) { + reason = `git ${subcommand} timed out after ${timeoutMs}ms`; + } else if (error.killed && error.signal) { + reason = `git ${subcommand} killed by ${error.signal}`; + } else if (typeof error.code === 'number') { + reason = `git ${subcommand} exited with code ${error.code}`; + } else { + reason = error.message; + } + const detail = summarizeStderrForError(stderr); + return detail ? `${reason}: ${detail}` : reason; +} + +/** + * Squashes multi-line / carriage-return-heavy stderr (e.g. git progress + * meters that emit `Updating files: 0% (149/14834)\r...` repeatedly) + * into a single short line suitable for a one-liner error message. + * Keeps the most recent non-empty line and caps total length. + * + * Exported for tests. + */ +export function summarizeStderrForError(stderr: string): string { + if (!stderr) { + return ''; + } + const lines = stderr.split(/[\r\n]+/g).map(line => line.trim()).filter(line => line.length > 0); + if (lines.length === 0) { + return ''; + } + const last = lines[lines.length - 1]; + const MAX = 200; + return last.length > MAX ? `${last.slice(0, MAX - 1)}…` : last; +} + /** * The well-known SHA-1 of git's empty tree, used as a fallback when a * repository has no commits (no `HEAD` to read into the temp index). diff --git a/src/vs/platform/agentHost/test/node/agentHostGitService.integrationTest.ts b/src/vs/platform/agentHost/test/node/agentHostGitService.integrationTest.ts index 9da0a2473a0..45e965346b5 100644 --- a/src/vs/platform/agentHost/test/node/agentHostGitService.integrationTest.ts +++ b/src/vs/platform/agentHost/test/node/agentHostGitService.integrationTest.ts @@ -33,7 +33,7 @@ function createGitService(disposables: Pick): AgentHostG const fileService = disposables.add(new FileService(logService)); disposables.add(fileService.registerProvider(Schemas.file, disposables.add(new DiskFileSystemProvider(logService)))); const env: Partial = { tmpDir: URI.file(tmpdir()) }; - return new AgentHostGitService(fileService, env as INativeEnvironmentService); + return new AgentHostGitService(fileService, env as INativeEnvironmentService, logService); } suite('AgentHostGitService - getSessionGitState (real git)', () => { diff --git a/src/vs/platform/agentHost/test/node/agentHostGitService.test.ts b/src/vs/platform/agentHost/test/node/agentHostGitService.test.ts index 87b4a024db5..452c179a0a0 100644 --- a/src/vs/platform/agentHost/test/node/agentHostGitService.test.ts +++ b/src/vs/platform/agentHost/test/node/agentHostGitService.test.ts @@ -5,7 +5,7 @@ import assert from 'assert'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { EMPTY_TREE_OBJECT, getBranchCompletions, parseDefaultBranchRef, parseGitDiffRawNumstat, parseGitHubRepoFromRemote, parseGitStatusV2, parseHasGitHubRemote, parseUntrackedPaths } from '../../node/agentHostGitService.js'; +import { EMPTY_TREE_OBJECT, formatGitError, getBranchCompletions, parseDefaultBranchRef, parseGitDiffRawNumstat, parseGitHubRepoFromRemote, parseGitStatusV2, parseHasGitHubRemote, parseUntrackedPaths, summarizeStderrForError } from '../../node/agentHostGitService.js'; import { buildGitBlobUri } from '../../node/gitDiffContent.js'; import { URI } from '../../../../base/common/uri.js'; @@ -269,5 +269,66 @@ suite('AgentHostGitService', () => { test('exports the well-known empty-tree object SHA', () => { assert.strictEqual(EMPTY_TREE_OBJECT, '4b825dc642cb6eb9a060e54bf8d69288fbee4904'); }); + + suite('formatGitError', () => { + test('reports timeout when our timer fired and summarises progress-meter stderr', () => { + const err = Object.assign(new Error('Command failed'), { killed: true, signal: 'SIGTERM' as const }); + const progress = 'Updating files: 0% (7/14834)\rUpdating files: 0% (149/14834)\r'; + assert.strictEqual( + formatGitError(['worktree', 'add', '-b', 'x', '/tmp/y', 'origin/main'], 30_000, true, err, progress), + 'git worktree timed out after 30000ms: Updating files: 0% (149/14834)', + ); + }); + + test('reports kill signal when killed but not by our timer', () => { + const err = Object.assign(new Error('Command failed'), { killed: true, signal: 'SIGTERM' as const }); + assert.strictEqual( + formatGitError(['worktree', 'add'], 30_000, false, err, ''), + 'git worktree killed by SIGTERM', + ); + }); + + test('reports numeric exit code when git failed normally', () => { + const err = Object.assign(new Error('Command failed'), { code: 128 }); + assert.strictEqual( + formatGitError(['worktree', 'add', '/tmp/y', 'missing-branch'], 30_000, false, err, 'fatal: invalid reference: missing-branch\n'), + 'git worktree exited with code 128: fatal: invalid reference: missing-branch', + ); + }); + + test('falls back to error message when there is no signal or exit code', () => { + const err = new Error('spawn git ENOENT'); + assert.strictEqual( + formatGitError(['status'], 5_000, false, err, ''), + 'spawn git ENOENT', + ); + }); + }); + + suite('summarizeStderrForError', () => { + test('returns empty string for empty input', () => { + assert.strictEqual(summarizeStderrForError(''), ''); + }); + + test('returns empty string for whitespace-only input', () => { + assert.strictEqual(summarizeStderrForError(' \r\n\r\n '), ''); + }); + + test('keeps the last non-empty line of a multi-line progress meter', () => { + const progress = 'Updating files: 0% (7/14834)\rUpdating files: 0% (149/14834)\r'; + assert.strictEqual(summarizeStderrForError(progress), 'Updating files: 0% (149/14834)'); + }); + + test('passes through a normal single-line message', () => { + assert.strictEqual(summarizeStderrForError('fatal: invalid reference: x\n'), 'fatal: invalid reference: x'); + }); + + test('truncates very long lines with an ellipsis', () => { + const long = `fatal: ${'a'.repeat(500)}`; + const result = summarizeStderrForError(long); + assert.strictEqual(result.length, 200); + assert.ok(result.endsWith('…'), 'expected trailing ellipsis'); + }); + }); });