-
Notifications
You must be signed in to change notification settings - Fork 13
session: nothing reclaims a session whose runner died holding it #19
Description
Summary
A runner's claim on a session is a compare-and-set, not a lease. SessionStore.transition says so plainly:
That is the whole of the "one runner at a time" guard — it stops a second
runner from *claiming* a session, and it does not detect a runner that died
holding one. A session stuck in `running` after a crash has to be released
deliberately, which is a legal `running -> idle` transition.
But no release path exists anywhere in the package: no method performs that "deliberate" transition, nothing consults runner_pid, and there is no CLI or API to do it either. Verified with a child process that claims the session and dies mid-turn (os._exit, standing in for a crash or OOM-kill):
status after crash: running
runner_pid: 24442 alive: no (dead)
resume attempt: SessionBusy - session 'crashme' is running; expected one of ['idle', 'interrupted', 'awaiting_approval', 'failed', 'created']
Every later Session.run() raises SessionBusy forever. The record even carries the dead runner_pid, so the store knows who died holding it — and offers no way to act on that knowledge short of hand-editing SQLite.
README states the limitation ("a runner claim is a claim rather than a lease — nothing reclaims a session whose runner died holding it"), and #7 defers to "the separate lease issue" in its step 3. This is that issue.
Why this matters
The session layer's headline is durability: "a new process pointed at the same directory can pick a session up by id." A crash is precisely when that promise is needed, and it is precisely when it fails — the one process that could release the claim is the one that died. A session holding a human-approval gate wedges in running with its holds intact and its queued decisions unread; an operator's only remedy is sqlite3 sessions.sqlite "UPDATE sessions SET status=...", which bypasses the lifecycle validation and writes no transition row, corrupting the very audit trail the store exists to keep. Under #7 (HTTP workers restarting) this stops being an edge case and becomes routine.
Where in the code
grapharc/session/store.py:393-399— thetransition()docstring quoted above: the claim guard, and the admission that nothing detects a dead runnergrapharc/session/store.py:420-423—runner_pidis stamped on the way intorunningand cleared on every way out, so arunningrow always names the process that would have to be deadgrapharc/session/runtime.py:52-54— the module docstring's honest bullet: "nothing reclaims one whose runner died holding it"grapharc/session/runtime.py:369-375— the claim itself (expect=RESUMABLE), which is what every future runner loses against a wedged rowREADME.md:499— the documented limitation- Issue server: the HTTP API does not use the durable session layer #7 , "What to change" step 3 — names this as the separate lease issue
Confirm it:
uv run python - <<'EOF' import os, subprocess, sys, tempfile, textwrap from grapharc.session import SessionBusy, SessionManager from grapharc.session.demo import GRAPH_NAME root = tempfile.mkdtemp() m = SessionManager(root) m.create(GRAPH_NAME, session_id="crashme") child = textwrap.dedent(f""" import os from grapharc.session import SessionStatus, SessionStore from grapharc.session.store import RESUMABLE store = SessionStore({os.path.join(root, 'sessions.sqlite')!r}) store.transition("crashme", SessionStatus.RUNNING, expect=RESUMABLE, reason="turn started") os._exit(1) # crash mid-turn """) subprocess.run([sys.executable, "-c", child]) rec = m.store.require("crashme") print("status:", rec.status.value, "runner_pid:", rec.runner_pid) try: m.resume("crashme").run({"inbox": ["hello"]}) except SessionBusy as exc: print("wedged forever:", exc) EOF
What to change
The hard part is semantic — a pid can be recycled, so "the pid is dead" is evidence and "the pid is alive" is not proof the runner is — which is why this proposes a deliberate, recorded release rather than an automatic one:
- Add
SessionStore.release_dead_runner(session_id, *, reason="")(name negotiable): under the existingBEGIN IMMEDIATE, verify the row isrunning, verifyrunner_pidnames a process that no longer exists on this host (os.kill(pid, 0)→ProcessLookupError; refuse when the pid is alive or is our own), then perform a legalrunning -> failedtransition withlast_errorand the transitionreasonnaming the dead pid.failedrather thanidle, because a turn that died is a turn that did not settle — andfailedis already re-runnable while keepingpending_approvalintact, so an open hold survives the reclaim (the same reason_settlekeeps holds on a failed turn). - Surface it on
SessionManager(e.g.manager.reclaim(session_id)) so an operator does not have to touch the store class directly. - Decide whether
Session.run()ever calls it automatically. Recommendation: not by default — pid liveness is host-local and pid reuse makes auto-reclaim a way for two live runners to fight — but this deserves a design comment before code. - Update the honesty paragraphs this obsoletes:
store.py:393-399,runtime.py:52-54, andREADME.md:499("nothing reclaims" becomes "reclaimed deliberately via ...").
Deliberately out of scope: a heartbeat/expiry lease that works across hosts (the store is a local SQLite file; design that under #7 if the HTTP layer ever needs it), and any automatic background sweeper.
How to verify
uv run pytest tests/test_session.py -q
uv run pytest -q
uv run ruff check .The decisive new test is cross-process, using the existing run_child helpers in tests/test_session.py: a child claims the session and os._exits; the parent reclaims, sees a running -> failed transition row naming the dead pid, and then runs the session to completion with every node appearing exactly once. A second test asserts reclaim refuses when the recorded runner is alive (a child parked on a barrier). Revert the source change and watch both go red.
Acceptance criteria
- A session whose runner died can be released without touching SQLite by hand
- The release is refused while the recorded runner is alive
- The release writes a transition row with the reason and the dead pid — the audit trail shows the reclaim rather than hiding it
- Open approval holds survive the reclaim; nothing runs unapproved because of it
- A second runner cannot claim a genuinely running session (existing tests unchanged)
-
store.py,runtime.pydocstrings andREADME.md:499updated to match -
uv run pyteststays green anduv run ruff check .is clean - Any README or cookbook sentence this changes is updated in the same pull request
Skill level — experience required
This spans the store's transaction discipline, the session lifecycle table, and cross-process crash semantics, and the failure modes are the quiet kind: pid reuse making a dead runner look alive, two processes reclaiming at once (the BEGIN IMMEDIATE pattern must carry the liveness check too), and a reclaim that accidentally drops an open hold. Please open a design comment here before writing code — in particular on whether reclaim lands on failed vs interrupted, and on whether run() may ever auto-reclaim. If you have built job-queue or workflow-lease systems, this is a well-shaped one to take.