Repository navigation
fix(storage): bound storage helper probes by the carrier's remaining budget - #437
Merged
Merged
Conversation
…budget Native storage helper probes kept fixed 15 s (reads, target observation, helper create/start) and 10 s (cleanup) caps although each carrier has a 90 s HACK_NATIVE_STORAGE_TIMEOUT_MS budget. On loaded hosted runners a slow `docker start -ai` of the helper refused at 15 s as storage-helper-start probe-timeout and failed `hack exec`/`up`. Each probe and command now takes its timeout from nativeComposeStorageProbeTimeoutMs: daemon commands get the remaining carrier budget, reads the remaining budget up to the probe's 60 s ceiling, and cleanup keeps its own 10-60 s bound that is never clipped to the deadline. A timeout before the deadline still reports probe-timeout; one at the deadline reports storage-budget. Document the fail-closed saved exec contract: a closing storage proof refusal fails exec after its command ran. Refs HACK-1369 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… helper start The probe-budget test drove only the unrecorded root proof, so a 15 s cap on the recorded original-command child (the path exec and up storage proofs take) survived. Add a case that builds the carrier with originalCommandRecords and sends a readonly verify request with a 20 s virtual helper start, asserts every helper command was published and settled as an original command, and apply the helper's daemon-side state change when its child exits so recorded admission re-checks see the created helper. Document that recorded commands share the remaining-budget bound. Refs HACK-1369 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…rnal completion The recorded exec/up case called finish() with a synthetic completion. The finalizer consumes the completion before its try/finally, so the refusal left the held invocation directories open; hosted CI's full run then garbage collected them and failed the file with ERR_INVALID_STATE. Journal the intent like the witness owner does, complete it, and let finish() retire the invocation files. Collect garbage after each test so any future leak is reported against this file. Refs HACK-1369 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
Every Docker probe and command inside a native storage carrier had a fixed timeout in
src/lib/native-compose-storage-witness-docker.ts: 15 s for reads, target observation, volume create and helper create/start, and 10 s for helper removal and cleanup reads. This held even though #375 gave each carrier a 90 sHACK_NATIVE_STORAGE_TIMEOUT_MSbudget. On loaded hosted runners, a slowdocker start -aiof the helper hit the 15 s cap. It refused asE_COMPOSE_FAILEDwith stagestorage-helper-startand reasonprobe-timeout, while most of the carrier budget was still unused. That failedhack exec/upin the native-config-compose and native-config-networks e2e scenarios. Forexec, this could happen after the guest command had already printed its result, because the failing step was the closing storage proof.Fix
A new function,
nativeComposeStorageProbeTimeoutMs({ kind, deadline }), now sets every probe and command timeout in the carrier:command(volume create, helper create/start): gets the carrier's whole remaining budget. A hung command still times out exactly when the carrier's budget ends.read(reads, target observation, saved-recovery reads): gets the remaining budget, capped at 60 s. 60 s is the mostcreateNativeComposeProbeaccepts.cleanup(post-exit reads, helperrm): has its own budget, the remaining budget clamped to 10–60 s. It is never clipped to the carrier's deadline, so cleanup still runs after the carrier expires, and a hung engine still cannot stall cleanup forever.Every timeout is finite and named. A timeout that ends before the carrier's deadline still reports
probe-timeout. One that ends at the deadline reportsstorage-budget, as before. No check, retention or recovery rule changed. Diagnostics code (compose-startup-budgetresolution, effect diagnostics) is untouched; the only change incompose-startup-budget.tsis one doc-comment sentence that described the old fixed cap (to stay out of the way of G50/HACK-1361 and G51/HACK-1365).Exec decision (documented). The closing storage proof after a saved
execstays fail-closed. If it refuses (including a probe timeout),execfails with that refusal even though the guest command already ran, and the command's exit code is not reported. Ownership and saved state are unchanged, and the next admission proves storage again. This is now spelled out indocs/reference/native-compose-commands.mdand in therunNativeComposeStorageVerifiedExecdoc comment.Docs updated:
docs/reference/native-compose-storage-xattr-carrier.md(budget layering),docs/cli.md(storage budget section),docs/reference/native-compose-commands.md(exec contract).Tests
New file:
tests/native-compose-storage-probe-budget.test.ts. It runs the realcreateNativeComposeDockerStorageXattrCarrierthrough a full readonly root proof (target, helper create, policy, start, response, cleanup, closing observation) against a stub engine:createNativeComposeProbethat answers in-process.shell.runthat rewrites them into local/bin/shchildren, so the real capture, process-group and settlement paths still run.Date.nowspy). There is no real 15 s wait.HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1.DOCKER_HOST=tcp://…andDOCKER_API_VERSIONstop the engine-ID fence from ever choosing a direct Unix socket. A tripwiredockeron PATH must never run.What the tests cover:
rmgets 10–60 s; every probe gets 1–60 s.storage-helper-start/storage-budget.Mutation proofs:
timeoutMs: 15_000on the helper start makes test 1 fail with exactly the CI symptom{ stage: "storage-helper-start", reason: "probe-timeout" }.runNativeComposeStorageVerifiedExecswallow the closing refusal fails test 4.Evidence
mise x bun@1.4.2).bun install --frozen-lockfile; no lockfile changes.native-compose-storage-witness-docker,-command-record,-witness-docker-io,-witness-dependency,-witness-xattr-owner,native-compose-effect-diagnostics,compose-startup-budget,agent-instruction-source(295 tests total).bun run --cwd packages/cli typecheck: 0 errors. Rootbun run check(includes privacy:check): 0 errors (existing complexity warnings only).origin/next..HEAD: no leaks.Gaps
execanduprun, using a stub engine. It does not drive thehack exec/hack upCLI end to end: that needs a real Docker engine, and these tests may not use one. The hosted native-config-compose and native-config-networks e2e scenarios are still the end-to-end check, and only hosted CI on this PR shows whether those flakes are gone.HACK_NATIVE_STORAGE_TIMEOUT_MS.compose-startup-budget.tsdoc-comment edit may textually conflict with G50/G51 if they edit the same lines. The conflict would be trivial.Round 2: recorded exec/up path (review follow-up)
The verifier found that the tests above missed the path
execandupactually take.prepareNativeComposeCommandStoragecreates every carrier withoriginalCommandRecords: true, so their readonlyverifyproofs run each helper command through the recorded original-command branch ofrunNativeComposeStorageDockerCommand(publication handshake,boundaryDeadline, settlement). The first test used an unrecordedrootrequest, so capping only the recorded child at 15 s (Math.min(15_000, boundaryDeadline - performance.now())) left every test green. The code fix already reached this branch, because both branches use the same computed timeout. But no test proved it.Changes in this round (test and docs only, no product code change):
next(5208ee5) into the branch. The merge was clean.exec/updo (originalCommandRecords: true, default 90 s budget fromresolveNativeStorageTimeoutMs) and sends a readonlyverifyrequest. The helper start takes 20 s of virtual time. The test checks that the proof completes, that create and start were each granted more than 20 s and at most the budget, and that the carrier'scommands.jsonrecord holdscreate,startandremove, each with a published child and a clean settlement. That last check proves the recorded branch really ran. The test journals the carrier intent the way the witness owner does (initializeNativeComposeStorageCarrierJournalandbeginNativeComposeStorageCarrierIntent). The journal'srecordCreatedis the invocation's callback. After the proof, the test callsjournal.complete()and passes the completion tofinish(), which retires the invocation files and closes their held directories. The test asserts that the invocation directory is gone.afterEachnow callsBun.gc(true). If this file ever leaks a held directory again, the leak is reported against this file instead of a later, unrelated one.The first push of this round, 16782e3, failed hosted
test(run 40058406790) with 1 fail and 3 errors out of 8256 tests. My recorded case calledfinish()with a synthetic completion. The finalizer consumes the completion before itstry/finallyblock, so the refusal left the three held invocation directories (the instance,storage-carriersand invocation directories) open. Garbage collection later closed them duringnative-compose-inputs.test.tsand raisedERR_INVALID_STATE, which Bun counted against the test that was running ("omitted native compiler context preserves Compose routing probing and branch omission"). 8d3d1f3 fixes this with the real journal completion. With theBun.gcguard in place, removing thefinish()call makes this file fail with exactly those three FileHandle errors, so the guard catches the leak.created. Each recorded command's host identity reads (/bin/ps, plussysctlon macOS) pass through to the host. They are read-only.docs/reference/native-compose-storage-xattr-carrier.mdnow says that recorded original commands (theverifyproofs thatexecanduprun) also share the remaining-budget bound, together with their publication handshake and identity reads.Not done: driving
prepareNativeComposeCommandStorageend to end throughports.carrierwith the real Docker carrier. That path also needs an enrolled witness and the carrier journal. The new case uses the same carrier constructor and options thatprepareNativeComposeCommandStoragepasses, and the same request kind.Mutation results on the new test file (run at 16782e3; the first two were run again at 8d3d1f3 with the same result):
Math.min(15_000, remaining): 3 tests fail.Math.min(15_000, opts.timeoutMs, …)): 3 tests fail.rmfixed at 10 s. These steps are not slow in the CI failure, and no test pins their exact bound.Evidence for this round:
bun install --frozen-lockfile, with nobun.lockchange.probe-budget,witness-docker,witness-docker-io,command-record,carrier-journal-completion,effect-diagnosticsandcompose-startup-budget.bun run --cwd packages/cli typecheck: 0 errors.bun run check: exit 0,privacy-check: ok, the same 34 existing warnings. Biome on the changed test file: clean.diron thegit log -p origin/next..HEADexport: no leaks.HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1andDOCKER_HOST=tcp://127.0.0.1:9. The tripwiredockernever ran.native-compose-storage-witness-xattr-owner.test.tshad several of its 5 s-default tests time out locally while the shared host's load average was 75–280. One example is "command storage carriers use the unset named storage budget". Run alone with--timeout 120000, that test passed in 16 s, so it is slow on this host, not hung. That file uses a fake carrier transport and never reaches the Docker carrier this PR changes, and the file itself is unchanged fromnext. It passed in hosted CI at d15116e. Hosted CI at this head is the full-suite evidence.Hosted CI at 8d3d1f3 (pull_request run 40062046367): every check passed. See the next section.
Round 3: independent recheck at 8d3d1f3
No code change in this round. The PR head is still 8d3d1f3, which already contains
next(5208ee5). This round rechecked the Round 2 fix from a fresh worktree:bun install --frozen-lockfile, private HACK_HOME and DOCKER_CONFIG,HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1,DOCKER_HOST=tcp://127.0.0.1:9).timeoutMs: record ? Math.min(15_000, boundaryDeadline - performance.now()) : timeoutMs), 1 test fails: "a recorded exec/up verify proof completes a helper start slower than the former 15 s cap". After the source was restored, the tree was clean.bun run --cwd packages/cli typecheck: 0 errors. Rootbun run check: exit 0,privacy-check: ok, the same 34 existing warnings. gitleaks 8.30.1diron thegit log -p next..HEADexport: no leaks.testjob (38m12s), docker-e2e shards 1–4, Candidate core on macOS and Ubuntu, Config compiler ×4, runtime state models, linux-process-lifetime, runtime-images and secret-scan. In the e2e shards, native-config-networks passed (1901.4s) and native-config-compose passed (514.9s).The remaining gaps are unchanged from Round 2. No test pins the exact bound of the volume-create, read, target-observation and
rmcall sites. Nothing drivesprepareNativeComposeCommandStorageend to end with the real Docker carrier.Refs HACK-1369
🤖 Generated with Claude Code