From 1697f360ad229434e89bb0b23cae4bee1f1fc551 Mon Sep 17 00:00:00 2001 From: Artokun Date: Wed, 12 Aug 2026 22:44:08 -0700 Subject: [PATCH 1/7] =?UTF-8?q?chore(1161):=20claim=20=E2=80=94=20graph=5F?= =?UTF-8?q?set=5Fwidget=20must=20not=20hang=20on=20a=20metadata=20await=20?= =?UTF-8?q?after=20a=20restart?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 (1M context) From 90990a397528f51c49ec0031b9628f8d17fbceaf Mon Sep 17 00:00:00 2001 From: Artokun Date: Wed, 12 Aug 2026 22:54:33 -0700 Subject: [PATCH 2/7] fix(1161): bound the object-info read, and retire the request it abandons The only open P1. After a ComfyUI restart, graph_set_widget stopped replying -- 30s timeouts on every node and every widget -- while every other command on the same tab answered instantly, including graph_edit_node mutating the same node. graph_set_widget is the one executor that consults /object_info before writing (added by #718 so the workflow-instance stamp can be re-checked after the await), and objectInfoCache.read awaited that fetch UNBOUNDED. A restart can leave the tab holding a half-open connection, so getNodeDefs() never settles and the call parks forever. The 30s in the report is the caller's timeout; nothing panel-side ever fired. Coalescing is what made it permanent instead of transient. A hung request stays in the `inflight` slot, and `if (inflight && inflightGeneration === generation) return inflight` means every later read JOINS that same dead promise rather than issuing its own. Nothing settles it, so nothing clears it, and the command is broken for the rest of the session. That accounts for the detail that makes the report look strange -- every node, every widget, forever, while the bridge and the binding are provably healthy. Both halves are needed and both are pinned by mutation: - BOUND the wait. Removing it makes the never-settles test hang forever, which is the bug. - RETIRE the abandoned request. Bounding alone leaves the dead promise joinable, so the next read bounds out too -- a permanent 8s tax in place of a permanent hang. That mutant hangs as well, on the "next read does not join it" test. The request is NOT cancelled: a late response still runs the generation check and still populates the cache, so recovery costs one refused call rather than a tab reload. That is the startup seed's stated contract -- "giving up on the WAIT is the absence of evidence, so it must not latch" -- and this is the same decision about the same endpoint, which is why it uses the same 8s. Placed at the MECHANISM, not the call site, so every caller is covered by construction and the panel needs no change. #1095's review is the reason that is explicit: gating one caller while its siblings stay open can be worse than gating none. A timeout arm returns no payload, so the caller's fence fails closed and refuses that one call exactly as it does for a fetch that failed outright. A real rejection still propagates unchanged -- only the timeout arm is sentinel-wrapped, or the fence would read "the fetch failed" as "I did not wait long enough". 4102 unit tests pass (5 new); typecheck and node --check clean. Closes #1161. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 16 ++++ browser_tests/unit/object-info-cache.test.mjs | 94 +++++++++++++++++++ web/js/lib/object-info-cache.js | 70 +++++++++++++- 3 files changed, 178 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d42bd124..31ccb3f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,22 @@ All notable changes to this project are documented here. This project adheres to ## [Unreleased] +### Fixed + +- **Setting a widget no longer hangs for 30 seconds after a ComfyUI restart (#1161).** + Once ComfyUI had been restarted mid-session, setting any widget on any node timed out, + every time, while every other panel command answered instantly — reading the graph, + renaming a node, listing workflows, queueing a run. Setting a widget is the one action + that checks the backend's node definitions before it writes, and that check could wait + forever: a restart can leave the browser holding a connection that never answers and + never fails. + It stayed broken rather than recovering because callers share one in-flight request, so + every later attempt waited on the same dead one. The check now gives up after a few + seconds and refuses that single call with a retry hint, and the abandoned request is + discarded so the next attempt makes a fresh one — which succeeds as soon as the backend + is reachable. A slow answer that arrives late is still used, so recovery costs one + refused call rather than reloading the tab. + ## [0.14.24] - 2026-08-12 ### Fixed diff --git a/browser_tests/unit/object-info-cache.test.mjs b/browser_tests/unit/object-info-cache.test.mjs index 7020e9f1..545762c9 100644 --- a/browser_tests/unit/object-info-cache.test.mjs +++ b/browser_tests/unit/object-info-cache.test.mjs @@ -237,3 +237,97 @@ test("#716: a retired request cannot overwrite a newer value — deterministical "the retired response must not have replaced the newer value", ); }); + +// ── #1161: a fetch that never settles must not park the tool forever ───────── +// +// The only open P1. After a ComfyUI restart the tab can hold a half-open connection, so +// `getNodeDefs()` never settles. `graph_set_widget` is the one command that consults +// /object_info before writing, so it alone hung — 30s timeouts on every node while every +// other command answered instantly. The coalescing below made it PERMANENT rather than +// transient: the hung request stays in the inflight slot and every later read joins the +// same dead promise, so nothing ever clears it. +// +// A hand-driven timer, so these assert the behaviour rather than sleep for 8 seconds. +function boundedCache({ waitMs = 8000 } = {}) { + const timers = []; + const cache = createObjectInfoCache({ + waitMs, + setTimer: (fn, ms) => { + const t = { fn, ms }; + timers.push(t); + return t; + }, + clearTimer: (t) => { + const i = timers.indexOf(t); + if (i !== -1) timers.splice(i, 1); + }, + }); + return { cache, fireTimers: () => [...timers].forEach((t) => t.fn()) }; +} + +test("#1161: a fetch that never settles is bounded instead of parking the caller", async () => { + const { cache, fireTimers } = boundedCache(); + const read = cache.read(() => new Promise(() => {})); // never settles, like a half-open socket + fireTimers(); + assert.equal(await read, null, "the read gives up on THIS call rather than hanging"); +}); + +test("#1161: the abandoned request is retired, so the next read does not join it", async () => { + // This is the half that bounding alone would miss. If the dead promise stayed in the + // inflight slot, the next read would join it and bound out too — a permanent 8s tax in + // place of a permanent hang. The next read must issue its OWN request. + const { cache, fireTimers } = boundedCache(); + let calls = 0; + const hang = () => { + calls++; + return new Promise(() => {}); + }; + const first = cache.read(hang); + fireTimers(); + assert.equal(await first, null); + assert.equal(calls, 1); + + const defs = { KSampler: {} }; + const second = await cache.read(() => { + calls++; + return Promise.resolve(defs); + }); + assert.equal(calls, 2, "the second read must issue a fresh request, not join the dead one"); + assert.deepEqual(second, defs, "…and it recovers as soon as the backend answers"); +}); + +test("#1161: a healthy fetch is unaffected by the bound", async () => { + const { cache } = boundedCache(); + const defs = { KSampler: {} }; + assert.deepEqual(await cache.read(() => Promise.resolve(defs)), defs); +}); + +test("#1161: a rejection still reaches the caller unchanged", async () => { + // Only the timeout arm is sentinel-wrapped; a real failure must propagate as before, + // or the fence would read "the fetch failed" as "I did not wait long enough". + const { cache } = boundedCache(); + await assert.rejects(cache.read(() => Promise.reject(new Error("backend down"))), /backend down/); +}); + +test("#1161: a late response still populates the cache — giving up on the wait latches nothing", async () => { + // The startup seed's contract, which this must match: the request is not cancelled, so a + // response arriving after the bound still establishes the payload and the NEXT call + // succeeds. The recovery costs one refused call, not a tab reload. + const { cache, fireTimers } = boundedCache(); + const defs = { KSampler: {} }; + let release; + const slow = new Promise((resolve) => { release = resolve; }); + const first = cache.read(() => slow); + fireTimers(); + assert.equal(await first, null, "the bounded call refuses"); + release(defs); + await slow; + await new Promise((r) => setTimeout(r, 0)); // let the abandoned request settle + let refetched = 0; + const second = await cache.read(() => { + refetched++; + return Promise.resolve({ other: {} }); + }); + assert.equal(refetched, 0, "the late response seeded the cache, so no refetch was needed"); + assert.deepEqual(second, defs); +}); diff --git a/web/js/lib/object-info-cache.js b/web/js/lib/object-info-cache.js index 81b2023c..9955de0c 100644 --- a/web/js/lib/object-info-cache.js +++ b/web/js/lib/object-info-cache.js @@ -65,11 +65,41 @@ export const CACHE_OUTCOME = Symbol.for("comfyui-mcp.objectInfoOutcome"); /** How long a fetched payload may be reused. */ export const OBJECT_INFO_CACHE_TTL_MS = 1500; +/** + * #1161 — how long a read will WAIT for a fetch before giving up on THIS call. + * + * Unbounded was the P1: after a ComfyUI restart the tab can hold a half-open connection, + * so `getNodeDefs()` never settles. `graph_set_widget` is the only command that consults + * /object_info before writing, so it alone parked forever — every other command on the + * same tab answered instantly, which is exactly how the report reads. + * + * The coalescing below is what made it permanent rather than transient: a hung request + * stays in the `inflight` slot, so every later read JOINS the same dead promise instead + * of issuing its own. Nothing settles it, so nothing clears it, and the command is broken + * for the rest of the session. + * + * This is the same hazard the startup seed already bounds, in the same words: "a request + * that never settles (a hung/half-open connection) would otherwise block the awaiting + * tool FOREVER. Bounding the wait converts that hang into the correct outcome." Matching + * that 8s deliberately — the two waits are the same decision about the same endpoint, and + * a reader comparing them should not have to wonder why they differ. + */ +export const OBJECT_INFO_READ_WAIT_MS = 8000; + /** * @param {{ttlMs?: number, now?: () => number}} [opts] `now` is injectable so tests do not * depend on wall-clock timing, which is how a cache test becomes a flaky test. */ -export function createObjectInfoCache({ ttlMs = OBJECT_INFO_CACHE_TTL_MS, now = () => Date.now() } = {}) { +export function createObjectInfoCache({ + ttlMs = OBJECT_INFO_CACHE_TTL_MS, + now = () => Date.now(), + waitMs = OBJECT_INFO_READ_WAIT_MS, + // Injectable so the bound can be tested without a real 8s wait — the same reason `now` + // is injectable. A test that sleeps for the timeout is a slow test that eventually + // becomes a flaky one. + setTimer = (fn, ms) => setTimeout(fn, ms), + clearTimer = (t) => clearTimeout(t), +} = {}) { let value = null; let at = 0; let inflight = null; @@ -156,7 +186,43 @@ export function createObjectInfoCache({ ttlMs = OBJECT_INFO_CACHE_TTL_MS, now = inflight = request; inflightGeneration = issuedAt; inflightId = requestId; - return request; + if (!(waitMs > 0)) return request; + // #1161 — BOUND the wait, and retire the slot when it fires. + // + // Bounding alone would not fix the reported bug. The hung request would stay in + // `inflight`, so the next read would join it and bound out too: a permanent 8s tax + // in place of a permanent hang. Better, and still broken. Releasing the slot is what + // lets the NEXT call issue a fresh request — which is the one that recovers once the + // connection is healthy again. + // + // The request is NOT cancelled. A late response still runs the generation check and + // still populates the cache, so the recovery costs one refused call rather than a + // reload — the same contract the startup seed states: giving up on the WAIT is the + // absence of evidence, and must not latch anything. + let timer = null; + const timedOut = Symbol("object-info-read-timeout"); + const outcome = await Promise.race([ + // A rejection must reach the caller unchanged on the fast path, so it is not + // wrapped here; only the timeout arm needs a sentinel. + request, + new Promise((resolve) => { + timer = setTimer(() => resolve(timedOut), waitMs); + }), + ]).finally(() => { + if (timer !== null) clearTimer(timer); + }); + if (outcome !== timedOut) return outcome; + // Abandoned: keep the promise's rejection observed, or a fetch that fails after the + // bound becomes an unhandled rejection that this file did nothing about. + request.catch(() => {}); + if (inflightId === requestId) { + inflight = null; + inflightGeneration = -1; + inflightId = 0; + } + // No payload: the caller's fence fails closed and refuses THIS call with "retry in a + // moment", exactly as it does for a fetch that failed outright. + return null; }, /** From 8689f2bcb3e73c13fa409064237916ed46419db2 Mon Sep 17 00:00:00 2001 From: Artokun Date: Wed, 12 Aug 2026 23:24:35 -0700 Subject: [PATCH 3/7] fix(1161): bound every reader, retire by generation, and say why Review found the previous commit did not fix the P1 it claimed to, plus three further defects. Reworked rather than patched. THE HEADLINE MISS. read() returns the in-flight promise BEFORE the bound was installed, so only the call that ISSUED the fetch was bounded and every call that JOINED got the raw unbounded promise. Verified by the reviewers against the shipped module: joiners stayed pending with zero timers armed for them. #716 built this cache for BURSTS of widget writes, so after a restart the first call gave up at 8s while every overlapping call parked until the orchestrator's 30s timeout -- the reported symptom, intact, in the case that matters most. I tested only the single-reader path when the bug report is about repeated calls. The bound now lives where a promise is HANDED OUT, not where it is created, so issuer and joiner are covered by one path. RETIRE BY GENERATION. Releasing the `inflight` slot alone left the abandoned request still matching `issuedAt === generation`, so a late pre-restart payload could overwrite a NEWER schema and re-stamp its TTL -- authorizing a write against a pack uninstalled during the restart and reporting success, which is the #458 hole the fence exists to close. Two requests were never live in one generation before this bound existed. Advancing the generation is how this file already retires a request it no longer trusts. That deliberately gives up the "a late response still populates the cache" property the previous commit claimed. The two cannot both hold, and correctness wins: discarding a late payload costs a refetch, while letting a stale one win costs a fabricated success. The test that asserted the old behaviour was asserting the wrong thing and is inverted. SAY WHY. The timeout returned a bare `null`, and both call sites compute `outcome?.failures ?? []` -- so the refusal named no cause and cited a route list it never printed, telling the user to hand-check a backend that answers /object_info perfectly well. It now returns the #982 outcome wrapper carrying a failure that states the read was abandoned and the backend may be healthy. RE-CHECK THE TTL BEFORE REFUSING. An invalidation plus a successful refetch can land while an earlier read is still waiting. Refusing then told the user to reconnect ComfyUI at the exact moment the panel held a fresh authoritative schema. Smaller: the sentinel is module-scoped rather than allocated per read; a throwing setTimer degrades to no bound instead of rejecting a read whose fetch already succeeded; a throwing clearTimer cannot turn a successful read into a failure. TESTS. Rewritten so they cannot detect a bug by HANGING -- node --test has no default timeout, so the previous mutants wedged the suite instead of naming the broken invariant. Nothing awaits a promise that may not settle; every wait is resolved by firing an injected timer, and the clock is injected too, matching the rest of the file. New coverage for the joiner burst, the stale-overwrite, the mid-wait payload, the throwing timer and the timer cleanup. Mutation-checked, and every mutant now FAILS rather than hangs: unbounding the joiner, dropping the generation advance, and removing the TTL re-check each fail their own test. 4105 unit tests pass; typecheck and node --check clean. Co-Authored-By: Claude Opus 5 (1M context) --- browser_tests/unit/object-info-cache.test.mjs | 170 ++++++++++++------ web/js/lib/object-info-cache.js | 127 +++++++++---- 2 files changed, 201 insertions(+), 96 deletions(-) diff --git a/browser_tests/unit/object-info-cache.test.mjs b/browser_tests/unit/object-info-cache.test.mjs index 545762c9..a0080e99 100644 --- a/browser_tests/unit/object-info-cache.test.mjs +++ b/browser_tests/unit/object-info-cache.test.mjs @@ -8,7 +8,13 @@ // real clock is a slow test that eventually becomes a flaky one. import { test } from "node:test"; import assert from "node:assert/strict"; -import { OBJECT_INFO_CACHE_TTL_MS, createObjectInfoCache } from "../../web/js/lib/object-info-cache.js"; +import { + OBJECT_INFO_CACHE_TTL_MS, + // #1161 — the timeout arm returns the outcome WRAPPER so the refusal can state its + // cause; the tests assert that shape rather than a bare null. + CACHE_OUTCOME, + createObjectInfoCache, +} from "../../web/js/lib/object-info-cache.js"; const DEFS = { KSampler: {}, CLIPTextEncode: {} }; const clock = (start = 1000) => { @@ -238,96 +244,144 @@ test("#716: a retired request cannot overwrite a newer value — deterministical ); }); -// ── #1161: a fetch that never settles must not park the tool forever ───────── + +// ── #1161: a fetch that never settles must not park the tool ───────────────── // // The only open P1. After a ComfyUI restart the tab can hold a half-open connection, so -// `getNodeDefs()` never settles. `graph_set_widget` is the one command that consults +// getNodeDefs() never settles. graph_set_widget is the one command that consults // /object_info before writing, so it alone hung — 30s timeouts on every node while every -// other command answered instantly. The coalescing below made it PERMANENT rather than -// transient: the hung request stays in the inflight slot and every later read joins the -// same dead promise, so nothing ever clears it. +// other command answered instantly — and coalescing made it permanent, because every +// later read joined the same dead promise. // -// A hand-driven timer, so these assert the behaviour rather than sleep for 8 seconds. -function boundedCache({ waitMs = 8000 } = {}) { - const timers = []; +// These never await a promise that may not settle: a test that detects its bug by hanging +// `node --test` (which has no default timeout) reports a wedged suite instead of naming +// the broken invariant. Every wait here is resolved by firing the injected timer, and the +// clock is injected too, matching the rest of this file. +function boundedCache({ waitMs = 8000, ttlMs = OBJECT_INFO_CACHE_TTL_MS } = {}) { + const timers = new Set(); + const clock = { t: 1_000_000 }; const cache = createObjectInfoCache({ + ttlMs, waitMs, + now: () => clock.t, setTimer: (fn, ms) => { const t = { fn, ms }; - timers.push(t); + timers.add(t); return t; }, - clearTimer: (t) => { - const i = timers.indexOf(t); - if (i !== -1) timers.splice(i, 1); - }, + clearTimer: (t) => timers.delete(t), }); - return { cache, fireTimers: () => [...timers].forEach((t) => t.fn()) }; + return { + cache, + clock, + armed: () => timers.size, + fireTimers: () => [...timers].forEach((t) => { timers.delete(t); t.fn(); }), + }; } +const timedOut = (outcome) => + outcome && typeof outcome === "object" && outcome[CACHE_OUTCOME] === true && outcome.defs === null; + test("#1161: a fetch that never settles is bounded instead of parking the caller", async () => { const { cache, fireTimers } = boundedCache(); - const read = cache.read(() => new Promise(() => {})); // never settles, like a half-open socket + const read = cache.read(() => new Promise(() => {})); // half-open socket fireTimers(); - assert.equal(await read, null, "the read gives up on THIS call rather than hanging"); + const outcome = await read; + assert.ok(timedOut(outcome), "the read gives up on THIS call rather than hanging"); + // The refusal must say WHY: a bare null makes the caller's note misdiagnose a hung read + // as an unreachable backend, naming a route list it never printed. + assert.match(String(outcome.failures?.[0] ?? ""), /did not answer within 8000ms/); }); -test("#1161: the abandoned request is retired, so the next read does not join it", async () => { - // This is the half that bounding alone would miss. If the dead promise stayed in the - // inflight slot, the next read would join it and bound out too — a permanent 8s tax in - // place of a permanent hang. The next read must issue its OWN request. +test("#1161: a JOINER is bounded too — the burst case the bug was reported on", async () => { + // The defect the first attempt shipped: read() returned the in-flight promise BEFORE the + // bound was installed, so only the issuing call was bounded and every overlapping call + // parked until the orchestrator's 30s timeout. #716 built this cache for bursts, so that + // is the case that matters most. + const { cache, fireTimers, armed } = boundedCache(); + const issuer = cache.read(() => new Promise(() => {})); + const joinerA = cache.read(() => new Promise(() => {})); + const joinerB = cache.read(() => new Promise(() => {})); + assert.equal(armed(), 3, "every caller must arm its own bound, not just the issuer"); + fireTimers(); + for (const [name, p] of [["issuer", issuer], ["joinerA", joinerA], ["joinerB", joinerB]]) { + assert.ok(timedOut(await p), `${name} must give up rather than park`); + } +}); + +test("#1161: the abandoned request is retired, so the next read issues a fresh one", async () => { const { cache, fireTimers } = boundedCache(); let calls = 0; - const hang = () => { - calls++; - return new Promise(() => {}); - }; - const first = cache.read(hang); + const first = cache.read(() => { calls++; return new Promise(() => {}); }); fireTimers(); - assert.equal(await first, null); + assert.ok(timedOut(await first)); assert.equal(calls, 1); - const defs = { KSampler: {} }; - const second = await cache.read(() => { - calls++; - return Promise.resolve(defs); - }); - assert.equal(calls, 2, "the second read must issue a fresh request, not join the dead one"); + const second = await cache.read(() => { calls++; return Promise.resolve(defs); }); + assert.equal(calls, 2, "the next read must not join the dead request"); assert.deepEqual(second, defs, "…and it recovers as soon as the backend answers"); }); -test("#1161: a healthy fetch is unaffected by the bound", async () => { - const { cache } = boundedCache(); +test("#1161: an abandoned request cannot overwrite a NEWER schema", async () => { + // The generation must advance when the slot is abandoned. Releasing it alone left the + // abandoned request still matching `issuedAt === generation`, so a late pre-restart + // payload could overwrite a newer one and re-stamp its TTL — a write authorized against + // a pack that was uninstalled during the restart, reported as success (#458). + const { cache, clock, fireTimers } = boundedCache(); + let releaseStale; + const stale = new Promise((resolve) => { releaseStale = resolve; }); + const first = cache.read(() => stale); + fireTimers(); + assert.ok(timedOut(await first)); + + const fresh = { KSampler: {} }; + assert.deepEqual(await cache.read(() => Promise.resolve(fresh)), fresh); + + releaseStale({ GoneNode: {}, KSampler: {} }); // the pre-restart map lands late + await stale; + await new Promise((r) => setTimeout(r, 0)); + + clock.t += 1; // still inside the TTL of the FRESH value + let refetched = 0; + const after = await cache.read(() => { refetched++; return Promise.resolve({}); }); + assert.equal(refetched, 0, "the fresh value is still cached"); + assert.deepEqual(after, fresh, "the retired request must not have overwritten it"); +}); + +test("#1161: a payload that lands WHILE we wait is used, not refused", async () => { + // Refusing here would tell the user to reconnect ComfyUI at the exact moment the panel + // holds a fresh authoritative schema. + const { cache, fireTimers } = boundedCache(); + const pending = cache.read(() => new Promise(() => {})); + cache.invalidate(); const defs = { KSampler: {} }; assert.deepEqual(await cache.read(() => Promise.resolve(defs)), defs); + fireTimers(); + assert.deepEqual(await pending, defs, "the waiting read must use the payload that arrived"); }); -test("#1161: a rejection still reaches the caller unchanged", async () => { - // Only the timeout arm is sentinel-wrapped; a real failure must propagate as before, - // or the fence would read "the fetch failed" as "I did not wait long enough". +test("#1161: a healthy fetch and a rejection are both unaffected by the bound", async () => { const { cache } = boundedCache(); - await assert.rejects(cache.read(() => Promise.reject(new Error("backend down"))), /backend down/); + const defs = { KSampler: {} }; + assert.deepEqual(await cache.read(() => Promise.resolve(defs)), defs); + const other = boundedCache().cache; + await assert.rejects(other.read(() => Promise.reject(new Error("backend down"))), /backend down/); }); -test("#1161: a late response still populates the cache — giving up on the wait latches nothing", async () => { - // The startup seed's contract, which this must match: the request is not cancelled, so a - // response arriving after the bound still establishes the payload and the NEXT call - // succeeds. The recovery costs one refused call, not a tab reload. - const { cache, fireTimers } = boundedCache(); +test("#1161: a throwing timer degrades to no bound, never to a failed read", async () => { const defs = { KSampler: {} }; - let release; - const slow = new Promise((resolve) => { release = resolve; }); - const first = cache.read(() => slow); - fireTimers(); - assert.equal(await first, null, "the bounded call refuses"); - release(defs); - await slow; - await new Promise((r) => setTimeout(r, 0)); // let the abandoned request settle - let refetched = 0; - const second = await cache.read(() => { - refetched++; - return Promise.resolve({ other: {} }); + const cache = createObjectInfoCache({ + waitMs: 8000, + setTimer: () => { throw new Error("no timers here"); }, + clearTimer: () => {}, }); - assert.equal(refetched, 0, "the late response seeded the cache, so no refetch was needed"); - assert.deepEqual(second, defs); + assert.deepEqual(await cache.read(() => Promise.resolve(defs)), defs); +}); + +test("#1161: the timer is cleared when the fetch wins the race", async () => { + // Without the cleanup an 8s timer is left armed for every read — invisible in behaviour + // and exactly the kind of thing no assertion notices. + const { cache, armed } = boundedCache(); + await cache.read(() => Promise.resolve({ KSampler: {} })); + assert.equal(armed(), 0, "a settled read must leave no timer armed"); }); diff --git a/web/js/lib/object-info-cache.js b/web/js/lib/object-info-cache.js index 9955de0c..738822f1 100644 --- a/web/js/lib/object-info-cache.js +++ b/web/js/lib/object-info-cache.js @@ -113,6 +113,86 @@ export function createObjectInfoCache({ // rather than merely forgetting the value it will produce. let generation = 0; + // #1161 — the sentinel is module-scoped, not allocated per read: a fresh Symbol per call + // is behaviourally identical and only makes garbage on the hot path. + const TIMED_OUT = Symbol("object-info-read-timeout"); + + /** + * Hand a caller the in-flight request, but never let it wait forever. + * + * Applied to EVERY caller — issuer and joiner alike — because the bound belongs where a + * promise is handed out, not where it is created. Bounding only the issuer left every + * concurrent reader on the raw promise, which is how the first attempt at this fix + * failed to fix the burst case the bug was reported on. + * + * @param {Promise} request the in-flight fetch + * @param {number} requestId its identity, so only its OWN waiter retires it + */ + async function boundRead(request, requestId) { + if (!(waitMs > 0)) return request; + let timer = null; + let outcome; + try { + outcome = await Promise.race([ + // A rejection must reach the caller unchanged, so the request arm is not wrapped; + // only the timeout arm needs a sentinel. + request, + new Promise((resolve) => { + // Armed OUTSIDE the executor's throw path would be cleaner still, but the + // executor is where `resolve` lives. A throwing setTimer must not reject the + // read and discard a fetch that already succeeded, so it degrades to "no bound" + // rather than to a failure. + try { + timer = setTimer(() => resolve(TIMED_OUT), waitMs); + } catch { + /* no timer — this arm simply never resolves, and the request arm decides */ + } + }), + ]); + } finally { + if (timer !== null) { + try { + clearTimer(timer); + } catch { + /* a throwing clear must not turn a successful read into a failure */ + } + } + } + if (outcome !== TIMED_OUT) return outcome; + // Abandoned. Keep the rejection observed, or a fetch that fails after the bound becomes + // an unhandled rejection this file did nothing about. + request.catch(() => {}); + // A payload may have landed WHILE we waited — an invalidation plus a successful + // refetch on another path. Refusing then would tell the user to reconnect ComfyUI at + // the exact moment the panel holds a fresh authoritative schema. + if (value !== null && now() - at < ttlMs) return value; + if (inflightId === requestId) { + inflight = null; + inflightGeneration = -1; + inflightId = 0; + // ADVANCE THE GENERATION, which the first attempt did not. Releasing the slot alone + // left the abandoned request still matching `issuedAt === generation`, so a late + // pre-restart payload could overwrite a NEWER schema and re-stamp its TTL — the + // fabricated-success hole #458 exists to close. Two requests were never live in one + // generation before this bound existed; retiring by generation is how this file + // already retires a request it no longer trusts. + generation++; + } + // The #982 outcome wrapper, not a bare null: the refusal must say WHY. Without it both + // call sites compute an empty failure list, and the user is told to hand-check a + // backend that answers /object_info perfectly well, against a route list that is + // absent — a hung read misdiagnosed as an unreachable one. + return { + [CACHE_OUTCOME]: true, + defs: null, + failures: [ + `/object_info did not answer within ${waitMs}ms — the request was abandoned for this ` + + `call and a fresh one will be issued on the next. This is what a half-open ` + + `connection after a ComfyUI restart looks like; the backend itself may be healthy.`, + ], + }; + } + return { /** * Read through the cache. @@ -125,7 +205,14 @@ export function createObjectInfoCache({ // that check an invalidation could be overtaken by the very request it was meant to // retire. Coalescing matters: a burst arriving faster than the fetch completes would // otherwise still issue one request per caller, which is the reported symptom moved. - if (inflight && inflightGeneration === generation) return inflight; + // #1161 — a JOINER is bounded too. The first version of this fix installed the bound + // only below, on the path that ISSUES the request, so this early return handed every + // concurrent caller the raw unbounded promise and the P1 survived for exactly the + // case that matters: #716 built this cache for BURSTS of widget writes, so after a + // restart the issuing call gave up at the bound while every overlapping call parked + // until the orchestrator's 30s timeout. The bound belongs where a promise is handed + // to a caller, not where it is created. + if (inflight && inflightGeneration === generation) return boundRead(inflight, inflightId); const issuedAt = generation; // An id captured BEFORE the promise exists, because `finally` must not name the // binding it is being assigned to: a fetchDefs() that throws SYNCHRONOUSLY runs the @@ -186,43 +273,7 @@ export function createObjectInfoCache({ inflight = request; inflightGeneration = issuedAt; inflightId = requestId; - if (!(waitMs > 0)) return request; - // #1161 — BOUND the wait, and retire the slot when it fires. - // - // Bounding alone would not fix the reported bug. The hung request would stay in - // `inflight`, so the next read would join it and bound out too: a permanent 8s tax - // in place of a permanent hang. Better, and still broken. Releasing the slot is what - // lets the NEXT call issue a fresh request — which is the one that recovers once the - // connection is healthy again. - // - // The request is NOT cancelled. A late response still runs the generation check and - // still populates the cache, so the recovery costs one refused call rather than a - // reload — the same contract the startup seed states: giving up on the WAIT is the - // absence of evidence, and must not latch anything. - let timer = null; - const timedOut = Symbol("object-info-read-timeout"); - const outcome = await Promise.race([ - // A rejection must reach the caller unchanged on the fast path, so it is not - // wrapped here; only the timeout arm needs a sentinel. - request, - new Promise((resolve) => { - timer = setTimer(() => resolve(timedOut), waitMs); - }), - ]).finally(() => { - if (timer !== null) clearTimer(timer); - }); - if (outcome !== timedOut) return outcome; - // Abandoned: keep the promise's rejection observed, or a fetch that fails after the - // bound becomes an unhandled rejection that this file did nothing about. - request.catch(() => {}); - if (inflightId === requestId) { - inflight = null; - inflightGeneration = -1; - inflightId = 0; - } - // No payload: the caller's fence fails closed and refuses THIS call with "retry in a - // moment", exactly as it does for a fetch that failed outright. - return null; + return boundRead(request, requestId); }, /** From 773d002166347da05fec6ecf1bcf1f5c22e8046e Mon Sep 17 00:00:00 2001 From: Artokun Date: Wed, 12 Aug 2026 23:40:21 -0700 Subject: [PATCH 4/7] docs(1161): the release note asserted a property the rework removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entry still said a late answer is still used. That was true of the first attempt and stopped being true when the rework advanced the generation on abandonment, deliberately, so an abandoned request cannot overwrite a newer schema. I changed the behaviour and left the note claiming the old one — which is worse than an unclear note, because it tells a reader the opposite of what ships. Corrected to say the late answer is discarded, and why that trade is the right one: re-asking costs a refetch, while letting a stale payload win costs a write authorized against a backend picture that no longer holds. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31ccb3f6..bd408e33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,9 @@ All notable changes to this project are documented here. This project adheres to every later attempt waited on the same dead one. The check now gives up after a few seconds and refuses that single call with a retry hint, and the abandoned request is discarded so the next attempt makes a fresh one — which succeeds as soon as the backend - is reachable. A slow answer that arrives late is still used, so recovery costs one - refused call rather than reloading the tab. + is reachable. An answer that arrives after the panel has given up is discarded rather + than used, so it cannot overwrite a newer one — the cost is re-asking, never a write + authorized against a stale picture of the backend. ## [0.14.24] - 2026-08-12 From 084eab8c15cb168e8035eb1b5b70390177a73c58 Mon Sep 17 00:00:00 2001 From: Artokun Date: Thu, 13 Aug 2026 00:01:50 -0700 Subject: [PATCH 5/7] fix(1161): use the repo's bounded-step helper, and stop retiring the request Round 2 found four regressions, all traceable to ONE decision: the timeout retired the request instead of merely giving up on waiting for it. Reworked around that, and around the primitive this repo already had. USE THE EXISTING HELPER. web/js/lib/bounded-step.js exists, and its header says exactly what I did: "A second timeout helper written alongside the first is how this repo keeps producing near-duplicate bugs, so there is one here and both callers import it." I hand-rolled a third across two attempts and re-derived its hardening badly both times -- its settle-at-most-once contract is what stops a joiner receiving a payload the call already gave up on, and its never-hang guarantee is what a throwing timer must not be able to defeat. NOT a bare withTimeout, though: that helper never REJECTS by contract, and this cache must keep propagating a failed fetch. Three pre-existing #716 tests pin that, and they caught the regression when I swapped it in -- without the fix a network error that fails instantly would be reported as "/object_info did not answer within 8000ms", which is simply false. The outcome is reified before it is bounded and unwrapped after, so rejection stays rejection and only a real timeout produces the timeout outcome. STOP RETIRING THE REQUEST. The bound now ONLY stops the caller waiting. It does not clear the inflight slot and does not advance the generation, and dropping those is the whole correction: - `generation++` discarded the fetch's OWN late payload, so a merely SLOW backend -- remote, tunnelled, or still loading custom nodes behind a 5MB /object_info -- refused forever. Measured by review: 4/4 sequential reads refuse and the cache never populates. On main the first call hung but the late payload WAS cached, so the second worked. That turned "slow first call, then fine" into "never works" -- worse than the bug. - Clearing the slot dropped #716's one-fetch-per-burst invariant, since the abandoned fetch cannot be cancelled: a probe measured eight outstanding hung requests against a six-connection browser limit. - Neither protected a joiner anyway: it still awaited the abandoned promise directly and received the payload the generation bump had "retired". Leaving the request alone keeps every existing invariant intact -- one fetch per burst, the late payload cached for the next call, staleness still governed by invalidate() -- and the bound alone is sufficient for the P1, which was callers waiting forever. TESTS THAT CANNOT HANG. `node --test` has no default timeout and waits for the event loop to drain, so a never-settling read wedged the suite even when the assertion before it had already failed. I claimed round 1's rewrite fixed this; it did not, and round 2 said so. Every read is now awaited through a bounded helper, so the joiner mutant fails in under a second with "a burst read never settled -- the bound did not apply to it" instead of reporting a hung suite. Mutation-checked: unbounding the joiner, and re-adding the retire-on-timeout, each fail with a named message. 4102 unit tests pass; typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- browser_tests/unit/object-info-cache.test.mjs | 158 +++++++----------- web/js/lib/object-info-cache.js | 132 +++++++-------- 2 files changed, 122 insertions(+), 168 deletions(-) diff --git a/browser_tests/unit/object-info-cache.test.mjs b/browser_tests/unit/object-info-cache.test.mjs index a0080e99..e84036b0 100644 --- a/browser_tests/unit/object-info-cache.test.mjs +++ b/browser_tests/unit/object-info-cache.test.mjs @@ -245,7 +245,7 @@ test("#716: a retired request cannot overwrite a newer value — deterministical }); -// ── #1161: a fetch that never settles must not park the tool ───────────────── +// ── #1161: a fetch that never settles must not park the caller ─────────────── // // The only open P1. After a ComfyUI restart the tab can hold a half-open connection, so // getNodeDefs() never settles. graph_set_widget is the one command that consults @@ -253,10 +253,15 @@ test("#716: a retired request cannot overwrite a newer value — deterministical // other command answered instantly — and coalescing made it permanent, because every // later read joined the same dead promise. // -// These never await a promise that may not settle: a test that detects its bug by hanging -// `node --test` (which has no default timeout) reports a wedged suite instead of naming -// the broken invariant. Every wait here is resolved by firing the injected timer, and the -// clock is injected too, matching the rest of this file. +// The bound ONLY stops the caller waiting. It deliberately does not retire the request: +// two earlier attempts did, and each converted a merely SLOW backend into a permanent +// refusal that was measurably worse than the bug. Everything the cache already guarantees +// — one fetch per burst, the late payload cached for the next call, staleness governed by +// invalidate() — is left intact. +// +// Nothing here awaits a promise that may not settle: a test that detects its bug by +// hanging `node --test` (which has no default timeout) reports a wedged suite instead of +// naming the broken invariant. function boundedCache({ waitMs = 8000, ttlMs = OBJECT_INFO_CACHE_TTL_MS } = {}) { const timers = new Set(); const clock = { t: 1_000_000 }; @@ -264,124 +269,89 @@ function boundedCache({ waitMs = 8000, ttlMs = OBJECT_INFO_CACHE_TTL_MS } = {}) ttlMs, waitMs, now: () => clock.t, - setTimer: (fn, ms) => { - const t = { fn, ms }; - timers.add(t); - return t; - }, + setTimer: (fn, ms) => { const t = { fn, ms }; timers.add(t); return t; }, clearTimer: (t) => timers.delete(t), }); - return { - cache, - clock, - armed: () => timers.size, - fireTimers: () => [...timers].forEach((t) => { timers.delete(t); t.fn(); }), - }; + return { cache, clock, armed: () => timers.size, fireTimers: () => [...timers].forEach((t) => { timers.delete(t); t.fn(); }) }; } +const gaveUp = (o) => o && typeof o === "object" && o[CACHE_OUTCOME] === true && o.defs === null; -const timedOut = (outcome) => - outcome && typeof outcome === "object" && outcome[CACHE_OUTCOME] === true && outcome.defs === null; +// Await a read WITHOUT the ability to hang. `node --test` has no default timeout and waits +// for the event loop to drain, so a read that never settles wedges the whole suite — +// reporting a timeout with no clue which invariant broke, and leaving a pending promise +// alive even when the assertion before it already failed. Round 2 of this review caught +// exactly that. A real bound here turns "the suite hung" into a named failure. +const settled = (p, what) => + Promise.race([ + p, + new Promise((_, reject) => + setTimeout(() => reject(new Error(`${what} never settled — the bound did not apply to it`)), 250), + ), + ]); test("#1161: a fetch that never settles is bounded instead of parking the caller", async () => { const { cache, fireTimers } = boundedCache(); - const read = cache.read(() => new Promise(() => {})); // half-open socket + const read = cache.read(() => new Promise(() => {})); fireTimers(); - const outcome = await read; - assert.ok(timedOut(outcome), "the read gives up on THIS call rather than hanging"); - // The refusal must say WHY: a bare null makes the caller's note misdiagnose a hung read - // as an unreachable backend, naming a route list it never printed. + const outcome = await settled(read, "the bounded read"); + assert.ok(gaveUp(outcome), "the read gives up on THIS call rather than hanging"); + // It must say WHY: a bare null makes both call sites compute an empty failure list, so + // the refusal names no cause and tells the user to hand-check a healthy backend. assert.match(String(outcome.failures?.[0] ?? ""), /did not answer within 8000ms/); }); test("#1161: a JOINER is bounded too — the burst case the bug was reported on", async () => { - // The defect the first attempt shipped: read() returned the in-flight promise BEFORE the - // bound was installed, so only the issuing call was bounded and every overlapping call - // parked until the orchestrator's 30s timeout. #716 built this cache for bursts, so that - // is the case that matters most. + // The first attempt bounded only the issuing call, so every overlapping call still + // parked until the orchestrator's 30s timeout. #716 built this cache for bursts, so + // that is the case that matters most. const { cache, fireTimers, armed } = boundedCache(); - const issuer = cache.read(() => new Promise(() => {})); - const joinerA = cache.read(() => new Promise(() => {})); - const joinerB = cache.read(() => new Promise(() => {})); - assert.equal(armed(), 3, "every caller must arm its own bound, not just the issuer"); + const reads = [cache.read(() => new Promise(() => {})), cache.read(() => new Promise(() => {})), cache.read(() => new Promise(() => {}))]; + assert.equal(armed(), 3, "every caller arms its own bound, not just the issuer"); fireTimers(); - for (const [name, p] of [["issuer", issuer], ["joinerA", joinerA], ["joinerB", joinerB]]) { - assert.ok(timedOut(await p), `${name} must give up rather than park`); - } + for (const [i, p] of reads.entries()) assert.ok(gaveUp(await settled(p, `caller ${i}`)), `caller ${i} must give up rather than park`); }); -test("#1161: the abandoned request is retired, so the next read issues a fresh one", async () => { +test("#1161: a SLOW backend still works — the late payload is cached for the next call", async () => { + // The regression both earlier attempts shipped. Retiring the request on timeout threw + // away its own late answer, so a remote or still-loading ComfyUI refused FOREVER: worse + // than the bug, which at least cached the payload once it arrived. const { cache, fireTimers } = boundedCache(); - let calls = 0; - const first = cache.read(() => { calls++; return new Promise(() => {}); }); - fireTimers(); - assert.ok(timedOut(await first)); - assert.equal(calls, 1); const defs = { KSampler: {} }; - const second = await cache.read(() => { calls++; return Promise.resolve(defs); }); - assert.equal(calls, 2, "the next read must not join the dead request"); - assert.deepEqual(second, defs, "…and it recovers as soon as the backend answers"); -}); - -test("#1161: an abandoned request cannot overwrite a NEWER schema", async () => { - // The generation must advance when the slot is abandoned. Releasing it alone left the - // abandoned request still matching `issuedAt === generation`, so a late pre-restart - // payload could overwrite a newer one and re-stamp its TTL — a write authorized against - // a pack that was uninstalled during the restart, reported as success (#458). - const { cache, clock, fireTimers } = boundedCache(); - let releaseStale; - const stale = new Promise((resolve) => { releaseStale = resolve; }); - const first = cache.read(() => stale); + let land; + const slow = new Promise((resolve) => { land = resolve; }); + const first = cache.read(() => slow); fireTimers(); - assert.ok(timedOut(await first)); - - const fresh = { KSampler: {} }; - assert.deepEqual(await cache.read(() => Promise.resolve(fresh)), fresh); - - releaseStale({ GoneNode: {}, KSampler: {} }); // the pre-restart map lands late - await stale; + assert.ok(gaveUp(await settled(first, "the first call")), "the first call gives up waiting"); + land(defs); + await slow; await new Promise((r) => setTimeout(r, 0)); - - clock.t += 1; // still inside the TTL of the FRESH value let refetched = 0; - const after = await cache.read(() => { refetched++; return Promise.resolve({}); }); - assert.equal(refetched, 0, "the fresh value is still cached"); - assert.deepEqual(after, fresh, "the retired request must not have overwritten it"); + const second = await cache.read(() => { refetched++; return Promise.resolve({ other: {} }); }); + assert.equal(refetched, 0, "the late answer populated the cache"); + assert.deepEqual(second, defs, "…so the next call succeeds instead of refusing forever"); }); -test("#1161: a payload that lands WHILE we wait is used, not refused", async () => { - // Refusing here would tell the user to reconnect ComfyUI at the exact moment the panel - // holds a fresh authoritative schema. +test("#1161: one fetch per burst survives the bound — no request storm", async () => { + // Retiring the slot let each bounded-out read issue ANOTHER un-abortable /object_info, + // against a browser limit of six connections per host. The slot must stay. const { cache, fireTimers } = boundedCache(); - const pending = cache.read(() => new Promise(() => {})); - cache.invalidate(); - const defs = { KSampler: {} }; - assert.deepEqual(await cache.read(() => Promise.resolve(defs)), defs); + let issued = 0; + const reads = []; + for (let i = 0; i < 5; i++) reads.push(cache.read(() => { issued++; return new Promise(() => {}); })); fireTimers(); - assert.deepEqual(await pending, defs, "the waiting read must use the payload that arrived"); + for (const p of reads) assert.ok(gaveUp(await settled(p, "a burst read"))); + assert.equal(issued, 1, "a burst against a hung backend must still cost exactly one fetch"); }); -test("#1161: a healthy fetch and a rejection are both unaffected by the bound", async () => { - const { cache } = boundedCache(); +test("#1161: a healthy fetch, a rejection, and the timer cleanup are all unaffected", async () => { + const { cache, armed } = boundedCache(); const defs = { KSampler: {} }; assert.deepEqual(await cache.read(() => Promise.resolve(defs)), defs); + assert.equal(armed(), 0, "a settled read leaves no timer armed"); + // A REJECTION must still propagate. withTimeout never rejects by contract, so the + // outcome is reified before it is bounded and unwrapped after — otherwise a network + // error that fails instantly would be reported to the user as "did not answer within + // 8000ms", which is false. Three #716 tests pin this, and they caught it. const other = boundedCache().cache; await assert.rejects(other.read(() => Promise.reject(new Error("backend down"))), /backend down/); }); - -test("#1161: a throwing timer degrades to no bound, never to a failed read", async () => { - const defs = { KSampler: {} }; - const cache = createObjectInfoCache({ - waitMs: 8000, - setTimer: () => { throw new Error("no timers here"); }, - clearTimer: () => {}, - }); - assert.deepEqual(await cache.read(() => Promise.resolve(defs)), defs); -}); - -test("#1161: the timer is cleared when the fetch wins the race", async () => { - // Without the cleanup an 8s timer is left armed for every read — invisible in behaviour - // and exactly the kind of thing no assertion notices. - const { cache, armed } = boundedCache(); - await cache.read(() => Promise.resolve({ KSampler: {} })); - assert.equal(armed(), 0, "a settled read must leave no timer armed"); -}); diff --git a/web/js/lib/object-info-cache.js b/web/js/lib/object-info-cache.js index 738822f1..569898d4 100644 --- a/web/js/lib/object-info-cache.js +++ b/web/js/lib/object-info-cache.js @@ -60,6 +60,8 @@ * single definition would become the cached schema. A Symbol cannot appear in JSON, so * only a producer that deliberately tagged its result can be mistaken for one. */ +import { withTimeout } from "./bounded-step.js"; + export const CACHE_OUTCOME = Symbol.for("comfyui-mcp.objectInfoOutcome"); /** How long a fetched payload may be reused. */ @@ -113,85 +115,67 @@ export function createObjectInfoCache({ // rather than merely forgetting the value it will produce. let generation = 0; - // #1161 — the sentinel is module-scoped, not allocated per read: a fresh Symbol per call - // is behaviourally identical and only makes garbage on the hot path. - const TIMED_OUT = Symbol("object-info-read-timeout"); + // #1161 — the timeout note, built once. It is the whole reason the bound returns the + // #982 wrapper rather than a bare null: without a stated cause both call sites compute + // an empty failure list, and the refusal tells the user to hand-check a backend that + // answers /object_info perfectly well. + const timedOutOutcome = () => ({ + [CACHE_OUTCOME]: true, + defs: null, + failures: [ + `/object_info did not answer within ${waitMs}ms, so this call gave up waiting. The ` + + `request is still running and its answer will be cached if it arrives, so retrying ` + + `shortly is the right move; the backend itself may be healthy.`, + ], + }); /** * Hand a caller the in-flight request, but never let it wait forever. * * Applied to EVERY caller — issuer and joiner alike — because the bound belongs where a - * promise is handed out, not where it is created. Bounding only the issuer left every - * concurrent reader on the raw promise, which is how the first attempt at this fix - * failed to fix the burst case the bug was reported on. + * promise is handed out, not where it is created. The first attempt at #1161 bounded + * only the issuer, so every concurrent reader still parked on the raw promise, and the + * burst case the bug was actually reported on was unfixed. + * + * Uses the repo's ONE bounded-step primitive rather than a second timeout helper, whose + * header says exactly why: "A second timeout helper written alongside the first is how + * this repo keeps producing near-duplicate bugs." Two hand-rolled attempts here proved + * it. Its at-most-once contract is what stops a joiner receiving a payload this call + * already gave up on, and it never rejects, so a throwing timer cannot reinstate the + * unbounded wait it exists to remove. * - * @param {Promise} request the in-flight fetch - * @param {number} requestId its identity, so only its OWN waiter retires it + * IT ONLY STOPS THE CALLER WAITING. It does not retire the request, and that is the + * whole correction: an earlier version cleared the inflight slot and advanced the + * generation, which discarded the fetch's own late payload and converted a merely SLOW + * backend — a remote or tunnelled ComfyUI, a 5MB /object_info still loading custom + * nodes — into a permanent refusal, measurably worse than the bug. Leaving the request + * alone keeps every existing invariant: one fetch per burst, the late payload cached for + * the next call, and staleness still governed by invalidate()'s generation bump. */ - async function boundRead(request, requestId) { - if (!(waitMs > 0)) return request; - let timer = null; - let outcome; - try { - outcome = await Promise.race([ - // A rejection must reach the caller unchanged, so the request arm is not wrapped; - // only the timeout arm needs a sentinel. - request, - new Promise((resolve) => { - // Armed OUTSIDE the executor's throw path would be cleaner still, but the - // executor is where `resolve` lives. A throwing setTimer must not reject the - // read and discard a fetch that already succeeded, so it degrades to "no bound" - // rather than to a failure. - try { - timer = setTimer(() => resolve(TIMED_OUT), waitMs); - } catch { - /* no timer — this arm simply never resolves, and the request arm decides */ - } - }), - ]); - } finally { - if (timer !== null) { - try { - clearTimer(timer); - } catch { - /* a throwing clear must not turn a successful read into a failure */ - } - } - } - if (outcome !== TIMED_OUT) return outcome; - // Abandoned. Keep the rejection observed, or a fetch that fails after the bound becomes - // an unhandled rejection this file did nothing about. - request.catch(() => {}); - // A payload may have landed WHILE we waited — an invalidation plus a successful - // refetch on another path. Refusing then would tell the user to reconnect ComfyUI at - // the exact moment the panel holds a fresh authoritative schema. - if (value !== null && now() - at < ttlMs) return value; - if (inflightId === requestId) { - inflight = null; - inflightGeneration = -1; - inflightId = 0; - // ADVANCE THE GENERATION, which the first attempt did not. Releasing the slot alone - // left the abandoned request still matching `issuedAt === generation`, so a late - // pre-restart payload could overwrite a NEWER schema and re-stamp its TTL — the - // fabricated-success hole #458 exists to close. Two requests were never live in one - // generation before this bound existed; retiring by generation is how this file - // already retires a request it no longer trusts. - generation++; - } - // The #982 outcome wrapper, not a bare null: the refusal must say WHY. Without it both - // call sites compute an empty failure list, and the user is told to hand-check a - // backend that answers /object_info perfectly well, against a route list that is - // absent — a hung read misdiagnosed as an unreachable one. - return { - [CACHE_OUTCOME]: true, - defs: null, - failures: [ - `/object_info did not answer within ${waitMs}ms — the request was abandoned for this ` + - `call and a fresh one will be issued on the next. This is what a half-open ` + - `connection after a ComfyUI restart looks like; the backend itself may be healthy.`, - ], - }; - } + // NOT a bare `withTimeout(request, …)`. That helper NEVER rejects by contract — a + // rejected promise degrades through `onTimeout()` exactly as a timeout does — and this + // cache must keep propagating a failed fetch. Three #716 tests pin that, and they caught + // the regression: without this, a network error that fails instantly would be reported + // to the user as "/object_info did not answer within 8000ms", which is simply false. + // + // So the outcome is REIFIED before it is bounded and unwrapped after: the helper still + // provides the settle-at-most-once and never-hang guarantees, while rejection stays a + // rejection and only a real timeout produces the timeout outcome. + const REJECTED = Symbol("object-info-read-rejected"); + const boundRead = (request) => + withTimeout( + request.then( + (v) => ({ v }), + (e) => ({ [REJECTED]: e }), + ), + waitMs, + () => ({ timedOut: true }), + { setTimer, clearTimer }, + ).then((r) => { + if (r && r.timedOut) return timedOutOutcome(); + if (r && REJECTED in r) throw r[REJECTED]; + return r.v; + }); return { /** @@ -212,7 +196,7 @@ export function createObjectInfoCache({ // restart the issuing call gave up at the bound while every overlapping call parked // until the orchestrator's 30s timeout. The bound belongs where a promise is handed // to a caller, not where it is created. - if (inflight && inflightGeneration === generation) return boundRead(inflight, inflightId); + if (inflight && inflightGeneration === generation) return boundRead(inflight); const issuedAt = generation; // An id captured BEFORE the promise exists, because `finally` must not name the // binding it is being assigned to: a fetchDefs() that throws SYNCHRONOUSLY runs the @@ -273,7 +257,7 @@ export function createObjectInfoCache({ inflight = request; inflightGeneration = issuedAt; inflightId = requestId; - return boundRead(request, requestId); + return boundRead(request); }, /** From 7de88a6711f2acb48564e0a769c0b5a195405517 Mon Sep 17 00:00:00 2001 From: Artokun Date: Thu, 13 Aug 2026 00:24:17 -0700 Subject: [PATCH 6/7] =?UTF-8?q?docs(1161):=20the=20release=20note=20descri?= =?UTF-8?q?bed=20a=20version=20that=20no=20longer=20ships=20=E2=80=94=20ag?= =?UTF-8?q?ain?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second time on this entry. I rewrote it for round 2's behaviour (retire the request, discard the late answer), then round 3 removed the retirement because it converted a slow backend into a permanent refusal — and I left the note asserting the opposite in both directions: that a fresh request is issued (it is not) and that a late answer is discarded (it is used). A changelog that confidently states the inverse of the code is worse than a vague one, and this is the second time in this issue I have shipped that. Rewritten to claim only what the code does, and to say nothing about recovery from a genuinely hung socket, which is still an open question on this branch. Also fixed, both found by review: - The timeout note was 220 characters and the oracle truncates each failure entry at 200, so the sentence the user reads ended mid-word. Now 146. - A comment claimed graph_set_widget is the ONLY command consulting /object_info; graph_remove_widget does too. And two test weaknesses of the same kind as the code ones: - `gaveUp()` checked only the wrapper tag and `defs === null`, so four tests passed with `failures` EMPTY — the one field the wrapper exists to carry. It now requires a non-empty cause, and one short enough to survive truncation. - `boundedCache()` hardcoded 8000 instead of importing OBJECT_INFO_READ_WAIT_MS, so no test exercised the bound the panel actually ships. 4102 unit tests pass; typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 15 +++++++------ browser_tests/unit/object-info-cache.test.mjs | 21 ++++++++++++++++--- web/js/lib/object-info-cache.js | 7 +++---- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd408e33..104b89bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,14 +14,13 @@ All notable changes to this project are documented here. This project adheres to renaming a node, listing workflows, queueing a run. Setting a widget is the one action that checks the backend's node definitions before it writes, and that check could wait forever: a restart can leave the browser holding a connection that never answers and - never fails. - It stayed broken rather than recovering because callers share one in-flight request, so - every later attempt waited on the same dead one. The check now gives up after a few - seconds and refuses that single call with a retry hint, and the abandoned request is - discarded so the next attempt makes a fresh one — which succeeds as soon as the backend - is reachable. An answer that arrives after the panel has given up is discarded rather - than used, so it cannot overwrite a newer one — the cost is re-asking, never a write - authorized against a stale picture of the backend. + never fails. It stayed broken rather than recovering because callers share one request, + so every later attempt waited on the same dead one. + The check now gives up after a few seconds and refuses that one call, saying that the + definitions could not be read in time and that the backend itself may be healthy — + rather than leaving the call to time out with no explanation. An answer that arrives + after the panel stopped waiting is still used, so a slow backend costs one refused call + rather than staying broken. ## [0.14.24] - 2026-08-12 diff --git a/browser_tests/unit/object-info-cache.test.mjs b/browser_tests/unit/object-info-cache.test.mjs index e84036b0..9be8763f 100644 --- a/browser_tests/unit/object-info-cache.test.mjs +++ b/browser_tests/unit/object-info-cache.test.mjs @@ -13,6 +13,8 @@ import { // #1161 — the timeout arm returns the outcome WRAPPER so the refusal can state its // cause; the tests assert that shape rather than a bare null. CACHE_OUTCOME, + // The SHIPPED bound, so a test cannot pass against a value the panel does not use. + OBJECT_INFO_READ_WAIT_MS, createObjectInfoCache, } from "../../web/js/lib/object-info-cache.js"; @@ -262,7 +264,7 @@ test("#716: a retired request cannot overwrite a newer value — deterministical // Nothing here awaits a promise that may not settle: a test that detects its bug by // hanging `node --test` (which has no default timeout) reports a wedged suite instead of // naming the broken invariant. -function boundedCache({ waitMs = 8000, ttlMs = OBJECT_INFO_CACHE_TTL_MS } = {}) { +function boundedCache({ waitMs = OBJECT_INFO_READ_WAIT_MS, ttlMs = OBJECT_INFO_CACHE_TTL_MS } = {}) { const timers = new Set(); const clock = { t: 1_000_000 }; const cache = createObjectInfoCache({ @@ -274,7 +276,19 @@ function boundedCache({ waitMs = 8000, ttlMs = OBJECT_INFO_CACHE_TTL_MS } = {}) }); return { cache, clock, armed: () => timers.size, fireTimers: () => [...timers].forEach((t) => { timers.delete(t); t.fn(); }) }; } -const gaveUp = (o) => o && typeof o === "object" && o[CACHE_OUTCOME] === true && o.defs === null; +// The cause is part of the contract, not decoration: without it both call sites compute an +// empty failure list and the refusal tells the user to hand-check a healthy backend. Review +// found four of these tests passed with `failures` empty, so the helper asserts it. +const gaveUp = (o) => + o && + typeof o === "object" && + o[CACHE_OUTCOME] === true && + o.defs === null && + Array.isArray(o.failures) && + o.failures.length > 0 && + // …and short enough to survive the oracle's 200-char per-entry truncation, or the + // sentence the user reads ends mid-word. + String(o.failures[0]).length <= 200; // Await a read WITHOUT the ability to hang. `node --test` has no default timeout and waits // for the event loop to drain, so a read that never settles wedges the whole suite — @@ -297,7 +311,8 @@ test("#1161: a fetch that never settles is bounded instead of parking the caller assert.ok(gaveUp(outcome), "the read gives up on THIS call rather than hanging"); // It must say WHY: a bare null makes both call sites compute an empty failure list, so // the refusal names no cause and tells the user to hand-check a healthy backend. - assert.match(String(outcome.failures?.[0] ?? ""), /did not answer within 8000ms/); + assert.match(String(outcome.failures?.[0] ?? ""), new RegExp(`did not answer within ${OBJECT_INFO_READ_WAIT_MS}ms`)); // eslint-disable-line + void /x/; // ${OBJECT_INFO_READ_WAIT_MS}ms/); }); test("#1161: a JOINER is bounded too — the burst case the bug was reported on", async () => { diff --git a/web/js/lib/object-info-cache.js b/web/js/lib/object-info-cache.js index 569898d4..90033921 100644 --- a/web/js/lib/object-info-cache.js +++ b/web/js/lib/object-info-cache.js @@ -71,7 +71,7 @@ export const OBJECT_INFO_CACHE_TTL_MS = 1500; * #1161 — how long a read will WAIT for a fetch before giving up on THIS call. * * Unbounded was the P1: after a ComfyUI restart the tab can hold a half-open connection, - * so `getNodeDefs()` never settles. `graph_set_widget` is the only command that consults + * so `getNodeDefs()` never settles. `graph_set_widget` (and `graph_remove_widget`) consult * /object_info before writing, so it alone parked forever — every other command on the * same tab answered instantly, which is exactly how the report reads. * @@ -123,9 +123,8 @@ export function createObjectInfoCache({ [CACHE_OUTCOME]: true, defs: null, failures: [ - `/object_info did not answer within ${waitMs}ms, so this call gave up waiting. The ` + - `request is still running and its answer will be cached if it arrives, so retrying ` + - `shortly is the right move; the backend itself may be healthy.`, + `/object_info did not answer within ${waitMs}ms, so this call stopped waiting. The ` + + `request is still running; retry shortly. The backend may be healthy.`, ], }); From 271982af4491dd615ccd870bd752d904b1649052 Mon Sep 17 00:00:00 2001 From: Artokun Date: Fri, 14 Aug 2026 17:01:15 -0700 Subject: [PATCH 7/7] test(1178): drop the superseded cache bound, and pin the behaviour it would have broken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bound this branch proposed has been overtaken by main. #1179 shipped the fix for the hang in the layer round 3 of this PR's own review identified — per-transport, inside `fetchWholeObjectInfo` — and it is released (0.14.25). Keeping a second bound on the burst cache above it is not redundant, it is harmful, and it is measured: branch (8s cache bound) refused at 8003ms, defs=null main (no cache bound) answered at 10010ms with a usable schema Same input both times — the #1161 shape, `api.getNodeDefs()` never settling while `GET /object_info` answers. A cache-level bound has to be SHORTER than the oracle's deadline to fire at all, so it can only ever pre-empt the fallback route #982 added for exactly this failure. It converts a write main SUCCEEDS at into a refusal. Second, measured effect: the branch's timeout outcome carried no `outcomes` tags, so #1223's snapshot fallback was disqualified — with the reason "the backend ANSWERED the schema probe with something unusable", which in a timeout is not true. A refusal asserting something that did not happen is #982's own defect. So the code change is reverted to main, and what this branch established is kept as three guards on the production loader shape: - a hung client route still reaches the fallback THROUGH the cache, asserted on ELAPSED time (the answer arrives at the client route's share of the budget, which is the proof that the oracle's bound is what released the call); - the cache arms NO timer of its own — re-applying this branch's own commit fails this test; - a fully silent backend rides through the cache as SILENCE, tags intact, and never as a usable schema. Each was mutation-tested by breaking the thing it guards: re-applying 7de88a67 kills the second, removing the oracle's per-transport bound kills the first and third, and dropping the `outcomes` tags kills the third. The CHANGELOG entry is dropped too: main already carries it, shipped in 0.14.25. Refs artokun/comfyui-mcp-panel#1161 --- browser_tests/unit/object-info-cache.test.mjs | 309 +++++++++++------- web/js/lib/object-info-cache.js | 106 +----- 2 files changed, 191 insertions(+), 224 deletions(-) diff --git a/browser_tests/unit/object-info-cache.test.mjs b/browser_tests/unit/object-info-cache.test.mjs index 9be8763f..65fe9212 100644 --- a/browser_tests/unit/object-info-cache.test.mjs +++ b/browser_tests/unit/object-info-cache.test.mjs @@ -8,15 +8,9 @@ // real clock is a slow test that eventually becomes a flaky one. import { test } from "node:test"; import assert from "node:assert/strict"; -import { - OBJECT_INFO_CACHE_TTL_MS, - // #1161 — the timeout arm returns the outcome WRAPPER so the refusal can state its - // cause; the tests assert that shape rather than a bare null. - CACHE_OUTCOME, - // The SHIPPED bound, so a test cannot pass against a value the panel does not use. - OBJECT_INFO_READ_WAIT_MS, - createObjectInfoCache, -} from "../../web/js/lib/object-info-cache.js"; +import { OBJECT_INFO_CACHE_TTL_MS, createObjectInfoCache } from "../../web/js/lib/object-info-cache.js"; +import { fetchWholeObjectInfo } from "../../web/js/lib/object-info-oracle.js"; +import { noBackendAnswerEstablished } from "../../web/js/lib/object-info-snapshot.js"; const DEFS = { KSampler: {}, CLIPTextEncode: {} }; const clock = (start = 1000) => { @@ -246,127 +240,200 @@ test("#716: a retired request cannot overwrite a newer value — deterministical ); }); - -// ── #1161: a fetch that never settles must not park the caller ─────────────── +// --------------------------------------------------------------------------- +// #1178 — WHERE THE BOUND FOR #1161 LIVES, and why it may not live here. // -// The only open P1. After a ComfyUI restart the tab can hold a half-open connection, so -// getNodeDefs() never settles. graph_set_widget is the one command that consults -// /object_info before writing, so it alone hung — 30s timeouts on every node while every -// other command answered instantly — and coalescing made it permanent, because every -// later read joined the same dead promise. +// #1161 was the 30s hang: after a ComfyUI restart the tab can hold a half-open +// connection, so `api.getNodeDefs()` never settles and every `graph_set_widget` +// parked until the caller's timeout. Three attempts on #1178 put the bound in THIS +// file — on the burst cache's own read — and every one of them traded the hang for a +// refusal. #1179 shipped the answer instead: bound each TRANSPORT inside +// `fetchWholeObjectInfo`, so a hung client route falls through to the `GET +// /object_info` fallback #982 added for exactly this failure, and the write SUCCEEDS. // -// The bound ONLY stops the caller waiting. It deliberately does not retire the request: -// two earlier attempts did, and each converted a merely SLOW backend into a permanent -// refusal that was measurably worse than the bug. Everything the cache already guarantees -// — one fetch per burst, the late payload cached for the next call, staleness governed by -// invalidate() — is left intact. +// A bound here cannot do that. It sits ABOVE the oracle, so it can only choose how to +// FAIL — and because it would have to be shorter than the oracle's deadline to fire at +// all, it pre-empts the fallback route before that route is ever asked. Measured on the +// #1178 branch against this same production loader shape: refused at 8003ms, where the +// shipped code answers with a usable schema at 10010ms. // -// Nothing here awaits a promise that may not settle: a test that detects its bug by -// hanging `node --test` (which has no default timeout) reports a wedged suite instead of -// naming the broken invariant. -function boundedCache({ waitMs = OBJECT_INFO_READ_WAIT_MS, ttlMs = OBJECT_INFO_CACHE_TTL_MS } = {}) { - const timers = new Set(); - const clock = { t: 1_000_000 }; - const cache = createObjectInfoCache({ - ttlMs, - waitMs, - now: () => clock.t, - setTimer: (fn, ms) => { const t = { fn, ms }; timers.add(t); return t; }, - clearTimer: (t) => timers.delete(t), - }); - return { cache, clock, armed: () => timers.size, fireTimers: () => [...timers].forEach((t) => { timers.delete(t); t.fn(); }) }; -} -// The cause is part of the contract, not decoration: without it both call sites compute an -// empty failure list and the refusal tells the user to hand-check a healthy backend. Review -// found four of these tests passed with `failures` empty, so the helper asserts it. -const gaveUp = (o) => - o && - typeof o === "object" && - o[CACHE_OUTCOME] === true && - o.defs === null && - Array.isArray(o.failures) && - o.failures.length > 0 && - // …and short enough to survive the oracle's 200-char per-entry truncation, or the - // sentence the user reads ends mid-word. - String(o.failures[0]).length <= 200; +// These three tests pin that outcome from the call site's side, so the next person to +// reach for a cache-level bound gets a failing test with the reason rather than a fourth +// rediscovery. Refs artokun/comfyui-mcp-panel#1161. +// --------------------------------------------------------------------------- -// Await a read WITHOUT the ability to hang. `node --test` has no default timeout and waits -// for the event loop to drain, so a read that never settles wedges the whole suite — -// reporting a timeout with no clue which invariant broke, and leaving a pending promise -// alive even when the assertion before it already failed. Round 2 of this review caught -// exactly that. A real bound here turns "the suite hung" into a named failure. -const settled = (p, what) => - Promise.race([ - p, - new Promise((_, reject) => - setTimeout(() => reject(new Error(`${what} never settled — the bound did not apply to it`)), 250), - ), - ]); +/** A transport that never settles — the half-open connection #1161 was reported on. */ +const NEVER_SETTLES = () => new Promise(() => {}); -test("#1161: a fetch that never settles is bounded instead of parking the caller", async () => { - const { cache, fireTimers } = boundedCache(); - const read = cache.read(() => new Promise(() => {})); - fireTimers(); - const outcome = await settled(read, "the bounded read"); - assert.ok(gaveUp(outcome), "the read gives up on THIS call rather than hanging"); - // It must say WHY: a bare null makes both call sites compute an empty failure list, so - // the refusal names no cause and tells the user to hand-check a healthy backend. - assert.match(String(outcome.failures?.[0] ?? ""), new RegExp(`did not answer within ${OBJECT_INFO_READ_WAIT_MS}ms`)); // eslint-disable-line - void /x/; // ${OBJECT_INFO_READ_WAIT_MS}ms/); -}); +/** + * A deterministic scheduler for the oracle's injected `timers`/`now`. + * + * The clock advances ONLY when a timer fires, so `elapsed()` is the simulated time the + * bound actually waited — which is the assertion that distinguishes "the oracle's bound + * answered" from "something else did". A test that slept for a real 10s would be the slow + * test this file's header warns about. + */ +function fakeSchedule() { + let t = 0; + let seq = 0; + const pending = new Map(); + return { + now: () => t, + timers: { + setTimer: (fn, ms) => { + const id = ++seq; + pending.set(id, { at: t + ms, fn }); + return id; + }, + clearTimer: (id) => pending.delete(id), + }, + elapsed: () => t, + /** Advance to the earliest armed timer and fire it. False when none is armed. */ + fireNext() { + let pick = null; + for (const [id, entry] of pending) if (pick === null || entry.at < pick.entry.at) pick = { id, entry }; + if (pick === null) return false; + pending.delete(pick.id); + t = pick.entry.at; + pick.entry.fn(); + return true; + }, + }; +} -test("#1161: a JOINER is bounded too — the burst case the bug was reported on", async () => { - // The first attempt bounded only the issuing call, so every overlapping call still - // parked until the orchestrator's 30s timeout. #716 built this cache for bursts, so - // that is the case that matters most. - const { cache, fireTimers, armed } = boundedCache(); - const reads = [cache.read(() => new Promise(() => {})), cache.read(() => new Promise(() => {})), cache.read(() => new Promise(() => {}))]; - assert.equal(armed(), 3, "every caller arms its own bound, not just the issuer"); - fireTimers(); - for (const [i, p] of reads.entries()) assert.ok(gaveUp(await settled(p, `caller ${i}`)), `caller ${i} must give up rather than park`); -}); +/** + * Await `promise`, firing scheduled timers whenever it is not making progress on its own. + * + * NEVER awaits a promise that may not settle without also driving the clock forward, which + * is what turns a broken bound into a FAILING test rather than a hung `node --test` run. + */ +async function settleWith(schedule, promise) { + let done = false; + let result; + let failure; + promise.then( + (v) => { + done = true; + result = v; + }, + (e) => { + done = true; + failure = e ?? new Error("rejected with a falsy value"); + }, + ); + for (let i = 0; i < 100 && !done; i++) { + // Drain microtasks first: a step that answers on its own must not be charged a timer. + for (let k = 0; k < 20 && !done; k++) await Promise.resolve(); + if (done) break; + // NOTHING ARMED IS NOT THE SAME AS STUCK. The last bounded step clears its timer the + // moment it settles, so the final turns of the chain run with an empty schedule — an + // earlier version of this driver read that as "no progress possible" and failed a test + // whose subject had in fact already answered. Give the chain a full macrotask before + // concluding anything, and only then give up. + if (!schedule.fireNext()) { + await new Promise((resolve) => setImmediate(resolve)); + if (done) break; + if (!schedule.fireNext()) break; + } + } + assert.ok(done, "the read never settled — the bound the oracle installs did not fire"); + if (failure) throw failure; + return result; +} -test("#1161: a SLOW backend still works — the late payload is cached for the next call", async () => { - // The regression both earlier attempts shipped. Retiring the request on timeout threw - // away its own late answer, so a remote or still-loading ComfyUI refused FOREVER: worse - // than the bug, which at least cached the payload once it arrived. - const { cache, fireTimers } = boundedCache(); - const defs = { KSampler: {} }; - let land; - const slow = new Promise((resolve) => { land = resolve; }); - const first = cache.read(() => slow); - fireTimers(); - assert.ok(gaveUp(await settled(first, "the first call")), "the first call gives up waiting"); - land(defs); - await slow; - await new Promise((r) => setTimeout(r, 0)); - let refetched = 0; - const second = await cache.read(() => { refetched++; return Promise.resolve({ other: {} }); }); - assert.equal(refetched, 0, "the late answer populated the cache"); - assert.deepEqual(second, defs, "…so the next call succeeds instead of refusing forever"); +test("#1178/#1179: a hung client route still reaches the fallback THROUGH the burst cache", async () => { + // The production loader shape, exactly as `graph_set_widget` and `graph_remove_widget` + // build it: the whole-schema oracle, read through this cache. The client route never + // answers; the HTTP route does. The write must be AUTHORIZED, not refused. + const schedule = fakeSchedule(); + const cache = createObjectInfoCache({ now: clock().now }); + const deadlineMs = 1000; + let httpCalls = 0; + const outcome = await settleWith( + schedule, + cache.read(() => + fetchWholeObjectInfo({ + getNodeDefs: NEVER_SETTLES, + fetchApi: async () => { + httpCalls += 1; + return { ok: true, status: 200, json: async () => ({ KSampler: {}, VAELoader: {} }) }; + }, + deadlineMs, + timers: schedule.timers, + now: schedule.now, + }), + ), + ); + assert.equal(httpCalls, 1, "the fallback route #982 added must actually be asked"); + assert.deepEqual(Object.keys(outcome.defs ?? {}), ["KSampler", "VAELoader"], "and its answer must reach the fence"); + // ELAPSED, not merely the outcome: the answer arrives when the CLIENT ROUTE'S share of + // the oracle budget runs out (half of it, per object-info-oracle.js), which is the proof + // that the oracle's bound is what released this call. A cache-level bound would have to + // fire before this to matter, and firing before this is precisely what skips the fallback. + assert.equal(schedule.elapsed(), deadlineMs / 2, "released by the oracle's client-route bound"); }); -test("#1161: one fetch per burst survives the bound — no request storm", async () => { - // Retiring the slot let each bounded-out read issue ANOTHER un-abortable /object_info, - // against a browser limit of six connections per host. The slot must stay. - const { cache, fireTimers } = boundedCache(); - let issued = 0; - const reads = []; - for (let i = 0; i < 5; i++) reads.push(cache.read(() => { issued++; return new Promise(() => {}); })); - fireTimers(); - for (const p of reads) assert.ok(gaveUp(await settled(p, "a burst read"))); - assert.equal(issued, 1, "a burst against a hung backend must still cost exactly one fetch"); +test("#1178: the burst cache arms NO timer of its own — the bound belongs one layer down", async () => { + // The decisive guard, written against the CALL rather than the source text so a rename + // cannot slip past it. Re-introducing `withTimeout(...)` in `read()` — for the issuer or + // for a joiner — arms a real timer here and fails this test with the reason. + const realSetTimeout = globalThis.setTimeout; + let armed = 0; + globalThis.setTimeout = (...args) => { + armed += 1; + return realSetTimeout(...args); + }; + try { + const cache = createObjectInfoCache({ now: clock().now }); + let release; + const gate = new Promise((r) => (release = r)); + const issuer = cache.read(async () => { + await gate; + return DEFS; + }); + const joiner = cache.read(async () => DEFS); // coalesces onto the request above + release(); + assert.equal(await issuer, DEFS); + assert.equal(await joiner, DEFS, "a joiner gets the same answer, unbounded and unmodified"); + assert.equal( + armed, + 0, + "the cache must not bound its own read: a bound here can only pre-empt the oracle's " + + "fallback route, which is how #1178's three attempts each turned the hang into a refusal", + ); + } finally { + globalThis.setTimeout = realSetTimeout; + } }); -test("#1161: a healthy fetch, a rejection, and the timer cleanup are all unaffected", async () => { - const { cache, armed } = boundedCache(); - const defs = { KSampler: {} }; - assert.deepEqual(await cache.read(() => Promise.resolve(defs)), defs); - assert.equal(armed(), 0, "a settled read leaves no timer armed"); - // A REJECTION must still propagate. withTimeout never rejects by contract, so the - // outcome is reified before it is bounded and unwrapped after — otherwise a network - // error that fails instantly would be reported to the user as "did not answer within - // 8000ms", which is false. Three #716 tests pin this, and they caught it. - const other = boundedCache().cache; - await assert.rejects(other.read(() => Promise.reject(new Error("backend down"))), /backend down/); +test("#1178/#1223: a fully silent backend rides through the cache as SILENCE, not as an answer", async () => { + // What the cache hands back when neither route answers has to stay the ORACLE's outcome, + // tags and all. #1223's snapshot fallback is licensed on exactly that distinction, so a + // cache that substituted a note of its own would disable the fallback for every caller AND + // make the refusal name a cause that never happened — the #982 defect, committed one layer + // up. (Measured on the #1178 branch: `outcomes` arrived null and the refusal read "the + // backend ANSWERED the schema probe with something unusable", which it had not.) + const schedule = fakeSchedule(); + const cache = createObjectInfoCache({ now: clock().now }); + const outcome = await settleWith( + schedule, + cache.read(() => + fetchWholeObjectInfo({ + getNodeDefs: NEVER_SETTLES, + fetchApi: NEVER_SETTLES, + deadlineMs: 1000, + timers: schedule.timers, + now: schedule.now, + }), + ), + ); + // NO FABRICATED SUCCESS. The read is what AUTHORIZES the write, so it runs before any + // mutation — a call that ends here refused without touching the graph, and says so. + assert.equal(outcome.defs, null, "silence must never read as a usable schema"); + assert.ok(outcome.failures.length >= 2, "every route that did not answer is named"); + assert.equal( + noBackendAnswerEstablished(outcome.outcomes), + true, + "the transport tags must survive the cache, or #1223's snapshot fallback is dead on this path", + ); }); diff --git a/web/js/lib/object-info-cache.js b/web/js/lib/object-info-cache.js index 90033921..81b2023c 100644 --- a/web/js/lib/object-info-cache.js +++ b/web/js/lib/object-info-cache.js @@ -60,48 +60,16 @@ * single definition would become the cached schema. A Symbol cannot appear in JSON, so * only a producer that deliberately tagged its result can be mistaken for one. */ -import { withTimeout } from "./bounded-step.js"; - export const CACHE_OUTCOME = Symbol.for("comfyui-mcp.objectInfoOutcome"); /** How long a fetched payload may be reused. */ export const OBJECT_INFO_CACHE_TTL_MS = 1500; -/** - * #1161 — how long a read will WAIT for a fetch before giving up on THIS call. - * - * Unbounded was the P1: after a ComfyUI restart the tab can hold a half-open connection, - * so `getNodeDefs()` never settles. `graph_set_widget` (and `graph_remove_widget`) consult - * /object_info before writing, so it alone parked forever — every other command on the - * same tab answered instantly, which is exactly how the report reads. - * - * The coalescing below is what made it permanent rather than transient: a hung request - * stays in the `inflight` slot, so every later read JOINS the same dead promise instead - * of issuing its own. Nothing settles it, so nothing clears it, and the command is broken - * for the rest of the session. - * - * This is the same hazard the startup seed already bounds, in the same words: "a request - * that never settles (a hung/half-open connection) would otherwise block the awaiting - * tool FOREVER. Bounding the wait converts that hang into the correct outcome." Matching - * that 8s deliberately — the two waits are the same decision about the same endpoint, and - * a reader comparing them should not have to wonder why they differ. - */ -export const OBJECT_INFO_READ_WAIT_MS = 8000; - /** * @param {{ttlMs?: number, now?: () => number}} [opts] `now` is injectable so tests do not * depend on wall-clock timing, which is how a cache test becomes a flaky test. */ -export function createObjectInfoCache({ - ttlMs = OBJECT_INFO_CACHE_TTL_MS, - now = () => Date.now(), - waitMs = OBJECT_INFO_READ_WAIT_MS, - // Injectable so the bound can be tested without a real 8s wait — the same reason `now` - // is injectable. A test that sleeps for the timeout is a slow test that eventually - // becomes a flaky one. - setTimer = (fn, ms) => setTimeout(fn, ms), - clearTimer = (t) => clearTimeout(t), -} = {}) { +export function createObjectInfoCache({ ttlMs = OBJECT_INFO_CACHE_TTL_MS, now = () => Date.now() } = {}) { let value = null; let at = 0; let inflight = null; @@ -115,67 +83,6 @@ export function createObjectInfoCache({ // rather than merely forgetting the value it will produce. let generation = 0; - // #1161 — the timeout note, built once. It is the whole reason the bound returns the - // #982 wrapper rather than a bare null: without a stated cause both call sites compute - // an empty failure list, and the refusal tells the user to hand-check a backend that - // answers /object_info perfectly well. - const timedOutOutcome = () => ({ - [CACHE_OUTCOME]: true, - defs: null, - failures: [ - `/object_info did not answer within ${waitMs}ms, so this call stopped waiting. The ` + - `request is still running; retry shortly. The backend may be healthy.`, - ], - }); - - /** - * Hand a caller the in-flight request, but never let it wait forever. - * - * Applied to EVERY caller — issuer and joiner alike — because the bound belongs where a - * promise is handed out, not where it is created. The first attempt at #1161 bounded - * only the issuer, so every concurrent reader still parked on the raw promise, and the - * burst case the bug was actually reported on was unfixed. - * - * Uses the repo's ONE bounded-step primitive rather than a second timeout helper, whose - * header says exactly why: "A second timeout helper written alongside the first is how - * this repo keeps producing near-duplicate bugs." Two hand-rolled attempts here proved - * it. Its at-most-once contract is what stops a joiner receiving a payload this call - * already gave up on, and it never rejects, so a throwing timer cannot reinstate the - * unbounded wait it exists to remove. - * - * IT ONLY STOPS THE CALLER WAITING. It does not retire the request, and that is the - * whole correction: an earlier version cleared the inflight slot and advanced the - * generation, which discarded the fetch's own late payload and converted a merely SLOW - * backend — a remote or tunnelled ComfyUI, a 5MB /object_info still loading custom - * nodes — into a permanent refusal, measurably worse than the bug. Leaving the request - * alone keeps every existing invariant: one fetch per burst, the late payload cached for - * the next call, and staleness still governed by invalidate()'s generation bump. - */ - // NOT a bare `withTimeout(request, …)`. That helper NEVER rejects by contract — a - // rejected promise degrades through `onTimeout()` exactly as a timeout does — and this - // cache must keep propagating a failed fetch. Three #716 tests pin that, and they caught - // the regression: without this, a network error that fails instantly would be reported - // to the user as "/object_info did not answer within 8000ms", which is simply false. - // - // So the outcome is REIFIED before it is bounded and unwrapped after: the helper still - // provides the settle-at-most-once and never-hang guarantees, while rejection stays a - // rejection and only a real timeout produces the timeout outcome. - const REJECTED = Symbol("object-info-read-rejected"); - const boundRead = (request) => - withTimeout( - request.then( - (v) => ({ v }), - (e) => ({ [REJECTED]: e }), - ), - waitMs, - () => ({ timedOut: true }), - { setTimer, clearTimer }, - ).then((r) => { - if (r && r.timedOut) return timedOutOutcome(); - if (r && REJECTED in r) throw r[REJECTED]; - return r.v; - }); - return { /** * Read through the cache. @@ -188,14 +95,7 @@ export function createObjectInfoCache({ // that check an invalidation could be overtaken by the very request it was meant to // retire. Coalescing matters: a burst arriving faster than the fetch completes would // otherwise still issue one request per caller, which is the reported symptom moved. - // #1161 — a JOINER is bounded too. The first version of this fix installed the bound - // only below, on the path that ISSUES the request, so this early return handed every - // concurrent caller the raw unbounded promise and the P1 survived for exactly the - // case that matters: #716 built this cache for BURSTS of widget writes, so after a - // restart the issuing call gave up at the bound while every overlapping call parked - // until the orchestrator's 30s timeout. The bound belongs where a promise is handed - // to a caller, not where it is created. - if (inflight && inflightGeneration === generation) return boundRead(inflight); + if (inflight && inflightGeneration === generation) return inflight; const issuedAt = generation; // An id captured BEFORE the promise exists, because `finally` must not name the // binding it is being assigned to: a fetchDefs() that throws SYNCHRONOUSLY runs the @@ -256,7 +156,7 @@ export function createObjectInfoCache({ inflight = request; inflightGeneration = issuedAt; inflightId = requestId; - return boundRead(request); + return request; }, /**