Repository navigation
test: bound synchronous spawns in tests and name the command on timeout (HACK-2247) - #857
Conversation
…ut (HACK-2247) A synchronous spawn blocks the event loop, so Bun's per-test timeout cannot fire while it waits. On macOS a test child hung before it started and python-free-bootstrap waited for hours. - tests/helpers/bounded-spawn-sync.ts: spawnSyncBounded, a Bun.spawnSync with a default 30 s bound that kills with SIGKILL and throws an error naming the command. - python-free-bootstrap uses it for every spawn (the `sh -n` check had no bound). - The shared fixture helpers (fixture-git runFixtureGitSync, secret-fixture runFixtureGit, lifecycle-mux-fixture ps probes) are bounded. - tests/sync-spawn-bound.test.ts fails on any new synchronous spawn without a timeout in tests/ or scripts/check-*.ts; the 80 existing sites in 49 files are a ratchet that may only go down. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Review of #857: counting parentheses in raw text let a `(` or `)` inside a string move a call's boundary, so a bounded call could be reported and an unbounded one could borrow the next call's timeout. The check now parses each file with the TypeScript compiler API and counts a call as bounded only when one of its arguments is an object literal with its own `timeout` property. Strings and comments that mention spawnSync or timeout no longer count, so the baseline drops to 65 sites in 42 files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Ready for lead: head Ran
Not verified
For you to route
Test-only; no product code changed. |
|
Lead, one fact for the "not verified" item: on the Mac where the original hang happened, the stuck child (thread still at its first instruction after 5 h 20 min) ended within two seconds of a plain SIGTERM, and its parent test then failed normally. So a bound that signals the child does release this kind of hang. This PR goes into the next Mac batch. 🤖 Generated with Claude Code |
After merging next, tests/sync-spawn-bound.test.ts failed on two files that landed after this branch's base: tmux-socket-guard.test.ts (12) and nss-read-safety.test.ts (1). Bun.spawnSync sites use spawnSyncBounded; it still calls Bun.spawnSync, so the tmux socket guard under test still applies. The Node execFileSync/spawnSync/execSync calls, and the Bun.spawnSync call that must throw from the guard, keep their API and get an explicit timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Ready for lead: head Answer: #797, #795, #852 and #858 add no unbounded synchronous spawn. I merged each PR head onto 6dc8bf0 in a throwaway checkout, each alone and then all four together, and ran What changed since 78f14a4
Ran (Linux)
Not waited for
|
What
HACK-2247 item 1. A synchronous spawn blocks the event loop, so Bun's per-test timeout cannot fire while it waits. On macOS a test child hung before it started (the system policy daemon crashed while it was being started) and
tests/python-free-bootstrap.test.tswaited for five hours.tests/helpers/bounded-spawn-sync.ts:spawnSyncBounded(cmd, options)isBun.spawnSyncwith a default 30 s bound. At the bound it kills the child with SIGKILL and throwsSyncSpawnTimeoutError, whose message names the command (for examplegit commit -m … did not finish within 30000 ms and was killed). Every other result is returned unchanged.tests/python-free-bootstrap.test.ts: every spawn goes through the helper. Most already had a 10 stimeout, but a timed-outBun.spawnSynconly showed up later as an unexplained exit-code mismatch, and thesh -nsyntax check had no bound.fixture-git.tsrunFixtureGitSync,secret-fixture.tsrunFixtureGit, and thepsprobes inlifecycle-mux-fixture.ts(including the one in its generated stand-in script).checkout-write-guard.tswas already bounded.tests/sync-spawn-bound.test.ts: a lint-style test. It parsestests/**/*.tsandscripts/check-*.tswith the TypeScript compiler API and fails on any newBun.spawnSync/spawnSync/execFileSync/execSynccall that has no object-literal argument with its owntimeoutproperty. Strings and comments neither hide a call nor give it a bound. The 65 existing sites in 42 files are listed per file as a ratchet: a file may not gain a site, and a file that loses one must lower its count.There is no product change. Items 2 to 4 of the ticket need a Mac and stay with the lead.
Limits
timeout). Spawns inside generated fixture scripts (code held in a string) are not seen. AsyncBun.spawn+await exitedis not covered.spawnSyncwhile it waits to reap. Whether the macOS hang (a child at its first instruction while the policy daemon is down) is killable is not verified here; that needs the Mac.Validation (Linux)
tests/bounded-spawn-sync.test.ts: asleep 30child is killed at 200 ms and the error namessleep 30. A child that SIGSTOPs itself before doing anything (a stand-in for "never starts") is killed at the bound. Normal and failing results come back unchanged.Bun.spawnSync(["/bin/sh", "-c", "printf (("])to a test file makessync-spawn-boundfail withtests/python-free-bootstrap.test.ts: 1 unbounded (0 known). Reverted. The scanner's own test covers the reviewer's cases: parentheses inside strings,timeoutin a comment or string, a nestedtimeout, and a shorthand{ timeout }.bun run typecheck,tsc -p packages/cli(it includestests/),bun run check, andbiome checkon the changed files.Not verified: macOS.
Follow-up: remaining unbounded synchronous spawns (tests and check scripts)
42 files, 65 sites (the ratchet list in tests/sync-spawn-bound.test.ts)
tests/background-command-journal.test.ts(1)tests/background-command-process.test.ts(1)tests/ci-change-scope.test.ts(5)tests/compose-startup-phases.test.ts(1)tests/daemon-detach.test.ts(1)tests/docker-context.test.ts(1)tests/doctor-adopted-generated-files.test.ts(1)tests/doctor-fix-adopted-project.test.ts(1)tests/e2e-shard.test.ts(2)tests/e2e/run-dependency-cache.test.ts(1)tests/env-protection.test.ts(1)tests/env-value-stdin-cli.test.ts(1)tests/fixture-hermeticity.test.ts(1)tests/gateway-tokens.test.ts(1)tests/git-info-exclude.test.ts(1)tests/lifecycle-command-child.test.ts(2)tests/lifecycle-json.test.ts(1)tests/lifecycle-private-output.test.ts(3)tests/mcp-native-adapter.test.ts(3)tests/mcp-startup-supervision.test.ts(2)tests/native-compose-adoption-worktrees-fixture.test.ts(1)tests/native-interactive-launcher.test.ts(1)tests/native-preflight-cli.test.ts(1)tests/native-project-branch-cli.test.ts(1)tests/native-up-already-running-cli.test.ts(1)tests/portable-bootstrap-env-refusal.test.ts(1)tests/pre-commit-hook.test.ts(2)tests/prerelease-plan.test.ts(1)tests/project-down-worktree-safety.test.ts(1)tests/project-env-layer-lock.test.ts(1)tests/project-env-worktree-local.test.ts(1)tests/project-target-inspect.test.ts(1)tests/projects-registry-worktrees.test.ts(1)tests/projects-registry.test.ts(1)tests/qualify-provider-pin.test.ts(1)tests/registry-touch-worktrees.test.ts(1)tests/run-exec-branch-default.test.ts(1)tests/session-list-stop-tmux.test.ts(2)tests/test-shard.test.ts(4)tests/tmux-backend-locale.test.ts(2)tests/upgrade-apply.test.ts(1)tests/upgrade-check.test.ts(7)For routing: product paths that wait on a child with no bound
This is a read-only audit of
src/andindex.tsatorigin/next8e7242d. Line numbers point at the spawn. Nothing here is changed by this PR.src/ui/gum.ts:172Bun.spawnSyncgumlog/stylesrc/ui/gum.ts:185Bun.spawngum(style/join/format/table/version)exitedsrc/daemon/launchd.ts:127Bun.spawnlaunchctlwithProcessDeadline; macOSsrc/lib/shell.ts:65(exec),:187(run);:526execOrThrowdelegates toexecBun.spawntimeoutMsis optional and unbounded by default outsidewithProcessDeadline. Roughly 160 callers pass none. macOS examples:securityatsrc/lib/nodes-registry.ts:492andsrc/commands/global.ts:4468,brew --prefixatsrc/lib/native-hack-local-route-host.ts:384,psatsrc/lib/project-lifecycle-controller.ts:1530src/ui/clipboard.ts:49Bun.spawnpbcopy/wl-copy/xclipexitedsrc/daemon/process.ts:249Bun.spawnps -axosrc/backends/native-project-recovery.ts:141Bun.spawn/bin/ps -axosrc/lib/secrets-hygiene.ts:675Bun.spawngit rev-parsesrc/lib/lifecycle-command-child.ts:62Bun.spawnsh -clifecycle hooksrc/commands/gateway.ts:1403,:1414,:1430,:1447Bun.spawnhack daemon start/stop, re-execsrc/commands/remote.ts:740,src/control-plane/extensions/enable.ts:165,src/control-plane/extensions/supervisor/commands.ts:1487Bun.spawnhackre-execsrc/control-plane/extensions/tailscale/commands.ts:333,src/control-plane/extensions/cloudflare/commands.ts:1046Bun.spawntailscale/cloudflaredprobessrc/tui/hack-tui.ts:2219,:2516,:2738Bun.spawnhack logsone-shot and actionssrc/ui/docker-logs.ts:391,:452Bun.spawndocker compose logs/tail(non-follow)Intentionally unbounded (interactive or long-lived by design) and not listed: foreground
run/tty paths,docker compose upin the foreground, log follow, thedocker eventswatcher, supervisor jobs, fzf/gum prompts, andsudopassword prompts. A kill timer only helps onceBun.spawnhas returned; a hang inside the spawn call itself would need a different guard.Release signal
test: test-only.Refs HACK-2247 (item 1).
🤖 Generated with Claude Code