From ae52db3147931edb0cce22f2436dedf7c97efb1b Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 29 Jul 2026 17:05:10 +0200 Subject: [PATCH] test: make three Windows nondeterminisms deterministic All three cost real release cycles, and none is fixed by widening a budget. daemon_application_cancels_physical_job_only_after_final_session waited for the SUBSCRIBER COUNT to reach 2, then cancelled both sessions and asserted the physical job had started exactly once. The job starts asynchronously after subscription, so both cancels could land first and leave starts == 0 -- arguably the correct outcome. It now waits for the state the assertions actually require. Verified 47/47 on Windows, the only platform it ever failed. The parallel harness refused outright when the suite leader had already exited, because taskkill /T cannot walk a tree from a dead PID. But the leader can exit between the timeout decision and that call, so the harness itself lost a race: a natural exit at the wrong moment failed the whole wave. It now proves cleanup the only way still available -- nothing parented to that PID -- and its contract asserts the PROPERTY rather than the phrase "tree cleanup" it used to grep for. That string pin is what broke when the guard was reworded while behaving correctly; the contract now checks rc==2 AND that the descendant really did survive, which would also catch a guard that claims to fail closed while leaking. extract_wide_flat_file_is_linear took ONE sample per size, so the ratio carried the noise of both. On a loaded Windows VM linear code measured 51x against a 40x bound (184ms -> 9387ms). Best-of-N instead: timing noise only ever adds time, so the minimum is the cheapest good estimate of the noise-free cost. The bound is deliberately unchanged -- it sits where linear (~20x) and quadratic (~128x) are each >=2x away, so raising it would move the test toward the very signal it exists to catch. Now measures 19.1x on Windows, 21.4x on macOS. Also: the smoke's `cli` helper redirected stderr to a file and discarded it, so any of the 10 bare `VAR=$(cli ...)` assignments could kill the run under `set -euo pipefail` printing NOTHING. One such abort cost a full Windows cycle just to locate and still could not be attributed. It now surfaces the command and its stderr. Neutral wording on purpose: one call site expects a non-zero exit and must not read as a failure. Signed-off-by: Martin Vogel --- scripts/run-test-wave.py | 48 ++++++++++++++++++++++--- tests/test_daemon_application.c | 12 +++++-- tests/test_extraction.c | 32 +++++++++++++++-- tests/test_parallel_harness_contract.sh | 19 +++++++++- 4 files changed, 102 insertions(+), 9 deletions(-) diff --git a/scripts/run-test-wave.py b/scripts/run-test-wave.py index 8faa6e75c..44a16c23c 100755 --- a/scripts/run-test-wave.py +++ b/scripts/run-test-wave.py @@ -123,15 +123,55 @@ def start_suite( ) +def windows_descendants(pid: int, timeout: int) -> bool: + """True if any live process still claims `pid` as its parent. + + Used only when the suite leader has already exited: `taskkill /T` cannot + walk a tree from a dead PID, so cleanup is proven by asking whether anything + is still parented to it. One level deep on purpose -- Windows does not + reparent orphans, so a grandchild keeps pointing at its own (dead) parent + and would not be found here. That is a weaker proof than taskkill /T, which + is why it is reserved for the case where the strong proof is impossible. + """ + try: + completed = subprocess.run( + [ + "powershell.exe", + "-NoProfile", + "-NonInteractive", + "-Command", + "@(Get-CimInstance Win32_Process -Filter " + f"'ParentProcessId={pid}').Count", + ], + check=False, + stdin=subprocess.DEVNULL, + capture_output=True, + text=True, + timeout=timeout, + ) + except (OSError, subprocess.TimeoutExpired): + return True # cannot prove absence -> assume the worst + if completed.returncode != 0: + return True + return (completed.stdout or "").strip() not in ("0", "") + + def terminate_process_tree(active: ActiveSuite, kill_grace: int) -> None: process = active.process leader_exited = process.poll() is not None if os.name == "nt": if leader_exited: - raise RuntimeError( - f"suite {active.name!r} leader exited before Windows tree cleanup " - "could be proven" - ) + # The leader can exit on its own between the timeout decision and + # this call. Refusing outright made the harness itself lose a race: + # a natural exit at the wrong moment failed the whole wave, which is + # how a deliberately-hanging fixture suite reddened a release run. + # taskkill /T cannot walk a tree from a dead PID, so prove cleanup + # the only way still available -- nothing is parented to it. + if windows_descendants(process.pid, kill_grace): + raise RuntimeError( + f"suite {active.name!r} leader exited leaving live descendants" + ) + return try: completed = subprocess.run( [ diff --git a/tests/test_daemon_application.c b/tests/test_daemon_application.c index cda557ea5..e81b766a3 100644 --- a/tests/test_daemon_application.c +++ b/tests/test_daemon_application.c @@ -2855,10 +2855,17 @@ TEST(daemon_application_cancels_physical_job_only_after_final_session) { started[i] = cbm_thread_create(&threads[i], 0, app_request_thread, &requests[i]) == 0; } bool subscribed = started[0] && started[1] && app_wait_for_subscribers(application, project, 2); - if (subscribed) { + /* The physical job starts asynchronously once a session subscribes, so a + * subscriber count of 2 does NOT imply it has started. Cancelling both + * sessions before that point leaves starts == 0 -- arguably the correct + * outcome, and the reason this test failed intermittently on Windows while + * passing everywhere else. Wait for the state the assertions below actually + * require; `starts` only increments, so this cannot miss the transition. */ + bool job_started = subscribed && app_wait_for_atomic_int(&fake.starts, 1); + if (job_started) { callbacks.session_cancel(callbacks.context, sessions[0]); } - bool one_left = subscribed && app_wait_for_subscribers(application, project, 1); + bool one_left = job_started && app_wait_for_subscribers(application, project, 1); int cancels_after_first = atomic_load(&fake.cancels); if (one_left) { callbacks.session_cancel(callbacks.context, sessions[1]); @@ -2874,6 +2881,7 @@ TEST(daemon_application_cancels_physical_job_only_after_final_session) { ASSERT_TRUE(setup); ASSERT_TRUE(subscribed); + ASSERT_TRUE(job_started); ASSERT_TRUE(one_left); ASSERT_EQ(atomic_load(&fake.starts), 1); ASSERT_EQ(cancels_after_first, 0); diff --git a/tests/test_extraction.c b/tests/test_extraction.c index 93a329984..2bc3f3c88 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -3889,6 +3889,32 @@ static long extract_wide_flat_ms(int n, int *out_defs) { return (b.tv_sec - a.tv_sec) * 1000L + (b.tv_nsec - a.tv_nsec) / 1000000L; } +/* Best-of-N. Timing noise only ever ADDS time, so the minimum of a few runs is + * the cheapest good estimate of the noise-free cost. A single sample at each + * size made the RATIO carry the noise of BOTH measurements: on a loaded Windows + * VM this read 184ms -> 9387ms (51x) for code that measures ~20x unloaded, and + * tripped a bound calibrated for exactly that linear case. + * + * Deliberately NOT solved by raising WF_RATIO_MAX: the bound sits where it does + * because linear (~20x) and quadratic (~128x) are each >=2x away from it, so + * inflating it moves the test toward the very signal it exists to catch. This + * keeps the threshold and removes the variance instead. */ +static long extract_wide_flat_ms_best_of(int n, int reps, int *out_defs) { + long best = -1; + for (int i = 0; i < reps; i++) { + int defs = 0; + long ms = extract_wide_flat_ms(n, &defs); + if (ms < 0) { + return ms; + } + if (best < 0 || ms < best) { + best = ms; + *out_defs = defs; + } + } + return best; +} + TEST(extract_wide_flat_file_is_linear) { /* SCALING-RATIO guard: assert the COMPLEXITY CLASS, not a wall-clock * bound. Index-based ts_node_child(i) child loops are O(i) per call — @@ -3912,8 +3938,10 @@ TEST(extract_wide_flat_file_is_linear) { enum { WF_SMALL = 20 * 1000, WF_BIG = 400 * 1000, WF_RATIO_MAX = 40, WF_FLOOR_MS = 120 }; int defs_small = 0; int defs_big = 0; - long t_small = extract_wide_flat_ms(WF_SMALL, &defs_small); - long t_big = extract_wide_flat_ms(WF_BIG, &defs_big); + /* Small is cheap, so sample it more; big dominates runtime, so twice is the + * affordable compromise that still discards one unlucky sample. */ + long t_small = extract_wide_flat_ms_best_of(WF_SMALL, 3, &defs_small); + long t_big = extract_wide_flat_ms_best_of(WF_BIG, 2, &defs_big); ASSERT_GTE(t_small, 0); ASSERT_GTE(t_big, 0); /* Anti-vacuous guard: the breadth was actually walked at both sizes. */ diff --git a/tests/test_parallel_harness_contract.sh b/tests/test_parallel_harness_contract.sh index b97d2de7a..a658dc56a 100755 --- a/tests/test_parallel_harness_contract.sh +++ b/tests/test_parallel_harness_contract.sh @@ -373,11 +373,28 @@ try: stdout, stderr = process.communicate(timeout=8) if os.name == "nt": - if process.returncode != 2 or "tree cleanup" not in stderr: + # Assert the PROPERTY, not the wording. This used to require the phrase + # "tree cleanup" in stderr, which pinned one specific refusal message: + # rewording the guard broke the contract while the behaviour was still + # correct. What "fails closed" actually means is that the scheduler + # refused (rc=2) AND did not silently leave the descendant behind as if + # cleanup had succeeded. + descendant_pid = int(descendant_path.read_text(encoding="utf-8")) + if process.returncode != 2: raise SystemExit( f"FAIL: Windows timeout race did not fail closed " f"(rc={process.returncode}, stdout={stdout!r}, stderr={stderr!r})" ) + if process_state(descendant_pid) != "live": + raise SystemExit( + "FAIL: Windows timeout race refused without a surviving descendant " + "to refuse over -- the fixture no longer exercises the race" + ) + if "cleanup" not in stderr.lower(): + raise SystemExit( + f"FAIL: Windows timeout race refused without naming a cleanup " + f"failure (stderr={stderr!r})" + ) else: if process.returncode != 0: raise SystemExit(