fix(engine): session-end durability + raw KeyboardInterrupt handling#62
Conversation
Record the session without usage, save immediately, then fold usage in via StoryTask.attach_session_usage — a host kill during usage read, hooks, or follow-up verification can no longer lose a completed session, and usage stays best-effort metadata rather than a durability gate. Make the durable record actionable on resume: SessionRecord now carries the session's result_json, and _finish_inflight continues from a durably-completed dev/review session into the normal verify/decide pipeline (journaled as resume-verify) instead of rolling finished work back through resume-restart. Incomplete evidence — record missing, not completed, or without a result — falls through to today's restart path. Also add an explicit KeyboardInterrupt branch to the run loop (Windows console-ctrl events can surface as a raw KeyboardInterrupt without routing through the installed signal handler): tear down the agent session, persist a controlled stop, and re-raise when nested. The branch latches _stopping and guards kill_session so a second interrupt or a stop signal landing mid-teardown still records the stop.
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR persists completed adapter sessions to disk before running post-session hooks, adds a ChangesSession durability and resume
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Engine
participant SessionRecord
participant Disk
participant PostSessionHooks
Engine->>SessionRecord: record_session(result_json)
Engine->>Disk: _save()
Engine->>Engine: read_usage
Engine->>SessionRecord: attach_session_usage
Engine->>Disk: _save()
Engine->>PostSessionHooks: emit post_session
sequenceDiagram
participant Engine
participant ResumableSession
participant DevPhase
participant ReviewAndCommit
participant VerifyDecision
Engine->>ResumableSession: scan task.sessions for completed result_json
ResumableSession-->>Engine: role, SessionResult
Engine->>DevPhase: _dev_phase(resume_result)
Engine->>ReviewAndCommit: _review_and_commit(resume_result)
DevPhase->>VerifyDecision: continue verify/decide
ReviewAndCommit->>VerifyDecision: continue verify/decide
Related issues: Suggested labels: bug, engine, durability Suggested reviewers: none identified from summaries Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Augment PR SummarySummary: This PR improves run/session durability and resume behavior by ensuring completed sessions are persisted before best-effort metadata and hooks run. Changes:
Technical Notes: Usage is treated as best-effort metadata (not a durability gate); completed-session evidence is preserved to support reliable resume/continuation. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bmad_loop/engine.py (2)
1363-1387: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSkip baseline/attempt reset on resumed dev results.
_dev_phasere-capturesbaseline_commit/baseline_untrackedand bumpstask.attempteven whenresume_resultis the already-finished session being replayed through verify/decide. That shifts the squash/rollback baseline to the post-session tree and advances the sequence number before the recorded result is processed. Guard both updates behindresume_result is None.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bmad_loop/engine.py` around lines 1363 - 1387, In _dev_phase, the baseline and attempt counters are being reset even when replaying an already-finished resumed session via resume_result, which shifts the rollback/squash reference and advances the sequence unexpectedly. Update _dev_phase so task.baseline_commit, task.baseline_untracked, and the task.attempt increment only happen for fresh dev runs, and skip those updates when resume_result is provided before the verify/decide flow consumes it.
1503-1520: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
review_cyclewhen replaying a resumed review result.
resume_resultalready belongs to the current cycle, so incrementing first makes the replayed pass look like the next one. That mislabels the journal/session IDs and burns one review-budget slot early on every crash-resume.🐛 Proposed fix
while task.review_cycle < self.policy.limits.max_review_cycles: - task.review_cycle += 1 + if resume_result is None: + task.review_cycle += 1 refileable_followup = False # only a completed pass this cycle can set it advance(task, Phase.REVIEW_RUNNING) self._save()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bmad_loop/engine.py` around lines 1503 - 1520, In the review loop inside engine.py, the current logic increments task.review_cycle before checking resume_result, which makes a replayed resumed review appear as the next cycle and shifts session/journal IDs. Update the control flow so a resumed result is processed under the existing cycle value, and only increment review_cycle for a newly run review session. Use the surrounding while loop, resume_result handling, and _run_session call as the anchor points for the fix.
🧹 Nitpick comments (1)
tests/test_engine.py (1)
179-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
journal.entries()over manual JSONL parsing for consistency.Other tests use
resumed.journal.entries(); this test hand-rolls the same parsing viajson.loadsover raw lines, duplicatingJournal.entries()'s logic.♻️ Proposed simplification
- entries = [ - json.loads(line) for line in (engine.run_dir / "journal.jsonl").read_text().splitlines() - ] + entries = engine.journal.entries() stops = [e for e in entries if e["kind"] == "run-stop"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_engine.py` around lines 179 - 183, The test is manually parsing journal.jsonl instead of using the shared journal API, which duplicates Journal.entries() behavior. Update the KeyboardInterrupt assertion in the test that reads engine.run_dir to use engine.journal.entries() (or the equivalent journal object already available in the test) and keep the run-stop filtering/assertion the same. This keeps the test consistent with other cases and avoids repeating JSONL parsing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/bmad_loop/engine.py`:
- Around line 1363-1387: In _dev_phase, the baseline and attempt counters are
being reset even when replaying an already-finished resumed session via
resume_result, which shifts the rollback/squash reference and advances the
sequence unexpectedly. Update _dev_phase so task.baseline_commit,
task.baseline_untracked, and the task.attempt increment only happen for fresh
dev runs, and skip those updates when resume_result is provided before the
verify/decide flow consumes it.
- Around line 1503-1520: In the review loop inside engine.py, the current logic
increments task.review_cycle before checking resume_result, which makes a
replayed resumed review appear as the next cycle and shifts session/journal IDs.
Update the control flow so a resumed result is processed under the existing
cycle value, and only increment review_cycle for a newly run review session. Use
the surrounding while loop, resume_result handling, and _run_session call as the
anchor points for the fix.
---
Nitpick comments:
In `@tests/test_engine.py`:
- Around line 179-183: The test is manually parsing journal.jsonl instead of
using the shared journal API, which duplicates Journal.entries() behavior.
Update the KeyboardInterrupt assertion in the test that reads engine.run_dir to
use engine.journal.entries() (or the equivalent journal object already available
in the test) and keep the run-stop filtering/assertion the same. This keeps the
test consistent with other cases and avoids repeating JSONL parsing logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15adc9d9-787b-4833-8826-15b1af2fe222
📒 Files selected for processing (4)
src/bmad_loop/engine.pysrc/bmad_loop/model.pytests/test_engine.pytests/test_model.py
…esumed result A resumed session result replays the attempt/cycle it was recorded under: advancing the counters or re-capturing the baseline would desync the session task_id from the durable record (a second host death would then fall back to restart-rollback of finished work), shift the rollback/squash reference onto the completed session's own tree, and burn a review-budget slot per crash-resume. Guard all three behind the replay so only fresh sessions advance them.
|
CodeRabbit's two outside-diff Major findings (baseline/attempt reset on resumed dev results; review_cycle increment on replay) are both fixed in 69e2db9 — the counter advances and the baseline capture are guarded behind |
pbean
left a comment
pbean
left a comment
There was a problem hiding this comment.
Approving. The session-end durability barrier (record-before-hooks, usage folded in afterward) and the resume-continuation seam are well-built, and the follow-up commit's attempt/cycle/baseline preservation guard closes the obvious re-entrancy hazards a naive replay would open. _resumable_session correctly consumes only adapter-vouched completed evidence, and the single-consume discipline (resume_result nulled after use) holds in both _dev_phase and _review_and_commit.
Three edges I'll clean up maintainer-side in a fast follow-up PR — all NON-BLOCKING, and each only bites the new crash-in-the-post-session-window path this PR introduces, so none regress current behavior:
1. Final-review-cycle replay drops a recorded clean review to defer. _review_and_commit's loop is guarded by while task.review_cycle < self.policy.limits.max_review_cycles:. When a host death lands in the post-session window of the last allowed review cycle, on resume task.review_cycle is already == max_review_cycles, so the replayed continuation evaluates the guard as false and never enters the loop body — the durably-recorded clean review is dropped and the story falls through to defer. The replay branch already skips the review_cycle += 1 increment, so relaxing the guard to also enter when resume_result is not None burns no extra budget.
2. SessionRecord.result_json aliases the dict _reconcile_generic_terminal_status mutates in place. _run_session stores result_json=result.result_json (same object), saves it pre-reconcile, and only later does _dev_phase call reconcile, which mutates that dict in place. A crash between reconcile's spec write and the dev-decision _save() persists a pre-reconcile dict; on replay reconcile early-returns (fm_status == success_status) and never re-folds followup_review_recommended, so task.followup_review_recommended reads stale and the follow-up review can be silently skipped. Fix is a defensive copy at record time plus a heal on reconcile's early-return path (sync status + fold followup from the already-read frontmatter).
3. Triage/sweep and plugin-label sessions persist large, never-consumed result_json (state.json bloat). _run_session now records result_json for every session, but _resumable_session only ever matches dev/review task ids under DEV_RUNNING/REVIEW_RUNNING. role="triage" sweep sessions and labeled plugin-workflow sessions can never be consumed, so their payloads are pure state.json bloat. The same defensive-copy site gates these to None.
I'll open the follow-up right after merge, referencing this PR and #57. Thanks @dracic — merging.
Three follow-ups to the session-end durability / resume-continuation work in #62 (which closed #57). Each only bites the new crash-in-the-post- session-window replay path, so none regress steady-state behavior. (a) Final-review-cycle replay dropped a recorded clean review to defer. `_review_and_commit`'s loop was guarded by `review_cycle < max`, but a host death in the post-session window of the *last* allowed cycle resumes with `review_cycle == max`, so the replayed clean pass never entered the loop and the story fell through to defer. Relax the guard to also enter while a `resume_result` is pending; the replay branch already skips the re-increment, so no extra budget is burned. (b) `SessionRecord.result_json` aliased the dict `_reconcile_generic_terminal_status` mutates in place. A crash between reconcile's spec write and the dev-decision save persisted a pre-reconcile dict; on replay reconcile early-returns and never re-folded `followup_review_recommended`, so the follow-up review could be silently skipped. Fix: persist a defensive copy at record time, and on reconcile's idempotent early-return sync the stale dict from the already-read frontmatter (status + followup). Fold followup only when the frontmatter carries the key — on the generic path that is the same source `devcontract.synthesize_result` reads it from, so a present key can never disagree, but an absent one must not clobber the session's own result.json value to a phantom False. (c) Triage/sweep and labeled plugin-workflow sessions persisted large, never-consumed `result_json` into state.json (`_resumable_session` only ever matches dev/review task ids). The same defensive-copy site gates these to None via `resumable = label is None and role in ("dev", "review")`. Tests: final-cycle clean replay -> DONE (zero re-run, no defer); final- cycle dirty replay -> defer with no extra budget; reconcile early-return heals a stale resumed dict; record isolation under post-record mutation; result_json persisted only for resumable roles. Refs #62, #57 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
_run_sessionrecords the completedSessionRecordwithout usage and saves immediately; usage is folded in afterwards via the newStoryTask.attach_session_usageand a second save lands beforepost_sessionfires. A host kill during usage read, hooks, or follow-up verification can no longer lose a completed session. Usage stays best-effort metadata, never a durability gate.SessionRecordnow persists the session'sresult_json, and_finish_inflightcontinues from a durably-completed dev/review session into the existing verify/decide pipeline (journaledresume-verify) instead of re-driving finished work through resume-restart rollback. Incomplete evidence (record missing / not completed / no result) falls through to today's restart path unchanged. Checked against Hardening: reconcile seam is unreachable for stalled sessions — a finished-but-lost-Stop session cannot be recovered as completed #61 first — different mechanism (that is the live-run stalled-verdict reconcile seam; this is the resume path acting on durably-recorded completed evidence).KeyboardInterruptbranch in the run loop, mirroringRunStopped: kill the agent session, persist a controlled stop (run-stopwithreason="KeyboardInterrupt"), re-raise when nested. The branch latches_stoppingand guardskill_sessionso a second interrupt or a stop signal landing mid-teardown still records the stop.SESSION_END_TRACE_FILE/trace_rundiagnostic scaffolding from the reference patches is ported.Tests
post_sessionfires (spied_emitloads state from disk);read_usageraising leaves the completed session persisted withusage=None.attach_session_usageunit matrix: fold into record + totals,KeyErroron unknown task, no-op onNone, no double-count.result_json) still take resume-restart;SessionRecord.result_jsonserialization round-trip incl. legacy dicts.test_module_skills_sync.pyremain. black/ruff/isort/bandit clean at trunk-pinned versions.Closes #57
Summary by CodeRabbit
New Features
Bug Fixes