Fix: assert worker concurrency by overlap, not wall-clock

[ChaoWao]

Summary

test_parallel_wall_time asserted elapsed < 0.8 * serial — a wall-clock magnitude proxy for "the workers ran concurrently." A magnitude bound is the wrong shape for a concurrency property: it conflates concurrency with host timer accuracy and scheduling latency. So it is replaced, not widened.

This is still live, and it taxes unrelated PRs

Not a historical flake — measured frequency and a fresh occurrence:

ut (macos-latest, 3.10) jobs sampled, Jul 20-26	218
test_parallel_wall_time failures among them	4 (~1.8%)
ut (ubuntu-latest, 3.10) jobs sampled	101
failures among them	0
most recent occurrence	2026-07-27T01:58Z, got 0.54s

That last one landed on add-mx-fp8-fp4-dtypes — a PR adding FP8/FP4 dtype support, which has nothing to do with fork, shared memory, or scheduling. That is the actual cost of leaving this in: a ~2% chance of a red check on work that cannot have caused it, and a maintainer spending time deciding whether it is theirs.

On the root cause: still open, and this PR does not claim otherwise

macOS time.sleep over-sleeps under load (measured 0.2 s → 0.24-0.31 s), but that is not enough to explain CI's 0.49-0.57 s for three parallel sleeps — that needs ~2.5-2.9x, and the measurement found 1.2-1.55x. The failure also did not reproduce on bare metal under harsher conditions than the runner.

So whether the runner sees worse inflation or genuine partial serialisation is unresolved. This PR does not claim "no defect" and does not auto-close #1496.

What does bound the risk is structural rather than empirical: _SubWorkerPool is defined in this test file and never imports simpler.worker. Whatever the macOS runner does to three forked processes, no simpler code path is implicated — the old assertion was measuring a host-scheduler property through a POC fixture. That holds whether or not the serialisation is real, which is why it is the sound basis for changing the test now, ahead of an answer.

The change

• Overlap assertion — each worker records its [begin, end] work window in its mailbox; assert the three windows mutually overlap (max(begins) < min(ends)). Relational, not a bound: it holds whether a sleep takes 0.2 s or 0.6 s, so it is immune to the timer inflation identified above, yet fails if the pool serialises. time.monotonic() is system-wide on both Linux and macOS, so the cross-process comparison is valid.
• Distinct per-worker result tags so result routing is actually verified. Previously all three callables returned 200 and the assertion was result == 200, so a result delivered to the wrong mailbox was silent.
• Codify the principle in docs/testing.md: off-hardware tests must not assert wall-clock magnitude as a proxy for a non-temporal property — with an exception for tests where wall time genuinely is the subject (a timeout firing, a deadline respected).

The new assertion is strictly more sensitive than the one it replaces: it fails on any serialisation, where the old bound only fired when serialisation was severe enough to push past 0.48 s.

If it flakes anyway

Then #1496 gets its answer. An overlap failure is direct evidence of serialisation — more than the original investigation produced — and the issue should be reopened with that failure attached rather than a new one filed.

Testing

• pre-commit run on both files — markdownlint-cli2, ruff check/format, pyright all pass
• All 6 cases in test_hostsub_fork_shm.py pass (direct invocation; module imports only stdlib + torch)
• Negative proof: forced serial dispatch makes latest_begin > earliest_end → the overlap assertion trips, so it is not vacuous
• 10/10 under taskpolicy -b + 7 CPU hogs — the regime that inflated sleeps to ~300 ms; overlap stays green, as expected for an inflation-immune assertion
• CI ut (macos-latest) / ut (ubuntu-latest) confirm green

Refs #1496 (left open pending the macOS answer)

[coderabbitai]

Warning

Review limit reached

@ChaoWao, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 678420af-67d9-4caa-aa4c-624f0264d0f9

📥 Commits

Reviewing files that changed from the base of the PR and between f7909e2 and c19c79e.

📒 Files selected for processing (2)

• docs/testing.md
• tests/ut/py/test_hostsub_fork_shm.py

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[ChaoWao]

