[Bugfix] Give SHM connector lock files a segment-derived lifecycle (#7636 Issue 24)

[yanbao1217]

Follow-up of the Unified Full-duplex Framework (#7413): addresses issue 24 of #7636 (SHM connector lock-file lifecycle contract), from @Sy0307's review summary (pullrequestreview-5197739857).

Purpose

SharedMemoryConnector creates a zero-byte lock file /dev/shm/shm_{key}_lockfile.lock per transfer as an flock carrier. Lock files were only removed on the successful receiving read, or by cleanup() / close() for keys still in the sender's in-process _pending_keys set:

What broke:

• a failed put() — the lock file is created before shm_write_bytes() runs (shm_connector.py:49 on main), the key is tracked only after success (:56) — leaked the lock file even on a graceful close();
• a failed read whose segment shm_read_bytes() had already unlinked left the lock file behind forever (:81-86 removed it only when deserialized): read succeeds, deserialize_obj() fails, segment gone, lock orphaned, no removal path left at all;
• an abnormally terminated process (SIGKILL, or exit without close()) stranded one lock file per unconsumed transfer: resource_tracker reaps the segments at owner death — the warnings seen after the duplex process exits — while the lock files are plain files in tmpfs with no backstop at all; they accumulate until the machine reboots.

Root cause: the lock file had a lifecycle of its own, tracked only in process memory. Fix — the contract: a lock file has no lifecycle of its own; it exists exactly while its segment does.

1. put(): when shm_write_bytes() raises, remove the lock file while still holding it, then re-raise.
2. _get_data_with_lock(): on failure, remove the lock file when the segment is already gone; keep it while the segment lives (the transfer can be retried).
3. Once per process, the first constructed connector sweeps orphan lock files with a check–lock–recheck–unlink protocol: prefilter on mtime age (60 s grace, keeps the sweeper out of a creator's open() -> flock() window) and segment absence; O_RDONLY open + non-blocking flock (fails while a critical section holds the lock); recheck under the lock — segment still absent, mtime still past grace, path still resolves to the locked inode, owned by this uid — then remove while holding the lock.
4. The class docstring spells out the contract; _pending_keys becomes purely cleanup() / close() bookkeeping. cleanup() / close() behavior is unchanged. No threads, no timers, no lock-file content format.

Residual risks accepted: a µs-scale interleaving (consumer releases the lock → same-key put() creates a new inode → the sweep's unlink lands on the new file) can produce "segment alive, lock gone" — the receiving get() then returns None and the sender's close() reclaims the segment; fixing that would need a lock-file content protocol, deliberately avoided here. A shm_write_bytes() failure after segment creation but before returning can still leak a half-written segment (pre-existing). A forked child inherits the once-per-process flag; the next fresh interpreter sweeps.

Note for maintainers: SharedMemoryConnector.cleanup() currently has no production callers (only unit tests; the chunk adapter's cleanup() is a method on the adapter, not the connector). Lock reclamation in practice rides on the receiving read, close() at shutdown, and now the startup sweep — wiring cleanup() into request teardown might be worth a separate discussion. This file is byte-identical to pre-#7413 main; the leak predates the framework.

Test Plan

New TestLockFileLifecycle in tests/distributed/omni_connectors/test_shm_connector.py (13 cases, core_model and cpu): roundtrip; failed put() plus same-key retry; failed reads with dead/live segments (the deserialize-after-read case is the leak that had no removal path at all); sweep preconditions (stale orphan, live segment, fresh file, held flock, unrelated files); once-per-process; and a crashed-subprocess orphan swept afterwards.

1. Run the changed file and the whole affected directory at core_model and cpu.
2. Assert /dev/shm holds zero *lockfile* residue after the full directory run.
3. Drive the cross-process failure paths with a standalone stdlib-only harness that loads the real module source — put() failure, receiver deserialize failure after read, sender SIGKILL, sender clean exit without close(), partial multi-chunk consumption with both processes killed — on unpatched main and on this branch.
4. Local gates: ruff check / ruff format, mypy==1.11.1 (--follow-imports silent --ignore-missing-imports), tools/pre_commit/check_forbidden_imports.py on both changed files.
5. The remaining GPU e2e for this connector (test_bagel_shared_memory_connector.py, advanced_model) runs in CI on merge; it was not run locally.

Quick repro of the bug (on main the second command fails with ImportError and the lock file stays forever; on this branch it prints still there: False):

python -c "open('/dev/shm/shm_repro_leak_lockfile.lock','wb').close()"  # crash residue: segment already reaped by resource_tracker
python -c "import os, time; os.utime('/dev/shm/shm_repro_leak_lockfile.lock', (time.time()-120, time.time()-120)); from vllm_omni.distributed.omni_connectors.connectors.shm_connector import _sweep_stale_lock_files; _sweep_stale_lock_files(); print('still there:', os.path.exists('/dev/shm/shm_repro_leak_lockfile.lock'))"
rm -f /dev/shm/shm_repro_leak_lockfile.lock

pytest tests/distributed/omni_connectors/test_shm_connector.py -m "core_model and cpu"

vLLM Version: 0.28.0 (local cpu suites; the connector path does not touch vLLM APIs) — CI lane runs the repo's 0.29 line

vLLM-Omni Commit: f90c267 (tested base), rebased on 23d8c83

Test Result

CPU-marked suites on a dev box (an RTX 5090 is available but not exercised by these suites), Python 3.12, editable checkout of this branch:

• pytest tests/distributed/omni_connectors/test_shm_connector.py -m "core_model and cpu": 29 passed (16 pre-existing + 13 new).
• Whole affected directory, pytest tests/distributed/omni_connectors -m "core_model and cpu": 379 passed, 1 skipped, 30 deselected in 28 s. The one skip is test_omni_connector_configs.py:66 ("No config files found or directory missing") — environment-conditional and pre-existing; the deselected are non-CPU marks (mooncake/NIXL guards, advanced_model GPU e2e).
• /dev/shm contains zero *lockfile* residue after the full directory run.
• The standalone cross-process repro (step 3 above): on unpatched main every path leaves lock files behind — the resource_tracker warnings and the reaped-but-locked state from the issue reproduce one-to-one; on this branch all paths are clean and the baselines are unchanged.
• Gates from step 4 all pass (the first draft used stdlib re for the name match — replaced with prefix/suffix slicing in 4d0ac49); SPDX headers untouched; the new tests carry the module's core_model and cpu marks.
• One deliberate broad-catch addition, for the record: the sweep and the put() failure cleanup swallow OSError around best-effort os.remove calls, matching the existing cleanup() / close() idiom in the same file; nothing on a fail-fast path swallows.

🤖 Generated with Claude Code

[vllm-omni-review-bot]

This PR appears to belong to: docs/design/module/omni_connector.md, docs/design/module/input_output_modality_contracts.md, docs/design/module/diffusion/parallelism.md.

Module owners: @princepride @fake0fan @natureofnature

Routing: @princepride via module of the changed files, module named in the PR description, CODEOWNERS; @fake0fan via module of the changed files, module named in the PR description; @natureofnature via module of the changed files, module named in the PR description

@yanbao1217, 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.

[vllm-omni-review-bot]

Omni ReviewBot triage note

Automated triage of commit fc17ff94fb4b produced:

• Priority: high. Prompt maintainer attention is suggested.

These are automated triage suggestions only — the final decision belongs to the maintainers.

[yanbao1217]

Self-review:

• Re-read the final diff against the lifecycle contract documented on SharedMemoryConnector. The new removal paths for failed put(), failed reads after segment loss, and startup sweeping preserve the rule that an orphan lock is reclaimable only after its backing segment is gone; existing cleanup(), close(), and _pending_keys behavior is unchanged.
• Re-checked the sweep against its main false-positive cases: fresh locks in the creator open() -> flock() window, actively held locks, live segments, unrelated filenames, and foreign-uid files are all skipped. Per-file syscall failures are contained and do not abort the sweep.
• Tests pass: 29/29 in test_shm_connector.py, and 379 passed / 1 environment-dependent skip in tests/distributed/omni_connectors under core_model and cpu. /dev/shm has no *lockfile* residue after the suite.
• Verified the cross-process failure modes with a standalone harness against both unpatched main and this branch: failed put(), deserialize failure after read, sender SIGKILL, clean exit without close(), and partial multi-chunk consumption. The leaks reproduce on main and are cleaned on this branch.
• ruff, mypy==1.11.1, and forbidden-import checks pass. I found no additional correctness issue beyond the residual races already documented in the PR description.

[NickCao]

Out of scope for this PR: we are implementing file lock manually using fcntl in multiple places, these should be replaced with the filelock library.

[yanbao1217]

Thanks for taking a look! Happy to track the migration in a separate issue if that helps.

[vllm-omni-review-bot]

Omni ReviewBot: no human activity for 7 days

@yanbao1217 this pull request has had no human commit, comment or review since 2026-09-24. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state.

To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline.

[vllm-omni-review-bot]

Omni ReviewBot routing record

Assigned Strict on cursor (cursor-grok-4.6-high) under experiment fleet-strict-cursor-grok46-zcode-glm53flash-5050-c5-z10-20261002.