Test: pin the chip-run lane's blocking wait and drop its unused API

[ChaoWao]

Two code-health items on ChipRunLane, both surfaced while reviewing #1650 and left open when it merged (48dc78bf).

1. The blocking wait had no test holding it in place

ChipRun::wait_until(Deadline::max()) blocks on the device rather than re-polling — but nothing enforced that. The stubbed poll_run returns COMPLETE as soon as g_complete is set, so every wait_until in all twelve cases returned on its first pass, and ExpiredWaitLeavesTheRunLive passes an already-elapsed deadline so it too does one iteration.

A re-polling implementation would have passed the entire suite. That is exactly how the original spin — measured at 636,336 poll_run calls in a 200 ms run (~3.2 M/s, each one an rtSetDevice plus a device read in production) — reached main behind a green CI.

UnboundedWaitBlocksInsteadOfPolling closes it. The run is completable only by wait_run, so the blocking path is the sole way for the wait to finish, and it asserts both halves:

• the device wait was actually reached (wait0 in the event log)
• the poll count stays at most one

On the escape hatch: on that setup a re-polling implementation loops forever, and a hang is a poor CI failure — it burns the job timeout and reports nothing. So the poll stub completes the run itself past a bound (g_poll_completes_after, set by this one test). The regression then fails in milliseconds with

unbounded wait polled 64 times instead of blocking

Verified non-vacuous by mutation: with block_on_front() removed, the test without the hatch hangs (killed at 60 s); with the hatch it fails in 0 ms with the message above. Restored, all thirteen pass.

2. ChipRun::wait_until_launched is deleted

Zero callers, zero bindings — exported and untested from birth. Confirmed with a positive control, since an absence claim from one grep spelling is worth little:

$ git grep -c wait_until_launched
src/common/worker/chip_run_lane.cpp:1     # the definition
src/common/worker/chip_run_lane.h:1       # the declaration

Note ChipRun::launched() is a different accessor — it reads crossed_launch_fence, is bound as a property, and the child loop does use it. That one stays.

W1b will expose a live RunHandle and may well want a launch-only wait; deciding its shape then, against a real caller, beats keeping one shaped by guesswork that no test covers.

Validation

• 91/91 cpp UT (ctest -LE requires_hardware — CI's own filter, not -L no_hardware, which omits 18 tests including this one)
• 1277 passed / 13 skipped — full tests/ut/py
• a2a3 onboard sweep: 54 passed, plus 24 passed / 2 skipped in the resource phase

Both totals are unchanged from the base 48dc78bf, which is expected and not a
sign of a stale measurement:

• ctest counts binaries, not gtest cases. This PR adds a case inside
  test_chip_run_lane, so ctest stays 91 while the binary itself goes
  12 → 13 (--gtest_list_tests).
• The Python total is identical because this PR touches zero .py files
  (git diff 48dc78bf...HEAD --name-only | grep '\.py$' → empty). Measured
  1277 on the base as well, not inferred.

Not in scope

Issue #1742 (launched semantics in simpler_finalize_run) is a different launched — a bool in the platform-layer C API, unrelated to the lane's accessor. Same defect class, different file and layer; left alone.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change removes ChipRun::wait_until_launched(Deadline) and adds test controls and coverage for unbounded waits that reach wait_run without repeated polling.

Changes

Chip run waiting

Layer / File(s)	Summary
Remove launch waiting API src/common/worker/chip_run_lane.h, src/common/worker/chip_run_lane.cpp	Removes the public ChipRun::wait_until_launched(Deadline) declaration and implementation.
Test unbounded wait behavior tests/ut/cpp/hierarchical/test_chip_run_lane.cpp	Adds configurable poll completion and verifies that an unbounded wait reaches wait_run with at most one poll.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• hw-native-sys/simpler#1694: Removes related launch-signal, launch-wait, and LAUNCHING-phase handling.

Poem

A rabbit saw the launch wait go,
While wait_run blocked below.
Polls stayed few, the tests now know,
The run can wait without a show.
Wiggle ears—then off we go!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the regression test and removal of the unused API.
Description check	✅ Passed	The description directly explains the regression test, API removal, validation results, and out-of-scope changes.

---

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]

Review — #1743

Stated goal: two code-health follow-ups on ChipRunLane from #1650 — (1) add a real regression test pinning the unbounded wait's blocking behavior, (2) delete the dead wait_until_launched. Real goal matches — verified both independently, not from the PR body.

Change Breakdown

Bucket	Files	+/−	=
Core (src/common/worker/chip_run_lane.{cpp,h})	2	+0/−17	17
Test (tests/ut/cpp/hierarchical/test_chip_run_lane.cpp)	1	+34/−0	34
TOTAL	3	+34/−17	51

Trivial size, no split warranted.

Mechanism Brief

