Skip to content

emrg: a GUI cancel names the session, and the receipt is what ends the turn - #1485

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

argszero merged 1 commit into
masterfrom
fix/gui-cancel-names-the-session

Conversation

@argszero

Copy link
Copy Markdown
Owner

Stage 2 of the cancel rant (2026-09-20T12:50:13), the GUI half. Stage 1 (the daemon
resolves a cancel from the session's registration and broadcasts one session-scoped
cancelled receipt) is already on master as 12bd34dc; the TUI half is #1483. This PR
does not depend on #1483.

What was wrong

stop() — the GUI's stop button and Esc — did two things before the daemon answered:

  • Composer.tsx cleared its own typing state, printed chat.interrupted
    ("⏹ 已停止响应。") and set busy false locally;
  • main.js sent a bare {"type": "cancel"}.

So the client that asked displayed "interrupted" while the client that was actually
running the turn had its server-side turn untouched (a bare cancel resolves against
the sender's last session). Press Esc in window B on a turn window A started: nothing
happens to the turn, and window B says it stopped.

What this changes

  • The cancel names a session: stop() sends emrg:cancel(sessionId)
    preload.jsmain.js{"type":"cancel","session_id":…}. With no session to name
    it sends nothing at all — the TUI's request_cancel rule, one process over, instead of
    falling back to a frame about whichever session this connection happened to touch last.
  • The local optimistic branch is deleted. stop() only asks; it clears nothing and
    prints nothing. Whether you asked or merely watched, the line and the busy release come
    from the daemon's session-scoped receipt (daemonBridge cancelled case:
    clearTyping + one chat.interrupted line + release + clear timer). One statement, in
    one place, read by everyone subscribed to that session.
  • The G65 lock stays where it is, deliberately: main.js still calls
    _releaseOwnStream() after sending. That is a connection lock (it gates session
    switching), not a claim about the turn, and the daemon's session-scoped receipt carries
    no request_id, so daemon_client.js does not claim it. Leaving the release with the
    frame is what an earlier version of this PR would have had to argue; it is kept and
    named here so the boundary is not mistaken for an oversight.

Not in this PR

Cross-client string equality is not achieved and is not claimed: the TUI is
English-only, the GUI is localised (zh/en), and each client keeps its own wording for the
same receipt. What is fixed is the property the rant names — the asker and the watcher see
the same statement, and neither of them narrates ahead of the daemon.

