fix(agent): cap recovery chain depth and force-save at the cap#6082
Merged
SivanCola merged 2 commits intoJul 6, 2026
Conversation
Field reports of frequent "session changed on disk" notices (esengine#6069) are undiagnosable today: the stale-prefix adoption and the recovery-not-needed adoption emit the same notice text, nothing logs which trigger fired, and the revision ledger that drove the conflict decision is discarded. - Log every recoverSnapshotConflict outcome (stale-prefix adopt, recovery-not-needed adopt, failed adoption, recovery fork) with the conflict kind, save mode, message counts, and the BaseRevision/DiskRevision pair from SessionSnapshotConflictError. - Differentiate the recovery-not-needed notice text ("local changes already covered") so user screenshots identify the trigger. - The recovery-not-needed path whose disk adoption fails used to drop the snapshot silently; it now leaves a warning so "my last turns vanished" reports can be tied to it.
A save-conflict loop used to fork recovery branches from recovery branches without bound — esengine#5993 reached 8 nested levels, each fork polluting the session list. The conflict triggers behind that loop are fixed, but nothing prevents a future regression from spawning chains again: BranchMeta.Recovered was display-only and SaveRecoveryBranch never looked at the parent's recovery state. - Stamp BranchMeta.RecoveryDepth on every recovery fork (parent depth + 1; legacy recovery metas count as depth 1). - SaveRecoveryBranch refuses to fork a parent already at SessionRecoveryMaxDepth (3) with ErrSessionRecoveryDepthExceeded. - At the cap the controller force-saves the transcript onto the current branch instead: this runtime is the only writer of recovery branches it created, so the force write keeps the data, re-anchors the persistence baseline (no immediate re-conflict), and stops the file proliferation. A warn log plus a distinct notice make the condition visible.
github-actions
Bot
added
agent
This was referenced Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Defense-in-depth for the #5993 class: a save-conflict loop used to fork recovery branches from recovery branches without bound — the report reached 8 nested levels, each fork polluting the session list. The conflict triggers behind that loop are fixed (1.17.x + #6063), but nothing prevents a future regression from spawning chains again:
BranchMeta.Recoveredwas display-only andSaveRecoveryBranchnever looked at the parent's recovery state.BranchMeta.RecoveryDepthon every recovery fork (parent depth + 1; legacy recovery metas without the field count as depth 1). Always derived from the parent chain, never trusted from the caller's meta, which copies tab/session meta wholesale.SaveRecoveryBranchrefuses to fork a parent already atSessionRecoveryMaxDepth(3) with a typedErrSessionRecoveryDepthExceeded.recoverSnapshotConflictforce-saves the transcript onto the current branch instead of forking: this runtime is the only writer of the recovery branches it created (recovery paths are digest-derived; identical snapshots dedupe toExisting), so the force write keeps the data, re-anchors the persistence baseline so the next snapshot does not re-conflict, and stops the file proliferation. A warn log (with the conflict's revision ledger) plus a distinct notice make the condition visible instead of silent.Semantic choice worth reviewing: at the cap the fallback prefers keeping the in-memory transcript (force write over the runtime's own recovery branch) over adopting whatever is on disk — recovery exists to prevent data loss, and the branch has no other legitimate writer.
Verification
TestSaveRecoveryBranchStampsAndCapsChainDepth: depth 1 stamped on first fork; nested fork increments to 2; legacyRecoveredmeta treated as depth 1; parent at cap refuses with the typed error and creates no file.TestSnapshotConflictAtRecoveryDepthCapForceSavesCurrentBranch: diverged conflict at a depth-capped path →Snapshot()succeeds, no*-recovery-*.jsonlcreated, disk holds the local transcript, depth-cap notice emitted, and the follow-up snapshot does not re-conflict.internal/agent,internal/control,internal/cli,internal/serve,internal/acp, desktop (55s).go vetclean.Cache impact
Cache-impact: none - session persistence/recovery only; no provider request, system prompt, or tool schema changes.
Cache-guard: not applicable (no watchlist files); recovery-path suites above cover the change.
System-prompt-review: N/A
Refs #5993, #6054.