Skip to content

[CI] Synchronize shared offload unlink test before observing pathname - #55604

Merged
khluu merged 1 commit into
vllm-project:mainfrom
khluu:codex/fix-offload-unlink-test-race
Sep 7, 2026
Merged

khluu merged 1 commit into
vllm-project:mainfrom
khluu:codex/fix-offload-unlink-test-race

Conversation

@khluu

@khluu khluu commented Sep 6, 2026

Copy link
Copy Markdown
Member

The V1 Core KV Offload lane failed in build 87386 because test_mp_barrier_unlinks_file_and_survives_sigkill observed the mmap pathname before its creator unlinked it. The constructor's barrier synchronizes mapping, then the creator unlinks; a peer may return and inspect the pathname while that unlink is still pending.

Synchronize the test workers again after construction, before reporting the pathname state. Keep the existing assertions for a shared inode, pathname removal, and mappings surviving until SIGKILL. Production behavior is unchanged.

Duplicate checks: searched open PRs for shared_offload_region and unlink barrier. #53073 changes creator ownership, #54619 reaps orphaned regions, and #51317 changes region reclamation. Their changes do not provide this post-constructor synchronization in the existing test.

Validation on Linux, public main commit 6865e67f0b plus this patch:

  • VLLM_TARGET_DEVICE=cpu .venv/bin/python -m pytest -q tests/v1/kv_offload/cpu/test_shared_offload_region.py: 40 passed.
  • Deterministic reproduction, using a temporary test-only 0.5-second delay in os.unlink: the original test failed its pathname assertion; the patched test passed with the same delay. The delay was removed afterward and is not part of this change.
  • pre-commit run --files tests/v1/kv_offload/cpu/test_shared_offload_region.py: all applicable hooks passed.

CI follow-up: in build 87454 on head 08e1e2f78c, the tests/v1/kv_offload suite passed 570 tests, 1 skipped, including the previously failing test. The broader V1 Core + KV + Metrics job continues with other suites, so this is not a claim that the whole build has passed.

AI assistance was used to investigate, implement, and validate this change. Published as a draft for human review; human review is not claimed as completed. No model behavior changes or model evaluations are involved.

Wait until every constructor returns before observing pathname removal.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: khluu <khluu000@gmail.com>
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: a968c92e-0e89-4eb9-b49b-b6a327813cdb

📥 Commits

Reviewing files that changed from the base of the PR and between 6865e67 and 08e1e2f.

📒 Files selected for processing (1)
  • tests/v1/kv_offload/cpu/test_shared_offload_region.py

📝 Summary

Summary by CodeRabbit

  • Tests
    • Updated multiprocessing barrier test synchronization to verify ordering around shared offload region setup.
    • Added coverage ensuring worker views are created only after the creator completes the required file-handling step.

Walkthrough

The multiprocessing test worker now performs a second barrier wait after SharedOffloadRegion construction and before creating its worker view.

Changes

Shared offload synchronization test

Layer / File(s) Summary
Barrier synchronization checkpoint
tests/v1/kv_offload/cpu/test_shared_offload_region.py
The worker waits at a second barrier after constructing SharedOffloadRegion and before creating its worker view.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: etelis


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

Comment @coderabbitai help to get the list of available commands.

@khluu

khluu commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

/ci run

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #87454 for commit 08e1e2f78cc0.

@pavelzak

pavelzak commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

+1, my NVFP4 PR #52501 hit the same failure in the H200 V1 Core + KV + Metrics lane: Buildkite #87502, commit 7b47e5428.

test_mp_barrier_unlinks_file_and_survives_sigkill failed at line 939 on assert not any(r["path_exists"] for r in results). Both workers reported the same inode, and the log showed the creator unlinking the file. The KV-offload suite finished with 1 failed, 569 passed, 1 skipped.

That PR only changes NVFP4 scale handling in modelopt.py, so this is another occurrence blocking unrelated work. Thanks for addressing the race.

@khluu
khluu marked this pull request as ready for review September 7, 2026 19:51

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@khluu
khluu merged commit 58f0921 into vllm-project:main Sep 7, 2026
25 of 26 checks passed
ItsRoy69 pushed a commit to ItsRoy69/vllm that referenced this pull request Sep 10, 2026
…vllm-project#55604)

Signed-off-by: khluu <khluu000@gmail.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Jyotirmoy Roy <jyotirmoyroy649@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants