diff --git a/browser_tests/unit/session-rebind.test.mjs b/browser_tests/unit/session-rebind.test.mjs index f13bb21f..ac3f46b0 100644 --- a/browser_tests/unit/session-rebind.test.mjs +++ b/browser_tests/unit/session-rebind.test.mjs @@ -4,10 +4,14 @@ import test from "node:test"; import assert from "node:assert/strict"; +// #1138 adds one structural test (the call site's polarity), which the rest of this +// pure-logic file does not need. +import { readFileSync } from "node:fs"; import { shouldResumeAfterComfyReconnect, shouldRehelloAfterCommand, + shouldNudgeAfterMidTaskReconnect, planSoftReloadRecovery, performSoftReloadRecovery, isTransientReconnectError, @@ -520,3 +524,102 @@ test("#419: N open tabs each independently reconnect — no subset left dead", a assert.equal(r.result.reconnect, true); } }); + +// ── #1138: a mid-task "you dropped" nudge needs a drop to have happened ────── +// +// The nudge injects a user message and writes to the durable transcript. Both are false, +// and the injection is harmful, unless the orchestrator really died: telling a working +// agent to resume makes it restart or duplicate what it is doing. +// +// The gap heuristic was right; 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, read as the strongest possible evidence of a real restart. So the guard +// was inverted exactly where it mattered: the better established that nothing dropped, the +// more confidently it nudged. Reachable on a live socket because `ready` repeats on one and +// #310 re-advertises after every successful free_vram. + +test("#1138: NEVER dropped must not nudge, however long the session has run", () => { + // The reported shape: a live socket, a re-advertise, a ready ack, and no drop anywhere. + // A real Date.now() against the 0 sentinel is ~56 years, which the old guard passed. + assert.equal( + shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: 0, now: Date.now() }), + false, + ); + for (const noDrop of [undefined, null, 0, -1, NaN, Infinity, "0", "1700000000000", {}]) { + assert.equal( + shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: noDrop, now: Date.now() }), + false, + `no recorded drop must never nudge: ${JSON.stringify(noDrop)}`, + ); + } + assert.equal(shouldNudgeAfterMidTaskReconnect(), false, "no arguments must not nudge"); +}); + +test("#1138: a REAL restart still nudges — the fix must not disable the feature", () => { + // The behaviour #278/#588 rely on: a long gap after an actual drop is a real restart. + const now = 1_700_000_000_000; + assert.equal( + shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now - 30_000, now }), + true, + ); + // Exactly at the boundary counts as real (>= minGapMs), so the threshold is not + // silently exclusive. + assert.equal( + shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now - 6000, now }), + true, + ); +}); + +test("#1138: a FAST reconnect after a real drop still does not nudge", () => { + // The pre-existing half of the rule, preserved: a sidebar remount or a brief WS blip + // means the orchestrator never died, so the turn kept running. + const now = 1_700_000_000_000; + assert.equal(shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now - 1, now }), false); + assert.equal(shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now - 5999, now }), false); + assert.equal(shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now, now }), false); +}); + +test("#1138: a NEGATIVE gap is not evidence of a long outage", () => { + // A wall-clock adjustment, or a drop stamped after this read, must not read as an + // ancient drop — the same class of mistake as the 0 sentinel, one step along. + const now = 1_700_000_000_000; + assert.equal(shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now + 60_000, now }), false); + assert.equal(shouldNudgeAfterMidTaskReconnect({ now, bridgeDroppedAt: now + 1 }), false); + // …and an unreadable clock decides nothing. + assert.equal(shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now - 30_000, now: NaN }), false); + assert.equal(shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now - 30_000, now: "later" }), false); +}); + +test("#1138: the guard is INVERSION-SENSITIVE, unlike a source scan", () => { + // #1096's review proved by mutation that a token-presence source scan stays green when + // the guard is inverted. This asserts the two sides disagree, so an inverted or dropped + // condition cannot pass: the never-dropped case and the real-restart case must differ. + const now = 1_700_000_000_000; + const neverDropped = shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: 0, now }); + const realRestart = shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: now - 30_000, now }); + assert.notEqual( + neverDropped, + realRestart, + "if these ever agree, the guard has stopped distinguishing no-drop from a real restart", + ); + assert.equal(neverDropped, false); + assert.equal(realRestart, true); +}); + +test("#1138 wiring: the call site NEGATES the predicate and returns", () => { + // The predicate is tested by behaviour above; this covers the one thing a pure test + // cannot see — that the call site consults it in the right POLARITY. Dropping the + // negation would invert the fix into "nudge only when nothing dropped", which is worse + // than the original bug. An exact-substring claim on purpose: #1096's review proved by + // mutation that loose token-presence scans over this file assert nothing at all. + const src = readFileSync(new URL("../../web/js/comfyui-mcp-panel.js", import.meta.url), "utf8"); + assert.ok( + src.includes( + "if (!shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: lastBridgeDownAt, now: Date.now() }))", + ), + "the ready-ack mid-task branch must NEGATE the predicate and read the live drop stamp", + ); + // The reverted sessionStorage twin must not return without its own review: every + // confirmed leak on this branch came from persisting that timestamp. + assert.doesNotMatch(src, /BRIDGE_DOWN_AT_KEY/, "the persisted drop timestamp stays reverted"); +}); diff --git a/web/js/comfyui-mcp-panel.js b/web/js/comfyui-mcp-panel.js index 9aa1ae25..ee7c20c0 100644 --- a/web/js/comfyui-mcp-panel.js +++ b/web/js/comfyui-mcp-panel.js @@ -453,6 +453,7 @@ import { import { shouldResumeAfterComfyReconnect, shouldRehelloAfterCommand, + shouldNudgeAfterMidTaskReconnect, performSoftReloadRecovery, retryDuringReconnect, dedupeWorkflowTabRecords, @@ -27249,7 +27250,28 @@ function buildPanel() { // the agent's turn kept running — so a "you dropped" nudge is false AND // would inject a spurious turn into a live session. A real ComfyUI restart // takes many seconds to come back, so a long gap since the drop = real. - if (Date.now() - lastBridgeDownAt < 6000) return; + // + // #1138 — AND the bridge must actually have dropped. `lastBridgeDownAt` is 0 + // until the bridge socket closes, and 0 does not mean "no drop" to the + // subtraction below: `Date.now() - 0` is ~56 years, the LONGEST possible gap, + // which this heuristic reads as the most certain evidence of a real restart. + // So the guard was exactly inverted in the case it exists to catch — the + // better established it is that nothing dropped, the more confidently it + // nudged. The intent above was right from the start; only the sentinel's + // arithmetic betrayed it. + // + // Reachable on a LIVE socket because `ready` repeats on one (see the client's + // own note): a re-advertise draws a fresh handshake, and #310 re-advertises + // after every successful free_vram by design. So a user who freed VRAM + // mid-task could be told their connection had dropped — and the agent told to + // resume work it was still doing — with no drop anywhere in the session. + // + // The decision moved into session-rebind.js so it is unit-tested by BEHAVIOUR. + // A source-scan over this file could only assert the guard's tokens are present, + // and #1096's review demonstrated by mutation that such a test stays green when + // the guard is inverted — which is the one regression that matters here. + if (!shouldNudgeAfterMidTaskReconnect({ bridgeDroppedAt: lastBridgeDownAt, now: Date.now() })) + return; appendSystem(tr("panel.reconnected_picking_up_where_we_left_off", "Reconnected — picking up where we left off.")); showThinking(); client.sendUserMessage( diff --git a/web/js/lib/session-rebind.js b/web/js/lib/session-rebind.js index 8a654afb..ae85ff8d 100644 --- a/web/js/lib/session-rebind.js +++ b/web/js/lib/session-rebind.js @@ -37,6 +37,48 @@ export function shouldRehelloAfterCommand(cmd, reply) { return cmd === "free_vram" && Boolean(reply && reply.ok); } +// #1138 — may a `ready` ack arriving mid-task nudge the agent to resume? +// +// The nudge injects a user message ("your connection dropped mid-task … continue exactly +// what you were doing") and writes a line into the durable transcript. Both are false, and +// the injection is actively harmful, unless the orchestrator really did die: a turn that +// kept running does not need resuming, and telling a working agent to resume makes it +// restart or duplicate what it is doing. +// +// The panel's existing rule is a gap heuristic — a real ComfyUI restart takes many seconds +// to come back, so a LONG gap since the drop means the restart was real, while a fast +// reconnect (a sidebar remount, a brief WS blip) means it was not. That rule is right. +// +// What it missed is that "no drop at all" is not the same as "a very old drop", and the +// subtraction cannot tell them apart: the panel's `lastBridgeDownAt` is 0 until the bridge +// socket closes, and `Date.now() - 0` is ~56 years — the LONGEST possible gap, which the +// heuristic reads as the strongest possible evidence of a real restart. So the guard was +// exactly inverted where it mattered most: the better established it was that nothing had +// dropped, the more confidently it nudged. +// +// That is reachable on a LIVE socket, because `ready` repeats on one — 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 had. +// +// `bridgeDroppedAt` is therefore required to be a positive timestamp: absent, zero or +// unreadable means no drop was recorded, and no drop means no nudge. +export function shouldNudgeAfterMidTaskReconnect({ + bridgeDroppedAt = 0, + now = 0, + minGapMs = 6000, +} = {}) { + if (typeof bridgeDroppedAt !== "number" || !Number.isFinite(bridgeDroppedAt)) return false; + // Not `> 0` by accident: a drop is stamped with Date.now(), so any real value is far + // above zero, and a non-positive one is the never-dropped sentinel or a corrupt clock. + if (bridgeDroppedAt <= 0) return false; + if (typeof now !== "number" || !Number.isFinite(now)) return false; + // A gap that reads NEGATIVE (a clock adjustment, or a drop stamped after this read) + // is not evidence of a long outage either — treat it as fast, i.e. no nudge. + const gap = now - bridgeDroppedAt; + if (gap < 0) return false; + return gap >= minGapMs; +} + // #332 — During the post-restart reconnect window a Manager-backed fetch (e.g. // panel_list_nodes) throws a bare transport error ("Failed to fetch") before any // HTTP status exists, so the usual 404/"unreachable" handling never fires. Match