Repository navigation
fix(process): kill the gate tree, bound the launch lock, allow one suite - #133
Merged
Merged
Conversation
Three defects found while measuring the suite: (1) the pre-commit hook killed only the direct gate child, so a timed-out verify_diary left a full pytest running for 10+ minutes and raced the next run; the gate is now a process-group leader and the timeout path uses taskkill /T or killpg. (2) ensure_server_running waited on a module lock forever; it now gives up after 150s and returns False instead of hanging. (3) two suites could run on one checkout; a session file lock makes the second exit 3 with a clear message, while nested pytest spawned by tests inherits MSCB_SUITE_LOCK_INHERITED and skips. Real-process checks: a grandchild was gone after _kill_tree; a second pytest exited 3 while the lock was held.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
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.
Three defects found while measuring why the suite looked like an 18-minute hang. All three produced real orphaned processes or unbounded waits on this machine.
1. The hook killed only the direct child (
.githooks/pre-commit)On
TimeoutExpiredit calledproc.kill(). The gate is a Python script that spawns its own children:verify_diary -> pytest tests/. The direct child died, the grandchild kept running for 10+ minutes and competed with the next run. Observed twice (PIDs 7732 -> 14156 -> 16264).Now gates are process-group leaders (
CREATE_NEW_PROCESS_GROUP/start_new_session) and the timeout path kills the tree (taskkill /PID <pid> /T /Forkillpg), withproc.kill()as a fallback.2. The launch lock was unbounded (
src/core/embedder/onnx_client.py)ensure_server_running()didwith _client_lock:with no timeout. A leaked thread holding it turns the next call into an indefinite wait: in the full run one test was reported at 654s while the same file takes 2.7s alone and that test < 0.01s. It now acquires with_LOCK_TIMEOUT = 150sand returnsFalse(fail closed) instead of hanging.3. Two suites could run on one checkout (
tests/conftest.py)A session file lock (
.pytest_cache/suite.lock) makes the second run exit 3 with an explicit message. A nested pytest spawned by a test inheritsMSCB_SUITE_LOCK_INHERITED=1and skips the check, sotest_bootstrap_pipelinestill works. The OS releases the lock when a process dies, so a crash cannot leave it stale.Evidence
Real processes, not fakes:
The new tests are the negative controls:
test_timeout_kills_the_process_treefails if the/Tflag or fallback disappears, andtest_second_acquire_fails_and_release_freesfails if the lock stops excluding.Limits
pytest testswas not run to completion for this commit (1092s under a concurrent benchmark); the commit used the documentedMSCB_PRECOMMIT_FAST=1mode, so gate-zero runs in CI.experiments/bench_2026_11/run.pyto the same lock is a reasonable follow-up.