fix(1096): a resume must not announce a tab the graph route cannot reach - #1134
Draft
artokun wants to merge 8 commits into
Draft
fix(1096): a resume must not announce a tab the graph route cannot reach#1134artokun wants to merge 8 commits into
artokun wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
Author
NO-SHIP — the approach is wrong, not the details. Holding.The review invalidated the premise rather than the implementation: a full hello is not a silent repair. My commit claimed "the session, the thread and the agent are untouched", corrected that once for the clean-agent case, and it is still wrong for three more reasons — each CONFIRMED. A benign blip now injects a false "you dropped mid-task" turn into a live agentA hello is answered with a full handshake, including a
The user reads a false statement, and the agent restarts or duplicates what it is doing. Before this diff the branch sent nothing, so no ready ack could be manufactured on a surviving bridge — I created this. It re-injects the canvas-tool disclosure into a session that already proved its toolsThe landed hello bumps The re-greeting noise #588 bannedA hello re-greets ("agent ready") and re-runs handshake side effects, now on every ComfyUI blip more than 5s apart. And the debounce is structurally wrong twice over
The test proved nothing (mutation-verified)A verifier copied the branch, inverted the debounce and relocated the send, and my test still passed. Token-presence assertions cannot express the ordering and nesting claims the test names. What this actually means for #1096The important consequence is bigger than this PR: #310's So the honest ordering is:
Step 1 is the real work and is worth its own issue. Holding this branch rather than shipping a change that turns tab refocus into a false mid-task claim. |
artokun
added a commit
that referenced
this pull request
Aug 12, 2026
The panel injects a user message — "✅ Your connection dropped mid-task … continue exactly what you were doing" — plus a transcript line, whenever a `ready` ack arrives with MID_TASK_KEY armed. Both are false unless the orchestrator really died, and the injection is worse than false: telling an agent that is still working to resume makes it restart or duplicate what it is doing. The existing guard's INTENT was already right, and its comment says so: a fast reconnect "means the orchestrator never died — the agent's turn kept running — so a 'you dropped' nudge is false AND would inject a spurious turn into a live session". The sentinel's arithmetic betrayed it. `lastBridgeDownAt` is 0 until the bridge socket CLOSES, and `Date.now() - 0` is ~56 years — the LONGEST possible gap, which a long-gap-means-real-restart heuristic reads as the strongest possible evidence. So the guard was exactly inverted in the case it exists to catch: the better established that nothing had dropped, the more confidently it nudged. Reachable on a LIVE socket, because `ready` repeats on one (the client's own note) and a re-advertise draws a fresh handshake — and #310 re-advertises after every successful free_vram by design. So freeing VRAM mid-task could tell a user their connection had dropped when nothing in the session ever had. Found while reviewing #1134, where I proposed re-advertising on every ComfyUI reconnect to repair a dropped tab mapping (#1096). That would have widened this from one trigger to every reconnect. #1134 is withdrawn; this is the prerequisite. The decision moved into session-rebind.js so it is tested by BEHAVIOUR, not by grep: #1096's review proved by mutation that a token-presence source scan over the panel file stays green when the guard is INVERTED, which is the single regression that matters here. One test asserts the never-dropped and real-restart answers must differ, so an inverted or deleted condition cannot pass. Also handled, because they are the same mistake one step along: a NEGATIVE gap (a clock adjustment, or a drop stamped after the read) is not evidence of an ancient drop, and an unreadable clock decides nothing. Both refuse. SWEPT the four sibling `Date.now() - x` guards; this was the only one wrong, and why is worth recording. `lastFailedMismatchRehelloAt` guards `x && …` explicitly. `rebootResumeMidAt` is gated on a companion non-null flag. For `localEndAt` the zero case SHOULD proceed ("never ended locally" ⇒ nothing to suppress). `lastBridgeDownAt` is the only one whose zero case must SUPPRESS — the opposite polarity to what the subtraction yields, which is why the sentinel bit here and nowhere else. A real restart still nudges, including exactly at the 6s boundary. 3975 tests pass (3974 + 1 todo), typecheck clean, node --check OK. Refs #1138
artokun
added a commit
that referenced
this pull request
Aug 12, 2026
#1139) * chore(1138): claim — a live-socket re-advertise must not read as a fresh connect * fix(1138): a mid-task "you dropped" nudge needs a drop to have happened The panel injects a user message — "✅ Your connection dropped mid-task … continue exactly what you were doing" — plus a transcript line, whenever a `ready` ack arrives with MID_TASK_KEY armed. Both are false unless the orchestrator really died, and the injection is worse than false: telling an agent that is still working to resume makes it restart or duplicate what it is doing. The existing guard's INTENT was already right, and its comment says so: a fast reconnect "means the orchestrator never died — the agent's turn kept running — so a 'you dropped' nudge is false AND would inject a spurious turn into a live session". The sentinel's arithmetic betrayed it. `lastBridgeDownAt` is 0 until the bridge socket CLOSES, and `Date.now() - 0` is ~56 years — the LONGEST possible gap, which a long-gap-means-real-restart heuristic reads as the strongest possible evidence. So the guard was exactly inverted in the case it exists to catch: the better established that nothing had dropped, the more confidently it nudged. Reachable on a LIVE socket, because `ready` repeats on one (the client's own note) and a re-advertise draws a fresh handshake — and #310 re-advertises after every successful free_vram by design. So freeing VRAM mid-task could tell a user their connection had dropped when nothing in the session ever had. Found while reviewing #1134, where I proposed re-advertising on every ComfyUI reconnect to repair a dropped tab mapping (#1096). That would have widened this from one trigger to every reconnect. #1134 is withdrawn; this is the prerequisite. The decision moved into session-rebind.js so it is tested by BEHAVIOUR, not by grep: #1096's review proved by mutation that a token-presence source scan over the panel file stays green when the guard is INVERTED, which is the single regression that matters here. One test asserts the never-dropped and real-restart answers must differ, so an inverted or deleted condition cannot pass. Also handled, because they are the same mistake one step along: a NEGATIVE gap (a clock adjustment, or a drop stamped after the read) is not evidence of an ancient drop, and an unreadable clock decides nothing. Both refuse. SWEPT the four sibling `Date.now() - x` guards; this was the only one wrong, and why is worth recording. `lastFailedMismatchRehelloAt` guards `x && …` explicitly. `rebootResumeMidAt` is gated on a companion non-null flag. For `localEndAt` the zero case SHOULD proceed ("never ended locally" ⇒ nothing to suppress). `lastBridgeDownAt` is the only one whose zero case must SUPPRESS — the opposite polarity to what the subtraction yields, which is why the sentinel bit here and nowhere else. A real restart still nudges, including exactly at the 6s boundary. 3975 tests pass (3974 + 1 todo), typecheck clean, node --check OK. Refs #1138 * fix(1138): persist the drop timestamp, and CONSUME it Requiring a recorded drop closed the false positive but opened a false negative I should have caught before pushing: MID_TASK_KEY lives in sessionStorage and survives a page reload, while lastBridgeDownAt is module state and does not. So "reloaded the browser after a real ComfyUI restart" — a case the nudge genuinely exists for (#278/#588) — would have stopped nudging, because the marker survived and the evidence did not. The two readings are now symmetric: the timestamp is written beside the marker on every bridge close. Only a session that never observed a drop AT ALL suppresses, which is exactly the case where the panel cannot know whether the agent kept running, and where suppressing is the safe answer. That persistence then had to be consumed, and noticing why is the more interesting half. A drop authorizes ONE nudge. Left standing, an hours-old timestamp keeps passing the long-gap test, so the next mid-task `ready` ack — and a free_vram re-advertise draws one (#310) — would nudge on evidence from a drop already accounted for. That is precisely the false positive this change removes, re-entering through the door the persistence opened. So the timestamp is cleared where MID_TASK_KEY is cleared, for the same reason and in the same place. `Number()` of an absent value is NaN, which the predicate refuses, so an unreadable or empty store reads as "no drop recorded" rather than as 0-treated-as-a-date — the original bug's shape, kept out of its own fix. 3975 tests pass (3974 + 1 todo), typecheck clean, node --check OK. Refs #1138 * revert(1138): drop the persisted timestamp — the simple guard was the safe one Reverting my own second commit. The review returned 14 confirmed findings on this branch and the great majority are against that commit, not the fix underneath it. I added it to close a false negative and it introduced a family of leaks instead, because it introduced STATE and state has exits I did not enumerate: - Written on every bridge close but consumed only inside the mid-task branch, while the reboot-resume and soft-reload branches `return` first — so a drop observed with no turn in flight survives to authorize a later nudge. Exactly the hazard I believed I had closed by consuming it, closed on one path out of three. - The fresh-session path clears MID_TASK_KEY but not this key, so a brand-new agent session inherits the previous session's drop. - handleRebootResumeAck retires MID_TASK_KEY specifically so the generic nudge cannot fire for the same drop; it does not retire this, which re-opens what that retirement bought. - Chrome COPIES sessionStorage into a duplicated tab, so a duplicated tab inherits a drop it never observed and nudges a live agent mid-turn. The reload false negative it was meant to fix is real but cheap and self-announcing: an idle resumed session that was not nudged is visible to the user, who can prompt it. Every defect above is silent and injects a turn into a working agent, which is the failure this whole issue is about. Keeping the guard simple — require a drop THIS page observed — is the trade worth making, and the honest note is that the plain version was already correct. Also corrected, since it was wrong in the reverted comment AND its commit message: `ssGet` returns null for an absent key and `Number(null)` is 0, not NaN. The behaviour was still right (0 is refused) but the stated reason was not — the same asserting-without-checking I had claimed to keep out of this fix. Left standing for their own issue, both PRE-EXISTING and neither introduced here: every FAILED reconnect attempt re-stamps lastBridgeDownAt, so the gap can measure the backoff delay rather than the outage and a long restart may read as short; and the nudge's other ready-ack branches were never audited for the same sentinel exposure. 3975 tests pass (3974 + 1 todo), typecheck clean. Refs #1138 * test(1138): pin the call site's POLARITY, which a pure test cannot see The predicate is tested by behaviour, but nothing covered the one property that lives at the call site: that it is NEGATED. Dropping the `!` would invert the fix into "nudge only when nothing dropped" — worse than the original bug and invisible to every existing test. An exact-substring assertion on purpose. #1096's review proved by mutation that loose token-presence scans over this file assert nothing, so this claims the whole guard line rather than the presence of its parts, and separately asserts the reverted sessionStorage twin has not returned (every confirmed leak on this branch came from persisting it). 3976 tests pass (3975 + 1 todo), typecheck clean. Refs #1138
The reporter had a resumed session announced with an active Unsaved Workflow while every panel_graph_outline answered `no connected tab with id tmp:2806… Connected: none`. So the agent believed it had a live canvas to finish a partially-applied mutation sequence against, and had none. Not a new mechanism — the mechanism was already measured. #310 established that a ComfyUI bounce drops the orchestrator's tab mapping and that the next graph tool then answers "Connected: none" until this tab re-advertises. It wired that conclusion to `free_vram` alone, because free_vram was the bounce in hand. On `reconnected` the panel asks shouldResumeAfterComfyReconnect whether to respawn: bridge DOWN → connectAgent(), whose fresh connect sends a hello, so the mapping is re-established as a side effect. Covered. bridge UP → the guard declines, CORRECTLY (bouncing a live session is the spurious "you reconnected" #278 removed) — and the handler then returned having done nothing at all. The mapping stayed dropped underneath a session that had just said it resumed. That second row is #1096. The fix is the cheapest frame available: re-advertise on the EXISTING socket. It is not a bounce and does not reopen #278 — the orchestrator carries routing state across a same-socket re-hello explicitly ("the shared session continues", branch does not connect, does not re-resume, and prints no chat line. DEBOUNCED, because a hello is not free and I measured that today rather than assuming it: the orchestrator runs its panel sync on every hello (~1s), and that is the step which blocks on the panel operation lock — a stale lock from a dead pid made it time out at 60s per hello. `reconnected` can fire repeatedly while ComfyUI's socket flaps, so one re-advertise per 5s window; monotonic, so a wall-clock adjustment cannot make the gap look negative and re-arm it early. The predicate is complementary to shouldResumeAfterComfyReconnect on the bridge axis, and a test asserts exactly one of the two acts — so a later edit cannot leave the case uncovered again, nor make both fire. Scope stated: this repairs the tab mapping. The `Connected: none` TEXT is composed by the orchestrator from its own registry, so the panel can only fix the fact, not the wording. 3974 tests pass (3973 + 1 todo), typecheck clean, node --check OK. Refs #1096
Correcting my own previous commit, which asserted "the session, the thread and the agent are untouched". That was not established, and reading `sendHello` shows it is wrong in one state: its header records that a hello BINDS the socket to an agent session and "not always the one it was bound to a moment ago", and that with the session key CLEARED the hello "spawns a clean agent outright" — `hello` is in AGENT_SESSION_RESET_FRAMES for exactly that reason. So the previous version could turn a ComfyUI blip into a spurious agent START whenever no session was live: the same class #278 removed, arriving through a different door. And it would have bought nothing, because with no session there is no agent for a graph tool to route to, so the dropped tab mapping harms nobody until the next connect — which hellos on its own anyway. `hasResumableSession` is therefore a REQUIRED input, not a refinement: with a session the hello rebinds, which is the whole intent; without one it must not fire. The call site reuses the value already computed for the resume decision, and a test pins that the input is wired rather than left to its default. Recorded rather than dropped: a hello also bumps `agentSessionEpoch`, which is how the panel separates a straggler command from the outgoing agent from evidence about the new one. That affects the canvas-tool DISCLOSURE inference, not whether a command executes — so it is a reason to keep this debounced and conditional, not a reason to abandon it. The complementarity test is now stated properly too: with a resumable session exactly one of the two reconnect paths acts on either side of the bridge axis, and with NO session both correctly decline. The gap this fix closes was "both declined", so that is the invariant worth pinning. 3974 tests pass (3973 + 1 todo), typecheck clean, node --check OK. Refs #1096
artokun
force-pushed
the
fix/1096-resume-before-socket
branch
from
August 13, 2026 04:03
e628e83 to
adfe9fd
Compare
Review found the re-advertise stamping its 5s window BEFORE the send. That is the "recorded before it happened" defect this file already names one screen up, at the #607 re-hello, using the same 5000ms constant: "Stamping either before the send is the recurring ... defect (#621's canvas lock holder, #836's panel-op release, agentSessionEpoch in #633)." The consequence is specific to what this change exists to do. A hello can be refused outright when no route identity is established yet, or dropped when the socket is superseded mid-send. Either spends the debounce window without re-advertising anything, so the dropped tab mapping -- the entire subject of #1096 -- stays dropped for the full gap while the panel believes it has just repaired it. The window is now armed inside the send's resolution, and only when the hello actually landed. Not stamping on failure cannot storm: this branch requires a live bridge, and a hello that failed to land means the socket is gone or superseded, in which case the predicate declines until it returns and the reconnect path's own open-hello re-registers the tab anyway. The existing wiring assertion could not see this. It matched the token `lastReconnectRehelloAt = now`, which reads identically before the send and inside its resolution -- so it passed against both shapes. It now asserts the ORDERING against the send, plus the landed-only condition. Mutation-checked: restoring the stamp to before the send fails it. Remaining review findings are NOT addressed here and the PR stays draft. The largest is that a re-advertise draws a fresh `ready` ack whose handler consumes MID_TASK_KEY before deciding, so a benign ComfyUI blip can silently disarm the resume nudge for the rest of a turn. That hole is pre-existing -- #310's free_vram re-advertise does the same -- but this change adds a far more frequent trigger for it, and it interacts with the #1145/#1163 nudge work now on main, so it wants its own reasoning rather than a reflex guard. 4101 unit tests pass; typecheck and node --check over web/js are clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wice Review (CONFIRMED) found the wiring test deriving the predicate's argument slice twice under two names. `src.indexOf(needle, 0)` is the same call as `src.indexOf(needle)`, so `callAt === at` and the two slices were byte-identical -- verified by executing both against the branch's own panel source. Harmless to correctness, but it read as two independent checks of different regions when it was one region checked twice, which is the kind of thing that makes a suite look broader than it is. Sliced once, reused. 4101 unit tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
Review outcome: this should not merge in its current formA high-effort review of the rebased branch returned two CONFIRMED correctness defects, and both come from the same root: a hello is not a cheap frame. The change treats "re-advertise the tab" as a light repair, but a landed hello has three established side effects, and firing one on every benign ComfyUI reconnect drags all three along. 1. The rehello disarms the mid-task resume nudge (CONFIRMED)
So: agent is mid-build, the user opens an asset preview, ComfyUI's WS blips, this code rehellos, the orchestrator answers This directly regresses #1138 / #1145 / #1163, which are already on main. #310's 2. The rehello re-injects the canvas-tool disclosure (CONFIRMED)Every landed hello bumps Concretely: twenty turns into a working session, a blip happens while the user reads a reply; they type "make the sampler 30 steps" and get a tool-availability lecture and an upgrade checklist. Once per blip. Also raised
Fixed on the branch already
What I think this needsNot a patch. The premise — repair the mapping with a full hello — is what carries the side effects, so guarding each one individually would mean three new guards around a frame that was chosen for being cheap and is not. Worth considering instead: a frame that re-advertises the tab without the hello's session semantics, so no Leaving this draft rather than merging it. The rebase itself is sound and verified — 65 → 69 tests with nothing lost, all 10 main exports intact — so the branch is a good base for whichever direction is chosen. One rebase blemish worth recording: one line was dropped from an intermediate commit message ( |
…survive it The review's one CONFIRMED test finding was that the #1096 source scan stayed green when the guard was negated and the debounce inverted — the exact technique this test file already cites at #1138 for disproving such scans. So the window, the in-flight latch and the arm-only-when-the-hello-LANDED ordering move out of the call site into `createReconnectRehelloGate` in session-rebind.js, where they are asserted against observed behaviour. The harness records the SEND, so "the socket is not up, therefore nothing left the panel" is now a claim about the frame rather than about a returned boolean — which is the negative case this whole class of bug turns on. Eleven mutations were applied to the result (inverted window, stamp moved ahead of the send, stamp on a refused hello, dropped predicate, truthy-instead-of-true, removed latch, un-dispatched call site, wall clock, assumed socket, stubbed send) and all eleven are killed. Also closes the review's PLAUSIBLE source mismatch. The guard read `hasResumableSession` from the active thread RECORD while the hello it gates carries `resume` from SESSION_KEY. The two can disagree, and when they do the guard passes while the frame goes out with `resume: null` — which by sendHello's own contract spawns a clean agent, the spurious start this input exists to prevent. It now asks the source the frame itself will read. The two CONFIRMED correctness defects are NOT addressed, and are recorded in the predicate's header rather than reassured away — the previous note called the epoch bump "worth knowing", which reads as a dismissal of a user-visible effect: - a landed hello bumps agentSessionEpoch, invalidating canvasToolsProvenEpoch, so a session that had proven its canvas tools is re-asked and the user's next message is prefixed with the disclosure paragraph; - the ready ack consumes MID_TASK_KEY before deciding whether to nudge, so a re-advertise disarms the mid-task resume nudge with outageMs 0 and a later real restart finds no marker. Both live inside the hello path rather than in the decision to re-advertise, and both are already reachable on main through the post-free-VRAM re-advertise. The remedy named in review is a re-advertise that does not carry the hello's session semantics; that is its own change and it gates this one. Refs #1096
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #1096.
A ComfyUI
reconnectedwhere the bridge survived leaves the orchestrator's tabmapping dropped, and nothing re-establishes it. Every graph tool then answers
Connected: nonewhile a resumed session sits on screen saying otherwise — thereporter's
panel_graph_outlinecalls against a tab id the orchestrator no longer held.The uncovered case
On
reconnectedthe panel asksshouldResumeAfterComfyReconnectwhether to respawn:re-established as a side effect.
false, correctly — bouncing a live session is thespurious "you reconnected" Reconnect active workflow session after tool-triggered ComfyUI reboot #278 removed — and the panel then returns having done
nothing. The mapping stays dropped.
That second branch is #1096. This branch re-advertises the tab on the existing socket.
What the announcement is gated on
Only on things that were observed, never on what is expected:
client.isConnected(), read at the moment of the attempt — not a cached flagBoolean(ssGet(SESSION_KEY))— the same sourcegetResumereadssendHelloresolving literallytrue; only then is the debounce window armedNothing is announced to the user: no chat line, no reconnect, no session frame.
Changed since the review
The debounce is now a unit, not an inline block. The review's one CONFIRMED test
finding was that the source scan stayed green when the guard was negated and the debounce
inverted. The window, the in-flight latch and the arm-only-when-landed ordering moved into
createReconnectRehelloGateinsession-rebind.js, and the tests assert on the sendrather than on a returned boolean — so "the socket is not up, therefore nothing left the
panel" is a claim about the frame. Eleven mutations were applied and all eleven are
killed: inverted window, stamp ahead of the send, stamp on a refused hello, dropped
predicate, truthy-instead-of-
true, removed latch, un-dispatched call site, wall clock,assumed socket, stubbed send, and the anchor check that proves each mutation applied.
The session source mismatch (review: PLAUSIBLE) is fixed. The guard read
hasResumableSessionfrom the active thread record while the hello it gates carriesresumefromSESSION_KEY. When those disagree the guard passes and the frame goes outwith
resume: null, which bysendHello's own contract spawns a clean agent — thespurious start the input exists to prevent. It now asks the source the frame will read.
The reassuring comment is gone. The predicate's header used to file the epoch bump
under "worth knowing"; it now records both open defects as open.
What still blocks this
Both are inside the hello path, not in the decision to re-advertise, and both are
already reachable on
mainthrough #310's post-free_vramre-advertise. Both werere-read in the merged tree on 2026-08-14 and are unchanged:
The re-advertise disarms the mid-task resume nudge. The orchestrator answers a
hello with a
readyack even on a live socket, and the ack handler consumesMID_TASK_KEYbefore it decides whether to nudge. The marker is spent withoutageMs: 0, so a later genuine restart finds nothing and the resumed agent sits idleover half-finished work. Regresses A re-advertise on a live socket injects a false "you dropped mid-task" turn and re-asks a proven agent for its tools #1138 / The mid-task nudge measures one backoff step, not the outage — a fast-returning restart loses its nudge #1145 / The mid-task nudge can fire on a turn the user just started, and never fires when the panel itself respawns the orchestrator #1163.
Evidence,
web/js/comfyui-mcp-panel.js(merged tree): the handler opens at 28574with
if (ack?.kind === "ready" && ssGet(MID_TASK_KEY)) {and clears itunconditionally on the very next line, 28575 —
ssSet(MID_TASK_KEY, null);— whilethe decision that should have gated that clear is 31 lines later, at 28606,
if (!shouldNudgeAfterMidTaskReconnect({ outageMs: bridgeOutage.outageMs() })) return;.A re-hello on a live socket reaches the clear with
outageMs()at0; the guard thenreturns
false(session-rebind.js:91,if (outageMs <= 0) return false;) and thehandler returns — but the marker is already spent.
The re-advertise re-injects the canvas-tool disclosure. A landed hello bumps
agentSessionEpoch, invalidatingcanvasToolsProvenEpoch, so a session that hadproven its canvas tools by executing them is re-asked and the user's next message is
silently prefixed with the disclosure paragraph.
Evidence:
sendHello's.then((sent) => {…})bumps the generation at 19472(
agentSessionEpoch++;, guarded on 19471 only byif (sent)) — so it fires onevery landed hello, with no notion of whether that hello was a first advertise or
a re-advertise of a session that already exists.
canvasToolsProvenEpochis only ever stamped to the then-current epoch (28203)and is read back against it (19686), so the bump strands the proof.
The remedy named in review is a re-advertise that does not carry the hello's session
semantics — no
readyack, no epoch bump. That work is not in this branch, has nobranch of its own, and no PR is open for it. It removes the same exposure #310 already
carries, and it gates this one. The debounce here bounds how often these happen, never
whether; it must not be read as a mitigation.
Why this cannot be patched here. Both defects are properties of the hello path itself,
which this branch only calls. Fixing them from inside
shouldRehelloAfterComfyReconnector
createReconnectRehelloGatewould mean special-casing the ack handler and the epochbump for one caller while #310's post-
free_vramre-advertise keeps hitting bothunguarded — a second, divergent code path for the same event, which is a worse defect than
the one it papers over. The honest sequencing is: land step 1 (a session-semantics-free
re-advertise), then rebase this branch onto it, at which point this PR becomes a few lines
of gating.
Also unaddressed, from the first review and not re-raised in the second: the window has
no trailing edge, so a second genuine bounce inside the 5s gap is dropped rather than
deferred; and the gate is still constructed per call site, so the three sibling re-hello
paths can still double up on one physical event.
Verified
npm run test:unitin full (chains i18n, i18n-render, tool-vocabulary and panel-scopegates) on the merged tree: 4421 pass / 0 fail (4422 tests, 1 todo).
session-rebind.test.mjsalone is 78/78.npm run typecheckclean.origin/main(now through v0.14.41); no conflict markers inweb/orbrowser_tests/.Green is not readiness here. The suite passes because the code does what this branch
intends; the two blockers above are behaviours of the hello path that no test on this
branch asserts against, and that the previous review found by reading, not by running.
See the status note at the top.