Retire the blocking endpoint channel and tighten the mailbox contract

[ChaoWao]

Follow-ups to the review on #1728, which merged as 9a03d92c. All five items addressed.

② Blocking channel deleted (was: "delete or explain")

Once both endpoint kinds became progress-driven, the blocking channel lost its
last production caller. Deleted end to end:

• WorkerEndpoint::run (pure virtual) and run_with_accept off the interface
• LocalMailboxEndpoint::run / run_with_accept, RemoteL3Endpoint::run
• WorkerThread::dispatch_process and the non-progress half of WorkerThread::loop
• the Python base-frame _TASK_READY branch
• progressable() — with no blocking path left, its false case selects nothing

The review's survey was one caller short. It listed only C++ UTs, but
test_l3_sub_worker_maps_rewrites_and_unmaps_host_buffer scripts _TASK_READY
on the base frame, so deleting the Python branch broke it. That test is
rescripted onto the task frame.

Tests reaching the channel covered invariants that outlive it, so they move to
the progress path rather than being dropped:

Test	Disposition
LocalMailboxPublishesAcceptanceBeforeCompletion	Ported. Still pins that acceptance is a distinct event ahead of completion.
AcceptanceIsReadableAfterTaskDone	Ported. Still pins that the ACK survives a TASK_DONE landing with no poll in between.
RemoteTaskErrorMapsToTaskFailure	Ported; poll_progress reproduces the TASK_FAILURE mapping unchanged.
BareHostPointerWithoutSidecarIsRejectedAtSubmission	Changes shape honestly — encoding happens while publishing, so an unsidecar'd bare pointer is now rejected at submit_progress rather than reported as a completion. Renamed to match, and it asserts the message since submit_progress has several other runtime_error paths.
SuccessCompletionMapsToSuccess	Dropped as redundant; TaskDispatchUsesProgressSubmissionAndPolling already covers it. Its last_frame assertion moved there.
FakeEndpoint	Now progress-driven, queueing the completion its next poll reports — which preserves the completion-time acceptance run_with_accept used to give it.

① Guard removed, fixture fixed

Verified both halves of the claim before acting: all three production mailboxes
are MAILBOX_SIZE, and CPython's SharedMemory reports the exact requested
length (size=64 → len(buf) == 64), so a one-frame buffer is not page-rounded
back into range. The fixture now allocates a whole mailbox, and the guard plus
both task_buf is not None checks are gone — buf is a whole mailbox by
construction. A short buffer now fails fast at _buffer_field_addr with a clear
ValueError during setup rather than producing a garbage address.

③ Invariant stated at the ternary

frame_index reads as if slot_id were a frame number. A present-tense comment
now states it is a native pipeline slot bounded by the runtime
PipelineContract, mapped 1:1 by two-frame endpoints and pinned to frame 0 by
single-frame ones.

④ Fallback removed — and verified non-vacuous

ProgressStopReleasesWaitingControl polled progress when the control future was
not ready, so it passed whether or not request_progress_stop woke the waiter —
the one thing it exists to prove. Fallback removed; verified by mutation:
with command_cv_.notify_all() deleted the test hangs, and with it restored it
passes in 50 ms.

⑤ Stale comments

Three words per iteration, not two; the admission-fence handler takes the task
buffer.

Stale references the removal left behind

Per doc-consistency.md §1, grepping the deleted names surfaced references my
change made wrong — docs/worker-manager.md (blocking-dispatch wording, ×3),
docs/task-flow.md, python/bindings/worker_bind.h, python/simpler/worker.py,
test_error_propagation.py, and the four build_task_payload throws that still
named RemoteL3Endpoint::run. All updated to describe the paths that now exist.

Validation

• 73/73 cpp UT (ctest -L no_hardware)
• 1189 passed / 13 skipped — full tests/ut/py
• a2a3 onboard sweep: 54 passed, plus 24 passed / 2 skipped in the resource phase; quarantined sdma lane 1 passed
• a2a3sim sweep: 45 passed, plus 21 passed / 4 skipped

One open thread

CodeRabbit suggests a bounded cv_ wait in WorkerThread::loop() so the
in-flight branch does not spin a core. Declined and left unresolved for a human
call: codestyle.md rule 5 forbids backoff timers on the dispatch path, nothing
notifies cv_ on child completion (so a wait_for would be a pure timer that
adds latency to every task), and the spin is pre-existing — this PR only deleted
the other loop branch. A real wakeup primitive would be the rule-compliant fix
and deserves its own PR.

[coderabbitai]

📝 Walkthrough

Walkthrough

The worker execution path now uses progress-based submission and polling. Synchronous endpoint execution and dispatch_process were removed. Mailbox handling requires a dedicated task frame, and tests now validate progress events, errors, shutdown, and frame separation.

Changes

Progress-driven worker execution