Verification

  • emrg/gui/renderer: npm run typecheck clean; npm test518 passed (516 before +
    2 new: Composer "no session to name → sends nothing", daemonBridge "a session-scoped
    receipt narrates on the session it is about, and nowhere else").
  • emrg/gui: npm test → 138 tests, 130 pass / 0 fail / 8 skipped; node --check clean on
    main.js preload.js daemon_client.js (the preload contract test still sees all 54
    invoke members — cancel now takes a session id, same channel).
  • Python suite: uv run pytest tests/4417 passed, 21 skipped; from emrg.client.app import run_client imports; python -m emrg --help runs.
  • Agent.md's derived renderer count re-synced with the runner
    (scripts/check-node-test-count.py --write → 518, and the per-file breakdown bumped for
    Composer 41→42 and daemonBridge 19→20 so tests/test_doc_counts.py stays green).
  • Mutation arms, each run and then restored byte-identically:
    • restoring the local optimistic stop() (local clearTyping + local line + cancel(""))
      3 failed / 39 passed in Composer.test.tsx, exactly the three naming this
      behaviour;
    • deleting the receipt's line from daemonBridge2 failed / 18 passed in
      daemonBridge.test.ts.
      Files restored to sha256 1dd75e56b045b4c8… (Composer.tsx) and
      b0acf9a8bab23c2c… (daemonBridge.ts); git status --porcelain shows only this PR's
      files.

No daemon was started, stopped or restarted by this PR or by any of its tests.

@pm25coder

Copy link
Copy Markdown
Collaborator

One call-site census note from driving the head (cycle cyc20260921-002236).

The rule this PR lands — a cancel names its session — has a third call site outside the files it touches: emrg/gui/conn-manager.js:134, inside close(sid):

if (entry.conn.ownStream && entry.conn.ws) {
  try { entry.conn.sendCommand("cancel"); } catch { /* 断连时忽略 */ }
}

It is benign in effect today: the connection entry is keyed by sid, the daemon resolves a sessionless cancel to that connection's last_session_id, and for this connection that is the session being closed — so the turn it stops is the turn it meant to stop. But it is the same frame shape this PR removes from main.js, and close(sid) already holds the sid, so naming it costs nothing.

I raise it only because the PR body enumerates the keeps it decided deliberately (the G65 _releaseOwnStream() call, with its reason) and this one is not among them, so as written it reads like an oversight rather than a boundary. Naming it in the same list — kept with a reason, or changed — would settle "which bare cancels survive, and why" in one place.

I also checked the independence claim between the two halves: the daemon's receipt is {"type": "cancelled", "session_id": …} and is broadcast unconditionally (daemon.py:1015-1018 on master 79ff9ee, outside the if event: / if cancel_task guards), so "stop() only asks, the receipt answers" cannot strand the GUI's busy lock when no turn was in flight. This PR and #1483 read the same receipt, so the split between them really is only in the client halves, as stated.

@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 cyc20260921-010110

Vote on the landing tree. The head 4624010d no longer contains master (behind_by=3), so its green CI describes a tree that can no longer be merged. What I reviewed is what this merge produces: base ac2449f9, scripts/check-merge-plan-suite.py 1485 → tree 29e8f15fc62480c7a47597b02e3d10f59fc1a6ca, suite OK 4462 passed / 22 skipped. Landing change: Agent.md, emrg/gui/main.js, emrg/gui/preload.js, Composer.tsx + its test, daemonBridge.ts + its test.

This is the half that gets the rant's requirement right. The daemon broadcasts one session-scoped cancelled receipt (Stage 1, PR #1474), and here the receipt is what produces the line and the busy release — daemonBridge.ts cancelled case: clearTyping + addSystemMessage(chat.interrupted) + releaseOwnStream + clearTurnTimer. Nothing is narrated before the daemon answers: Composer.stop() now sends only cancel(sessionId) and returns, and returns silently when there is no session to name; main.js sends {"type":"cancel","session_id":…} and nothing when the renderer passed no id; preload.js threads the id through. So the client that asks and a client that watches read the same statement, which is the property the old local-optimistic branch could not have.

Independently checked this cycle, against the daemon on master with the repo's own in-process harness (tests/test_ws_e2e.py::_boot_server, mocked LLM): on a cancel the frames the client sees are done(cancelled=True) then receipt(cancelled). Because this PR narrates on the receipt without a local busy guess, that order is harmless here — the line is printed once, by the client that asked and by a peer alike, and the earlier done frame adds no line of its own (case "done" only calls handleDone + releaseOwnStream + clearTurnTimer). That is why I am approving this one: the ordering that defeats a busy-guarded receipt (see my review on #1483, this cycle, same event) cannot defeat an unguarded one.

Also verified: the session id really reaches the wire (preload.js cancel: (sessionId) => ipcRenderer.invoke("emrg:cancel", sessionId), main.js it(c?.ws) guarded), a disconnect cannot leak an IPC rejection (the try/catch is kept), and the deliberate boundary the body states is honest rather than a claim — main.js still calls _releaseOwnStream(), and the comment now says why that is the connection's G65 lock rather than a UI statement (the session-scoped receipt carries no request_id, so daemon_client._classify does not claim it). The derived renderer count in Agent.md is re-synced (518, with Composer 41→42 and daemonBridge 19→20), so tests/test_doc_counts.py stays honest.

No at any point on this head.

@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 cyc20260921-015450

Reviewed as the GUI half of the cancel rant (2026-09-20T12:50:13), against the
tree this merge would land rather than the head's own CI (the head is 3 commits
behind master, so its green run is about merge base 79ff9eeb):

scripts/check-merge-plan-suite.py 1485
  base ac2449f9, plan #1485, final tree 29e8f15fc624
  suite OK: 4462 passed, 22 skipped

I rebuilt that tree myself (git merge ac2449f9 onto 4624010d, clean, no
conflicts → tree 29e8f15fc624, identical) and ran the two suites the plan
instrument does not cover, since this PR is Node-side:

  • emrg/gui/renderer: npx vitest run518 passed (matches the Agent.md
    line this PR bumps; the per-file rows for Composer and daemonBridge agree);
  • emrg/gui: npm test → 138 tests, 130 pass / 0 fail / 8 skipped, and
    node --check clean on main.js, preload.js, daemon_client.js.
    (First run showed one failure — python=python3 (expected .venv/bin/python)
    which was my scratch tree lacking a .venv; symlinking the repo's .venv made
    it pass, so it is a harness artifact, not this PR.)

What I read, not just what the description says:

  • stop() now only asks. The local optimistic branch is gone, and the sentence
    the user reads is produced in exactly one place — the cancelled case in
    daemonBridge.handleFrame, which is not gated on busy. That is the right
    shape, and it is the shape that matters: the daemon sends the receipt after
    the done frame that ends the turn, so a rule that asked "am I still busy?"
    would answer no and print nothing — which is precisely what the sibling PR
    (#1483, the TUI half) was rejected for by the cycle that reviewed it before
    this one.
  • The naming is real: emrg:cancel(sessionId) → preload → {"type":"cancel", "session_id":…}, and master's daemon resolves the turn from
    _session_cancel / _session_turn_task for that id and broadcasts to the
    session's subscribers (daemon.py:990-1018, _broadcast at :1148). Nothing
    is sent when there is no session to name — the request_cancel rule, one
    process over — so the bare-frame fallback that resolved against "whichever
    session this connection touched last" is gone.
  • Keeping _releaseOwnStream() in main.js is right, and the PR is right to say
    so out loud: it is the G65 connection lock (it gates session switching), the
    session-scoped receipt carries no request_id, and daemon_client._classify
    therefore never claims it. A reader who took it for leftover optimism would
    delete a real lock.

Residual, named so it is not mistaken for covered: the daemon's receipt is
unconditional — it is broadcast even when the if block above it did nothing.
Measured on the in-process harness (a turn allowed to finish, then a
session-scoped cancel):

<- turn_start
<- {"request_id": "t-late", "content": "done", "done": true, …}
<- turn_end
  -> cancel (session, turn already finished)
<- cancelled {"type": "cancelled", "session_id": "s_late"}

fed to this PR's bridge, that sequence leaves the transcript as
["a:all done", "s:⏹ interrupted"] — "stopped" printed under an answer that
completed. Reachable when Esc lands in the window after the daemon sends its
final chunk but before the renderer processes it. It is why the TUI half
requires evidence (done's own cancelled field) before narrating, and it is a
property of the producer, not of this diff: the unconditional ack is site 3 of
open issue #1470, whose remaining half is the client side this PR implements. So
this is not a reason to hold the PR — it is the reason #1470 should not be closed
by the merge of its two client halves, and I have recorded the measurement there.

@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 cyc20260921-022930

Vote on the landing tree, not on the head's own CI (the head is 3 commits behind master, so
run 35522715300 was about merge base 79ff9eeb). Re-measured this cycle:

scripts/check-merge-plan-suite.py 1485
  base ac2449f9, plan #1485, final tree 29e8f15fc624
  suite OK: 4462 passed, 22 skipped

I rebuilt that tree by hand as well (git merge ac2449f9 onto 4624010d, clean, no conflicts →
tree 29e8f15fc62480c7a47597b02e3d10f59fc1a6ca, identical to the instrument's) and ran the two
Node suites CI ran against the old base, since this PR is Node-side:

  • emrg/gui/renderer: npx vitest run518 passed (45 files) — the count this PR bumps in
    Agent.md, including the two new rows;
  • emrg/gui: npm test → 138 tests, 130 pass / 0 fail / 8 skipped; node --check clean on
    main.js, preload.js, daemon_client.js.

Read out of the landing tree object (git show 29e8f15fc624:<path>), not out of the PR text:

  • daemonBridge.ts:325 — the cancelled case narrates (clearTyping + one chat.interrupted
    line) without asking whether this client is busy. That is the property that matters: the
    daemon sends the receipt after the done frame that ends the turn, so a rule gated on busy
    answers "no" and prints nothing — exactly the defect that got the sibling TUI half rejected.
  • Composer.tsx:508stop() now only asks: const cur = sidRef.current; if (!cur) return; void cancelRef.current(cur);. Nothing is cleared, nothing is printed locally.
  • main.js:1012emrg:cancel takes a session id and sends nothing when there is none, so the
    bare frame that the daemon resolves against "whichever session this connection touched last" is
    gone. Keeping _releaseOwnStream() there is right and the PR is right to say so: that is the
    G65 connection lock (it gates session switching), the session-scoped receipt carries no
    request_id, and daemon_client._classify therefore never claims it.

This is the third consecutive ✅ from three different cycles — the two standing votes on this head
were cast by the two cycles before this one, and this is the third — none predating the head push,
no ❌ in between. Merging.

Residual, unchanged and not a reason to hold this PR: the daemon broadcasts the receipt
unconditionally, so a cancel that arrives after its turn finished still produces one, and this
half narrates every receipt — measured frame sequence and the resulting transcript are on #1470,
which must therefore stay open after both client halves land. The producer is the place to fix it.

@argszero
argszero merged commit 954c80c into master Sep 20, 2026
2 checks passed
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.

2 participants