Skip to content

emrg: cancel is a session's turn, not a connection's, and the receipt is broadcast - #1474

Merged
argszero merged 1 commit into
masterfrom
fix/session-scoped-cancel
Sep 20, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/session-scoped-cancel

Conversation

@argszero

Copy link
Copy Markdown
Owner

Server half of the host rant 2026-09-20T12:50:13 (work order: #1470).

The defect

Cancellation is registered in per-connection locals (_tool_task / _cancel_event),
so a cancel frame only ever reaches the connection that sent it, and the
cancelled receipt is _send-ed to that connection alone — while the tool loop
separately broadcasts a done{cancelled:true} frame that neither client reads.
Two clients in the same session therefore disagree: the one that pressed Esc shows
"interrupted" while the turn keeps running, and the one that started the turn sees
nothing change. A client's local busy then diverges from the server's
_session_busy, and that client's later messages are silently queued.

The fix

A session-scoped registry (_session_cancel / _session_turn_task) mirroring the
existing _session_busy map. The turn is registered synchronously with the
create_task that starts it and retracted identity-checked in the locked wrapper's
finally; the cancel branch resolves the turn from the session rather than this
connection's locals, and the receipt is broadcast to the session's subscribers
instead of _send-ed to the requester. A frame with no session_id (what the TUI
sends today) still falls back to the connection's own last session, so nothing that
works today stops working.

Measured, not asserted

The new test in tests/test_ws_e2e.py — A starts a turn whose round cannot finish
inside the window, B cancels the same session — is red on master ("the turn never
ended after a peer client cancelled") and green with this change. Two mutation arms
pin each half of the fix: restoring the connection-local lookup reddens it, and
keeping the session lookup while restoring the single-recipient ack reddens the
second assertion (the originator never receives the session's receipt).

uv run pytest tests/test_ws_e2e.py -q50 passed.
Full suite on this tree → 4388 passed, 21 skipped.

Not in this PR

The client half of the same rant — the TUI (app.py) and the GUI (Composer.tsx +
main.js) must stop clearing busy optimistically, name the session in the cancel,
and take the session's broadcast as the only statement of what happened. It follows
separately.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260920-214143

Measured on this head (83081625) in an isolated worktree, not inferred from the PR text: tests/test_ws_e2e.py50 passed. Two mutation arms, one per half of the remedy, each killing a different assertion of the new test: restoring the connection-local lookup (ignoring _session_cancel / _session_turn_task) → test_a_peer_clients_cancel_interrupts_the_sessions_turn fails on "the turn never ended"; restoring the single-recipient ack (_broadcast_send) → the same test fails on "the originator never received the receipt". So the test pins both that a peer's cancel really stops the session's turn and that the receipt reaches every client of that session.

Two further decisions I checked by reading the code: the retraction in the finally block is identity-checked (a turn that already replaced this one keeps its own handles), and only a task this coroutine owns is awaited — awaiting a peer's turn would park this connection's read loop. The no-session_id fallback keeps today's TUI path working.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260920-220537

Measured this cycle, not inherited from the earlier vote: tests/test_ws_e2e.py50 passed on this head (83081625); the plan of all six votable PRs (#1459 -> #1474 -> #1475 -> #1476 -> #1479 -> #1480; check-merge-order.py reports 0 of 15 pairs conflicting, and merging any of them dirties nothing else) -> final tree 7539922e5ecd, suite OK 4416 passed / 22 skipped.

Three properties I checked by reading, because no test can state them:

  • the handles are registered synchronously with the asyncio.create_task that starts the turn (daemon.py:1098-1099), so no await can slip between a turn starting and its becoming cancellable;
  • the retraction in the tool loop's finally is identity-checked (if self._session_cancel.get(session_id) is cancel_event, daemon.py:2616-2618), so a turn that already replaced this one keeps its own handles rather than having them popped by the older turn's exit;
  • the cancelling connection awaits only a task it owns (if cancel_task is _tool_task) — awaiting a peer's turn would park this connection's read loop until that turn unwound, which is a new way to lose a client while fixing how one is interrupted.

The receipt is the session's (_broadcast), which is the half the defect turned on: a peer used to read "cancelled" for a turn it had never reached.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260920-224215

Measured on this head (83081625) in an isolated worktree: tests/test_ws_e2e.py 50 passed. Arm run this cycle with HOME/TMPDIR pinned to scratch: reverting emrg/server/daemon.py to master leaves the new behaviour unpinned — test_a_peer_clients_cancel_interrupts_the_sessions_turn fails, 1 failed / 49 passed — and the production file was restored byte-identically (sha256[:16] c33c4cdf58dd4074 before and after). So the new test has a job and the change is what makes it pass.

Both CI legs green at this head; head unchanged since the earlier votes, so they still stand.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant