Repository navigation
LOU-K5: restore checkpointing for the remainder of a run after resumeAfterApproval() - #13
Merged
Merged
Conversation
resumeAfterApproval() correctly deleted the stale pre-pause checkpoint under snapshot.sessionId, but then forced sessionId/checkpointStore to undefined on the follow-up AgentExecutor.execute() call as a belt-and-suspenders guard against re-rehydrating that stale state. The guard was overbroad: it also silently disabled checkpointing for the entire remainder of the resumed run, so a crash a few tool-calls after a human approval would lose all progress made since the approval. Now the real sessionId/checkpointStore are threaded through instead. This stays safe because AgentExecutor's rehydration branch only fires when checkpointStore.load(sessionId) finds something, and the delete above guarantees it won't - so the resumed run always falls into the normal "build from scratch" path using the reconstructed messages/initialSteps, and AgentExecutor's per-tool-result checkpoint.save() calls resume writing durable progress from there on. Adds tests proving: a new checkpoint is written for a tool-call step taken after a successful resume, the stale pre-pause checkpoint is still never rehydrated, and resuming without a checkpointStore remains fully backward compatible. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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
resumeAfterApproval()(src/execution/resume.ts) correctly deletes the stale pre-pause checkpoint stored undersnapshot.sessionIdbefore continuing the run - that checkpoint predates the pause (checkpoints are only saved after a tool result, and a pause happens before the deferred tool runs), so it would silently clobber the just-reconstructed messages/steps if it were ever loaded.But the follow-up
AgentExecutor.execute()call that actually continues the run then forcedsessionId: undefined, checkpointStore: undefinedas a "belt-and-suspenders" guard against re-triggering rehydration. That guard was overbroad: forcing both toundefineddoesn't just prevent rehydration for this one call, it also disablesAgentExecutor's normal per-tool-resultcheckpointStore.save()calls for the entire rest of the resumed run. If the process crashed three tool-calls after a human approved a paused tool call, all progress since the approval was lost, because durability had been silently turned off for the remainder of that run - not just for the resume step itself.Fix
resumeAfterApproval()now threads the realsessionId(snapshot.sessionId) andcheckpointStore(its own dedicated parameter) through to the follow-upAgentExecutor.execute()call instead of forcing them toundefined.ResumeExecuteOptionskeeps both fields omitted (unchanged) - they're derived internally, not accepted fromexecuteOptions, so a caller can't override them.Safety argument (why rehydration still can't fire)
AgentExecutor's rehydration branch inrunAgentLoop()is:It's keyed strictly on
checkpointStore.load(sessionId)returning non-null - there's no interaction withinitialStepsor any other option.resumeAfterApproval()deletes whatever is undersnapshot.sessionIdbefore making theexecute()call, and there's no interveningawaitback into caller code between that delete andexecute()'sload(). So by the time the rehydration check runs, there is nothing left to find, and execution falls into the normal "build from scratch" path using the reconstructedmessages/skipSystemPromptInjection/initialSteps- exactly as it did before this fix, just with checkpointing now live for the rest of the run.Accepted risk: a genuine race exists only if some other process/caller writes a new checkpoint under the exact same
sessionIdin the narrow window between the delete and the resumedexecute()call'sload(). This is documented in code rather than solved with cross-process locking, consistent with how this file already treatssessionIdas identifying a single logical run (e.g.ApprovalStore.resolve()is delete-on-read with no additional lock either).Changes
src/execution/resume.ts: passsessionId/checkpointStorethrough instead of forcingundefined; rewrote the doc comments explaining the old vs. new behavior and why it's still safe.src/execution/resume.test.ts: added tests proving (a) a new checkpoint is written for a tool-call step taken after a successful resume when acheckpointStoreis supplied, and (b) resuming without acheckpointStoreremains fully backward compatible. Existing stale-checkpoint-never-rehydrated coverage still passes unchanged.Test plan
npm run typecheck- cleannpm run lint- no new errors/warnings introduced (pre-existing unrelated errors/warnings in other files, confirmed via targetedeslintrun on only the touched files)npx vitest run src/execution/resume.test.ts src/execution/checkpoint.test.ts src/execution/AgentExecutor.test.ts- 44/44 passing🤖 Generated with Claude Code