Skip to content

Recover agent context from torn run-state.json backups - #1195

Open
nordicnode wants to merge 1 commit into
CodebuffAI:mainfrom
nordicnode:oss/runstate-recovery-loader
Open

Recover agent context from torn run-state.json backups#1195
nordicnode wants to merge 1 commit into
CodebuffAI:mainfrom
nordicnode:oss/runstate-recovery-loader

Conversation

@nordicnode

@nordicnode nordicnode commented Sep 1, 2026

Copy link
Copy Markdown

Summary

Recovery half of #1169, split out per review so the fsync durability fix (#1169, now fsync + retry tests only) can land on its own. A resumed chat could start amnesiac through a chain with no single point of failure (details and repro in #1168):

  • loadMostRecentChatState recovers instead of giving up. When run-state.json is unreadable it now tries every intact older generation in the chat directory: the rotated run-state.json.bak (each synchronous save rotates the current file aside before overwriting) and any complete checkpoint .tmp left behind when a process died between write and rename. Candidates are ordered newest-first — the generation that lost the least agent context wins, and recency beats a fixed .bak-first order when a newer checkpoint exists. mtime ties break on path comparison so coarse-mtime filesystems stay deterministic.
  • The loss is surfaced, not silent. loadMostRecentChatState returns runStateRestored, and the resume flow prepends an error-variant notice ("the assistant starts this chat without memory of earlier turns; the transcript below is intact") when it is false. Previously the transcript rendered normally and the model just "forgot" — indistinguishable from a broken assistant.
  • clearChatState also removes the .bak, so deleting a chat does not leave recoverable state behind.

Root cause

The atomic write makes the rename indivisible, but without an fsync a power loss can 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). loadMostRecentChatState treated any unreadable primary as fatal and handed back a placeholder RunState without sessionState; the SDK then started a fresh session next turn, which reads as the assistant having "forgotten" everything.

Changes

  1. cli/src/utils/run-state-storage.tsreadRunStateWithRecovery(chatDir): primary → newest-first candidates (.bak + checkpoint temps, mtime-ordered with path tie-break) → null; self-heals the primary from the winner (best-effort, try/catch); per-attempt and final-failure warnings via bestEffortLog.
  2. saveChatState rotates the previous primary into run-state.json.bak before overwriting (best-effort; the overwrite proceeds regardless). renameSync is a metadata-only directory operation — no data copy — so the per-step cost is one extra inode rename on an already multi-syscall save.
  3. cli/src/hooks/use-send-message.ts — resume path prepends the context-loss notice when runStateRestored is false.

Tests

  • 7 new tests in cli/src/utils/__tests__/run-state-storage.test.ts: .bak recovery + self-heal, newest-temp recovery (mtimes pinned with utimesSync so back-to-back writes cannot flake on mtime quantization), newer temp beats .bak (the recency rule; fails under a fixed .bak-first order), healthy primary flagged restored, all-candidates-unreadable fallback with the loss flagged, backup rotation on save, backup cleanup on delete.
  • NODE_ENV=production bun test src/utils/__tests__/run-state-storage.test.ts — 44 pass (37 pre-existing + 7), 0 fail.

Validation

  • NODE_ENV=production bun test src/utils/__tests__/run-state-storage.test.ts src/utils/__tests__/write-file-atomic.test.ts — 54 pass, 0 fail.
  • bun x tsc --noEmit -p cli/tsconfig.json — 10 errors, all pre-existing on main and all in unrelated test files (tar types, react-dom/server types); none in the three changed files.
  • bunx prettier --check clean on all three changed files.

Refs #1168, #1169

Bench (rotation cost, review's perf note)

renameSync on a 5 MiB run-state-sized file measured on this machine (bun 1.4.0, 500 iterations, filesystem: tmpfs — statfsSync.type 16914836 = TMPFS_MAGIC). The real chat dir lives on the user's home filesystem, so absolute numbers there differ, but the property being measured is not FS-dependent:

save path ms/op
write + fsync + rename (no rotation) 2.024
write + fsync + rename with .bak rotation 1.981

Rotation adds no measurable cost (delta −0.04 ms/op, within noise): it is one extra metadata-only directory-entry re-link, never a data copy — renaming a multi-MB file does not move its blocks. Worst case on a non-tmpfs disk is still one inode rename vs the ~5 MiB write+fsync the save already performs.

Tradeoff: async checkpoints do not rotate .bak

.bak reflects the last synchronous save (exit flush, step-boundary writes). The async checkpoint writer (saveChatStateAsync) skips rotation deliberately: an un-awaited renameSync in it would contend with the in-flight writeFileAtomicAsync rename for the same target, and deferring it to renameWithRetry is a follow-up. A crash between sync saves leaves the .bak up to one sync-save older than the newest complete checkpoint temp — which the recovery order in this PR already prefers. Recovery quality is unaffected; only the .bak's freshness lags.

When run-state.json is unreadable, loadMostRecentChatState now falls back to
the previous generation (.bak rotated aside by each synchronous save) and to
complete checkpoint temps left by SIGKILLed writes, ordered newest-first with
a deterministic tie-break, self-heals the primary from whichever recovered,
and clears the .bak on chat deletion. The async atomic write retries
EPERM/EBUSY/EACCES renames briefly for Windows AV/indexer locks. The resume
flow prepends an error-variant notice when nothing could be recovered, so the
context loss is surfaced instead of reading as a broken assistant.

Companion to CodebuffAI#1166; details and repro in CodebuffAI#1168. Recovery half of CodebuffAI#1169, split
per review so the fsync durability fix can land on its own.
@codebuff-team

Copy link
Copy Markdown
Contributor

Solid piece of work. The core idea in readRunStateWithRecovery (run-state-storage.ts) — falling back newest-first through .bak and orphaned .tmp checkpoints, self-healing the primary, and reporting whether recovery actually happened via runStateRestored — is a sensible way to close the "assistant silently forgets everything" failure mode described in #1168/#1169. The mtime-with-path-tiebreak ordering and the explicit "newer temp beats stale .bak" test are the right level of paranoia for a recovery path like this, and rotating the primary into .bak in saveChatState before overwrite is cheap (one rename) for the safety it buys.

A few things worth double-checking before this lands:

  1. SavedChatState now requires runStateRestored: boolean. If there are other code paths in run-state-storage.ts outside this diff that construct/return a SavedChatState (legacy format handling, older-generation fallbacks not shown here), they'll need this field too or the build breaks. Worth grepping for every return site.
  2. The self-heal write (writeFileAtomic(runStatePath, JSON.stringify(recovered))) re-serializes with plain JSON.stringify rather than whatever custom serializer saveChatState uses for poisoned payloads. Since the recovered value already round-tripped through JSON.parse, this should be safe, but it's a second serialization code path to keep in sync if the primary one changes.
  3. use-send-message.ts prepends the "context lost" notice whenever runStateRestored is false — including the case where run-state.json never existed at all (e.g. a chat resumed before its first save completed). That may be intentional per the PR's stated goal of surfacing previously-silent loss, but confirm that path doesn't fire on genuinely fresh/empty chats, which would be a confusing false alarm for users.

Good test coverage (7 new cases) and a clear root-cause writeup. This is exactly the kind of change worth a maintainer's time to review and port, modulo the compile-completeness check above.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants