mirror of
https://github.com/microsoft/vscode.git
synced 2026-06-07 16:16:58 +01:00
Agent host: clearer worktree git timeout errors and 60s budget (#318242)
* Agent host: clearer worktree git timeout errors and 60s budget The 30s timeout in addWorktree/addExistingWorktree/removeWorktree was fine under normal conditions but bumped to 60s to absorb transient disk-I/O contention on the remote (e.g. many concurrent npm installs across multiple agent sessions). When git timed out, _runGit re-threw `new Error(stderr || error.message)`, which lost the timeout indicator. For `git worktree add`, stderr only contains git's progress meter (`Updating files: 0% (149/14834)`), so the surfaced error looked like git progress instead of a timeout. Now _runGit uses its own timer to flag the timeout case definitively and a new formatGitError helper produces messages like: git worktree timed out after 60000ms: <stderr tail> git worktree exited with code 128: fatal: invalid reference: ... The underlying ExecFileException is preserved as Error.cause. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback on agent-host git error reporting - Use child.on('exit') to clear the timeout, so 'timer' is no longer referenced in the execFile callback before its declaration. - Add summarizeStderrForError() to squash carriage-return-heavy progress meter output into a single short line for the thrown error message, with a 200-char cap. - Log the full unmodified stderr at warn level via ILogService whenever git exits with an error, so the raw output is still available even though the thrown error message is summarized. - Add unit tests for summarizeStderrForError and update the existing formatGitError timeout test to exercise multi-line input. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Rob Lourens
GitHub
@@ -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<boolean> {
|
||||
@@ -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<void> {
|
||||
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<void> {
|
||||
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<boolean> {
|
||||
@@ -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<string, string>; readonly maxBuffer?: number }): Promise<string | undefined> {
|
||||
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).
|
||||
|
||||
@@ -33,7 +33,7 @@ function createGitService(disposables: Pick<DisposableStore, 'add'>): AgentHostG
|
||||
const fileService = disposables.add(new FileService(logService));
|
||||
disposables.add(fileService.registerProvider(Schemas.file, disposables.add(new DiskFileSystemProvider(logService))));
|
||||
const env: Partial<INativeEnvironmentService> = { tmpDir: URI.file(tmpdir()) };
|
||||
return new AgentHostGitService(fileService, env as INativeEnvironmentService);
|
||||
return new AgentHostGitService(fileService, env as INativeEnvironmentService, logService);
|
||||
}
|
||||
|
||||
suite('AgentHostGitService - getSessionGitState (real git)', () => {
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user