The docs/testing.md principle is right and worth landing on its own. Three problems with the code change, one of which contradicts the PR's own docstring.

---

1. The measured mechanism does not reach the observed magnitude

The PR attributes the flake to macOS time.sleep over-sleeping, measured at 0.2 s → 0.24-0.31 s. Run the arithmetic against what CI actually reported:

For three workers sleeping concurrently, elapsed ≈ max(individual sleep). So:

	elapsed
measured over-sleep, fully parallel	0.24-0.31 s
assertion bound	0.48 s
what CI actually failed with	0.49-0.57 s

The measured inflation passes the bound with 35% to spare. To reach 0.49-0.57 s while parallel, a single sleep would have to inflate ~2.5-2.9x, and the measurement found 1.2-1.55x. There is a factor of two unaccounted for.

The PR is explicit that the failure was not reproduced ("not reproducible on bare metal even under harsher conditions"). So what was demonstrated is that a mechanism exists which inflates sleeps; what was not demonstrated is that it inflates them enough. "Not a code defect" is the conclusion that justifies deleting the assertion, and that conclusion is not established by this evidence.

The alternative it displaces — partial serialisation on the runner — remains equally unproven, but it is the one that predicts 0.49-0.57 s. It should not be dropped in favour of a mechanism whose measured size is half of what is needed.

2. Dropping the assertion removes the property, and a non-magnitude alternative exists

After this change nothing verifies that the workers run concurrently. If a regression made _SubWorkerPool.dispatch block until completion, this test would pass: three workers, correct results, no errors, serial execution.

The new principle says "Assert the property directly — the functional result or an observable effect — instead." This change asserts only the functional result and drops the property.

Concurrency has a direct, non-magnitude formulation: have each worker write its own start and end timestamps into its mailbox, and assert the three intervals overlap. That is relational, not a bound — it holds whether a sleep takes 0.2 s or 0.6 s, so it is immune to exactly the over-sleeping this PR identifies, while still failing if the pool serialises. It satisfies the principle rather than being an exception to it.

If overlap is rejected for a reason I am not seeing, that reasoning belongs in the PR, because "wall-clock magnitude is a bad proxy" does not by itself imply "assert nothing".

3. The docstring claims coverage the test cannot have

Case 5 — the pool drives N workers at once without cross-talk.
[...] must return its own result

All three workers are registered with _make_sleep_fn(sleep_sec) for the same sleep_sec = 0.2, so every callable returns int(0.2 * 1000) = 200, and the assertion is result == 200 for each. If worker 0's result were routed into worker 1's mailbox the test would still pass.

Cross-talk and result-routing are therefore not covered, and "at once" is precisely what was just removed. Both claims in the new docstring are unsupported by the new test body.

Cheap fix that makes the claim true: give each worker a distinct duration (sleep_sec = 0.1 + 0.05 * i, or just distinct return values) so each result is unique to its worker. That is worth doing regardless of how 1 and 2 are resolved — it is the routing check the docstring already promises.

---

Suggested split

The docs/testing.md principle is good and I would land it as-is, including the exception clause for tests where wall time genuinely is the subject.

For the test: make the results distinguishable (3), and replace the deleted bound with an interval-overlap assertion (2) rather than nothing. If the overlap assertion then proves flaky on the macOS runner, that is a much stronger result than this PR currently has — it would be direct evidence for serialisation, which is the open question in #1496.

Note that closing #1496 with Fixes #1496 records "flaky proxy, no defect" as the outcome. Given point 1, I do not think the evidence supports that yet.

[ChaoWao]

All three points taken — revised in c19c79e.

1. Magnitude gap — agreed, the claim was unsupported. Measured inflation (0.2 s → 0.24-0.31 s, 1.2-1.55x) gives elapsed ≈ 0.31 s for three parallel sleeps, ~35% under the 0.48 s bound; reaching 0.49-0.57 s parallel needs ~2.5-2.9x, which was never measured. The commit message and PR description now state this plainly: the root cause (worse VM inflation vs. partial serialisation) is open, not "no defect". Nothing is closed.

