Skip to content

perf(harness): bound the load_sensitive lane and fix its startup race - #4012

Merged
Sinity merged 11 commits into
masterfrom
feature/harness/a4-serial-lane
Aug 19, 2026
Merged

perf(harness): bound the load_sensitive lane and fix its startup race#4012
Sinity merged 11 commits into
masterfrom
feature/harness/a4-serial-lane

Conversation

@Sinity

@Sinity Sinity commented Aug 19, 2026

Copy link
Copy Markdown
Owner

Summary

The warm devtools verify gate spends most of its wall clock in one place: the
load_sensitive lane, 80.71s of a 110.67s run. This bounds that lane's
concurrency 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-23438ba4 puts the lane at 80.71s against
8.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 real
polylogued subprocess with its own archive root (workspace_env) and its own
loopback port — they share no global resource.

load_sensitive records a real observation: the parallel lane's full worker
count 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.TimeoutExpired after 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_start returns as soon as DaemonLifecycle.start persists
its row. In polylogue/daemon/cli.py that happens early — install_signal_handlers
runs 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_wait
then takes BEGIN EXCLUSIVE on ops.db inside that window, parking a daemon
startup write inside a blocking sqlite3_step, where CPython cannot run the
Python SIGTERM handler until the busy timeout expires — past the test's 90s bound.

Solution

Bounded lane (devtools/verify.py). The load_sensitive lane 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_steps takes serial_worker_args; the closed-world reconstruction
in _native_pytest_command_is_closed_world passes the observed worker request for
both lanes, which is exact for the lane under test and irrelevant for its sibling.
Nothing here is collection-affecting, so devtools/pytest_collection_contract.py
and the testmon environment digest are untouched — no graph invalidation.

Bin packing (tests/integration/test_daemon_resilience.py). Under dynamic
scheduling the longest member (test_large_session_file, ~23s of real 50K-message
ingest) 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_group bins pack the members longest-first so the makespan is bounded by
the 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_ready polls 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.0 bound 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-init just seeded for it and every run bootstraps while printing "lane
verifies start warm"; and import hypothesis fails outright (sysconfig raising
AttributeError: 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's home and 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_base failure I hit and
worked 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_peer asserts that
_socket_peer_disconnected observes the peer's FIN within the call, with no retry
or 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 on
2026-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 assertions
are 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-lane on the lane's own venv.

A correction to the baseline's framing. "7 load_sensitive tests / 80.7s" was
testmon'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_sensitive class in tests/unit/daemon/test_web_reader.py — and this PR
makes 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:

