[allenhutchison]

Human overview

When Claude Code hung up right after reading ended, serve could return 0 before the worker had set its real exit code (2 for a refused cursor file, 1 for auth failures). serve now waits up to 5 seconds for that verdict once ended has been pushed. This also removes the flake in test_a_scoped_cursor_file_for_another_room_ends_the_channel.

Closes #159

AI reviewer

Root cause: ChannelWorker pushes ended (via _ended) and only afterwards returns its code to run_worker, which writes outcome["code"]. serve returned outcome["code"] as soon as stdin hit EOF, so an EOF landing between the push and the write returned the initial 0. This is a product race, not only a test race: a session that closes stdin on reading ended loses the non-zero code, and linger/terminate never run.

Change (claude_channel.py):

• _Pusher.ending (threading.Event) is set when an ended push is attempted, before the closed check. serve sets closed and then reads ending, so whichever side moves second sees the other: an ended that passes the closed check is always waited for (Vycari Review VR-572588c6 found the check-then-set window in the first version). A set after serve has looked is harmless; nothing reads it again.
• serve keeps a worker_done Event, set by run_worker right after outcome["code"] is written.
• In the finally, if ending is set, serve waits on worker_done (shielded, in a thread, so the loop stays free) for at most VERDICT_WAIT_SECONDS (5.0).
• Normal EOF without ended is unchanged: no wait, returns 0 as documented.

