test(1178): pin why the /object_info bound cannot live in the burst cache - #1178
Draft
artokun wants to merge 9 commits into
Draft
test(1178): pin why the /object_info bound cannot live in the burst cache#1178artokun wants to merge 9 commits into
artokun wants to merge 9 commits into
Conversation
…it after a restart Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
Author
Diagnosis — an unbounded fetch, made permanent by request coalescingFound by reading the two awaits on the write path. It accounts for every detail of the report, including the ones that make it look strange. The mechanism
After a ComfyUI restart the tab can hold a half-open connection, so Then coalescing makes it permanent. The cache joins concurrent misses: if (inflight && inflightGeneration === generation) return inflight;The hung request stays Why it matches the report exactly
The repo already states the principleThe startup seed bounds its wait at 8s, and its comment says why in as many words:
That is this bug, on the sibling path, already understood and already solved. The write path's fetch simply never got the same treatment — and a bound there degrades to the same correct outcome the seed already produces: refuse this one call with "retry in a moment", leaving the fence's fail-closed behaviour intact. What I still want to establish before changing anything
|
…dons 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) <noreply@anthropic.com>
Owner
Author
Review outcome: the diagnosis holds, this implementation does not16 CONFIRMED findings. The root cause in the diagnosis above is unchallenged — an unbounded It breaks the one-request-per-generation invariant (CONFIRMED, several finders)The timeout arm releases the With the slot released and the generation unchanged, an abandoned request and its replacement are both "current". Both can populate
|
Owner
Author
Addendum — I under-reported this. The headline finding is worse than what I posted.The full review adds a defect I missed entirely, and it is the most important one: The bound never applies to a coalescing reader (CONFIRMED, verified by execution)
Verified by running the shipped module: three reads against a never-settling fetch, and the joiners stay pending indefinitely with zero timers armed for them. That means the fix does not fix the reported bug in the case it most matters. #716 added this cache for bursts of I claimed this change fixed a P1. It does not, and I should have tested a concurrent burst rather than only the single-reader path. Also confirmed, and also missed by meA read can refuse while holding a perfectly good schema. The timeout path returns Corrected designAdding to the list above:
Still draft, and now for a stronger reason than I first gave: this does not solve the P1 it claims to. |
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Owner
Author
Round 2: 16 CONFIRMED, and the decisive one is that I should not have written this by handThe rework fixed the headline defect from round 1 (joiners are bounded now, verified) but introduced its own, and the review found the reason underneath all of them. The repo already has this primitive
I wrote a third by hand, across two attempts, and re-derived its hardening badly both times. What it guarantees is precisely what my version got wrong:
That third row is the hole I created while fixing the previous hole: Also confirmed
What this needsRewrite the bound in terms of Then re-examine the two questions the helper does not answer, which are genuinely about this cache rather than about timeouts: whether a bounded-out read should retire the shared request at all (it drops the one-request-per-burst property #716 added this cache for), and what the bound should be given a 1500ms TTL and the caller's 30s timeout — 8s was chosen by analogy with the startup seed, which is reasoning from similarity rather than from consequence. Still draft. The diagnosis has held through two rounds; the implementation has not, twice, and the reason is now clear enough to be worth stating rather than iterating past. |
…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) <noreply@anthropic.com>
… — again
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) <noreply@anthropic.com>
Owner
Author
Round 3: 25 verified findings, and the actual answer — the bound is in the wrong layerThree attempts, three permanent-refusal cases. The review found why they all failed, and it is not a tuning problem. The fix belongs in the oracle, not the cache
Bounding the cache means a hung Bounding inside the oracle, per transport, both stops the hang and lets the fallback answer. The widget write then succeeds instead of being refused. That is why every round of mine traded one permanent refusal for another: I kept choosing how to FAIL, when the fix is not to fail. What that means for this branchEvery remaining defect is downstream of that layering choice:
Fixed while the review ran
RecommendationDo not merge this branch. The P1 is real and the diagnosis has held across all three rounds, but the fix belongs in I am leaving the branch as the record of what was ruled out, with the reasoning, so the next attempt starts from the answer instead of re-deriving three dead ends. |
artokun
added a commit
that referenced
this pull request
Aug 13, 2026
…the fallback Three rounds of review on #1178 established that bounding the CACHE cannot fix this, and the third found why: the bound sat one layer above the oracle. fetchWholeObjectInfo has TWO transports -- api.getNodeDefs() and a direct GET /object_info, the latter added by #982 precisely because the client route can stop working while the HTTP endpoint answers fine. Both were awaited UNBOUNDED. So a client route that HANGS rather than throwing parked the whole oracle on the first transport, and the route that would have answered was never asked. Bounding the cache can only choose how to give up. Bounding each TRANSPORT means a hung first route falls through to the second, which answers -- so the widget write SUCCEEDS instead of being refused. That is why every attempt on #1178 traded one permanent refusal for another: each was choosing how to fail, when the fix is not to fail. The outcome shape is unchanged. A transport that does not answer becomes a failures.push() entry, which is the vocabulary this function already speaks, so the cache is untouched and every #982/#458 guarantee stands. No wrapper, no retirement, no generation bookkeeping -- those only existed because the bound was in the wrong place. Three things this had to get right, each pinned: - A TIMEOUT MUST NOT READ AS A THROW. withTimeout never rejects by contract, so a naive wrap would collapse "it threw" into "it timed out" and the refusal would name the wrong cause. The outcome is reified before bounding and unwrapped after, and a test asserts a throwing route keeps its own message while a hanging one keeps its own. - THE BODY IS A SECOND I/O STEP. res.json() sat inside the try/catch this replaced, and an existing #982 test caught it escaping. Reading a 5MB schema over a half-open connection can stall after the headers arrive, so it is bounded rather than merely re-caught. - THE REFUSAL WORDING IS UNCHANGED. A parse failure keeps the sentence an existing test pins. This change is about the waits; rewording refusals would be a separate, reviewable decision. Mutation-checked, and both mutants FAIL rather than hang: unbounding the first transport reports "the oracle call never settled — a transport was not bounded", and returning instead of falling through fails the test that proves the fallback answers. 4102 unit tests pass; typecheck and node --check over web/js are clean. Closes #1161. Replaces #1178, kept open as the record of what was ruled out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
artokun
added a commit
that referenced
this pull request
Aug 13, 2026
…ting Addresses the confirmed findings from the review of the per-transport version. A DEADLINE, NOT PER-STEP BOUNDS. There are three I/O steps -- client route, response, body -- and giving each its own 6s multiplied into an 18s worst case nobody chose, stacked on the 8s startup seed wait. One budget for the whole question makes the worst case a single number, and lets a fast first step hand its unused time to a slow second one, which is what happens when the client route fails instantly and the fallback has a 5MB payload to move. A step reached with no budget left reports without issuing a request whose answer it has already decided not to wait for. SIZED FROM THE PAYLOAD, NOT FROM A PING. I justified 6000ms by citing ~167ms -- that measures a fast LAN RESPONSE, while the bounded work is a 5,413,770-byte DOWNLOAD this repo measured at ~14.5s (#610). 6s would have refused an ORDINARY install, and since the bound does not cancel, it would have started a second full download of the same payload on the way to refusing: ~10.8MB of transfer to fail a write that previously succeeded. Now 20s -- above the measurement, still inside the bridge's 30s command timeout so the caller sees this oracle's answer rather than a bare timeout. IT MUST NEVER REJECT. This module states "every failure path returns defs: null", and replacing both try/catch blocks re-protected only the awaits. Review proved the regression by running main and HEAD side by side: a response with a throwing `ok` accessor, and a Proxy whose ownKeys trap throws (reached through usableDefs), both returned a failures list on main and REJECTED here. graph_remove_widget and graph_get_object_info await this with no try/catch of their own, so the agent would have received a raw error instead of the documented refusal. Both reads are guarded again, and both cases are pinned. THE WIDENING IS RECORDED, NOT PATCHED OVER. A deadline treats a client route that does not answer IN TIME as one that answered nothing, so a client which filters deliberately and is merely SLOW can now have the raw route consulted over it -- against the sentence in this file's own header. Refusing instead is precisely the P1, as three attempts on #1178 established. The trade is deliberate and is written at the invariant it modifies, including its bounds: only a deliberate NARROWING can be overridden, only while that client is slower than the budget, and only for a write to a type it withheld. Tests now cover the SHIPPED constant, which nothing did before -- setting it to 0 disables the bound by withTimeout's contract and previously left the whole gate green. Mutation-checked: a zero budget fails 11 tests, and giving each step the full deadline fails the shared-budget test. 4106 unit tests pass; typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
artokun
added a commit
that referenced
this pull request
Aug 13, 2026
…rough to the one that works (#1179) * chore(1161): claim — bound each /object_info transport so the fallback is reachable Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): bound each /object_info transport so a hung route reaches the fallback Three rounds of review on #1178 established that bounding the CACHE cannot fix this, and the third found why: the bound sat one layer above the oracle. fetchWholeObjectInfo has TWO transports -- api.getNodeDefs() and a direct GET /object_info, the latter added by #982 precisely because the client route can stop working while the HTTP endpoint answers fine. Both were awaited UNBOUNDED. So a client route that HANGS rather than throwing parked the whole oracle on the first transport, and the route that would have answered was never asked. Bounding the cache can only choose how to give up. Bounding each TRANSPORT means a hung first route falls through to the second, which answers -- so the widget write SUCCEEDS instead of being refused. That is why every attempt on #1178 traded one permanent refusal for another: each was choosing how to fail, when the fix is not to fail. The outcome shape is unchanged. A transport that does not answer becomes a failures.push() entry, which is the vocabulary this function already speaks, so the cache is untouched and every #982/#458 guarantee stands. No wrapper, no retirement, no generation bookkeeping -- those only existed because the bound was in the wrong place. Three things this had to get right, each pinned: - A TIMEOUT MUST NOT READ AS A THROW. withTimeout never rejects by contract, so a naive wrap would collapse "it threw" into "it timed out" and the refusal would name the wrong cause. The outcome is reified before bounding and unwrapped after, and a test asserts a throwing route keeps its own message while a hanging one keeps its own. - THE BODY IS A SECOND I/O STEP. res.json() sat inside the try/catch this replaced, and an existing #982 test caught it escaping. Reading a 5MB schema over a half-open connection can stall after the headers arrive, so it is bounded rather than merely re-caught. - THE REFUSAL WORDING IS UNCHANGED. A parse failure keeps the sentence an existing test pins. This change is about the waits; rewording refusals would be a separate, reviewable decision. Mutation-checked, and both mutants FAIL rather than hang: unbounding the first transport reports "the oracle call never settled — a transport was not bounded", and returning instead of falling through fails the test that proves the fallback answers. 4102 unit tests pass; typecheck and node --check over web/js are clean. Closes #1161. Replaces #1178, kept open as the record of what was ruled out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): one shared budget, sized from the payload, and never rejecting Addresses the confirmed findings from the review of the per-transport version. A DEADLINE, NOT PER-STEP BOUNDS. There are three I/O steps -- client route, response, body -- and giving each its own 6s multiplied into an 18s worst case nobody chose, stacked on the 8s startup seed wait. One budget for the whole question makes the worst case a single number, and lets a fast first step hand its unused time to a slow second one, which is what happens when the client route fails instantly and the fallback has a 5MB payload to move. A step reached with no budget left reports without issuing a request whose answer it has already decided not to wait for. SIZED FROM THE PAYLOAD, NOT FROM A PING. I justified 6000ms by citing ~167ms -- that measures a fast LAN RESPONSE, while the bounded work is a 5,413,770-byte DOWNLOAD this repo measured at ~14.5s (#610). 6s would have refused an ORDINARY install, and since the bound does not cancel, it would have started a second full download of the same payload on the way to refusing: ~10.8MB of transfer to fail a write that previously succeeded. Now 20s -- above the measurement, still inside the bridge's 30s command timeout so the caller sees this oracle's answer rather than a bare timeout. IT MUST NEVER REJECT. This module states "every failure path returns defs: null", and replacing both try/catch blocks re-protected only the awaits. Review proved the regression by running main and HEAD side by side: a response with a throwing `ok` accessor, and a Proxy whose ownKeys trap throws (reached through usableDefs), both returned a failures list on main and REJECTED here. graph_remove_widget and graph_get_object_info await this with no try/catch of their own, so the agent would have received a raw error instead of the documented refusal. Both reads are guarded again, and both cases are pinned. THE WIDENING IS RECORDED, NOT PATCHED OVER. A deadline treats a client route that does not answer IN TIME as one that answered nothing, so a client which filters deliberately and is merely SLOW can now have the raw route consulted over it -- against the sentence in this file's own header. Refusing instead is precisely the P1, as three attempts on #1178 established. The trade is deliberate and is written at the invariant it modifies, including its bounds: only a deliberate NARROWING can be overridden, only while that client is slower than the budget, and only for a write to a type it withheld. Tests now cover the SHIPPED constant, which nothing did before -- setting it to 0 disables the bound by withTimeout's contract and previously left the whole gate green. Mutation-checked: a zero budget fails 11 tests, and giving each step the full deadline fails the shared-budget test. 4106 unit tests pass; typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): reserve a floor so the fallback is actually issued The shared budget had no floor, so a hung client route spent all of it and the #982 fallback was skipped rather than hurried -- verified by execution, fetchApi call count 0. That turned a 30s hang into a 20s refusal with the P1 intact, and the refusal then named a route nothing had contacted, which is #982's own defect. Each step now spends at most what is left minus a reserve for the steps after it; the client route may take half, so the fallback always has the other half. A step reached with no budget reports NOT_TRIED in its own words rather than borrowing the timeout's. Two more contract breaks, both proven by running the previous commit side by side, where it rejected out of a module documenting that every failure path returns defs: null: - ${responseStatus} interpolates below the guard, so Object.create(null) as a status rejected with a TypeError - describeFailure called String(err) outside every guard, so a thrown value with a throwing toString escaped The test harness advanced no clock when it fired an injected timer, so the whole suite ran in a state production cannot reach and the flagship test passed while the fallback was never sent. The harness now owns the clock and moves it on fire; with the reserve zeroed, 4 tests fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): make the fallback's budget a floor, not a leftover Subtracting a reserve left the fallback dependent on the client route finishing on time, and it does not always. Measured across small deadlines, the per-step overhead alone overran the subtraction and the fallback was skipped again (fetchApi call count 0) -- the same defect through another door. The fallback now takes max(what is left, the floor), so nothing the first step does can starve it. The response and the body share ONE end time rather than each drawing a fresh floor, so a stalled body cannot restart the clock and the worst case stays at a single deadline. The NOT_TRIED test moves off a contrived clock jump, which the floor now makes unreachable, onto a zero budget, which is a real caller-supplied input. Removing the max() fails the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): a refusal must quote the budget the step actually got Browser-verified against a live 4304-type install: the fallback was issued (fetchApi call count 1, 4304 types, ~450ms) after the client route spent its 10s half -- but the failure text read "did not answer within the 20000ms budget" for a step that waited 10000ms. Quoting a number that was never spent is #982's own defect, a refusal asserting what it did not establish. Each step now names its own share and the whole budget, so both are readable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(1161): the changelog must describe the floor, not just the shared budget Sharing the budget was the bug: a first route that never answers used all of it and the second route was never issued. The entry now says the second route is guaranteed its own share, and that a refusal reports the wait each step was actually given. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): correct a measurement I invented, then reasoned from twice The header claimed the bounded work -- one GET of /object_info -- was "measured at ~14.5s (#610)". It is not. #610 measured "the forced /object_info + combo refresh": the download PLUS registerNodesFromDefs plus rebuilding every combo widget in the graph. This oracle does none of that. The actual measurement of the thing being bounded is 5,413,770 bytes / 167 ms on a 63-pack install (#767), cited independently in object-info-cache.js, single-node-def.js and the panel's add_node path, and measured live at ~450ms on a 4304-type install. I wrote the wrong number into the source last round after a review asserted it. Later reviews then cited this comment back as the repo's measurement, and three finders used it to call the 10s per-step share a regression. It is a citation loop that turned an unmeasured figure into a fact, so the header now says which number measures what and warns against trusting either secondhand. The >= 15000 assertion rested on the same wrong figure. It now checks that a STEP's own share clears the real measurement by a wide margin, which is the property that actually matters once the budget is split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(1161): a throttled or resumed tab must not starve the fallback Chrome clamps hidden-tab timers to ~1/min after five minutes backgrounded, and sleep/resume does the same. spent() clamps a backward clock jump but must not clamp a forward one, so the timer can fire long after its bound with the deadline already overrun -- which under a subtracted reserve produced fetchApi call count 0, the P1 itself, in a tab that was merely in the background. Verified at 0/10s/60s/600s late: the fallback is issued and answers in every case. Reverting the floor to a leftover fails this and one other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): bound the call by grants, not by a clock that can lie Review found the oracle could run ~50s against a 20s deadline: spent() clamped a backwards Date.now() jump per READING, which silently refunded everything already spent, so each later step redrew a full budget. graph_set_widget waits on the history seed (8s) in front of this, so the command overran the bridge's 30s timeout and produced a bare timeout naming no routes -- the #1161 symptom itself. A high-water ratchet was tried and rejected: it samples only at step boundaries, so a jump between samples still refunds, and a test for it passed with or without the ratchet. The safety property cannot depend on now(). Each step is handed a grant, and a step cannot outlast the grant its TIMER enforces in real time, so bounding the sum of grants bounds the call whatever the clock reports. A timeout charges the full grant -- it is the one real-time measurement here that cannot lie -- which closes the stalled-clock case a new test caught immediately: three hung steps ran 9000ms against a 6000ms deadline while the clock reported zero throughout. The floor now holds by construction rather than by a max(): the client route is granted at most deadline - consumed - FLOOR, so deadline - consumed >= FLOOR when the fallback is reached is arithmetic. Also from the review: - the body's NOT_TRIED branch was untested; deleting it left the suite green while turning a resolve-always module into a TypeError rejection for callers that have no catch. Now pinned, and the mutation fails. - the four-branch outcome demux was hand-written at all three call sites, where omitting a branch THROWS ("err" in outcome on a Symbol) rather than misbehaving quietly -- a bug already shipped once this issue. One outcomeKind() now holds the dangerous test. - the header documented the deny-all exposure window as "slower than the budget" (20s) when the split caps the client route at half that. The window is twice what was written; it now says so and names the share, not the deadline. Mutations verified caught: zeroing the timeout charge, treating 0ms elapsed as unmeasurable, and removing the floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): divide the budget in advance and never consult a clock Review confirmed by execution that the previous design's headline claim was false in both directions. Only steps that TIMED OUT were charged their grant; a step that ANSWERED was still charged what now() reported, so: - a frozen clock (VM suspend, a clock-shimming extension) ran one call 49,998ms against a 20,000ms deadline; a 0.1x clock 46,091ms; one -9,990ms NTP correction 29,990ms -- all past the bridge's 30s command timeout, which is the #1161 symptom the module exists to replace. - REGRESSION: the "unmeasurable" arm charged an answering step its ENTIRE remaining grant, so one backwards or NaN reading during a healthy 450ms response left the body unread and refused the write. The parent authorized the same input. Three designs in a row tried to make an untrustworthy measurement safe, and each moved the hole rather than closing it. So this one does not measure. Every step is charged its full grant unconditionally. A step cannot outlast the grant its TIMER enforces in real wall time, whatever now() reports, so grants summing to the budget bound the call with no clock in the argument. The module no longer reads a clock at all. Shares replace the floor and cover the step the floor missed: the client route takes half, the response half of the remainder, the body the rest -- 10s/5s/5s on the shipped budget. Reserving for the fallback but not for its BODY was the same starvation defect one level down, and review confirmed a response that used everything left the body unreadable after a 200 OK. A non-finite deadlineMs made consumed infinite, budget - consumed NaN, and every later grant NaN -- and Math.max(0, NaN) is NaN, not 0, so the fallback was skipped entirely (call count 0). Normalising it to ZERO merely relocated the damage: the oracle then attempted nothing. A value that cannot be a budget now takes the shipped default; an explicit non-positive number is obeyed. Tests: a clock that THROWS proves the accounting never reads one, which no future edit can undo silently. outcomeKind is pinned directly against both Symbols, because the share model makes "not-tried" unreachable through the public API while leaving the branch live -- and a missed branch there THROWS. Mutations verified caught: not charging the grant, giving the client route the whole budget, leaving the body no share. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): tiny budgets, a null timers object, and the numbers in the comments Verified by direct execution while round 8 was still running, so these are not speculative: - Math.floor(left * share) truncated every grant to 0 on a budget under 3ms, so the oracle attempted NOTHING -- fetchApi call count 0 at deadlineMs 1 and 2, the exact signature this change exists to remove, reproduced by the arithmetic meant to prevent it. Every step now gets at least 1ms while any budget remains. - `timers: null` is not `timers: undefined`, and only the latter reaches withTimeout's own default, so an explicit null read .setTimer off null and REJECTED out of a module documented to always resolve, to two callers with no catch. - The first step's not-attempted message said "the budget was already spent" when nothing had been spent. Naming a cause that did not happen is #982's defect, and a hard-to-reach branch is no excuse for it. And the numbers a reader checks: - The header still claimed "the smallest share any single step gets is 10s" after the body was given its own share, when the smallest is 5s. That is the same stale-number trap the ~14.5s figure came from, so the arithmetic is now spelled out and a share change has to update it. - get-errors-budget.js legitimately cites 14.5s for GET_ERRORS_REFRESH_CAP_MS, because it bounds the forced REFRESH -- download plus registerNodesFromDefs plus every combo rebuild. This module bounds only the fetch. Noticing that the two files disagree is what produced the wrong number here, so the header now says why both are right. Five mutations that previously survived the whole suite are now caught: BODY_SHARE 1 -> 0.5, FALLBACK_RESPONSE_SHARE 0.5 -> 0.4, reverting the refusal strings from ${budget} to ${deadlineMs}, dropping the max(1, ...), and dropping the nullish guard on timers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): measure on a MONOTONIC clock instead of refusing to measure Round 8 found a shipped regression: charging every step its full grant stranded the time a fast step did not use. Reproduced against this branch, its parent and main -- a client route answering null in 200ms was still charged 10s, leaving the fallback 5s + 5s instead of ~19.8s, so: client 200ms, headers 300ms, body 7000ms parent/main ANSWER 7.5s, HEAD REFUSED 5.5s client 200ms, headers 6000ms, body 500ms parent/main ANSWER 6.7s, HEAD REFUSED 5.2s The second case refused with 14.8s of a 20s budget never granted to anything. The client route is normally present and fast-failing, so this was the common path. The root mistake goes back three rounds. Every scenario that broke the earlier clock-based designs -- an NTP correction, a DST or manual change, a VM suspend/resume, a frozen reading -- is a WALL-CLOCK hazard, and the default was Date.now(). The conclusion drawn was "stop measuring", when the available conclusion was "stop measuring with the one clock in the platform that may lie". performance.now() is monotonic by specification, and this repo already measures its other elapsed-time windows on it -- monotonicNow() in the panel, session-rebind.js and reconnect-staleness.js all say so. So a step that answers early now hands back what it did not use, and a step that times out is charged in full without consulting anything. Both halves are load-bearing and both are now pinned: the CAP stops a hung client route starving the fallback (the original P1), the RECLAIM stops a fast one spending time it never used. All three regression scenarios ANSWER again, at the same virtual timings main achieves. Also: reading an injected clock is wrapped, because `now` is a caller-supplied option and an unreadable clock must not reject out of a module documented to always resolve. An unusable reading charges the full grant, so the error is always in the conservative direction. And the reclaim is clamped to the grant -- a forward jump would otherwise charge a billion milliseconds and skip the fallback, the same call-count-0 signature arriving through the reclaim instead of the cap. Mutations verified caught: removing the reclaim, removing the timeout charge, unwrapping the clock read, and dropping the min(spent, grant) clamp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): normalise the clock reading, re-pin the shares, correct an overclaim Round 9 candidates, three fixed and one deliberately rejected. FIXED -- a clock returning a Symbol or a null-prototype object REJECTED out of this module. The clock CALL was guarded; the arithmetic on its result was not, and `readClock() - startedStep` throws for both. That is the third time this issue a guarded read was undone by an unguarded USE of the same value, after ${responseStatus} and String(err), so the reading is now normalised to a finite number at the source rather than at each use. FIXED -- BODY_SHARE 1 -> 0.5 survived the suite again. The body's grant halves from ~20s to ~10s and the assertion only required > 9000ms, which accepted both. Tightened to see the share it is meant to pin. FIXED -- the floor comment claimed max(1, ...) makes every budget reachable. It does not rescue deadlineMs=1: one millisecond cannot be divided across three steps that each need at least one, so the client route takes it and the fallback is not reached. Arithmetic rather than a bug, but the overclaim is the kind of statement that later gets cited as established. REJECTED, with the reasoning recorded in the source -- a timed-out step is charged its nominal grant, so a LATE-firing timer (a hidden tab clamped to ~1/min after five minutes, or a laptop resume) lets real elapsed exceed the budget. True, and charging the measured overrun instead was tried: it makes the accounting honest and the OUTCOME worse. A tab backgrounded for ten minutes then reaches the fallback with a zero budget, so the write is REFUSED outright rather than retried by the route that still works -- caught by the existing late-timer test, which is what that test is for. The command has already blown its 30s budget in that world; the useful thing left is to ask, and the fallback usually answers in ~450ms. Preserving a number the environment has already broken, at the cost of the one route that can still answer, is the same trade that produced the "refuses what main serves" regressions in rounds 7 and 8. Mutations verified caught: each of the three shares, and dropping the clock normalisation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): the design comment described a design this no longer has The rationale block above runStep still said "no clock is consulted", "this one does not measure at all", and "EVERY STEP IS CHARGED ITS FULL GRANT, UNCONDITIONALLY" -- all true of the previous commit and none of this one. In a module whose comments are its design record, that does not merely go stale; it tells the next maintainer the opposite of what the code does. It now records all five attempts, including the fourth (stop measuring) that shipped a regression, and states the conclusion the first four missed: every scenario that broke them is a WALL-CLOCK hazard, and the default was Date.now. Where the design still depends on the clock is stated rather than glossed: the reclaim believes a clock that ADVANCES, so one that under-reports without going backwards charges an answering step too little. performance.now() cannot do that, which is why it is the default and why `now` is a test seam. Newly pinned, both of which review found unasserted: - The DEFAULT clock. Swapping it to Date.now left the whole suite green while being the single choice this design rests on. A source guard, in the idiom the #982 refusal wording already uses, since every test overrides the default. - The unmeasurable-elapsed arm. Flipping it to charge zero also left the suite green, so a clock that could not be read would have handed every later step a fresh budget -- the refund defect behind the ~50s overruns. Also: runStep's docstring claimed a full charge that is now true of one branch only, and the min(left, ...) in the grant is marked as a guard for a future share rather than something that binds for any share defined today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): a budget past the timer's range must not invert into no bound setTimeout coerces a delay above 2^31-1 to 1ms. Verified: at deadlineMs of 3e9 or MAX_SAFE_INTEGER the grants exceeded that ceiling, so with a real timer every step would fire almost immediately -- the bound silently becoming NO bound, which is exactly the unbounded await this module exists to remove, reached by asking for MORE time rather than less. The budget is now clamped to what a timer can express, which honours a caller asking for "very long" (2^31-1 is ~24.8 days) rather than quietly inverting it. This sits with the existing normalisation: a non-number takes the shipped default, a non-positive number is obeyed, and an unrepresentable one is clamped. Removing the clamp fails the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(1161): the real-elapsed test could not see the overrun it was named for It summed only FIRED timer durations, so a step that ANSWERS -- which costs real time too -- contributed nothing, and the test passed regardless of what the reclaim charged. Review confirmed the gap by running the ordering it could not see: with a frozen clock, "client answers at 9999ms, response answers at 9999ms, body hangs" runs 39,998ms against a 20,000ms budget. Each answering step now advances the clock by exactly what it took, so the reclaim is charged against real work. Two orderings are pinned: three honest slow steps (18s inside a 20s budget) must ANSWER without adding up past it, and a hung client route followed by a slow-but-working fallback must still recover. A first attempt drove this with a polling virtual clock and did not terminate inside 120s; the deterministic version needs no timers for steps that answer. The frozen-clock overrun itself is NOT closed, and the module says so where it matters. The reclaim believes a clock that ADVANCES. performance.now() guarantees that and is the default; a clock that under-reports without going backwards is reachable only through the injected test seam or a platform with no performance object. Closing it means charging the nominal grant, which is exactly the change round 8 proved refuses payloads main serves -- so the exposure is recorded rather than traded for a worse one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(1161): the changelog repeated the measurement error it should not The entry said the budget is sized against the real payload 'rather than against a fast local reply'. That framing came from the ~14.5s figure I wrongly attributed to the download; the 167ms measurement IS the payload transfer, and 14.5s measures the forced refresh (download plus registerNodesFromDefs plus every combo rebuild). Leaving the sentence in would have shipped the same wrong premise to users after correcting it in the source. It now states the budget plainly and says the fetch measures well under a second on a large install, which is what was actually measured. Also adds the reclaim: a route that answers quickly hands back the time it did not use, which is what lets a slow install still finish inside the budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): the clock normalisation disabled the reclaim it was meant to protect Round 10 found that round 9's fix regressed the thing round 8 fixed. Demanding `typeof reading === "number"` rejected every clock the subtraction itself would have accepted -- a `now` returning a Date, or a numeric string -- so those read as unmeasurable, charged the full grant, and silently turned the reclaim off. Measured: grants fell back to 10000/5000/5000, which is exactly the round-8 regression shape, reintroduced by a fix for something else. The guard now coerces exactly as the arithmetic would, inside the try, so it accepts what worked before and rejects only what actually throws (Symbol, null-prototype object). Verified: Date, numeric string and plain number all keep the reclaim; Symbol and null-proto still resolve rather than reject. Also fixed, all found by review and confirmed by execution: - The comment retracting an earlier overclaim installed a NEW false claim: it said deadlineMs=1 cannot reach the fallback. It can, and does -- a client route that returns instantly hands its grant back. Two revisions of that comment were wrong in opposite directions, neither checked before being written. - The "DEFAULT clock is monotonic" source guard was VACUOUS for the exact mutation its own comment names: `performance.now` also appears in the rationale and the capability check, so a bare identifier match stayed true when the arm became Date.now. It now pins the arm. - That guard's Date.now counter also never stripped comments, because the file is CRLF: a line ends "\r", and `//.*$` cannot match there. The strip silently did nothing. - Deleting the timeout charge left the suite green -- including the test rewritten to see that overrun. Pinned by its observable consequence: a hung step that costs nothing leaves the fallback the whole budget instead of what is left. - FALLBACK_RESPONSE_SHARE was left free when BODY_SHARE was re-pinned; 0.5 -> 0.9 survived. Both are pinned now. - The 2^31 clamp boundary was unpinned: at a budget of exactly 2^31 the largest grant still lands under the ceiling, so setting the cap to 2^31 survived every grant assertion. Pinned through the budget the refusal quotes, which is the one observable that distinguishes them. Six mutations verified caught, three of which survived before this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): reading the injected timers object could itself throw bounded-step.js's header argues at length that every injected guard is an operation that can fail, and wraps onTimeout and clearTimer accordingly. Reading the injected object was the one case it missed: `timers.setTimer` on a throwing getter, or on a Proxy whose get trap throws, threw SYNCHRONOUSLY -- before the returned promise existed -- so withTimeout rejected out of a function whose contract three lines above its signature says it never does. That contract is what the /object_info oracle relies on to promise the same thing to its own callers, and two panel commands await it with no catch. Verified: both shapes rejected before this, both resolve now, and a timers object that cannot be read falls back to the real timer -- the same answer as supplying none, and always safe. A present-but-not-callable setTimer is treated the same way rather than being called and throwing later. Fixed in the primitive rather than in the oracle because the contract is stated there and the defect violates it there; media-preview.js and run-completion-frame.js pass the same object through the same read. The mutation that unwraps the read fails the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1161): clamping an over-range budget parked the call instead of bounding it The 2^31 clamp added last round made things worse, not better. setTimeout coerces a delay above 2^31-1 to 1ms, so an over-range budget hands every step a grant that fires immediately -- the bound becoming no bound. Clamping to 2^31-1 removed that, and replaced it with a ~24.8-DAY grant. Measured on real timers at deadlineMs 5e9 with a hung client route: before the clamp the call answered in 3ms with the fallback issued; with the clamp it never returned at all, fetchApi call count 0. That is the canonical #1161 signature, reintroduced by a guard written to prevent it -- the second time in two rounds a fix for a review finding produced the defect it was fixing. A nine-digit millisecond count is not a budget a caller can have meant, so it now takes the shipped default like any other unusable value. A budget AT the ceiling is expressible and is honoured, which is the caller's to choose. Both directions are pinned so neither fix can be undone in favour of the other: an over-range budget must yield the DEFAULT grant and still issue the fallback, and a hung route on such a budget must report rather than park. The mutation back to clamping fails the suite. The clamp test also asserted an outcome its own harness made impossible -- it fired every timer immediately, so no step could answer, while expecting a schema. It now asserts what that harness can actually show: the fallback is REACHED and the refusal quotes the budget in force. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 tasks
… would have broken 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 7de88a6 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 #1161
Owner
Author
Round 4: main answered this, and shipping the branch as-written would now undo itRound 3 recommended putting the bound in the oracle rather than the cache. That happened: #1179 landed So the question is no longer "does this fix #1161". It is "what does this branch still do on top of a fixed main", and I measured it rather than reasoning about it. Measured: the cache bound pre-empts the fallback routeSame input both runs — the #1161 shape,
This is round 3's finding, now with a number on it. A cache-level bound has to be shorter than the oracle's deadline to fire at all, so it can only ever fire before the fallback route #982 added for exactly this failure is asked. It does not choose how fast to fail; it chooses to fail where main succeeds. Measured: it also disables #1223's snapshot fallback, and misreports whyThe branch's timeout outcome carries no
which in a timeout is the one thing that did not happen. That is #982's own defect — a refusal asserting a cause it never established — committed a layer up. What I did with the branchMerged
Each was mutation-tested by breaking the thing it guards: re-applying the branch's cache (kills 2), removing the oracle's per-transport bound (kills 1 and 3), dropping the RecommendationLand these guards or close the PR — either is fine, and the code that was under review here should not ship. The value left in this branch is that the next person to reach for a cache-level bound now gets a failing test with the reason, instead of a fourth rediscovery. Refs #1161 |
artokun
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs artokun/comfyui-mcp-panel#1161— no longer open as of 2026-08-13, via #1179(
1759e2a9), released in 0.14.25. The oracle-layer bound is inmainand is the shippingbehaviour.
What is actually in this branch now
Test-only against
origin/main. Every production hunk the earlier revisions carried — theOBJECT_INFO_READ_WAIT_MSbound inobjectInfoCache.read(), and aCHANGELOG.mdentry for aversion that never shipped it — has been dropped.
web/js/lib/object-info-cache.jsisbyte-identical to
main, and still carries no#1161reference, because the whole fix livesone layer down in
object-info-oracle.js.npm run test:unit: 4411 pass, 0 fail, 1 todo (4412 tests), merged up tomainatafb8db64.What the three guards pin
They are written against the cache's behaviour under a call, not against its source text, so
a rename cannot slip past them.
#1178/#1179: a hung client route still reaches the fallback THROUGH the burst cache.Builds the production loader shape exactly as
graph_set_widgetbuilds it — the oracle readthrough this cache — with
getNodeDefsthat never settles and a workingGET /object_info.Asserts the fallback route is asked (
httpCalls === 1), that its schema reaches the fence,and — the load-bearing part — that the release happens at the client route's share of the
oracle budget. Elapsed time is asserted on an injected clock, so it proves which layer's
bound answered rather than merely that an answer arrived.
#1178: the burst cache arms NO timer of its own.The decisive one. Counts realsetTimeoutcalls across an issuer read and a joining read. Re-introducing a bound inread()— for either — arms a timer here and fails with the reason attached.#1178/#1223: a fully silent backend rides through the cache as SILENCE, not as an answer.With neither route answering, the oracle's outcome must survive the cacheintact —
defs: null, every unanswered route named, and the transport tags still readable bynoBackendAnswerEstablished(). panel_set_widget refuses live edit when backend schema probe times out #1223's snapshot fallback is licensed on exactly thatdistinction, so a cache that substituted a note of its own would disable that fallback for
every caller and make the refusal name a cause that never happened.
Mutation-verified, both directions
Promise.racewrapping both the issuerand the joiner in
read()) → guard 2 fails. Guards 1 and 3 survive this one, and honestlyso: their subject settles in simulated time, so a real 8s timer never fires inside a
millisecond-long test.
return payloadin place ofreturn defs) → guards 1and 3 both fail.
Each guard is therefore killed by at least one mutation; neither mutation is caught by the
#716tests that were already there.The record: three approaches that were ruled out
Preserved as commits rather than as prose, because the diffs are the evidence:
90990a39— bound the issuer, release theinflightslot. Missed every joiningreader: the early return handed out the raw unbounded promise, so a burst after a restart —
the case Add bulk widget writes to avoid repeated full object_info fetches #716 built this cache for — was unfixed. Also broke the one-request-per-generation
invariant and returned a bare
null, which computes an empty failure list at both call sites.8689f2bc— bound both, retire by generation. Retiring discarded the fetch's own latepayload, so a merely slow backend (remote, tunnelled, large
/object_info) became apermanent refusal — worse than the bug.
084eab8c— bound both viabounded-step.js, do not retire. No longer discards the latepayload, but converts a 30s hang into a permanent 8s-per-call refusal: 29 widget writes cost
29 × 8s and all refuse, with refusal text advising a retry guaranteed to fail.
Why the layer is wrong
fetchWholeObjectInfohas two transports:getNodeDefs()(the frontend client route) and adirect
GET /object_info, added by #982 for exactly this failure — a client route that stopsworking while the HTTP endpoint answers fine.
A bound above the oracle can only choose how to fail. It cannot advance a hung
getNodeDefs()to the fallback, because the oracle still awaits that first transport. And tofire at all it must be shorter than the oracle's deadline — which is precisely what pre-empts
the fallback before that route is ever asked. Measured on this branch's code against the
production loader shape: refused at 8003ms, where
mainanswers with a usable schema at10010ms. The refusal also printed
Tried one routewhile the second route had never beenattempted.
Bounding inside the oracle, per transport, both stops the hang and makes the fallback
reachable, so the widget write succeeds rather than being refused. That is #1179:
OBJECT_INFO_DEADLINE_MS = 20000spent down per step, with a reserved floor guaranteeing thefallback is still issuable when the client route is the thing that hung. Its live verification
measured the real #1161 state end to end at 11.7s, fallback issued, 4304 types returned — which
an 8s cache-level bound would never have seen.
Related work that did land
Refs artokun/comfyui-mcp-panel#1161)api.getNodeDefs()call sites,graph_add_node/panel_refresh_nodes(graph_add_node and panel_refresh_nodes still hang on the half-open socket #1161 fixed for set_widget #1180)/object_infotimeout must not refuse a safe widget edit (panel_set_widget refuses live edit when backend schema probe times out #1223)Disposition
Two defensible outcomes, and the maintainer should pick:
mainas atest(...)commit. Itis 200 lines, adds no production surface, and gives the next person who reaches for a
cache-level bound a failing test with the reason instead of a fourth rediscovery.
three ruled-out approaches survive only as an unreferenced branch.
Recommendation: land the guards. The mutation results above show they catch the exact regression
the three attempts each walked into, and #1178's real cost was that nothing in the tree said the
bound could not live in the cache.