Skip to content

Navigation Menu

Sign in
Sign up

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

Open
Labels
enhancementNew feature or request experience requiredDeep familiarity with the codebase or domain needed; not a starter task help wantedExtra attention is needed

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 — 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request experience requiredDeep familiarity with the codebase or domain needed; not a starter task help wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions

      AltStyle によって変換されたページ (->オリジナル) /