Repository navigation
[Bugfix] Lock shared memory before opening segments - #9285
Conversation
Signed-off-by: Ranran Haoran Zhang <ranranhaoranzhang@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/input_output_modality_contracts.md, docs/design/module/observability.md, docs/design/module/omni_connector.md. Module owners: @princepride @alex-jw-brooks @amy-why-3459 Routing: @princepride via module of the changed files, CODEOWNERS; @alex-jw-brooks via module of the changed files; @amy-why-3459 via module of the changed files @WindChimeRan, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
linyueqian
left a comment
There was a problem hiding this comment.
Thanks for splitting this out. Taking the publication lock before opening the segment closes the race at its source: the writer creates and sizes the segment only while it holds the lock, so a key-based reader now either gets BlockingIOError while the writer is mid-publish or FileNotFoundError before anything exists, and both return None for a retry instead of opening a zero-sized segment. The new test is a good regression: it pauses the writer inside ftruncate and asserts the reader never calls SharedMemory before the lock, so it fails on the old ordering on Linux too, not only on macOS. Returning len(data_bytes) instead of the handle size is equivalent on the metadata path and correct on the key path.
One question about the macOS row in the table: on the key path the reader takes the full shm.size, which is page-rounded on macOS, and the msgspec msgpack decoder rejects trailing bytes. Does the macOS delivery you measured go through this key path, or does it rely on the payload framing in #8946? If the latter, it may be worth saying so in the description so this PR is not read as the whole macOS fix.
Static review only; I did not run the tests locally. Main CUDA passes at c9b7acf (build 17898).
|
@linyueqian Thanks. updated the PR description for better clarification. Yes this PR is a general fix. key-only macOS delivery still needs the Metal adapter’s payload framing through #8946’s storage hooks. |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Reviewed c9b7acfa2675c87c8b4c814e286f0664c577fd08: publication-lock ordering, metadata/key reads, cleanup and the controlled writer-pause regression. No actionable issue found in this scope. The named baseline-fail/head-pass contributor receipts support the ordering fix; I did not execute tests locally. This does not validate standalone macOS key-only framing, which the author clarified still depends on #8946.
Current-head GitHub checks and CUDA ready build 17898 pass. Prior review questions were addressed.
Let external platforms adapt segment names, coordination paths and payload framing while reusing Omni connector locking, wakeups and cleanup. The default Linux storage format remains unchanged. Rebase the remaining storage adaptation on main after the publication-ordering fix landed in vllm-project#9285. Signed-off-by: Ranran Haoran Zhang <ranranhaoranzhang@gmail.com>
Resolve conflicts with vllm-project#9285 (lock before opening segments): the key-based read now takes the lock for the derived segment name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: RGB-loop <91311486+RGB-loop@users.noreply.github.com>
Fix macOS shared-memory payload loss when a reader opens a segment before the writer finishes sizing it.
Observed payload delivery when a reader polls while the writer is paused before sizing:
Key-only macOS delivery still requires the Metal adapter’s payload framing through #8946’s storage hooks.
The ordering regression is stricter: it checks that the reader never opens the segment before acquiring the lock. That test fails before and passes after on both platforms, even though Linux recovered and delivered the payload before the fix.
Acquire the existing nonblocking publication lock before opening the segment. Busy or missing segments still return
None.Extracted from #8946, which retains the fix until this lands.
More background about the Lock who are not familiar with it (it's actually myself) see codex elaboration
Codex Elaboration
The connector transfers data between processes—for example, audio codes from the Talker stage to Code2Wav. Publishing a payload takes several steps:
ftruncate.The reader should acquire that same lock before touching the segment. Previously, it opened the segment first to discover its size, then acquired the lock before reading the bytes.
That leaves a small window: the reader can open a segment that exists but is still empty. Holding the writer’s lock doesn’t help if the reader opens the segment before checking that lock.
The consequences differ:
For a stage-to-stage transfer, that macOS failure can leave the receiving stage waiting for data it will never receive, potentially causing a timeout or incomplete output.
The fix changes the reader’s sequence:
If the writer is busy, the reader immediately returns
Noneand retries later, without attempting to open unfinished storage.For Linux, I would not describe this as a demonstrated major data-loss issue. Our forced-interleaving checks delivered the payload both before and after the fix. The regression test fails on the old code because it detects the premature open—not because it proves lost data.
The fix is still worthwhile: it makes the shared-memory publication rule explicit and avoids relying on a particular platform’s error handling. Our evidence supports a proven macOS failure, a shared ordering defect, and preserved Linux behavior. It does not establish that every Linux/Python combination is immune.
macOS validation
macOS, Python 3.12.13, Torch 2.13.0, vLLM 0.32.0; base
851964d9.PYTORCH_ENABLE_MPS_FALLBACK=0.Linux validation
B300 Linux x86_64, Python 3.12.3, PyTorch 2.13.0+cu130, vLLM 0.32.0; pytest 9.1.1 and pytest-mock 3.15.1.
The regression pauses a real writer before
ftruncate, checks thatget()returnsNonewithout opening the segment, then verifies payload delivery after publication.851964d9: fails the early-open assertion (2 != 1calls toSharedMemory). The head test was copied onto the base without changing production code.c9b7acfa: passes, including payload delivery.The full
test_shm_connector.pysuite also passed: 44 passed, no skips. Linux payload loss was not reproduced; the regression guards publication ordering.Contributor intent
AI assistance: Codex for code, tests and this description.