test(teardown): reap detached processes holding TEST_SKILL_DIR before rm (#662) - #1072
Open
fujibee wants to merge 2 commits into
Open
test(teardown): reap detached processes holding TEST_SKILL_DIR before rm (#662)#1072fujibee wants to merge 2 commits into
fujibee wants to merge 2 commits into
Conversation
… rm (#662) teardown_test_env did a bare `rm -rf "$TEST_SKILL_DIR"` while the detached codex children — codex-bridge-launcher.sh and the codex-bridge.js it starts, which codex-monitor.sh spawns to outlive the test — keep resolving SKILL_DIR to TEST_SKILL_DIR and writing its run/ dir. They are in no pidset the tests kill, so the rm races them and fails "rm: Directory not empty" (or, on Windows, "Device or resource busy" on the bridge's open messages.db). #662 == #1036 == #1049 (five observed instances across macOS/Linux/Windows shards; the #1049 CI red is this one). teardown_test_env now reaps any process whose argv references TEST_SKILL_DIR (SIGTERM, then SIGKILL) before the rm. The launcher records no pidfile of its own, so it is matched by command line, not by a pidfile sweep. The scope is the unique mktemp TEST_SKILL_DIR, so it can never reach a developer's live bridge or another test — never a blanket pkill. The guard fails closed: outside the well-known temp roots it reaps only under a TMPDIR that is set and non-empty after stripping a trailing slash, so an unset / "" / "/" TMPDIR (ubuntu-latest's mktemp uses /tmp with TMPDIR unset) cannot degenerate the match pattern to "?*" and turn the kill loose. Named limits, in the code: it matches only argv-visible holders (today's two are, via the launcher's script path and the bridge's --workspace-root); lsof would close the cwd-only gap but is too slow to run in every teardown. Windows handle-release timing and the loaded-host wait budget are left for CI (dedicated runners) to measure, not claimed. tests/test_teardown_reap.bats: the reaper kills a matching holder; leaves a non-referencing process alone (scope safety); is a no-op when nothing holds it; refuses a rooty TEST_SKILL_DIR; and fails closed on a non-temp TEST_SKILL_DIR for unset / "" / "/" TMPDIR (the degeneration control). Holders are orphaned so they are init-reaped rather than becoming kill -0-answering zombies.
…tays a single rm (#662) The first cut called the reaper in every teardown. Its scan is a full `ps -eo pid=,args=`, and paying that in each of the suite's hundreds of tests — the vast majority of which hold nothing — dominated the shard runtime and pushed the CI bats shards over their timeout (all shards cancelled at ~30 min). The race the reaper fixes is rare (only the codex tests spawn the detached launcher) and announces itself as a non-zero rm ("Directory not empty" / "Device or resource busy"), so pay the cost exactly there: try the plain rm first and reap + retry only when it fails. A passing test's teardown is now a single rm again, and the reaper still runs — with the same TEST_SKILL_DIR-scoped, fail-closed guard — on the teardown that actually races a holder. The direct-reaper unit tests in test_teardown_reap.bats are unchanged (they call _reap_test_skill_dir_procs directly).
This was referenced Sep 9, 2026
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.
What
teardown_test_envdid a barerm -rf "$TEST_SKILL_DIR"while the detached codexchildren (codex-bridge-launcher.sh + the codex-bridge.js it starts, spawned by
codex-monitor.sh to outlive the test) keep writing
$TEST_SKILL_DIR/run— they are inno pidset the tests kill, so the rm races them and fails
rm: Directory not empty(or,on Windows,
Device or resource busyon the bridge's open messages.db). #662 == #1036== #1049 (five instances across macOS/Linux/Windows; #1049's CI red is this one).
teardown_test_envnow reaps any process whose argv references TEST_SKILL_DIR(SIGTERM→SIGKILL) before the rm — the launcher records no pidfile, so it is matched by
command line. Scope is the unique mktemp
TEST_SKILL_DIR, so it never reaches a realbridge or another test (never a blanket pkill), and the guard fails closed: outside
the well-known temp roots it reaps only under a TMPDIR that is set and non-empty after
stripping a trailing slash, so
unset/""/"/"(ubuntu-latest's mktemp uses /tmpwith TMPDIR unset) cannot degenerate the pattern to
?*and turn the kill loose.Named limits, in the code: argv-visible holders only (today's two are); lsof not used
(too slow per-teardown). Windows handle-release timing and the loaded-host wait budget
are for CI to measure, not claimed.
Tests (
tests/test_teardown_reap.bats, 5)kill a matching holder · scope-safety (a non-referencing process survives) · no-op when
nothing holds it · refuses a rooty TEST_SKILL_DIR · fails closed on a non-temp
TEST_SKILL_DIR for unset/""/"/" TMPDIR (the co2-required degeneration control). Holders
orphaned so they are init-reaped, not
kill -0-answering zombies. Enforceable assertions(
refute/plain/[ ]; enforced-assertions baseline unchanged).Verification
Static: syntax, parse (5), baseline unchanged. The degeneration control (test 5) is
verified locally — passes under the fix, and a mutation (
|| return 0→|| :) reds it.The remaining timing-sensitive tests are left to CI's dedicated runners (this host is at
load ~240, so single-run local timing is untrustworthy per the day's rule).
Base:
main(lands after 1.3.0 merges; merge-ready and waiting).