Skip to content

[BUG] Completed dev session not persisted before post_session hooks/verification (session-end durability gap) #57

Description

@dracic

Summary

When a dev/review session finishes, the engine records the completed SessionRecord only in memory and then runs post-session hooks + follow-up verification before the run state is ever flushed to disk. If the host kills the process (or a Windows console-control event surfaces as a raw KeyboardInterrupt) inside that window, the completed session is lost: on resume the TUI reads a stale dev-running task with no session evidence, and — because status classification then re-drives — this can feed the destructive auto-rollback path with a task that had, in fact, already completed.

This is platform-neutral, but the window is materially wider on Windows, where a completed claude.exe can be slower to release the ConPTY and where console-ctrl events can arrive as a bare KeyboardInterrupt rather than through the installed signal handler.

Where it happens (current main)

1. No durability barrier between record_session and post_session
src/bmad_loop/engine.py _run_session (~L1799–1823):

result = adapter.run(spec)
usage  = adapter.read_usage(result)
task.record_session(SessionRecord(..., usage=usage))   # in-memory only
self.journal.append("session-end", ...)
self._emit("post_session", task, ...)                  # hooks + verification run here
return result

There is no self._save() between recording the completed session and emitting post_session. The completed session only becomes durable at some later _save() after verification/hooks have run — anything that kills the process in between loses it. Usage tally is bundled into the same record_session call, so session durability is implicitly gated on usage metadata being available, even though usage is not a correctness input.

2. Raw KeyboardInterrupt has no branch in the run loop
src/bmad_loop/engine.py main loop (~L239–289):

except RunStopped:
    kill_session(self.state.run_id); ...; self.state.stopped = True
except Exception as exc:      # KeyboardInterrupt is BaseException, not caught here
    ...
finally:
    self._save()

A bare KeyboardInterrupt (e.g. a Windows GenerateConsoleCtrlEvent that does not route through the signal handler as RunStopped) escapes both except arms. finally: _save() does persist some state, but stopped/crashed are never set and kill_session(...) never runs — so the run is left in an ambiguous status with an orphaned agent window still alive.

Impact

  • Lost completed-session record → resume/TUI misreads a finished task as dev-running.
  • That misread can trigger re-drive / destructive auto-rollback on work that was done.
  • Orphaned agent session on raw-KeyboardInterrupt teardown.

Severity: low frequency (narrow kill window), but high blast radius when it hits (destructive rollback of completed work). Sibling to #48 (premature-completion → agent kill), same "engine acts on a mis-read of session state" family.

Proposed fix

A. Make the completed session durable before post-session work, and decouple it from usage tally.
In _run_session, reorder to:

  1. task.record_session(SessionRecord(...)) without usage.
  2. self._save() — the completed session is now on disk.
  3. Compute/attach usage as metadata via a new StoryTask.attach_session_usage(task_id, usage)
    that finds the just-recorded session and folds usage into the token totals.

New model.py helper (src/bmad_loop/model.py, alongside record_session at ~L166):

def attach_session_usage(self, task_id: str, usage: TokenUsage | None) -> None:
    if usage is None:
        return
    for record in reversed(self.sessions):
        if record.task_id != task_id:
            continue
        if record.usage is None:
            record.usage = usage
            self.tokens.add(usage)
        return
    raise KeyError(task_id)

Keep the existing durability _save() before post_session as well, so hooks/verification never run against an unflushed completed session.

B. Add an explicit except KeyboardInterrupt branch in the run loop that mirrors except RunStopped: kill_session(...), set self.state.stopped = True, journal a run-stop with reason="KeyboardInterrupt", and re-raise when nested. This closes the orphan-window / ambiguous-status gap on the Windows console-ctrl path.

Tests

  • Session-end durability: assert _save() is called (session on disk) before post_session is emitted, and that a simulated host-kill after record_session leaves a recoverable completed session rather than a stale dev-running task.
  • attach_session_usage: usage folds into totals for the matching session; KeyError on unknown task_id; no-op on None.
  • KeyboardInterrupt branch: run loop sets stopped, kills the session, journals the stop, and re-raises when nested.

Scope / non-goals

  • Do not port the SESSION_END_TRACE_FILE / trace_run breadcrumb scaffolding that accompanied the reference patches — it was diagnostic weight for a since-closed engine-death investigation and should not land.
  • Usage remains best-effort metadata; it must never gate session-progress durability.

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions