fix(1138): a live-socket re-advertise must not read as a fresh connect - #1139
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.
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
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
… 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
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
Review returned 22 verified findings — and most of them are already revertedImportant framing: the run reviewed the branch including the persisted-timestamp commit (5dadb04), which I reverted before it finished. Its own synthesis says the findings are "dominated by one" defect — the persistence having a single consume site — and that is the commit that is gone. So the majority describe code no longer on the branch. What the review found there is still worth recording, because it is a much sharper account of why that commit was wrong than my revert message managed:
Both confirm the revert was right, and for a stronger reason than I gave: the persistence did not merely add uncleaned exits, it broke its own stated invariant on six of seven paths. What survives against the branch as it now standsOne CONFIRMED defect, and it is PRE-EXISTING — not introduced here. Every FAILED reconnect attempt re-stamps My predicate inherits that faithfully — it does not cause it and does not worsen it, and the guard is strictly better than the sentinel bug it replaces. But it means the long-gap heuristic is measuring the wrong interval in a whole class of real restarts, which deserves its own issue rather than a patch smuggled in here. A false claim in my reverted comment, already corrected: Test coverage for the panel-side wiring: addressed by the polarity test just pushed, which asserts the whole guard line rather than its tokens. Where this leaves #1139Ready for merge as one substantive commit plus its tests: require that the bridge actually dropped, with a behaviourally-tested predicate and a structural test for the call site's polarity. 3976 tests pass, typecheck clean. Not re-running the review for the reverted state: the revert strictly REMOVES the code the findings target, and the one live finding is pre-existing and documented above rather than fixed here. |
Closes #1138. Unblocks #1096; fixes an exposure that exists on main today via #310.
A re-advertise is answered with a full handshake including a
readyack — the panel's own comment says'ready' repeats on a live socket. On a socket that never dropped, that drives three behaviours which only make sense for a fresh connection: the mid-task resume injection, the re-greeting #588 banned, and theagentSessionEpochbump that throws away a working agent's canvas-tool proof.The mid-task one is the sharp edge. Its only bridge-survived guard is
Date.now() - lastBridgeDownAt < 6000, andlastBridgeDownAtis only stamped when the bridge socket closes — so on a surviving bridge it is still 0 and the guard cannot fire. A user who runsfree_vrammid-task (which #310 correctly re-advertises after) can therefore have✅ Your connection dropped mid-task…continue exactly what you were doinginjected into an agent that is still working, and read a claim about a drop that never happened.Found while reviewing #1134, where I tried to use a re-advertise for #1096's dropped tab mapping and would have widened this from one trigger to every ComfyUI reconnect. Withdrawing that and fixing this first is the right order: the mapping repair needs a re-advertise, so the re-advertise has to be safe on a live session before anything else can use it.
What I intend to establish first
That the same session really does survive a re-advertise, rather than assuming it. The orchestrator logs
same-socket re-hello … routing state carried; the shared session continues(#884), which is why skipping the epoch bump looks correct — the agent the proof was taken against is the one still running. But #291 documents the opposite case: a hello with the session key cleared spawns a clean agent, and then the proof MUST reset. So the condition is not "is this a re-advertise" but "does this hello carry the same session id it was bound to", and I want that read off the payload rather than inferred from the call site.Whether the ready ack can be attributed reliably. Suppressing the mid-task branch needs to distinguish an ack answering this re-advertise from one answering a genuine reconnect that raced it. If that cannot be done from what the panel holds, the honest fix is narrower — gate on the bridge never having dropped rather than on which hello it answers.
Draft while I read onAck's ready branch and the hello payload.