2. Overlap, not nothing — the deleted bound is replaced by an interval-overlap check, exactly as described. Each worker writes [t_begin, t_end] into its mailbox (offsets 24/32, published before TASK_DONE); the test asserts max(begins) < min(ends). Relational, not a magnitude bound — holds at 0.2 s or 0.6 s, fails if the pool serialises. Verified non-vacuous: forced serial dispatch makes latest_begin > earliest_end, so the assertion trips.

3. Distinct results — each worker now returns a unique tag (100, 200, 300); the result check is per-worker, so a misrouted result fails. The docstring's cross-talk / own-result claim now holds.

Split — docs/testing.md principle kept as-is (including the exception clause). Refs #1496, not Fixes — the issue stays open. If the overlap assertion flakes on the macOS runner, that is direct evidence for serialisation, the open question in #1496.

[ChaoWao]

Re-reviewed c19c79e7. All three points are addressed, and the implementation is correct on the details that could have quietly broken it:

• time.monotonic() is the right clock — system-wide on both Linux and macOS, so the cross-process comparison is valid. perf_counter() would have been wrong here.
• state is still written last in _mb_write_result, after both timestamps, so the parent cannot observe TASK_DONE with stale window values.
• max(begin) < min(end) is the correct test for N intervals sharing a common point, not just pairwise overlap.
• Offsets 24/32 fit inside MAILBOX_SIZE = 256.
• Distinct tag=100*(i+1) makes the routing assertion real — worker 0's result landing in worker 1's mailbox now fails.

The overlap assertion is strictly stronger than what it replaces: it fails on any serialisation, where the old bound only fired when serialisation was severe enough to cross 0.48 s. If the runner really does serialise at ~2%, this should catch it at ≥2%, with a message that says so directly.

One thing left: Fixes #1496 still overstates the outcome

The magnitude claim is withdrawn from the body, which is right. But auto-closing #1496 records the conclusion "flaky proxy, no defect", and this PR does not establish that — it replaces an indirect assertion with a direct one and has not yet seen the direct one survive a macOS run.

I would still close #1496, but for a reason the evidence actually supports, and it is a stronger reason than the one currently claimed:

_SubWorkerPool is defined in this test file and never imports simpler.worker. Whatever the macOS runner does to three forked processes, there is no simpler code path implicated — the old assertion was measuring a host-scheduler property through a POC fixture. That is true regardless of whether the serialisation is real, which is exactly why it is the better reason to close on.

Concretely, I would either:

• reword the body so the close reason is "the assertion tested a host-scheduler property through a test-local fixture; replaced with a direct relational assertion", not "no defect, timer artifact"; or
• drop Fixes #1496, merge, and close [Bug] test_parallel_wall_time: forked workers run near-serially on macOS CI (~2% of runs) #1496 manually once the macOS job has run this green a few times.

Either is fine. What I would avoid is leaving "macOS time.sleep over-sleeping caused it" as the recorded answer, since the factor-of-two gap was never closed and someone will read that later as settled.

And if the overlap assertion ever does fail on macOS — reopen #1496 with that failure attached. It would be the first direct evidence of serialisation, which is more than the original investigation ever produced.

Approving on the code. The only change I am asking for is to the close framing.

[ChaoWao]

ut-a5 fails on this PR and on the re-run with OSError: Environment variable 'ASCEND_HOME_PATH' is not set. in two a5 hardware tests (test_dynamic_alloc_hw.py, test_platform_comm.py). Flagging rather than ignoring (discipline: a red check is not silently "flaky"):

• Unrelated to this PR. The change is a pure-Python fork/shm test + a doc. test_hostsub_fork_shm.py isn't selected in ut-a5 (the run selects 4 a5-hw tests, 971 deselected), and the sibling ut-a2a3 passes.
• Reproduced on re-run — same ASCEND_HOME_PATH error; the a5 toolchain env isn't provisioned on the runner.
• Looks like known infra — there's a ci-a5-test-runner branch with its own failing runs.
• This change's own verdict is green: ut (macos-latest) and ut (ubuntu-latest) both pass.

Failing run: https://github.com/hw-native-sys/simpler/actions/runs/30233519385/job/89879523505