fix: model cross-process session ownership - #48
Conversation
PR screenshotsNo screenshots requested. Add this to the PR description: ```screenshots
/?directory=/tmp/mock-project
full:/sessions/ses_mock_done?directory=/tmp/mock-project |
Status: work stopped mid-redesign — nothing pushedStopped at the user's request. The remote branch and this PR are unchanged. Everything Done locally
1 — durable ownership replaced with run-scoped leases. 2 — the lease is taken before 3 — E2E status mock corrected. It emitted Semantics worth flagging for review: there is no longer a Also rejected explicitly, with a comment in the source so it is not re-proposed: keeping a Not done
Green locally
Do not merge. |
leoncheng57
added
priority:low
The runtime registry kept a claim for every session it had ever seen and
never cleared it, so a session run here once reported current-server/idle
forever. When that session later ran in an external TUI its absence from
this server's process-local /session/status was read as proof of idleness
and a concurrent prompt was accepted. Delete leaked claims too.
Ownership is now run-scoped. A lease is taken for one run and released at
every terminal boundary: run end, abort, delete, startup-grace expiry, TTL
and BFF restart. Absent status is never evidence of idleness again.
The lease is taken BEFORE prompt_async rather than after. prompt_async can
return before the loop reports busy, and the old post-return claim left a
window where a duplicate prompt was accepted, Stop was disabled and the
interrupted banner fired on a run that was about to start. The new
`starting` state closes that window and a failed prompt_async releases the
lease so no phantom run blocks retries.
There is no longer a current-server/idle state. A finished run yields
`completed`, which is informational only: it enables the honest interrupted
banner but grants no authority, so prompting still needs an explicit
confirmContinue (renamed from claimUnknown). The composer surfaces this as
a one-click "Continue here" with the reason, because the instant our run
ends another process could have taken the session.
Rejected, and noted in the source so it is not re-proposed: keeping a
durable claim and invalidating it when time.updated advances without us.
That detects an external run only after it has written, which is exactly
the window a concurrent prompt corrupts.
The E2E mock listed every session as {type:"idle"}; 1.18.21 omits idle
sessions from /session/status entirely. That fixture is why the suite never
caught any of this.
Pushed as
|
| Review item | State |
|---|---|
| 1 — durable ownership → run-scoped leases | Done |
2 — lease before prompt_async, starting state |
Done |
| 3 — E2E mock status semantics | Mock fixed; dependent specs not updated |
4 — merge origin/main (#45, #47, #49) |
Done, clean |
State machine
┌─────────────┐
│ unknown │ ◀── resting state; also every session
└─────────────┘ after a BFF restart
│
begin() ──────┤ requires confirmContinue
▼
┌─────────────┐ grace expires, no busy ever seen
│ starting │ ─────────────────────────▶ unknown
└─────────────┘ abort ──▶ completed
│ status busy/retry
▼
┌──────────────────┐
│ running/retrying │ abort ──▶ completed
└──────────────────┘
│ status absent after busy was seen
▼
┌─────────────┐ TTL expires, or delete
│ completed │ ─────────────────────────▶ unknown
└─────────────┘
abortable is true for starting, running, retrying. Prompting without
confirmContinue is permitted for none of these states — a live lease means a run is
already in flight, so that path returns SESSION_ALREADY_RUNNING instead.
completed grants no authority. It exists so the interrupted banner can fire on a
terminal boundary we personally witnessed, rather than on absent status, which may mean an
external TUI is mid-turn.
CI will be red, and it is my unfinished work
E2E: 19 failing / 145 passing. All fallout from fixing the mock — fixtures that used to
report idle now report unknown, so the composer renders "Continue here" and
POST /prompt returns 409 without confirmContinue, while the specs still assert the old
contract. Not a product regression; the specs need rewriting against the new union.
Green: npm run typecheck, npm test 319/319, npm run build, git diff --check.
New unit coverage: absent status, startup gap, duplicate rejection during starting,
abort during starting, typed abort conflicts, external reuse after a prior local run,
delete cleanup, lease release on prompt_async failure, BFF-restart unknown state,
directory scoping, grace expiry, TTL expiry.
Two open decisions for the reviewer
- Every follow-up turn now goes through the continue path. Taking "released at
terminal boundaries" literally means we cannot prove ownership the instant our own run
ends. I made it one click rather than a modal, but it is a real UX change. The looser
alternative is to letcompletedcarry prompt authority for a bounded TTL — less
honest (a TUI opened seconds after your run ends still slips through), more convenient. AGENTS.mddecision feat: add read-only Plan/Build mode #5 still documents the old ownership model and now contradicts
the code. Not touched in this commit.
Remaining before this is mergeable
- Update the 19 E2E specs to the new runtime contract
- Rewrite
AGENTS.mddecision feat: add read-only Plan/Build mode #5 - Resolve open decision 1 above
Pushed — supersedes my previous comment
CI will be red. That is expected and it is unfinished work, not a product regression. 1 — durable ownership replaced with run-scoped leases
It is now a 2 — the lease is taken before
|
1 participant
Summary
Testing
Addresses blockers G2/G5.