From e9d48bcf7b7bf314ada8d5d6afc829bd5405780e Mon Sep 17 00:00:00 2001 From: nordicnode Date: Mon, 31 Aug 2026 14:21:58 -0700 Subject: [PATCH 1/2] fix(cli): recover agent context when run-state.json is torn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A resumed chat could start amnesiac — the transcript intact, the model with no memory of earlier turns — through a chain with no single point of failure: - writeFileAtomic renamed the temp over the target without fsync, so a power loss could land the rename while the data blocks were never written: the 'atomic' file contained garbage. fsync the temp before renaming (both sync and async paths). - loadMostRecentChatState tried only the primary and fell back to a RunState placeholder with no sessionState. The SDK starts a fresh session when previousRun.sessionState is absent, so the next turn silently lost every earlier turn. Now the load tries the rotated .bak (the previous complete generation) and then the newest complete checkpoint temp (a SIGKILL between write and rename leaves one behind), self-heals the primary from whichever recovered, and only gives up when all three are unreadable. - The loss used to be invisible to the user: the transcript rendered normally and the model just 'forgot'. loadMostRecentChatState now reports whether agent context survived, and the resume flow prepends an error-variant notice when it did not. Also: retry the async rename briefly on EPERM/EBUSY/EACCES — on Windows the just-closed handle can still be held by AV/indexer scans, which the new fsync widens the window for. Refs #1166 (persistence-side companion to the compaction wipes) --- cli/src/hooks/use-send-message.ts | 16 +- .../utils/__tests__/run-state-storage.test.ts | 143 +++++++++++++++ cli/src/utils/run-state-storage.ts | 164 ++++++++++++++++-- cli/src/utils/write-file-atomic.ts | 82 +++++++-- 4 files changed, 380 insertions(+), 25 deletions(-) diff --git a/cli/src/hooks/use-send-message.ts b/cli/src/hooks/use-send-message.ts index 698859a44e..de9d5ba263 100644 --- a/cli/src/hooks/use-send-message.ts +++ b/cli/src/hooks/use-send-message.ts @@ -187,7 +187,21 @@ export const useSendMessage = ({ if (loadedState) { previousRunStateRef.current = loadedState.runState setRunState(loadedState.runState) - setMessages(sanitizeRestoredMessages(loadedState.messages)) + const restoredMessages = sanitizeRestoredMessages(loadedState.messages) + if (loadedState.runStateRestored) { + setMessages(restoredMessages) + } else { + // The agent's context was lost (torn run-state.json, nothing + // recoverable) while the transcript survived. Surface it: without + // this the model just answers as if the earlier turns never + // happened, which reads as the assistant being broken. + setMessages([ + createErrorChatMessage( + 'The saved agent context could not be restored, so the assistant starts this chat without memory of earlier turns. The transcript below is intact.', + ), + ...restoredMessages, + ]) + } if (loadedState.chatId) { setCurrentChatId(loadedState.chatId) } diff --git a/cli/src/utils/__tests__/run-state-storage.test.ts b/cli/src/utils/__tests__/run-state-storage.test.ts index 5fa887cf41..1f65071794 100644 --- a/cli/src/utils/__tests__/run-state-storage.test.ts +++ b/cli/src/utils/__tests__/run-state-storage.test.ts @@ -917,3 +917,146 @@ describe('poisoned payload persistence', () => { expect(block.outputRaw.self).toBe('[Circular]') }) }) + +describe('run state recovery', () => { + // Point persistence at a temp dir via the explicit test override. + const chatDir = path.join(TEST_ROOT, 'codebuff-test-recovery') + + const runStateWithSession = (marker: string): RunState => + ({ + sessionState: { + mainAgentState: { + messageHistory: [{ role: 'user', content: marker }], + }, + }, + output: { type: 'lastMessage', value: marker }, + traceSessionId: 'trace-1', + }) as unknown as RunState + + const runStatePath = path.join(chatDir, 'run-state.json') + const bakPath = runStatePath + '.bak' + const messagesPath = path.join(chatDir, 'chat-messages.json') + + const writePrimary = (contents: string) => + fs.writeFileSync(runStatePath, contents) + const validMessages = JSON.stringify([ + { + id: 'msg-1', + variant: 'user', + content: 'the prompt', + timestamp: new Date().toISOString(), + }, + ] as ChatMessage[]) + + beforeEach(() => { + fs.rmSync(chatDir, { recursive: true, force: true }) + fs.mkdirSync(chatDir, { recursive: true }) + setChatDirOverrideForTesting(chatDir) + }) + + afterEach(() => { + setChatDirOverrideForTesting(undefined) + }) + + test('recovers agent context from the .bak when the primary is torn', () => { + writePrimary('{"sessionState": {"main"') // torn: power loss after rename + fs.writeFileSync(bakPath, JSON.stringify(runStateWithSession('from-bak'))) + fs.writeFileSync(messagesPath, validMessages) + + const loaded = loadMostRecentChatState() + expect(loaded).not.toBeNull() + // Agent context survived — the model is NOT amnesiac next turn. + expect((loaded!.runState as any).sessionState).toBeDefined() + expect(loaded!.runStateRestored).toBe(true) + // Self-healed: the primary is the recovered generation again. + expect( + JSON.parse(fs.readFileSync(runStatePath, 'utf8')).output.value, + ).toBe('from-bak') + }) + + test('recovers from the newest complete checkpoint temp when bak is absent', () => { + writePrimary('{ torn') + fs.writeFileSync(messagesPath, validMessages) + // Two temps: an older torn one and a newer complete one (SIGKILL between + // write and rename leaves the latter behind). + fs.writeFileSync( + runStatePath + '.999.oldest.tmp', + '{"half":', + ) + fs.writeFileSync( + runStatePath + '.1234.newest.tmp', + JSON.stringify(runStateWithSession('from-tmp')), + ) + + const loaded = loadMostRecentChatState() + expect(loaded).not.toBeNull() + expect((loaded!.runState as any).sessionState).toBeDefined() + expect(loaded!.runStateRestored).toBe(true) + // Self-healed into the primary. + expect( + JSON.parse(fs.readFileSync(runStatePath, 'utf8')).output.value, + ).toBe('from-tmp') + }) + + test('flags a healthy primary as fully restored', () => { + writePrimary(JSON.stringify(runStateWithSession('healthy'))) + fs.writeFileSync(messagesPath, validMessages) + + const loaded = loadMostRecentChatState() + expect(loaded!.runStateRestored).toBe(true) + expect((loaded!.runState as any).sessionState).toBeDefined() + }) + + test('falls back to a context-less placeholder with the loss flagged when nothing recovers', () => { + writePrimary('{ torn') + fs.writeFileSync(messagesPath, validMessages) + + const loaded = loadMostRecentChatState() + expect(loaded).not.toBeNull() + // The amnesia carrier: no sessionState — the SDK will start a fresh + // session next turn. runStateRestored=false is what makes the UI say so + // instead of the model silently forgetting every earlier turn. + expect((loaded!.runState as any).sessionState).toBeUndefined() + expect(loaded!.runStateRestored).toBe(false) + // The transcript still survives. + expect(loaded!.messages.length).toBe(1) + }) + + test('saveChatState rotates the previous primary into .bak', () => { + saveChatState(runStateWithSession('generation-1'), [ + { + id: 'msg-1', + variant: 'user', + content: 'first', + timestamp: new Date().toISOString(), + }, + ]) + expect(fs.existsSync(bakPath)).toBe(false) + + saveChatState(runStateWithSession('generation-2'), [ + { + id: 'msg-2', + variant: 'user', + content: 'second', + timestamp: new Date().toISOString(), + }, + ]) + + expect(JSON.parse(fs.readFileSync(runStatePath, 'utf8')).output.value).toBe( + 'generation-2', + ) + expect(JSON.parse(fs.readFileSync(bakPath, 'utf8')).output.value).toBe( + 'generation-1', + ) + }) + + test('clearChatState removes the backup too', () => { + writePrimary(JSON.stringify(runStateWithSession('x'))) + fs.writeFileSync(bakPath, JSON.stringify(runStateWithSession('bak'))) + fs.writeFileSync(messagesPath, validMessages) + + clearChatState() + expect(fs.existsSync(runStatePath)).toBe(false) + expect(fs.existsSync(bakPath)).toBe(false) + }) +}) diff --git a/cli/src/utils/run-state-storage.ts b/cli/src/utils/run-state-storage.ts index 698b503fbd..583bdcd78a 100644 --- a/cli/src/utils/run-state-storage.ts +++ b/cli/src/utils/run-state-storage.ts @@ -25,6 +25,11 @@ type SavedChatState = { runState: RunState messages: ChatMessage[] chatId?: string + /** False only when run-state.json was unreadable AND nothing could be + * recovered from the backup or checkpoint temps, so the restored RunState + * is a placeholder without agent context — the model starts amnesiac next + * turn. The UI surfaces this so the loss is not silent. */ + runStateRestored: boolean } type LiveChatState = { @@ -244,6 +249,121 @@ type SerializedChatState = { messagesJson?: string } +/** The previous generation of run-state.json, rotated aside by saveChatState. + * loadMostRecentChatState recovers from it when the primary is torn. */ +const RUN_STATE_BACKUP_SUFFIX = '.bak' + +function tryReadRunState(filePath: string): RunState | undefined { + try { + return JSON.parse(fs.readFileSync(filePath, 'utf8')) as RunState + } catch { + return undefined + } +} + +/** + * Read run-state.json with best-effort recovery when it is torn. + * + * The atomic write makes the rename indivisible, but without an fsync a power + * loss can still land the rename while the file's data blocks were never + * written — leaving a truncated or empty primary (and even with the fsync, + * external corruption exists). Two older complete generations can be lying in + * the chat directory when that happens: + * + * - `run-state.json.bak` — the state the primary replaced at the last + * synchronous save (saveChatState rotates the previous file aside). + * - `run-state.json...tmp` — a write that was killed in the + * window between its write and its rename (writeFileAtomic unlinks its own + * temp on error, but a SIGKILL cannot). + * + * Try the primary, then the backup, then the newest readable temp, and + * self-heal the primary from whichever recovered. `fromPrimary` is false when + * recovery had to fall back, so the caller can warn that some agent context + * is missing instead of losing it silently. + */ +function readRunStateWithRecovery(chatDir: string): { + runState: RunState + fromPrimary: boolean +} | null { + const runStatePath = path.join(chatDir, RUN_STATE_FILENAME) + let primaryError: unknown + try { + return { + runState: JSON.parse(fs.readFileSync(runStatePath, 'utf8')) as RunState, + fromPrimary: true, + } + } catch (error) { + primaryError = error + } + + const bakPath = runStatePath + RUN_STATE_BACKUP_SUFFIX + const fromBackup = tryReadRunState(bakPath) + if (fromBackup !== undefined) { + bestEffortLog( + 'warn', + { runStatePath, bakPath }, + 'run-state.json was unreadable; restored agent context from the previous save', + ) + // Self-heal: make the recovered generation the primary again. + try { + writeFileAtomic( + runStatePath, + JSON.stringify(fromBackup), + ) + } catch { + // Best-effort; the backup file still exists for the next load. + } + return { runState: fromBackup, fromPrimary: false } + } + + // Newest temp first — each is a complete write that never got renamed. + const tmpPaths = fs + .readdirSync(chatDir) + .filter( + (file) => + file.startsWith(RUN_STATE_FILENAME + '.') && file.endsWith('.tmp'), + ) + .map((file) => path.join(chatDir, file)) + .sort((a, b) => { + try { + return fs.statSync(b).mtimeMs - fs.statSync(a).mtimeMs + } catch { + return 0 + } + }) + for (const tmpPath of tmpPaths) { + const recovered = tryReadRunState(tmpPath) + if (recovered !== undefined) { + bestEffortLog( + 'warn', + { runStatePath, tmpPath }, + 'run-state.json was unreadable; restored agent context from an unfinished checkpoint write', + ) + try { + writeFileAtomic(runStatePath, JSON.stringify(recovered)) + } catch { + // Best-effort; the temp file still exists for the next load. + } + return { runState: recovered, fromPrimary: false } + } + } + + bestEffortLog( + 'warn', + { + runStatePath, + primaryError: + primaryError instanceof Error + ? primaryError.message + : String(primaryError), + backupTried: bakPath, + tempsTried: tmpPaths.length, + }, + 'Could not read run state; restoring transcript without agent context', + ) + return null +} + /** * Serialize the two chat-state files independently, so a poisoned run state * cannot block persisting the transcript (and vice versa). Cyclic and @@ -341,10 +461,18 @@ export function saveChatState( // since (e.g. the chat deleted from /history mid-run). fs.mkdirSync(chatDir, { recursive: true }) if (serialized.runStateJson) { - writeFileAtomic( - path.join(chatDir, RUN_STATE_FILENAME), - serialized.runStateJson, - ) + const runStatePath = path.join(chatDir, RUN_STATE_FILENAME) + // Rotate the previous generation aside before overwriting: it is the + // newest complete state readRunStateWithRecovery can fall back to if + // this write's rename lands but its data is later found torn. + try { + if (fs.existsSync(runStatePath)) { + fs.renameSync(runStatePath, runStatePath + RUN_STATE_BACKUP_SUFFIX) + } + } catch { + // Rotation is best-effort; the overwrite below still proceeds. + } + writeFileAtomic(runStatePath, serialized.runStateJson) } if (serialized.messagesJson) { writeFileAtomic( @@ -507,14 +635,16 @@ export function loadMostRecentChatState( // must not lose the transcript, and vice versa. Restore whatever is // readable and fall back for the rest. let runState: RunState | null = null - try { - runState = JSON.parse(fs.readFileSync(runStatePath, 'utf8')) as RunState - } catch (error) { + let runStateRestored = false + const recovered = readRunStateWithRecovery(chatDir) + if (recovered) { + // Agent context is present — whether straight from the primary or + // recovered from a backup/temp (both carry a real sessionState). + runState = recovered.runState + runStateRestored = true + } else { logger.warn( - { - runStatePath, - error: error instanceof Error ? error.message : String(error), - }, + { runStatePath }, 'Could not read run state; restoring transcript without agent context', ) } @@ -563,7 +693,12 @@ export function loadMostRecentChatState( 'Loaded chat state from chat directory', ) - return { runState, messages, chatId: resolvedChatId } + return { + runState, + messages, + chatId: resolvedChatId, + runStateRestored, + } } catch (error) { logger.error( { @@ -583,13 +718,14 @@ export function clearChatState(): void { const runStatePath = getRunStatePath() const messagesPath = getChatMessagesPath() const metaPath = path.join(resolveCurrentChatDir(), CHAT_META_FILENAME) + const backupPath = runStatePath + RUN_STATE_BACKUP_SUFFIX - for (const filePath of [runStatePath, messagesPath, metaPath]) { + for (const filePath of [runStatePath, backupPath, messagesPath, metaPath]) { fs.rmSync(filePath, { force: true }) } logger.debug( - { runStatePath, messagesPath, metaPath }, + { runStatePath, backupPath, messagesPath, metaPath }, 'Cleared chat state files', ) } catch (error) { diff --git a/cli/src/utils/write-file-atomic.ts b/cli/src/utils/write-file-atomic.ts index 85b922e248..b10cd1825e 100644 --- a/cli/src/utils/write-file-atomic.ts +++ b/cli/src/utils/write-file-atomic.ts @@ -10,15 +10,41 @@ function tempPathFor(filePath: string): string { } /** - * Write a file atomically: write to a temp file in the same directory, then - * rename over the target. Chat files grow to multiple MB and are rewritten on - * every agent step, so a plain writeFileSync interrupted by a crash/kill - * leaves truncated JSON that hides the chat from /history. + * Flush a file's data to disk before its name goes live. Without this, the + * rename is durable but the data blocks behind it are not: after a power cut + * or hard hang the rename can survive while the file's contents were never + * written, leaving a truncated/garbage file exactly where the atomic rename + * was supposed to guarantee a complete one. Cheap on tmpfs-sized writes and + * called at most a few times per second per chat, so correctness wins. + */ +function fsyncFile(fd: number): void { + try { + fs.fsyncSync(fd) + } catch { + // EINVAL on some filesystems that do not support fsync; nothing useful to + // do — the rename below is still atomic against concurrent processes. + } +} + +/** + * Write a file atomically AND durably: write to a temp file in the same + * directory, fsync it, then rename over the target. Chat files grow to + * multiple MB and are rewritten on every agent step, so a plain + * writeFileSync interrupted by a crash/kill leaves truncated JSON that hides + * the chat from /history — and without the fsync, even this rename pattern + * leaves a truncated file after a power loss (the rename survives, the data + * does not; that torn file is what made resumed chats amnesiac). */ export function writeFileAtomic(filePath: string, data: string): void { const tmpPath = tempPathFor(filePath) try { - fs.writeFileSync(tmpPath, data) + const fd = fs.openSync(tmpPath, 'w') + try { + fs.writeFileSync(fd, data) + fsyncFile(fd) + } finally { + fs.closeSync(fd) + } fs.renameSync(tmpPath, filePath) } catch (error) { try { @@ -31,19 +57,55 @@ export function writeFileAtomic(filePath: string, data: string): void { } /** - * Async counterpart to writeFileAtomic. Used by the in-flight checkpoint writer - * so serializing + flushing a multi-MB transcript doesn't block the CLI's - * render/input thread. Same tmp-then-rename atomicity guarantee. + * Rename with a short bounded retry. On Windows the handle closed moments + * earlier can take a beat to be released by the OS (antivirus/indexer hold a + * scan lock), and the rename fails with EPERM/EBUSY/EACCES until it is — a + * transient the sync path also experiences but rarely sees in tests. + */ +async function renameWithRetry(from: string, to: string): Promise { + for (let attempt = 0; ; attempt++) { + try { + await fs.promises.rename(from, to) + return + } catch (error) { + const code = (error as { code?: string }).code ?? '' + if (attempt >= 4 || !/^(EPERM|EBUSY|EACCES)$/.test(code)) { + throw error + } + await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt)) + } + } +} + +/** + * Async counterpart to writeFileAtomic. Used by the in-flight checkpoint + * writer so serializing + flushing a multi-MB transcript doesn't block the + * CLI's render/input thread. Same tmp-fsync-rename guarantee. */ export async function writeFileAtomicAsync( filePath: string, data: string, ): Promise { const tmpPath = tempPathFor(filePath) + let fileHandle: fs.promises.FileHandle | undefined try { - await fs.promises.writeFile(tmpPath, data) - await fs.promises.rename(tmpPath, filePath) + fileHandle = await fs.promises.open(tmpPath, 'w') + await fileHandle.writeFile(data) + try { + await fileHandle.sync() + } catch { + // See the sync path. + } + await fileHandle.close() + fileHandle = undefined + await renameWithRetry(tmpPath, filePath) } catch (error) { + // closeSync equivalents: FileHandle.close is idempotent-safe to attempt. + try { + await fileHandle?.close() + } catch { + // Ignore; the original error is what matters. + } try { await fs.promises.unlink(tmpPath) } catch { From 914e0f321f0cc8f97a1a2298109de92b37ac1024 Mon Sep 17 00:00:00 2001 From: nordicnode Date: Tue, 1 Sep 2026 12:57:59 -0700 Subject: [PATCH 2/2] Add fsync-durability and Windows rename-retry coverage to atomic writes Companion regression tests for the fsync-before-rename durability fix: the fsync must precede the rename (power loss between them leaves the old file intact), and the async rename retries EPERM/EBUSY/EACCES with bounded backoff while rethrowing anything else on the first attempt. Recovery loader, backup rotation, and UI notice split out to a follow-up PR per review. --- cli/src/hooks/use-send-message.ts | 16 +- .../utils/__tests__/run-state-storage.test.ts | 143 --------------- .../utils/__tests__/write-file-atomic.test.ts | 73 +++++++- cli/src/utils/run-state-storage.ts | 164 ++---------------- test/setup-scm-loader.ts | 1 + 5 files changed, 88 insertions(+), 309 deletions(-) create mode 100644 test/setup-scm-loader.ts diff --git a/cli/src/hooks/use-send-message.ts b/cli/src/hooks/use-send-message.ts index de9d5ba263..698859a44e 100644 --- a/cli/src/hooks/use-send-message.ts +++ b/cli/src/hooks/use-send-message.ts @@ -187,21 +187,7 @@ export const useSendMessage = ({ if (loadedState) { previousRunStateRef.current = loadedState.runState setRunState(loadedState.runState) - const restoredMessages = sanitizeRestoredMessages(loadedState.messages) - if (loadedState.runStateRestored) { - setMessages(restoredMessages) - } else { - // The agent's context was lost (torn run-state.json, nothing - // recoverable) while the transcript survived. Surface it: without - // this the model just answers as if the earlier turns never - // happened, which reads as the assistant being broken. - setMessages([ - createErrorChatMessage( - 'The saved agent context could not be restored, so the assistant starts this chat without memory of earlier turns. The transcript below is intact.', - ), - ...restoredMessages, - ]) - } + setMessages(sanitizeRestoredMessages(loadedState.messages)) if (loadedState.chatId) { setCurrentChatId(loadedState.chatId) } diff --git a/cli/src/utils/__tests__/run-state-storage.test.ts b/cli/src/utils/__tests__/run-state-storage.test.ts index 1f65071794..5fa887cf41 100644 --- a/cli/src/utils/__tests__/run-state-storage.test.ts +++ b/cli/src/utils/__tests__/run-state-storage.test.ts @@ -917,146 +917,3 @@ describe('poisoned payload persistence', () => { expect(block.outputRaw.self).toBe('[Circular]') }) }) - -describe('run state recovery', () => { - // Point persistence at a temp dir via the explicit test override. - const chatDir = path.join(TEST_ROOT, 'codebuff-test-recovery') - - const runStateWithSession = (marker: string): RunState => - ({ - sessionState: { - mainAgentState: { - messageHistory: [{ role: 'user', content: marker }], - }, - }, - output: { type: 'lastMessage', value: marker }, - traceSessionId: 'trace-1', - }) as unknown as RunState - - const runStatePath = path.join(chatDir, 'run-state.json') - const bakPath = runStatePath + '.bak' - const messagesPath = path.join(chatDir, 'chat-messages.json') - - const writePrimary = (contents: string) => - fs.writeFileSync(runStatePath, contents) - const validMessages = JSON.stringify([ - { - id: 'msg-1', - variant: 'user', - content: 'the prompt', - timestamp: new Date().toISOString(), - }, - ] as ChatMessage[]) - - beforeEach(() => { - fs.rmSync(chatDir, { recursive: true, force: true }) - fs.mkdirSync(chatDir, { recursive: true }) - setChatDirOverrideForTesting(chatDir) - }) - - afterEach(() => { - setChatDirOverrideForTesting(undefined) - }) - - test('recovers agent context from the .bak when the primary is torn', () => { - writePrimary('{"sessionState": {"main"') // torn: power loss after rename - fs.writeFileSync(bakPath, JSON.stringify(runStateWithSession('from-bak'))) - fs.writeFileSync(messagesPath, validMessages) - - const loaded = loadMostRecentChatState() - expect(loaded).not.toBeNull() - // Agent context survived — the model is NOT amnesiac next turn. - expect((loaded!.runState as any).sessionState).toBeDefined() - expect(loaded!.runStateRestored).toBe(true) - // Self-healed: the primary is the recovered generation again. - expect( - JSON.parse(fs.readFileSync(runStatePath, 'utf8')).output.value, - ).toBe('from-bak') - }) - - test('recovers from the newest complete checkpoint temp when bak is absent', () => { - writePrimary('{ torn') - fs.writeFileSync(messagesPath, validMessages) - // Two temps: an older torn one and a newer complete one (SIGKILL between - // write and rename leaves the latter behind). - fs.writeFileSync( - runStatePath + '.999.oldest.tmp', - '{"half":', - ) - fs.writeFileSync( - runStatePath + '.1234.newest.tmp', - JSON.stringify(runStateWithSession('from-tmp')), - ) - - const loaded = loadMostRecentChatState() - expect(loaded).not.toBeNull() - expect((loaded!.runState as any).sessionState).toBeDefined() - expect(loaded!.runStateRestored).toBe(true) - // Self-healed into the primary. - expect( - JSON.parse(fs.readFileSync(runStatePath, 'utf8')).output.value, - ).toBe('from-tmp') - }) - - test('flags a healthy primary as fully restored', () => { - writePrimary(JSON.stringify(runStateWithSession('healthy'))) - fs.writeFileSync(messagesPath, validMessages) - - const loaded = loadMostRecentChatState() - expect(loaded!.runStateRestored).toBe(true) - expect((loaded!.runState as any).sessionState).toBeDefined() - }) - - test('falls back to a context-less placeholder with the loss flagged when nothing recovers', () => { - writePrimary('{ torn') - fs.writeFileSync(messagesPath, validMessages) - - const loaded = loadMostRecentChatState() - expect(loaded).not.toBeNull() - // The amnesia carrier: no sessionState — the SDK will start a fresh - // session next turn. runStateRestored=false is what makes the UI say so - // instead of the model silently forgetting every earlier turn. - expect((loaded!.runState as any).sessionState).toBeUndefined() - expect(loaded!.runStateRestored).toBe(false) - // The transcript still survives. - expect(loaded!.messages.length).toBe(1) - }) - - test('saveChatState rotates the previous primary into .bak', () => { - saveChatState(runStateWithSession('generation-1'), [ - { - id: 'msg-1', - variant: 'user', - content: 'first', - timestamp: new Date().toISOString(), - }, - ]) - expect(fs.existsSync(bakPath)).toBe(false) - - saveChatState(runStateWithSession('generation-2'), [ - { - id: 'msg-2', - variant: 'user', - content: 'second', - timestamp: new Date().toISOString(), - }, - ]) - - expect(JSON.parse(fs.readFileSync(runStatePath, 'utf8')).output.value).toBe( - 'generation-2', - ) - expect(JSON.parse(fs.readFileSync(bakPath, 'utf8')).output.value).toBe( - 'generation-1', - ) - }) - - test('clearChatState removes the backup too', () => { - writePrimary(JSON.stringify(runStateWithSession('x'))) - fs.writeFileSync(bakPath, JSON.stringify(runStateWithSession('bak'))) - fs.writeFileSync(messagesPath, validMessages) - - clearChatState() - expect(fs.existsSync(runStatePath)).toBe(false) - expect(fs.existsSync(bakPath)).toBe(false) - }) -}) diff --git a/cli/src/utils/__tests__/write-file-atomic.test.ts b/cli/src/utils/__tests__/write-file-atomic.test.ts index 0da7b44e97..bb7711e12d 100644 --- a/cli/src/utils/__tests__/write-file-atomic.test.ts +++ b/cli/src/utils/__tests__/write-file-atomic.test.ts @@ -1,4 +1,4 @@ -import { describe, test, expect, beforeEach, afterEach } from 'bun:test' +import { describe, test, expect, beforeEach, afterEach, spyOn } from 'bun:test' import * as fs from 'fs' import * as os from 'os' import * as path from 'path' @@ -124,4 +124,75 @@ describe('writeFileAtomicAsync', () => { ) expect(fs.readdirSync(tempDir)).toEqual(['out.json']) }) + + test('retries a transient Windows rename lock and succeeds', async () => { + const target = path.join(tempDir, 'out.json') + let attempts = 0 + const realRename = fs.promises.rename.bind(fs.promises) + const spy = spyOn(fs.promises, 'rename').mockImplementation( + async (from, to) => { + attempts++ + if (attempts <= 2) { + throw Object.assign(new Error('locked'), { code: 'EPERM' }) + } + return realRename(from, to) + }, + ) + try { + await writeFileAtomicAsync(target, 'recovered') + } finally { + spy.mockRestore() + } + + expect(attempts).toBe(3) + expect(fs.readFileSync(target, 'utf8')).toBe('recovered') + }) + + test('rethrows a non-transient rename error immediately', async () => { + const target = path.join(tempDir, 'out.json') + let attempts = 0 + const spy = spyOn(fs.promises, 'rename').mockImplementation(async () => { + attempts++ + throw Object.assign(new Error('missing'), { code: 'ENOENT' }) + }) + try { + await expect(writeFileAtomicAsync(target, 'data')).rejects.toThrow() + } finally { + spy.mockRestore() + } + + expect(attempts).toBe(1) + }) +}) + +describe('writeFileAtomic durability ordering', () => { + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codebuff-atomic-')) + }) + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }) + }) + + test('fsyncs the temp before the rename goes live', () => { + const target = path.join(tempDir, 'out.json') + const order: string[] = [] + const fsyncSpy = spyOn(fs, 'fsyncSync').mockImplementation(() => { + order.push('fsync') + }) + const realRename = fs.renameSync.bind(fs) + const renameSpy = spyOn(fs, 'renameSync').mockImplementation((from, to) => { + order.push('rename') + return realRename(from, to) + }) + try { + writeFileAtomic(target, '{"a":1}') + } finally { + fsyncSpy.mockRestore() + renameSpy.mockRestore() + } + + expect(order).toEqual(['fsync', 'rename']) + expect(fs.readFileSync(target, 'utf8')).toBe('{"a":1}') + }) }) diff --git a/cli/src/utils/run-state-storage.ts b/cli/src/utils/run-state-storage.ts index 583bdcd78a..698b503fbd 100644 --- a/cli/src/utils/run-state-storage.ts +++ b/cli/src/utils/run-state-storage.ts @@ -25,11 +25,6 @@ type SavedChatState = { runState: RunState messages: ChatMessage[] chatId?: string - /** False only when run-state.json was unreadable AND nothing could be - * recovered from the backup or checkpoint temps, so the restored RunState - * is a placeholder without agent context — the model starts amnesiac next - * turn. The UI surfaces this so the loss is not silent. */ - runStateRestored: boolean } type LiveChatState = { @@ -249,121 +244,6 @@ type SerializedChatState = { messagesJson?: string } -/** The previous generation of run-state.json, rotated aside by saveChatState. - * loadMostRecentChatState recovers from it when the primary is torn. */ -const RUN_STATE_BACKUP_SUFFIX = '.bak' - -function tryReadRunState(filePath: string): RunState | undefined { - try { - return JSON.parse(fs.readFileSync(filePath, 'utf8')) as RunState - } catch { - return undefined - } -} - -/** - * Read run-state.json with best-effort recovery when it is torn. - * - * The atomic write makes the rename indivisible, but without an fsync a power - * loss can still land the rename while the file's data blocks were never - * written — leaving a truncated or empty primary (and even with the fsync, - * external corruption exists). Two older complete generations can be lying in - * the chat directory when that happens: - * - * - `run-state.json.bak` — the state the primary replaced at the last - * synchronous save (saveChatState rotates the previous file aside). - * - `run-state.json...tmp` — a write that was killed in the - * window between its write and its rename (writeFileAtomic unlinks its own - * temp on error, but a SIGKILL cannot). - * - * Try the primary, then the backup, then the newest readable temp, and - * self-heal the primary from whichever recovered. `fromPrimary` is false when - * recovery had to fall back, so the caller can warn that some agent context - * is missing instead of losing it silently. - */ -function readRunStateWithRecovery(chatDir: string): { - runState: RunState - fromPrimary: boolean -} | null { - const runStatePath = path.join(chatDir, RUN_STATE_FILENAME) - let primaryError: unknown - try { - return { - runState: JSON.parse(fs.readFileSync(runStatePath, 'utf8')) as RunState, - fromPrimary: true, - } - } catch (error) { - primaryError = error - } - - const bakPath = runStatePath + RUN_STATE_BACKUP_SUFFIX - const fromBackup = tryReadRunState(bakPath) - if (fromBackup !== undefined) { - bestEffortLog( - 'warn', - { runStatePath, bakPath }, - 'run-state.json was unreadable; restored agent context from the previous save', - ) - // Self-heal: make the recovered generation the primary again. - try { - writeFileAtomic( - runStatePath, - JSON.stringify(fromBackup), - ) - } catch { - // Best-effort; the backup file still exists for the next load. - } - return { runState: fromBackup, fromPrimary: false } - } - - // Newest temp first — each is a complete write that never got renamed. - const tmpPaths = fs - .readdirSync(chatDir) - .filter( - (file) => - file.startsWith(RUN_STATE_FILENAME + '.') && file.endsWith('.tmp'), - ) - .map((file) => path.join(chatDir, file)) - .sort((a, b) => { - try { - return fs.statSync(b).mtimeMs - fs.statSync(a).mtimeMs - } catch { - return 0 - } - }) - for (const tmpPath of tmpPaths) { - const recovered = tryReadRunState(tmpPath) - if (recovered !== undefined) { - bestEffortLog( - 'warn', - { runStatePath, tmpPath }, - 'run-state.json was unreadable; restored agent context from an unfinished checkpoint write', - ) - try { - writeFileAtomic(runStatePath, JSON.stringify(recovered)) - } catch { - // Best-effort; the temp file still exists for the next load. - } - return { runState: recovered, fromPrimary: false } - } - } - - bestEffortLog( - 'warn', - { - runStatePath, - primaryError: - primaryError instanceof Error - ? primaryError.message - : String(primaryError), - backupTried: bakPath, - tempsTried: tmpPaths.length, - }, - 'Could not read run state; restoring transcript without agent context', - ) - return null -} - /** * Serialize the two chat-state files independently, so a poisoned run state * cannot block persisting the transcript (and vice versa). Cyclic and @@ -461,18 +341,10 @@ export function saveChatState( // since (e.g. the chat deleted from /history mid-run). fs.mkdirSync(chatDir, { recursive: true }) if (serialized.runStateJson) { - const runStatePath = path.join(chatDir, RUN_STATE_FILENAME) - // Rotate the previous generation aside before overwriting: it is the - // newest complete state readRunStateWithRecovery can fall back to if - // this write's rename lands but its data is later found torn. - try { - if (fs.existsSync(runStatePath)) { - fs.renameSync(runStatePath, runStatePath + RUN_STATE_BACKUP_SUFFIX) - } - } catch { - // Rotation is best-effort; the overwrite below still proceeds. - } - writeFileAtomic(runStatePath, serialized.runStateJson) + writeFileAtomic( + path.join(chatDir, RUN_STATE_FILENAME), + serialized.runStateJson, + ) } if (serialized.messagesJson) { writeFileAtomic( @@ -635,16 +507,14 @@ export function loadMostRecentChatState( // must not lose the transcript, and vice versa. Restore whatever is // readable and fall back for the rest. let runState: RunState | null = null - let runStateRestored = false - const recovered = readRunStateWithRecovery(chatDir) - if (recovered) { - // Agent context is present — whether straight from the primary or - // recovered from a backup/temp (both carry a real sessionState). - runState = recovered.runState - runStateRestored = true - } else { + try { + runState = JSON.parse(fs.readFileSync(runStatePath, 'utf8')) as RunState + } catch (error) { logger.warn( - { runStatePath }, + { + runStatePath, + error: error instanceof Error ? error.message : String(error), + }, 'Could not read run state; restoring transcript without agent context', ) } @@ -693,12 +563,7 @@ export function loadMostRecentChatState( 'Loaded chat state from chat directory', ) - return { - runState, - messages, - chatId: resolvedChatId, - runStateRestored, - } + return { runState, messages, chatId: resolvedChatId } } catch (error) { logger.error( { @@ -718,14 +583,13 @@ export function clearChatState(): void { const runStatePath = getRunStatePath() const messagesPath = getChatMessagesPath() const metaPath = path.join(resolveCurrentChatDir(), CHAT_META_FILENAME) - const backupPath = runStatePath + RUN_STATE_BACKUP_SUFFIX - for (const filePath of [runStatePath, backupPath, messagesPath, metaPath]) { + for (const filePath of [runStatePath, messagesPath, metaPath]) { fs.rmSync(filePath, { force: true }) } logger.debug( - { runStatePath, backupPath, messagesPath, metaPath }, + { runStatePath, messagesPath, metaPath }, 'Cleared chat state files', ) } catch (error) { diff --git a/test/setup-scm-loader.ts b/test/setup-scm-loader.ts new file mode 100644 index 0000000000..336ce12bb9 --- /dev/null +++ b/test/setup-scm-loader.ts @@ -0,0 +1 @@ +export {}