Layer / File(s)	Summary
Progress endpoint contract src/common/hierarchical/remote_endpoint.h, src/common/hierarchical/worker_manager.h	Removed blocking endpoint methods, progressable(), and WorkerThread::dispatch_process.
Unified worker progress loop src/common/hierarchical/worker_manager.cpp	Changed WorkerThread::loop to use submission, activation, polling, shutdown handling, and progress completion events.
Mailbox frame contract python/simpler/worker.py, docs/..., python/bindings/worker_bind.h	The mailbox loop now uses frame 1 for task handling. Documentation describes progress submission, task-frame publication, and shutdown behavior.
Progress API and mailbox validation tests/ut/cpp/hierarchical/test_*.cpp, tests/ut/py/test_worker/*	Updated tests to validate progress events, remote errors, acceptance ordering, shutdown behavior, and separate base and task frames.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WorkerThread
  participant WorkerEndpoint
  participant Mailbox
  WorkerThread->>WorkerEndpoint: submit_progress dispatch
  WorkerEndpoint->>Mailbox: publish task frame
  WorkerThread->>WorkerEndpoint: activate_progress
  WorkerThread->>WorkerEndpoint: poll_progress
  Mailbox-->>WorkerEndpoint: acceptance or completion event
  WorkerEndpoint-->>WorkerThread: progress result

Loading

Possibly related PRs

• hw-native-sys/simpler#1728: Both changes modify worker endpoint progress APIs, mailbox handling, and related tests.
• hw-native-sys/simpler#1541: This change replaces the acceptance-based dispatch flow with progress submission and polling.
• hw-native-sys/simpler#1663: Both changes modify worker mailbox shutdown and progress-dispatch behavior.

Poem

A rabbit submits a task with care,
Then polls for progress in the air.
The mailbox frames align,
Shutdown signals repeat in time,
And completion hops back there.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 29.63% 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 main changes: removing the blocking endpoint channel and tightening the mailbox contract.
Description check	✅ Passed	The description directly explains the blocking channel removal, mailbox contract updates, test migrations, stale-reference cleanup, and validation results.

---

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.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

tests/ut/cpp/hierarchical/test_remote_endpoint.cpp (1)

798-800: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename the test to match the new behavior.

The assertion now checks a synchronous throw from submit_progress. The test name BareHostPointerWithoutSidecarIsEndpointFailure still describes an endpoint-failure completion, which this test no longer exercises.

♻️ Proposed rename

-TEST(RemoteEndpoint, BareHostPointerWithoutSidecarIsEndpointFailure) {
+TEST(RemoteEndpoint, BareHostPointerWithoutSidecarRejectedAtSubmission) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ut/cpp/hierarchical/test_remote_endpoint.cpp` around lines 798 - 800,
Rename the test containing the submit_progress EXPECT_THROW assertion from
BareHostPointerWithoutSidecarIsEndpointFailure to a name describing the
synchronous submission-time rejection, rather than an endpoint-failure
completion.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/worker-manager.md`:
- Around line 147-148: Update section 3.3 in the worker-manager documentation to
remove the obsolete “blocking chip dispatch” wording and describe only the
progress-driven dispatch path, keeping the surrounding sticky-marker behavior
accurate.

In `@src/common/hierarchical/worker_manager.cpp`:
- Around line 496-506: The WorkerThread::loop() in-flight path currently spins
while poll_progress remains non-blocking. Add a short bounded cv_ wait when
inflight_ is nonzero, using the existing mutex and condition variable, while
preserving immediate processing for queued work and shutdown handling.

In `@tests/ut/py/test_worker/test_host_buffer_registration.py`:
- Line 250: Move task_frame.release() into the test’s existing finally block so
it executes on both success and assertion-failure paths, alongside
mailbox_buf.release(). Keep the normal assertions and cleanup behavior otherwise
unchanged.

---

Nitpick comments:
In `@tests/ut/cpp/hierarchical/test_remote_endpoint.cpp`:
- Around line 798-800: Rename the test containing the submit_progress
EXPECT_THROW assertion from BareHostPointerWithoutSidecarIsEndpointFailure to a
name describing the synchronous submission-time rejection, rather than an
endpoint-failure completion.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8e70b010-cc78-46fd-b299-93ca6b553bb8

📥 Commits

Reviewing files that changed from the base of the PR and between 9a03d92 and 6da2830.

📒 Files selected for processing (13)

• docs/task-flow.md
• docs/worker-manager.md
• python/bindings/worker_bind.h
• python/simpler/worker.py
• src/common/hierarchical/remote_endpoint.cpp
• src/common/hierarchical/remote_endpoint.h
• src/common/hierarchical/worker_manager.cpp
• src/common/hierarchical/worker_manager.h
• tests/ut/cpp/hierarchical/test_remote_endpoint.cpp
• tests/ut/cpp/hierarchical/test_scheduler.cpp
• tests/ut/py/test_worker/test_admission_fence.py
• tests/ut/py/test_worker/test_error_propagation.py
• tests/ut/py/test_worker/test_host_buffer_registration.py

💤 Files with no reviewable changes (2)

• src/common/hierarchical/remote_endpoint.cpp
• src/common/hierarchical/remote_endpoint.h