fix(test): guard both CPU samples symmetrically, so a budget-exhausted second walk skips instead of failing - #1186
Conversation
…e test test_sampler_measures_a_descendant_that_actually_burns_cpu guarded its two CPU readings in opposite directions. The first skipped on any gap at all; the second was a bare `assert after.cpu_seconds is not None`. The second walk is the likelier of the two to time out, since it runs later on a host that has had longer to get busy, so the unguarded side was the one that fired and it reds `test (windows-2025, py3.14)`. Extend the bounded-extension treatment the sibling test already implements (BACKLOG #1290, landed as ad6c1b9) to both readings. A gap whose cause SPENT its budget earns a retry at a longer budget and then a skip; a gap from any other cause is reported as a failure and is never downgraded, including one that arrives partway through the extension. A probe that ran and produced nothing is a probe defect, and a skip that swallows it disarms the test forever. The discriminator is ProbeDegraded.is_budget_exhausted, the split the probe's own vocabulary states once. An absent cpu_seconds carrying no cause is the POSIX partial read, which counts as a fast failure rather than as a missing cause. Note this TIGHTENS the first reading, from a blanket skip to the same rule. Add a liveness guard the extension makes necessary: `_BURN` is a bounded loop, so an extension can outlast it, and a reading taken after the burner exits is summed over a smaller subtree. That reports could-not-measure rather than accusing the probe of a flat counter. Add four scripted-sampler tests. The verdict arms are reachable only on a host too starved to enumerate, so without them the fast-failure-still-fails rule would ship with nothing able to exercise it on any runner. Test-side only; messagefoundry/ is untouched.
github-actions
Bot
added
the
ci-red
Why both halves ship, including the tighteningThis PR makes two guards symmetric, and the two directions are not equally risky. Recording the reasoning here rather than in a review thread, because the tightening is a deliberate behaviour change and the next reader deserves the argument rather than an assertion that two seats agreed. The loosening is free. The second reading moves from "always fail when The tightening is the part worth arguing. The first reading moves from "always skip when None" to the same rule, so a non-timeout gap there now fails where it used to skip. It is structurally inert in the case that worries us. The tightening fires only when a walk returns None without spending its budget. Host load produces the opposite failure: walks that spend their whole budget and time out. Those route to the bounded extension and then to a skip, so the discriminator sends a load-induced failure down the skip path and the tightening never sees it. That matters because this module's family is measured as severely host-load sensitive on a shared developer box -- same commit, 6 concurrent pytest processes gave 12 passed in 26.57s, 10 concurrent gave one test unfinished at 600s, at least a 22x swing, with CI green on all three platforms for that commit (detail on merged PR #1148, measured by another session, not by this PR). That measurement is not an argument against the tightening, because load produces budget exhaustion and budget exhaustion skips. What the tightening does catch is a walk that fails fast, which is a broken enumerator. BACKLOG #1290's own comment already states the rule: downgrading that is "how a skip-on-load hides a probe regression forever". Shipping only the loosening would leave the strict half unexercised, which is precisely the condition #1290 was written about. The caveat, stated so it is not discovered laterNobody has measured how often a fast-fail gap occurs on the first reading. The claim above is about which code path each cause takes, not about how often either cause occurs. No rate, frequency or percentage is claimed anywhere for the original If a fast-fail red starts appearing after this lands, that is the new guard working as designed. The right response is to read the enumerator, not to widen the skip back out. |
Lander reviewDisclosure first: I raised the finding this PR was briefed from, so I reviewed it as a reader looking for what my own brief got wrong rather than for confirmation. Two things here are better than what I asked for, and one thing I asked for is correctly absent. Ship the tightening. It is not the risk it looks likeMaking the two readings symmetric loosens the second and tightens the first, and only the tightening can create a new red. It is still right, because the tightening is structurally inert in the case that would hurt: it fires only when a walk returns None without spending its budget. Host load produces the opposite — walks that spend the whole timeout — and those route to the extension and then to a skip. So the peer's 22x load-sensitivity measurement is not an argument against it; under load the discriminator sends the failure down the skip path and the tightening never sees it. What it does catch is a walk that failed fast, which means the probe ran and produced nothing. That is a finding, and #1290's own text says downgrading it is how a skip-on-load hides a probe regression forever. The part I did not ask for and would not have thought ofThe new arms are unreachable on a healthy runner. Without The edge the extension itself created, found and closed
Checked rather than readThe probe API it leans on exists and draws the line in one place. The Scope. One file, No glyphs. 210 added lines, zero non-ASCII characters. Positive control in the same pass: the removed lines contain one non-ASCII character, an em dash this PR replaces with What it correctly did not doThe twin Stated no larger than it isNo rate, frequency or percentage is claimed anywhere for the original The local green is worth little on its own and the PR says so. The leg to read is Verdict: merge. |
1 participant
The defect: one test guarded its two CPU samples in opposite directions
tests/test_connscale_cpu_probe.py::test_sampler_measures_a_descendant_that_actually_burns_cputook two CPU readings and treated a gap differently each time:
if first.cpu_seconds is None: pytest.skip("OS CPU probe unavailable on this runner")assert after.cpu_seconds is not NoneThe second walk is the likelier of the two to time out. It runs later, on a host that has had longer
to get busy. So the unguarded side was the one that fired, and it reds
test (windows-2025, py3.14),a live required context.
The asymmetry is visible in the source and does not depend on any runtime measurement. That alone
justifies this change.
The fix: extend the treatment the sibling test already implements
test_subtree_re_resolution_picks_up_a_late_spawned_child, in the same file, already handles thiscondition correctly under BACKLOG #1290 (landed as
ad6c1b9d7, PR 598): when a walk produces nousable result it retries with a LONGER budget, and only then skips, with a message carrying an
explicit verdict. That treatment was never extended to the burn test. This extends it.
Both readings now go through one helper,
_cpu_seconds_or_verdict:_PROBE_TIMEOUT_Sraised to_STALLED_WALK_TIMEOUT_S, bounded by_granted_extension_walks;A fast failure still FAILS, and that is the point
A gap whose cause did NOT spend its budget is reported as a failure and is never downgraded to a
skip. #1290's own comment states the rule and it is the whole design:
An exhausted budget measures the runner, so the tree under test is not implicated and a skip is
honest. Anything faster means the probe RAN and produced nothing usable, which is a probe defect: if
that became a skip, the test would be disarmed forever and nothing would report it. The rule also
holds for a fast failure that appears partway THROUGH the extension, so a single load-induced timeout
cannot open a door that a defect then walks through.
The discriminator is
ProbeDegraded.is_budget_exhausted, the split the probe's own vocabulary alreadystates once and tells consumers to stay on. Not a second copy of the member list.
The one subtle arm:
degradedis set only on a FULL gap, so an absentcpu_secondscarrying NO causeis the POSIX partial read (handles read, CPU did not). That read ran and produced no CPU, so it counts
as a fast failure. Reading the missing cause as "nothing went wrong" would invent the timeout the
probe was careful not to claim.
This also TIGHTENS the first reading, and you should know that
Symmetry cuts both ways here, and only one of the two directions is risk-free:
Strictly looser than today, so it cannot introduce a new red.
So a runner whose CPU probe gaps for a non-timeout reason (
WALK_ERROR,WALK_EMPTY,WALK_NO_ROOT,READ_ERROR,READ_EMPTY) now fails where it previously skipped with the unattributable message "OSCPU probe unavailable on this runner". That is #1290's rule applied in the direction it points, and it
is what the
ProbeDegradeddocstring already says a consumer must do. I am naming it because it is abehavior change beyond the literal brief, not a silent side effect. If you want the first reading left
as a blanket skip, say so and I will cut it back, but I think the tighter form is right: a blanket skip
on the first reading is the same defect #1290 fixed on the second.
A liveness guard the extension makes necessary
_BURNis a BOUNDED loop (roughly 12 s, then it exits on its own) and the sampler re-walks every tick,so a departed PID drops straight out of the summed set. A second reading taken after the burner exits
is summed over a SMALLER subtree than the first, and
after - firstcan come out at or below zero withnothing wrong.
That is unreachable on the healthy path, where both readings land within about 3 s of the spawn. It
becomes reachable once a bounded extension has spent tens of seconds first. This is the same latent
edge the extension turned into a designed path in the sibling test, and it is closed here the same way:
if the burner did not outlive both readings, the test reports COULD NOT MEASURE rather than accusing
the probe of a flat counter.
One distinction inside that guard: a burner that exited 0 completed normally and earns the skip; a
burner that exited NON-ZERO never burned at all, which is a broken rig, so it FAILS as a positive-
control failure. Skipping that would hide a rig defect the same way a blanket skip hides a probe one.
Four new tests, because nothing else can reach this code
The verdict arms are reachable only on a host too starved to enumerate, which no healthy runner
reproduces on demand. Without a scripted sampler, the fast-failure-still-fails rule would ship with
nothing able to exercise it on any runner, ever. That is precisely the shape #1290 is about, so the
guard it motivates must not be built that way.
_canned_samplerserves a scripted sequence ofProcSamples, and the four tests pin:They touch no OS state and run on every platform, deterministically, under any load.
Reuse versus duplication: reused what was already shared, added one helper called twice
Deliberate, and recorded as asked.
REUSED as-is:
_granted_extension_walks,_STALLED_WALK_TIMEOUT_S,_STALL_EXTENSION_WALKS,_WATCHDOG_SHARE, andProbeDegraded.is_budget_exhausted. No new copy of any of them.ADDED: one helper called twice, which is itself the anti-duplication move. The alternative was writing
the same guard block out for
firstand forafter, which is how the two drifted apart in the firstplace.
NOT UNIFIED with the sibling's bounded-extension block, deliberately. The sibling's retry unit is a
WALK with nonlocal bookkeeping (
walks,walked_ok,child_alive_at_success,failed); mine is aREADING that returns a value. It also classifies differently and correctly so: the sibling compares
seconds SPENT against seconds ALLOWED because a walk reports only success or failure, while a reading
carries its own
degradedcause and can be asked directly. The probe's vocabulary says these are oneline drawn with the evidence each side has, and I kept them one vocabulary rather than one function.
Fusing them would have obscured both and would have meant editing the test I am supposed to prove I
did not break.
One more twin I found and deliberately did NOT import:
tests/test_connscale_smoke.py::_is_budget_exhausted.It classifies a cause RECORDED AS A STRING and treats an unrecognised one as not tolerable; mine
classifies a typed cause taken live off a
ProcSample, and itsNonearm carries the POSIX-partial-readmeaning above, which over there is only an incidental
ValueError. Both delegate to the same productionproperty, so the member set cannot drift between them — only the two adapters exist to converge, and
converging them would mean editing a second test module whose ownership by other sessions was not swept
for this brief. I put a pointer to the twin in each docstring so a reader finds both. Converging the
two adapters is a reasonable follow-up if you want it filed.
One shared-grant note:
_granted_extension_walkssubtracts_RESOLUTION_DEADLINE_S, a 30 s poll theburn test does not run, so the number it hands the burn test is conservative by roughly 27 s. That is
the safe direction. The burn test also carries ONE allowance across both readings rather than taking a
fresh grant for each, so a doubly-stalled run cannot outrun the watchdog the grant is trimmed to fit.
Being killed by the watchdog reports no verdict at all, which is strictly worse than the failure this
PR is fixing.
Evidence: n=1, stated as n=1
No base rate was measured, and I am not claiming one. No failure rate, no frequency, no percentage.
What is known, as given to me by the Lander that cut this brief:
degraded=ProbeDegraded.WALK_TIMEOUT.e8c71d9cf.main:test (windows-2025, py3.14)measured SUCCESS on main atd33237658.The asymmetry is what justifies the fix. It does not need a rate, and a wrong one would poison the
record.
A peer measurement on the sibling module, attributed and NOT mine
Relayed to me by the Manager, sourced to the last comment on merged PR 1148 (merge commit
6b8ce58d5,an ancestor of
origin/main). I did not run this and I did not verify it. Same commit, samemachine, only competing load changed:
At least a 22x swing from host load alone, on the SIBLING module. I did not measure my own module's
load sensitivity and I am making no claim about it. CI was green on all three platforms for that
commit, and the peer reads it as a shared-developer-box effect rather than an engine defect.
It does not change this fix, and it does not license a blanket skip. If anything it sharpens the #1290
rule: a host starved enough to exhaust a walk's budget is exactly the case that may skip, and a walk
failing FAST is exactly the case that must still fail. Load sensitivity is the argument FOR a bounded
extension, not for a wider skip.
Checks run
All in the foreground, in this worktree's own
.venv.ruff check .ruff format --check .mypy messagefoundry(strict)pytest tests/test_connscale_cpu_probe.pyThe file held 24 tests before; the four scripted-sampler tests bring it to 28. Ruff and the whole file
were re-run after the simplify edits, not only before.
Local runs report
INCOMPLETE RUN -- coverage was NOT collectedbecause thevaultextra is absentfrom this interpreter. That is expected here and is not a failure; it means modules gated on that extra
removed themselves at collection time, so these counts do not establish that the full suite is green.
Read those local pytest numbers with the load beside them. At the time of all three runs this box
carried 61
claude, 27nodeand 10pythonprocesses. The peer's degradation point above was 10concurrent pytest processes. So a local green here is NOT evidence the guard is right, and a local red
would not have been evidence I broke the sibling. Nothing was tuned to make a local run pass: no added
retry, no added sleep, no widened timeout. CI is the verdict.
What a local run CANNOT reach: the gap branches. A healthy probe never gaps, so the extension and
both verdict arms are unreachable in a real local run. That is exactly why the four scripted-sampler
tests exist; they are what actually exercises the new code, and they are deterministic.
mypydoes not covertests/(CI runs it onmessagefoundryandmessagefoundry_webconsoleonly).Everything new is type-hinted anyway.
CI legs to read after I exit
My process is gone when this PR opens. Somebody needs to read:
test (windows-2025, py3.14)— the required context this PR is about.testmatrix legs, ubuntu included. On a 60 s watchdog_granted_extension_walkstrimsto ZERO walks, so the burn test degrades to a single reading then a verdict. That is the correct
answer there, not a degradation, but it is a path the Windows legs will not show you.
lint/mypylegs, to confirm the hosted result matches the local one.What the
/simplifypass changed, and one thing it caught in my own proseFour cleanup agents (reuse, simplification, efficiency, altitude). Three findings applied:
ifthat tested the same twofacts the loop's own condition tests on its first pass. Folded the initial reading into the loop:
nesting drops from four levels to three,
causes.appendcollapses to one site, and the "when may aretry happen" rule is stated once instead of twice. I traced all four scripted-sampler tests through
the new shape and the
causes/used/ return values are identical."because a walk reports only success or failure". That is wrong.
FdSampler._resolve_degradedcarries the cause for a walk too;
_walk_succeededreads it and discards the cause, keeping theboolean. So the two mechanisms differ in the evidence each test KEEPS, not in what is available.
Corrected in place. An unchecked assertion in a comment becomes a permanent record, and this one
would have justified the split on a premise that does not hold.
_is_budget_exhaustedtwin, handled with docstring pointers asdescribed above.
The efficiency angle came back clean with arithmetic worth recording, since it is the hazard the
extension machinery exists to respect:
That assumes an attempt can spend BOTH a walk timeout and a per-PID read timeout, so 2x the budget per
attempt. The sibling test that established this pattern already runs at roughly 100 s against the same
120 s Windows watchdog, so the new test's margin is wider than the shipped one it copies, not tighter.
The compounding is Windows-only: the POSIX walk is
os.listdir/read_textwith no subprocesstimeout to exhaust.
Four more instances of this pattern in the same file, NOT fixed here
The altitude reviewer found the same asymmetric-guard shape on the file's other two acceptance tests.
I verified each by grep and bound it to its enclosing test rather than trusting a line number:
test_the_reported_peak_excludes_a_stale_ppid_adopted_subtreepytest.skip("process-table enumeration unavailable on this runner")test_the_reported_peak_excludes_a_stale_ppid_adopted_subtreepytest.skip("per-PID handle read unavailable on this runner")test_the_probe_covers_workers_spawned_after_it_resolved_within_one_steps_tick_budgetpytest.skip(f"OS handle probe unavailable on this runner ({first.degraded})")test_the_probe_covers_workers_spawned_after_it_resolved_within_one_steps_tick_budgetpytest.skip("process-table enumeration unavailable on this runner")Every one skips on a gap without classifying the cause. The third is the sharpest: it prints
first.degradedin the message and then never branches on it, so aWALK_ERRORis named and excused inthe same line. Two of the four need more than a one-line change — the helper that produces the value
discards
ProcSample.degradedbefore the guard ever sees it, so the cause would have to be surfacedfirst.
I did not build these. They are outside a brief scoped to one test, they touch two currently-passing
acceptance tests, and the reviewer agrees they belong in a separate narrow fix. Flagging them for the
Lander as the clearest candidate for the next item. A shared
_assert_measured_or_verdict(cause, which)used by all three tests is the shape that would close themtogether.
Proposed ledger text, for the Lander
I did NOT edit
docs/BACKLOG.md— the engine copy is a public stub and the real ledger is vault-only.No new number is cited anywhere in this PR; #1290 is the existing row whose treatment is extended.
Suggested amendment to #1290, if you want one:
Nothing here contradicts the brief
Both line references verified by symbol against the working tree before I touched anything: the
guarded first reading and the bare second assert were where the brief said. No product code was
touched;
messagefoundry/is untouched and this is entirely test-side. I found no product defect toreport.