diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index 9f3e339eb..c1f8ab9ba 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -273,6 +273,38 @@ describe('runNonInteractive', () => { expect(mockShutdownTelemetry).toHaveBeenCalled(); }); + it('on EPIPE, destroys stdout and returns normally instead of process.exit', async () => { + // Regression: process.exit(0) on EPIPE bypassed runExitCleanup → flush() + // and dropped queued JSONL writes for `qwen -p ... | head -1` patterns. + // process.exit is mocked to throw in beforeEach, so reaching the + // assertion also proves the bypass route is gone. + setupMetricsMock(); + const stdoutDestroySpy = vi + .spyOn(process.stdout, 'destroy') + .mockReturnValue(process.stdout); + + mockGeminiClient.sendMessageStream.mockImplementation( + async function* mockStream(): AsyncGenerator { + process.stdout.emit( + 'error', + Object.assign(new Error('EPIPE'), { code: 'EPIPE' }), + ); + yield { type: GeminiEventType.Content, value: 'Hello' }; + yield { + type: GeminiEventType.Finished, + value: { + reason: undefined, + usageMetadata: { totalTokenCount: 0 }, + }, + }; + }, + ); + + await runNonInteractive(mockConfig, mockSettings, 'test', 'p1'); + + expect(stdoutDestroySpy).toHaveBeenCalled(); + }); + it('should handle a single tool call and respond', async () => { setupMetricsMock(); const toolCallEvent: ServerGeminiStreamEvent = { diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index dc90c7775..23965c7d9 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -175,16 +175,22 @@ export async function runNonInteractive( let totalApiDurationMs = 0; const startTime = Date.now(); - const stdoutErrorHandler = (err: NodeJS.ErrnoException) => { - if (err.code === 'EPIPE') { - process.stdout.removeListener('error', stdoutErrorHandler); - process.exit(0); - } - }; - const geminiClient = config.getGeminiClient(); const abortController = options.abortController ?? new AbortController(); + // EPIPE: don't process.exit here — that bypasses the caller's + // runExitCleanup → flush() and drops queued JSONL writes. Destroy + // stdout instead and let the natural return drive cleanup. (Aborting + // is also wrong: the abort path runs handleCancellationError → exit + // 130 and re-introduces the same bypass.) + let pipeBroken = false; + const stdoutErrorHandler = (err: NodeJS.ErrnoException) => { + if (err.code === 'EPIPE' && !pipeBroken) { + pipeBroken = true; + process.stdout.destroy(); + } + }; + // Setup signal handlers for graceful shutdown const shutdownHandler = () => { debugLogger.debug('[runNonInteractive] Shutdown signal received'); diff --git a/packages/cli/src/utils/cleanup.test.ts b/packages/cli/src/utils/cleanup.test.ts index 0b254bac6..f2d5a43df 100644 --- a/packages/cli/src/utils/cleanup.test.ts +++ b/packages/cli/src/utils/cleanup.test.ts @@ -5,19 +5,19 @@ */ import { vi } from 'vitest'; -import { registerCleanup, runExitCleanup } from './cleanup'; +import { + _resetCleanupFunctionsForTest, + registerCleanup, + runExitCleanup, +} from './cleanup'; describe('cleanup', () => { - const originalCleanupFunctions = global['cleanupFunctions']; - beforeEach(() => { - // Isolate cleanup functions for each test - global['cleanupFunctions'] = []; - }); - - afterAll(() => { - // Restore original cleanup functions - global['cleanupFunctions'] = originalCleanupFunctions; + // The previous `global['cleanupFunctions'] = []` setup was dead code — + // the array is module-private, not on `global`. Tests passed by accident + // because `runExitCleanup` itself clears at the end. A test that throws + // before reaching `runExitCleanup` would leak state into the next case. + _resetCleanupFunctionsForTest(); }); it('should run a registered synchronous function', async () => { @@ -65,4 +65,79 @@ describe('cleanup', () => { expect(errorFn).toHaveBeenCalledTimes(1); expect(successFn).toHaveBeenCalledTimes(1); }); + + describe('timeout failsafes', () => { + // Without these the async-jsonl flush() could hang exit forever on slow + // disks / dead sockets — sync writes were inherently bounded, async aren't. + + it('caps a hung cleanup at the per-fn timeout and proceeds to the next one', async () => { + const hangFn = vi.fn(() => new Promise(() => {})); + const nextFn = vi.fn(); + + registerCleanup(hangFn); + registerCleanup(nextFn); + + const start = Date.now(); + await runExitCleanup({ + _testPerFnTimeoutMs: 50, + _testOverallTimeoutMs: 5_000, + }); + const elapsed = Date.now() - start; + + expect(hangFn).toHaveBeenCalledTimes(1); + expect(nextFn).toHaveBeenCalledTimes(1); + expect(elapsed).toBeLessThan(500); + }); + + it('caps overall wall-clock time when many cleanups all hang', async () => { + // 100 × 50ms perFn ≈ 5000ms drain — structurally impossible for "drain + // finished naturally" to satisfy < 800ms, so the upper bound proves + // wallClock actually fired. Lower bound proves we waited for it and + // didn't short-circuit. 800ms slack absorbs CI scheduler jitter. + for (let i = 0; i < 100; i++) { + registerCleanup(() => new Promise(() => {})); + } + + const start = Date.now(); + await runExitCleanup({ + _testPerFnTimeoutMs: 50, + _testOverallTimeoutMs: 100, + }); + const elapsed = Date.now() - start; + + expect(elapsed).toBeLessThan(800); + expect(elapsed).toBeGreaterThanOrEqual(80); + }); + + it('still calls fast cleanups normally when timeouts are configured', async () => { + const fastFn = vi.fn().mockResolvedValue(undefined); + registerCleanup(fastFn); + + await runExitCleanup({ + _testPerFnTimeoutMs: 1_000, + _testOverallTimeoutMs: 2_000, + }); + + expect(fastFn).toHaveBeenCalledTimes(1); + }); + + it('does not let a rejected cleanup poison the chain', async () => { + // The original `for…await` already swallowed sync throws; this guards + // the new withTimeout wrapper against rejected-async-cleanup leaks. + const rejectFn = vi.fn().mockRejectedValue(new Error('boom')); + const nextFn = vi.fn(); + + registerCleanup(rejectFn); + registerCleanup(nextFn); + + await expect( + runExitCleanup({ + _testPerFnTimeoutMs: 50, + _testOverallTimeoutMs: 1_000, + }), + ).resolves.toBeUndefined(); + expect(rejectFn).toHaveBeenCalledTimes(1); + expect(nextFn).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/cli/src/utils/cleanup.ts b/packages/cli/src/utils/cleanup.ts index c7103792f..b64f87c2c 100644 --- a/packages/cli/src/utils/cleanup.ts +++ b/packages/cli/src/utils/cleanup.ts @@ -14,15 +14,93 @@ export function registerCleanup(fn: (() => void) | (() => Promise)) { cleanupFunctions.push(fn); } -export async function runExitCleanup() { - for (const fn of cleanupFunctions) { - try { - await fn(); - } catch (_) { - // Ignore errors during cleanup. +/** + * Per-cleanup ceiling. Caps any single hung cleanup (slow disk on + * `chatRecording.flush`, MCP disconnect on a dead socket, telemetry HTTP + * stall) so it can't starve the rest of the cleanup chain. + */ +const PER_CLEANUP_TIMEOUT_MS = 2_000; + +/** + * Wall-clock ceiling for the whole cleanup pass. Pre-async-jsonl, sync + * fs writes were inherently bounded by their syscall return; with the + * write queue moved off-thread, an unbounded `await flush()` could now + * hang exit indefinitely. This ceiling guarantees the process always + * exits within a bounded time, even if a cleanup never resolves. + */ +const OVERALL_CLEANUP_TIMEOUT_MS = 5_000; + +/** + * Awaits `promise`, but resolves to `undefined` if `ms` elapses first. + * Rejection collapses to the same undefined resolution — caller treats + * cleanup errors as best-effort. Timer is unrefed so it can't keep the + * event loop alive on its own. + */ +function withTimeout(promise: Promise, ms: number): Promise { + return new Promise((resolve) => { + const timer = setTimeout(() => resolve(undefined), ms); + timer.unref?.(); + promise.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + () => { + clearTimeout(timer); + resolve(undefined); + }, + ); + }); +} + +export interface RunExitCleanupOptions { + /** TEST ONLY — override per-cleanup-function timeout (default 2s). */ + _testPerFnTimeoutMs?: number; + /** TEST ONLY — override overall wall-clock timeout (default 5s). */ + _testOverallTimeoutMs?: number; +} + +export async function runExitCleanup( + options: RunExitCleanupOptions = {}, +): Promise { + const perFn = options._testPerFnTimeoutMs ?? PER_CLEANUP_TIMEOUT_MS; + const overall = options._testOverallTimeoutMs ?? OVERALL_CLEANUP_TIMEOUT_MS; + + const drain = (async () => { + for (const fn of cleanupFunctions) { + try { + await withTimeout(Promise.resolve().then(fn), perFn); + } catch (_) { + // Ignore errors during cleanup. + } } + })(); + + // clearTimeout when drain wins; unref keeps the handle from blocking exit. + let wallClockTimer: NodeJS.Timeout | undefined; + const wallClock = new Promise((resolve) => { + wallClockTimer = setTimeout(() => resolve(), overall); + wallClockTimer.unref?.(); + }); + + try { + await Promise.race([drain, wallClock]); + } finally { + if (wallClockTimer) clearTimeout(wallClockTimer); + cleanupFunctions.length = 0; // Clear the array } - cleanupFunctions.length = 0; // Clear the array +} + +/** + * Test-only: clear the registered cleanup functions array. Module-private + * state otherwise leaks across vitest cases — the previous test isolation + * via `global['cleanupFunctions']` was a no-op (the array isn't on global) + * and only happened to work because `runExitCleanup` itself clears at the + * end. Naming follows the `_reset*ForTest` convention from + * d6485964c (paths, jsonl-utils, ripGrep). + */ +export function _resetCleanupFunctionsForTest(): void { + cleanupFunctions.length = 0; } export async function cleanupCheckpoints() { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index e335bbbe7..de4d151b1 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1588,9 +1588,11 @@ export class Config { return; } try { - // Finalize the current session's metadata before cleanup. + // Finalize the current session's metadata before cleanup, then drain + // the async write queue so no records are lost on exit. try { this.chatRecordingService?.finalize(); + await this.chatRecordingService?.flush(); } catch { // Best-effort — don't block shutdown } diff --git a/packages/core/src/services/chatRecordingService.autoTitle.test.ts b/packages/core/src/services/chatRecordingService.autoTitle.test.ts index 152c1d6f3..935f34b56 100644 --- a/packages/core/src/services/chatRecordingService.autoTitle.test.ts +++ b/packages/core/src/services/chatRecordingService.autoTitle.test.ts @@ -68,7 +68,7 @@ async function flushMicrotasks(): Promise { function findCustomTitleRecord(): ChatRecord | undefined { return vi - .mocked(jsonl.writeLineSync) + .mocked(jsonl.writeLine) .mock.calls.map((c) => c[1] as ChatRecord) .find((r) => r.type === 'system' && r.subtype === 'custom_title'); } @@ -133,6 +133,10 @@ describe('ChatRecordingService - auto-title trigger', () => { vi.spyOn(fs, 'existsSync').mockReturnValue(false); chatRecordingService = new ChatRecordingService(mockConfig); + + // writeLine is async; mockResolvedValue lets the writeChain settle when + // tests await flushMicrotasks() / chatRecordingService.flush(). + vi.mocked(jsonl.writeLine).mockResolvedValue(undefined); }); afterEach(() => { @@ -174,7 +178,8 @@ describe('ChatRecordingService - auto-title trigger', () => { it('does not overwrite a manual title', async () => { chatRecordingService.recordCustomTitle('chose-this-myself', 'manual'); - vi.mocked(jsonl.writeLineSync).mockClear(); + await chatRecordingService.flush(); + vi.mocked(jsonl.writeLine).mockClear(); chatRecordingService.recordAssistantTurn({ model: 'qwen-plus', @@ -326,10 +331,13 @@ describe('ChatRecordingService - auto-title trigger', () => { expect(svc.getCurrentCustomTitle()).toBe('Auto-generated title'); expect(svc.getCurrentTitleSource()).toBe('auto'); - // finalize() was called by the constructor — the re-appended record - // must carry titleSource: 'auto', not 'manual'. + // finalize() was called by the constructor — drain the queued async + // write before inspecting the mock. + await svc.flush(); + + // The re-appended record must carry titleSource: 'auto', not 'manual'. const finalizeRecord = vi - .mocked(jsonl.writeLineSync) + .mocked(jsonl.writeLine) .mock.calls.map((c) => c[1] as ChatRecord) .find((r) => r.type === 'system' && r.subtype === 'custom_title'); expect(finalizeRecord?.systemPayload).toEqual({ @@ -362,9 +370,10 @@ describe('ChatRecordingService - auto-title trigger', () => { expect(svc.getCurrentCustomTitle()).toBe('User chose this'); expect(svc.getCurrentTitleSource()).toBe('manual'); + await svc.flush(); const finalizeRecord = vi - .mocked(jsonl.writeLineSync) + .mocked(jsonl.writeLine) .mock.calls.map((c) => c[1] as ChatRecord) .find((r) => r.type === 'system' && r.subtype === 'custom_title'); expect(finalizeRecord?.systemPayload).toEqual({ @@ -395,9 +404,10 @@ describe('ChatRecordingService - auto-title trigger', () => { // Must stay undefined so the JSONL isn't upgraded to a misleading // `titleSource: 'manual'` we can't actually verify. expect(svc.getCurrentTitleSource()).toBeUndefined(); + await svc.flush(); const finalizeRecord = vi - .mocked(jsonl.writeLineSync) + .mocked(jsonl.writeLine) .mock.calls.map((c) => c[1] as ChatRecord) .find((r) => r.type === 'system' && r.subtype === 'custom_title'); // Payload must NOT contain a titleSource field when source is unknown. @@ -494,7 +504,8 @@ describe('ChatRecordingService - auto-title trigger', () => { // User renames while the title LLM call is still pending. chatRecordingService.recordCustomTitle('user-chosen', 'manual'); - vi.mocked(jsonl.writeLineSync).mockClear(); + await chatRecordingService.flush(); + vi.mocked(jsonl.writeLine).mockClear(); // Now the LLM call returns a title. resolveLlm({ ok: true, title: 'Auto Title', modelUsed: 'qwen-turbo' }); diff --git a/packages/core/src/services/chatRecordingService.customTitle.test.ts b/packages/core/src/services/chatRecordingService.customTitle.test.ts index 0842ca635..4fa9246de 100644 --- a/packages/core/src/services/chatRecordingService.customTitle.test.ts +++ b/packages/core/src/services/chatRecordingService.customTitle.test.ts @@ -79,18 +79,22 @@ describe('ChatRecordingService - recordCustomTitle', () => { vi.spyOn(fs, 'existsSync').mockReturnValue(false); chatRecordingService = new ChatRecordingService(mockConfig); + + // writeLine is async; mockResolvedValue lets the writeChain settle on flush. + vi.mocked(jsonl.writeLine).mockResolvedValue(undefined); }); afterEach(() => { vi.restoreAllMocks(); }); - it('should record a custom title as a system record', () => { + it('should record a custom title as a system record', async () => { chatRecordingService.recordCustomTitle('my-feature'); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledOnce(); + expect(jsonl.writeLine).toHaveBeenCalledOnce(); - const writtenRecord = vi.mocked(jsonl.writeLineSync).mock + const writtenRecord = vi.mocked(jsonl.writeLine).mock .calls[0][1] as ChatRecord; expect(writtenRecord.type).toBe('system'); expect(writtenRecord.subtype).toBe('custom_title'); @@ -101,24 +105,26 @@ describe('ChatRecordingService - recordCustomTitle', () => { expect(writtenRecord.sessionId).toBe('test-session-id'); }); - it('should maintain parent chain when recording title after other records', () => { + it('should maintain parent chain when recording title after other records', async () => { chatRecordingService.recordUserMessage([{ text: 'hello' }]); chatRecordingService.recordCustomTitle('my-feature'); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledTimes(2); + expect(jsonl.writeLine).toHaveBeenCalledTimes(2); - const userRecord = vi.mocked(jsonl.writeLineSync).mock + const userRecord = vi.mocked(jsonl.writeLine).mock .calls[0][1] as ChatRecord; - const titleRecord = vi.mocked(jsonl.writeLineSync).mock + const titleRecord = vi.mocked(jsonl.writeLine).mock .calls[1][1] as ChatRecord; expect(titleRecord.parentUuid).toBe(userRecord.uuid); }); - it('should include correct metadata in the record', () => { + it('should include correct metadata in the record', async () => { chatRecordingService.recordCustomTitle('test-title'); + await chatRecordingService.flush(); - const writtenRecord = vi.mocked(jsonl.writeLineSync).mock + const writtenRecord = vi.mocked(jsonl.writeLine).mock .calls[0][1] as ChatRecord; expect(writtenRecord.cwd).toBe('/test/project/root'); @@ -129,15 +135,16 @@ describe('ChatRecordingService - recordCustomTitle', () => { }); describe('finalize', () => { - it('should re-append cached custom title to EOF', () => { + it('should re-append cached custom title to EOF', async () => { chatRecordingService.recordCustomTitle('my-feature'); - vi.mocked(jsonl.writeLineSync).mockClear(); + await chatRecordingService.flush(); + vi.mocked(jsonl.writeLine).mockClear(); chatRecordingService.finalize(); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledOnce(); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[0][1] as ChatRecord; + expect(jsonl.writeLine).toHaveBeenCalledOnce(); + const record = vi.mocked(jsonl.writeLine).mock.calls[0][1] as ChatRecord; expect(record.type).toBe('system'); expect(record.subtype).toBe('custom_title'); expect(record.systemPayload).toEqual({ @@ -146,22 +153,24 @@ describe('ChatRecordingService - recordCustomTitle', () => { }); }); - it('should not write anything when no custom title was set', () => { + it('should not write anything when no custom title was set', async () => { chatRecordingService.finalize(); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).not.toHaveBeenCalled(); + expect(jsonl.writeLine).not.toHaveBeenCalled(); }); - it('should re-append the latest title after multiple renames', () => { + it('should re-append the latest title after multiple renames', async () => { chatRecordingService.recordCustomTitle('first-name'); chatRecordingService.recordCustomTitle('second-name'); - vi.mocked(jsonl.writeLineSync).mockClear(); + await chatRecordingService.flush(); + vi.mocked(jsonl.writeLine).mockClear(); chatRecordingService.finalize(); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledOnce(); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[0][1] as ChatRecord; + expect(jsonl.writeLine).toHaveBeenCalledOnce(); + const record = vi.mocked(jsonl.writeLine).mock.calls[0][1] as ChatRecord; expect(record.systemPayload).toEqual({ customTitle: 'second-name', titleSource: 'manual', diff --git a/packages/core/src/services/chatRecordingService.test.ts b/packages/core/src/services/chatRecordingService.test.ts index 6307c6d67..15173c32b 100644 --- a/packages/core/src/services/chatRecordingService.test.ts +++ b/packages/core/src/services/chatRecordingService.test.ts @@ -82,8 +82,10 @@ describe('ChatRecordingService', () => { chatRecordingService = new ChatRecordingService(mockConfig); - // Mock jsonl-utils - vi.mocked(jsonl.writeLineSync).mockImplementation(() => undefined); + // Mock jsonl-utils. writeLine is async — mockResolvedValue returns + // a settled Promise so the writeChain in ChatRecordingService advances + // when flushed. + vi.mocked(jsonl.writeLine).mockResolvedValue(undefined); }); afterEach(() => { @@ -91,13 +93,13 @@ describe('ChatRecordingService', () => { }); describe('recordUserMessage', () => { - it('should record a user message immediately', () => { + it('should record a user message immediately', async () => { const userParts: Part[] = [{ text: 'Hello, world!' }]; chatRecordingService.recordUserMessage(userParts); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledTimes(1); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[0][1] as ChatRecord; + expect(jsonl.writeLine).toHaveBeenCalledTimes(1); + const record = vi.mocked(jsonl.writeLine).mock.calls[0][1] as ChatRecord; expect(record.uuid).toBe('00000000-0000-0000-0000-000000000001'); expect(record.parentUuid).toBeNull(); @@ -110,15 +112,16 @@ describe('ChatRecordingService', () => { expect(record.gitBranch).toBe('main'); }); - it('should chain messages correctly with parentUuid', () => { + it('should chain messages correctly with parentUuid', async () => { chatRecordingService.recordUserMessage([{ text: 'First message' }]); chatRecordingService.recordAssistantTurn({ model: 'gemini-pro', message: [{ text: 'Response' }], }); chatRecordingService.recordUserMessage([{ text: 'Second message' }]); + await chatRecordingService.flush(); - const calls = vi.mocked(jsonl.writeLineSync).mock.calls; + const calls = vi.mocked(jsonl.writeLine).mock.calls; const user1 = calls[0][1] as ChatRecord; const assistant = calls[1][1] as ChatRecord; const user2 = calls[2][1] as ChatRecord; @@ -135,7 +138,7 @@ describe('ChatRecordingService', () => { }); describe('recordAtCommand', () => { - it('should record @-command metadata as a system payload', () => { + it('should record @-command metadata as a system payload', async () => { const userParts: Part[] = [{ text: 'Hello, world!' }]; const payload: AtCommandRecordPayload = { filesRead: ['foo.txt'], @@ -146,11 +149,12 @@ describe('ChatRecordingService', () => { chatRecordingService.recordUserMessage(userParts); chatRecordingService.recordAtCommand(payload); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledTimes(2); - const userRecord = vi.mocked(jsonl.writeLineSync).mock + expect(jsonl.writeLine).toHaveBeenCalledTimes(2); + const userRecord = vi.mocked(jsonl.writeLine).mock .calls[0][1] as ChatRecord; - const systemRecord = vi.mocked(jsonl.writeLineSync).mock + const systemRecord = vi.mocked(jsonl.writeLine).mock .calls[1][1] as ChatRecord; expect(userRecord.type).toBe('user'); @@ -162,16 +166,16 @@ describe('ChatRecordingService', () => { }); describe('recordAssistantTurn', () => { - it('should record assistant turn with content only', () => { + it('should record assistant turn with content only', async () => { const parts: Part[] = [{ text: 'Hello!' }]; chatRecordingService.recordAssistantTurn({ model: 'gemini-pro', message: parts, }); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledTimes(1); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[0][1] as ChatRecord; + expect(jsonl.writeLine).toHaveBeenCalledTimes(1); + const record = vi.mocked(jsonl.writeLine).mock.calls[0][1] as ChatRecord; expect(record.type).toBe('assistant'); // The service wraps parts in a Content object using createModelContent @@ -181,7 +185,7 @@ describe('ChatRecordingService', () => { expect(record.toolCallResult).toBeUndefined(); }); - it('should record assistant turn with all data', () => { + it('should record assistant turn with all data', async () => { const parts: Part[] = [ { thought: true, text: 'Thinking...' }, { text: 'Here is the result.' }, @@ -197,9 +201,9 @@ describe('ChatRecordingService', () => { totalTokenCount: 160, }, }); + await chatRecordingService.flush(); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[0][1] as ChatRecord; + const record = vi.mocked(jsonl.writeLine).mock.calls[0][1] as ChatRecord; // The service wraps parts in a Content object using createModelContent expect(record.message).toEqual({ role: 'model', parts }); @@ -207,7 +211,7 @@ describe('ChatRecordingService', () => { expect(record.usageMetadata?.totalTokenCount).toBe(160); }); - it('should record assistant turn with only tokens', () => { + it('should record assistant turn with only tokens', async () => { chatRecordingService.recordAssistantTurn({ model: 'gemini-pro', tokens: { @@ -217,9 +221,9 @@ describe('ChatRecordingService', () => { totalTokenCount: 30, }, }); + await chatRecordingService.flush(); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[0][1] as ChatRecord; + const record = vi.mocked(jsonl.writeLine).mock.calls[0][1] as ChatRecord; expect(record.message).toBeUndefined(); expect(record.usageMetadata?.totalTokenCount).toBe(30); @@ -227,7 +231,7 @@ describe('ChatRecordingService', () => { }); describe('recordToolResult', () => { - it('should record tool result with Parts', () => { + it('should record tool result with Parts', async () => { // First record a user and assistant message to set up the chain chatRecordingService.recordUserMessage([{ text: 'Hello' }]); chatRecordingService.recordAssistantTurn({ @@ -246,17 +250,17 @@ describe('ChatRecordingService', () => { }, ]; chatRecordingService.recordToolResult(toolResultParts); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledTimes(3); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[2][1] as ChatRecord; + expect(jsonl.writeLine).toHaveBeenCalledTimes(3); + const record = vi.mocked(jsonl.writeLine).mock.calls[2][1] as ChatRecord; expect(record.type).toBe('tool_result'); // The service wraps parts in a Content object using createUserContent expect(record.message).toEqual({ role: 'user', parts: toolResultParts }); }); - it('should record tool result with toolCallResult metadata', () => { + it('should record tool result with toolCallResult metadata', async () => { const toolResultParts: Part[] = [ { functionResponse: { @@ -275,9 +279,9 @@ describe('ChatRecordingService', () => { } as any; chatRecordingService.recordToolResult(toolResultParts, metadata); + await chatRecordingService.flush(); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[0][1] as ChatRecord; + const record = vi.mocked(jsonl.writeLine).mock.calls[0][1] as ChatRecord; expect(record.type).toBe('tool_result'); // The service wraps parts in a Content object using createUserContent @@ -286,7 +290,7 @@ describe('ChatRecordingService', () => { expect(record.toolCallResult?.callId).toBe('call-1'); }); - it('should chain tool result correctly with parentUuid', () => { + it('should chain tool result correctly with parentUuid', async () => { chatRecordingService.recordUserMessage([{ text: 'Hello' }]); chatRecordingService.recordAssistantTurn({ model: 'gemini-pro', @@ -302,12 +306,13 @@ describe('ChatRecordingService', () => { }, ]; chatRecordingService.recordToolResult(toolResultParts); + await chatRecordingService.flush(); - const userRecord = vi.mocked(jsonl.writeLineSync).mock + const userRecord = vi.mocked(jsonl.writeLine).mock .calls[0][1] as ChatRecord; - const assistantRecord = vi.mocked(jsonl.writeLineSync).mock + const assistantRecord = vi.mocked(jsonl.writeLine).mock .calls[1][1] as ChatRecord; - const toolResultRecord = vi.mocked(jsonl.writeLineSync).mock + const toolResultRecord = vi.mocked(jsonl.writeLine).mock .calls[2][1] as ChatRecord; expect(userRecord.parentUuid).toBeNull(); @@ -317,15 +322,15 @@ describe('ChatRecordingService', () => { }); describe('recordSlashCommand', () => { - it('should record slash command with payload and subtype', () => { + it('should record slash command with payload and subtype', async () => { chatRecordingService.recordSlashCommand({ phase: 'invocation', rawCommand: '/about', }); + await chatRecordingService.flush(); - expect(jsonl.writeLineSync).toHaveBeenCalledTimes(1); - const record = vi.mocked(jsonl.writeLineSync).mock - .calls[0][1] as ChatRecord; + expect(jsonl.writeLine).toHaveBeenCalledTimes(1); + const record = vi.mocked(jsonl.writeLine).mock.calls[0][1] as ChatRecord; expect(record.type).toBe('system'); expect(record.subtype).toBe('slash_command'); @@ -335,16 +340,17 @@ describe('ChatRecordingService', () => { }); }); - it('should chain slash command after prior records', () => { + it('should chain slash command after prior records', async () => { chatRecordingService.recordUserMessage([{ text: 'Hello' }]); chatRecordingService.recordSlashCommand({ phase: 'result', rawCommand: '/about', }); + await chatRecordingService.flush(); - const userRecord = vi.mocked(jsonl.writeLineSync).mock + const userRecord = vi.mocked(jsonl.writeLine).mock .calls[0][1] as ChatRecord; - const slashRecord = vi.mocked(jsonl.writeLineSync).mock + const slashRecord = vi.mocked(jsonl.writeLine).mock .calls[1][1] as ChatRecord; expect(userRecord.parentUuid).toBeNull(); @@ -352,6 +358,77 @@ describe('ChatRecordingService', () => { }); }); + describe('flush', () => { + it('resolves immediately on a service with no enqueued writes', async () => { + // The writeChain starts as Promise.resolve(), so flush() on a fresh + // service should settle in a single microtask — important because + // Config.shutdown awaits flush on every exit path, even for sessions + // that never recorded anything. + await expect(chatRecordingService.flush()).resolves.toBeUndefined(); + expect(jsonl.writeLine).not.toHaveBeenCalled(); + }); + + it('a failed write does not block subsequent records', async () => { + // Regression guard: the inner .catch swallows fs errors and keeps + // the chain alive so the next record's write still runs. + vi.mocked(jsonl.writeLine).mockRejectedValueOnce( + new Error('simulated EACCES'), + ); + chatRecordingService.recordUserMessage([{ text: 'first' }]); + chatRecordingService.recordUserMessage([{ text: 'second' }]); + await chatRecordingService.flush(); + + expect(jsonl.writeLine).toHaveBeenCalledTimes(2); + const second = vi.mocked(jsonl.writeLine).mock.calls[1][1] as ChatRecord; + expect( + (second.message as { parts: Array<{ text: string }> }).parts[0].text, + ).toBe('second'); + }); + }); + + describe('ensureChatsDir caching', () => { + it('does not cache when mkdirSync throws so the next write retries', async () => { + // Regression: a transient mkdir failure used to poison the cache and + // silently drop the rest of the session's records. We have to fail + // both mkdir AND the wx-create, otherwise ensureConversationFile's + // own cache short-circuits ensureChatsDir on the second call. + const mkdirSpy = vi.spyOn(fs, 'mkdirSync'); + mkdirSpy.mockImplementationOnce(() => { + throw Object.assign(new Error('EACCES'), { code: 'EACCES' }); + }); + mkdirSpy.mockImplementation(() => undefined); + + const writeSpy = vi.spyOn(fs, 'writeFileSync'); + writeSpy.mockImplementationOnce(() => { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + writeSpy.mockImplementation(() => undefined); + + chatRecordingService.recordUserMessage([{ text: 'first' }]); + await chatRecordingService.flush(); + chatRecordingService.recordUserMessage([{ text: 'second' }]); + await chatRecordingService.flush(); + + // ≥ rather than === leaves room for a future flush()-side retry. + expect(mkdirSpy.mock.calls.length).toBeGreaterThanOrEqual(2); + }); + + it('caches after a successful mkdir so steady-state writes skip the syscall', async () => { + const mkdirSpy = vi + .spyOn(fs, 'mkdirSync') + .mockImplementation(() => undefined); + + chatRecordingService.recordUserMessage([{ text: 'first' }]); + await chatRecordingService.flush(); + chatRecordingService.recordUserMessage([{ text: 'second' }]); + await chatRecordingService.flush(); + chatRecordingService.recordUserMessage([{ text: 'third' }]); + await chatRecordingService.flush(); + + expect(mkdirSpy).toHaveBeenCalledTimes(1); + }); + }); + // Note: Session management tests (listSessions, loadSession, deleteSession, etc.) // have been moved to sessionService.test.ts // Session resume integration tests should test via SessionService mock diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index c833ae367..7732be1a5 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -240,6 +240,20 @@ export class ChatRecordingService { /** UUID of the last written record in the chain */ private lastRecordUuid: string | null = null; private readonly config: Config; + /** + * Cached chats-dir / conversation-file path so per-record appendRecord + * doesn't re-stat them on every write. The first call performs the + * mkdir / wx-create; subsequent calls short-circuit. + */ + private chatsDirEnsured = false; + private cachedConversationFile: string | undefined; + /** + * Serialized async write queue for appendRecord. We update lastRecordUuid + * synchronously so the next createBaseRecord sees the right parentUuid, + * but the actual fs write runs in this chain so the event loop is not + * blocked. Must be flushed before process exit (see {@link flush}). + */ + private writeChain: Promise = Promise.resolve(); /** In-memory cache of the current session's custom title (for re-append on exit) */ private currentCustomTitle: string | undefined; /** @@ -332,38 +346,42 @@ export class ChatRecordingService { const projectDir = this.config.storage.getProjectDir(); const chatsDir = path.join(projectDir, 'chats'); + if (this.chatsDirEnsured) { + return chatsDir; + } try { fs.mkdirSync(chatsDir, { recursive: true }); + // Only cache success — keep transient mkdir failures self-healing. + this.chatsDirEnsured = true; } catch { - // Ignore errors - directory will be created if it doesn't exist + // ignored } - return chatsDir; } /** * Ensures the conversation file exists, creating it if it doesn't exist. - * Uses atomic file creation to avoid race conditions. + * Uses atomic file creation to avoid race conditions. Result is cached so + * subsequent appendRecord calls skip the wx-create entirely. * @returns The path to the conversation file. * @throws Error if the file cannot be created or accessed. */ private ensureConversationFile(): string { + if (this.cachedConversationFile) { + return this.cachedConversationFile; + } const chatsDir = this.ensureChatsDir(); const sessionId = this.getSessionId(); const safeFilename = `${sessionId}.jsonl`; const conversationFile = path.join(chatsDir, safeFilename); - if (fs.existsSync(conversationFile)) { - return conversationFile; - } - try { - // Use 'wx' flag for exclusive creation - atomic operation that fails if file exists - // This avoids the TOCTOU race condition of existsSync + writeFileSync + // Use 'wx' flag for exclusive creation - atomic operation that fails if + // the file already exists. EEXIST is the expected steady-state path on + // resume; we treat it as success. fs.writeFileSync(conversationFile, '', { flag: 'wx', encoding: 'utf8' }); } catch (error) { const nodeError = error as NodeJS.ErrnoException; - // EEXIST means file already exists, which is expected and fine if (nodeError.code !== 'EEXIST') { const message = error instanceof Error ? error.message : String(error); throw new Error( @@ -372,6 +390,7 @@ export class ChatRecordingService { } } + this.cachedConversationFile = conversationFile; return conversationFile; } @@ -395,17 +414,45 @@ export class ChatRecordingService { /** * Appends a record to the session file and updates lastRecordUuid. + * + * lastRecordUuid is updated synchronously so the next createBaseRecord sees + * the correct parentUuid without waiting for the previous write. The actual + * fs write is enqueued on {@link writeChain} and runs async; per-file + * mutex inside {@link jsonl.writeLine} preserves on-disk ordering. + * + * **Known tradeoff (parentUuid chain integrity on write failure):** if the + * enqueued write rejects (e.g., disk full, permission dropped), the error + * is logged but subsequent records still claim the failed record's uuid + * as their parent. On resume, readers that walk parentUuid (e.g. + * sessionService.reconstructHistory) will silently drop records whose + * ancestor is missing on disk. This matches the sync version's behavior + * when its own throw was caught and logged by the caller — under normal + * local-disk writes failures are rare enough to accept the fire-and-forget + * simplification. */ private appendRecord(record: ChatRecord): void { + let conversationFile: string; try { - const conversationFile = this.ensureConversationFile(); - - jsonl.writeLineSync(conversationFile, record); - this.lastRecordUuid = record.uuid; + conversationFile = this.ensureConversationFile(); } catch (error) { debugLogger.error('Error appending record:', error); throw error; } + this.lastRecordUuid = record.uuid; + this.writeChain = this.writeChain + .catch(() => {}) + .then(() => jsonl.writeLine(conversationFile, record)) + .catch((err) => { + debugLogger.error('Error appending record (async):', err); + }); + } + + /** + * Awaits all queued async writes. Call before process exit / session + * teardown to ensure no records are dropped. + */ + async flush(): Promise { + await this.writeChain; } /** diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 7cbf33677..b0d01aa4c 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -21,6 +21,37 @@ import type { PermissionDecision } from '../permissions/types.js'; const debugLogger = createDebugLogger('RIPGREP'); +/** + * Per-process cache for `.qwenignore` discovery. The same directories show + * up across many Grep invocations in a typical session — without caching, + * each invocation pays 2-3 sync syscalls per searchPath. Bounded so a + * pathologically long session can't grow without limit. + * + * `dirIsDir`: searchPath → boolean (is the path itself a directory?) + * `qwenIgnore`: dir → string | null (cached `.qwenignore` path or null) + * + * **Known staleness window:** a `.qwenignore` created mid-session, or a + * searchPath whose type flips (dir→file or vice versa), will not be + * picked up until the entry rotates out of the FIFO (256 entries). Users + * rarely add ignore files mid-session; a process restart resets the cache. + */ +const dirIsDirCache = new Map(); +const qwenIgnoreCache = new Map(); +const RIPGREP_CACHE_MAX = 256; +function trimCache(m: Map): void { + if (m.size <= RIPGREP_CACHE_MAX) return; + const oldest = m.keys().next().value; + if (oldest !== undefined) m.delete(oldest as K); +} + +/** + * Test-only: clear ripGrep's module-level discovery caches between cases. + */ +export function _resetRipGrepCachesForTest(): void { + dirIsDirCache.clear(); + qwenIgnoreCache.clear(); +} + /** * Parameters for the GrepTool (Simplified) */ @@ -253,15 +284,25 @@ class GrepToolInvocation extends BaseToolInvocation< // Load .qwenignore from each workspace directory, not just the primary one const seenIgnoreFiles = new Set(); for (const searchPath of paths) { - const dir = - fs.existsSync(searchPath) && fs.statSync(searchPath).isDirectory() - ? searchPath - : path.dirname(searchPath); - const qwenIgnorePath = path.join(dir, '.qwenignore'); - if ( - !seenIgnoreFiles.has(qwenIgnorePath) && - fs.existsSync(qwenIgnorePath) - ) { + let isDir = dirIsDirCache.get(searchPath); + if (isDir === undefined) { + try { + isDir = fs.statSync(searchPath).isDirectory(); + } catch { + isDir = false; + } + dirIsDirCache.set(searchPath, isDir); + trimCache(dirIsDirCache); + } + const dir = isDir ? searchPath : path.dirname(searchPath); + let qwenIgnorePath = qwenIgnoreCache.get(dir); + if (qwenIgnorePath === undefined) { + const candidate = path.join(dir, '.qwenignore'); + qwenIgnorePath = fs.existsSync(candidate) ? candidate : null; + qwenIgnoreCache.set(dir, qwenIgnorePath); + trimCache(qwenIgnoreCache); + } + if (qwenIgnorePath && !seenIgnoreFiles.has(qwenIgnorePath)) { rgArgs.push('--ignore-file', qwenIgnorePath); seenIgnoreFiles.add(qwenIgnorePath); } diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 8428e4090..79467fb0e 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -18,6 +18,7 @@ import { ToolErrorType } from '../tools/tool-error.js'; import { BINARY_EXTENSIONS } from './ignorePatterns.js'; import type { Config } from '../config/config.js'; import { createDebugLogger } from './debugLogger.js'; +import { isNodeError } from './errors.js'; import type { InputModalities } from '../core/contentGenerator.js'; import { detectEncodingFromBuffer } from './systemEncoding.js'; import { extractPDFText, parsePDFPageRange } from './pdf.js'; @@ -581,17 +582,24 @@ export async function processSingleFileContent( ): Promise { const rootDirectory = config.getTargetDir(); try { - if (!fs.existsSync(filePath)) { - // Sync check is acceptable before async read - return { - llmContent: - 'Could not read file because no file was found at the specified path.', - returnDisplay: 'File not found.', - error: `File not found: ${filePath}`, - errorType: ToolErrorType.FILE_NOT_FOUND, - }; + let stats: import('node:fs').Stats; + try { + // Async stat doubles as the existence check — ENOENT is handled below + // and surfaces the same FILE_NOT_FOUND error type as the old explicit + // existsSync gate, with one fewer sync syscall on the hot path. + stats = await fs.promises.stat(filePath); + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'ENOENT') { + return { + llmContent: + 'Could not read file because no file was found at the specified path.', + returnDisplay: 'File not found.', + error: `File not found: ${filePath}`, + errorType: ToolErrorType.FILE_NOT_FOUND, + }; + } + throw error; } - const stats = await fs.promises.stat(filePath); if (stats.isDirectory()) { return { llmContent: diff --git a/packages/core/src/utils/jsonl-utils.ts b/packages/core/src/utils/jsonl-utils.ts index 7c74282ef..639076a1a 100644 --- a/packages/core/src/utils/jsonl-utils.ts +++ b/packages/core/src/utils/jsonl-utils.ts @@ -109,23 +109,39 @@ export async function read(filePath: string): Promise { } } +/** + * Per-directory cache: once we've successfully created a parent dir we don't + * need to mkdir again on subsequent writes. Cuts an async syscall off every + * hot-path write (chat session JSONL appends). + */ +const ensuredDirs = new Set(); + +/** + * Test-only: clear the per-directory mkdir cache. Needed when tests mutate + * fs state at the same directory path across cases. + */ +export function _resetEnsuredDirsCacheForTest(): void { + ensuredDirs.clear(); +} + /** * Appends a line to a JSONL file with concurrency control. - * This method uses a mutex to ensure only one write happens at a time per file. + * Uses a per-file mutex so concurrent callers serialize, and `fs.promises` + * so the actual I/O does not block the event loop. */ export async function writeLine( filePath: string, data: unknown, ): Promise { const lock = getFileLock(filePath); - await lock.runExclusive(() => { + await lock.runExclusive(async () => { const line = `${JSON.stringify(data)}\n`; - // Ensure directory exists before writing const dir = path.dirname(filePath); - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true }); + if (!ensuredDirs.has(dir)) { + await fs.promises.mkdir(dir, { recursive: true }); + ensuredDirs.add(dir); } - fs.appendFileSync(filePath, line, 'utf8'); + await fs.promises.appendFile(filePath, line, 'utf8'); }); } diff --git a/packages/core/src/utils/paths.test.ts b/packages/core/src/utils/paths.test.ts index 9f8b63ef9..c8b5c0343 100644 --- a/packages/core/src/utils/paths.test.ts +++ b/packages/core/src/utils/paths.test.ts @@ -7,7 +7,15 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { + describe, + it, + expect, + beforeAll, + beforeEach, + afterAll, + vi, +} from 'vitest'; import { escapePath, resolvePath, @@ -18,6 +26,7 @@ import { shortenPath, tildeifyPath, getProjectHash, + _resetValidatePathCacheForTest, } from './paths.js'; import type { Config } from '../config/config.js'; @@ -431,6 +440,14 @@ describe('validatePath', () => { }); }); + beforeEach(() => { + // Module-level isDirectory cache persists across tests; tests here + // mutate the same absolute paths between cases (create file, remove, + // re-create as potentially-different type) so we reset to avoid stale + // lookups masking regressions. + _resetValidatePathCacheForTest(); + }); + afterAll(() => { fs.rmSync(workspaceRoot, { recursive: true, force: true }); }); @@ -482,6 +499,36 @@ describe('validatePath', () => { expect(() => validatePath(config, workspaceRoot)).not.toThrow(); }); + it('does not cache ENOENT — recreating the path between calls succeeds', () => { + // Regression guard: a path that's missing at first-check, then created, + // must NOT be rejected on the second call. Positive stats are cached; + // ENOENT paths are not. This lets the model create a file with Edit + // and then have the next tool call see it. + const ephemeralDir = path.join(workspaceRoot, 'late-created'); + expect(() => validatePath(config, ephemeralDir)).toThrowError( + /Path does not exist:/, + ); + fs.mkdirSync(ephemeralDir); + try { + expect(() => validatePath(config, ephemeralDir)).not.toThrow(); + } finally { + fs.rmSync(ephemeralDir, { recursive: true, force: true }); + } + }); + + it('caches positive isDirectory — repeat call does not re-stat', () => { + const spy = vi.spyOn(fs, 'statSync'); + const dir = path.join(workspaceRoot, 'subdir'); + try { + validatePath(config, dir); + const afterFirst = spy.mock.calls.length; + validatePath(config, dir); + expect(spy.mock.calls.length).toBe(afterFirst); + } finally { + spy.mockRestore(); + } + }); + it('validates paths in allowed directories', () => { const extraDir = fs.mkdtempSync(path.join(os.tmpdir(), 'validate-extra-')); try { diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index 6067c5dc4..0a75d7048 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -14,6 +14,31 @@ import { isNodeError } from './errors.js'; export const QWEN_DIR = '.qwen'; export const GOOGLE_ACCOUNTS_FILENAME = 'google_accounts.json'; +/** + * Cache for `validatePath`'s isDirectory check. Only positive results are + * cached — ENOENT and other errors fall through every time so a freshly + * created file is picked up immediately. Same path validated by back-to-back + * tool calls (very common: model reads several files in one dir) used to + * cost one syscall each. + * + * **Known tradeoff:** if a path is deleted and recreated as a different + * type (dir→file or file→dir) within the same process, the cache returns + * the stale type. The downstream tool will then hit a meaningful error + * (e.g., "not a directory") instead of a clean "does not exist", but no + * files are corrupted. This is rare enough in model-driven workflows that + * we accept the staleness for the common-case perf win. + */ +const isDirectoryCache = new Map(); +const VALIDATE_PATH_CACHE_MAX = 1024; + +/** + * Test-only: clear the validatePath stat cache. Module-level state would + * otherwise leak across vitest cases — `beforeEach(() => _resetValidatePathCacheForTest())`. + */ +export function _resetValidatePathCacheForTest(): void { + isDirectoryCache.clear(); +} + /** * Special characters that need to be escaped in file paths for shell compatibility. * Includes: spaces, parentheses, brackets, braces, semicolons, ampersands, pipes, @@ -314,16 +339,24 @@ export function validatePath( return; } - try { - const stats = fs.statSync(resolvedPath); - if (!allowFiles && !stats.isDirectory()) { - throw new Error(`Path is not a directory: ${resolvedPath}`); + let isDirectory = isDirectoryCache.get(resolvedPath); + if (isDirectory === undefined) { + try { + isDirectory = fs.statSync(resolvedPath).isDirectory(); + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'ENOENT') { + throw new Error(`Path does not exist: ${resolvedPath}`); + } + throw error; } - } catch (error: unknown) { - if (isNodeError(error) && error.code === 'ENOENT') { - throw new Error(`Path does not exist: ${resolvedPath}`); + if (isDirectoryCache.size >= VALIDATE_PATH_CACHE_MAX) { + const oldest = isDirectoryCache.keys().next().value; + if (oldest !== undefined) isDirectoryCache.delete(oldest); } - throw error; + isDirectoryCache.set(resolvedPath, isDirectory); + } + if (!allowFiles && !isDirectory) { + throw new Error(`Path is not a directory: ${resolvedPath}`); } } diff --git a/packages/core/src/utils/workspaceContext.ts b/packages/core/src/utils/workspaceContext.ts index 5f052100d..aaabcd17c 100755 --- a/packages/core/src/utils/workspaceContext.ts +++ b/packages/core/src/utils/workspaceContext.ts @@ -23,6 +23,16 @@ export class WorkspaceContext { private directories = new Set(); private initialDirectories: Set; private onDirectoriesChangedListeners = new Set<() => void>(); + /** + * Memoized realpath results. Every workspace-bounded tool call ultimately + * routes through {@link fullyResolvedPath} → `fs.realpathSync`; without + * this cache the same path gets re-resolved on every Read/Glob/Grep/Ls + * invocation. Bounded so long sessions touching many files don't grow + * without limit; FIFO eviction is good enough — the working set tends to + * be the small set of paths the model is actively manipulating. + */ + private resolvedPathCache = new Map(); + private static readonly RESOLVED_PATH_CACHE_MAX = 1024; /** * Creates a new WorkspaceContext with the given initial directory and optional additional directories. @@ -201,10 +211,21 @@ export class WorkspaceContext { * Fully resolves a path, including symbolic links. * If the path does not exist, it returns the fully resolved path as it would be * if it did exist. + * + * Result is memoized in {@link resolvedPathCache}. Filesystem-state cache: + * if a file is renamed / a symlink is retargeted mid-session the cache + * goes stale, which is the same correctness profile as any single + * `realpathSync` call (it captures a moment in time). The win is cutting + * 8+ syscalls per tool-heavy prompt down to 1. */ private fullyResolvedPath(pathToCheck: string): string { + const cached = this.resolvedPathCache.get(pathToCheck); + if (cached !== undefined) { + return cached; + } + let resolved: string; try { - return fs.realpathSync(pathToCheck); + resolved = fs.realpathSync(pathToCheck); } catch (e: unknown) { if ( isNodeError(e) && @@ -215,10 +236,21 @@ export class WorkspaceContext { !this.isFileSymlink(e.path) ) { // If it doesn't exist, e.path contains the fully resolved path. - return e.path; + resolved = e.path; + } else { + // Don't cache exceptions — the path may exist on retry. + throw e; } - throw e; } + if ( + this.resolvedPathCache.size >= WorkspaceContext.RESOLVED_PATH_CACHE_MAX + ) { + // FIFO eviction: drop the oldest insertion (Map preserves insert order). + const oldest = this.resolvedPathCache.keys().next().value; + if (oldest !== undefined) this.resolvedPathCache.delete(oldest); + } + this.resolvedPathCache.set(pathToCheck, resolved); + return resolved; } /**