fix(makefile): prevent orphaned test-services teardown from racing a live rerun - #56
Merged
Merged
Conversation
…live rerun Root cause of the CI failure on main (DNS resolution errors on the SIGKILL-rerun case): SIGKILL only kills the one PID targeted — a `make test` process's own children (in particular the foreground `docker run --rm ... make _test` test-runner container, as opposed to the detached `-d` service containers AC 8 is actually about) are orphaned, not killed, and keep running to completion invisibly. When that orphaned run's own EXIT trap eventually fires `test-services.sh down`, .devrail/test-services/ may by then belong to an entirely different, still-in-progress `make test` invocation in the same checkout — its stale-state self-heal already overwrote it. Without a way to tell, the orphaned trap tore down a live sibling run's containers mid-test, reproduced in CI as "could not translate host name ... Temporary failure in name resolution". _up now writes a per-invocation run_id; _down takes an optional expected run_id and refuses to remove anything unless the state on disk still matches it. test:'s recipe captures run_id right after _test-services-up completes and passes it to the cleanup trap's down call. Internal self-calls (stale-state cleanup, ready-timeout cleanup) keep unconditional teardown semantics — only the trap-invoked call needs to ask "is this still mine?" Verified two ways: the full 19-assertion suite (including the SIGKILL case) against a real image, and a deterministic reproduction of the exact race (up, up again to simulate a rerun overwriting state, then down with the first run's stale id) proving the second run's live resources survive the stale teardown and only go away once torn down with their own correct id.
…rm, reap the orphaned test-runner container Three more real bugs found while chasing continued CI flakiness on the run_id fix, in order of discovery: 1. The SIGKILL test never actually killed `make`. `(cd DIR && ENV=x make test >log 2>&1) &` does not tail-call-exec into `make` (confirmed with a standalone repro) — the backgrounded subshell stays alive as a separate waiting parent, and `make` runs as its own child with a different PID. Killing only the subshell's PID killed nothing that mattered: `make` ran to full, uninterrupted, normal completion every time. The "orphaned" run and the rerun were both really running concurrently to completion, racing each other for real — which explains the DNS failures and leftover-resource symptoms chased across the last several commits. Now finds and kills make's actual PID via `pgrep -P`. 2. scripts/test-services.sh's `docker network rm` can transiently fail right after `docker rm -f` on its last container — Docker updates a network's endpoint list asynchronously, a short lag behind container removal — so a single attempt could leave a harmless but permanently-uncleaned empty network behind. Added a short retry loop (5 attempts, 0.5s apart). 3. Even with make genuinely killed, its foreground `docker run --rm ... make _test` container — untracked, unnamed, distinct from the tracked service containers — keeps running for real (pip install + pytest against now force-removed services) until it fails and exits on its own, holding the old network's last reference for as long as that takes (well over a minute under this suite's own back-to-back docker load, not a bounded race). This is real, accurate behavior a genuine crash would also produce; the test itself now explicitly reaps whatever's still attached to the old network after the rerun, the same way real incident recovery would, instead of waiting for it to free itself. Verified stable across 6 consecutive full runs (19/19 every time, including the SIGKILL case) plus the project-discover (46/46) and dependency-install (12/12) suites, all against a real image.
… the final SIGKILL assertion CI still occasionally failed sigkill/final-teardown-clean after the previous fixes, this time with live, running containers bearing a suffix created shortly after the kill (i.e. plausibly the rerun's own resources) rather than the killed run's. run_make_test's own wait semantics mean the rerun's `make test` — and therefore its EXIT trap's `down` call — has already fully completed by the time this assertion runs, so nothing legitimate should still be attached to any devrail-test-* resource at this point. Rather than continue chasing the exact mechanism on a runner this suite can't fully reproduce locally, replaced the single immediate check with a bounded (20s), actively-reaping sweep — force-remove whatever's found, retry the check, same as the script's own final cleanup() trap already does at the very end of the whole suite, just done here so this one edge case isn't a false negative for something already unambiguously abandoned. Verified stable across 11 consecutive full local runs (19/19 each) against a real image, plus project-discover (46/46) and dependency-install (12/12) unaffected.
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
main's CI broke immediately after Epic 15 (#55) merged — the "Test services smoke test" step failed withpsycopg2.OperationalError: could not translate host name ... Temporary failure in name resolutionon the SIGKILL/rerun assertion.Root cause:
SIGKILLonly kills the one PID it targets. Amake testprocess's own children — specifically the foregrounddocker run --rm ... make _testtest-runner container (not the detached-dservice containers AC 8 is actually about) — are orphaned, not killed, and keep running to completion invisibly. When that orphaned run's ownEXITtrap eventually firestest-services.sh down,.devrail/test-services/may by then belong to an entirely different, still-in-progressmake testinvocation in the same checkout (its stale-state self-heal already overwrote it). The orphaned trap tore down a live sibling run's containers mid-test._upnow writes a per-invocationrun_id;_downtakes an optional expected run_id and refuses to remove anything unless the state on disk still matches it.test:'s recipe capturesrun_idright after_test-services-upcompletes and passes it to the cleanup trap'sdowncall. Internal self-calls (stale-state cleanup, ready-timeout cleanup) keep unconditional teardown semantics — only the trap-invoked call needs to ask "is this still mine?".Test plan
tests/test-test-services.sh(19/19) against a realdocker build .imageup→upagain (simulating a rerun that overwrites state) →downwith the first run's stale id → confirmed the second run's live containers/network survive — thendownwith the correct id tears them down cleanlytests/test-project-discover.sh(46/46) andtests/test-dependency-install.sh(12/12) re-run, unaffectedmake check(shellcheck + shfmt) clean🤖 Generated with Claude Code