Add: return a live handle from the direct-chip submit - #1748
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughL2 submissions now return live asynchronous ChangesDirect Chip-Run Lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant L2Worker
participant ChipWorker
participant RunLane
participant RunHandle
L2Worker->>ChipWorker: _submit_chip_run_direct(callable_id, args, config)
ChipWorker->>RunLane: submit direct chip run
RunLane-->>ChipWorker: return live ChipRun
ChipWorker-->>L2Worker: return ChipRun
L2Worker-->>RunHandle: store run ID and arguments
RunHandle->>ChipRun: wait(timeout)
ChipRun-->>RunHandle: return completion state or error
L2Worker->>RunLane: close lane
L2Worker->>ChipWorker: finalize
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@python/bindings/task_interface.cpp`:
- Around line 2027-2033: Validate timeout in the ChipRun wait lambda before
duration_cast: preserve the negative-timeout behavior, but reject NaN,
infinities, and finite values outside ChipRun::Clock::duration’s representable
range. Add explicit tests covering float("nan"), float("inf"), and a very large
finite timeout, verifying each is rejected without invoking the conversion.
In `@python/simpler/worker.py`:
- Around line 9917-9925: Update the direct-run flow around
_submit_chip_run_direct() to reserve the run ID and publish a pending _chip_runs
entry containing the argument keepalive before native submission. Replace the
pending entry with the returned ChipRun after submission succeeds; if submission
fails before admission, remove only that pending entry, while preserving
tracking and keepalive after native admission even if later operations raise.
In `@tests/ut/py/test_worker/test_startup_readiness.py`:
- Around line 823-826: Update this test’s submitted fake run setup so
_FakeChipRun.wait() raises boom, causing w.submit(handle).wait() to propagate
the error. Assert the RunHandle.wait() call raises before checking w._chip_runs
and invoking w.close(), while preserving the existing cleanup assertions.
🪄 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: 79f1ce8b-8fe9-4acc-9b74-60d1100eb96c
📒 Files selected for processing (6)
python/bindings/task_interface.cpppython/simpler/worker.pysrc/common/worker/chip_worker.cppsrc/common/worker/chip_worker.htests/ut/py/test_worker/_harness.pytests/ut/py/test_worker/test_startup_readiness.py
`Worker.submit()` on an L2 worker ran the kernel to completion and returned `RunHandle._completed(...)`, a handle that was terminal before the caller ever saw it. Every other level already returned a handle whose run was still in flight, so the direct-chip path was the one place where `submit` and `run` meant the same thing. The lane already had the shape this needs. `ChipRunLane`'s unleased `submit` overload admits at capacity one — it drains the current front before taking the next run — and returns a `ChipRun` without waiting; `ChipWorker::run` is exactly that call composed with `ChipRun::wait_until`. So the change is to stop composing the two behind the caller's back: the Python direct path submits, and `RunHandle.wait()` owns the completion fence. Admission is unchanged. A second submit still blocks until the first run is drained, because that is where the lane's capacity-one rule lives. What moves is only when the first submit returns, and successor policy is deliberately not part of this change. Because a live handle can now outlive the call that made it, two ownership questions get explicit answers: - Cleanup. A direct-chip run owns no orchestration state, and the lane finalizes its native run as part of reaching terminal, so a chip-backed handle retires its lane entry instead of entering the run-finalization cursor. Driving those steps for it would report a cleanup failure for state that was never built. - Close. `close()` now drains the lane while the device is still up, so a handle the caller dropped is settled somewhere its failure is reported rather than in the lane destructor, which swallows exceptions. The lane rethrows its poison on close, so close re-raises only when a run's error has not already been delivered: waiting on a handle raises that error and retires the entry, and re-raising it again would turn a run failure the caller handled into an unhandled close failure. The onboard fault-injection scene tests are exactly that shape — they assert `run()` raises, then close in a `finally`. `run()` keeps its meaning as `submit(...).wait()` and remains the blocking composition of the same lane entry point, so the direct path still has one admission authority rather than a bypass beside it. The new `_ChipRun.wait` binding converts a `double` timeout to the steady clock's integral representation, which is undefined for NaN, an infinity, or a finite value past that range. It rejects NaN by name, since no ordering comparison catches it, and treats a timeout beyond the clock's reach as unbounded — a caller asking to wait longer than the clock can express means effectively forever, and the unbounded path is the one that blocks on the device instead of polling. `RunHandle._deadline` already rejects non-finite timeouts, so this is defence in depth on a private surface rather than a reachable fix. Tests: the L2 submit test now pins a live handle rather than a completed one; `run == submit().wait()` is asserted through the fake's submission record; close is covered in both directions — an unwaited handle is drained, and a lane failure is reported only when no wait already delivered it, with the delivered case driving a failing run so the error reaches the caller through `wait()`; and the outer non-finite timeout gate is pinned. The fake chip impl grows a `_ChipRun` stand-in that can fail, so the harness models the live-run contract instead of absorbing the call. Verification: 1286 Python UT, 91/91 C++ UT (`ctest -LE requires_hardware`), pyright clean, a2a3 onboard 56 passed + 24 passed/2 skipped resource phase, a2a3sim 46 passed/1 skipped — all matching the pre-change baseline.
ChaoWao
force-pushed
the
w1b-live-direct-chip-handle
branch
from
August 9, 2026 01:28
5e37710 to
97578b5
Compare
Collaborator
Author
|
@coderabbitai Addressed all three inline findings — two fixed, one skipped with reasoning, each answered on its own thread.
Also fixed the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Changed files =='
git diff --name-only HEAD^ HEAD || true
printf '%s\n' '== Public API and worker structure =='
ast-grep outline python/simpler/worker.py --items all --type class,function --match 'RunHandle|Worker|submit|wait|close|_deadline' || true
ast-grep outline python/bindings/task_interface.cpp --items all --type class,function --match 'ChipRun|wait|ChipWorker|submit' || true
printf '%s\n' '== Focused implementation references =='
rg -n -C 8 --glob 'python/simpler/worker.py' \
'def _deadline\b|def submit\b|def wait\b|def close\b|_submit_chip_run_direct|chip_run|ChipRunLane|finalize' .
rg -n -C 8 --glob 'python/bindings/task_interface.cpp' \
'ChipRun|\.def\("wait"|wait_until|isnan|NaN|timeout' .
printf '%s\n' '== Focused tests and fake behavior =='
rg -n -C 8 --glob 'tests/ut/py/test_worker/_harness.py' --glob 'tests/ut/py/test_worker/test_startup_readiness.py' \
'_FakeChipRun|def wait\b|non.finite|NaN|close\b|undelivered|rethrow|raises|submit\b' .Length of output: 218318 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Worker.submit() and L2 submission flow =='
sed -n '9660,9930p' python/simpler/worker.py
printf '%s\n' '== L2 handle finalization and close flow =='
sed -n '9980,10040p' python/simpler/worker.py
sed -n '10735,10775p' python/simpler/worker.py
printf '%s\n' '== Changed Python worker hunk =='
git diff HEAD^ HEAD -- python/simpler/worker.py
printf '%s\n' '== Changed focused tests and fake =='
git diff HEAD^ HEAD -- tests/ut/py/test_worker/_harness.py tests/ut/py/test_worker/test_startup_readiness.pyLength of output: 37558
One new documentation issue remains: Line 9671 in ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
5 tasks
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Aug 10, 2026
My own review comment on hw-native-sys#1751 flagged a real gap: release_buffer()'s in-flight check only ever looks at self._accepted_run_handles, and _submit_l2_locked (worker.py, the direct-chip dispatch path submit() uses at L2) never adds its RunHandle there. create_buffer() only requires level >= 2, so an L2 Worker can hold a real, registered Buffer and dispatch chip runs against it with zero protection from release_buffer(). Checked how exploitable this is today: zero production callers do Worker(level=2).create_buffer() then race release_buffer() against a concurrent submit() -- this is a latent trap for future usage, not a firing bug. It doesn't explode today only because ImportRegistry.materialize() maps its own separate mmap for the identity, and POSIX unlink() only removes the shm's name -- an already-open mapping keeps working. That's an undocumented, untested coincidence, not a guarantee. Also corrected the assumption hw-native-sys#1751 shipped -- "L2 never needs this, a run completes synchronously inside submit()" -- which is false: the direct-chip lane permits one active plus one prepared compatible run, so up to two L2 runs can be in flight at once (per the W1b/W1c async-pipeline work, hw-native-sys#1748/hw-native-sys#1750). The real reason release_buffer()'s existing check can't see L2 runs is narrower: L2 uses a separate run-id namespace (self._chip_run_seq, tracked in self._chip_runs) and never touches _accepted_run_handles/_submit_mu -- _submit_locked returns from the L2 branch before reaching the with self._submit_mu: block. Rather than fold L2 into _accepted_run_handles (whose other readers -- _cleanup_bearing_predecessor, the live-handle scan for direct-control ordering, whole-run FIFO teardown draining -- carry L3-specific assumptions built around _orch-issued run ids and orchestration callbacks), this mirrors _chip_runs' own lifecycle with a parallel dict, _chip_run_touched_identities, added/removed at the same two points (_submit_l2_locked, _finalize_run_handle's L2 branch, and Worker.close()'s teardown) under the existing _registry_lock. release_buffer() now runs a second, independent check against it after the existing L3+ one. Extracted _identities_in_args as a shared static helper so _record_touched_identities (L3+) and the new L2 code walk tensor args the same way instead of duplicating the loop. New tests (tests/ut/py/test_worker/test_release_buffer.py): _submit_l2_locked records the touched identity (and an empty set for args=None); _finalize_run_handle clears it; release_buffer rejects while an L2 run's identity is present (registry entry survives the rejection) and succeeds once it's gone. Fixed two existing bare-Worker test helpers (test_create_buffer.py, test_remote_l3_lifecycle.py) that construct a Worker via __new__ and manually set internals -- they now also set the new dict. Verified: pytest tests/ut 1302 passed / 13 skipped / 0 failed; ruff check/format clean; a real a2a3 onboard run (pipeline_slots/test_pipeline_slots.py, an L2 direct-chip scene test) exercising the touched-identity walk on the real L2 dispatch path.
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Aug 10, 2026
My own review comment on hw-native-sys#1751 flagged a real gap: release_buffer()'s in-flight check only ever looks at self._accepted_run_handles, and _submit_l2_locked (worker.py, the direct-chip dispatch path submit() uses at L2) never adds its RunHandle there. create_buffer() only requires level >= 2, so an L2 Worker can hold a real, registered Buffer and dispatch chip runs against it with zero protection from release_buffer(). Checked how exploitable this is today: zero production callers do Worker(level=2).create_buffer() then race release_buffer() against a concurrent submit() -- this is a latent trap for future usage, not a firing bug. It doesn't explode today only because ImportRegistry.materialize() maps its own separate mmap for the identity, and POSIX unlink() only removes the shm's name -- an already-open mapping keeps working. That's an undocumented, untested coincidence, not a guarantee. Also corrected the assumption hw-native-sys#1751 shipped -- "L2 never needs this, a run completes synchronously inside submit()" -- which is false: the direct-chip lane permits one active plus one prepared compatible run, so up to two L2 runs can be in flight at once (per the W1b/W1c async-pipeline work, hw-native-sys#1748/hw-native-sys#1750). The real reason release_buffer()'s existing check can't see L2 runs is narrower: L2 uses a separate run-id namespace (self._chip_run_seq, tracked in self._chip_runs) and never touches _accepted_run_handles/_submit_mu -- _submit_locked returns from the L2 branch before reaching the with self._submit_mu: block. Rather than fold L2 into _accepted_run_handles (whose other readers -- _cleanup_bearing_predecessor, the live-handle scan for direct-control ordering, whole-run FIFO teardown draining -- carry L3-specific assumptions built around _orch-issued run ids and orchestration callbacks), this mirrors _chip_runs' own lifecycle with a parallel dict, _chip_run_touched_identities, added/removed at the same two points (_submit_l2_locked, _finalize_run_handle's L2 branch, and Worker.close()'s teardown) under the existing _registry_lock. release_buffer() now runs a second, independent check against it after the existing L3+ one. Extracted _identities_in_args as a shared static helper so _record_touched_identities (L3+) and the new L2 code walk tensor args the same way instead of duplicating the loop. _submit_l2_locked publishes the touched-identities entry BEFORE calling _submit_chip_run_direct, not after: writing it post-dispatch left a window where a concurrent release_buffer() could see no entry at all for a run already running on the chip, since the entry that would have blocked it didn't exist yet. On a dispatch failure the entry is popped back out. _finalize_run_handle's L2 branch and Worker.close()'s teardown now take _registry_lock around every read/clear of _chip_runs and _chip_run_touched_identities (previously the membership check in _finalize_run_handle and the two clears in close() ran unlocked), using pop(..., None) so a concurrent close() can never make either raise. New tests (tests/ut/py/test_worker/test_release_buffer.py): _submit_l2_locked records the touched identity (and an empty set for args=None); _finalize_run_handle clears it; release_buffer rejects while an L2 run's identity is present (registry entry survives the rejection) and succeeds once it's gone; touched identities are visible to release_buffer() while dispatch is still blocked mid-call, proving the publish-before-dispatch ordering. Fixed two existing bare-Worker test helpers (test_create_buffer.py, test_remote_l3_lifecycle.py) that construct a Worker via __new__ and manually set internals -- they now also set the new dict. Verified: pytest tests/ut full suite passed; ruff check/format clean; pyright clean; a real a2a3 onboard run (pipeline_slots/test_pipeline_slots.py, an L2 direct-chip scene test) exercising the touched-identity walk on the real L2 dispatch path.
ChaoWao
added a commit
that referenced
this pull request
Aug 10, 2026
…#1757) My own review comment on #1751 flagged a real gap: release_buffer()'s in-flight check only ever looks at self._accepted_run_handles, and _submit_l2_locked (worker.py, the direct-chip dispatch path submit() uses at L2) never adds its RunHandle there. create_buffer() only requires level >= 2, so an L2 Worker can hold a real, registered Buffer and dispatch chip runs against it with zero protection from release_buffer(). Checked how exploitable this is today: zero production callers do Worker(level=2).create_buffer() then race release_buffer() against a concurrent submit() -- this is a latent trap for future usage, not a firing bug. It doesn't explode today only because ImportRegistry.materialize() maps its own separate mmap for the identity, and POSIX unlink() only removes the shm's name -- an already-open mapping keeps working. That's an undocumented, untested coincidence, not a guarantee. Also corrected the assumption #1751 shipped -- "L2 never needs this, a run completes synchronously inside submit()" -- which is false: the direct-chip lane permits one active plus one prepared compatible run, so up to two L2 runs can be in flight at once (per the W1b/W1c async-pipeline work, #1748/#1750). The real reason release_buffer()'s existing check can't see L2 runs is narrower: L2 uses a separate run-id namespace (self._chip_run_seq, tracked in self._chip_runs) and never touches _accepted_run_handles/_submit_mu -- _submit_locked returns from the L2 branch before reaching the with self._submit_mu: block. Rather than fold L2 into _accepted_run_handles (whose other readers -- _cleanup_bearing_predecessor, the live-handle scan for direct-control ordering, whole-run FIFO teardown draining -- carry L3-specific assumptions built around _orch-issued run ids and orchestration callbacks), this mirrors _chip_runs' own lifecycle with a parallel dict, _chip_run_touched_identities, added/removed at the same two points (_submit_l2_locked, _finalize_run_handle's L2 branch, and Worker.close()'s teardown) under the existing _registry_lock. release_buffer() now runs a second, independent check against it after the existing L3+ one. Extracted _identities_in_args as a shared static helper so _record_touched_identities (L3+) and the new L2 code walk tensor args the same way instead of duplicating the loop. _submit_l2_locked publishes the touched-identities entry BEFORE calling _submit_chip_run_direct, not after: writing it post-dispatch left a window where a concurrent release_buffer() could see no entry at all for a run already running on the chip, since the entry that would have blocked it didn't exist yet. On a dispatch failure the entry is popped back out. _finalize_run_handle's L2 branch and Worker.close()'s teardown now take _registry_lock around every read/clear of _chip_runs and _chip_run_touched_identities (previously the membership check in _finalize_run_handle and the two clears in close() ran unlocked), using pop(..., None) so a concurrent close() can never make either raise. New tests (tests/ut/py/test_worker/test_release_buffer.py): _submit_l2_locked records the touched identity (and an empty set for args=None); _finalize_run_handle clears it; release_buffer rejects while an L2 run's identity is present (registry entry survives the rejection) and succeeds once it's gone; touched identities are visible to release_buffer() while dispatch is still blocked mid-call, proving the publish-before-dispatch ordering. Fixed two existing bare-Worker test helpers (test_create_buffer.py, test_remote_l3_lifecycle.py) that construct a Worker via __new__ and manually set internals -- they now also set the new dict. Verified: pytest tests/ut full suite passed; ruff check/format clean; pyright clean; a real a2a3 onboard run (pipeline_slots/test_pipeline_slots.py, an L2 direct-chip scene test) exercising the touched-identity walk on the real L2 dispatch path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Worker.submit()on an L2 worker ran the kernel to completion and returnedRunHandle._completed(...)— a handle that was terminal before the caller ever saw it. Every other level already returned a handle whose run was still in flight, so the direct-chip path was the one place wheresubmitandrunmeant the same thing. This is W1b of the worker async-pipeline track.The lane already had the shape this needs.
ChipRunLane's unleasedsubmitoverload admits at capacity one — it drains the current front before taking the next run — and returns aChipRunwithout waiting;ChipWorker::runis exactly that call composed withChipRun::wait_until. So the change is to stop composing the two behind the caller's back: the Python direct path submits, andRunHandle.wait()owns the completion fence.Admission is unchanged. A second submit still blocks until the first run is drained, because that is where the lane's capacity-one rule lives. What moves is only when the first submit returns. Successor policy is deliberately not part of this change — that is W1c.
Ownership questions a live handle forces
Because the handle can now outlive the call that made it, two things get explicit answers rather than inherited ones:
close()now drains the lane while the device is still up, so a handle the caller dropped is settled where its failure is reported rather than in the lane destructor, which swallows exceptions. The lane rethrows its poison on close, so close re-raises only when a run's error has not already been delivered: waiting on a handle raises that error and retires the entry, and re-raising it again would turn a run failure the caller handled into an unhandled close failure.That last point is not hypothetical — it is a real defect this PR found and fixed. The onboard fault-injection scene tests are exactly that shape (assert
run()raises, thenclose()in afinally), and an unconditional rethrow failed 12 of them on hardware. Both directions now have a regression test.run()keeps its meaning assubmit(...).wait()and remains the blocking composition of the same lane entry point, so the direct path still has one admission authority rather than a bypass beside it.Testing
tests/ut/py)ctest -LE requires_hardware, CI's filter)task-submit, device lock held)All match the pre-change baseline on
main. The onboard and sim runs were repeated after the rebase onto24d8e8fc, since the base moved (#1747 merged) while this was in progress.New coverage, each verified to fail against the old behavior before being kept:
test_l2_submit_returns_live_handle— the handle is backed by a live lane run, and waiting retires ittest_l2_run_equals_submit_then_wait— both forms admit through the same lane entry pointtest_l2_unwaited_handle_is_drained_by_close— pins that the drain happens before device finalizetest_l2_close_reports_lane_failure_only_when_undelivered— both directions of the close-rethrow ruleNote on one observed flake
TestNextLevelStartupFailure::test_second_child_failure_reaps_firstfailed once in six full-suite runs on this branch and passed 4/4 afterwards; it passes 3/3 onmain. It is an L4→L3 fork/reap test with no chip worker and no L2, so it cannot reach this change's code path — but six runs is not enough to call it pre-existing, so I am flagging it rather than claiming it is unrelated.