fix: a governance server that hangs is worse than one that errors (#130, #131, #132) - #134
fix: a governance server that hangs is worse than one that errors (#130, #131, #132)#134c-1k wants to merge 13 commits into
Conversation
… hanging (#132) `/v1/authorize` awaited `pool.get()` and `governor.authorize()` with no deadline. The ledger client underneath has no request timeout and never rejects an unreachable cluster — it retries forever — so a dependency outage arrived here as a promise that simply never settled, and the endpoint stopped answering at all. From the socket that is indistinguishable from a governor that is merely slow. `usertrust-claude-code` resolves the ambiguity by failing CLOSED, so an unbounded wait there is not a slow tool call but a blocked one: the outage propagates into the client instead of being reported by the server. `close()` had the same defect and it is worse: `destroyAll()` awaited the same never-settling construction promise, so a server whose ledger was unreachable could not shut down either. One `withDeadline` in `deadline.ts` rather than a copy per call site — a per-site copy is N chances to get the bound wrong and N things to keep in step. Also fixes a trap this opens: shadow mode swallowed anything that was not exactly 500, so the moment a 503 existed an infrastructure outage would have been reported to the caller as a clean 200 "would_deny" — a dependency outage laundered into a policy opinion. Only governance verdicts (4xx) are shadowed now. settle/abort deliberately bound only governor CONSTRUCTION, not the settle/abort call: a timed-out settle has an unknown outcome on the money path, and reporting it as failed would invite a double-settle. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
…ails loud (#130) ROOT CAUSE of the `/v1/authorize` hang. `createGovernor({ dryRun: false })` — the default, and the only thing a real deployment runs — never returned. `tigerbeetle-node` exposes NO request timeout (`ClientInitArgs` is `cluster_id` + `replica_addresses`, nothing else) and treats an unreachable or unresponsive cluster as transient: it retries the handshake forever and its promise NEVER rejects. `createTBEngine` awaited that promise, so `createGovernor()` never settled and every request behind it hung. The consequence worth naming: the `catch` that wraps engine creation in `LedgerUnavailableError` — carrying the exact hint an operator needs — was UNREACHABLE in the single most likely production misconfiguration, because there was no rejection to catch. The error existed; nothing could raise it. Measured, all against the real native client: healthy cluster (0.17.9) createTreasury OK 0.7s nothing listening on the port retries ConnectionRefused never settles a FOREIGN listener on the port silent never settles version-mismatched cluster rejects "client evicted" 0.7s So only the reachable-but-wrong cases ever failed loud. A caller-side deadline is the only mechanism the client leaves available; `tigerbeetle.connectTimeoutMs` (default 5s) is the knob, and a timed-out client is destroyed rather than left retrying on its own handles behind a caller that has already given up. Applied verbatim to BOTH governors — `tests/harden/engine-factory-parity.test.ts` compares them as source text and fails on a one-sided edit. The new test drives the REAL native client against a closed port; a mock cannot reproduce a defect that lives in the client's own retry behaviour. It fails against the unfixed build by hanging out to its 20s ceiling. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
The finding underneath #130. `tests/integration.test.ts` was the only end-to-end coverage of a real governor and it read "integration: real governor in dryRun mode"; every other server test injects `createFakeGovernor`. `dryRun` defaults to false. So the one configuration exercised end to end was the one users do not get, the suite was green, and a total hang in the primary endpoint shipped. Covers the default config in BOTH directions, because one without the other is not coverage: - ledger unreachable -> 503 ledger_unavailable, bounded (integration.test.ts). Fails against the unfixed build by hanging to its ceiling. - ledger REACHABLE -> authorize/settle/budget over a real cluster (server.tb.test.ts). Without this the failure-path test alone would be satisfied by a deadline that refuses everything — a guard that always fires passes every failure-mode test ever written. The real-cluster half self-skips on absent USERTRUST_TB_ADDRESS and is named in the tb-integration CI job, matching the existing `.tb.test.ts` convention. It is the only place the server drives a real governor over a real ledger. Also pins that a ledger outage is NOT shadowed into a 200 would_deny under evaluate_only. Swept for the same shape elsewhere — fake-by-default with the real dependency only under a non-default flag. `packages/openclaw` looks identical to a grep but is not: its real-cluster path is covered by `envelope-integration.test.ts` in the same CI job. The server was the only package with no real-ledger coverage in any job. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
The README quickstart config carried no `dryRun` and no TigerBeetle, so following it verbatim reproduced #130: a server that answers /v1/health and nothing else. Says outright that the default needs a reachable cluster, what the two timeouts bound, how they must be ordered against each other and the client's own, and that 5xx is never shadowed under evaluate_only. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
A deadline ABANDONS an operation; it does not cancel it. Both new deadlines therefore had the defect their own fix was written to prevent — something real was created and then became unreachable. P1 (server.ts) — an `authorize()` that completes after the deadline still takes a ledger hold, and its transferId reached nobody: the client got a 503, so no settle or abort can ever name it. AGENTS.md gives every hold exactly one terminal outcome and makes no exception for "the server stopped waiting". Left alone, each timed-out authorize permanently retires part of the tenant's budget, and a retry loop against a slow ledger exhausts the budget while every request reports a timeout — a hang traded for a slow leak. P2 (pool.ts) — a governor that resolves after `destroyAll()` gave up was never destroyed. AGENTS.md is explicit that an undestroyed TigerBeetle client is what keeps the event loop from draining, so that leak can stop the process exiting: the shutdown fix would have reintroduced the shutdown hang by another route. Both are the same shape, so both are fixed in one place: `withDeadline` takes an `onAbandoned` continuation, attached BEFORE the race so nothing lands in the gap. A late REJECTION is swallowed deliberately — it produced nothing to reclaim, has no listener left, and would otherwise take the process down over a failure the caller was already told about. Mutation-verified by name: removing only the reclamation (leaving the deadline intact) fails exactly the two new tests and nothing else. Found by Codex review of c86a50b. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
Codex gate — iteration roundVerdict: "The new deadlines can abandon successful late operations, leaking pending holds and live governors. These affect money accounting and shutdown behavior." Not a no-op review — it named every changed file ( Findings — both accepted and fixed in 6a5ccb8Both are correct, and both are the same defect class as the bug this PR fixes: a deadline abandons an operation, it does not cancel it, so something real was created and then became unreachable. I verified each cited invariant in [P1] [P2] Fixed in one place rather than two, since it is one shape: Mutation-verified by name: removing only the reclamation, leaving the deadline itself intact, fails exactly "voids an authorization that lands AFTER the deadline gave up on it" and "destroys a governor that resolves after the shutdown deadline" — and nothing else. The surgical mutation is the point: the deadline tests still pass, so the two new tests are pinning the reclamation specifically. Gates after the fix: A max-effort certification round on the new SHA follows. |
…for direct callers Two findings from the max-effort Codex certification of 6a5ccb8. Both verified against the code rather than accepted on the citation. P1 — teardown that RESURRECTED what it tore down. Closing the client makes tigerbeetle-node reject the in-flight request with "Client was closed.", which `isConnectionError` correctly classifies as a connection error — so `withReconnect` built a FRESH client and retried. The `destroy()` added to clean up a timed-out handshake therefore left a brand-new client hammering the same dead cluster, and AGENTS.md is explicit that an undestroyed TigerBeetle client is what keeps the event loop from draining: the cleanup could stop the process exiting. This was already visible and I had missed it — a live server run logged `[TB] Reconnection attempt 1/5` immediately after answering 503 for an unreachable ledger. Nothing had asked it to reconnect. Destruction is now terminal, checked both in `reconnect()` and in `withReconnect`. Three existing tests destroyed a client purely to stop its health-check interval and then called `reconnect()` on it. That contract is exactly what is now invalid, so they keep a live client instead and destroy at the end — the interval is 30s and they advance 16s, so it never fired for them anyway. Their subjects (backoff exhaustion, dedup) are unchanged. P2 — `createUsertrustServer` and `GovernorPool` are exported, so a caller can pass a config that never went through `ServerConfigSchema`, including one written before `requestTimeoutMs` existed. Only `loadServerConfig` applies schema defaults, and `setTimeout(fn, undefined)` fires on the next tick — so adding the field would have BROKEN the programmatic path rather than defaulting it, turning every request into an instant `governor_timeout`. `requestTimeoutOf()` applies the default at both boundaries. Both mutation-verified by name, and the first attempt at each was a FALSE PASS worth recording: - mutating one of the two destroy guards left the other catching it, so the test passed against "broken" code. Only removing both reproduces the defect. - the timeout test first used the instant fake governor, which wins a 1ms race, so a missing default was invisible. It now takes 60ms, like any real ledger round trip, and fails in 5ms without the fallback. Re-ran the full tb-integration job against a real TigerBeetle 0.17.9 after changing shared ledger-client code: 4 files, 12 tests green. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
Codex gate — max certification roundVerdict: "Timeout cleanup can resurrect and leak the TigerBeetle client it intends to stop. The new server timeout also lacks a runtime default on the public programmatic path." Named every relevant file ( Findings — both accepted and fixed in 7ac5b12[P1] Teardown resurrected what it tore down. Closing the client makes tigerbeetle-node reject the in-flight request with This was already in my own evidence and I read past it. A live server run in the PR description logged Three existing tests in [P2] The new field would have broken direct callers rather than defaulting for them. Both mutation-verified by name — and both first attempts were FALSE PASSESRecording these, because each would have shipped a test that cannot fail:
A single-layer mutation against a defense-in-depth fix is not a control. Checking that is the only reason either of these is a real test. Gates on 7ac5b12
A further max round on 7ac5b12 follows, since the remediation touched the shared client. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ac5b123d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ts, order the timeout chain Three findings from the chatgpt-codex-connector review of 7ac5b12. All three verified against the code first. [P1] close() awaited the best-effort abort of every pending hold BEFORE pool.destroyAll(), and that abort was deliberately unbounded. So a stalled ledger did not merely fail to void a hold — it stopped teardown from ever reaching the governor destroy that would have voided it anyway, hanging shutdown. The /v1/abort ROUTE keeps its unbounded abort, because there a caller is waiting to be told the outcome; on the shutdown and sweep paths nobody is waiting and the catch already swallows. [P2] `destroyed` was checked only on ENTRY to reconnect(). _doReconnect sleeps up to 8s between attempts, so a destroy() landing mid-backoff was undone by the next iteration assigning a fresh native client — teardown completing while the client it tore down came back. Re-checked every iteration. [P2 — from the #135 review, and the one that mattered most] THE TIMEOUT CHAIN WAS NOT MONOTONIC, so the labelled error lost a race to the generic one 100% of the time. These defaults were chosen as "below the client's 15s timeout", but that 15s is usertrust-claude-code's hook PROCESS timeout; its HTTP request aborts at 5s (hooks/lib.mjs:168). A 5s ledger deadline answered at ~5.03s, so the client had already given up and the user got a generic transport error instead of the labelled 503 this entire PR exists to produce. Corrected to a chain that is monotonic with headroom: connectTimeoutMs 3s < requestTimeoutMs 4s < client HTTP 5s < hook process 15s Measured against a real server with the plugin's actual 5s bound (`--max-time 5`): HTTP=503 in 3.03s carrying `ledger_unavailable` and the address tried. Same probe before this commit: 5.03s, i.e. never delivered. Both new tests mutation-verified by name. The mid-backoff test lives in client.test.ts rather than the real-client suite because createClient does not throw against a dead port — the backoff path only exists when construction itself fails, which that suite already mocks. Re-ran the tb-integration job against real TigerBeetle 0.17.9: 4 files, 12 green. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
From the chatgpt-codex-connector review of cf1d747. All four are the documentation being wrong, which in a safe-rollout guide is the same severity as code being wrong — someone follows it exactly and ends up somewhere else. [P1] The quickstart did not actually enter stage 1. `enforcement` defaults to `enforce`, and `UT_FAIL_OPEN=1` only covers transport and server failures — it does NOT soften a 402/403, which the hook enforces as a real deny. So a reader following the quickstart and trusting "stage 1 cannot block" could be blocked by a matching policy or an exhausted budget. Step 2 now sets `"enforcement": "evaluate_only"` and says why both halves are required. [P2] The preflight could not show the diagnosis it promised. `curl -f` discards the body on an HTTP error — exactly the body carrying the reason — so the 503 guidance underneath it was unreachable and the piped node parse got empty input. Now captures status and body, prints the body, and aborts the hold ONLY on 200 (previously it would have aborted an undefined transferId). [P2] "This is stage 3" labelled setting UT_FAIL_OPEN=1 as the fail-CLOSED posture, reversing the table directly above it — in the paragraph a reader consults to decide whether outages should block their agent. [P2] The timeout chain named the wrong client timeout. The 15s in hooks.json bounds the PROCESS; the HTTP request aborts at 5s in hooks/lib.mjs:168. So the labelled 503 the previous commit advertised was, at the server's then-default 5s, arriving after this hook had stopped listening. Documents both timeouts, and the server side of the ordering is fixed in #134 (3s < 4s < 5s < 15s), measured at 3.03s under a real 5s client bound. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
Connector review round — 2 findings, both fixed in 6b9f1e5
[P1] [P2] The finding that mattered most came from #135, and it invalidated this PR's headline claimThe connector flagged on #135 that I chose these defaults as "below the client's 15s". With the real 5s bound, the arithmetic goes the other way: So the labelled Chain is now monotonic with headroom, and each bound has a reason rather than a vibe: Re-measured against a real server under the actual client bound ( Gates on 6b9f1e5
Outstanding, stated plainlyThe max-effort Codex CLI certification of this remediation has not run — two attempts died on an OpenAI org spend limit ( |
…wait timeout Four findings from the max-effort Codex certification of 6b9f1e5. Three are holes in the fix itself; the first reintroduces the failure this branch exists to prevent, by another route. [P1] A per-await timeout bounds nothing a caller can observe. A cold tenant waits for governor construction and THEN for authorize(), so two 4s timeouts are an 8s request — past the 5s at which usertrust-claude-code aborts. The client would be gone before its own server answered. Worse, an authorize landing in that window runs no cleanup (its own timer had not expired), so the server records a PENDING hold whose transferId nobody ever received. `Deadline` now carries one absolute budget through every await of a request; the error still reports the whole budget, because that is the number that explains the request. [P1] `requestTimeoutMs` was a `.default()`, and `ServerConfig` is the schema's inferred OUTPUT and the public argument type — so the field became REQUIRED for every TypeScript caller, breaking existing object literals with TS2741. My own test hid that behind `as ServerConfig`, which is exactly how a break gets certified as working. It is `.optional()` now, with the default living only in `requestTimeoutOf`; the cast is gone from the test. [P2] connectTimeoutMs restarted for the second handshake call, so a slow treasury followed by a stalled wallet ran to nearly 2x the setting — long enough for the server's generic deadline to fire first and replace the actionable `ledger_unavailable` with an opaque `governor_timeout`. One absolute budget across both calls, verbatim in both governors. [P2] A late `onAbandoned` that itself rejects was unobserved. The callback is typed void-returning but an async function is assignable to it, so a cleanup failure could terminate Node — a server dying while cleaning up after a timeout is worse than one that leaks what it was cleaning up. Also fixes a hint that this branch turned from harmless into user-facing: LedgerUnavailableError told operators to run `npx usertrust tb start`, which prints "not yet implemented" and names port 3000 while the client defaults to 3001. It was unreachable before the connect deadline; it is now the FIRST thing an operator without a cluster sees, so it names commands that work. Every fix mutation-verified by name. Two of the tests I wrote first could not fail and were replaced rather than kept: - a cleanup-failure test driven through the server proved nothing, because the server's own callback already catches; the guard is tested where it lives. - a handshake-budget test against a dead port could not distinguish shared from restarted, because a dead cluster never reaches the second call. It needed a client that succeeds slowly and then stalls. Under mutation it now measures 656ms against a 550ms bound. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
Codex gate — max certification (credits recovered)Verdict: "Sequentially restarted deadlines can outlive the caller and temporarily strand ledger holds, while the public config change breaks existing TypeScript callers. The handshake timeout and abandoned-work cleanup also have correctness gaps." Named the changed files throughout ( Four findings, all accepted, fixed in 67d6637Three of the four are holes in this PR's own fix, and the first reintroduces the failure the branch exists to prevent. [P1] A per-await timeout bounds nothing a caller can observe. A cold tenant waits for governor construction and then for [P1] The new config field broke TypeScript callers rather than defaulting for them — and my own test hid it. [P2] [P2] A late Also fixed: a hint this branch turned from harmless into user-facing
Two of my own tests could not fail, and were replaced rather than keptRecording these, because both would have shipped as coverage:
That is three false-passing mutations across this PR. The generalisation: a passing mutation is a finding about the test at least as often as a clearance for the code. Gates on 67d6637
|
…ig input optional Five findings from the max certification of 67d6637. Three are defects in the previous round's remediation — the only code in this PR nobody had reviewed. [P1] `close()` could STILL hang, by a third route. `destroyAll` bounded governor CONSTRUCTION and then awaited `governor.destroy()` unbounded — and destroy voids pending transfers BEFORE closing the native client, so a governor built while TigerBeetle was healthy and destroyed after it died blocks forever in `voidAllPending()`. Bounding construction had only moved the hang. [P1] The shutdown sweep took N x requestTimeoutMs. `abortEntry` created its deadline internally while `close()` awaits entries sequentially. The comment I wrote in that exact function says a per-await bound would do this — the prose predicted the bug and was ignored by its own author. One budget for the whole sweep, and the comment is now a test: `sweeps N stalled holds within ONE budget`. [P1] `TrustConfig["tigerbeetle"]` required `connectTimeoutMs`, so an existing consumer passing `{ addresses, clusterId }` fails to compile with TS2741. Same break the previous round caught on `requestTimeoutMs`, in the package I did not re-check after fixing the first one — fixing the instance instead of the class. Optional now, with the default in DEFAULT_TB_CONNECT_TIMEOUT_MS. [P2] The server README still named `npx usertrust tb start`, the third copy of a command that prints "not yet implemented". Fixed in the plugin README and in LedgerUnavailableError last round; this one was left. [P2 — RESIDUAL, deliberately not fixed] Reclaiming a late authorization calls `Governor.abort()`, the only void path the interface exposes, which unconditionally does `recordFailure()` and appends `llm_call_failed` — though no provider call ever happened. Five such timeouts open the provider circuit, and the audit chain carries a failure record for a call never made: the cleanup degrades the system it cleans. The fix is a neutral void, which means new Governor API and a decision about which audit event truthfully describes "hold released, no call attempted" — frozen-format territory that deserves a designed answer, not one invented alongside an outage fix. Written up in the server README under "Known residual" so it reads as seen-and-deferred rather than missed. Both new tests mutation-verified by name: the sweep measures 1208ms (4 x 300ms) per-entry against a 900ms bound, and the destroy hang runs out an 8s ceiling. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
Codex max certification of
|
| round | instance | what was fixed |
|---|---|---|
| 1 | /v1/authorize hangs |
the reported outage |
| 2 | close() hangs |
bounded governor CONSTRUCTION in destroyAll |
| 3 | close() still hangs |
governor.destroy() left unbounded — it voids pending transfers before closing the native client, so a governor built while TigerBeetle was healthy and destroyed after it died blocks in voidAllPending() forever |
Three instances of one mechanism means the per-site patch was the wrong shape. The replacement was already designed and already argued for in deadline.ts — "the budget is per REQUEST, not per await, and that distinction is load-bearing" — it simply had not been applied to destroy() or the shutdown sweep. It is now.
Sweep, so this is a claim and not a hope. Every await on a governor or pool operation in packages/server/src:
$ grep -rnE "await (governor|pool)\.[a-zA-Z]+\(|await [a-z]+\.run\(|await withDeadline\(" packages/server/src/*.ts
9 bounded call sites. 3 raw awaits, each deliberate and documented:
server.ts:240governor.settle— unbounded on purpose: a timed-out settle has an unknown outcome on the money path, and reporting it as failed invites a double-settle.server.ts:278governor.aborton the/v1/abortroute — same reason; a caller is waiting to be told the outcome.server.ts:494pool.destroyAll()— bounded transitively; every await inside it now carries a budget.
The other findings
[P1] The shutdown sweep took N × requestTimeoutMs. abortEntry created its deadline internally while close() awaits entries sequentially. The comment I wrote in that exact function says a per-await bound would do this — the prose predicted the defect and was read past by its own author. One budget for the whole sweep now, and the comment is a test: sweeps N stalled holds within ONE budget, which measures 1208ms (4 × 300ms) under mutation against a 900ms bound.
[P1] TrustConfig["tigerbeetle"] required connectTimeoutMs — TS2741 for any consumer passing { addresses, clusterId }. The identical break the previous round caught on requestTimeoutMs, in the package I did not re-check after fixing the first one. Optional now, default in DEFAULT_TB_CONNECT_TIMEOUT_MS.
[P2] The server README still named npx usertrust tb start — the third copy of a command that prints "not yet implemented". Two were fixed last round; this one was left. All three now name commands that work (grep -c across the plugin README, errors.ts, and this README: the only remaining occurrences are deliberate negations telling you not to use it).
Known residual — deliberately not fixed here
[P2] Reclaiming a late authorization records a provider failure that never happened. When /v1/authorize times out and authorize() lands afterwards, the server voids the unreachable hold via Governor.abort() — the only void path the interface exposes — which unconditionally calls recordFailure() and appends llm_call_failed. No provider call was ever made. Five such timeouts open the provider circuit and start rejecting healthy requests, and the audit chain carries a call-failure record for a call that did not happen. The cleanup degrades the system it is cleaning up.
The correct fix is a neutral void: release the hold without provider-failure accounting. That means new Governor API and a decision about which audit event truthfully describes "hold released, no call attempted" — frozen-format territory that deserves a designed answer, not one invented alongside an outage fix at 1am. Seen and deferred, not missed. Written up under "Known residual" in the server README so a reader six weeks from now finds it where the behaviour lives.
Gates on 157c292
vitest run— 4390 passed, 0 failed, 15 skippedbiome check .— exit 0 ·tsc -b— exit 0- engine-factory parity — identical in both governors
tb-integrationagainst real TigerBeetle 0.17.9 — 4 files, 12 green
Round-4 certification running on 157c292.
…es on the clock Two findings from the max certification of 157c292. The first is the FOURTH instance of one mechanism, and it is the one that shows the previous three fixes were at the wrong level. [P1] `close()` resolved while the TigerBeetle client stayed OPEN. Bounding `governor.destroy()` from the server only abandons the promise — it cannot close a client the server does not own, and headless destroy has already marked itself destroyed so a retry is a no-op. AGENTS.md is explicit that an open client is what keeps the event loop from draining, so shutdown "succeeding" here still leaves a process that will not exit. The fix belongs where the resource lives. `destroy()` voids leftover holds and THEN closes the client; voiding is a ledger request and an unreachable cluster never rejects one, so the void has to be bounded for the close to be reachable. Both governors now bound the whole void section against TEARDOWN_VOID_BUDGET_MS and always reach `engine.destroy()`. `govern.ts` was worse than `headless.ts`: its `voidAllPending()` had no try/catch at all, so a THROW also skipped the close. headless had the catch and this comment: "a voidAllPending throw must not skip destroy() and hang the process on the open TigerBeetle socket." The comment names the exact consequence and defends against the wrong failure — a catch covers a throw, and what happens is a hang. Second time tonight that prose on this branch predicted a defect and was read past. Abandoning the void is safe: TigerBeetle auto-voids pending transfers after 300s. Teardown that never finishes is not. [P2] `Deadline.run` could accept a value after its budget was spent. With the budget already exhausted the timer is scheduled at 0ms, and promise reactions run before timers — so an already-fulfilled `op` won the race, returned to a caller that had long since timed out, and skipped reclamation. It now decides on the CLOCK, refusing before assembling a race the clock cannot win, with the reclamation continuation attached first so the refused value is still voided. Mechanism tally, unbounded await on a ledger operation, one per round: r1 /v1/authorize hangs r2 close() hangs (bounded construction) r3 close() still hangs (bounded destroy call) r4 close() returns, client open Fixed at the source in r4 rather than adding a fifth bound. Sweep: `grep -rnE "await (governor|pool)\.[a-zA-Z]+\(" packages/server/src` — 3 raw awaits remain, all deliberate and documented (2 money-ambiguity on settle/abort, 1 transitively bounded). Both mutation-verified by name: teardown hangs out a 30s ceiling unbounded, and the clock check fails its test when removed. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
…ation Two findings from the max certification of b51b17a. Both are defects the PREVIOUS round's fix introduced — the pattern this branch keeps demonstrating. [P2] The bound that lets destroy() always reach the ledger client held the event loop open. `Promise.race([work, setTimeout(...)])` leaves the losing timer REFERENCED when the work wins — which is every healthy governor, including every dry-run one with nothing to void. So destroy() returned promptly and Node could not exit for another 5 seconds. The fix for "the process cannot exit" delayed process exit, on every teardown. Extracted as `raceWithBudget`, which clears the timer in a `finally`, and used by both governors. Tested as a real process exit rather than an in-runner assertion: a leaked timer does not fail an assertion, it delays a process, and the runner's own handles would mask it. Under mutation the child takes 5112ms against a 4500ms bound. [P2] `Deadline.run` left `op` unobserved on the early-timeout path when no cleanup callback was supplied. The continuation was attached only `if (onAbandoned !== undefined)` — and most callers pass no cleanup, so the common path was the unguarded one. A slow factory can exhaust the budget, return a promise, receive its 503, and reject a second later into nothing: an unhandled rejection that can terminate Node. The observer is now unconditional; only the reclamation inside it is conditional. Both mutation-verified by name, the second by exit code — an escaped rejection is not an assertion failure, it is a non-zero exit, and the run confirms 1 vs 0. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
…source, not dist Three findings from the max certification of 7344a8a. Two are the SAME mechanism at two more sites, so this replaces the rule rather than patching a third instance. [P2 x2] Winning a race is not the same as being on time. Promise reactions run before timers, so an event loop delayed across the deadline lets a queued `op` reaction settle before the overdue timer callback — the value comes back with the budget already spent, the timeout flag still false, and the reclamation declines to run. For /v1/authorize that is a hold retained after the caller has gone. The same hole existed in the core handshake: `createTreasury` completing as the budget expired let `createFundedBudgetWallet` start against a budget already gone, so two sequential calls could take ~2x connectTimeoutMs and still report success. Both now check the CLOCK on entry AND on exit, rather than trusting the race. That is the rule stated once and applied at every site, which is what should have happened when the first instance was fixed one round ago: r5 entry check (server Deadline) r6 exit check + core handshake, both ends Reclamation is now a single `reclaim()` used by both the late-arrival continuation and the late-winner path, with a `reclaimed` flag so it runs at most once. [P1] `teardown-timer-leak.test.ts` imported `packages/core/dist/headless.js` from a child process. `dist/` is gitignored and CI's test job runs vitest straight after `npm ci` with no build, so the test would have failed on every clean checkout with ERR_MODULE_NOT_FOUND — and vitest's source alias does not reach a child process. Confirmed by deleting dist/ locally, which reproduced it exactly. Now runs the SOURCE through the repo's `tsx`. That prompted a full cold-CI simulation — `rm -rf packages/*/dist packages/*/tsconfig.tsbuildinfo` then the whole suite — which is green: 4395 passed. Worth doing after any test that spawns a process. Also documents what a version-mismatched TigerBeetle looks like, in both READMEs: a client newer than the server is NOT a connection error, the cluster accepts the socket and then EVICTS the client, so recovery ends in `ledger_unavailable` and reads identically to having no cluster at all. Measured tonight against 0.16.74. Signed-off-by: Cam <cam@camwhiteus.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Cam <cam@camwhiteus.com>
Codex max certification of
|
| round | site | fix applied |
|---|---|---|
| r5 | Deadline.run entry |
check the clock before racing |
| r6 | Deadline.run exit, and the core handshake |
"the clock decides on the way out too" |
| r7 | the work starts BEFORE the check, and a late rejection skips it | open |
Diagnosis: the helpers take a Promise, so the work has already started before they are called. withConnectDeadline("createTreasury", tbClient.createTreasury()) evaluates its argument eagerly — the ledger request is issued, then the guard runs. So every "check the clock first" fix could only ever check the clock after the thing it was meant to gate. The guard then throws while that live promise runs unobserved, and destroying the client rejects it into nothing: an unhandled rejection that can terminate Node 22.
The replacement (not applied here): take a thunk, not a promise. run(what, () => op), started after the check. The class then stops being expressible. The second finding — a late rejection wins the race, jumps through finally, skips the post-race clock check, so /v1/authorize can return a late 403/500 or even a 200 shadow denial instead of 503 governor_timeout — lives in the same helper and belongs in the same pass.
Constraints for whoever picks it up: both governors are pinned byte-identical by engine-factory-parity.test.ts, so the edit must be verbatim in both. Three existing tests must stay green — "refuses an already-fulfilled op when the budget is already spent", "refuses a value that WINS the race but arrives after the budget", "bounds the whole handshake, not each ledger call separately" — and the new one to add is "work is never started once the budget is spent", asserting the ledger call was never issued, not merely that it timed out.
Everything from rounds 1-6 is fixed and green
vitest run— 4395 passed, 0 failed, 15 skippedbiome check .— exit 0 ·tsc -b— exit 0 · engine-factory parity — identicaltb-integrationagainst real TigerBeetle 0.17.9 — 4 files, 12 green- cold-CI simulation (
rm -rf packages/*/dist packages/*/tsconfig.tsbuildinfo, then the full suite) — green
Findings by round: 2 → 4 → 5 → 2 → 2 → 3 → 2 open.
Known residuals on this branch, both deliberate
- The clock-vs-race mechanism above — diagnosed, replacement identified, not applied.
- Reclaiming a late authorization records a provider failure that never happened —
Governor.abort()is the only void path and unconditionally callsrecordFailure()plus anllm_call_failedaudit event, though no provider call was made. Five such timeouts open the provider circuit. The fix needs new Governor API and a decision about which audit event truthfully describes "hold released, no call attempted" — frozen-format territory. Written up under "Known residual" in the server README.
Neither is hidden, and neither should be read as covered by this PR's green gates.
Handover detail for the two open findings (addendum to the comment above)Exact sites, as of
Constraint: Must stay green:
The new test, and the discriminating assertion: "work is never started once the budget is spent" must assert the ledger call was never issued — e.g. Also relevant to the fix: the reason the eager promise matters is not just ordering. When the entry guard throws, that live promise has no listener, and destroying the client rejects it — an unhandled rejection that can terminate Node 22. A thunk removes the unobserved promise along with the ordering bug. |
Round 7's two open findings. Both are the same mechanism the branch has
been chasing since round 5, and both stop being expressible rather than
being patched at one more site.
The helpers took a `Promise`, so the work had already started before they
were called. `withConnectDeadline("createTreasury", tbClient.createTreasury())`
evaluates its argument first: the ledger request is issued and THEN the
clock is consulted, so every "check the clock first" fix could only ever
check it after the thing it was meant to gate. Worse, the entry throw left
that live promise with no listener at all — the race is never assembled on
that path — while the caller's catch destroys the client underneath it,
rejecting the in-flight request into nothing. That is an unhandled
rejection that can terminate Node 22.
Both helpers now take a thunk, invoked after the check. On a refused call
nothing is created: no hold to strand, no governor to destroy, no
unobserved promise. Applied verbatim to both governors; the parity test
compares them as source text.
Second finding, same helper: a rejection that won the race jumped straight
out through `finally` and skipped the post-race clock check, so `Deadline`
decided on the clock for values and on the race for errors. `server.ts`
shadows every mapped status under 500, so a late 4xx came back as a clean
200 {"decision":"would_deny"} on a request whose deadline had already
blown — a dependency failure laundered into a policy opinion, the same
class this package fixed once already for 503s. Late rejections are now
timeouts.
Accepted cost, deliberately: a late LedgerUnavailableError names the
TigerBeetle addresses and this reports governor_timeout instead. An
on-time one still surfaces unchanged and both map to 503, so only the
reason string is lost on the late path — worth keeping the clock the
single decider and HTTP status mapping out of deadline.ts.
Tests, by the assertion that discriminates:
- core "work is never started once the budget is spent" asserts the ledger
call was never ISSUED (createFundedBudgetWallet call count), not that it
timed out. A timeout assertion passes against the eager code too, which
issues the request and then throws; only the count separates "gated"
from "issued and abandoned". Mocks TrustTBClient rather than
tigerbeetle-node because the condition is time passing BETWEEN the two
handshake calls, which a real client cannot reach.
- server "reports a late rejection as a timeout" covers the second finding
with the same stalled-loop reproduction the value case already used.
- Two positive controls, because an all-refusal suite cannot detect a
guard that is stuck shut: "still issues the second call when the budget
allows it" and "still reports an ON-TIME failure as itself".
"refuses an already-fulfilled op when the budget is already spent" keeps
its name and its refusal assertion; its tail now asserts the work was
never started, which is strictly stronger than the reclamation it used to
assert. "observes a late rejection even when no cleanup was supplied"
moves to the timer path, now the only way an op outlives the call.
Mutation-verified by name, in both directions: eager order in the core
helper fails the count assertion (1, not 0); eager order in Deadline fails
the server one; removing the failure-path clock check returns
PolicyDeniedError instead of GovernorTimeoutError; converting EVERY
rejection fails the on-time control. Each leaves the other tests green.
Out of scope, unchanged: Governor.abort() recording a provider failure
that never happened, which needs new Governor API and a frozen-format
decision. Still written up under Known residual.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Cam <cam@camwhiteus.com>
Closes #130, #131, #132.
usertrust-serverv3.3.1 could not complete a single authorization in its default configuration./v1/authorizenever responded — not slowly, ever.Root cause (#130)
tigerbeetle-nodeexposes no request timeout —ClientInitArgsiscluster_id+replica_addressesand nothing else — and it treats an unreachable or unresponsive cluster as transient: it retries the handshake forever and its promise never rejects.createTBEngineawaited that promise, socreateGovernor({ dryRun: false })never settled and every request behind it hung.dryRun: trueskips engine construction entirely, which is why the one existing integration test passed.The part worth naming: the
catcharound engine creation that raisesLedgerUnavailableError— carrying the exact hint an operator needs — was unreachable in the most likely production misconfiguration, because there was no rejection to catch. The error existed; nothing could raise it.Measured against the real native client:
createTreasuryOKConnectionRefusedforeverOnly the reachable-but-wrong cases ever failed loud.
On the reporter's machine the default TB port
127.0.0.1:3001was held by an unrelatednext-server— so the client connected at the TCP level and waited forever for a reply in a protocol the listener does not speak. Both modes hang, so the fix does not depend on which one you hit.The fixes
#130 —
tigerbeetle.connectTimeoutMs(default 5s) bounds the handshake, making the existingLedgerUnavailableErrorpath reachable. A timed-out client is destroyed rather than left retrying behind a caller that has already given up. Applied verbatim to both governors;engine-factory-parity.test.tscompares them as source text.#132 — a server-side deadline on every governor await. This is the class fix and it stands on its own: a cluster that dies after a governor is built stalls inside
authorize(), where no construction-time deadline can see it.close()had the same defect —destroyAll()awaited the same never-settling promise, so a server whose ledger was unreachable could not shut down either.Two traps this opened, both fixed here:
500. The moment a 503 existed, an outage would have been returned as a clean200 {"decision":"would_deny"}— a dependency outage laundered into a policy opinion. Only 4xx verdicts are shadowed now.settle/abortdeliberately bound only governor construction, not the settle/abort call: a timed-out settle has an unknown outcome on the money path, and reporting it as failed would invite a double-settle.#131 — end-to-end coverage of the default config, in both directions. A failure-path test alone would be satisfied by a deadline that refuses everything.
Before / after — the issue's exact probe
The reason string names the address, so the next operator diagnoses this from the response instead of from
sample.Measured under the real client bound (
curl --max-time 5), which is the number that matters. The first version of this PR used a 5s ledger deadline and answered at 5.03s — andusertrust-claude-codeaborts its HTTP request at 5s (hooks/lib.mjs:168; the 15s inhooks.jsonbounds the process, not the request). So the labelled 503 arrived after the client had stopped listening, and every user would have seen a generic transport error instead. Caught by the connector review on #135. The chain is now monotonic with headroom:Same server against a real cluster,
dryRunstill false:authorize200 in 0.70s →settle200 with a real receipt and audit hash →budget49980. Shutdown with an unreachable ledger: clean exit in 2s.Mutation verification (by name, not count)
integration.test.ts→ "answers 503 when the ledger is unreachable…", "does not shadow a ledger outage…"ledger-connect-deadline.test.ts→ "rejects createGovernor with LedgerUnavailableError instead of hanging forever"withDeadline→ passthroughIn every case the pre-existing dryRun test still passed — which is precisely the blind spot #131 is about. Each failure is a hang, i.e. the defect itself, not an incidental assertion.
Positive control: the full
tb-integrationjob run locally against a real TigerBeetle 0.17.9 — 4 files, 12 tests, all green with the deadline armed. Without it, an always-refusing guard would have passed every failure test here.Gates
vitest run— 4373 passed, 0 failed, 15 skippedbiome check .— exit 0 (39 pre-existing warnings)tsc -b packages/core packages/server— exit 0tb-integrationjob locally against real TB 0.17.9 — 12 passedNotes
ServerConfiggainsrequestTimeoutMs, so the five test config helpers were updated.vault/derive.test.tsandcli/secret.test.ts(scrypt, vitest's default 5s timeout) time out under heavy load — reproduced at load average ~44, and identically on unmodified master. Not caused by this change, but worth raising separately now that the fleet runs many lanes at once.🤖 Generated with Claude Code
Round 7 remediation —
bad416fBoth open findings are closed. The two helpers now take a thunk, not a promise, so the
clock check happens before the work is started rather than after it was already issued; and
Deadline.runapplies the same clock check on the failure path, which a rejection usedto skip by jumping out through
finally.Accepted cost, recorded deliberately: converting every late rejection to
GovernorTimeoutErrorloses one piece of operator detail. A lateLedgerUnavailableErrornames the TigerBeetle addresses in its reason string; past the budget that request now
reports
governor_timeoutinstead. An on-timeLedgerUnavailableErrorstill surfacesunchanged, and both map to 503, so only the reason string is lost and only on the late path.
The alternative — convert only what would map to
<500, which is the shadow casespecifically — was rejected because it couples
deadline.tsto HTTP status mapping. Keepingthe clock the single decider is worth the reason string. A permanent test,
"still reports an ON-TIME failure as itself", pins the half that must not change.
Known residual 2 is untouched and still open by design:
Governor.abort()recording aprovider failure that never happened. It needs new Governor API plus a decision about which
audit event truthfully describes "hold released, no call attempted" — frozen-format
territory, out of scope for this round. Still written up in the server README.