Skip to content

session: nothing reclaims a session whose runner died holding it #19

Description

@Shashankss1205

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 — the transition() docstring quoted above: the claim guard, and the admission that nothing detects a dead runner
  • grapharc/session/store.py:420-423runner_pid is stamped on the way into running and cleared on every way out, so a running row always names the process that would have to be dead
  • grapharc/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 row
  • README.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:

  1. Add SessionStore.release_dead_runner(session_id, *, reason="") (name negotiable): under the existing BEGIN IMMEDIATE, verify the row is running, verify runner_pid names 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 legal running -> failed transition with last_error and the transition reason naming the dead pid. failed rather than idle, because a turn that died is a turn that did not settle — and failed is already re-runnable while keeping pending_approval intact, so an open hold survives the reclaim (the same reason _settle keeps holds on a failed turn).
  2. Surface it on SessionManager (e.g. manager.reclaim(session_id)) so an operator does not have to touch the store class directly.
  3. 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.
  4. Update the honesty paragraphs this obsoletes: store.py:393-399, runtime.py:52-54, and README.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.py docstrings and README.md:499 updated to match
  • uv run pytest stays green and uv 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestexperience requiredDeep familiarity with the codebase or domain needed; not a starter taskhelp wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions