perf(harness): bound the load_sensitive lane and fix its startup race - #4012
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Ref polylogue-8226a
Ref polylogue-b9yw7
… eagerly Ref polylogue-b9yw7
Pre-existing break from #4026: the step was added to build_verify_steps without updating the test that pins the quick label list. Ref polylogue-4026
Sinity
force-pushed
the
feature/harness/a4-serial-lane
branch
from
August 19, 2026 08:15
9238bfb to
27dcee2
Compare
Sinity
added a commit
that referenced
this pull request
Aug 19, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWcPJJJvuF25CqVwTFgSQC
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.
Summary
The warm
devtools verifygate spends most of its wall clock in one place: theload_sensitivelane, 80.71s of a 110.67s run. This bounds that lane'sconcurrency instead of pinning it to a single process, and fixes a startup race
in the two tests that made the lane flaky whether or not it ran concurrently.
On a quiet host the lane goes from ~72s to ~35s; under fleet load, measured
head-to-head, from 91-131s to 39-41s.
Problem
Receipt
20260818T184401Z-full-1494889-23438ba4puts the lane at 80.71s against8.76s for the parallel lane and ~22s for every static gate combined. All seven
members are in
tests/integration/test_daemon_resilience.py, each driving a realpolyloguedsubprocess with its own archive root (workspace_env) and its ownloopback port — they share no global resource.
load_sensitiverecords a real observation: the parallel lane's full workercount starves daemon startup and flakes these tests. It does not follow that the
members contend with each other, and the lane was reading that marker as "run one
at a time" rather than "do not run at the parallel lane's width".
While measuring, the more interesting problem surfaced. Repeating the two SIGTERM
tests strictly serially under host load produced a failure in 1 of 3 runs
(102.4s,
subprocess.TimeoutExpiredafter 90s), and again in a 5-run capture.The lane is flaky on master today, and lane concurrency is not what causes it:
_wait_for_lifecycle_startreturns as soon asDaemonLifecycle.startpersistsits row. In
polylogue/daemon/cli.pythat happens early —install_signal_handlersruns immediately after it, and the startup lifecycle event, source-root creation
and the maintenance loops all run later and all write the ops tier. Acting on the
lifecycle row alone races the rest of startup, and the race widens exactly when
the host is busy.
test_sigterm_with_locked_ops_exits_without_normal_sqlite_waitthen takes
BEGIN EXCLUSIVEonops.dbinside that window, parking a daemonstartup write inside a blocking
sqlite3_step, where CPython cannot run thePython SIGTERM handler until the busy timeout expires — past the test's 90s bound.
Solution
Bounded lane (
devtools/verify.py). Theload_sensitivelane now runs--dist=loadgroup -n min(adaptive, SERIAL_LANE_MAX_WORKERS)instead of-n 0.The cap is 4, with the measurement that produced it recorded at the constant.
_native_pytest_stepstakesserial_worker_args; the closed-world reconstructionin
_native_pytest_command_is_closed_worldpasses the observed worker request forboth lanes, which is exact for the lane under test and irrelevant for its sibling.
Nothing here is collection-affecting, so
devtools/pytest_collection_contract.pyand the testmon environment digest are untouched — no graph invalidation.
Bin packing (
tests/integration/test_daemon_resilience.py). Under dynamicscheduling the longest member (
test_large_session_file, ~23s of real 50K-messageingest) is declared sixth of seven, so it starts last and the makespan became
"when the longest test happened to begin" — 35.1s against a 23.2s floor. Four
xdist_groupbins pack the members longest-first so the makespan is bounded bythe largest bin. The bins are a scheduling hint, not a correctness contract: the
tests are independent, so a wrong or missing group costs wall clock, never a
false result.
Readiness gate (same file).
_wait_for_api_readypolls the daemon's API port,which only answers after the startup sequence completes, and both SIGTERM tests
now wait on it before signalling or locking. This removes an unintended
precondition rather than relaxing an assertion — every existing assert, including
the
< 25.0bound and the thread-dump log checks, is unchanged.Lane interpreter pin — found here, shipped in #4017, dropped from this PR.
Provisioning this lane produced a venv on uv's CPython 3.14.5 while the checkout
runs the flake's free-threaded 3.14.4. That breaks a lane two ways: the
interpreter is a testmon digest input, so the lane can never match the graph
lane-initjust seeded for it and every run bootstraps while printing "laneverifies start warm"; and
import hypothesisfails outright (sysconfigraisingAttributeError: installed_base), so the lane cannot collect the corpus at all.This PR's first measurement attempt died on it.
lane-a1 found the same root cause independently and fixed it more completely in
#4017, so my commits are dropped rather than duplicated. Theirs resolves the
interpreter via
sys._base_executable(the build a venv was created from)where mine parsed
pyvenv.cfg'shomeand guessed among likely binary names —mine breaks on a relocated venv. Theirs also scrubs
_PYTHON_SYSCONFIGDATA_NAME/_PYTHON_HOST_PLATFORM/PYTHONPYCACHEPREFIX,which is the actual mechanism behind the
installed_basefailure I hit andworked around without diagnosing. No delta from mine was worth keeping.
Loopback-socket probe routed into the lane (
tests/unit/daemon/test_web_reader.py).test_socket_peer_disconnected_detects_closed_loopback_peerasserts that_socket_peer_disconnectedobserves the peer's FIN within the call, with no retryor grace window — the case pyproject's marker text names — and was the one
function in that file the marker did not cover. It passed in 11 retained receipts,
then failed once (
assert False is True) in a 1990-test batch under load on2026-08-19 and passed standalone at 0.68s immediately after. Evidence and
recommendation:
.agent/scratch/gate-only-flakes-evidence-2026-08-19.md.An incomplete tmpfs cleanup now explains itself (
devtools/pytest_supervisor.py,devtools/verify_runs.py) — see the b9yw7 section below.Rejected: reducing
test_large_session_file's 50K-message scale. Its assertionsare about behaviour at that scale (2 GB RSS bound, FTS population), so shrinking
it would weaken what it asserts. Its wall time is genuine ingest — the JSONL
writer accounts for 0.28s of it.
Verification
All commands run in
/realm/worktrees/a4-serial-laneon the lane's own venv.A correction to the baseline's framing. "7
load_sensitivetests / 80.7s" wastestmon's selection in that receipt, not the lane's membership. Full membership
was already 13 — the seven daemon-resilience tests plus a six-test
load_sensitiveclass intests/unit/daemon/test_web_reader.py— and this PRmakes it 14 (see the marker addition below). Both figures below are the full
14-member lane.
Head-to-head, same host, load average 8-21 (concurrent fleet lanes),
devtools test ... -m load_sensitive:-n 0(master's shape)-n 4 --dist=loadgroup(this PR)On the seven daemon-resilience members alone, the same comparison was 91.18s /
131.10s serial against 39.00s, 40.42s, 41.46s, 41.41s bounded; on a quieter host
earlier in the session, 71.95s serial against 35.08s at four workers.
The concurrency cliff that sets the cap, same corpus and containment:
The readiness gate, five consecutive repeats of the SIGTERM pair under load —
12.43s, 12.14s, 12.24s, 12.64s, 12.61s, all green. Before the fix the samecommand gave
9.36s / 102.40s (1 failed) / 10.65s, so the variance collapse isthe evidence, not just the greens.
Full gate,
devtools verifyon this branch:Receipts: before
20260818T184401Z-full-1494889-23438ba4(serial lane 80.71s,7 tests, workers=9 available); after
20260819T005619Z-full-3842391-dfb668e3(terminal green, wall 81.68s) and
20260819T004849Z-full-3759625-54e8f8d8.Two honest caveats about the receipt pair. The lane selected 0 tests in the green
"after" run — testmon had already attested those seven from the immediately
preceding execution — so its 5.41s lane step is not a like-for-like measurement;
the
devtools testtable above is. And in20260819T004849Zthe lane took114.70s at workers=1: under load average 22
adaptive_pytest_worker_countcollapsed to a single worker, so that receipt measures the fallback path, not this
change. That collapse is the intended safety property — the cap only raises a
ceiling — but it does mean the win lands on a quiet host, which is where the
80.71s baseline was measured.
Re-verified after rebasing onto
a9bf50133(21 PRs merged in between):devtools verify --quicksuccess,mypyclean across 2607 files, and a focusedrun over every touched module 432 passed in 149.60s. Merge-gate receipt at
head
27dcee229: exit=0, 151.55s.Anti-vacuity re-checked after the rebase rather than trusting the pre-rebase
result: disabling
_restore_owner_writeturns all three read-only tests redtogether (
..._removes_read_only_artifact_trees,..._removes_a_read_only_seeded_cache,..._sweep_reclaims_a_dead_runs_read_only_tree...) and green again on restore, sothe sweep genuinely depends on #4018's repair rather than carrying its own copy.
No further full
devtools verifywas run on this branch, deliberately.devtools/pytest_supervisor.pyis a testmon environment-digest input(
devtools/pytest*.py,testmon_bootstrap.py:327), so the b9yw7 fix changes thedigest: this tree computes
polylogue-5fe2342e...against four recorded graphs that do not include it, i.e.absent→ a full bootstrap. Per tonight's gate policy that run belongs on the maincheckout, scheduled centrally. Consequence worth planning for: once this merges,
every checkout's digest changes and the fleet pays one bootstrap — worth batching
with other digest-touching work.
Not addressed: the ≤30s target was not reached.
test_large_session_filealone is~23s of real ingest on a quiet host and sets the floor; getting under it means
changing what that test covers.
Repaired in passing while rebasing (both pre-existing on master, neither mine)
test_quick_verify_omits_pytestwas failing on master. feat(devtools): gate oracle integrity — tests must certify live code, hermetically #4026 added thelab policy oracle-integritystep tobuild_verify_stepswithout updating thetest that pins the quick lane's label list, so master emitted 11 labels against
an assertion expecting 10. Verified pre-existing: my branch's diff touches
neither the builder region nor that list. One line added to the expected list.
oracle-integritybaseline entries reanchored. That gate keys findingsby line number, and this branch's additions shifted two pre-existing entries
(
test_daemon_resilience.py136→165,test_web_reader.py1456→1464). Hand-editedthose two rather than regenerating the baseline, so nothing unrelated could be
silently blessed — entry count is 29 before and after, and the diff is exactly
two line numbers.
Findings for the coordinator (bd is read-only here)
polylogue-b9yw7 (
cleanup.complete=false): ROOT-CAUSED AND FIXED.tests/infra/workload_artifacts._make_read_onlystrips write bits from theseeded-archive tree it publishes — from the directories as well as the
files — so the cache is immutable once built. A directory without its write
bit cannot have entries unlinked from it, so any test that builds such a cache
under the run's basetemp (
query_cardinality_archiveputs one atwork/"seeded-cache") left a treeshutil.rmtreesimply could not remove.cleanup_managed_tmpfs_pathreturned False, and because_release_baseline_allowedrequirescleanup.complete is True, a greencomplete-corpus run lost release authority. Confirmed live 2026-08-19: 16
leaked
/dev/shm/pytest-polylogue-*trees,rmrefusing with Permissiondenied on
.../seeded-cache/artifacts/*/wire/*.jsonl,chmod -R u+wclearingthem, and the next receipt going 39.5s OK. Receipts:
20260819T003921Z(wsb worktree) and20260819T010648Z(main), bothcleanup:falseon the s11 lane. It blocked two merges before diagnosis.Correction to my earlier comment on this PR. I previously proposed an
unsynchronized-reclaimer race between
cleanup_managed_pytest_basetempand thesupervisor's exit pass. That was wrong. The two reclaimers are real, but they
are not why cleanup failed — a permission bit was, and the mechanism above
reproduces deterministically where my race never did. Recording it because the
wrong theory is on this PR already.
Three changes, per the operator directive that tmpfs must clean up on its own,
never leak, and clean ASAP:
master. This PR keeps exactly one implementation of it
(
_restore_owner_write) and addsforce_rmtreeas the shared thin entrypoint calling it, because fix(harness): tmpfs cleanup removes read-only artifact trees #4018 patched only the supervisor's exit pass
while
verify_runs.cleanup_managed_pytest_basetempand the sweep have thesame defect and the same need.
describe_managed_tmpfs_cleanuproutesthrough it too, so there is one removal sequence rather than two. My own
duplicate implementation was dropped on rebase, including its
S_IXUSRaddition —
_make_read_onlystrips only write bits, so fix(harness): tmpfs cleanup removes read-only artifact trees #4018'sS_IWUSR-only repair is correct and mine was over-broad.failed to unlink stayed until a human noticed.
sweep_stale_managed_basetempsnow runs at verify/test preflight, applying the same conservative ownership
test as the exit path: reclaim only when the claim lock is free AND the
recorded owner is positively dead; an unknown or live owner is left alone,
and the shared
-seeded-corpus cache is never a candidate. It is placedimmediately before basetemp admission, so the space it returns is headroom
the admission policy can then admit against.
that one asserts the boolean
cleanup_managed_tmpfs_pathAPI, this one the(complete, reason, residual)tuple and the precondition that a plainrmtree raises), verified to actually go red: it plants a read-only
work/seeded-cache/artifacts/*/wire/*.jsonlsubtree in a managed path,asserts a plain
rmtreeraises, then asserts cleanup returns True and thetree is gone. With the fix reverted it fails
assert (False, ['wor...sions.jsonl']) == (True, [])— and note the residuallist names the exact live evidence path, so the diagnostic added earlier in
this PR would have identified this in one receipt.
A sweep test covers the three-way discrimination (dead owner reclaimed, live
owner spared, shared cache spared). Disposition: I believe this closes
b9yw7, with one honest residual — a basetemp with no claim file and no live
owner is still not reclaimable, because "unclaimed" cannot be distinguished
from "claimed a moment from now" without inventing an age threshold. Real
managed runs always write a claim, so this affects hand-made trees rather than
run debt. Your call whether that residual warrants keeping b9yw7 open.
Found and fixed in passing: my own survivor test leaked its
/dev/shmfixtureevery run (monkeypatch is still active inside the
finally, so the cleanuprmtreewas the patched no-op) — six leaked dirs, nowmonkeypatch.undo()first. Removed the strays by hand.
Design question for the coordinator, not changed here:
_release_baseline_allowedrequires
cleanup.complete is True, so release-baseline authority on a complete,green corpus is gated on whether a disposable scratch directory unlinked. The
quiescence signal that would justify withholding trust (
controller_group_alive)is already checked separately, upstream of this flag. Loosening a merge-authority
gate is your call, so I left it alone.
Residual flake, unresolved.
test_sigterm_read_only_daemon_records_forensicsfails roughly 1 in 12runs with a different mode than the one this PR fixed: the daemon exits
cleanly on SIGTERM (
128+SIGTERM, lifecycle row correct) and its log carriesthe
received SIGTERM; dumping all thread stacksheader, but the dump body(
Current thread) is absent. So the forensic dump the daemon promises issometimes not produced.
Bounded, not root-caused: 8/8 clean in a standalone subprocess probe both
with and without this PR's readiness gate; 10/10 clean under bare pytest;
~1/12 only under the managed
devtools testharness. Falsified: thatlogger.errorinside the signal handler (polylogue/daemon/lifecycle.py:247)deadlocks on the logging lock — a minimal reproduction of exactly that shape
deadlocked 0/12 times. I had read a single instrumented failure as confirming
it; it did not, and I am flagging that rather than shipping the theory.
Note
faulthandler.dump_tracebackthere sits undercontextlib.suppress(Exception), so if it is raising, nothing records why.This is a pre-existing daemon forensics gap, not something this PR introduces —
but the readiness gate changes which mode dominates, so it is now the visible
one. I did not weaken the assertion, retry it, or skip the test. Your call
whether this blocks the merge; the rest of the PR is independent of it.
New, unfiled: a daemon whose ops tier is locked by another connection
during startup can be held well past its shutdown contract — over 90s against
a documented "well under the normal 30s SQLite wait" — because the blocking
SQLite wait is not signal-interruptible. This PR makes the test stop entering
that window; it does not change the daemon. Worth its own bead against
polylogue/daemon/cli.py, and worth deciding whether the contract is meant tohold during startup at all.
8226a-C: the serial lane's cost is now bounded rather than serialized;
residual cost is the 23s ingest floor named above.