Choice: product fix rather than holding stdin open in the test. Holding stdin open would hide a real exit-code loss. Alternative not taken: joining the worker unconditionally, because the worker can sit in a blocking HTTP read on ordinary shutdown (same reason as #156).

Known limits: if the worker has not finished within 5s, serve still returns 0 (the initial value); the wait only delays shutdown, bounded. outcome is written on the worker thread and read after worker_done, so no extra locking.

Verification:

• New test test_serve_returns_the_verdict_of_a_worker_that_pushed_ended_before_stdin_closed: the worker returns only after serve has set stop (it is shutting down) and after the test has seen serve return, bounded at 1s. Without the wait serve returns 0 first; with it, serve blocks on the worker, which returns 2. The driver closes stdin in a finally and reports its error. Checked against the original claude_channel.py: fails assert 0 == 2, 3 of 3 runs.
• test_an_ended_push_marks_the_pusher_ending_even_when_the_channel_has_closed pins the set-before-check order (fails on the first version of this PR).
• The original test passed 30 of 30 isolated runs. It never failed in isolation before either, so that run is not evidence on its own; the new test is the proof.
• uv run ruff format --check, ruff check, ty check src/pepper_cli/ clean. Full uv run pytest at 9659cda: 2652 passed, 2 skipped.
• Simplify: diff reviewed directly, nothing to change. code-review (medium): 5 findings, 1 fixed at the time (do not set ending when the channel is already closed; reversed in round 2 for the ordering above), 4 not fixed: two are the bounded 5s delay and the 0-on-timeout above, one is a benign cross-thread read, one was the test's sleep-based timing, which I then replaced with an Event.

Checklist (repo-specific)

• uv run ruff format --check and uv run ruff check clean (no new lint ignores)
• uv run ty check src/pepper_cli/ clean
• uv run pytest passes; coverage floor in .claude/maintainerd.json not lowered
• Server-contract changes: none
• No token or credential is logged, printed, or put in a fixture
• README and the bundled SKILL.md: no command or output change

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lk5Upqk2oCRiV3ZENbLyLL

Confidence Score: 5/5

The PR appears safe to merge; both previous test issues are addressed.

Summary

serve waits up to five seconds for the worker's exit code when stdin closes after an ended push.

• Claude channel shutdown keeps the worker's exit code after ended.

Diagram

sequenceDiagram
    participant Worker
    participant Pusher
    participant Serve
    participant Session
    Worker->>Pusher: Push ended
    Pusher->>Pusher: Set ending before checking closed
    Pusher->>Session: Send ended if open
    Session->>Serve: Close stdin
    Serve->>Serve: Set closed and stop
    Serve->>Worker: Wait for worker_done, up to 5 seconds
    Worker->>Serve: Save exit code and set worker_done
    Serve->>Session: Return exit code

Loading

_{Reviews (2) · Last reviewed commit: "fix(claude-channel): mark ending before ..." · Reviewed by Greptile}

[vycari-review]

Vycari Review (shadow)

Shadow round 2 on 9659cda. This comment is non-gating and needs no response: it is collected to measure agreement with the current reviewer. The round's verdict is the Vycari Review (shadow) check run.

Open: P0 0 · P1 1 · P2 1 · confirmed fixed 0 · withdrawn 0

Judge's notes

Reordering the ending marker fixes the skipped-wait race from the last round. The prior finding that a timed-out verdict wait returns 0 is still open; the PR body discloses it as a known limit. The remaining new finding is a pre-existing stdin-closed-first gap, rated P2.

P1 · VR-c55f8f41 · After the 5 second verdict wait times out, serve returns 0 even though the worker's code is non-zero and still pending

src/pepper_cli/claude_channel.py lines 1228–1231 · symbol serve · lens fail-open · status still-open

When ending is set and worker_done does not fire within VERDICT_WAIT_SECONDS, the finally block proceeds and serve returns outcome['code'], which is still the initial 0. The PR body acknowledges this as a known limit. The result is that a worker that already decided on exit 2 or 1 is reported to the process as a clean exit 0, and the wait only delays shutdown by 5 seconds before doing so.

Evidence: Read serve lines 1223-1232 and run_worker 1206-1215. Timeout falls through to 'return outcome["code"]' with no signal to the caller. The PR body documents the timeout returning 0 as a known limit. Triggering needs the worker to stall in push beyond 5 seconds, which is rare in practice, so the severity is P2.

Failure scenario

The worker has pushed 'ended' (code 2) but is blocked in portal.call, for example because the write stream is not drained, for more than 5 seconds. Stdin closes. serve waits 5 seconds, returns 0, and the process exits 0 while the worker would have returned 2. Scripts that check the exit code see success for a refused cursor file.

How this was verified (traced-callers)

Read serve lines 1223-1232. A timeout of worker_done.wait is ignored and the initial outcome code 0 is returned. This is a documented, bounded limit. Reachable from: stdin EOF after a worker push of ended that stalls longer than 5s.

Proving test

Test: work pushes 'ended' then blocks on an Event for 6 seconds before returning 2; close stdin after wait_notifications(1); assert serve returns 2. On the PR head it returns 0 after about 5 seconds, so the assertion fails. The author's test with a 0.1 second sleep does not reach this path.

P2 · VR-572588c6 · Concurrent shutdown still converts a terminal failure into exit 0

src/pepper_cli/claude_channel.py lines 1113–1116 · symbol _Pusher.push · lens concurrency-time · status still-open

Moving ending.set() before the closed check ensures serve waits, but the closed check can still discard the verdict. If shutdown sets closed after ending is set, push raises ChannelClosedError before ChannelWorker._ended returns its _Ended exception. ChannelWorker.run catches ChannelClosedError and returns 0, so worker_done now publishes a completed but incorrect success verdict.

Evidence: _accept_scope catches the cursor-binding refusal and calls _ended at src/pepper_cli/claude_channel.py:721-726. _ended pushes before constructing _Ended(code) at :401-409. The reordered marker at :1113-1114 permits shutdown's closed assignment at :1229 before the check at :1115-1116. ChannelWorker.run explicitly converts the resulting ChannelClosedError to 0 at :783-784; run_worker publishes that 0 and signals worker_done at :1215-1216. Thus the wait at :1231-1233 does execute and does not time out, ruling out the previously reported skipped-wait and timeout mechanisms. The added closed-pusher test at tests/test_claude_channel.py:1304-1312 asserts the marker and exception but never checks the real worker's resulting verdict.

Failure scenario

A scoped worker detects a cursor belonging to another room and starts pushing ended with verdict 2. After ending.set(), stdin closes and serve sets pusher.closed and stop. The worker resumes, raises ChannelClosedError, and returns 0. serve observes ending, waits successfully for worker_done, and exits 0 despite the refused cursor. The reordered marker therefore leaves the prior failure scenario reachable through verdict replacement rather than a skipped wait.

How this was verified (traced-callers)

Read _ended (push before returning _Ended) and ChannelWorker.run (ChannelClosedError returns 0). Confirmed the closed-before-push race yields 0, which matches the base behaviour and the documented stdin-closed-first semantics. Reachable from: pepper agents claude-channel -> serve (claude_channel.py:1355) -> run_worker -> ChannelWorker.run.

Proving test

Use the mismatched scoped-cursor setup with the real ChannelWorker. Pause the pusher's ending.set() immediately after setting the event; close stdin and wait until serve sets stop, then release the worker. Assert the returned exit code is 2. Head returns 0 even though worker_done completes before the verdict timeout.

[allenhutchison]

Round 2 summary (head 9659cda):

• Greptile, test can miss the race: fixed in b676811. The test orders on serve's stop and on serve returning, not on a sleep. Fails on the original product code (assert 0 == 2, 3/3).
• Greptile, driver errors can hang the test: fixed in b676811 (finally closes stdin, errors reported back).
• Vycari Review VR-572588c6, check-then-set between push and serve: valid, fixed in 9659cda. push now sets ending before it checks closed. serve sets closed and then reads ending, so if the worker passed the closed check, serve sees ending and waits, with the portal and write stream still open, so the ended send goes through and the verdict comes back. The other order (the worker's check sees closed) means ended never reached the session, which is the documented "session closed stdin first" case and returns 0. New unit test test_an_ended_push_marks_the_pusher_ending_even_when_the_channel_has_closed pins the order.
• Vycari Review VR-c55f8f41, 0 after the 5s timeout: not changed. It is the documented, bounded limit in the PR body, and the maintainer already accepted it on this PR. A worker would have to stall inside an ended push for over 5s to reach it.
• PR body: the verification paragraph claimed the earlier test was deterministic, which Greptile showed it was not; rewritten to describe the new test and the revert check.
• Windows CI on 9e3f361 failed on tests/test_tui_live.py::test_a_pin_change_delivered_after_the_thread_is_gone_is_ignored (NoMatches '#thread-new'). That is TUI code this PR does not touch; main's Windows runs are green. Watching the re-run.

Local: ruff format/check, ty, full pytest (2652 passed, 2 skipped) at 9659cda.

[allenhutchison]

Tracking: the timeout and post-hang-up exit codes (VR-c55f8f41) are in #176, and the unrelated Windows flake is in #177.

https://claude.ai/code/session_01Lk5Upqk2oCRiV3ZENbLyLL