test(teardown): bound the holder-readiness wait and fail loudly on timeout (#1123) - #1125
Merged
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 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.
test_teardown_reap.batshung the CI shard it landed in: 419 of 435 testscompleted, then thirty minutes of silence and a terminated job leaving
xargsand several
bashorphans (#1123).The cause is not the reaper.
_start_holderran inside a command substitutionand started a holder that never closed stdout and stderr, so the capturing pipe
never reached EOF and
_start_holderitself did not return./dev/null._wait_for_holder_readywith an explicitbound, non-zero when the bound is reached.
the failure, still removes the directory, and returns non-zero at the end, so a
reaper that gives up is visible rather than silent.
Measured before and after, isolating the stall: before the change the file
produced no output for sixty seconds and was killed; after it, no timeout, five
of its six tests pass and the sixth fails only because
psis refused in thesandbox that ran it. Regression tests short-circuit each bound and go red when it
is reached.
Based on #1072, which this makes landable.