Repository navigation
fix(native): drain bounded runtime requests to EOF with the shared exit bound - #447
Merged
Merged
Conversation
…rain late The foreground graph owner drained its child's stdout and stderr for only 250 ms after exit. Under heavy oversubscription the pipe readers could be scheduled after that window, so the closed native error code (or readiness line) was dropped and a generic startup failure was reported. Replace the window with the named, documented NATIVE_EXIT_DRAIN_BOUND_MS (5 s), still measured from the observed exit. EOF still ends a normal drain at once; the bound only ends a drain that a descendant holding the pipes keeps open. Tests: a writer that emits the code only after the stand-in is reaped is kept (fails with the 250 ms window), and a held descendant still settles within the bound while it is alive (fails with a widened drain). Refs HACK-1350 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 4dfbb6a)
…it bound Bounded one-shot requests (status, logs, exec, one-off, read and recovery) stopped reading the child's stdout and stderr 250 ms after its exit. Under heavy oversubscription the closed native error code could arrive after that window, so it was dropped and a generic request failure was reported. Share NATIVE_EXIT_DRAIN_BOUND_MS (5 s, measured from the observed exit) from the runtime client with the foreground graph owner, which #396 moved off the same 250 ms window. EOF still ends a normal drain at once; the bound only ends a drain that a descendant holding a pipe keeps open. Both boundaries take an optional exitDrainBoundMs test seam that defaults to the constant. Tests: a stderr writer that emits the closed code only after the stand-in is reaped is kept for status, read, logs and exec (fails with a 250 ms window); the held-descendant cases now settle within the named bound plus a margin (fail with an unbounded drain). The graph owner's late-writer case injects a wide named bound and uses named handshake budgets so host load cannot flake it; its held-descendant cases keep the production bound. Refs HACK-1387 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…s and exec refusals The logs and exec "inherited pipe refuses output" cases spawned a keeper that slept 800 ms: longer than the former 250 ms window, but inside the shared NATIVE_EXIT_DRAIN_BOUND_MS, so its EOF now settled the output and the requests resolved instead of refusing. The keeper now holds the inherited streams until the test releases it after the refusal (self-capped at 60 s), so the drain can only end at the bound, and the case budgets derive from that bound. Refs HACK-1387 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The late-writer cases waited only for the stand-in's pid file to exist and
then read it. writeFileSync creates the file before it writes the digits, so
under load the read could return "": Number("") is 0, kill(0, 0) always
succeeds, and the reaped-stand-in wait expired. Both files now read pid
handshakes through waitForPid, which waits for a safe-integer pid above 1,
and the late-writer stand-ins publish their pid as a torn handshake (empty
file, 50 ms gap, then the pid) so an existence-only reader fails every run.
The keeper pid reads use the same helper.
Refs HACK-1387
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
roodboi
marked this pull request as ready for review
October 24, 2026 05:48
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
…n bound and Loki hint wiring (#478) Refs HACK-1425. Follow-ups from the verifier reviews of #447 (merged as 686b6b8) and #449 (merged as 4c9eb3e). None of them blocked those PRs. ## Root cause 1. **No test pinned the production drain bound.** Every drain test read `NATIVE_EXIT_DRAIN_BOUND_MS` itself, or injected a wide value through the `exitDrainBoundMs` test seam. The held-descendant cases only checked an upper bound (`settled - exited < bound + margin`). So setting the constant to 250 (or changing the `?? NATIVE_EXIT_DRAIN_BOUND_MS` default to `?? 250`) passed the whole suite. 2. **Three bound drains had no late-writer case.** The one-off drain and the two recovery drains (selection and cleanup) use the same code path as status, read, logs and exec. Only those four had a "closed code written after the reported exit" case. 3. **Three wiring mutations in `src/commands/project.ts` survived the tests.** `tests/logs-loki-empty-fallback-cli.test.ts` never passed `--pretty`, `--query` or `--until` through the CLI. Replacing `pretty: args.options.pretty === true`, or the selectors' `query: opts.queryOpt` or `until: opts.until`, with a constant did not fail any test. 4. **The `hack global logs-reset` text was out of date.** The command runs `down -v`, which (since #449) also deletes the `alloy-data` volume that holds Alloy's read positions. The confirm prompt and the CLI summary named only Loki and Grafana. ## Fix - `tests/native-runtime-client.test.ts` - New case `the production exit drain bound keeps its documented floor and outlasts a late writer`. It checks that `NATIVE_EXIT_DRAIN_BOUND_MS >= 5000` (the documented floor, now written in the constant's doc comment) and that the bound is larger than the late-writer delay plus a clock tolerance. - The held-descendant `exit` cases (read, selection, cleanup) do not pass the seam. They now also check `settled - exited >= NATIVE_EXIT_DRAIN_BOUND_MS - 50 ms`, which proves the production default is the bound in effect. - `LATE_WRITER_SELECTIONS` gains `one-off`, `selection` and `cleanup`. - `tests/native-project-process.test.ts`: the same lower bound on the foreground graph owner's two held-descendant cases. This covers that file's own `?? NATIVE_EXIT_DRAIN_BOUND_MS` default. - `tests/logs-loki-empty-fallback-cli.test.ts` gets two CLI cases. Both use the stub `docker`, the fake Loki, and a private HOME via `privateCliEnv`: - `--loki --no-follow --pretty migrate`: the hint repeats `--pretty`. - `--no-follow --query '<custom LogQL>' --until 1m`: Loki receives the custom query exactly as given, and the hint lists `--query '…'` and `--until 1m` as selectors that `--compose` cannot apply. - `src/commands/global.ts`: the summary is now "Wipe Loki, Grafana and Alloy volumes (fresh logs, dashboards and Alloy read positions)". The prompt now says it deletes "ALL Loki logs, Grafana state and Alloy read positions". `docs/reference/cli.md` was regenerated with `bun run docs:cli-reference`. `docs/architecture.md` already described the volume removal, so it is unchanged. There is no production behavior change apart from the logs-reset text and a doc comment. ## Evidence Mutation runs (each mutation applied on its own and restored from a backup afterward): | Mutation | Killed by | | --- | --- | | `NATIVE_EXIT_DRAIN_BOUND_MS = 250` | documented-floor case | | runtime client `selected ?? 250` | read, selection and cleanup held-descendant `exit` cases | | graph owner `opts.exitDrainBoundMs ?? 250` | `ready` and `early failure` held- descendant cases | | fixed 250 ms window for `one-off` only | `authored one-off keeps a closed code written after the reported exit` | | fixed 250 ms window for `recovery-selection` only | `authored selection keeps a closed code …` | | fixed 250 ms window for `recovery-cleanup` only | `authored cleanup keeps a closed code …` | | `pretty: args.options.pretty === true` → `false` | `explicit --loki snapshot hint repeats --pretty` | | selectors `query: opts.queryOpt` → `undefined` | `explicit --query snapshot hint names --query and --until` | | selectors `until: opts.until` → `undefined` | `explicit --query snapshot hint names --query and --until` | Local gates at this head (Bun 1.4.2): - `bun run check` passed, including `privacy:check`. - `bun run --cwd packages/cli typecheck` and root `bun run typecheck` reported 0 errors. - Biome reported no issues on the changed files. - gitleaks 8.30.1 on the `origin/next..HEAD` patch found no leaks. - Focused tests: `native-runtime-client`, `native-project-process`, `logs-loki-empty-fallback- cli`, `global-command` and `prerequisites-matrix` gave 74 pass, 0 fail. The full-suite evidence is hosted CI's sharded `test` job at this PR head. ## Gaps - The new lower-bound checks rely on the wall clock of two processes and a 50 ms tolerance. The lower direction does not depend on host load, because a timer cannot fire before its delay, so these checks should not add flakiness. - The new prompt text has no test. `confirmSafe` declines in non-interactive runs (`initialValue: false`), so a CLI test cannot see the prompt without a TTY harness. This matches the coverage before this PR. - The pin is a floor (`>=`), not an exact value. Raising the bound still passes, which is intended. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
invokeNativeRuntime(src/backends/native-runtime-client.ts) stopped reading a bounded request's stdout and stderr 250 ms after the child's observed exit (drainTimer = setTimeout(() => drain.abort(), 250)). The native runtime writes its closed error code before it exits, so the bytes are already in the pipe, but on a heavily oversubscribed host the stderr reader can be scheduled after that window. The reader was cancelled with a partial or empty buffer, the structured code was lost, and the caller saw the genericNative runtime request failed; inspect owned state before retrying.instead of(code). This affects every bound drain: status, logs, exec, one-off, read (plan/inspect) and recovery. #396 fixed the sibling in the foreground graph owner (src/backends/native-project-process.ts, HACK-1350).Fix
NATIVE_EXIT_DRAIN_BOUND_MS(5 s, measured from the observed exit, never from spawn) now lives innative-runtime-client.tsand is shared with the graph owner, which imports it instead of defining its own copy. The doc comment covers both call sites and both tickets.exitDrainBoundMstest seam (validated as a positive safe integer) that defaults to the constant. Production callers do not pass it.Tests
tests/native-runtime-client.test.tsauthored %s keeps a closed code written after the reported exitfor status, read, logs and exec: the stand-in spawns a detached writer that inherits only stderr, exits, and the writer emits the closed code 1 s after the test has seen the stand-in reaped. The code is kept and the message stays closed. The case injects a wide named bound (LATE_WRITER_DRAIN_BOUND_MS, 25 s) so host load cannot push the delayed write past it; it proves EOF, not a fixed window, ends the drain.NATIVE_EXIT_DRAIN_BOUND_MS + DRAIN_SETTLE_MARGIN_MSwith the production bound; they prove the bound still ends a drain a descendant holds open.tests/native-project-process.test.ts(hardening #396's late-reader case, which failed once in ~80 iterations under load)closed code written after the reported exit is kept regardless of a late readerinjectsLATE_WRITER_DRAIN_BOUND_MSinstead of relying on the 1 s delay landing inside the 5 s production bound on a loaded host.tests/native-authored-project-logs.test.ts,tests/native-authored-project-exec.test.ts(second commit)The two "inherited pipe refuses output" cases spawned a keeper that slept 800 ms: longer than the old 250 ms window but inside the shared 5 s bound, so after the fix its EOF settled the output and the requests resolved instead of refusing (
did not settle). The first full-suite run caught this (1 real failure in each file; the other 17 + 7 failures in those files were the suites' "prior lifetime unconfirmed" cascade). The keeper now holds the inherited streams until the test releases it after the refusal (self-capped at 60 s), so only the bound can end the drain, and the case budgets derive fromNATIVE_EXIT_DRAIN_BOUND_MS.Named budgets replace the literal 10 s / 20 s / 30 s / 60 s windows:
HANDSHAKE_BUDGET_MS(20 s, pid handshakes),STARTUP_TIMEOUT_MS(60 s),DESCENDANT_CAP_MS(60 s),CASE_TIMEOUT_MS. The held-descendant cases keep the production bound.Mutation proofs
250window instead of the boundexitcases (read/selection/cleanup) fail at the 30 s request timeout (27 pass); theabortcases still settle through cancellation250window(graph_budget)(10 pass)Load run
20/20 consecutive passes of
tests/native-runtime-client.test.ts+tests/native-project-process.test.ts(41 tests each run, 44–46 s per run) on this 18-core Mac with 12 extrayesloops on top of the existing load; load average 17–35 during the run. The loops are owned by the runner script and stopped by its EXIT/INT/TERM/HUP trap (0yesprocesses afterwards). Logs: scratchpadG65/load-run-*.log.Evidence
mise x bun@1.4.2;bun install --frozen-lockfile; nobun.lockchange.bun run --cwd packages/cli typecheck: 0 errors. Rootbun run check(includesprivacy:check): 0 errors; the 35 warnings are pre-existing and none is in this unit's files (the new??default was moved into a helper soinvokeNativeRuntimestays at complexity 20).origin/next..HEAD: no leaks.bun test tests/native-runtime-client.test.ts tests/native-project-process.test.ts --timeout 150000with privateHOME/HACK_HOME/DOCKER_CONFIGandHACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1: 41 pass, 0 fail.HOME): at 8dca82e:Ran 8188 tests across 569 filesin 53 min, 8029 pass / 126 skip / 33 fail. 26 of the failures were this unit's own regression in the logs/exec keeper fixtures, fixed in e4be689 (see Tests). The other 7 (tests/bun-crash-loop.test.ts,tests/mcp-publication-readiness.test.ts,tests/test-config-compiler.test.ts,tests/e2e/run-dependency-cache.test.ts) were an artifact of my runner's PATH: theirbunsubprocesses resolved to the system Bun 1.3.14 and refused the 1.4.2 pin. All four files pass on a serial rerun with--timeout 60000and the mise bun on PATH (25/6/39/5 pass, 0 fail).package.jsonscripts or pinned toolchain files changed, so no reference, plugin or catalog regeneration applies.Gaps
-x) as the parent of the fix commit, and the diff againstnextcurrently includes it. Once fix(native): keep the closed exit code when the graph owner's pipes drain late #396 merges, this branch rebases ontonextand keeps fix(native): keep the closed exit code when the graph owner's pipes drain late #396's behaviour; the shared constant then moves fromnative-project-process.tstonative-runtime-client.tsin this PR only.exitDrainBoundMsis a test seam on two internal boundaries; no production caller sets it. TheBun.spawnchild's exit observation is still the bound's origin, so a child that never exits stays governed by the request timeout, unchanged.Refs HACK-1387
🤖 Generated with Claude Code