Repository navigation
Replace blind sleeps with explicit concurrency and shutdown-readiness checks - #128
Merged
Merged
Conversation
valthon
force-pushed
the
fix/phase-group-parallel-test
branch
from
October 15, 2026 00:42
7adf767 to
394f46b
Compare
The terminatePidsSync straggler test slept 500 ms to let bash install `trap '' INT`. On a loaded host the SIGINT could land before the trap, so the child exited gracefully and the SIGKILL path went untested. The child now prints `ready` after installing the trap and the test awaits that line. It blocks in `read` rather than `sleep 300`: an exec'd or forked sleep either leaves /proc/<pid>/cmdline empty for a moment (read as "exited" by the liveness check) or outlives the SIGKILL holding the stdout pipe open, which kept jest from exiting. Claude-Session: https://claude.ai/code/session_01PPcjf9SpvBzUMjiuGoKZt6 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_019LgGtRheR6Hac9WHX739y3
"runs children concurrently when parallel" asserted ["fast","slow"] from a 5 ms versus 40 ms sleep, which only holds if the two children start less than 35 ms apart. On a loaded runner they can start further apart and the order flips (CI run 38503698391). The runner itself does start children together; the test's wall-clock margin was the flake. Each gated step now records its start, waits on a shared rendezvous that releases only once every party has started, then records its end. A runner that is not concurrent never releases the barrier, so the bounded wait rejects and the test fails rather than hanging or passing. The same sleep-ordering shape is fixed in the phase's parallelize test, and the in-flight peak assertions (cap and unbounded, phase and group) now reach their peak through the barrier instead of outlasting sleep(10). A group twin of the unbounded-concurrency case is added. rendezvous, createGatedStep and createInFlightTrackingStep live in the orchestration fixture with their own unit tests. Claude-Session: https://claude.ai/code/session_01PPcjf9SpvBzUMjiuGoKZt6 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_019LgGtRheR6Hac9WHX739y3
valthon
force-pushed
the
fix/phase-group-parallel-test
branch
from
October 20, 2026 19:00
394f46b to
58dfc77
Compare
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.
What
Stop using blind sleeps as a proxy for verifying parallel activity and shutdown readiness. Check that tasks actually overlap and that the child process has installed its signal handler before exercising forced termination. This makes the checks reflect the behavior they are intended to verify.
The intent is to verify the existing guarantees more directly:
Here, making the tests “deterministic” means removing timing races in the tests' observations. It does not impose a fixed execution or completion order on parallel work, serialize the framework, or replace real flow activity with mocks. Existing sequential-order, failure-handling, and timeout tests remain in place.
One motivating failure occurred in #127's CI (run 38503698391).
Parallel phase and group tests
ClusterBuildPhaseGroup › runs children concurrently when parallelran a 40 ms step and a 5 ms step and asserted the 5 ms one finished first. That only holds if the two children start less than 35 ms apart. The runner does start them together (Bluebird.mapwithconcurrency: Infinity), so the defect was the test's margin, not the runner. The phase'sparallelizetest made the same assertion, and the four cap and unbounded peak-in-flight tests relied on every body starting withinsleep(10).Each of these now records its start and waits on a
rendezvous(parties)barrier that releases only once every party has started. The wait rejects after a named timeout (RendezvousTimeoutMs), so a non-concurrent runner fails the test instead of hanging it or passing. Peak counts are reached through the barrier, sized by a namedcap. A group twin of the unbounded-concurrency test is added, because no group test checked that the runner honours an unbounded group.rendezvous,planGatedStepandplanInFlightTrackingStep(actor-firstplan*factories with namedrun*runners and typed probe state inctx.outputs) live in the orchestration fixture, with their own tests. Those cover release, hold until the last party arrives, rejection at the timeout, the default ceiling, and no timer left pending.terminatePidsSyncstraggler testThe test slept 500 ms so bash could install
trap '' INTbefore the SIGINT. On a loaded host the signal can arrive first, and then the SIGKILL path goes untested. The child now printsreadyafter installing the trap, and the test waits for that line. It then blocks in bash's built-inreadrather thansleep 300. Asleepchild either briefly empties/proc/<pid>/cmdlinewhen exec'd, which the liveness check reads as "exited", or outlives the SIGKILL and holds the stdout pipe open, which keeps Jest from exiting. The test also fails fast if bash exits before printingready. Readiness and closure each wait on the event with a 20 s ceiling, and afinallykills any surviving child, closes its stdin and awaitsclose, so a termination regression cannot leave the child holding Jest open. The fixed 200 ms post-kill sleep is gone.Verification
Mutations of the runner:
Repeat runs, before the last round of review fixes:
The last round changed only test error paths: rejecting on a spawn error and on an early exit, and a fake-timer default-ceiling test. Since then each has passed once, in the per-commit gate below.
Per-commit gate: CI's
test:cisequence at each commit, without theql-tool-appJest project, whose tests never finish on the dev host (a jsdom synchronous XHR tolocalhost:80, which the host's WSL2 mirrored networking never refuses). CI runs them. The pre-commit hook was skipped (--no-verify) for the same reason.a59190d8394f46b2Current series, rebased on
masterff3b06e1: at each commit a clean platform-root checkout ran install (sibling links verified), build, lint and everytest:unit, all RC 0; at the tip the fullcluster-toolJest project passed 2,100/2,100. History check clean.Review: an adversarial Codex review found two issues: the straggler child had no unconditional cleanup or readiness deadline, and the fixture factories used
create*names and inline runners against STYLE.md. Both were fixed, folded into the commits above, and confirmed by a scoped re-review with no further findings. The mutation and repeat-run results above predate that fix.🤖 Generated with Claude Code
https://claude.ai/code/session_01PPcjf9SpvBzUMjiuGoKZt6
https://claude.ai/code/session_019LgGtRheR6Hac9WHX739y3
Rebased onto master
56a538b2as58dfc776, patch-identical (git range-diff). Gate on the rebased head: both commits install and build with lint; the tip ranpnpm run test:ci(4,489 Jest tests passed, everytest:unitlane passed) with #132's fix applied only for the test step, RC 0.