Fix: mint owner_instance_id after fork; check device backing owner - #1744
Conversation
📝 WalkthroughWalkthrough
ChangesDevice import ownership
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HostWorker
participant ChipProcess
participant ImportRegistry
participant DeviceDescriptor
HostWorker->>HostWorker: regenerate owner_instance_id
HostWorker->>ChipProcess: pass owner_instance_id
ChipProcess->>ImportRegistry: configure device endpoint context
ImportRegistry->>DeviceDescriptor: validate backing owner
ImportRegistry-->>ChipProcess: materialize or reject descriptor
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
24ae7a4 to
128c7ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/simpler/buffer.py`:
- Around line 564-573: Replace DEVICE backing validation in buffer.py lines
564-573 with a serialized chip-specific ownership identifier, update the
ownership contract at buffer.py lines 546-548, and remove the chip-topology
claim at lines 584-585. In worker.py lines 2470-2475 configure each chip
endpoint with its own identity, and at line 7107 pass the identity for idx
rather than the shared Worker identity. Add a test in
tests/ut/py/test_worker/test_endpoint_capability.py lines 110-118 rejecting
materialization between sibling chip contexts that share a Worker identity but
have different chip identities.
🪄 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: 1b6b06af-c16d-432e-b1f2-cf08de20d014
📒 Files selected for processing (5)
python/simpler/buffer.pypython/simpler/worker.pytests/ut/py/test_buffer.pytests/ut/py/test_worker/test_endpoint_capability.pytests/ut/py/test_worker/test_host_worker.py
128c7ff to
bcb5178
Compare
Two gaps in the buffer identity / materialize path, both left open by hw-native-sys#1729. owner_instance_id was minted before the Worker it names existed: Worker.__init__ minted the nonce at Python object construction time -- before add_worker(), before any os.fork(), before init(). For a next-level child Worker that nonce was fixed in the parent process and only later copied (via fork COW) into the child that actually becomes its owner, so nothing tied the identity to a real fork having happened. Worker.init() now re-mints owner_instance_id right after its NEW -> INITIALIZING transition. A next-level child's init() runs only inside the process that was forked to host it (_start_hierarchical calls inner.init(...) only in the os.fork() child branch), so the nonce a Worker's buffers and endpoint identity actually use is never older than its own fork. __init__ keeps its mint as a fallback: several test files build a "ready" Worker by setting _lifecycle directly without ever calling init(), and still need a well-formed nonce. materialize() trusted a raw device pointer with no endpoint check: ImportRegistry.materialize()'s DEVICE_MALLOC/VMM_WINDOW branch decoded the descriptor body straight into a pointer and handed it back -- its own docstring said as much: "a DEVICE backing resolved here yields a device pointer, which is only meaningful on its owner chip. The endpoint x address_space matrix is a separate change; until it lands, that invariant rests on the caller." Submit-time checks already reject a device tensor before dispatch, but nothing stopped a caller that reaches materialize() directly. ImportContext(is_host_endpoint, owning_chip_instance_id) closes that: materialize() now refuses any DEVICE backing on a host endpoint outright, and on a device endpoint refuses one whose owner_instance_id doesn't match the nonce that endpoint was set up to serve. Wired into the three ImportRegistry() construction sites -- _sub_worker_loop (host), _run_chip_main_loop (device, given the parent's owner_instance_id at fork), and _run_l2_materialized (a device_ids-bearing L2 leaf, checked against its own nonce). This check is Worker-grained, not chip-grained: owner_instance_id is minted once per Worker incarnation, not once per chip, so a Worker with more than one entry in device_ids gives every one of its chip children the same nonce -- the wire BufferDescriptor has no field that distinguishes sibling chips (owner_worker_id is host-side-only free/copy provenance, never serialized). A DEVICE backing minted for chip 0 of such a Worker therefore also passes this check on sibling chip 1. It still rejects a different Worker's device buffer, and any host endpoint outright; the exact-chip half of the endpoint x address_space matrix for a multi-device Worker stays with the existing submit-time (target_worker_id, ptr) check in orchestrator.py, which this backstop complements rather than replaces. Closing that gap fully would need either a wire ABI field the frozen P1-A identity doesn't have, or a live per-chip pointer allowlist -- both bigger than a materialize-time backstop should take on; ImportContext's docstring states this bound explicitly. Added test_host_endpoint_materialize_refuses_a_device_tensor_directly and test_chip_materialization_refuses_a_foreign_chips_device_tensor, mirroring hw-native-sys#1729's existing sub-worker analog but exercising materialize() itself (bypassing submit) for both rows of the endpoint matrix. Updated three existing test_buffer.py cases and two test_host_worker.py harness call sites to pass ImportContext / the new owner_instance_id parameter. Verified: pytest tests/ut 1279 passed / 13 skipped / 0 failed; ruff check/format and pyright clean; a real a2a3 onboard run (2 devices) across host_build_graph and tensormap_and_ringbuffer examples exercised the touched chip-fork path with no failures.
Review — #1744 (G4 + ImportContext, the last two steps of the capability-judgment chain)This is CI triage (2 failures, both confirmed infra — not this diff)Pulled the actual logs, not guessed:
Both on the same Code verified against the PR body's claims
Minor noteBranch is one commit behind current VerdictNo Must-fix, no Should-fix beyond what's already in the PR body. Once the two |
Two gaps in the buffer identity / materialize path, both left open by #1729
owner_instance_idwas minted before the Worker it names existedWorker.__init__minted the nonce at Python object construction time — beforeadd_worker(), before anyos.fork(), beforeinit(). For a next-level childWorker that nonce is fixed in the parent process and only later copied (via
fork COW) into the child that actually becomes its owner, so nothing tied the
identity to a real fork having happened.
Worker.init()now re-mintsowner_instance_idright after itsNEW -> INITIALIZINGtransition. A next-level child'sinit()runs onlyinside the process that was forked to host it (
_start_hierarchicalcallsinner.init(...)only in theos.fork()child branch), so the nonce aWorker's buffers and endpoint identity actually use is never older than its
own fork.
__init__keeps its mint as a fallback: several test files build a"ready" Worker by setting
_lifecycledirectly without ever callinginit(),and still need a well-formed nonce.
materialize()trusted a raw device pointer with no endpoint checkImportRegistry.materialize()'sDEVICE_MALLOC/VMM_WINDOWbranch decodedthe descriptor body straight into a pointer and handed it back — its own
docstring said as much: "a DEVICE backing resolved here yields a device
pointer, which is only meaningful on its owner chip. The endpoint x
address_space matrix is a separate change; until it lands, that invariant
rests on the caller." Submit-time checks already reject a device tensor
before dispatch, but nothing stopped a caller that reaches
materialize()directly.
ImportContext(is_host_endpoint, owning_chip_instance_id)closes that:materialize()now refuses any DEVICE backing on a host endpoint outright,and on a device endpoint refuses one whose
owner_instance_iddoesn't matchthe nonce that endpoint was set up to serve. Wired into the three
ImportRegistry()construction sites —_sub_worker_loop(host),_run_chip_main_loop(device, given the parent'sowner_instance_idatfork), and
_run_l2_materialized(adevice_ids-bearing L2 leaf, checkedagainst its own nonce).
Known bound (flagged by review, documented rather than closed here): this
check is Worker-grained, not chip-grained.
owner_instance_idis minted onceper Worker incarnation, not once per chip, so a Worker with more than one
entry in
device_idsgives every one of its chip children the same nonce —the wire
BufferDescriptorhas no field that distinguishes sibling chips(
owner_worker_idis host-side-only free/copy provenance, never serialized).A DEVICE backing minted for chip 0 of such a Worker therefore also passes
this check on sibling chip 1. It still rejects a different Worker's device
buffer, and any host endpoint outright; the exact-chip half of the endpoint x
address_spacematrix for a multi-device Worker stays with the existingsubmit-time
(target_worker_id, ptr)check inorchestrator.py, which thisbackstop complements rather than replaces. Closing that gap fully needs
either a wire ABI field the frozen P1-A identity doesn't have, or a live
per-chip pointer allowlist — both bigger than a materialize-time backstop
should take on.
ImportContext's docstring now states this bound explicitly.Test plan
pytest tests/ut— 1279 passed, 13 skipped, 0 failedruff check/ruff format --check,pyright— cleanmaterialize()directly (bypassing submit),mirroring Update: cut task args over to the self-describing Tensor wire ABI #1729's existing sub-worker test: a host endpoint rejects a
DEVICE descriptor, and a device endpoint rejects one minted by a
different chip's owner
host_build_graphandtensormap_and_ringbufferexamples — exercises the touched chip-forkpath, no failures