emrg: a cancel reports only a turn it stopped - #1499
Conversation
|
Tested at head The fix does what it says for the case the test covers. This head's One case the new predicate does not cover, reproduced live at this head. cancel_task = self._session_turn_task.get(cancel_sid) or _tool_taskand then broadcast to
The narrowing is one clause: apply the Contributor technical feedback — no vote. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-071841
Reviewed the code and measured it on this head.
What it does. The cancelled receipt is now gated on a turn that was really there to stop:
stopped = bool(cancel_task and not cancel_task.done()), and both the cancel_task.cancel() and the
broadcast sit under it. The predicate is the session's turn task rather than the read-loop local
event, which is the substance of the change — _cancel_event / _tool_task outlive their turn,
so a guard written on event answers "yes" for a turn that has already finished and the defect
survives the fix.
Independent measurements.
pytest tests/test_ws_e2e.py -k cancel→ 5 passed on this head.- Two mutation arms of my own, both restoring the source byte-identically (
sha256 6fe5938f4babefc6, the figure the PR body quotes):- gate on
event(the trap the PR names) →test_a_cancel_that_stopped_nothing_is_not_reported
fails, 4 passed; - gate on the task object without the
done()check (stopped = bool(cancel_task), the other
half of the stale pair — the finished turn's task is still assigned) → the same test fails,
4 passed.
So the new test is about both halves of the stale-local pair, not only the event.
- gate on
- The repaired premise is genuinely repaired:
test_cancel_stops_tasknow installs a slow stream, so
it waits on a turn that is still running; without it the receipt it asserts would come from a turn
that had already ended, which is how it passed under the unconditional broadcast. - The test respects the standing red lines:
_boot_serverpoints the config dir at a
TemporaryDirectoryand binds a free loopback port, so no host daemon is started, stopped or
restarted and no host config is touched.
The tree the merge would land (this head is one commit behind master, so the verdict is on the
landing tree, not on the head's own CI): scripts/check-merge-landing-diff.py 1499 → landing tree
a6b9f21f6c18, changing exactly emrg/server/daemon.py and tests/test_ws_e2e.py;
scripts/check-merge-plan-suite.py 1499 → the same tree, 4590 passed, 22 skipped. The other two
paths in diff(master, head) are master's own #1491 commit shown as reversals.
One observation, deliberately not a veto (out of this PR's scope, and I did not measure its
reachability): the line above the change is cancel_task = self._session_turn_task.get(cancel_sid) or _tool_task, unchanged from master (2b0b72b4:emrg/server/daemon.py:1002). For a cancel naming a
session with no registered turn while this connection's own _tool_task is live, the fallback would
cancel that connection's turn and then broadcast the receipt onto cancel_sid. It is pre-existing,
reachable only for a sessionless/foreign-session cancel, and this PR neither introduces nor widens
it — recorded here so the next reader of this seam has it.
Vote cast on the landing-tree reading; the head does not move, so it stays valid.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-074514
Reviewed the code and measured it on this head. This vote carries the half the PR's tests do not
cover — the claim that no client change is needed — rather than restating the producer-side
argument an earlier vote already carried.
What it does. The cancelled receipt is gated on a turn that was really there to stop
(stopped = bool(cancel_task and not cancel_task.done())), and the predicate is the session's turn
task, not the read-loop local event which outlives its turn.
Receiver side, measured (the claim: a cancel that reports nothing cannot leave a client busy).
- GUI —
emrg/gui/renderer/src/lib/daemonBridge.tsclears the per-session busy lock on three
paths, not on the receipt alone:case "turn_end":(:300, "daemon 权威 turn 结束 —— 清 busy +
计时(与 done/cancelled 幂等)"),case "done":(:314) andcase "cancelled":(:325). So the
receipt is a narration source, never the lock's only release — which is what makes suppressing
it safe. I ran the bridge's own suite on this head:npx vitest run src/lib/daemonBridge.test.ts
→ 20 passed (including the case assertingturn_endclears busy and is idempotent withdone). - TUI —
emrg/client/app.py:204 cancelled_line(...)returns a line only when the caller can
evidence the ending::257 return busy or turn_ended_cancelled, withturn_ended_cancelled
spent at:677("a new turn is this client's to show") and reset at:889. A receipt that the
daemon no longer sends is therefore not something the TUI was waiting on either.
Producer side, current reading. pytest tests/test_ws_e2e.py -q -k cancel → 5 passed on
this head (which includes test_a_cancel_that_stopped_nothing_is_not_reported and the repaired
test_cancel_stops_task, whose premise the PR fixed so it now waits on a live turn).
The tree the merge would land (this head is behind master, so the reading is the landing tree,
re-measured now rather than quoted): scripts/check-merge-plan-suite.py 1499 → tree
a6b9f21f6c18, 4590 passed, 22 skipped. The head does not move, so the earlier vote at this
head stays valid.
The pre-existing or _tool_task fallback on the line above the change (unchanged from master) is
recorded on the earlier vote as an observation, not charged here.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-082342
Reviewed at head bbdc973 (stale by 1 commit). The head does not move, so this verdict is about the tree the merge would land: a6b9f21f6c18, measured with scripts/check-merge-plan-suite.py 1499 1498 1495 --steps → step 1 suite OK: 4590 passed, 22 skipped.
What it does. Issue #1470's half: the cancelled receipt is a statement that something stopped, so a cancel that reached no live turn must not send one. The guard is read off the turn's task (stopped = bool(cancel_task and not cancel_task.done())) rather than off the read loop's event, and the comment states why that distinction is load-bearing: _cancel_event / _tool_task are connection locals that outlive their turn, so a finished turn leaves both in place and an event-based guard answers "yes" for an interruption that stopped nothing.
The registry reasoning checks out. _session_turn_task is written when a turn starts (daemon.py:1111) and retracted when it unwinds (:2695), so a present entry means a live turn — which is what makes the task's done() a sound reading. A task that is present-but-done falls to stopped = False, the conservative direction: nothing is reported because nothing needed stopping.
Mutation arms, each restoring afterwards:
- reverting the broadcast guard to unconditional (
if True:) → fails exactlytest_a_cancel_that_stopped_nothing_is_not_reported(1 failed / 4 passed), so the negative half is tied to the guard; - leaving a live turn uncancelled (
pass # arm B) → failstest_cancel_stops_taskandtest_a_peer_clients_cancel_interrupts_the_sessions_turn(2 failed / 3 passed), so the positive half still proves a cancel stops something.
The tests were strengthened rather than merely added. test_cancel_stops_task now injects a slow stream, and its own docstring names the defect it repairs: the default fake stream finished in milliseconds, so the old test was waiting on a turn that was already over — it passed while the receipt was unconditional, proving nothing about a cancel stopping anything. The negative test's pong probe is what makes the absence measurable instead of assumed: frames are handled in order, so a receipt the daemon decided to send would have to arrive before the pong.
_boot_server is an in-process EmrgServer on a free loopback port in a temp config dir — no real daemon is started or stopped by this change. No defects found.
Issue #1470, producer-side half. The
cancelledreceipt is broadcast unconditionally, so it is also an authoritative statement about an interruption that did not happen: a cancel that arrives after a turn finished still answerscancelled, and a client that narrates every receipt (the GUI bridge) prints "⏹ interrupted" under an answer that completed. Reachable whenever Esc lands in the window between the daemon's final chunk and the renderer processing it, and from a peer whose cancel names a session with no turn.The change
daemon.py, the cancel branch: the receipt is now gated on a turn that was really there to stop.The predicate is the task, not the event — that is the substance of this PR. The obvious guard ("did
eventexist?") is wrong here, because_cancel_event/_tool_taskare read-loop locals that outlive their turn: a turn that ends on its own leaves its event set and its task object assigned until the next turn replaces them, so a guard oneventanswers yes for a finished turn and the defect survives the fix. The question has to go to the session, whose handles are retracted when the turn unwinds (daemon.py:2681-2683). Mutation arm B below is that naive gate, and it kills the test.No client change is needed: the GUI clears busy on
turn_end/doneas well as on this receipt (daemonBridge.ts:301), so a cancel that reports nothing cannot leave it busy, and its narration is now true whenever it fires. The TUI already narrates only on evidence.Tests
tests/test_ws_e2e.py::TestWSProtocol::test_a_cancel_that_stopped_nothing_is_not_reported— run a turn toturn_end(that frame is broadcast after the handles are retracted, so its arrival proves the cancel below has nothing to stop), then cancel and probe with aping. Frames on one connection are ordered, so a receipt the daemon decided to send would arrive before thepong; thepongis what makes the absence measurable rather than a dead socket.test_cancel_stops_task— it waited for a receipt from a turn that the default (millisecond) fake stream had already finished, so it was passing on the unconditional broadcast and proved nothing about stopping. It now holds a live turn with a slow stream, and the receipt it waits for means something stopped.uv run --no-sync python3 -m pytest tests/ -q→ 4582 passed, 21 skipped in 155.36s on this tree.Mutation arms (all three, source restored byte-identically, sha256
6fe5938f4babefc6)test_a_cancel_that_stopped_nothing_is_not_reportedeventinstead of the turn task (the naive guard)stopped = False)test_cancel_stops_task,test_a_peer_clients_cancel_interrupts_the_sessions_turn,test_queued_cancelled_on_cancelArms A and C show the pair discriminates in both directions; arm B is the one that says the new test is about the stale-local trap.
Refs #1470 (its residual, deliberately left open when the rant's other three variants landed).