Repository navigation
test: fail and contain process-state leaks between test files - #405
Merged
Merged
Conversation
Every file in `bun test` shares one Bun process. A test that changed PATH, HOME, HACK_HOME or cwd and then failed before its restore leaked that change into every later file, which then failed with "Executable not found in $PATH: git/bun" and hid the first failure (HACK-1327). - Preload tests/helpers/global-state-guard-preload.ts from bunfig.toml. It snapshots the environment, cwd, umask, exit code, guarded global functions and process listeners around every test and at every file boundary, fails the leaking test or the run with a value-free message naming the file, and restores the environment, cwd, umask and exit code so later files run clean. Replaced functions and listeners are reported but kept, because they may still sandbox a timed-out test's unsettled work. - Fix leaks it and a review found: fzf replaced the process.env object; the VM-file, retained-build and retained-source-bind helpers could fail setup after changing PATH/HOME with no restore registered; the retained routing and retained-file fixtures kept HACK_HOME/PATH on the fixture when their lifetime was unknown; the protected-files negative left a process.kill spy for later files. Refs HACK-1327 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…chctl shim per test The M3 full suite at the first guard commit failed 16 shared-host-lifecycle cases (helpers reported as foreign listeners or never ready) and flagged tests/e2e-launchd-guard.test.ts for a process exit listener. - Root cause of the shared-host cascade: the engine-identity ambient-proxy test set HTTP(S)_PROXY and their lower-case forms, then restored them with a delete. Bun 1.4 copies assignments to proxy keys into the environment that fetch and Bun.spawn (without env) use, but never a delete, so every later loopback fetch in the process went through the dead proxy while process.env looked clean. restoreEnv now assigns "" before deleting. - The guard now also compares Bun's native environment at each file boundary (one /usr/bin/env child per file), names the file, and turns proxies off natively with NO_PROXY=* because Bun cannot take a deleted key back. Restores of deleted keys assign "" first as well. - The e2e harness's launchctl shim registered a process exit listener that lived for the whole Bun process; releaseLaunchctlGuard removes the shim and listener, and e2e-launchd-guard calls it after each test. Refs HACK-1327 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve bunfig.toml: keep both test preloads, this branch's global-state guard (HACK-1327) and next's checkout-write guard (HACK-1343, #390). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`lokiTail` registered a `process.once("SIGINT")` handler and never removed
it, so every follow that ended by socket close (the normal path) left a
stale handler in the process. In the shared `bun test` process the next
`process.emit("SIGINT")`, in tests/native-authored-project-oneoff.test.ts,
consumed those `once` handlers and the global-state guard blamed that test
for a listener change it did not cause. `runAuthoredCommand` in
src/lib/native-authored-command.ts is symmetric; the one-off file passes on
its own.
The handler is now installed only while the tail is open and removed in a
`finally`, so a SIGINT-fired (self-removing) handler and a normal close both
leave the listener list as it was. Two regression tests cover both paths
and fail without the fix. The guard now reports listener counts
(`2 -> 0`) so a consumed handler from an earlier file is distinguishable
from one added by the test it names.
Refs HACK-1392, HACK-1327
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
roodboi
marked this pull request as ready for review
October 24, 2026 02:43
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
#405 (HACK-1392) landed its own fix for the Loki follow SIGINT leak: the handler is installed just before the tail awaits close and removed in a finally. Keep that version and drop this branch's close-handler removal and its two SIGINT tests in favour of next's; keep the injectable notice scheduler, the Alloy positions volume and the selector-carrying hints. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 24, 2026
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
Follow-ups to #433 (HACK-1375) and #435 (HACK-1376), both already on `next`. ## Root cause 1. **The unset compare-and-swap was not tested.** `unsetProjectEnvValue` passes `expectedExisting: text` to the layer write. If you delete that line, every lock, hydrate and config test still passes, because only the set path had a paused-writer conflict test. 2. **Some layer locks can never be reclaimed, and the message did not say so.** `withEnvLayerLock` reuses the projects-registry owner protocol. That protocol never removes a malformed receipt, a receipt it does not trust (another uid, or writable by group or others), a receipt whose PID now belongs to a live process, or a dead owner behind a leftover `<lock>.recovery` guard. In each of these cases every `env set/add/unset/hydrate` on the layer waited 10 s and then printed `Another hack env command is writing <file>; … Retry when it finishes.` No retry could ever succeed, and the message did not name the lock file. 3. **A key can become unignored after a later `git init`.** In a non-git directory, the first `--secret` creates the key and writes its root `.gitignore` entry. Outside git, an existing key deliberately triggers no `.gitignore` write. So if `.gitignore` is deleted and the directory then gets `git init`, the key is not ignored, and Hack is not running at that moment. 4. **The re-include refusal named the wrong cause.** The ignore probe that runs after the entry is in place produced the same "Hack could not add it to <.gitignore>" message as a failed write. That is wrong when `.gitignore` already contains `.hack.secret.key` followed by `!.hack.secret.key`. ## Fix - `src/lib/env-layer-lock.ts`: `EnvLayerLockTimeoutError` now has a `reason` (`held` | `recovery_guard`) and the `lockPath`. The message is still one value-free line with no stack: - `held`: `Another hack env command is writing <file>; gave up after waiting 10s for its lock at <lock>. Nothing was written. Retry when it finishes. If no hack env command is running, Hack cannot reclaim that lock itself: remove <lock> and retry.` - `recovery_guard`: when the registry reports a dead owner behind a busy guard, the message keeps the registry's `recovery guard is busy (interrupted recovery requires offline inspection)` detail and names both `<lock>.recovery` and `<lock>` as the files to remove. - The lock helper still never removes anything itself. `src/lib/projects-registry-lock.ts` is unchanged; the classification reads the registry's existing timeout detail. That file belongs to G73 / HACK-1377. - `src/lib/project-env-config.ts`: `projectEnvKeyUnignoredError` now takes a `cause`. `unwritable` keeps the existing message. `reincluded` says the root `.gitignore` lists the key but a later rule in that file re-includes it, and asks the user to remove that rule. Once the entry is present, no deeper ignore file can apply to a key at the project root, so a later rule in that same file is the only possible cause. - `src/commands/doctor.ts`: exports `checkProjectEnvKey` so the real doctor check can be tested end to end. It also moves a misplaced docblock onto that function. The check's behavior is unchanged. - `docs/env.md`: documents unreclaimable locks and the recovery step, the re-include cause, and the `git init` decision. It also adds an `env-key-unignored` example to the doctor table. ### Decision on a later `git init` (item 3) Watching for `git init` is out of scope for env writes, because Hack runs no code when it happens. Outside git, an existing key still needs no entry, and Hack does not recreate a root `.gitignore` the user deleted. That behavior is unchanged and covered by the existing test. The guarantee comes back at the next Hack touchpoint: - `hack doctor`'s env key check reports `env-key-unignored` (warn) with a remedy. This check already existed; the new test proves it catches this exact sequence. - The first `--secret` set or hydrate in the new repository adds the entry before it uses the key, or it refuses (#433 behavior). ## Tests (each proven by mutation) | Test | Mutation that makes it fail | | --- | --- | | `project-env-layer-lock`: *an unset refuses with a conflict when a writer that takes no lock changed the layer after its read*. A `spyOn` pauses `resolvePrimaryLocalProjectDir`, the one awaited call between the local unset's read and its write. The test edits the layer, resumes, and expects `AtomicWriteConflictError` with the edit kept. | delete `expectedExisting: text,` from the unset: 1 fail | | `project-env- layer-lock`: *a layer lock Hack cannot reclaim…* uses three receipts: malformed, group-writable with an exited PID, and PID 1 standing in for a reused PID. Set and unset each refuse with the exact message, the receipt is left untouched, and removing the named lock lets the retry succeed. The existing held-lock test asserts the new message too. | restore the old message: 2 fail | | `project-env-layer-lock`: *a dead owner behind an interrupted recovery guard…* covers a set (300 ms wait) and a hydrate (default 10 s): both give the `recovery_guard` message with both paths, write nothing, and leave the lock and guard in place. After both are removed, the set succeeds. | classify the guard case as `held`: 1 fail | | `project-env-key-ignore`: the re- include test now expects the `reincluded` message, does not contain "could not add it", and checks that `.gitignore` was not rewritten. | send `unwritable` for the post-entry probe: 1 fail | | `project-env-key-ignore`: *a key whose entry was removed before a later git init…* runs the exact reported sequence through the source CLI, then the real `checkProjectEnvKey`: status `warn`, `env-key-unignored`, and no key bytes or absolute root in the output. The next `--secret` set repairs the entry and doctor returns `ok`. | drop `inspectLocalKeyIgnored` from doctor: 1 fail | Every fixture uses a private HOME, HACK_HOME, global config and Docker config, `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`, and git without global or system config. The env layer is written before any `--secret`. The in-process doctor call swaps in the same private environment and restores it afterwards. ## Evidence - Manual repro before the fix: (2) dead owner plus guard, and a malformed receipt, gave the old "Retry when it finishes." message. (3) after `git init`, `git check-ignore` exits 1 and `git status` lists `.hack.secret.key`. (4) gave the "could not add it" message. After the fix, the same script prints the messages above, and removing the named lock makes the retry exit 0. - Focused: `tests/project-env-layer-lock.test.ts` 12/12 and `tests/project-env-key- ignore.test.ts` 8/8 pass locally. - `bun run check` (includes `privacy:check`) exit 0. `bun run --cwd packages/cli typecheck` exit 0. gitleaks 8.30.1 (`gitleaks dir` on the `git log -p origin/next..HEAD` export): no leaks. - The CLI surface and consumer agent guidance did not change, so no reference or plugin regeneration was needed. - Full suite: hosted CI `test` at this PR head (see checks). ## Gaps - In a wider local run of 11 related env/doctor files, 13 tests in `project-env-write-refusals`, `project-env-stable-ciphertext` and `project-env-worktree-key` failed with `beforeEach/afterEach hook timed out` while the host load average was 51–60 on 18 cores. None of them asserts a message this PR changes. Hosted CI is the evidence for them. - The refusal is still a `ClosedCliError` with no `E_*` code (as before). Adding a lock-busy code would change the error-code registry and its generated docs, so it is left for a separate decision. - Classifying `recovery_guard` depends on the registry's existing timeout text. If G73 rewords that text, the guard test fails in the merge train rather than silently falling back to `held`. - The `held` message cannot tell a live writer from an unreclaimable lock without parsing the receipt (code G73 is editing), so it covers both cases: retry, or remove the lock when no hack env command is running. - The unset CAS test uses `spyOn` on an ESM namespace export (`worktree-local-config.ts`), in process and restored in `finally`. ## Round 2: merge with current next While this PR was being verified, `next` moved from 7d9c89f to 5208ee5 (#404, #446, #430, #439, #421, #448, #452, #405), and the PR became `CONFLICTING`. - Merge commit 7ac7cbb merges `origin/next` (5208ee5) into the branch. The only conflict was the `checkProjectEnvKey` docblock in `src/commands/doctor.ts`, which #448 also moved. It is resolved to this PR's side: the extended docblock plus `export async function checkProjectEnvKey`. Compared with next, `doctor.ts` now differs only in those lines. - No other code changed in this round. The diff against next is still the same 6 files. - Gates on the merged head: `bun run check` (includes `privacy:check`) exit 0. `bun run --cwd packages/cli typecheck` exit 0. Focused tests `project-env-layer-lock`, `project-env-key- ignore` and `cli-spawn-env-isolation` pass 27/27 (`--timeout 120000`) under #405's new preloads. gitleaks 8.30.1 (`gitleaks dir` on the `git log -p origin/next..HEAD` export): no leaks. - Full suite: hosted CI run 40056707145 at 7ac7cbb (base 5208ee5): `test` passed in 38m45s, plus docker-e2e 1-4/4, Candidate core (ubuntu), Config compiler (ubuntu x64/arm, macos-15-intel), Runtime state models, linux-process-lifetime, runtime-images and secret-scan. The two `macos-latest` jobs were still queued for a runner after about an hour. The earlier green run 39978726682 was at ed877f5, before the base moved, so it does not count for this head. Refs HACK-1386 🤖 Generated with [Claude Code](https://claude.com/claude-code)
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
…60 min limit (#442) ## Root cause The hosted `test` job ran every `bun run test` file (568 root `tests/` files plus `packages/db`) in one Turbo task on `blacksmith-6vcpu-macos-15`. That step alone took about 39 min (next run 39873410893: `Tests (Turbo)` 23:42:18 to 00:21:20). Slower runners reached the 60 min job limit and were cancelled. Turbo buffers task output until the end, so a cancelled run did not show which file was running. ## Fix - `scripts/test-shard-plan.ts` finds the same file set that `bun run test` runs: root `tests/` (with `bunfig.toml` `root = "./tests"` and its preload) and `packages/db` (from its own directory). It splits the files into shards, longest first. Durations come from `scripts/test- shard-durations.json`. That table was first built from the per-test times in the hosted log above. A file with no recorded duration is planned at 5 s, and each file adds 0.3 s for process start. The split is deterministic. - `scripts/test-shard.ts run --shard=i/4`: - runs `check:bun-version` and `test:preflight` first, as `bun run test` does; - runs each file with `bun test ./<file>` in its own process, from the file's suite directory, so the bunfig preload and the db path aliases apply; - prints `start` and `PASS`/`FAIL` lines with the file's duration and the shard's elapsed time, and a heartbeat every 2 min while a file runs; - stops a file after 12 min, and stops the shard at 30 min or on SIGINT/SIGTERM (which is what a GitHub cancellation or timeout sends). It then prints the running file, the files not started and the 10 slowest files; - fails when a file fails, times out, or Bun does not report exactly 1 file run; - writes per-file durations, uploaded as the `test-shard-durations-<i>` artifact. `update- durations` merges those artifacts back into the table. - Coverage check, run by every shard before it starts and by `verify --count=4`: the files found on disk must equal the files `git ls-files` lists. The union of all shards must equal that set, with no file in two shards. The set of workspaces with a `test` script, and the bunfig test root, must match the known suites. - `.github/workflows/ci.yml`: - adds the `test-shard` matrix (`test shard i/4`, `fail-fast: false`, `timeout-minutes: 40`). It keeps the existing runner, Bun 1.4.2, frozen install, Rust 1.97.1 and the compiler build on every shard. Typecheck, privacy, lint, the CLI build and the release smoke run on shard 1 only. - `test` is now a fan-in job (`needs: test-shard`, `if: always()`). It passes only when every shard succeeded, so the required check name `test` does not change. No admin change to the ruleset is needed. - `scripts/prerelease-plan.ts` `CI_JOBS` now also requires each `test shard i/4` job, the same way it requires the `docker-e2e` shards. ## Tests `tests/test-shard.test.ts` (37 tests) covers: - shard parsing and test-file name matching; - partition determinism, exact cover, balance, the fallback duration, and zero-second files being spread across shards; - the coverage check, with drop, duplicate and extra-file cases; - fixture git checkouts for drift, ignored files and a missing suite root; - the current checkout's 4-shard plan against `git ls-files`; - the workspace and bunfig checks; - the runner: per-file lines, file timeout, deadline, interrupt, the stop report, and the file- count rule; - real `bun test` child processes: exit status, suite cwd, stopping a hung file, and not waiting for a leftover process that holds stderr open; - the workflow wiring and the prerelease gate. Mutation proofs (each mutation was applied, the focused file was run, then the file was restored). Each one made at least one test fail: | Mutation | Failing tests | | --- | --- | | partition drops the last file | 7 | | no per-file start cost added | 2 | | coverage ignores missing files | 2 | | coverage ignores duplicate files | 1 | | no disk-vs-git drift check | 1 | | no workspace check | 1 | | any file count counts as a pass | 1 | | no file timeout | 2 | | stop report omits the running file | 1 | | `test` fan-in without `always()` | 1 | | prerelease gate drops one shard | 1 | | wait for stderr end of file | 1 | | no SIGTERM on abort | 1 | | wrong suite cwd | 1 | CLI mutation: when the partition drops a file, `bun scripts/test-shard.ts verify --count=4` exits 2 with `not run by any shard (1): tests/zig-aarch64-musl-linker.test.ts`. Restored, it exits 0 with all files in 4 shards (571 files when first written, 573 at `503dc18d`, 576 at `57736870`; planned about 10m25s to 10m32s per shard). ## Evidence - Bun 1.4.2 (`mise x bun@1.4.2`), Rust 1.97.1. - Local (loaded M5 Mac, private HOME/HACK_HOME/DOCKER_CONFIG, `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`): - `bun run check` (includes `privacy:check`) passed; - `bun run --cwd packages/cli typecheck` passed; - actionlint 1.7.12 found nothing on the 4 workflows; - focused tests passed: `test-shard`, `prerelease-plan`, `e2e-shard`, `native-plan-cli- wiring`, `ci-bun-crash-census` and `agent-instruction-source` (135 tests); - gitleaks 8.30.1 over `origin/next..HEAD` found no leaks. - Full suite: hosted CI is the full-suite evidence for this unit, so no M3 run is needed. Each hosted run executes every test file across the 4 shards; the fan-in `test` check passes only when all 4 shards pass. - Hosted shard runs: see "Hosted runs" below (3 runs, 2 of them green). ## Hosted runs Times are the `Tests (shard)` step; the whole job adds about 1 to 1.5 min of setup. Every shard job has `timeout-minutes: 40`. | Run | Head | Shard 1/4 | Shard 2/4 | Shard 3/4 | Shard 4/4 | Result | | --- | --- | --- | --- | --- | --- | --- | | [39888691204](https://github.com/hack-dance/hack/actions/runs/39888691204) | `a33511f4` | 11m48s (failed, test bug) | 11m39s | 10m32s | 12m57s | red: one test, see below | | [39977193863](https://github.com/hack-dance/hack/actions/runs/39977193863) | `503dc18d` | 11m49s | 11m30s | 11m44s | 12m16s | green, all 20 checks | | [40057462690](https://github.com/hack-dance/hack/actions/runs/40057462690) | `57736870` | 10m12s | 10m39s | 9m49s | 11m29s | green, all 20 checks | Whole shard jobs took 10m34s to 13m54s across the 3 runs, well under the 40 min timeout. For comparison, `next`'s serial `test` job ([39916341281](https://github.com/hack- dance/hack/actions/runs/39916341281), `7d9c89f8`) took 40m17s, with `Tests (Turbo)` at 39m21s, and run 39783802503 was cancelled at 60m25s. Same file set as the serial job: the shard logs of run 39977193863 list 573 unique PASS files. `next`'s serial log (39916341281) lists 572 files; the extra one is the new `tests/test- shard.test.ts`. Unique skipped tests are the same 157 names on both. Passed tests are 8057 on `next` and 8094 on the PR; the 37 extra are the new `test-shard` tests. Running each file in its own process did not skip anything new. ### Why run 39888691204 was red Shard 1 failed one test in `tests/project-exec-command.test.ts`: "exec warns value-free on stderr only after the service env changed since start". The clack CoreDNS/Caddy warnings went to stdout instead of stderr. This was a test that depended on file order, not a sharding bug and not a known flake. In the old single `bun test` process, an earlier file had already set `HACK_LOGGER=console`. Run on its own, on a host without `gum`, the logger falls back to clack, which writes to stdout. Repro on `origin/next`, running only that file with `gum` removed from `PATH`: 4 pass, 1 fail, the same test. Fix (`503dc18d`, `test: pin the console logger in the exec env-change test`): the test file sets `HACK_LOGGER=console` in `beforeAll` and restores the previous value in `afterAll`. With the fix the file gives 5 pass on its own; with `next`'s version of the file put back it gives 4 pass, 1 fail again. Run 39977193863 at `503dc18d` is green. ## Round 2 (verifier follow-up) - The verifier found that this body still said PENDING for the hosted and full-suite evidence, quoted 571 files instead of the plan's count, and did not explain the red run. This body now lists the hosted runs, the red-run cause and fix, and drops the M3 item. - File counts: the plan had 571 files when the CLI mutation above was first written, 573 at `503dc18d`, and 576 after merging `next` at `5208ee54` (which adds #405, the global-state- guard preload, and #452). - New head `57736870`: merge of `next` `5208ee54` into `503dc18d`. The merge was clean; no PR file changed. - Local gates at `57736870` (Bun 1.4.2, private HOME/HACK_HOME/DOCKER_CONFIG, `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`, a bogus `DOCKER_HOST`): - gitleaks 8.30.1 `dir` over the `git log -p origin/next..HEAD` export: no leaks; - `bun run check` (includes `privacy:check`): 2 of 2 tasks passed; - `bun run --cwd packages/cli typecheck`: exit 0; - focused tests `test-shard`, `prerelease-plan` and `project-exec-command`: 99 pass, 0 fail; - `bun scripts/test-shard.ts verify --count=4`: 576 files in 4 shards, each file exactly once, planned about 10m31s per shard. 7 files from `next` have no recorded duration yet and use the 5 s fallback. - Hosted CI at `57736870`: run [40057462690](https://github.com/hack- dance/hack/actions/runs/40057462690): all 20 checks green, including `test shard 1/4` to `4/4` and the `test` fan-in. The shard logs list 576 unique PASS files and no FAIL line, which matches the local plan. Shard jobs took 10m34s to 12m11s. - Required check: still `test` (ruleset on `next`). The fan-in job keeps that name, so no admin change is needed. ## Round 3 (re-check) - Remote head is still `57736870`, and `next` is still `5208ee54`, which is an ancestor of the head. No merge or new commit was needed, so nothing was pushed this round. - Re-checked this body against GitHub: `gh pr checks 442` shows all 20 checks green at `57736870`. The `Tests (shard)` step times of run 40057462690 match the table (10m12s, 10m39s, 9m49s, 11m29s). Its shard logs list 576 unique PASS files and no FAIL line. - `bun scripts/test-shard.ts verify --count=4` at `57736870`: 576 files in 4 shards, each exactly once, planned 10m31s to 10m32s per shard. - The Tests section above now gives the file count at each head instead of only the first count (571). ## Gaps - Each file now runs in its own process instead of sharing one `bun test` process. Files no longer share module state or global hooks across files. That is stricter isolation, so a test that depended on another file running first in the same process would now fail. The red run 39888691204 was such a case: a test passed in the shared process only because an earlier file had set `HACK_LOGGER=console`. It is fixed in `503dc18d` (see "Why run 39888691204 was red"). The green hosted runs are the evidence for the current tree. - `scripts/ci-bun-crash-census.ts` matches one exact job name, and its default is `test`. For runs after this change the Bun logs are in the `test shard i/4` jobs, so the census needs `--job "test shard i/4"` once per shard. This is documented in the verify skill. The script itself is not changed here. - After a stop, a child that ignores SIGTERM is SIGKILLed after 10 s. Its own descendants are not process-group killed, because changing process groups could change the lifecycle tests. Refs HACK-1381 🤖 Generated with [Claude Code](https://claude.com/claude-code)
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
…leak fixes (#464) Follow-ups to the global-state guard from #405 (HACK-1416, HACK-1388). Only test infrastructure, test fixtures and contributor guidance change. No product code changes. ## Root cause - **HACK-1416 (1), guard cascade.** At a file boundary the guard restored env, cwd, umask and exit code, but not process listeners. A listener that a finished file left behind (for example a `once` SIGINT handler) stayed in the shared Bun process. The next test that called `process.emit` for that signal ran or consumed it, and the per-test check blamed that later test (`1 -> 0`). - **HACK-1416 (2), test fidelity.** `Loki follow stops on SIGINT and leaves no listener behind` calls the handler from `process.listeners()` directly. That skips the `once` wrapper's self- removal, so the follow's `finally` `off()` does the removal, not the real signal path. - **HACK-1388 (1).** `a refused teardown still restores HACK_HOME ...` called the global `mock.restore()`. That restores every spy in the shared process, including spies other fixtures keep on purpose while their settlement is unknown. It also ran through raw `boundedTest`, so it skipped the file's `fixtureActive`/`fixtureUncertain` gate. - **HACK-1388 (2).** Three leak fixes from #405 were checked only by the runtime guard, not by a test of their own: the file-generation `afterEach` PATH restore, the routing runner's HACK_HOME restore for an unknown lifetime (the issue calls it the "routing worktree restore"), and the protected-files fixture's `process.kill` restore after the settlement veto latches (the "settlement-latch restore"). ## Fix - `tests/helpers/global-state-guard.ts`: new `restoreProcessListeners(snapshot)`. It edits only the difference: it removes listeners added since the snapshot and adds back missing snapshot listeners with `process.on`. Listeners present in both are left alone, so a kept `once` listener stays a `once`. Bun does not expose `once`, so a missing listener comes back as a persistent one, at the end of the list. Removing a leftover listener cannot let unsettled work reach the host. Replaced functions are still never restored. - `tests/helpers/global-state-guard-preload.ts`: at a file boundary, after reporting a change, the guard now calls `restoreProcessListeners(fileBaseline)` next to `restoreGlobalState`. The message now says "Restored the environment, cwd and process listeners ...". The per-test check is unchanged: it still fails the test and keeps listeners. The guard still fails the run; no allowlist, skip or special case was added. - New `tests/helpers/owned-process-listeners.ts`: `withOwnedProcessListeners(event, run)` removes the listeners present for `event`, runs `run`, and adds them back afterwards. A real `process.emit` inside `run` then reaches only listeners `run` installed. Listeners `run` leaves behind are kept, so the test and the guard still see them. - `tests/native-compose-adoption-routing-generation.test.ts`: - The gate is now `gatedTest(name, run(restored), timeoutMs)`. A case's gate reopens only if it calls `restored()` after every global double and root it created is restored. The unknown- lifetime, incomplete-setup and failed-cleanup handling is now `endRoutingFixture(lifetime, fixture)`. The ordinary runner `test(...)` uses both, with the same behavior as before. - The refused-teardown case now runs through `gatedTest`, ends its fixture through `endRoutingFixture` (the runner's own unknown-lifetime path), and restores only its own fixture's spies with the new `fixture.restoreDoubles()`. `mock.restore()` and the `mock` import are gone. - `tests/helpers/retained-routing-adoption.ts`: adds `restoreDoubles()` (restores this fixture's spies only, and `cleanup()` uses it too) and `doublesInstalled()`. - `tests/native-compose-adoption-file-generation.test.ts`: the `afterEach` body is now the named `releaseRetainedFileFixture()` (same logic), so a test can drive it. - `.ai/skills/hack-repo-verify/SKILL.md`: says listeners are put back at the file boundary, and points to `withOwnedProcessListeners` for tests that emit a real signal. ## Tests - `tests/global-state-guard.test.ts` - New unit case: `restoreProcessListeners` removes two leftovers (`on` and `once`), adds back a removed snapshot listener, reports only the changed event, and is a no-op the second time. A kept `once` listener still removes itself on a real `process.emit("SIGUSR2")`. This runs inside `withOwnedProcessListeners("SIGUSR2")`. - The nested `bun test` suite gets `c2-leak-signal-listener.test.ts`, which leaves a module- scope `once` SIGUSR2 listener. `d-later.test.ts` gets a test that emits SIGUSR2 around its own listener. With the guard, the run names `c2` (`process 'SIGUSR2' listeners changed (0 -> 1)`, plus the restore message), the later test passes, nothing blames `d-later.test.ts`, and 3 files are reported. - `tests/loki-short-lived-logs.test.ts`: new `Loki follow drops its SIGINT listener through a real SIGINT emit`. Inside `withOwnedProcessListeners("SIGINT")`, it opens a follow (exactly 1 SIGINT listener), calls `process.emit("SIGINT")`, and checks that the count is 0 at once, before the tail closes. Then the follow exits 0 and the count is still 0. - `tests/native-compose-adoption-routing-generation.test.ts`: the refused-teardown case installs a pass-through `spyOn(privateState, "readPrivate")` that stands for another fixture's spy, and checks that it is still installed after teardown. It also checks that the runner path restores HACK_HOME, that a refused `cleanup()` restores HACK_HOME and keeps the fixture's doubles, and that `restoreDoubles()` removes them. - `tests/native-compose-adoption-file-generation.test.ts`: new `an unknown lifetime still restores PATH before the teardown refuses`. It drives `releaseRetainedFileFixture()` with a retained (unknown) lifetime and checks: refusal, `uncertain` set, PATH back to its pre-case value, spawn spy kept. Its own lifetime then ends normally in `afterEach`. - `tests/native-config-protected-files-fixture.test.ts`: the closed-pipe survivor negative checks that `process.kill` is back to its pre-test function right after the veto's `signal.mockRestore()`. ### Mutation proofs (each applied and reverted with a script; Bun 1.4.2) | Mutation | Result | | --- | --- | | Preload never calls `restoreProcessListeners` | nested guard case fails: `d-later.test.ts` is blamed | | `restoreProcessListeners` keeps leftovers | unit case and nested case fail | | `restoreProcessListeners` never adds back missing listeners | unit case fails | | `restoreProcessListeners` does `removeAllListeners` and re-adds | unit case fails (the kept `once` turns persistent) | | Loki `process.once` becomes `process.on` (the `finally` `off` stays) | only the new real-emit case fails (count 1 right after the emit); both existing SIGINT cases still pass | | Loki SIGINT handler does not call `stop()` | new real-emit case fails (timeout) | | Loki `finally` `off()` removed | the two existing cases fail; the real- emit case passes, as expected (the `once` wrapper removes itself) | | Refused-teardown case uses global `mock.restore()` again | case fails: `readPrivate` is no longer the foreign spy | | `endRoutingFixture` skips `releaseEnvironment()` for an unknown lifetime | case fails: HACK_HOME is still the fixture home | | Gate already closed (`fixtureUncertain = true`) before the case | case refuses ("Prior retained routing fixture lifetime is unknown") — it is gated; on `next` it ran anyway | | Routing `cleanup()` restores HACK_HOME only after the lifetime check | case fails | | File-generation teardown restores PATH only after the lifetime check | new case fails: PATH is still the fixture root | | Protected-files negative drops `signal.mockRestore()` | case fails on its own `expect(process.kill)` (the guard also reports `process.kill replaced`) | ## Evidence - Bun 1.4.2 (`mise x bun@1.4.2`), this Mac, with private `HOME`, `HACK_HOME` and `DOCKER_CONFIG` and `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`. - `bun run --cwd packages/cli typecheck`: 0 errors. Root `bun run check`: passes (privacy-check ok; only existing warnings). `ultracite check` on the changed files: clean, apart from one existing complexity warning at `tests/helpers/retained-routing-adoption.ts:250` (unchanged code). - gitleaks 8.30.1 (`gitleaks dir` over a `git log -p origin/next..HEAD` export): no leaks. - Focused runs: `tests/global-state-guard.test.ts` 8/8; `tests/loki-short-lived-logs.test.ts` 21/21; the new routing, file-generation and protected-files cases pass on their own (the protected-files case passed 6 of 7 runs under host load of about 250; the one failure was not its new assertion, see Gaps). - The full routing-generation and file-generation files could not finish green locally: the host load average was 50-280 from other work, so slow cases hit their budgets (`known nonzero child ...` at 30 s, the HACK-1327 pattern) and each file's gate then refused later cases, as designed. The first file-generation case, `file8 refuses foreign receipt families ...`, times out at 15 s here with or without this change: the `origin/next` copy of the file fails the same way on this host. - **Hosted CI at `061638b6` (the PR head): `test` passes (38m15s; the full suite)**, plus all four docker-e2e shards, Candidate core (ubuntu), Config compiler (ubuntu x64/arm), Runtime state models, linux-process-lifetime, runtime-images and secret-scan. The macOS Candidate core and Config compiler jobs were still queued when this was written. - No CLI surface or consumer agent guidance changed, so no `docs:cli-reference` or `generate:agent-plugins` regeneration. ## Gaps - The per-test check still keeps a test's leftover listeners until its file ends. A later test **in the same file** that emits that signal can still be blamed. Use `withOwnedProcessListeners` for real emits. - `withOwnedProcessListeners` and `restoreProcessListeners` bring a removed listener back as a persistent one: Bun does not expose `once`. Between tests the guarded events normally have no listeners. - The gated refused-teardown case now refuses after an earlier routing case leaves an unknown lifetime, where before it ran anyway. That is the requested behavior; it adds one more line to an existing in-file cascade. - Under heavy local load (about 250), the protected-files `closed-pipe survivor` negative failed once on an existing `toBe` before reaching its new assertion (most likely its 2.5 s descendant-join deadline). This is existing load sensitivity and is not changed here. - HACK-1388 (3), `packages/db` running without the root `bunfig` preload, is info only and stays unchanged. Refs HACK-1416, HACK-1388 🤖 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.
Fixes the cross-file state leaks tracked in HACK-1327. Only test infrastructure, test fixtures and contributor guidance change. No product code changes.
Root cause
bun testruns every file in one Bun process, soprocess.env, the working directory, the umask,process.exitCode, global functions and process listeners are shared by all files. When a test changed one of them and failed (or timed out) before its restore ran, every later file inherited the change. With PATH pointing at a fixture directory, later files failed withExecutable not found in $PATH: git/bun(for exampletests/projects-registry-admission.test.ts,tests/env-backend-command.test.ts). Those cascades hid the first, real failure.Note:
Bun.spawnwithout anenvoption uses Bun's own copy of the environment, taken at startup. Code that passesenv: { ...process.env }, andnode:child_process, use the live (leaked) PATH. That is why only some later files failed.Sources found in this change:
tests/native-compose-adoption-file-generation.test.ts: when the fixture lifetime was unknown,afterEachthrew before restoring PATH, so PATH stayed on the fixture root (no git, no bun) for every later file.tests/helpers/retained-routing-adoption.ts(changed by test(adoption): give v14 compound routing operations the product's caller deadline #383) and its wrapper intests/native-compose-adoption-routing-generation.test.ts: with an unknown lifetime,cleanuprefused before restoring HACK_HOME, and the wrapper skippedcleanup, so HACK_HOME stayed on the fixture home.tests/helpers/retained-build-adoption.ts,tests/helpers/retained-source-bind-adoption.ts: PATH was changed before an awaited setup step; if that step threw, no fixture (and no cleanup) reached the caller.tests/helpers/native-compose-vm-files.ts: PATH, HOME, HACK_HOME and the Docker keys were deleted before the restore was registered, and one failing cleanup skipped the rest.tests/fzf.test.ts: replaced theprocess.envobject with a plain copy, which detached it fromBun.envand reset unrelated keys.tests/native-config-protected-files-fixture.test.ts: the intentional-unknown negative left a pass-throughprocess.killspy installed for every later file.tests/e2e/harness.tsviatests/e2e-launchd-guard.test.ts: the launchctl shim registered a processexitlistener for the whole Bun process (found by the guard on the M3 full run).shared-host-lifecyclefailures, helpers reported asforeign-listeneror never ready). The engine-identity ambient-proxy test (tests/native-compose-engine-identity.test.ts) setsHTTP_PROXY,HTTPS_PROXYand their lower-case forms, then restores them withrestoreEnv, which deleted them. Bun 1.4.2 copies assignments to the proxy keys (HTTP_PROXY,HTTPS_PROXY,NO_PROXY, lower-case too) into the environment thatfetchandBun.spawnwithoutenvuse, but a delete never reaches that copy, and after a delete Bun ignores later assignments to that key too. So every laterfetchin the process, loopback included, went through a dead proxy whileprocess.envlooked clean. Bisected on this Mac:engine-identity+shared-host-lifecyclealone gave 67 pass / 16 fail, with or without the guard preload. Full runs onnextpassed only becausetests/fzf.test.tsruns earlier and leftprocess.envas a plain copy, which Bun no longer reads: after it, proxy assignments never reachedfetchat all (checked with a standalone script). So the fzf fix above exposed this leak, and onnextthe engine-identity ambient-proxy case has not really exercised a proxy in full runs.Fix
bunfig.toml:tests/helpers/global-state-guard-preload.tswith the logic intests/helpers/global-state-guard.ts.process.envobject identity), cwd, umask,process.exitCode, guarded globals (fetch, timers,Date,Bun.spawn,Bun.spawnSync,Bun.which,process.exit,process.kill,process.platform) and the listeners of signal/exit/error process events.beforeEachruns before a file's own hooks and the preloadafterEachafter them, so a correctbeforeEach/afterEachpair passes. A mismatch fails that test with a message naming the file.onLoadfor test files is the file boundary (it runs after the previous file'safterAlland before the next file's module code; it returns the source unchanged). A mismatch is reported at once, naming the previous file, and the preloadafterAllfails the run with every leaking file. This catches module-scope andbeforeAll/afterAllleaks./usr/bin/env -0child per file, about 3 ms) and reportsnative env <KEY> changedfor a proxy key thatprocess.envno longer shows.NO_PROXY=*in Bun's native environment (and leavesprocess.envunchanged): later fetches go direct.env PATH changed,cwd changed,native env HTTP_PROXY changed), never values.restoreEnv(tests/helpers/env.ts) assigns""before deleting a key, so a proxy key ends empty (no proxy) in Bun's copy instead of keeping the test's value. This fixes the engine-identity source and every otherrestoreEnvcaller.releaseLaunchctlGuard()in the e2e harness removes the shim and itsexitlistener;e2e-launchd-guard.test.tscalls it after each test. The e2e runner still keeps one shim per process..ai/skills/hack-repo-verify/SKILL.md: explains the guard, says to fix the named file, and to restore env keys withrestoreEnv.next(616407a5); no conflicts.Tests
tests/global-state-guard.test.ts:bun testruns in a temp dir (stub executable on a temp PATH, temp HOME/TMPDIR, no proxy) compare a suite without the guard and with it. Without the guard, a file that sets PATH and throws makes a later file fail withExecutable not found in $PATH, and a file that setsHTTP_PROXYand deletes it makes a later loopbackfetchfail. With the guard, the run namesa-leak-in-test.test.ts(per test),b-leak-in-hooks.test.ts(per file,beforeAllwithout restore) andc-leak-native-proxy.test.ts(native env HTTP_PROXY changed), and both later checks pass.restoreEnvleaves no proxy in Bun's native environment, while a bare delete does (it fails if a Bun upgrade fixes the delete, so the workaround can go).tests/native-compose-adoption-routing-generation.test.ts: a refused teardown still restores HACK_HOME.tests/e2e-launchd-guard.test.ts: releasing the launchctl guard removes its shim andexitlistener.Mutation proofs (each applied and reverted)
native envmessage; later loopback fetch refused)disableNativeProxiesis a no-oprestoreEnvdeletes without the""assignmentnative-compose-engine-identity.test.ts(native env HTTP_PROXY/HTTPS_PROXY/http_proxy/https_proxy changed); with the guard's repairshared-host-lifecyclestill passes 83/83releaseLaunchctlGuardkeeps theexitlistenere2e-launchd-guardtest withprocess 'exit' listeners changed; the new release case failscleanuprestores HACK_HOME only after the lifetime checkreleaseEnvironmentis a no-opprocess.env = {...}The required injection (3) is kept as the nested regression tests above, so no injected test file remains in the tree.
Evidence
mise x bun@1.4.2).bun run --cwd packages/cli typecheck0 errors; rootbun run checkpasses (privacy-check ok; only existing warnings, none in changed files); gitleaks 8.30.1 overorigin/next..HEAD: no leaks.native-compose-engine-identity+shared-host-lifecycle: 83/83 (before the fix: 67 pass, 16 fail). Routing-generation + file-generation: 47/47.58ed88dd): 7,691 pass, 21 fail. The guard namedtests/e2e-launchd-guard.test.ts(exit listener; fixed). 16shared-host-lifecyclefailures (proxy root cause above; fixed). 1repository catalogfailure and 2new no-hostfailures also fail in every other M3 full run onnext(unrelated; see findings).8b207083: all 16 checks pass, includingtestand the four docker-e2e shards./has 57 GiB free (floor 58 GiB). This PR will be updated with the result.Gaps
dockeror resolvesHACK_HOME, it sees the real PATH and (when unset) the default~/.hack. Before this change it kept the fixture values and broke later files instead. The suite has no run-wide sandbox for HACK_HOME, DOCKER_HOST or DOCKER_CONFIG.envstill inherit the stale proxy key (next toNO_PROXY=*). The run fails anyway and names the file.mock.moduleandspyOnon module exports (and onDate.now) are process-global in Bun and are not tracked: the guard cannot see them. The retained-routing fixture deliberately keeps its module spies when its lifetime is unknown.onLoadfor each test file in order (verified on Bun 1.4.2). With--parallel, each worker gets its own preload. The native check needs/usr/bin/env; without it, it is skipped.Round G68 (HACK-1392): SIGINT listener leak found by the guard on
next+ this PRMerge gate b48 (#445) failed twice on
next+ this PR with four deterministic failures, reproduced locally in the same order (bun test tests/loki-short-lived-logs.test.ts tests/native-authored-project-oneoff.test.ts tests/global-state-guard.test.ts): the twoLoki follow ...tests from #417,public cancellation after run completion returns130 without rendering or replay, and the guard's(unnamed)file summary, each withprocess 'SIGINT' listeners changed.Root cause
One product leak, blamed on two files:
src/ui/loki-logs.tslokiTailregisteredprocess.once("SIGINT", () => stop())and never removed it. A follow that ends because the socket closes (the normal path, and what both fix(logs): escape Loki selectors once, quiet Alloy info logs and add empty-follow hints #417 tests do) left thatoncehandler in the process. Each follow test added one more.tests/native-authored-project-oneoff.test.tsruns after the Loki file and its cancellation test callsprocess.emit("SIGINT"). That fired the two stale Loki handlers, which removed themselves, so the SIGINT listener list changed (2 -> 0) during the one-off test and the guard named that test and file.runAuthoredCommandinsrc/lib/native-authored-command.ts(theprocess.on/process.offpair around line 920-965) is symmetric on every path; the other two registrations in that file (logs, line 98-141, and the startup wrapper, line 649-668) are too. The one-off file passes on its own with the guard active (50/50), and passes after the Loki fix when run after the Loki file.Fix
src/ui/loki-logs.ts: the SIGINT handler is a named function installed right before the tail awaits its close and removed in afinally, so a socket close, an error close and a SIGINT (where theoncehandler already removed itself andoffis a no-op) all leave the listener list as it was. Everything betweennew WebSocketand that await is synchronous, so the registration point has no observable change.tests/helpers/global-state-guard.ts: the listener message now carries counts,process 'SIGINT' listeners changed (2 -> 0), so a handler consumed by the named test (an earlier file's leak) reads differently from one it added (0 -> 1). Counts only, no values. No allowlist, skip or special case was added; the guard's own tests stay green.Tests
tests/loki-short-lived-logs.test.ts:Loki follow removes its SIGINT listener once the tail closes: one listener while the tail is open, the same list as before once the socket closes.Loki follow stops on SIGINT and leaves no listener behind: the follow's own handler (called directly rather thanprocess.emit, which would reach every listener in the shared test process) closes the tail with exit 0 and the list is back to the baseline.tests/global-state-guard.test.ts: the listener change line includes the counts.Mutation proofs (applied and reverted): with the
lokiTailfix reverted, both new tests fail on their ownexpect(and the guard names the file,0 -> 1,1 -> 2, ...); with the count change reverted, the guard test fails (Expected to contain: "process 'SIGUSR2' listeners changed (0 -> 1)").Evidence
mise x bun@1.4.2), this Mac, at9870275f: the three files above 77/77 with the preload active (before: 72 pass, 4 fail);tests/log-backend.test.ts5/5;bun run --cwd packages/cli typecheck0 errors; rootbun run checkpasses (privacy-check ok, 34 existing warnings, none in changed files);ultracite checkon the four changed files clean; gitleaks 8.30.1 (gitleaks dirover agit log -p origin/next..HEADexport): no leaks.next(a74eedbc); no conflicts.9870275f: see the PR checks.9870275f: queued behind other units on the shared M3 lock; this PR will be updated with the result.Gaps
process.emit("SIGINT")still reaches every SIGINT listener in the shared process, so another file's leaked handler would again be charged to that test; the counts in the guard message now make that readable, and the guard already names the leaking file at its own boundary.Refs HACK-1327, HACK-1392
🤖 Generated with Claude Code