Fix: expose host buffers to L3 subworkers - #1390
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughHost-buffer zero-copy support now maps buffers into local L3 children, rewrites task host pointers there, and broadcasts registration changes to both ChangesLocal L3 host-buffer flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant L3Children
participant SubWorker
participant Task
Worker->>L3Children: create_host_buffer control
L3Children->>SubWorker: Map shared-memory buffer
Task->>SubWorker: Ready task with host pointer
SubWorker->>Task: Rewrite pointer to mapped child address
Worker->>L3Children: free_host_buffer control
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the host buffer registration mechanism to support broadcasting control commands (such as mapping and unmapping host buffers) to both next-level and sub-workers (local L3 children). It updates the sub-worker loop to handle these control commands, rewrite host addresses in task arguments, and clean up shared memory on shutdown. Additionally, the documentation has been updated to reflect these changes, and new unit tests have been added to verify the behavior. There are no review comments to address, and the changes look solid.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/simpler/worker.py (1)
963-966: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog exception instead of silently ignoring it.
Silently swallowing exceptions during shutdown reduces observability and can make debugging resource leaks difficult. Based on learnings, treat this as a diagnostic-consistency best practice. Consider logging the exception to
sys.stderrinstead of usingpass.💡 Proposed refactor to log the exception
for host_shm, _lo, _hi, _base in host_buf_table.values(): try: host_shm.close() - except Exception: # noqa: BLE001 - pass + except Exception as e: # noqa: BLE001 + import sys + sys.stderr.write(f"sub_worker: failed to close host_shm: {e}\n")🤖 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 `@python/simpler/worker.py` around lines 963 - 966, Update the host_shm.close cleanup in the worker shutdown path to log caught exceptions to sys.stderr instead of silently executing pass. Preserve the existing broad exception handling and continue shutdown after logging the failure.Source: Learnings
🤖 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/comm-domain.md`:
- Around line 153-159: Update the explanation in the “Two sources are legal”
section of docs/comm-domain.md to state that fork-inherited mappings retain
their virtual addresses, while post-fork attachment may map the memory at a
different address and therefore requires pointer rewriting. Preserve the
existing born-shared and worker-allocation requirements.
In `@tests/ut/py/test_worker/test_host_buffer_registration.py`:
- Around line 130-137: Update the assertions in the host-buffer registration
test around create_host_buffer to verify the complete broadcast records,
including each map and unmap command value, rather than only comparing
WorkerType. Ensure both initial registration calls and cleanup calls assert the
expected command values.
---
Nitpick comments:
In `@python/simpler/worker.py`:
- Around line 963-966: Update the host_shm.close cleanup in the worker shutdown
path to log caught exceptions to sys.stderr instead of silently executing pass.
Preserve the existing broad exception handling and continue shutdown after
logging the failure.
🪄 Autofix (Beta)
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
Run ID: 9fdc5495-b1c8-439e-b2a5-c1b9ac3c0195
📒 Files selected for processing (3)
docs/comm-domain.mdpython/simpler/worker.pytests/ut/py/test_worker/test_host_buffer_registration.py
7144dfc to
98c3dab
Compare
98c3dab to
c748e48
Compare
- Broadcast host-buffer map and unmap controls to subworkers - Rewrite parent host addresses before SubWorker task decoding - Add focused regression coverage for post-fork visibility
2 participants
Summary
Testing
pytest tests/ut -m "not requires_hardware" -q— 575 passed, 13 skipped, 10 deselectedruff checkandruff format --checkpyright— 0 errorsmarkdownlint-cli2— 0 errorsHardware tests were not run because this change is limited to the Python host control path.