Repository navigation
fix(service): bind Linux health sentinel recovery to pidfd - #7133
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
# Conflicts: # tests/fixtures/test-layout-expected.json
# Conflicts: # tests/fixtures/test-layout-expected.json
Bun can hold its own pidfd for a spawned child and close it when the child exits, so a count taken after spawning ends one lower. The final leak assertion now uses the count from before the fixture exists.
|
Maintainer integration into
|
Codex Review SummaryThis 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. |
Summary
Closes #6987.
Linux health-sentinel recovery now acquires a pidfd before arming, rechecks the live parent relationship with that fd held, and sends SIGKILL through pidfd_send_signal. A recycled numeric PID cannot redirect recovery. The watching log identifies pidfd mode.
The dependency-free bun:ffi adapter uses libc syscall on Linux x64/arm64, including musl. Only unsupported architecture or startup ENOSYS permits the existing live-parent fallback. Identity, permission, resource, and library failures stand down; a bound signal failure never falls back to a numeric kill. The runner releases its owner on every exit and exception. macOS retains its documented live-parent limitation and Windows retains its existing process-handle path.
Current dev was checked first (base 7640a94, including #7107); no existing open PR matched 6987. Tests cover acquisition races, unsupported/error distinctions, reused/exited targets, repeated recovery, fd zero, and teardown. Structure and public documentation are synchronized.
Verification
bun install— passed, no dependency or lockfile changes.bun test tests/service/service-health-sentinel-linux.test.ts tests/service/service-health-sentinel-policy.test.ts tests/service/service-health-sentinel-runner.test.ts tests/service/service-health-sentinel-process.test.ts tests/service/service-health-sentinel-launch.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 128 passed, 0 failed; 2 Linux-native tests excluded on the local macOS host.bun test tests/service/service-health-sentinel-linux.test.ts tests/service/service-health-sentinel-process.test.ts— 31 passed, 0 failed; 2 Linux-native tests excluded on macOS.bun run typecheck,bun run structure:check,bun run privacy:scan— passed.cd docs-site && bun install --frozen-lockfile && bun run build— passed, 577 pages and 81,200 internal links checked.OCX_TEST_NO_QUEUE=1 bun run test:changed --parallel=1— passed: 89 tests, 0 failures, 2 Linux-only exclusions on macOS, across 4 files. The original queued invocation was stopped only in this lane after multiple concurrent worktrees accumulated behind the global user lock; this is the runner's supported intentional-overlap setting with one worker, not a test bypass.ci.ymllane=all run; this PR stays draft pending its results and independent process-safety/security review.d23328ed3b5b880bd9775654f38b81224af13a09: PR Linux CI, lane=all Linux/macOS/Windows dispatch, service lifecycle — queued at the time of this update. Native Linux syscall/fd tests must actually execute there; local macOS success is not Linux acceptance. Native Linux arm64 and musl are not separately covered by the current hosted matrix.a3d3c7c80, after mergingorigin/devineea1e2422; conflict intests/fixtures/test-layout-expected.jsonresolved by keeping both sides' entries):bun test tests/service/service-health-sentinel-*.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 128 passed, 2 Linux-native skips on macOS;OCX_TEST_NO_QUEUE=1 bun run test:changed -- --parallel=1— 89 passed, 0 failed, 2 Linux-native skips;bun test tests/ci-workflows/file-size-ratchet.test.ts tests/ci-workflows/structure-ssot.test.ts— 60 passed;bun run typecheck,bun run structure:check,bun run privacy:scan— passed.a3d3c7c80: the PR CI triggered by this push, including the two native Linux pidfd tests actually executing (they skip on macOS). The earlier lane=all run targetedd23328ed3and does not cover this head; CI dispatch is on hold, so no new lane=all run was started.Security review (round 3)
Reviewed head
070fcdfdcmerged withdev. One defect fixed, everything else holds.a3d3c7c80): a throwing pidfd kill left recovery history unsettled.terminate()calledapi.signal(fd, SIGKILL)without a guard. The runner writes the durableintententry before callingterminateTarget, so an FFI exception would escape the runner, leaveintentunsettled, and kill the sentinel with an unhandled rejection. The numericprocess.killpath it replaced caught this case and returnedfailed. The pidfd handle now returnsfailedthe same way: the entry settles asfailed, the sentinel stands down, and it never falls back to a numeric PID. A regression case is in the existing exception test.pidfd_open: closed. The fd is opened first, andprocess.ppid === targetPid(plus stdin EOF) is checked again while the fd is held, after bothpidfd_openand the signal-zero probe. If the parent exits before the open, the kernel reparents the sentinel to an existing reaper, which can never carry the reused PID, so the recheck fails and the fd is closed. I confirmed that Bun 1.4.3 readsprocess.ppidlive: a child saw it change from its parent's PID to 1 after the parent exited.dlcloses the libc handle. A bound fd is released inrunHealthSentinel'sfinally(covered for EOF, refusal, exception, recovered, failed, not_target). Close is idempotent, never retried after EINTR, and blocks later signals.pidfd_openalways sets close-on-exec on the fd it returns (kernel behaviour, flags = 0), so the fd cannot leak into a spawned child.pidfd_open) and 424 (pidfd_send_signal) are in the shared syscall table from 403 onward, so they are the same on x86_64 and aarch64; any other architecture is refused beforedlopen. Calling libcsyscallwithout its variadic signature is safe on both ABIs: glibc's x86_64syscallis plain assembly that ignores%al, musl's C version only uses%alto decide whether to spill vector registers, and aarch64 Linux passes variadic integers in the same registers. The musl loader path names (x86_64,aarch64) are correct.__errno_locationin a second FFI call, so main-thread runtime work in between could in theory overwrite it. That can only change which outcome is chosen, never which PID gets signalled. A spurious ENOSYS falls back to the documented live-ppid check, any other wrong code refuses recovery, and at kill time ESRCH versus another error only picksnot_targetorfailed. I accepted this residual; avoiding it would need inline assembly or a compiled shim.refusedblocksbindTarget, so the sentinel never arms.unsupporteduses the same live-ppid-then-kill path as macOS, with its documented residual. A bound handle never reachesprocess.kill, even after close or a failed signal.linux. macOS takes the sameprocess.killpath as before. Windows still checks the handle inbindTargetandterminateTarget, and itsclose()moved into the sharedfinally, which releases it on the same exits as before and also on exceptions.Checklist