test: convert remaining TRUE-FLAKE-RISK wall-clock assertions to ordering witnesses (#438 family) - #549
Merged
Conversation
`assert elapsed < 2.0` made the OS scheduler part of the assertion. Measured on pristine fork/main with no diff applied, it FAILED at 2.9s. Profiling the window showed 2.336s of 2.35s was `_snapshot_child_pids` (a ps-based child scan run before the handshake) — the threshold was dominated by setup cost it was never meant to measure. Now asserts the ordering fact the bound stood in for: `_run_stdio` must unwind BY ITSELF via the inner connect_timeout (task is done, not pending, after a bounded wait), it raised TimeoutError, and the hanging initialize() was actually torn down. The 10s wait is a hang-guard 50x the 0.2s connect_timeout, not the assertion. RED-proved: removing the `asyncio.wait_for(session.initialize(), ...)` wrapper (the pre-NousResearch#59349 bug) fails on the new assertion by name. Source mutation reverted; this diff is test-only. Side effect: 4.4s -> 0.9s.
Same family as #438/#534: each asserted a *non-blocking* property by measuring elapsed real time, which makes the OS scheduler part of the assertion. - tests/acp_adapter/test_acp_mcp_discovery.py `elapsed < 0.2` - tests/hermes_cli/test_mcp_startup.py `elapsed < 0.2` - tests/hermes_cli/test_update_check.py `elapsed < 1.0` - tests/hermes_cli/test_api_key_providers.py `elapsed < 1.5` (vs a 2.0s sleep) Each now uses the #438 witness shape: an `entered` Event proving the background work really started, and a `returned` Event set in a `finally` on every exit path, asserted UNSET when the caller returns. That is the ordering fact the bound stood in for — the work is still in flight, so the caller cannot have waited on it. Side effect: removing the fixed sleeps drops these four files from 70.9s to 9.3s.
…ssion fails fast The stub's `stop.wait()` had no timeout. Under the inline-discovery mutation that RED-proves this test, the caller blocked forever inside the stub, so the suite HUNG instead of failing on the witness. A witness that can only be reached by hanging is not a gate. 10s is orders of magnitude above the real rendezvous and finite, so the regression now fails fast.
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.
test: convert remaining TRUE-FLAKE-RISK wall-clock assertions to ordering witnesses
Card
t_7e1f87ff. Continues the family started in #438 (and #534 /test_mcp_tool_issue_948.py).Same rule, applied to the members those PRs left behind:
The headline: one of these was failing on
fork/mainright nowtests/tools/test_mcp_stdio_init_timeout.pywas not a theoretical flake. On a pristinefork/maincheckout with zero diff applied, it failed:That bound was not measuring the behavior it protects
Profiling the measured window (
cProfile, cumulative):2.336s of the 2.35s window is
_snapshot_child_pids— aps-based child-process scanthat runs before the handshake and has nothing to do with the
connect_timeoutboundbeing asserted. Exactly the trap #426/#438 documented: the threshold silently absorbed
setup cost it was never meant to measure. The instrument was the defect.
What changed (5 conversions)
tests/tools/test_mcp_stdio_init_timeout.pyassert elapsed < 2.0TimeoutError, and the hanginginitialize()was actually torn downtests/acp_adapter/test_acp_mcp_discovery.pyassert elapsed < 0.2entered+discover_returnedEvents;returnedasserted unset when the caller returnstests/hermes_cli/test_mcp_startup.pyassert elapsed < 0.2(+ a poll loop)tests/hermes_cli/test_update_check.pyassert elapsed < 1.0prefetch_update_checktests/hermes_cli/test_api_key_providers.pyassert elapsed < 1.5(vs a 2.0s sleep — 0.5s margin)detect_zai_endpointreturnsEvery wait is finite (10s — orders of magnitude above a real rendezvous, so it is not
itself a timing assertion) so a genuine regression fails fast instead of hanging.
RED proofs — every conversion still gates
Each was verified by breaking the behavior it protects and confirming the new assertion
fails by name:
asyncio.wait_for(session.initialize(), ...)(the pre-NousResearch#59349 bug)_run()called inlineThe RED exercise caught a real defect in my own draft, which is the argument for making
it mandatory.
test_mcp_startup's stub used an unboundedstop.wait(). Under the inlinemutation the caller blocked inside the stub forever, so the suite hung rather than
failing on the witness — a witness only reachable by hanging is not a gate. Fixed to
stop.wait(timeout=10.0)(separate commit).A first mutation attempt on
mcp_startupcalled_discover()inline, which self-deadlockson the re-entrant
_mcp_discovery_lock. That deadlock was an artifact of the mutation, nota real regression, so it was discarded in favour of calling the inner discovery directly.
All source mutations were reverted — this diff is test-only (
git statusshows 5 testfiles;
git diff --name-onlyagainst the source files is empty).Verification (exact observed counts)
4 files, 113 tests passed, 0 failed in 70.9s4 files, 113 tests passed, 0 failed in 9.3sscripts/run_tests.sh tests/tools/test_mcp_stdio_init_timeout.py tests/acp_adapter/ tests/hermes_cli/ -q:576 files, 4789 tests passed, 0 failed (100% complete) in 95.3s5 files, 114 tests passed, 0 failed× 3. The convertedtest_mcp_stdio_init_timeoutpasses under load where the original failed idle.
Side effect
Removing the fixed sleeps: those 4 files go 70.9s → 9.3s, and
test_mcp_stdio_init_timeoutgoes 4.4s → 0.9s.Deliberately NOT converted (legitimate — do not "fix" these)
assert ev.wait(timeout=N)— asserts the event, not the duration. The number onlystops a hang. (The overwhelming majority of raw grep hits.)
tests/test_tui_entry_mcp_owner.py:35 assert elapsed >= 0.04,tests/tui_gateway/test_compute_host_phase1.py:260. These prove an injected waithappened; load makes them more reliable. Converting them would be a mistake.
tests/tools/test_local_background_child_hang.py(< 10.0vs a 15s+ hang),tests/agent/test_codex_ttfb_watchdog.py(< 20/< 30vs multi-minute stalls),tests/agent/test_cascading_interrupt_6600.py(< 10.0vs 30s+). Ratio matters.tests/test_hermes_state.py(adversarialFTS5 sanitizer runtime),
tests/agent/test_redact.py(regex backtracking blowup),tests/context_engine/test_lcm_media_token_accounting.py(token cost, not wall clock).The bound is the point of the test.
Follow-up candidates (not in this PR — kept reviewable)
Bounded scope per the card. These are plausible members that need individual profiling +
RED proofs and belong in their own diff:
tests/agent/test_auxiliary_explicit_cancellation.py— fourassert elapsed < 0.75tests/agent/test_memory_async_sync.py:153—assert elapsed < 0.5tests/hermes_cli/test_kanban_init_lock_bounded.py:69—assert elapsed < 1.0tests/agent/test_compression_review_76354.py:488—assert elapsed < idle * 1.8tests/tools/test_async_delegation.py:123—assert elapsed < 4.0(already backstoppedby an
active_count()witness, so low priority)Not for merge
Opening for review only — do not merge.