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(