test(stella-cli): close two unguarded races in the stop-escalation probe (#1721) - #1861
Open
macanderson wants to merge 3 commits into
Open
test(stella-cli): close two unguarded races in the stop-escalation probe (#1721)#1861macanderson wants to merge 3 commits into
macanderson wants to merge 3 commits into
Conversation
added 2 commits
August 6, 2026 03:43
Deleting a `mod` declaration silently disables the file it pointed at, and nothing in `make gate` noticed. rustc, rustfmt and clippy all walk `mod` declarations from the crate root, so an orphaned file is invisible to the compiler, the formatter and the linter at the same moment — and `cargo test` still reports green. Measured on the real regression, by removing #1739's restored line again: cargo test -p stella-graph --lib -- --list | rg -c '^store::tests::' 0 with the declaration deleted — and the suite reports ok 19 with it restored 19 tests vanished and three gates stayed green. Only a reviewer reading the diff caught it the first time. Adds `module-reachability` to GATE_GUARDS_FAST: a text-level walk of `mod` declarations from each crate's roots (src/lib.rs, src/main.rs, every [[bin]] path, and Cargo's auto-discovered src/bin/*.rs), reporting any tracked .rs under src/ that nothing reaches. Toolchain-free, so it rides the cheapest rung and the author's own pre-push hook. Never scoped by CARGO_SCOPE — this is a fact about the repository, not about a crate. `#[path = "…"]` relocates a module's file and this walker does not model it. There are none in the tree (#1393 removed the last), so rather than guess it fails loudly and says so. A silent wrong answer from a reachability check is worse than no check. The first draft was exactly that wrong answer, and the fixture that catches it is in the suite. It skipped string literals on the theory that a `mod x;` inside one is rare and the failure would be benign. Both halves were wrong: the risk is not a `mod` inside a string, it is a `/*` inside one. This tree has `"guard-deny-path: protected/**"` in stella-cli/src/agent/tests.rs, whose `/*` opened a block comment that never closed — blanking 450 lines, hiding three real declarations, and reporting three perfectly reachable files as orphans. The stripper now tracks strings, escapes and raw strings. scripts/test-module-reachability.sh (`make module-reachability-test`, hermetic) covers both directions, because they fail differently and a suite checking one is blind to the other. Misses: an orphan file, and a store.rs missing its `mod tests;` — the #1739 shape exactly. Fabricates: `/*` inside a plain and a raw string literal, and the inverse, that a `mod` named only in a comment still does NOT count. Plus the shapes that must stay quiet — flat, nested, mod.rs, `#[cfg(test)]` on its own line and inline, `pub(crate) mod`, and src/bin auto-discovery, which a naive walker reports as an orphan. Gate wiring: GATE_STEPS in the Makefile, the block in AGENTS.md, the command list in CONTRIBUTING.md (all three, which is what gate-parity enforces — twenty-three steps becomes twenty-four), a step in ci.yml beside the other toolchain-free guards, and an alias in check-gate-parity.sh because this is the second Python guard and the default expects `check-<step>.sh`. Closes #1750
`a_run_that_ignores_term_is_killed_after_the_grace_period` fails
intermittently under a full-suite run and passes alone. Both of its waits on
asynchronous state were missing, and the issue asks which one expired. The
honest answer is that neither was a bound that proved too small — there was no
bound in either place.
## The precondition, which is the one that explains the symptom
`spawn` returns once the PARENT has forked. The liveness lock is taken by the
CHILD, after its `exec`. So there is a window in which the run is registered
and holds no lock — and `stop` reads exactly that lock to decide whether there
is anything to stop:
if lock_is_held(&sidecar) != Some(true) { … "was already finished"; return }
Entering that branch returns in microseconds without signalling anything, and
`took >= STOP_GRACE` then fails against a run that was never asked to stop.
Under ~1400 concurrent tests, several of which spawn their own children, child
startup is exactly what slips. The test now waits for the child to hold its
lock before calling `stop`, which is the precondition the assertion always
assumed and never established.
## The observation
`stop` returns as soon as it has ISSUED the SIGKILL — unlike
`Supervisor::interrupt_and_drain`, which reaps with `child.wait()` first. The
kernel still has to schedule the target, terminate it, and release its flock,
so "the lock is free" is not true at the instant `stop` returns and was never
promised to be. That assertion had no wait at all, making it the only
lock-released-by-a-dying-process check in the file without an `eventually`
around it (the two siblings, at the `_ends` and `stopping_a_finished_run`
cases, both have one). It now has the same 10s budget they use.
## What was and was not reproduced
Neither race reproduced on this machine, which is not loaded enough. Measured
by instrumenting the test and running the full suite at --test-threads=16:
lock_at_spawn=Some(true) stop_took=8.047s the child won the race
at_return=Some(false) freed_after=13.959µs the kernel won the other
Both windows were open and both happened to close in time here. That is
evidence about this machine, not about the guard, and it is why neither
measurement is offered as the fix's justification — the code path is. Stating
it plainly rather than claiming a reproduction: the two waits are unguarded by
inspection, one of them explains the reported failure exactly, and closing
both costs nothing when they are already closed.
Neither constant moved. STOP_GRACE is untouched, the property is still
asserted against a real child process that ignores TERM, and nothing is
quarantined — #1610 quarantined this probe once and #1632 un-quarantined it,
and dropping the SIGKILL-escalation rung again would lose coverage the module
docs name explicitly.
`cargo test -p stella-cli --bin stella` — 1416 passed, 0 failed, across four
consecutive runs including two at --test-threads=16.
Closes #1721
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
Reviewer's GuideAdds proper synchronization around the stop-escalation test to remove two unguarded races, and introduces a new CI guard that ensures every Rust source file under a crate’s src/ is reachable from its crate root, integrating it into the Makefile, CI workflow, and gate documentation along with a dedicated test harness for the guard. Sequence diagram for stop-escalation test synchronizationsequenceDiagram
actor Test
participant Supervisor
participant ChildProcess
participant LockFile
participant Kernel
Test->>Supervisor: spawn
note over Supervisor,ChildProcess: Parent returns before child holds lock
Test->>LockFile: lock_is_held
alt [lock not yet held]
Test->>Test: wait_until_lock_held
Test->>LockFile: lock_is_held
end
Test->>Supervisor: stop
Supervisor->>ChildProcess: signal_group SIGKILL
Supervisor->>Supervisor: mark_stopped
Supervisor-->>Test: return
Test->>LockFile: eventually_check_lock_released
LockFile-->>Test: lock_is_held == false
Flow diagram for the new module-reachability guardflowchart TD
A[Start check-module-reachability.py] --> B[Discover crates under ROOT]
B --> C[For each crate: crate_roots]
C --> D[reachable_from roots]
D --> E[tracked_sources from git ls-files]
E --> F[Compare reachable vs tracked]
F --> G{problems?}
G -- yes --> H[Print problems\nexit 1]
G -- no --> I{orphans?}
I -- yes --> J[Print orphan files\nexit 1]
I -- no --> K[Print OK summary\nexit 0]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Which wait expired
Neither. There was no bound in either place — both of this test's waits on asynchronous state were missing entirely. The issue rightly warns against widening a budget blindly; no budget was widened, and
STOP_GRACEis untouched.1. The precondition — the one that explains the reported symptom
spawnreturns once the parent has forked. The liveness lock is taken by the child, after itsexec. So there is a window in which the run is registered and holds no lock — andstopreads exactly that lock to decide whether there is anything to stop:Entering that branch returns in microseconds without signalling anything, and the very next assertion —
took >= STOP_GRACE— then fails against a run that was never asked to stop. Under ~1400 concurrent tests, several of which spawn their own children and signal process groups, child startup is precisely what slips.The test now waits for the child to hold its lock before calling
stop. That is the precondition the assertion always assumed and never established.2. The observation
stopreturns as soon as it has issued the SIGKILL:— unlike
Supervisor::interrupt_and_drain, which reaps withlet _ = self.child.wait();before returning. The kernel still has to schedule the target, terminate it, and release its flock, so "the lock is free" is not true at the instantstopreturns and was never promised to be.That assertion had no wait at all, making it the only lock-released-by-a-dying-process check in the file without an
eventuallyaround it — its two siblings (line 166 and thestopping_a_finished_runcase) both have one, with the same 10s budget now used here.What was and was not reproduced
The flake did not reproduce on this machine, which is not loaded enough. Saying so rather than implying otherwise. Measured by instrumenting the test and running the full suite at
--test-threads=16:Both windows were open; both happened to close in time here. That is evidence about this machine, not about the guard — which is why the measurements are not offered as the fix's justification. The code path is: two waits on asynchronous state with no wait around them, one of which explains the reported failure exactly. Closing both costs nothing on a machine where they were already closed.
What is preserved
STOP_GRACEandPOLLunchanged — the test still waits out the real constant that ships.Verification
Four consecutive full-suite runs, two under raised thread pressure.
Closes #1721
Summary by Sourcery
Tighten the stop-escalation test to wait for the child process' liveness lock and for lock release, and add a module reachability guard with accompanying scripts and CI/Makefile wiring to ensure all Rust source files under src/ are reachable from crate roots.
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests: