ci(tooling): port the second fault-handler belt, calibrate the gate bound (BACKLOG #1304) - #818
Open
wshallwshall wants to merge 1 commit into
Open
ci(tooling): port the second fault-handler belt, calibrate the gate bound (BACKLOG #1304)#818wshallwshall wants to merge 1 commit into
wshallwshall wants to merge 1 commit into
Conversation
…ound (BACKLOG #1304) The tooling tier carried one of the engine tier's two hang-diagnostic belts. PYTHONFAULTHANDLER=1 plus -o faulthandler_timeout= was absent from `Harness tests (pytest)`. Ported at 150 against that step's own 120, pinned by tests/test_ci_faulthandler_belts.py. All three mutations verified red first. Measured, and it corrects ci.yml's stated reason: pytest-timeout's thread method DOES fire on a wedged subprocess.wait. It dumps from a watchdog thread and calls os._exit(1), so it never needs to interrupt the main thread. The port is a second opinion on an independent mechanism, not the thing that names the failure. GATE_TIMEOUT_S carried no calibration. Worst case 4.6s over n=127 real launches, and calibrating it found a defect: at 60 against addopts' --timeout=60 the diagnostic could never fire locally. Moved to 45, a 9.8x margin.
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.
BACKLOG #1304 -- the instrumentation limb, not the cause
The cause is untouched. The item's own limit still holds: a 4-vCPU hosted runner is
not measurable from a developer box, so the distribution shape transfers and the
absolute numbers do not. Nothing here theorises about runner contention, xdist
pressure, or pwsh startup.
Four things landed: the answer to the item's one checkable question, the port that
answer implied, the missing calibration on the 60s bound, and tonight's fourth test
name. One thing is deliberately NOT built and is named below. One decision is the
owner's and is a recommendation only.
The tooling tier carried ONE of the two belts, not zero
That is the answer, and the halving is the point -- a bundled "it has no belts"
would have been the exact defect this item spent an amendment on.
--timeout-method=threadaddoptsinpyproject.toml, repo-widePYTHONFAULTHANDLER=1+-o faulthandler_timeout=env+ CLIThe control that makes the zero trustworthy.
PYTHONFAULTHANDLER: "1"was on 11steps in
ci.ymlbefore this change and is on 12 after -- so a bare grep of the filefinds it and concludes the tier has it. That is the same instrument error the item records twice ("does this FILE
contain a timeout" against "does this TEST have a bound"). The question here is "does
this STEP carry it", so the answer is read by parsing the YAML and addressing the
step by name. The engine step is the positive control: the same reader returns
present for
Tests (pytest)and absent forHarness tests (pytest)in one pass.What the port actually buys, which is less than the item hoped
I verified it on a deliberately wedged process before shipping it, and the result
contradicted the reason I was porting it for.
Paired local arms, on a test that blocks in
subprocess.runwith notimeout=ofits own -- the shape 164 of this tier's 317
subprocesscalls have, across 57 ofthe 138 files in
tests/tooling_manifest.txt(AST count at46ea10a78, against acontrol of 153 bounded calls, so a zero was reachable):
+ Timeout +(pytest-timeout)Timeout (0:00:03)!(faulthandler)Arm A already names the failure. The thread method fires and dumps the frame down
to
_winapi.WaitForSingleObject. Reading the pinnedpytest_timeouton disk sayswhy:
timeout_timerdumps from a watchdog THREAD and callsos._exit(1), so itnever needs to interrupt the wedged main thread.
So
ci.yml's stated reason for this belt is wrong. It says the belt "fires evenwhen the thread-timer CANNOT interrupt a main-thread C-level wait", naming
subprocess.wait. Measured false, above. I left the engine comment as it stands andrecorded the correction in the item and in the new comment: the belt is still worth
carrying, only its stated reason is wrong, and rewriting a contended region of
another tier's derivation is not this change's business. A reviewer may reasonably
ask for that one sentence to be fixed too; say so and it is a two-line follow-up.
What it does buy, and the comment says only this: a second watchdog on an
independent mechanism (CPython's
dump_traceback_later, not a Pythonthreading.Timer) writing down a different path -- a dup'd raw stderr fd, notconfig.get_terminal_writer()._pytest/faulthandler.pycomments that xdistmonkeypatches
sys.stderrwith a non-file, and whether the terminal-writer dumpsurvives a worker's
os._exitand reaches the controller log is NOT established here(no xdist on the measuring box).
PYTHONFAULTHANDLER=1additionally arms thefatal-signal handler before
pytest_configurearms pytest's own and afterpytest_unconfiguredisables it, and is inherited by Python children. faulthandleronly dumps; it never kills, so no run's outcome changes.
The 60s bound is calibrated, and calibrating it found a defect
tests/test_worktree_gate.py:36had no recorded calibration -- the surviving half ofthe item's not-measured list, and the half its own correction says still stands.
Worst case 4.6s per call, n=127 real launches across both gate files (p50 2.1s,
p99 4.2s, one sub-50ms sample excluded as a call that raised before launching).
Sequential, on a box already running several peer pytest sessions, so it is a
contended sample -- the useful direction, and still not a 4-vCPU runner.
The defect: at 60 the diagnostic could never fire locally. pytest-timeout arms in
pytest_runtest_protocol, covering setup + call + teardown; this bound starts later,inside the call. So pytest's window strictly contains it and at equal values pytest
expires first. Measured with a paired control rather than reasoned:
--timeout=5--timeout=30addoptscarries--timeout=60. So the bound was live on CI's tooling leg (whichoverrides to 120) and decorative on every local run -- exactly the silent
one-platform failure its 45s sibling's comment was written to prevent.
Moved to 45, a 9.8x margin, clearing both bounds and matching its sibling. This
is the one judgment call in the PR: it changes the signature string from
after 60 secondstoafter 45 seconds. Anyone grepping CI history before this merge wantsthe old string. The item records that.
Tonight's test names, recorded with the list left open
tests/test_worktree_gate_control_plane.py::test_a_bare_config_read_of_a_d0and::test_a_bare_config_read_of_a_d3, workers gw1 and gw0, PR 763 and PR 771 read fromthe job log. Same file as one already named in the item, same bound, two different
test functions.
Written in without a fresh total, deliberately. The item's own amendment records
that a counted enumeration nothing re-checks is the SDS-3.6 shape, and it has now
understated twice; a third count would just be the next thing to go stale. The note
says instead that the stable signature is the SCRIPT and the BOUND -- the test
function has changed on every sighting, while
worktree_gate.ps1andclaim-reconcile.ps1have not.Deliberately NOT built:
--max-worker-restart=0The engine step's third belt, and on the evidence it is the strongest remaining
candidate -- it is what answers a controller left polling a dead worker until the
30-minute step cap, which is the item's third observed mode.
It is not ported because it is a behaviour change, not instrumentation: a worker
death fails the run instead of being survived. The engine tier justified that on a
33-job sample showing the recovery path never once saved a run. There is no such
sample for this tier, and shipping it without one is the shape of transfer this item
exists to refuse. It needs its own measurement.
One mechanism finding that belongs to it, read from the pinned library rather
than inferred:
pytest_timeout.timeout_timerends inos._exit(1), and under-naworker that
os._exits closes its execnet channel with noworkerfinished, whichthe controller reports as a crashed worker. The item's amendment asked whether "a
timeout that takes the worker down with it would present exactly this way". This
establishes that such a path EXISTS. It does not establish that it fired, and the
amendment's refusal to assume in either direction stands.
Rerun budget -- recommendation only, the decision is the owner's
The item asks for one and calls the status quo an unwritten norm. It is a policy call
about how the queue is run, so nothing here implements it. Recommended shape:
real until someone says otherwise. The item's own sample is 4 clean / 3 failed / 1
clean; a streak of three is already outside what one rerun absorbs.
item's sharpest sentence is that rerunning is "manufacturing a green rather than
earning one", and the cost is not the rerun -- it is that afterwards a manufactured
green and an earned one are indistinguishable. A line in the PR body is the whole
remedy and it costs nothing.
Nothing distinguishes this item from a real regression at the moment it fires, and
the leg is the single largest contributor to a red default branch -- so the reader
who skips the log is exactly the reader who will discount a real one.
Not recommended: any automated retry. A retry inside the harness is the same act one
level down with the evidence hidden inside a passing test, which is the argument
run_gate's own docstring already makes for not retrying there.Checks
Ran:
ruff check,ruff format --check,mypy messagefoundry(strict, 267 files,clean),
scripts/docs/backlog_status_check.py,scripts/quality/licence_header_check.py,scripts/quality/control_char_check.py,scripts/quality/workflow_local_action_check.py,and pytest over
test_ci_faulthandler_belts(new),test_tooling_partition,test_ci_engine_step_excludes_webconsole,test_ci_tooling_gate,test_backlog_citation_check,test_backlog_status_check,test_backlog_hygiene_claim_extraction,test_ledger_check-- 146 passed -- plustest_worktree_gateandtest_worktree_gate_control_planeat the new bound.Mutation-proved the new guard before calling it done: dropping
-o faulthandler_timeout=reds 2 tests, inverting the ordering to 100 reds 1, anddropping the engine step's
PYTHONFAULTHANDLERreds 1. Each reds only its own test.Skipped, and it needs a hosted leg to read: the full suite. A peer session
measured a local full run reaching four percent in eighty-five minutes on this box.
Also skipped: any
-n 4arm of the timing measurement --pytest-xdistis notinstalled in the venv available to this worktree, which is why the xdist claims above
are marked not-established rather than measured.
Read the
repo harness testslegs on this PR specifically. It edits that job'sown step, so this PR is the first place the ported belt runs.