From ff7efdacf5b3251c1af7d8d4bb9272b7fae76478 Mon Sep 17 00:00:00 2001 From: Adam Weidman Date: Mon, 3 Aug 2026 22:53:19 +0000 Subject: [PATCH 1/4] fix(core): reuse in-memory conversation when session file reload fails ChatRecordingService.initialize() re-read the session file from disk when resuming and threw 'Failed to load resumed session data from file' if the read returned null, discarding the in-memory conversation it had just been handed. Because tryCompressChat() re-initializes the chat with the live session as resumed data, any reload hiccup (missing file, corrupt metadata line, or a transient I/O error) surfaced to the user as 'Failed to compress chat history: Failed to initialize chat: ...' and made /compress unusable. Fall back to the supplied in-memory conversation instead of throwing, and rewrite a clean session file from it (atomically, via temp file + rename) so later appends and future loads succeed. --- .../src/services/chatRecordingService.test.ts | 48 +++++++++++++++++++ .../core/src/services/chatRecordingService.ts | 48 ++++++++++++++++++- 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/packages/core/src/services/chatRecordingService.test.ts b/packages/core/src/services/chatRecordingService.test.ts index 133e9ffe4db..b5f138750bd 100644 --- a/packages/core/src/services/chatRecordingService.test.ts +++ b/packages/core/src/services/chatRecordingService.test.ts @@ -308,6 +308,54 @@ describe('ChatRecordingService', () => { )) as ConversationRecord; expect(conversation.sessionId).toBe('old-session-id'); }); + + it('should fall back to the in-memory conversation when the file cannot be reloaded', async () => { + // Regression test for the `/compress` "Failed to load resumed session + // data from file" bug: when resuming with a filePath that cannot be + // loaded from disk, initialize must NOT throw. It should adopt the + // in-memory conversation it was handed and rewrite a clean file. + const chatsDir = path.join(testTempDir, 'chats'); + fs.mkdirSync(chatsDir, { recursive: true }); + const missingFile = path.join(chatsDir, 'missing-session.jsonl'); + expect(fs.existsSync(missingFile)).toBe(false); + + const inMemoryConversation = { + sessionId: 'resumed-session-id', + projectHash: 'resumed-project-hash', + startTime: new Date().toISOString(), + lastUpdated: new Date().toISOString(), + messages: [ + { + id: 'msg-1', + type: 'user', + timestamp: new Date().toISOString(), + content: 'hello from memory', + }, + ], + } as unknown as ConversationRecord; + + await expect( + chatRecordingService.initialize({ + filePath: missingFile, + conversation: inMemoryConversation, + }), + ).resolves.not.toThrow(); + + // The in-memory conversation is adopted. + expect(chatRecordingService.getConversation()?.sessionId).toBe( + 'resumed-session-id', + ); + + // A clean, loadable file is rewritten from the in-memory copy so future + // loads and appends succeed. + const reloaded = (await loadConversationRecord( + missingFile, + )) as ConversationRecord; + expect(reloaded).not.toBeNull(); + expect(reloaded.sessionId).toBe('resumed-session-id'); + expect(reloaded.projectHash).toBe('resumed-project-hash'); + expect(reloaded.messages).toHaveLength(1); + }); }); describe('recordMessage', () => { diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index 18b977bf00e..2769e507513 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -462,7 +462,16 @@ export class ChatRecordingService { // Update the session ID in the existing file this.updateMetadata({ sessionId: this.sessionId }); } else { - throw new Error('Failed to load resumed session data from file'); + // The file could not be reloaded (missing, corrupt metadata, or an + // I/O error). Fall back to the in-memory conversation we were handed + // rather than failing the caller, and rewrite a clean file from it. + debugLogger.warn( + 'Failed to reload resumed session data from file; falling back ' + + 'to the in-memory conversation.', + ); + this.cachedConversation = resumedSessionData.conversation; + this.projectHash = this.cachedConversation.projectHash; + this.rewriteConversationFile(this.cachedConversation); } } else { // Create new session @@ -563,6 +572,43 @@ export class ChatRecordingService { } } + /** + * Rewrites the session file from an in-memory record, atomically (temp file + * + rename) so a partial write never clobbers the existing file. + */ + private rewriteConversationFile(conversation: ConversationRecord): void { + if (!this.conversationFile) return; + + // Normalize legacy `.json` paths to the `.jsonl` format we write. + if (this.conversationFile.endsWith('.json')) { + this.conversationFile = this.conversationFile + 'l'; + } + + const { messages, memoryScratchpad, ...metadata } = conversation; + const lines: string[] = [JSON.stringify(metadata)]; + for (const msg of messages) { + lines.push(JSON.stringify(msg)); + } + if (memoryScratchpad) { + lines.push(JSON.stringify({ $set: { memoryScratchpad } })); + } + const content = lines.join('\n') + '\n'; + + try { + fs.mkdirSync(path.dirname(this.conversationFile), { recursive: true }); + const tempFile = `${this.conversationFile}.tmp-${process.pid}`; + fs.writeFileSync(tempFile, content); + fs.renameSync(tempFile, this.conversationFile); + } catch (error) { + if (isNodeError(error) && error.code === 'ENOSPC') { + this.conversationFile = null; + debugLogger.warn(ENOSPC_WARNING_MESSAGE); + } else { + throw error; + } + } + } + private updateMetadata(updates: Partial): void { if (!this.cachedConversation) return; Object.assign(this.cachedConversation, updates); From c1f80ef2e8846bbb56b71361a1d2327deb739e83 Mon Sep 17 00:00:00 2001 From: Adam Weidman Date: Mon, 3 Aug 2026 22:53:39 +0000 Subject: [PATCH 2/4] fix(cli): record tool responses before quota-fallback early return When a quota error switched the active model mid-session, handleCompletedTools() marked the tools as submitted and returned early without ever recording their functionResponse parts. The model turn holding the matching functionCall was already in history, so this left a dangling tool call that violates the Gemini API invariant requiring every functionCall to be followed by its functionResponse. The malformed history persisted (the quota flag only resets on the next non-continuation query), so subsequent requests were sent with an unpaired call and the model continued/autocompleted the user's next message instead of answering it. Record the responses before returning, mirroring the two cancellation branches. The turn still does not auto-continue on the fallback model. --- .../cli/src/ui/hooks/useGeminiStream.test.tsx | 95 +++++++++++++++++++ packages/cli/src/ui/hooks/useGeminiStream.ts | 13 ++- 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 9a35305cb67..03b8caf7d14 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -1046,6 +1046,101 @@ describe('useGeminiStream', () => { }); }); + it('should record tool responses in history when the model was switched due to a quota error', async () => { + // Regression test: returning early on a quota-triggered model switch + // without recording the responses leaves the already-recorded + // functionCall unpaired, which corrupts all subsequent requests. + const responseParts: Part[] = [ + { + functionResponse: { + name: 'testTool', + id: 'call1', + response: { output: 'tool result' }, + }, + }, + ]; + const completedToolCalls: TrackedToolCall[] = [ + { + request: { + callId: 'call1', + name: 'testTool', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-id-quota', + }, + status: CoreToolCallStatus.Success, + responseSubmittedToGemini: false, + response: { + callId: 'call1', + responseParts, + errorType: undefined, + }, + tool: { displayName: 'MockTool' }, + invocation: { + getDescription: () => `Mock description`, + } as unknown as AnyToolInvocation, + } as TrackedCompletedToolCall, + ]; + + const client = new MockedGeminiClientClass(mockConfig); + + let capturedOnComplete: + | ((completedTools: TrackedToolCall[]) => Promise) + | null = null; + + mockUseToolScheduler.mockImplementation((onComplete) => { + capturedOnComplete = onComplete; + return [ + [], + mockScheduleToolCalls, + mockMarkToolsAsSubmitted, + vi.fn(), + mockCancelAllToolCalls, + 0, + ]; + }); + + await renderHookWithProviders(() => + useGeminiStream( + client, + [], + mockAddItem, + mockConfig, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + true, // modelSwitchedFromQuotaError + () => {}, + () => {}, + () => {}, + 80, + 24, + ), + ); + + await act(async () => { + if (capturedOnComplete) { + await new Promise((resolve) => setTimeout(resolve, 0)); + await capturedOnComplete(completedToolCalls); + } + }); + + await waitFor(() => { + expect(mockMarkToolsAsSubmitted).toHaveBeenCalledWith(['call1']); + // The tool response must be paired with its functionCall in history... + expect(client.addHistory).toHaveBeenCalledWith({ + role: 'user', + parts: responseParts, + }); + // ...but the turn must NOT auto-continue on the fallback model. + expect(mockSendMessageStream).not.toHaveBeenCalled(); + }); + }); + it('should NOT stop responding when only update_topic is called', async () => { const topicToolCalls: TrackedToolCall[] = [ { diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 9458c6a4ff4..33f59844536 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -2130,8 +2130,19 @@ export const useGeminiStream = ( markToolsAsSubmitted(callIdsToMarkAsSubmitted); - // Don't continue if model was switched due to quota error + // Don't continue if model was switched due to quota error, but still + // record the responses: the matching functionCall is already in history, + // and leaving it unpaired corrupts every subsequent request. if (modelSwitchedFromQuotaError) { + const combinedParts = geminiTools.flatMap( + (toolCall) => toolCall.response.responseParts, + ); + if (geminiClient && combinedParts.length > 0) { + await geminiClient.addHistory({ + role: 'user', + parts: combinedParts, + }); + } return; } From 9760f2efea0e0e4558db2217589c39d5cbf8ebbb Mon Sep 17 00:00:00 2001 From: Adam Weidman Date: Mon, 3 Aug 2026 23:07:22 +0000 Subject: [PATCH 3/4] fix(core,cli): preserve unreadable session files and defer steering hint Two refinements to the fixes in this PR: - chatRecordingService: the recovery rewrite now moves the unreadable session file aside instead of overwriting it. The reload may have failed only transiently (a lock or I/O blip) on a file that is actually intact, so its bytes are kept rather than destroyed. - useGeminiStream: move the quota-fallback early return above the steering hint block. The hint is built to be sent to the model, but this path returns without submitting, so consuming it there both dropped the user's steering text and wrote it into history ahead of the functionResponse parts. Leaving it unconsumed keeps the recorded turn to just the tool responses and lets the hint ride along with the next real submit. --- .../cli/src/ui/hooks/useGeminiStream.test.tsx | 10 ++++- packages/cli/src/ui/hooks/useGeminiStream.ts | 39 +++++++++---------- .../src/services/chatRecordingService.test.ts | 38 ++++++++++++++++++ .../core/src/services/chatRecordingService.ts | 24 +++++++++++- 4 files changed, 87 insertions(+), 24 deletions(-) diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 03b8caf7d14..abbe933abff 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -1083,6 +1083,7 @@ describe('useGeminiStream', () => { ]; const client = new MockedGeminiClientClass(mockConfig); + const mockConsumeUserHint = vi.fn(() => 'switch to the nprd database'); let capturedOnComplete: | ((completedTools: TrackedToolCall[]) => Promise) @@ -1119,6 +1120,8 @@ describe('useGeminiStream', () => { () => {}, 80, 24, + false, + mockConsumeUserHint, ), ); @@ -1131,13 +1134,16 @@ describe('useGeminiStream', () => { await waitFor(() => { expect(mockMarkToolsAsSubmitted).toHaveBeenCalledWith(['call1']); - // The tool response must be paired with its functionCall in history... + // The tool response must be paired with its functionCall in history, + // with no steering-hint text ahead of it... expect(client.addHistory).toHaveBeenCalledWith({ role: 'user', parts: responseParts, }); - // ...but the turn must NOT auto-continue on the fallback model. + // ...the turn must NOT auto-continue on the fallback model... expect(mockSendMessageStream).not.toHaveBeenCalled(); + // ...and the pending hint is left for the next real submit. + expect(mockConsumeUserHint).not.toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 33f59844536..7965852dc1a 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -2110,42 +2110,41 @@ export const useGeminiStream = ( (toolCall) => toolCall.response.responseParts, ); - if (consumeUserHint) { - const userHint = consumeUserHint(); - if (userHint && userHint.trim().length > 0) { - const hintText = userHint.trim(); - responsesToSend.unshift({ - text: buildUserSteeringHintPrompt(hintText), - }); - } - } - const callIdsToMarkAsSubmitted = geminiTools.map( (toolCall) => toolCall.request.callId, ); - const prompt_ids = geminiTools.map( - (toolCall) => toolCall.request.prompt_id, - ); - markToolsAsSubmitted(callIdsToMarkAsSubmitted); // Don't continue if model was switched due to quota error, but still // record the responses: the matching functionCall is already in history, - // and leaving it unpaired corrupts every subsequent request. + // and leaving it unpaired corrupts every subsequent request. Any pending + // steering hint is deliberately left unconsumed so it rides along with + // the next query the user actually submits. if (modelSwitchedFromQuotaError) { - const combinedParts = geminiTools.flatMap( - (toolCall) => toolCall.response.responseParts, - ); - if (geminiClient && combinedParts.length > 0) { + if (geminiClient && responsesToSend.length > 0) { await geminiClient.addHistory({ role: 'user', - parts: combinedParts, + parts: responsesToSend, }); } return; } + if (consumeUserHint) { + const userHint = consumeUserHint(); + if (userHint && userHint.trim().length > 0) { + const hintText = userHint.trim(); + responsesToSend.unshift({ + text: buildUserSteeringHintPrompt(hintText), + }); + } + } + + const prompt_ids = geminiTools.map( + (toolCall) => toolCall.request.prompt_id, + ); + // eslint-disable-next-line @typescript-eslint/no-floating-promises submitQuery( responsesToSend, diff --git a/packages/core/src/services/chatRecordingService.test.ts b/packages/core/src/services/chatRecordingService.test.ts index b5f138750bd..de67088cf4c 100644 --- a/packages/core/src/services/chatRecordingService.test.ts +++ b/packages/core/src/services/chatRecordingService.test.ts @@ -356,6 +356,44 @@ describe('ChatRecordingService', () => { expect(reloaded.projectHash).toBe('resumed-project-hash'); expect(reloaded.messages).toHaveLength(1); }); + + it('should preserve an unreadable session file instead of destroying it', async () => { + // The reload may have failed only transiently, so the original bytes + // must survive the recovery rewrite. + const chatsDir = path.join(testTempDir, 'chats'); + fs.mkdirSync(chatsDir, { recursive: true }); + const sessionFile = path.join(chatsDir, 'unreadable.jsonl'); + + // No usable metadata line => loadConversationRecord() returns null. + const originalBytes = '{"not":"a valid metadata line"}\n'; + fs.writeFileSync(sessionFile, originalBytes); + + await chatRecordingService.initialize({ + filePath: sessionFile, + conversation: { + sessionId: 'recovered-session-id', + projectHash: 'recovered-project-hash', + startTime: new Date().toISOString(), + lastUpdated: new Date().toISOString(), + messages: [], + } as unknown as ConversationRecord, + }); + + // The rewritten file is loadable again... + const reloaded = (await loadConversationRecord( + sessionFile, + )) as ConversationRecord; + expect(reloaded.sessionId).toBe('recovered-session-id'); + + // ...and the original bytes were kept alongside it. + const preserved = fs + .readdirSync(chatsDir) + .filter((f) => f.startsWith('unreadable.jsonl.unreadable-')); + expect(preserved).toHaveLength(1); + expect(fs.readFileSync(path.join(chatsDir, preserved[0]), 'utf-8')).toBe( + originalBytes, + ); + }); }); describe('recordMessage', () => { diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index 2769e507513..549aac4841d 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -573,8 +573,9 @@ export class ChatRecordingService { } /** - * Rewrites the session file from an in-memory record, atomically (temp file - * + rename) so a partial write never clobbers the existing file. + * Rewrites the session file from an in-memory record. Any existing + * (unreadable) file is preserved alongside rather than destroyed, and the + * new file is written atomically (temp file + rename). */ private rewriteConversationFile(conversation: ConversationRecord): void { if (!this.conversationFile) return; @@ -596,6 +597,25 @@ export class ChatRecordingService { try { fs.mkdirSync(path.dirname(this.conversationFile), { recursive: true }); + + // The existing file was unreadable, but it may have been only + // transiently so (a lock or I/O blip) rather than truly corrupt. Keep + // its bytes rather than destroying them. + if (fs.existsSync(this.conversationFile)) { + const backup = `${this.conversationFile}.unreadable-${Date.now()}`; + try { + fs.renameSync(this.conversationFile, backup); + debugLogger.warn( + `Preserved the unreadable session file at ${backup}.`, + ); + } catch (backupError) { + debugLogger.error( + 'Failed to preserve the unreadable session file.', + backupError, + ); + } + } + const tempFile = `${this.conversationFile}.tmp-${process.pid}`; fs.writeFileSync(tempFile, content); fs.renameSync(tempFile, this.conversationFile); From a9ed0651eabc0fb9d0bfdf84d5c3356c92395b86 Mon Sep 17 00:00:00 2001 From: Adam Weidman Date: Wed, 5 Aug 2026 17:08:23 +0000 Subject: [PATCH 4/4] fix(core): clean up the temp file when the session rewrite fails If writeFileSync succeeded but renameSync did not, the .tmp-* file was left orphaned next to the session file. Remove it on the failure path and rethrow, so the original error still surfaces. Cleanup is done in catch rather than finally: after a successful rename the temp path no longer exists, so unlinking unconditionally would issue a pointless syscall and swallow an ENOENT on every healthy write. Kept synchronous to match the rest of this service. appendRecord writes every message with fs.appendFileSync, and introducing an await between the write and the rename would let a sync append land on a file that is about to be replaced. --- .../src/services/chatRecordingService.test.ts | 33 +++++++++++++++++++ .../core/src/services/chatRecordingService.ts | 14 ++++++-- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/packages/core/src/services/chatRecordingService.test.ts b/packages/core/src/services/chatRecordingService.test.ts index de67088cf4c..8a63e3e541e 100644 --- a/packages/core/src/services/chatRecordingService.test.ts +++ b/packages/core/src/services/chatRecordingService.test.ts @@ -394,6 +394,39 @@ describe('ChatRecordingService', () => { originalBytes, ); }); + + it('should not leave a temp file behind when the rewrite fails', async () => { + const chatsDir = path.join(testTempDir, 'chats'); + fs.mkdirSync(chatsDir, { recursive: true }); + const sessionFile = path.join(chatsDir, 'rewrite-fails.jsonl'); + + // Fail the rename that publishes the temp file, leaving it orphaned. + const realRename = fs.renameSync; + vi.spyOn(fs, 'renameSync').mockImplementation((from, to) => { + if (String(from).includes('.tmp-')) { + throw new Error('simulated rename failure'); + } + return realRename(from, to); + }); + + await expect( + chatRecordingService.initialize({ + filePath: sessionFile, + conversation: { + sessionId: 'temp-cleanup-session', + projectHash: 'temp-cleanup-hash', + startTime: new Date().toISOString(), + lastUpdated: new Date().toISOString(), + messages: [], + } as unknown as ConversationRecord, + }), + ).rejects.toThrow('simulated rename failure'); + + const leftovers = fs + .readdirSync(chatsDir) + .filter((f) => f.includes('.tmp-')); + expect(leftovers).toEqual([]); + }); }); describe('recordMessage', () => { diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index 549aac4841d..186282eb1d0 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -617,8 +617,18 @@ export class ChatRecordingService { } const tempFile = `${this.conversationFile}.tmp-${process.pid}`; - fs.writeFileSync(tempFile, content); - fs.renameSync(tempFile, this.conversationFile); + try { + fs.writeFileSync(tempFile, content); + fs.renameSync(tempFile, this.conversationFile); + } catch (error) { + // The rename did not complete, so the temp file would be left behind. + try { + fs.unlinkSync(tempFile); + } catch { + // Ignore cleanup errors so the original failure still surfaces. + } + throw error; + } } catch (error) { if (isNodeError(error) && error.code === 'ENOSPC') { this.conversationFile = null;