fix(1096): a resume must not announce a tab the graph route cannot reach

browser_tests/unit/session-rebind.test.mjs

@@ -11,6 +11,8 @@ import { readFileSync } from "node:fs";
 import {
   shouldResumeAfterComfyReconnect,
   shouldRehelloAfterCommand,
+  shouldRehelloAfterComfyReconnect,
+  createReconnectRehelloGate,
   shouldNudgeAfterMidTaskReconnect,
   createBridgeOutageTracker,
   planSoftReloadRecovery,
@@ -938,3 +940,320 @@ test("#1145: a second outage measures itself, not the gap since the first", () =
   assert.equal(tracker.outageMs(), 1500, "the second outage is 1.5s, not 10 minutes");
   assert.equal(shouldNudgeAfterMidTaskReconnect({ outageMs: tracker.outageMs() }), false);
 });
+
+test("#1096: re-advertise only when the bridge SURVIVED and a session exists", () => {
+  // Both parts are load-bearing, for different reasons.
+  assert.equal(
+    shouldRehelloAfterComfyReconnect({ bridgeConnected: true, hasResumableSession: true }),
+    true,
+  );
+  // bridge DOWN: shouldResumeAfterComfyReconnect takes it and connectAgent() hellos on
+  // its own. Re-advertising too would queue a second orchestrator panel sync for nothing.
+  assert.equal(
+    shouldRehelloAfterComfyReconnect({ bridgeConnected: false, hasResumableSession: true }),
+    false,
+  );
+  // NO SESSION: sendHello's own header records that a hello with the session key cleared
+  // "spawns a clean agent outright". Re-advertising here would turn a ComfyUI blip into a
+  // spurious agent start — the class #278 removed — and would buy nothing, because with
+  // no session there is no agent for a graph tool to route to.
+  assert.equal(
+    shouldRehelloAfterComfyReconnect({ bridgeConnected: true, hasResumableSession: false }),
+    false,
+  );
+  assert.equal(shouldRehelloAfterComfyReconnect({}), false);
+  assert.equal(shouldRehelloAfterComfyReconnect(), false, "no arguments must not act");
+});
+
+test("#1096: neither input may be satisfied by a truthy non-boolean", () => {
+  // Compared against `true` explicitly on both axes: an unreadable connection state is
+  // not a live bridge, and an unreadable session state is not a session worth spawning
+  // or rebinding an agent for.
+  for (const stray of [1, "true", {}, [], "yes"]) {
+    assert.equal(
+      shouldRehelloAfterComfyReconnect({ bridgeConnected: stray, hasResumableSession: true }),
+      false,
+      `non-true bridge state must not act: ${JSON.stringify(stray)}`,
+    );
+    assert.equal(
+      shouldRehelloAfterComfyReconnect({ bridgeConnected: true, hasResumableSession: stray }),
+      false,
+      `non-true session state must not act: ${JSON.stringify(stray)}`,
+    );
+  }
+});
+
+test("#1096: the two reconnect paths never both act, and never both decline with a session", () => {
+  // The gap this fix closes was precisely "both declined". With a resumable session,
+  // exactly one path must act on either side of the bridge axis.
+  for (const bridgeConnected of [true, false]) {
+    const resumes = shouldResumeAfterComfyReconnect({
+      bridgeConnected,
+      rebootPending: false,
+      autoConnect: false,
+      hasResumableSession: true,
+    });
+    const rehellos = shouldRehelloAfterComfyReconnect({ bridgeConnected, hasResumableSession: true });
+    assert.notEqual(
+      resumes,
+      rehellos,
+      `exactly one path must act (bridgeConnected=${bridgeConnected})`,
+    );
+  }
+  // And with NO session both correctly decline — nothing to preserve, nothing to route.
+  assert.equal(
+    shouldResumeAfterComfyReconnect({
+      bridgeConnected: true,
+      rebootPending: false,
+      autoConnect: false,
+      hasResumableSession: false,
+    }),
+    false,
+  );
+  assert.equal(
+    shouldRehelloAfterComfyReconnect({ bridgeConnected: true, hasResumableSession: false }),
+    false,
+  );
+});
+
+// ---- #1096 the re-advertise GATE, tested by behaviour --------------------------
+//
+// The debounce and the send ordering used to live inline in the panel, where the only
+// available test was a source scan — and mutation testing proved that scan stayed green
+// with the debounce inverted and the send relocated. These exercise the real unit, so an
+// inverted window or a stamp moved ahead of the send fails an assertion instead.
+
+/** A gate on a hand-cranked clock, plus a send recorder. */
+function gateHarness({ minGapMs = 5000, clock = { t: 0 } } = {}) {
+  const sends = [];
+  const gate = createReconnectRehelloGate({ now: () => clock.t, minGapMs });
+  const send = (outcome) => () => {
+    sends.push(clock.t);
+    if (typeof outcome === "function") return outcome();
+    return outcome;
+  };
+  return { gate, sends, send, clock };
+}
+
+test("#1096 gate: a socket that is not up is never announced to", async () => {
+  // THE NEGATIVE CASE. The whole failure class here is announcing a tab the route cannot
+  // serve, so "bridge down ⇒ no frame left the panel" is the assertion that matters, and
+  // it is asserted on the SEND being absent, not on a returned boolean.
+  const h = gateHarness();
+  assert.equal(
+    await h.gate.attempt({ bridgeConnected: false, hasResumableSession: true }, h.send(true)),
+    false,
+  );
+  assert.deepEqual(h.sends, [], "a down bridge must not put a hello on the wire");
+  // …and an unreadable connection state is not an up one.
+  for (const stray of [1, "true", {}, [], null, undefined]) {
+    assert.equal(
+      await h.gate.attempt({ bridgeConnected: stray, hasResumableSession: true }, h.send(true)),
+      false,
+    );
+  }
+  assert.deepEqual(h.sends, [], "only an observed-true socket may be announced to");
+});
+
+test("#1096 gate: no session means no re-advertise", async () => {
+  // A hello with the session key cleared spawns a clean agent (sendHello's own contract),
+  // so this is the second half of the negative case, on the other input.
+  const h = gateHarness();
+  assert.equal(
+    await h.gate.attempt({ bridgeConnected: true, hasResumableSession: false }, h.send(true)),
+    false,
+  );
+  assert.equal(await h.gate.attempt({ bridgeConnected: true }, h.send(true)), false);
+  assert.equal(await h.gate.attempt({}, h.send(true)), false);
+  assert.equal(await h.gate.attempt(undefined, h.send(true)), false);
+  assert.deepEqual(h.sends, [], "no session ⇒ nothing announced");
+});
+
+test("#1096 gate: the uncovered case DOES re-advertise, exactly once per window", async () => {
+  const h = gateHarness({ minGapMs: 5000 });
+  const live = { bridgeConnected: true, hasResumableSession: true };
+  assert.equal(await h.gate.attempt(live, h.send(true)), true);
+  assert.deepEqual(h.sends, [0], "the uncovered case must actually re-advertise");
+  // Inside the window: suppressed. A hello runs the orchestrator's panel sync (~1s, and
+  // it blocks on the panel operation lock), so a flapping socket must not queue one per
+  // blip. Asserted on the send list — this is what the source scan could not see.
+  h.clock.t = 4999;
+  assert.equal(await h.gate.attempt(live, h.send(true)), false);
+  assert.deepEqual(h.sends, [0], "a second blip inside the window must not re-advertise");
+  // Window expired: it acts again.
+  h.clock.t = 5000;
+  assert.equal(await h.gate.attempt(live, h.send(true)), true);
+  assert.deepEqual(h.sends, [0, 5000]);
+});
+
+test("#1096 gate: the window is INVERSION-SENSITIVE", async () => {
+  // The two sides must disagree, so a flipped comparison cannot pass: inside the window
+  // suppresses, outside it acts. A token-presence scan sees these as identical.
+  const inside = gateHarness({ minGapMs: 5000 });
+  const outside = gateHarness({ minGapMs: 5000 });
+  const live = { bridgeConnected: true, hasResumableSession: true };
+  for (const h of [inside, outside]) await h.gate.attempt(live, h.send(true));
+  inside.clock.t = 100;
+  outside.clock.t = 60_000;
+  const suppressed = await inside.gate.attempt(live, inside.send(true));
+  const allowed = await outside.gate.attempt(live, outside.send(true));
+  assert.notEqual(
+    suppressed,
+    allowed,
+    "if these ever agree, the window has stopped distinguishing a blip from a real gap",
+  );
+  assert.equal(suppressed, false);
+  assert.equal(allowed, true);
+});
+
+test("#1096 gate: a hello that did NOT land leaves the window open", async () => {
+  // The "recorded before it happened" defect, asserted where it is observable. A hello
+  // refused for want of a route identity, or dropped on a superseded socket, advertised
+  // nothing — so it must not spend the window and leave the mapping this repairs dropped
+  // for the full gap. Arming ahead of the send is exactly how this used to be wrong.
+  const h = gateHarness({ minGapMs: 5000 });
+  const live = { bridgeConnected: true, hasResumableSession: true };
+  assert.equal(await h.gate.attempt(live, h.send(false)), false);
+  assert.deepEqual(h.sends, [0], "it tried");
+  h.clock.t = 1; // still deep inside the window
+  assert.equal(await h.gate.attempt(live, h.send(true)), true, "…so the very next event may retry");
+  assert.deepEqual(h.sends, [0, 1]);
+  // And only NOW is the window armed, by the send that actually landed.
+  h.clock.t = 2;
+  assert.equal(await h.gate.attempt(live, h.send(true)), false);
+  assert.deepEqual(h.sends, [0, 1]);
+});
+
+test("#1096 gate: only a literal true counts as landed", async () => {
+  // sendHello resolves the boolean "did this reach the wire". A truthy stand-in (a frame
+  // object, a 1) is not that proof, and treating it as one would arm the window on a
+  // hello nobody observed arriving.
+  for (const truthy of [1, "sent", {}, []]) {
+    const h = gateHarness({ minGapMs: 5000 });
+    const live = { bridgeConnected: true, hasResumableSession: true };
+    assert.equal(await h.gate.attempt(live, h.send(truthy)), false);
+    h.clock.t = 1;
+    assert.equal(await h.gate.attempt(live, h.send(true)), true, `${JSON.stringify(truthy)}`);
+  }
+});
+
+test("#1096 gate: a throwing or rejecting re-advertise is non-fatal and arms nothing", async () => {
+  const h = gateHarness({ minGapMs: 5000 });
+  const live = { bridgeConnected: true, hasResumableSession: true };
+  assert.equal(
+    await h.gate.attempt(live, h.send(() => { throw new Error("socket gone"); })),
+    false,
+  );
+  assert.equal(
+    await h.gate.attempt(live, h.send(() => Promise.reject(new Error("superseded")))),
+    false,
+  );
+  assert.deepEqual(h.sends, [0, 0]);
+  // Neither armed the window: the next event still gets to try.
+  assert.equal(await h.gate.attempt(live, h.send(true)), true);
+  // A missing client method resolves undefined — the same "nothing landed" case, and it
+  // must not throw out of the gate.
+  const h2 = gateHarness();
+  assert.equal(
+    await h2.gate.attempt(
+      { bridgeConnected: true, hasResumableSession: true },
+      h2.send(undefined),
+    ),
+    false,
+  );
+});
+
+test("#1096 gate: only one re-advertise is in flight at a time", async () => {
+  // The window alone is no protection during the interval that matters most: the first
+  // hello is still awaiting its identity resolve, which can outlast the gap on a busy
+  // install, and every further `reconnected` would stack another panel sync behind it.
+  let release;
+  const blocked = new Promise((r) => { release = r; });
+  const h = gateHarness({ minGapMs: 5000 });
+  const live = { bridgeConnected: true, hasResumableSession: true };
+  const first = h.gate.attempt(live, h.send(() => blocked));
+  h.clock.t = 999_999; // window wide open — only the latch can refuse this
+  assert.equal(await h.gate.attempt(live, h.send(true)), false, "a second send must not overlap");
+  assert.deepEqual(h.sends, [0], "exactly one hello on the wire");
+  release(true);
+  assert.equal(await first, true);
+  // …and the latch releases, so later events are governed by the window alone.
+  h.clock.t = 999_999 + 5000;
+  assert.equal(await h.gate.attempt(live, h.send(true)), true);
+});
+
+test("#1096 gate: an unreadable clock decides nothing", async () => {
+  for (const bad of [NaN, Infinity, -Infinity, "0", null, undefined]) {
+    const sends = [];
+    const gate = createReconnectRehelloGate({ now: () => bad });
+    assert.equal(
+      await gate.attempt(
+        { bridgeConnected: true, hasResumableSession: true },
+        () => { sends.push(1); return true; },
+      ),
+      false,
+      `an untimeable window must not be entered: ${String(bad)}`,
+    );
+    assert.deepEqual(sends, []);
+  }
+});
+
+test("#1096 wiring: the call site consults the gate, in the right polarity", () => {
+  // The gate's behaviour is covered above; this covers the two things a pure test cannot
+  // see — that the panel actually reaches it, and with which inputs. Exact substrings on
+  // purpose: #1096's own review proved by mutation that loose token scans over this file
+  // assert nothing at all.
+  const src = readFileSync(
+    new URL("../../web/js/comfyui-mcp-panel.js", import.meta.url),
+    "utf8",
+  );
+  const at = src.indexOf("reconnectRehelloGate.attempt(");
+  assert.notEqual(at, -1, "the reconnect handler must reach the gate");
+  // POLARITY. `void gate.attempt(…)` DISPATCHES it; `if (!gate.attempt(…))` or any other
+  // negated wrapper would read as wired while doing the opposite, and that is invisible
+  // to a token scan. Pinned on the bytes IMMEDIATELY BEFORE the call rather than on a
+  // line-shaped substring, so it survives reformatting and CRLF but still fails the
+  // moment a `!` or a condition is put in front of it.
+  assert.equal(
+    src.indexOf("void reconnectRehelloGate.attempt("),
+    at - "void ".length,
+    "the re-advertise must be dispatched, not negated or folded into a condition",
+  );
+  // It sits in the branch that used to `return` having done nothing…
+  const guardAt = src.indexOf("!shouldResumeAfterComfyReconnect({");
+  assert.ok(guardAt !== -1 && guardAt < at, "it must be inside the declined-resume branch");
+  // …bounded by that branch's OWN `return`, not by connectAgent(): the resume path which
+  // follows legitimately announces itself, and slicing that far would assert about it.
+  const block = src.slice(at, src.indexOf("return;", at));
+  // The live bridge state is OBSERVED at the moment of the attempt, never a cached flag —
+  // announcing a tab on a socket that was up a moment ago is the failure this fixes.
+  assert.ok(
+    block.includes("bridgeConnected: client.isConnected(),"),
+    "the live bridge state must be read from the client at the call site",
+  );
+  // The session input must ask the SAME question the frame does. `hasResumableSession`
+  // one screen up is derived from the active thread RECORD; the hello carries `resume`
+  // from SESSION_KEY. Gating on the record lets the guard pass while the frame goes out
+  // with `resume: null`, which spawns a clean agent — the spurious start this input
+  // exists to prevent. Pinned exactly, because "hasResumableSession is mentioned" was
+  // true of the wrong version too.
+  assert.ok(
+    block.includes("hasResumableSession: Boolean(ssGet(SESSION_KEY)),"),
+    "the session input must be read from the source the hello itself will carry",
+  );
+  // It re-advertises and nothing more — no connect, no session frame, no chat line.
+  // Bouncing a live session here is exactly what #278 removed.
+  assert.ok(block.includes("() => client?.rehello?.(),"), "the send must be the re-advertise");
+  assert.doesNotMatch(block, /connectAgent\(/, "it must not reconnect");
+  assert.doesNotMatch(block, /resume_session/, "it must not re-resume the session");
+  assert.doesNotMatch(block, /appendSystem\(/, "a silent repair needs no chat line");
+  // MONOTONIC, decided at the construction site and nowhere else — the gate's own default
+  // is Date.now() so it stays usable standalone, exactly as the outage tracker's is. A
+  // wall clock here lets an NTP/DST correction re-arm the window early.
+  assert.ok(
+    src.includes("createReconnectRehelloGate({ now: monotonicNow })"),
+    "the panel's re-advertise gate must measure on the monotonic clock",
+  );
+  // The inline debounce this replaced must not come back: it was unreachable from a test.
+  assert.doesNotMatch(src, /lastReconnectRehelloAt/, "the untestable inline window stays gone");
+});

web/js/comfyui-mcp-panel.js

@@ -504,6 +504,7 @@ import {
   shouldRehelloAfterCommand,
   shouldNudgeAfterMidTaskReconnect,
   createBridgeOutageTracker,
+  createReconnectRehelloGate,
   performSoftReloadRecovery,
   retryDuringReconnect,
   dedupeWorkflowTabRecords,
@@ -944,6 +945,15 @@ function monotonicNow() {
     ? performance.now()
     : Date.now();
 }
+// #1096 — the rate limit for the re-advertise on a bridge-survived ComfyUI reconnect. A
+// hello is not free: the orchestrator runs its panel sync on each one (~1s measured, and
+// the step that blocks on the panel operation lock), and `reconnected` can fire repeatedly
+// while ComfyUI's socket flaps. The gate itself lives in session-rebind.js so its debounce
+// and its arm-only-when-landed ordering are covered by BEHAVIOURAL tests — a source scan
+// over this file stayed green when either was mutated. What is decided HERE, and only
+// here, is the clock: MONOTONIC, so a wall-clock adjustment cannot make the gap look
+// negative and re-arm the window early.
+const reconnectRehelloGate = createReconnectRehelloGate({ now: monotonicNow });
 // The actual (idempotent) node-def registration; the coalescer owns the in-flight
 // slot lifecycle, so this MUST NOT clear it. Reuses a caller-supplied /object_info
 // payload when present (graph_add_node passes the fresh defs it just validated
@@ -30153,6 +30163,37 @@ function buildPanel() {
         hasResumableSession,
       })
     ) {
+      // #1096 — NOT nothing. Returning here is right about the SESSION (bouncing a live
+      // one is #278's spurious "you reconnected") and was wrong about the TAB MAPPING:
+      // ComfyUI just bounced, which drops the orchestrator's mapping for this tab (the
+      // mechanism #310 measured), and nothing on this path re-advertised it. The resumed
+      // session stayed on screen while every graph tool answered "Connected: none".
+      //
+      // A rehello re-targets the EXISTING socket and the orchestrator carries the routing
+      // state across it (#884), so this repairs the mapping without touching the session.
+      //
+      // RATE LIMITED, because a hello is not free: the orchestrator runs its panel sync
+      // on each one (~1s measured, and the step that blocks on the panel operation lock —
+      // a stale lock made it 60s per hello). `reconnected` can fire repeatedly during a
+      // flapping window, and re-advertising per blip would queue that work behind itself.
+      // The gate owns the window, the in-flight latch and the arm-only-when-the-hello-
+      // LANDED ordering; all three are pinned by behavioural tests in session-rebind.
+      //
+      // The session input is read from SESSION_KEY — the SAME source the frame will
+      // actually carry (`getResume`) — and deliberately NOT from `hasResumableSession`
+      // above, which is derived from the active thread RECORD. The two can disagree: a
+      // thread record can hold a session id while SESSION_KEY is empty, and then the
+      // guard would pass while the hello goes out with `resume: null`, which by
+      // sendHello's own contract SPAWNS A CLEAN AGENT. That is the spurious agent start
+      // this input exists to prevent, arriving because the guard asked a different
+      // question than the frame does. Ask the frame's question.
+      void reconnectRehelloGate.attempt(
+        {
+          bridgeConnected: client.isConnected(),
+          hasResumableSession: Boolean(ssGet(SESSION_KEY)),
+        },
+        () => client?.rehello?.(),
+      );
       return;
     }
     appendSystem(tr("panel.comfyui_is_back_reconnecting_the_agent", "ComfyUI is back — reconnecting the agent…"));