wait_until(Deadline::max()) is supposed to block via block_on_front() → worker->wait_native_run(...) rather than spin-poll (codestyle rule 5's blocking-primitive requirement). But every existing test's poll stub returns COMPLETE as soon as g_complete is set — so progress() alone always returns true on the first pass, block_on_front() never actually gets exercised, and a regression to naive re-polling would sail through the whole suite (which is exactly how #1650's original spin — 636336 poll_run calls in 200ms — reached main).

UnboundedWaitBlocksInsteadOfPolling closes that gap: the run is completable only via the wait_run stub, so wait_until is forced down the block_on_front() path, and the test asserts both that wait0 is in the event log (device wait actually reached) and that g_poll_count <= 1 (didn't spin). The g_poll_completes_after escape hatch exists purely so a regression fails in milliseconds with a readable message instead of hanging the CI job for its full timeout — this only fires for this one test.

I independently re-derived why the numbers work: wait_until's first progress() call polls once (front is LAUNCHED, not yet complete) → false; then unbounded ⇒ block_on_front() blocks on wait_native_run (logs wait0), calls finish() (→ TERMINAL), returns true ⇒ loop continues ⇒ progress() now short-circuits on TERMINAL with zero additional polls. Total poll count = 1, matching the assertion exactly.

Verified independently (not from PR body)

• wait_until_launched truly has zero callers/bindings, before and after: git grep -c wait_until_launched at baseline 48dc78bf returns exactly 2 hits (declaration + definition), and 0 anywhere in the tree after this PR — the removal is clean, no dangling references in docs/comments/bindings.
• ChipRun::launched() is a distinct, live accessor: bound as def_prop_ro("launched", ...) in task_interface.cpp:2022, and read at worker.py:2845/2850 in the child loop. Confirms the PR's claim that this one stays for a real reason, not by inertia.
• No pto-isa surface touched — pin (83d01313...) stays at the default advisory level, no escalation needed.
• CodeRabbit: no actionable comments. CI (what's finished so far): all green, no reds.

Issues Found

Must fix: none.

Should fix: none.

Consider:

1. The validation numbers in the PR body (91/91 cpp UT, 1277 passed / 13 skipped py UT) don't obviously reconcile with Refactor: centralize chip native run ownership #1650's own reported baseline (91/91 cpp — with one fewer test in this exact file — and 1252 py UT), even though this PR adds exactly one C++ test and touches zero Python files. This PR's single commit's parent is 48dc78bf directly (not rebased onto the later #1741/#1456, which likely account for the +25 py tests) — worth double-checking those numbers were captured on the actual PR branch rather than a local main that had already moved on. Doesn't affect correctness, just a reporting-hygiene nit.
2. Minor style: the g_poll_completes_after comment names UnboundedWaitBlocksInsteadOfPolling by identifier. It's stating a real present-tense invariant (only this one test sets the hatch) rather than narrating history, so it's defensible under comments.md, but it's borderline — not asking for a change.

Verdict

Approve. Both changes are exactly what they claim to be, verified against the actual code rather than the PR's own account of itself; the new test is non-vacuous by the author's own mutation check (which I re-derived rather than took on faith), and the dead-API removal is clean.

[ChaoWao]

Thanks — approving on a re-derivation rather than on my account of it is exactly the right check, and Consider #1 was worth asking. I chased both down; neither needs a code change, but #1 turned up something worth reporting.

Consider #1 — the numbers are right, but the reasoning behind the doubt was inverted

The premise was that #1741 / #1456 are later than this PR's base and "likely account for the +25 py tests". They are actually ancestors of 48dc78bf:

$ git log --oneline 48dc78bf | head -4
48dc78bf Refactor: centralize chip native run ownership in a C++ lane (#1650)
c5188699 Rename: simpler_setup.Tensor -> simpler_setup.TensorArg (#1741)
60ab6ab9 Add: support mixed local and remote L4 CommDomains (#1456)
c9e5f3cc Add: the W2 CPU-NPU comm endpoint model (#1696)

So this branch already contains them, and 1252 → 1277 happened before my base, not after. #1650's PR body reported 1252 because it was measured before those merged.

The two totals being unchanged from base is expected for a different reason each:

• ctest counts binaries, not gtest cases. My PR adds a case inside test_chip_run_lane, so ctest -LE requires_hardware stays 91 while the binary goes 12 → 13 (--gtest_list_tests). Both numbers are correct; they just measure different things.
• The Python total is identical because this PR touches zero .py files — git diff 48dc78bf...HEAD --name-only | grep '\.py$' is empty. I measured 1277 on the base as well rather than inferring it.

I've added both explanations to the PR body's Validation section, since the reporting was the actual weak point.

⚠️ The check did catch a real problem — in my process, not the diff

Re-running to answer this, the build-stamp guard fired:

ImportError: _task_interface was built from 48dc78bf87d0, but this source tree is at 4748fbe9dae0.

My extension was stale — stamped to the base, not my HEAD. It came from the mutation experiment: I rebuilt against a deliberately-broken lane to prove the new test fails, then restored the source but re-ran the Python suite without rebuilding.

The 1277 figure survives — I rebuilt and re-ran, same result, and it is structurally unaffected since this PR changes no Python. But the original figure was captured against a mismatched extension, so it was right by luck rather than by method. That is the same class of error as the stale test_scheduler binary on #1739 that let me report "73/73 green" over a red suite. Worth stating plainly rather than quietly fixing.

Consider #2 — leaving as-is

Agreed it is borderline. The comment names UnboundedWaitBlocksInsteadOfPolling because the hatch's safety depends on exactly one test setting it — a reader who adds a second caller needs to know that. It is a present-tense fact about current scope, not change narration, so it stays under comments.md.