test: close inherited bats fd 3 in all backgrounded process launches - #500
Merged
Conversation
bats keeps test-file teardown waiting until fd 3 is closed by every process that inherited it. A backgrounded watcher (or helper process) launched without 3>&- holds that fd for as long as it lives, so a launch whose kill/cleanup races or fails pins the whole bats run at the end of the file until the job-level timeout cancels it. The #251 sweep (06-27) added 3>&- to these launches in test_watch.bats only. This closes the remaining 18 sites: the #245 burst test it missed (the launch behind the run that stalled after test 786), 14 watcher launches in test_delivery.bats, 2 in test_install.bats, and the python listener in test_codex_monitor.bats. Redirect-only change; all 218 tests across the four files pass.
The first commit swept only line-final watch.sh launches; review found the same class surviving in three shapes it missed: continuation-style launches whose & sits on a redirect line, long-lived helpers (sleep 600 stand-in session pids, sleep 30/60 decoys, node fake servers, the compat stub), and mid-line launches (... & pid=$!). Any of these that outlives a failed assertion holds bats fd 3 and pins file teardown — the v1.1.11 release run's attempt 1 went silent 12m58s after its last test before the job timeout cancelled it. Rule is now uniform: every backgrounded launch under tests/ closes fd 3 at the launch site, including short-lived ones, so future copy-paste starts from a safe template. Redirect-only; 787 tests recognized, all 12 changed files pass locally.
fujibee
added a commit
that referenced
this pull request
Jul 28, 2026
…one (#505) * fix(liveness): make kill(2) and ps agree before calling a pid dead _agmsg_pid_alive already read the ESRCH/EPERM distinction. Add the ps cross-check #503 settled on, so "dead" needs a source that does not depend on signalling permission at all, and a fork-free fast path, so the common answer still costs no subshell — callers poll this in loops that exist to be fork-free. * fix(liveness): route every liveness check through the EPERM-aware helper `kill -0` answers "can I signal this", not "is this running". Under a sandbox a live watcher, bridge or app-server fails it, and every shipped caller read the exit status directly: status printed live processes as stale pidfiles, session-start and codex-monitor started a second bridge and app-server beside the running ones, and the launcher reclaimed a live owner's lock — the #485 duplicate-children shape. All seventeen sites now call _agmsg_pid_alive; instance-id.sh is sourced where it was only reachable transitively, or not at all. * test(liveness): pin EPERM as alive, and sweep for bare kill -0 pid 1 is a live process this user cannot signal, so it stages the distinction directly; the suite skips where no such pid exists. A dead-pid case guards the other direction, since "assume alive" must not make everything look alive. The sweep is #500's lesson as a test: a partially-hardened file reads like a fixed one, so no shipped script may call kill -0 outside the helper. * fix(liveness): reject 0 as a pid — it names this process group, not a process `kill -0 0` succeeds because 0 addresses the caller's own process group, so a digits-only check called it alive, and callers kill what liveness reports alive: `kill 0` TERMs the group, the caller included. A pidfile holding 0 was all it took. Leading zeros go too — nothing writes them and kill(1) may read them as octal. The check is split out as _agmsg_pid_valid and also gates the two launcher sites that kill a recorded pid without asking about liveness first. Reported by review of #505. * fix(liveness): bound a pid to pid_t, and stop testing liveness against a number Past INT32_MAX kill(1) rejects the ARGUMENT rather than reporting ESRCH, and everything that is not ESRCH reads as EPERM, i.e. alive. An oversized value in a pidfile was therefore alive forever: lock never reclaimed, bridge never restarted. Bounding the input is what keeps "not ESRCH" meaning "EPERM". The ESRCH tests stubbed kill and passed the literal 999, which is a RUNNING process on some CI hosts — so the new ps cross-check correctly called it alive and the tests failed there and only there. They now use a pid that is genuinely gone, and the cross-check gets a test of its own. Reported by review of #505. * fix(liveness): make the pid ceiling the platform's, not one number A Windows process id is a DWORD, and liveness there reads the native process table through tasklist rather than kill(1)'s signed pid_t. The INT32_MAX bound sat in front of that branch, so 2147483648..4294967295 — legitimate native pids — were called dead, and a live watcher or lock owner stale. Reported by review of #505. * docs(liveness): describe both pid ceilings, not just pid_t The comment under the ceiling selection still explained only the POSIX side, which reads as if the Windows branch were an exception to a rule rather than the other half of it. Reported by review of #505.
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
1 participant
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.
Summary
bats holds test-file teardown open until every process that inherited its fd 3 has closed it. A backgrounded process launched without
3>&-keeps that fd for its lifetime, so any launch that survives its cleanup (most easily: an assertion fails before thekillline is reached) pins the whole run at the end of the file until the job-level timeout cancels it.Direct evidence from the v1.1.11 release run (30249251822, attempt 1,
bats (macos-latest)): the job went silent for 12m58s after its last test line and was cancelled at 25m18s; attempt 3 of the same commit passed in 18m19s. The stall window matches the long-lived background processes fixed here (up tosleep 600stand-in session pids).The earlier fd-3 sweep (#251) covered only the line-final
&watcher launches intest_watch.bats. This PR applies one uniform rule — every backgrounded launch intests/closes fd 3 with3>&-— covering the classes the first sweep missed:&sits on a redirect line (test_watch.bats,test_actas_integration.bats,test_despawn.bats,test_resolve_project.bats)sleep 600stand-in session pids,sleep 30/sleep 60decoys, the python socket listener, node fake servers, the compat stub... & pid=$!), including the timeout watchdog pair intest_delivery.batstest_inbox.bats) and short-lived race senders (test_storage.bats,test_team.bats), for uniformity so future copy-paste starts from a safe template12 files, 44 sites, redirect-only (
3>&-added at each launch); no test logic touched.tests/test_helper.bashhas no backgrounded launches.Testing
bats --count tests/*.bats→ 787 (syntax intact)