lane shape runs result
-n 0 (master's shape) 6 98.00s, 99.01s, 105.03s, 106.88s, 120.13s, and one 103.46s with 1 failed
-n 4 --dist=loadgroup (this PR) 4 14 passed in 34.87s, 39.62s, 40.72s, 50.84s

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:

-n 0   71.95s  green
-n 4   35.08s  green
-n 5  107.76s  1 failed (SIGTERM deadline starved)
-n 7   95.20s  2 failed (both SIGTERM tests, 90s subprocess timeout blown)

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 same
command gave 9.36s / 102.40s (1 failed) / 10.65s, so the variance collapse is
the evidence, not just the greens.

Full gate, devtools verify on this branch:

"terminal_green": true, "complete_corpus_covered": true,
"non_green_count": 0, "attested_unchanged_count": 20507

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 test table above is. And in 20260819T004849Z the lane took
114.70s at workers=1: under load average 22 adaptive_pytest_worker_count
collapsed 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 --quick success, mypy clean across 2607 files, and a focused
run 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_write turns all three read-only tests red
together (..._removes_read_only_artifact_trees, ..._removes_a_read_only_seeded_cache,
..._sweep_reclaims_a_dead_runs_read_only_tree...) and green again on restore, so
the sweep genuinely depends on #4018's repair rather than carrying its own copy.

No further full devtools verify was run on this branch, deliberately.
devtools/pytest_supervisor.py is a testmon environment-digest input
(devtools/pytest*.py, testmon_bootstrap.py:327), so the b9yw7 fix changes the
digest: 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 main
checkout, 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_file alone 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_pytest was failing on master. feat(devtools): gate oracle integrity — tests must certify live code, hermetically #4026 added the
    lab policy oracle-integrity step to build_verify_steps without updating the
    test 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.
  • Two oracle-integrity baseline entries reanchored. That gate keys findings
    by line number, and this branch's additions shifted two pre-existing entries
    (test_daemon_resilience.py 136→165, test_web_reader.py 1456→1464). Hand-edited
    those 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_only strips write bits from the
    seeded-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_archive puts one at
    work/"seeded-cache") left a tree shutil.rmtree simply could not remove.
    cleanup_managed_tmpfs_path returned False, and because
    _release_baseline_allowed requires cleanup.complete is True, a green
    complete-corpus run lost release authority. Confirmed live 2026-08-19: 16
    leaked /dev/shm/pytest-polylogue-* trees, rm refusing with Permission
    denied on .../seeded-cache/artifacts/*/wire/*.jsonl, chmod -R u+w clearing
    them, and the next receipt going 39.5s OK. Receipts:
    20260819T003921Z (wsb worktree) and 20260819T010648Z (main), both
    cleanup:false on 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_basetemp and the
    supervisor'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:

    1. Permission-repairing removal — the repair itself is fix(harness): tmpfs cleanup removes read-only artifact trees #4018's, already on
      master.
      This PR keeps exactly one implementation of it
      (_restore_owner_write) and adds force_rmtree as the shared thin entry
      point 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_basetemp and the sweep have the
      same defect and the same need. describe_managed_tmpfs_cleanup routes
      through it too, so there is one removal sequence rather than two. My own
      duplicate implementation was dropped on rebase, including its S_IXUSR
      addition — _make_read_only strips only write bits, so fix(harness): tmpfs cleanup removes read-only artifact trees #4018's
      S_IWUSR-only repair is correct and mine was over-broad.
    2. Eager, self-healing sweep. Cleanup was purely trailing, so a tree that
      failed to unlink stayed until a human noticed. sweep_stale_managed_basetemps
      now 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 placed
      immediately before basetemp admission, so the space it returns is headroom
      the admission policy can then admit against.
    3. Red twin (kept alongside fix(harness): tmpfs cleanup removes read-only artifact trees #4018's, which covers a different surface —
      that one asserts the boolean cleanup_managed_tmpfs_path API, this one the
      (complete, reason, residual) tuple and the precondition that a plain
      rmtree raises), verified to actually go red: it plants a read-only
      work/seeded-cache/artifacts/*/wire/*.jsonl subtree in a managed path,
      asserts a plain rmtree raises, then asserts cleanup returns True and the
      tree is gone. With the fix reverted it fails
      assert (False, ['wor...sions.jsonl']) == (True, []) — and note the residual
      list 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/shm fixture
    every run (monkeypatch is still active inside the finally, so the cleanup
    rmtree was the patched no-op) — six leaked dirs, now monkeypatch.undo()
    first. Removed the strays by hand.

  • Design question for the coordinator, not changed here: _release_baseline_allowed
    requires 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_forensics fails roughly 1 in 12
    runs with a different mode than the one this PR fixed: the daemon exits
    cleanly on SIGTERM (128+SIGTERM, lifecycle row correct) and its log carries
    the received SIGTERM; dumping all thread stacks header, but the dump body
    (Current thread) is absent. So the forensic dump the daemon promises is
    sometimes 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 test harness. Falsified: that
    logger.error inside 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_traceback there sits under
    contextlib.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 to
    hold 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.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 91f38807-48e4-4173-a3dd-946a9d7cae31


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Sinity
Sinity force-pushed the feature/harness/a4-serial-lane branch from 9238bfb to 27dcee2 Compare August 19, 2026 08:15
@Sinity
Sinity merged commit 7e4bcc4 into master Aug 19, 2026
2 of 3 checks passed
@Sinity
Sinity deleted the feature/harness/a4-serial-lane branch August 19, 2026 08:36
Sinity added a commit that referenced this pull request Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant