fix(1161): bound each /object_info transport so a hung route falls through to the one that works - #1179
Conversation
…k is reachable 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.
…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>
Review outcome: right layer, real hole — 11 CONFIRMEDThe layer choice holds: bounding each transport is what makes the #982 fallback reachable, and reviewers confirmed it repairs three commands rather than one ( The deny-all override (CONFIRMED, reproduced)This module's header states the fallback is consulted only when the client route returned NOTHING usable, "so it can never override a narrower answer the client actually gave". A timeout breaks that. Reproduced with injected timers: a That is a genuine trade, not an oversight to fix quietly:
The honest resolution is probably to fall through and record the widening explicitly, which is exactly what this file already does for the analogous TTL race ("a real widening of an existing race… recorded here rather than waved past"). But that is a deliberate weakening of a stated guarantee, so it belongs in the open rather than inside a fix for something else. Also confirmed
Where this leaves itStill draft. The diagnosis and the layer are settled; what remains is one deliberate decision (accept and document the narrowing-override widening, or keep the invariant and accept a refusal) plus four mechanical corrections. I am not making that call inside a fix for a different bug. |
|
(Correcting the comment above: I posted it through a shell heredoc and the backticks were executed as command substitution, so three bullets lost the code identifiers they named. The full text follows — the mangling is mine, not the review's.) Review outcome: right layer, real hole — 11 CONFIRMEDThe layer choice holds: bounding each transport is what makes the #982 fallback reachable, and reviewers confirmed it repairs three commands rather than one ( The deny-all override (CONFIRMED, reproduced)This module's header states the fallback is consulted only when the client route returned NOTHING usable, "so it can never override a narrower answer the client actually gave". A timeout breaks that. Reproduced with injected timers: a That is a genuine trade, not an oversight to fix quietly:
The honest resolution is probably to fall through and record the widening explicitly, which is exactly what this file already does for the analogous TTL race ("a real widening of an existing race… recorded here rather than waved past"). But that is a deliberate weakening of a stated guarantee, so it belongs in the open rather than inside a fix for something else. Also confirmed
Where this leaves itStill draft. The diagnosis and the layer are settled; what remains is one deliberate decision — accept and document the narrowing-override widening, or keep the invariant and accept a refusal — plus four mechanical corrections. I am not making that call inside a fix for a different bug. |
Addendum — I understated two of theseThe full review returned 22 verified findings across eight defects. Two are worse than I described. The bound is calibrated against the wrong measurement, and the repo already measured the right oneI justified 6000ms by citing So 6s does not merely risk refusing a slow install. It refuses a normal one. And because
That is my error twice over: I reasoned from a number that measured something else, and I wrote a comment asserting the headroom was "generous rather than tight" on that basis. The right bound has to come from the payload measurement and the caller's real budget, not from matching the startup seed's 8s by analogy — which is the same reasoning-by-similarity that produced three dead ends on #1178. fetchWholeObjectInfo can now REJECT, and two callers have no catchI described this as a throwing-accessor edge case. It is a contract break, proven by running
The file states the contract two screens up — "every failure path returns
Where this leaves the branchNot mergeable as written, and the remaining work is now specific rather than exploratory:
The layer is right and the diagnosis has held across four rounds. The calibration and the contract are what remain, and both are mine. |
…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>
Round 5: 18 CONFIRMED. I am stopping here rather than attempting a sixth.Every candidate I flagged was confirmed, several by running Why I am handing this over instead of continuingRounds 1–3 bounded the cache; each version traded one permanent-refusal case for another. Round 4 moved the bound to the oracle — the right layer — and broke the resolve-always contract. Round 5 fixed the calibration and the contract, and:
That is not a fix converging on correctness. It is one that finds a new invariant every round, in a file whose header documents four of them and whose siblings document more. What I believe the right next step isNot another revision of this branch. The evidence says:
What ships from this issue regardlessNothing yet, and that is the honest outcome. The diagnosis is solid and has held across all five rounds: Both branches stay open as the record: #1178 for the three cache-layer approaches that were ruled out, and this one for the layer that is right and the four invariants it has to satisfy at once. |
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>
Correcting my own handoff: the branch did not fix the P1, and my test hid thatThe comment above says "the layer is right and the diagnosis has held across four rounds," and that the calibration and the contract were all that remained. That was wrong about the thing that matters most, and round 5 proved it by execution rather than by reading. The fallback was never issued at allThe single shared 20s deadline was consumed entirely by the hung client route. So the branch converted a 30s hang into a 20s refusal with the P1 fully intact. Worse, the refusal then read My flagship test passed in a state production cannot reachThe harness fired the injected timer without advancing the clock What changed in
|
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>
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>
Browser-verified against a live install — the fallback is now actually sentRun in the ComfyUI page against the served bundle (the junction serves this working tree, so this is the branch code), on a 4304-type install with the real ~5MB payload.
The first row is the one that matters: the previous commit produced What the live run caught that the suite did notThe hung-route refusal read "did not answer within the 20000ms budget" for a step that waited 10000ms — the client route is capped at half. Quoting a number that was never spent is #982's own defect, a refusal asserting what it did not establish, and I had reintroduced it in the message while fixing it in the logic. Fixed in Also on the branch since the correction above:
Full suite: 4112 pass, 0 fail. A review of these commits is running. The two open decisions from the correction are unchanged and still not mine to make here: the |
…d 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>
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>
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>
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>
Round 6: the clock stopped being the safety mechanismThe previous review found the oracle could run ~50s against a 20s deadline: I tried a high-water ratchet first and rejected it: it samples only at step boundaries, so a jump between samples still refunds — and the test I wrote for it passed with or without the ratchet, which is how I caught that it wasn't a fix. What replaced itThe safety property no longer reads the clock. Each step is handed a grant, and a step cannot outlast the grant its timer enforces in real time regardless of what
Also fixed from that review
Live verification (4304-type install, real payload,
|
| client route | fallback issued | elapsed | types | recorded failure |
|---|---|---|---|---|
| healthy | 0 | 345ms | 4304 | (none) |
| throws | 1 | 367ms | 4304 | threw: half-open socket |
| hangs (the P1) | 1 | 11.7s | 4304 | did not answer within its 10000ms share of the 20000ms budget |
10s for the client route's share plus ~1.7s for the fallback, inside the 30s command timeout, and the healthy path still costs no second request.
Mutations verified caught: zeroing the timeout charge, treating 0ms elapsed as unmeasurable, removing the floor, dropping the body branch. Suite 4115 pass, 0 fail.
Still open, still not decided here
The usableDefs divergence from main on an uninspectable payload, and the deny-all widening the header documents. Both are deliberate and both are recorded; neither belongs to me to settle inside a fix for a different bug. The sibling unbounded call sites are now #1180.
A review of this redesign is running. Not merging until it clears — the last two rounds each found a P1 in code I had already called correct.
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>
Round 7: five CONFIRMED, and the fix was to stop measuringThe review of the grant-accounting commit confirmed by execution that its headline claim was false in both directions. Only steps that TIMED OUT were charged their grant; a step that ANSWERED was still charged whatever
And a regression in the other direction: the "unmeasurable elapsed" 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 identical input. What changedThree designs in a row tried to make an untrustworthy Every step is charged its full grant, unconditionally. A step cannot outlast the grant its timer enforces in real wall time, whatever Shares replace the floor, and cover the step the floor missed: client ½, response ½ of the remainder, 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; review confirmed a response that used everything left the body unreadable after a 200 OK. A non-finite The tests were vacuous for the third time this issueThe clock test could not fail for the reason it named — every step in it timed out, where the charge was already clock-independent — and the load-bearing The suite now passes a clock that throws. If the call resolves, the accounting provably never read it, and no later edit can reintroduce a clock read silently. Re-measured
Fallback issued in all four. Live against the 4304-type install: healthy 777ms / 0 extra requests; throws 350ms; hangs 10.5s with 4304 types recovered; hangs + throwing clock 11.0s, same result. Suite 4117 pass / 0 fail. Mutations verified caught: not charging the grant, giving the client route the whole budget, leaving the body no share, dropping a demux branch.
A review of this commit is running. Still draft. |
… 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>
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>
Round 8: a shipped regression, and a mistake three rounds oldCharging every step its full grant bounded the total but stranded the time a fast step never used. Reproduced by execution against this branch, its parent, and
The second 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 third consecutive round where the regression ran in the same direction: refusing what The mistakeEvery scenario that broke the three earlier clock-based designs — an NTP step correction, a DST or manual change, a VM suspend/resume, a frozen reading — is a wall-clock hazard, and the default was I concluded "stop measuring." The available conclusion was "stop measuring with the one clock in the platform that is allowed to lie."
What ships now
Also fixed this round
VerifiedAll three regression scenarios ANSWER again at Suite 4122 pass / 0 fail. Six mutations verified caught: removing the reclaim, removing the timeout charge, dropping A review of this commit is running. Still draft. |
… 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>
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>
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>
…med 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>
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>
…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>
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>
…ounding 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>
… 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
Closes #1161. Replaces #1178, which is left open as the record of three approaches that were ruled out.
The bug
After a ComfyUI restart the tab can hold a half-open connection, so
api.getNodeDefs()never settles — it does not throw, it simply never answers. Every await in the/object_infooracle was unbounded, so it parked on the first transport forever. Every command that consults it hung until the caller's 30s timeout:graph_set_widget,graph_remove_widget, andgraph_get_object_info(which calls the oracle directly, with no cache in front).fetchWholeObjectInfoalready had a second transport — the rawGET /object_info, added by #982 for exactly this failure, and the very request the original reporter ran by hand to prove their backend was fine. It was never reached, because nothing gave up on the first one.Why the bound sits here and not on the cache
Three rounds on #1178 established that bounding the cache cannot fix this: a bound one layer above the oracle can only choose how to fail, so the reported hang stayed in place under a different name. Bounding the transports is what makes the fallback reachable, which turns a hang into a successful write.
What ships
A single deadline for the whole question (
OBJECT_INFO_DEADLINE_MS = 20000), spent down by each of the three I/O steps (client route, response, body) — not a per-step bound, which would multiply into a worst case nobody chose.The accounting deliberately does not trust the clock:
now()reports.min(measured, grant);0is measurable, negative/NaN is charged in full.deadline - consumed - FLOOR, sodeadline - consumed >= FLOORwhen the fallback is reached.Refusals name each step's own share rather than the whole deadline, because quoting a wait that never happened is #982's own defect.
What it took to get here, recorded honestly
Six review rounds. The last three each found a P1 in code I had already called correct:
fetchApicall count 0. The suite was green because the test harness fired injected timers without advancing the clock, a state production cannot reach.Separately, a
~14.5spayload figure in the header turned out to measure/object_infoplus the combo refresh, not the download. The download is 167ms (#767). I introduced that number, and later reviews cited my own comment back as the repo's measurement. The header now says which figure measures what.Verification
Live, against a 4304-type install with the real payload and
fetchApicounted:Unit suite 4115 pass / 0 fail. Mutations verified caught: zeroing the timeout charge, treating 0ms elapsed as unmeasurable, removing the floor, dropping the body's not-attempted branch.
Knowingly left open
usableDefsdiverges frommainon an uninspectable payload (mainreturns the fallback's schema, this returnsdefs: null). Fail-closed is right, but it is a behaviour change and should be read as one.api.getNodeDefs()call sites —graph_add_nodeandpanel_refresh_nodesstill hang on the same socket. Filed as graph_add_node and panel_refresh_nodes still hang on the half-open socket #1161 fixed for set_widget #1180 rather than folded in here, becauseadd_nodeuses a per-class routeset_widgetstructurally cannot.