fix(1138): a live-socket re-advertise must not read as a fresh connect

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");
+});

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(

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