Skip to content

SHM tensor transport over a Rust shared-memory arena (RFC #130, Step 2) - #168

Merged
NSagan271 merged 18 commits into
mstar-project:mainfrom
npuichigo:shm-arena-step2
Jul 29, 2026
Merged

SHM tensor transport over a Rust shared-memory arena (RFC #130, Step 2)#168
NSagan271 merged 18 commits into
mstar-project:mainfrom
npuichigo:shm-arena-step2

Conversation

@npuichigo

@npuichigo npuichigo commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

SHM tensor transport over a Rust shared-memory arena (RFC #130, Step 2)

Stacked on #164 — please merge that first. Until it lands, this diff includes Step 1's commits; the Step-2 change itself is the final commit (shm arena: tensor transport over persistent mmapped segments). Opened as a draft until #164 merges and the c16-32 gate below has run.

Replaces SharedMemoryCommunicationManager's per-tensor file open/write/read/unlink with a segmented /dev/shm arena — persistent mmaps + a first-fit allocator in Rust (rust/src/shm.rs), exposed to Python through the buffer protocol so staging is zero-copy on both sides. Opt-in via MSTAR_SHM_ARENA=1|AUTO (default 0 = file transport, unchanged), mirroring MSTAR_RUST_ZMQ.

How it maps

  • Producer: register_for_send reserves (segment, offset) per tensor and D2H-copies directly into the mapped segment on the existing dedicated copy stream; one stream sync covers the whole batch before the control message ships.
  • Descriptors: the location rides the existing TensorPointerInfo as two new optional fields (shm_segment, shm_offset) — no wire-shape change; the infos are annotated between store and send.
  • Consumer: opens the named segment once (lazy, cached), torch.frombuffers the bytes zero-copy, H2D on its dedicated stream. The H2D stream is synchronized before the edge can be ACKed — the producer reclaims the slot on ACK, and the source is a live mapping (the file transport got this implicitly from f.read() copying).
  • Reclaim: uuid-keyed on the sender (cleanup → free(segment, offset)), the same lifecycle the unlink had.

The three design questions from the plan

Pinning (neutral-vs-regression question). Answered by measurement, and the answer is register the arena: each mapped segment is cudaHostRegister-ed once per process, on both sides (MSTAR_SHM_ARENA_PIN, default on). Standalone bench, 256 MiB × 20 D2H through a side stream: pageable mmap 466 ms, registered segment 187 ms — identical to torch pinned memory — with registration a one-time ~21 ms per 256 MiB segment. A registered segment keeps the side-stream copies truly asynchronous, which is exactly what preserves the copy/compute overlap; a pinned staging buffer would reintroduce a full memcpy per tensor.

uuid-based reclaim. In the allocator: reserve_for(uuid) / free_uuid(uuid) (grouped, idempotent) on the Rust side; the manager uses per-tensor free(segment, offset) keyed by its own uuid map, matching the file transport's bookkeeping.

Arena-full policy. Growth over blocking: the arena grows segment by segment up to MSTAR_SHM_ARENA_MAX_SEGMENTS (default 32 × 256 MiB). Existing mappings never move or resize — that is what keeps CUDA registrations and open consumer views valid, and it's why growth is by segments rather than remapping. Oversized tensors get a dedicated segment. Only at the cap do sends backpressure (poll for consumer ACKs), failing loudly after MSTAR_SHM_ARENA_FULL_TIMEOUT_S rather than hanging.

Gate status

The qwen3-omni c16–32 throughput + profiler-overlap gate from the plan still needs a run on real hardware — the microbench above de-risks the pinning question specifically, not the end-to-end claim. Happy to coordinate on running it; everything is flag-gated off until then.

Review round 2 (current head)

Rebased on main after #164 merged — the diff is now the seven arena-only commits. Since the initial review, the head adds (see the response comment for detail):

  • Fragmentation observability: largest_free_block from the coalescing free-list; SegmentedShmArena.stats() + manager stats(); growth logs the snapshot; a warning fires on the exact signature (reserve fails while total free covers it). Deterministically tested (alternate-free pattern).
  • Graceful spill at the cap (MSTAR_SHM_ARENA_SPILL, default on): brief backpressure, then the tensor stages through the per-uuid file protocol — slower, never fails; consumer falls back per descriptor (mixed arena+spill edges tested); =0 restores strict fail-fast. Side effect: file-producer → arena-consumer deployments now interoperate.
  • Pinned-bytes budget (MSTAR_SHM_ARENA_PIN_MAX_MB): total registered bytes capped independently of the segment cap; over-budget and oversized one-shot segments stay unpinned.
  • Read-completion wake: start_read_tensors returns a real Future (watcher thread completes it on the h2d event) so the worker's eventfd fires the moment copies land — closes the 10 ms idle-tick gap flagged inline.
  • py.allow_threads on the arena FFI's slow paths; docs restructured ("Rust support" running list); module docstring condensed.

Tests

  • cargo test: arena create/open, reserve/free/first-fit reuse, segmented growth + dedicated oversized segments, uuid reclaim, cross-process visibility.
  • test/rust/test_arena_transport.py: producer→consumer roundtrip through two managers (multi-tensor, empty-tensor), descriptor stamping, sender-side reclaim, growth then backpressure-then-fail at the cap, and MSTAR_SHM_ARENA factory selection (0/1/AUTO/invalid).
  • Env vars documented in docs/environment_variables.rst.

🤖 Generated with Claude Code

@npuichigo

Copy link
Copy Markdown
Contributor Author

Pre-gate evidence for the host-side component — an A/B of the two managers in this PR (producer store_and_return_tensor_info + register_for_send, consumer start_read_tensors), same tensors, /dev/shm, CPU device so it isolates the transport work itself from CUDA copies:

Edge shape File (send+read) Arena (send+read) Speedup
32 × 64KB (streaming chunks) 13.4 ms 1.0 ms 13.6×
4 × 2MB (hidden states) 12.1 ms 1.4 ms 8.8×
1 × 64MB (latents/prefill) 148.7 ms 6.8 ms 21.9×

The file path's per-tensor costs compound — serialize copy + open/write + fresh tmpfs page allocation on send; read + bytearray copy on receive — where the arena is one memcpy each way into an already-mapped, already-faulted segment.

This stacks with the GPU component measured earlier (pageable vs registered mmap, 256 MiB × 20 D2H through a side stream: 466 ms vs 187 ms, with the registered copies staying genuinely async): the file path pays its D2H/H2D at pageable bandwidth, synchronously, on top of the host-side work above.

What this does not answer is the end-to-end share — that's exactly what the c16-32 gate measures, and these numbers don't substitute for it. But the mechanism question (is the arena actually cheaper per tensor, and by how much) now has a measured answer rather than an argument.

@NSagan271

NSagan271 commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

@npuichigo I'll fully review tomorrow, but I benchmarked Qwen3-Omni with MSTAR_SHM_ARENA on and off with max concurrency of 16 (closed-loop continuous batching, seed_tts dataset, 80 total requests per row, 3 rounds of warmup requests; H200 node with cuda 12.9 and torch 2.13).

Config GPUs MSTAR_SHM_ARENA Throughput (audio s/s) RTF mean RTF p50 RTF p99 Mean TTFA (s) Text ITL (ms)
2-GPU (talker+code2wav on GPU1) 2 1 85.54 0.172 0.170 0.254 0.516 14
2-GPU (talker+code2wav on GPU1) 2 0 85.39 0.172 0.168 0.236 0.520 14
3-GPU (code2wav on GPU2) 3 1 89.83 0.169 0.168 0.266 0.528 14
3-GPU (code2wav on GPU2) 3 0 80.87 0.186 0.179 0.290 0.572 14

In the two-GPU case, MSTAR_SHM_ARENA is on par with the original SHM implementation. For the three-GPU ablation, MSTAR_SHM_ARENA leads to a gain in throughput. I only ran the benchmark script once for each row (I will test more extensively tomorrow), but this is at least enough to say that there is no regression and potential e2e improvement.

@npuichigo

npuichigo commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for running the gate — and the split between the two configs is exactly the mechanism showing itself: with talker+code2wav colocated (2-GPU) the cross-process tensor traffic is minimal, so the transports tie; the 3-GPU layout pushes every talker→code2wav tensor through the SHM hop, which is where the per-tensor host-cost reduction (the 9-22× A/B above) and the pinned-segment copies surface as end-to-end throughput (+11%) and better RTF/TTFA. So the arena is neutral where transport is cold and wins where it's hot, which is the profile you'd want before flipping any default.

If you want the copy-overlap confirmation from the original gate criteria, an nsys/torch-profiler trace of the 3-GPU arena run should show the D2H/H2D on the side streams overlapping the next step (the registered segments are what keep those copies truly async).

@NSagan271 NSagan271 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In addition to the comments I left, I have a few general concerns about the SHM arena, specifically under heavy load with large tensors (e.g., for video understanding or generation models).

  1. Memory fragmentation: in the case where tensor shapes are heterogeneous (e.g., prefill-heavy workloads, or diffusion/flow generation with highly varied shapes), and there are many in-flight requests at any given time, memory fragmentation may occur. This could conceivably drive the arena size toward MSTAR_SHM_ARENA_MAX_SEGMENTS even though bytes_free is far from exhausted.

    The telltale signature is the largest contiguous free block collapsing while total free still looks healthy. I don't think we can see that today, and without it the first symptom is a timeout in production; I would recommend having a way to surface this metric (maybe as part of the existing --log-stats, and as a warning when the metric crosses a certain threshold)

    Mitigation-wise, since we can't lean on statically-known shapes here (outside decode-type flows, where fragmentation isn't much of an issue anyway). I would consider segregated free-lists / runtime size-classing or at least best-fit over first-fit, but let me know if you think this is worthwhile / how much of an issue you think fragmentation is in the current design.

  2. Hard failure at the cap is a regression: the old SharedMemoryCommunicationManager was bounded only by tmpfs, so an oversized or bursty workload got slower but never failed. I'd feel better if the full-arena path degraded gracefully instead of erroring; e.g. spilling to the existing file/pageable transport (or an unpinned overflow segment) when the arena is saturated.

  3. Pinned host memory is a separate, harder ceiling than the segment count: assuming a case with many large video tensors in SHM, I can envision cudaHostRegister pinning GBs of RAM, and pinned pages come out of the OS's pageable pool system-wide. I'd suggest a cap on total pinned bytes distinct from the segment count. It's also worth checking whether pinning one-shot oversized segments even pays off: for a single large transfer the registration cost may exceed the overlap benefit.

  4. /dev/shm sizing: tmpfs defaults to ~50% of RAM, so MAX_SEGMENTS × segment_size needs to be reconciled against the actual /dev/shm size. Probably worth documenting the relationship and failing early with a clear message if the configured arena can't fit.

To surface these before they bite, I think we'd want two stress tests (I'd be happy to run these): (a) a multi-hour soak at mixed concurrency with heterogeneous tensor sizes (e.g., BAGEL or Cosmo3 with varied output shapes, or Qwen3-Omni with varied multimodal inputs), watching segment count, largest_free_block / total_free, and backpressure waits over time; and (b) a large-tensor stress run (e.g., Cosmo3, or synthetic large tensors) driven to the cap, measuring pinned host bytes, /dev/shm usage, and failure rate vs. the old file transport's tail latency.

None of this blocks the core approach: the arena is definitely faster based on your microbenchmarks and also my 3-GPU Qwen3-Omni test. I think this could be a big performance gain in the heavy cases, as long as it holds up under pressure. Concretely, would you be open to (1) exposing a fragmentation metric, (2) a graceful spill-to-file fallback at the cap, and (3) a total-pinned-bytes cap?

Comment thread docs/environment_variables.rst
Comment thread docs/installation.rst Outdated
@@ -0,0 +1,331 @@
"""Tensor transport over a shared-memory arena.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This comment is a bit long to read if skimming the code; can probably be condensed.

Comment thread mstar/communication/arena.py Outdated
Comment thread mstar/graph/base.py
Comment thread rust/src/lib.rs Outdated
Comment thread rust/src/shm.rs
@npuichigo

Copy link
Copy Markdown
Contributor Author

All three concrete asks are in (d737de6), plus the read-wake fix from your inline comment:

  1. Fragmentation metric — the allocator was already a sorted free-list with eager adjacent coalescing, so largest_free_block is a cheap exact max() over it. SegmentedShmArena.stats() returns (total, free, largest_free_block); the manager surfaces it (plus pinned bytes) via stats() for stats logging, every segment-growth logs the snapshot, and the exact signature you described — a reserve failing while total free covers it — logs a warning naming the collapsed largest block at the moment it bites, not just on a polling interval.

  2. Graceful spill — at the segment cap a send now backpressures briefly (MSTAR_SHM_ARENA_SPILL_AFTER_S, 50 ms) and then stages that tensor through the per-uuid file protocol: slower, never fails, i.e. the old manager's saturation behavior. Spilled descriptors just keep shm_segment=None and the consumer falls back to the file read for exactly those — a side effect is that a file-producer + arena-consumer deployment now interoperates instead of erroring. MSTAR_SHM_ARENA_SPILL=0 restores strict backpressure + loud failure for anyone who prefers fail-fast.

  3. Pinned budgetMSTAR_SHM_ARENA_PIN_MAX_MB (default 4096) caps total registered bytes, separate from the segment cap; segments past the budget stay unpinned (copies work, no async overlap). And you were right about one-shot oversized segments: a dedicated segment for a single oversized tensor is never pinned — one transfer doesn't amortize the registration.

On your questions:

  • Best-fit / size-classing: I'd hold at first-fit + eager coalescing + alignment for now and let the metric decide. For precedent, vLLM's staging pool for its transfer engine (memory_pool.py) is exactly first-fit + coalescing; SGLang avoids the problem by construction with fixed-slot pools where it can, and its variable-size staging buffer doesn't track free blocks at all. The staging lifetime profile (staged → consumed → freed, arena drains between loads) means fragmentation can't accumulate across requests — it's transient at peak. If your heterogeneous soak shows largest_free_block/free collapsing, size-class free lists drop into the existing structure without touching the wire format.

  • Background growth: I'd skip it — the registration cost is ~20 ms once per segment, and with the spill tier a growth stall can no longer fail a request; a background thread + pre-grow watermark buys little for the added lifecycle complexity.

  • arena.py:303 (the 10 ms wake gap): fixed — start_read_tensors now returns a real Future, completed by a small watcher thread the moment the h2d-stream event fires, so EventWakeup.register_futures wakes an otherwise-idle worker immediately. The CUDA-event polling path for get_ready_tensors is unchanged.

  • allow_threads: good catch — reserve/reserve_for (whose growth path does the shm create + mmap) and free_uuid (many frees per uuid) now release the GIL; the sub-microsecond mutex getters deliberately don't.

  • TensorTransferInfo dataclasses: agreed it's cleaner in principle; my hesitation is that the flat-optional-fields shape is what the existing RDMA fields already do, and a subclass hierarchy changes the (de)serialization of every info on the wire. I'd propose doing that refactor for both transports together in a small follow-up rather than inside this PR — happy to take it either way.

  • yaml knobs: agreed, especially once [WIP] Multinode v1 #156's cluster section lands — will add the arena block to the yaml in a follow-up so it composes with that schema rather than inventing one here.

  • docs: installation now has a generic "Rust support" section with the running component list, the arena docstring is condensed, and the new knobs are in environment_variables.rst.

The soak + large-tensor stress runs you offered would be very welcome — stats()/the growth logs expose segment count, largest_free_block/free, and pinned bytes, which should give run (a) and (b) exactly the time series you described.

@npuichigo
npuichigo force-pushed the shm-arena-step2 branch 3 times, most recently from 60785ad to 6accf1a Compare July 18, 2026 08:05
@npuichigo

Copy link
Copy Markdown
Contributor Author

Rebased on main now that #164 is in — the diff is down to the seven arena-only commits (11 files), with all the round-2 items (fragmentation gauge, spill, pinned budget, read-wake) at the head. PR body has a summary of what changed since the first review pass. Ready for re-review whenever you are, @NSagan271.

@NSagan271 NSagan271 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some potential issues; once these are addressed I'll branch off of your branch to do the long-soak test. Let me know if all of these make sense or if you disagree with any (@merceod also feel free to take a look).

Can you also document somewhere (can be in the existing #130) the comments that are explicitly deferred (i.e., yaml knobs and TensorTransferInfo dataclasses)?

Comment thread mstar/communication/arena.py Outdated
Comment thread mstar/communication/arena.py Outdated
Comment thread rust/src/lib.rs Outdated
Comment thread mstar/communication/arena.py
Comment thread mstar/communication/arena.py Outdated
Comment thread mstar/communication/arena.py
@npuichigo

Copy link
Copy Markdown
Contributor Author

All make sense — everything addressed in 9ed4918:

  • &mut borrow across allow_threads — took your fix exactly: SegmentedShmArena now has interior mutability (segments: Mutex<Vec<Arc<ShmArena>>>), every method takes &self, growth serializes on the segments lock instead of the PyCell borrow. Concurrent stats()/free()/free_uuid() during a GIL-released growth can no longer raise.
  • GIL-held cudaHostRegister — you're right that the torch cudart binding holds the GIL. Registration now goes through ctypes (which releases the GIL for the call's duration), so the once-per-growth 256 MiB registration no longer stalls the worker's other Python threads; torch-binding fallback kept for environments without a loadable libcudart.
  • Event busy-wait — nice catch (and thanks for the measurement script): _CudaEventFuture now creates Event(blocking=True), so the wake watcher sleeps instead of burning a core per wait.
  • _wake_when_done race — queue/thread creation is now under a threading.Lock.
  • Oversized-segment pinning — agreed with your reversal, and I'd add the reasoning for the record: since freed dedicated segments are reused for later large tensors, the registration amortizes over the segment's lifetime, not one transfer — so plain pinning-within-budget beats both alternatives: transient oversized segments would pay create+register per large tensor (the exact cost the persistent-segment design avoids), and routing oversized tensors to the legacy file manager forfeits async overlap entirely for the largest transfers, which need it most. The nbytes > segment_size check is removed; the pinned-bytes budget remains the safety.
  • stats naming + logging — renamed to stats_summary() (the raw arena tuple keeps stats()), and under --log-stats the producer path now logs the snapshot at most once per MSTAR_SHM_ARENA_STATS_INTERVAL_S (default 60 s) — so your long soak will leave an occupancy/fragmentation time series in the logs with no extra flag.

Deferred items are now recorded in #130 as requested. Ready for the soak branch whenever you are.

@NSagan271

Copy link
Copy Markdown
Collaborator

@npuichigo Thank you for the speedy fixes! I just noticed one more thing: /dev/shm and pinned-RAM ceilings are per-entity; the configured maxima multiply by the number of entities (workers + api server data worker)

Each entity creates its own arena (mstar_arena_{my_entity_id}), so MSTAR_SHM_ARENA_MAX_SEGMENTS and MSTAR_SHM_ARENA_SEGMENT_MB bound one process, not the node. The node-wide /dev/shm ceiling is:

MAX_SEGMENTS × SEGMENT_MB × (num_workers + 1 api-server data worker)

With the defaults (32 × 256 MiB = 8 GiB/entity) an 8-worker node can reach 72 GiB of /dev/shm. tmpfs defaults to ~50% of RAM, so on a 256 GB box (~128 GiB /dev/shm) that's fine, but it's within ~2× of the limit and scales with worker count; a larger SEGMENT_MB or a denser node would blow past df -h /dev/shm and fail deep in a run rather than at startup.

There's a second, independent multiplier: pinned host RAM. It's also per-process, and a consumer pins its own segments plus the peer segments it reads, each process capped at MSTAR_SHM_ARENA_PIN_MAX_MB. So the node pinned-RAM ceiling is approx. PIN_MAX_MB × num_entities, and per-process pinned_bytes can exceed that process's own arena size (it includes peer pins).

Two asks:

  1. Document the relationship in the arena module docstring / environment_variables.rst: both the /dev/shm formula and the pinned-RAM formula, called out as ×(num entities).
  2. Fail fast at startup with a clear message when MAX_SEGMENTS × SEGMENT_MB × num_local_entities exceeds the detected /dev/shm size (statvfs on the shm dir), rather than surfacing as a tmpfs ENOSPC on a growth mid-run. Same spirit as the arena-full → spill path, but for the static tmpfs ceiling. Optionally warn when PIN_MAX_MB × num_local_entities exceeds a sensible fraction of physical RAM.

Also, there's some CI builds failing; I think you probably need to rebase on main (the arguments to register_for_send have changed upstream).

@npuichigo

Copy link
Copy Markdown
Contributor Author

Both done in 043afe6 (and rebased on main — the new register_for_send(request_id, tensor_infos, ...) signature is adopted; the arena override stamps the passed infos directly, which actually simplified the bookkeeping):

  1. Documented — the module docstring and the env-vars table now carry both ×(num entities) formulas: /dev/shm demand up to MAX_SEGMENTS × SEGMENT_MB × num_entities, pinned RAM approx PIN_MAX_MB × num_entities, with the explicit note that a consumer pins peer segments so one process can pin more than its own arena holds.
  2. Fail fast — construction now statvfses the shm mount: it raises with the sizing formula when ONE entity's MAX_SEGMENTS × SEGMENT_MB already exceeds /dev/shm total, warns when it exceeds currently free space (which naturally accounts for the other entities that already spun up — each later entity sees less headroom, so a node headed for your 72 GiB scenario warns at startup rather than ENOSPCing mid-run), and warns when the per-process pin budget is more than a quarter of physical RAM.

CI should be green again with the rebase. Ready for the soak branch.

@merceod
merceod self-requested a review July 19, 2026 01:16

@merceod merceod left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @npuichigo

The arena's data path is correctness-solid - every bitwise gate passed across 7 models x 9 configs including PD, TP2, SP2, streaming, and forced-spill pressure. But the PR cannot merge as-is: it's runtime-broken against current main (#177 rebase rot), and its fixed /dev/shm naming causes silent cross-server corruption plus a cross-user denial-of-service that we hit live on our cluster (when I was testing).

--IGNORE ISSUE 1 (ALREADY FIXED)--

Issue 1 (critical; rebase rot: broken against main's #177): #177 (merged after this PR's last rebase) changed register_for_send to take tensor_infos: list[TensorPointerInfo] - passed by keyword at both call sites. The arena manager still implements the old uuids signature, and GitHub reports MERGEABLE because the hunks don't overlap. On the merged tree, the first arena-enabled send dies with TypeError: unexpected keyword argument 'tensor_infos' (proven by AST + call-shape; the PR's own tests use the old convention too, which masks it from the arena tests while breaking the file-manager test). The fix is the same mechanical transform #177 applied to the other managers. With it applied (my local cand-168, 14 insertions), all my arena+SHM tests pass. _Also, with infos passed in directly, the infos_by_uuid stamping side-table becomes unnecessary.


Issue 2 (critical; fixed segment names collide across servers; create() truncates live arenas): Names are mstar_arena_{entity_id}.seg{i} in a global /dev/shm, every mstar server has worker_0 and api_server_preprocess_worker. The old transport was collision-free via per-tensor uuid4 filenames; the arena drops that property. Empirically demonstrated both ways: same user (our PTC cluster): starting server B truncated live server A's segments and A then returned 8 MiB of wrong bytes on one reques with zero errors in any log i.e. silent, intermittent corruption. Cross user (our coriander cluster, live): @NSagan271 's soak owned the names, and any other user's (my) arena server dies at startup with Permission denied (os error 13). Her run later left a worker_0.seg0 orphan that still blocks every other user's arena runs on the box. Fix is cheap and wire-compatible: make the base name instance-unique (socket-prefix hash or pid+startup-uuid) - consumers just open whatever name the descriptor carries.

Issue 3 (logical: worker-side backpressure waits on ACKs only the blocked thread could process): TENSOR_RECEIVED (which frees slots) is handled on the worker main loop ie. the same thread that spins in _reserve. Strict mode (MSTAR_SHM_ARENA_SPILL=0): arena-full is a guaranteed full-timeout stall then failure (demonstrated: 6/6-request burst took 31 s with 6 s timeouts, 5 loud 500s). Default spill mode: every at-cap tensor pays the whole 50 ms grace as dead time.Worse, the strict demo surfaced an error-semantics gap: one request's worker-side arena-full raise was masked, the walk completed, and the client received HTTP 200 with a 0-byte payload so silent data loss that neither the file transport nor spill mode can produce. The docs' "backpressure until consumers ACK" is only true on the api_server (threaded).

Issue 4 (no crash-cleanup; SIGKILL orphans up to 8 GB/entity): Drop-unlink works on graceful exit (verified), but kill -9 leaves all segments: measured 512 MiB orphaned per killed single-worker server, every cycle; same-name reuse bounds steady-state accumulation but that mechanism is F2's truncation, and the last run's orphans persist until reboot (@NSagan271 's was sitting there when I ran). fd counts stay flat so no fd leak. Suggest a startup sweep of unheld mstar_arena_* files.

Issue 5: shm.rs module docs describe a conductor-driven Event::Free reclaim that doesn't exist in this PR (misleading for #170 which builds on it); the Rust uuid-ledger API (reserve_for/free_uuid) is dead code from Python as the manager tracks (segment, offset) itself.

@NSagan271 please read the comments and let me know what you think as well.

@merceod

merceod commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Oh I see you already rebased and register_for_send was already fixed. Please ignore "Issue 1" I mentioned in my comment above then.

@NSagan271

Copy link
Copy Markdown
Collaborator

Hi @npuichigo

The arena's data path is correctness-solid - every bitwise gate passed across 7 models x 9 configs including PD, TP2, SP2, streaming, and forced-spill pressure. But the PR cannot merge as-is: it's runtime-broken against current main (#177 rebase rot), and its fixed /dev/shm naming causes silent cross-server corruption plus a cross-user denial-of-service that we hit live on our cluster (when I was testing).

Issue 1 (critical; rebase rot: broken against main's #177): #177 (merged after this PR's last rebase) changed register_for_send to take tensor_infos: list[TensorPointerInfo] - passed by keyword at both call sites. The arena manager still implements the old uuids signature, and GitHub reports MERGEABLE because the hunks don't overlap. On the merged tree, the first arena-enabled send dies with TypeError: unexpected keyword argument 'tensor_infos' (proven by AST + call-shape; the PR's own tests use the old convention too, which masks it from the arena tests while breaking the file-manager test). The fix is the same mechanical transform #177 applied to the other managers. With it applied (my local cand-168, 14 insertions), all my arena+SHM tests pass. _Also, with infos passed in directly, the infos_by_uuid stamping side-table becomes unnecessary.

Issue 2 (critical; fixed segment names collide across servers; create() truncates live arenas): Names are mstar_arena_{entity_id}.seg{i} in a global /dev/shm, every mstar server has worker_0 and api_server_preprocess_worker. The old transport was collision-free via per-tensor uuid4 filenames; the arena drops that property. Empirically demonstrated both ways: same user (our PTC cluster): starting server B truncated live server A's segments and A then returned 8 MiB of wrong bytes on one reques with zero errors in any log i.e. silent, intermittent corruption. Cross user (our coriander cluster, live): @NSagan271 's soak owned the names, and any other user's (my) arena server dies at startup with Permission denied (os error 13). Her run later left a worker_0.seg0 orphan that still blocks every other user's arena runs on the box. Fix is cheap and wire-compatible: make the base name instance-unique (socket-prefix hash or pid+startup-uuid) - consumers just open whatever name the descriptor carries.

Issue 3 (logical: worker-side backpressure waits on ACKs only the blocked thread could process): TENSOR_RECEIVED (which frees slots) is handled on the worker main loop ie. the same thread that spins in _reserve. Strict mode (MSTAR_SHM_ARENA_SPILL=0): arena-full is a guaranteed full-timeout stall then failure (demonstrated: 6/6-request burst took 31 s with 6 s timeouts, 5 loud 500s). Default spill mode: every at-cap tensor pays the whole 50 ms grace as dead time.Worse, the strict demo surfaced an error-semantics gap: one request's worker-side arena-full raise was masked, the walk completed, and the client received HTTP 200 with a 0-byte payload so silent data loss that neither the file transport nor spill mode can produce. The docs' "backpressure until consumers ACK" is only true on the api_server (threaded).

Issue 4 (no crash-cleanup; SIGKILL orphans up to 8 GB/entity): Drop-unlink works on graceful exit (verified), but kill -9 leaves all segments: measured 512 MiB orphaned per killed single-worker server, every cycle; same-name reuse bounds steady-state accumulation but that mechanism is F2's truncation, and the last run's orphans persist until reboot (@NSagan271 's was sitting there when I ran). fd counts stay flat so no fd leak. Suggest a startup sweep of unheld mstar_arena_* files.

Issue 5: shm.rs module docs describe a conductor-driven Event::Free reclaim that doesn't exist in this PR (misleading for #170 which builds on it); the Rust uuid-ledger API (reserve_for/free_uuid) is dead code from Python as the manager tracks (segment, offset) itself.

@NSagan271 please read the comments and let me know what you think as well.

I agree with all; and I think Issue 4 is also present in the exiting SHM transport (I've had to manually clean up files in the SHM directory)

@npuichigo

Copy link
Copy Markdown
Contributor Author

@merceod — thank you for this review; the bitwise gate matrix and especially catching the live cross-server truncation are exactly the kind of verification this transport needed before it touches shared clusters. Everything is addressed in 419d6df:

  • Issue 1 — was fixed in the 043afe6 rebase you spotted, and your side-note is taken too: with infos passed in directly, the _infos_by_uuid side-table was indeed unnecessary — removed, register_for_send stamps the infos it receives.
  • Issue 2 (the corruption/DoS) — base names are now instance-unique: mstar_arena_{entity}_{pid}_{token}. Wire-compatible exactly as you said (consumers open whatever name the descriptor carries). New test: two same-entity-id managers coexist and the first's staged data survives the second's creation — the truncation scenario is now a regression test.
  • Issue 4 (orphans) — the pid in the new names doubles as the liveness key: construction sweeps mstar_arena_* files whose owner pid is gone (/proc/<pid>), leaving anything it can't judge or can't remove (another user's) with a debug note. Your worker_0.seg0 scenario gets cleaned by the next arena start on the box instead of persisting to reboot. Tested both directions (dead-owner reclaimed, live-owner untouched). @NSagan271 — since you've hit the same with the file transport, the sweep pattern would port there too (per-tensor uuid names would need a registry file or age-based policy rather than pid-in-name); happy to do that as a small follow-up if you want it.
  • Issue 3 (backpressure semantics) — you're right on both counts. The grace now defaults to 0: on a worker, the ACKs that free slots are processed by the very thread that would sit in the wait, so any grace is pure dead time — spill is immediate. SPILL_AFTER_S remains for deployments where another thread drains ACKs, and the docs now say plainly that strict backpressure is only meaningful on the threaded api-server. The masked-raise → HTTP-200-empty case you demonstrated is a pre-existing worker error-swallow (the walk completes despite a raised transport error) — it's out of this PR's diff but real; with spill-by-default this path no longer triggers, and I'd propose tracking the swallow itself as its own issue since it can mask more than arena errors.
  • Issue 5 — the uuid-ledger API (reserve_for/free_uuid) is removed along with its bindings (no Python users; it can return with an actual consumer), and the module docs no longer describe a reclaim flow this PR doesn't implement.

Stack rebased; all green locally (18 arena-suite tests incl. the two new ones).

@npuichigo

Copy link
Copy Markdown
Contributor Author

One more fix from a self-review pass before the soak (fb9eaf2): the instance-unique naming in 419d6df quietly created a consumer-side leak — the peer-segment cache never evicted, which was survivable when a restarted producer reused names, but with unique names every producer restart mints a new generation that a long-lived consumer would map and pin forever (unlinked-but-mapped segments keep their memory, and their registered bytes stay pinned).

Cached peer entries now record their pinned size and are evicted once the backing /dev/shm file disappears: cudaHostUnregister (ctypes, GIL-released), mapping dropped, pinned accounting decremented. Eviction only runs while pending is empty — an mmap must outlive any in-flight h2d copy reading from it, and the pending futures are exactly those copies — and is time-gated off the hot path. Covered by a restart-shaped test.

Worth having in before the soak since producer restarts over hours would have shown monotonic consumer RSS/pinned growth.

npuichigo added a commit to npuichigo/mstar that referenced this pull request Jul 24, 2026
…guard

@NSagan271's mstar-project#183 is the canonical fix for the persisted-tensor leak,
in the conductor/worker domain: it stops counting routing.persist in
set_output_ref_counts at the source, leaving the persist flag + the
conductor's unpersist-with-K accounting to balance the references.

My previous persist-hold mechanism here (tracking references per
set_persist and releasing them on flag clear) was a second, redundant
fix for the same bug — and once mstar-project#183 removes the count, it would
over-dereference and free persisted tensors early. Reverted, so mstar-project#168
makes no changes to the shared persist / ref-count semantics and
rebases cleanly onto mstar-project#183.

Also dropping the double-ACK guard from the self-audit: as NSagan271
noted, a persisted tensor legitimately re-sent to the same node across
graph walks looks identical to a re-delivery, so the guard could
suppress a real ACK and undercount against the conductor's K — exactly
what mstar-project#183's accounting must not lose. Removing it restores the
pre-audit ACK behavior the conductor logic is designed around.

Kept: the worker SIGTERM -> SystemExit graceful-exit handler and the
SIGKILL-after-join escalation in Conductor.shutdown (matches the code
NSagan271 verified), which is what lets the arena's exit finalizer run
and unlink worker segments. The three tests tied to the reverted
shared code are removed with it; arena suite green.
@npuichigo

Copy link
Copy Markdown
Contributor Author

Rebased onto main (with #183) — c46fb04. Verified:

  • Persist leak is now fixed by Worker Bug Fixes #183 and inherited here: my branch makes zero changes to the persist / ref-count / ACK logic (the diff vs main on tensors.py is arena-selection plumbing only), so there's no double-fix — the leak goes through your worker-side fix.
  • Shutdown cleanup — kept the SIGTERM -> SystemExit worker handler + the SIGKILL-after-join escalation in Conductor.shutdown (no conflict; main's shutdown doesn't have the escalation). That's what lets the arena exit-finalizer run and unlink worker segments.
  • Rebase was clean (no conflicts); test/rust arena suite 21/21.

Ready for your final BAGEL soak whenever — live_slots should stay flat (persist leak gone via #183) and /dev/shm clean after Ctrl+C (SIGTERM fix). Stack tip: 168 c46fb04 → 170 a2d5e39 → 171 030d702.

@npuichigo

Copy link
Copy Markdown
Contributor Author

Heads up on the red CI (build job) — it's not from this PR, it's a repo-wide ruff-version drift:

  • The only failures are PLR0917 Too many positional arguments in benchmark/dataset.py and benchmark/request.py, both byte-identical to main here (this PR touches zero benchmark/ files).
  • Cause: the build job does an unpinned pip install ruff, which now pulls 0.16.0, where PLR0917 graduated from preview to stable — so the repo's existing select = ["PL"] starts enforcing it on pre-existing code. ruff ≤ 0.15 (where it's still preview) passes clean, which is why it was green before.

It'll hit every open PR (and main) until the build job is fixed. Since it's a pre-existing job this PR doesn't own, I'd rather not pin ruff inside the arena PR — the fix belongs on main so it unblocks everyone at once. Two one-liners:

  • pip install 'ruff<0.16' in the build job, or
  • add PLR0917 to [tool.ruff.lint] ignore in pyproject.toml (if the rule isn't wanted).

Happy to open a tiny CI-fix PR for either if useful — just say which you prefer. This PR's own files are ruff-clean under both versions.

@NSagan271 NSagan271 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did a final test, LGTM.

An unrelated thing I found is that tensors are sometimes leaked on request cancellation (which logically makes sense), but that is a system-wide issue and can really only be solved by cleaning up tensors after a TTL---not anything to do with this PR.

@NSagan271

Copy link
Copy Markdown
Collaborator

Also the ruff PR is in #187; once that merges CI should be green again @npuichigo

@NSagan271

Copy link
Copy Markdown
Collaborator

@npuichigo the ruff PR just merged, you can rebase and merge this branch!

npuichigo added 18 commits July 26, 2026 00:42
Replaces the per-tensor file open/write/read/unlink in
SharedMemoryCommunicationManager with a Rust segmented /dev/shm arena
(rust/src/shm.rs: persistent mmaps + first-fit allocator, buffer-protocol
views for zero-copy staging).

- Producer reserves (segment, offset) per tensor and D2H-copies straight
  into the mapped segment on the dedicated copy stream; one stream sync
  covers the batch before the control message ships.
- The location rides the existing descriptors (TensorPointerInfo grows
  optional shm_segment/shm_offset) — no wire-shape change.
- Consumer opens the named segment once, views the bytes zero-copy, and
  H2D-copies on its dedicated stream; the stream is synchronized before
  ACKing, since the producer reclaims the slot on ACK (the file
  transport's f.read() made that implicit).
- Pinning: each mapped segment is cudaHostRegister-ed once per process,
  both sides (MSTAR_SHM_ARENA_PIN, default on) — copies through the side
  streams then run at page-locked bandwidth and stay asynchronous, which
  is what preserves the copy/compute overlap the side streams exist for.
  Measured on the standalone bench: 256 MiB x20 D2H = 466 ms pageable
  mmap vs 187 ms registered (== torch pinned); registration is a one-time
  ~21 ms per 256 MiB segment.
- Capacity: the arena grows by segments up to
  MSTAR_SHM_ARENA_MAX_SEGMENTS (mappings never move — registrations and
  open consumer views stay valid; oversized tensors get a dedicated
  segment). At the cap, sends backpressure until consumers ACK, failing
  loudly after MSTAR_SHM_ARENA_FULL_TIMEOUT_S.
- Reclaim is uuid-keyed on the sender (cleanup maps uuid -> free), same
  lifecycle as the file transport's unlink.
- Selection: MSTAR_SHM_ARENA = 0 (default, files) / 1 / AUTO on the SHM
  protocol, mirroring MSTAR_RUST_ZMQ; documented in
  docs/environment_variables.rst.

Tests: cargo tests for the arena/allocator; pytest covers the
producer->consumer roundtrip through two managers, descriptor stamping,
uuid reclaim, growth + backpressure-then-fail, and factory selection.
The edge's H2D copies must complete before the ACK lets the producer
reclaim the slot. A blocking h2d-stream synchronize enforced that on the
host thread — serializing the transport tick on the slowest copy, which
compounds under concurrency. get_ready_tensors already polls a future
per pending edge, so the constraint moves onto the device timeline: one
CUDA event covers the batch (all copies queue on the h2d stream in
program order) and the edge reports ready when the event fires. No host
block; the wait_stream ordering for downstream kernels is unchanged.

The event path needs CUDA, so it rides the same c16-32 gate run as the
pinning claim; on CPU (and in CI) edges stay future=None as before.
An arena consumer already rejected file-producer descriptors explicitly;
the reverse (file consumer, arena producer) surfaced as a bare
FileNotFoundError on a path that never existed. Both directions now
raise the same explicit message: MSTAR_SHM_ARENA must match across the
deployment. Test covers both.
cpu torch + triton + numpy cover the tensor-transport import chain; the
job previously pinned the Step-1 communicator test only.
…dget

Review follow-ups on the arena's behavior under sustained heterogeneous
load:

- Fragmentation observability: the allocator (already a sorted,
  coalescing free-list) now exposes largest_free_block; SegmentedShmArena
  and the manager surface (total, free, largest, pinned) via stats().
  Growth logs the snapshot, and the exact fragmentation signature —
  a reserve failing while total free covers it — logs a warning naming
  the collapsed largest block.
- Graceful spill: at the segment cap, a send now backpressures briefly
  (MSTAR_SHM_ARENA_SPILL_AFTER_S, 0.05 s) and then stages the tensor
  through the per-uuid file protocol instead of failing — slower, never
  fails, the file transport's saturation behavior. Descriptors keep
  shm_segment=None for spilled tensors; the consumer reads those through
  a file fallback (which also makes a file-producer + arena-consumer
  deployment interoperate); reclaim unlinks spilled files.
  MSTAR_SHM_ARENA_SPILL=0 restores strict backpressure + timeout.
- Pinned budget: MSTAR_SHM_ARENA_PIN_MAX_MB (4096) caps TOTAL
  cudaHostRegister-ed bytes, distinct from the segment cap — pinned
  pages come out of the OS's pageable pool system-wide. Segments past
  the budget stay unpinned (copies work, no async overlap), as do
  oversized dedicated segments, whose one-shot transfer doesn't
  amortize the registration.
- Wake on read completion: start_read_tensors now returns a real Future
  (completed by a watcher thread when the h2d-stream event fires) so the
  worker's eventfd wakes the moment copies land, instead of an
  otherwise-idle worker discovering them on its next 10 ms poll tick.
- GIL released across the arena FFI's slow paths (reserve's
  segment-growth mmap, multi-allocation uuid frees).

Tests updated to the new contract (spill roundtrip + file reclaim +
stats gauge; strict mode still fails loudly; the mixed-transport test
now proves file->arena interop instead of a refusal). Docs: env vars
for the new knobs; installation's Rust section reframed as a running
component list; arena module docstring condensed.
Two saturation corners the spill tests didn't pin down: (1) a single
edge mixing arena-staged and spilled tensors — the consumer dispatches
per descriptor, one start_read_tensors call reads both; (2) the
fragmentation warning itself, constructed deterministically: fill to
the cap, free alternate slots (total free ample, largest block small),
then a reserve that fits the total but no block — asserts the warning
fires and the tensor spills while a small tensor still lands in a hole.
The lib-target comment explained the rlib in terms of specific future
consumers, and the crate docs described the crate as exactly its first
two modules. Both now say what is structurally true: one crate of
independent, individually opt-in components with a Python surface and a
plain-library form — new capabilities land as new modules.
…tats

- SegmentedShmArena gets interior mutability (segments behind a Mutex),
  so reserve takes &self: the binding releases the GIL across the
  growth path, and a &mut PyO3 borrow held there made any concurrent
  &self call (stats, free, free_uuid) raise "Already mutably borrowed".
  Growth now serializes on the segments lock instead of the PyCell
  borrow.
- cudaHostRegister goes through ctypes, which releases the GIL for the
  duration — registering a 256 MiB segment on the send path no longer
  stalls the process's other Python threads once per growth (the torch
  cudart binding holds the GIL). Torch-binding fallback kept for
  environments without a loadable libcudart.
- _CudaEventFuture uses Event(blocking=True): the default busy-waits,
  so the watcher thread turning events into wake futures burned a full
  core per wait.
- _wake_when_done guards its queue/thread creation with a lock (two
  concurrent first-callers could create two watchers and lose wakes).
- Oversized-segment pinning REVERSED per review: dedicated segments are
  reused for later large tensors, so an unpinned one degrades every
  subsequent transfer through it — they now pin like any segment,
  within the budget.
- Manager stats() renamed stats_summary() (the raw arena tuple keeps
  stats()), and under --log-stats the producer path logs the snapshot
  at most once per MSTAR_SHM_ARENA_STATS_INTERVAL_S (default 60 s), so
  a long soak leaves an occupancy/fragmentation time series in the
  logs.
Ceilings are per-entity and multiply across a node (every entity —
workers + the api-server data worker — creates its own arena, and a
consumer pins peer segments beyond its own): construction now fails
fast with the sizing formula when ONE entity's
MAX_SEGMENTS x SEGMENT_MB already exceeds /dev/shm (statvfs), warns
when it exceeds current free space, and warns when the per-process pin
budget is an outsized share of physical RAM. Both x(num entities)
formulas documented in the module docstring and env-vars table.

Rebased on main: register_for_send now receives TensorPointerInfos
directly; the arena override stamps the passed info as well as the
store-tracked ones, and the tests call with infos.
… out

Deep-review fixes (verified by reviewer across 7 models x 9 configs with
bitwise gates):

- Segment base names are now instance-unique
  (mstar_arena_{entity}_{pid}_{token}): a fixed per-entity name in the
  global /dev/shm namespace let a second server's create() truncate the
  first's LIVE segments (silent cross-server corruption, observed on a
  shared cluster) and made cross-user startups die on Permission
  denied. Wire-compatible: consumers open whatever name the descriptor
  carries. Test: two same-entity-id managers coexist and the first's
  staged data survives the second's creation.
- Startup sweep of orphaned segments: SIGKILL never runs Drop, leaving
  up to a full arena per kill until reboot. The pid embedded in the new
  names gives the sweep its liveness check (/proc/<pid>); files it
  cannot judge or cannot remove are left with a debug note. Test: a
  dead-owner file is reclaimed, a live-owner file is untouched.
- Spill grace defaults to 0: on a worker, the TENSOR_RECEIVED ACKs that
  free slots are processed by the same thread that would sit in the
  grace wait, so waiting was pure dead time per at-cap tensor (and
  strict mode a guaranteed stall-then-fail). Docs now state plainly
  that strict backpressure is only meaningful where another thread
  drains ACKs (the threaded api-server).
- The uuid-ledger arena API (reserve_for/free_uuid) had no Python
  users — removed along with its bindings; the module docs no longer
  describe a reclaim flow this PR does not implement. The
  _infos_by_uuid side-table is gone too (register_for_send stamps the
  infos it is passed).
… a leak)

Self-review follow-up: the consumer's peer-segment cache never evicted,
and the instance-unique naming fix turned that from stale-reuse into an
unbounded leak — every producer restart mints NEW segment names, so a
long-lived consumer mapped and pinned each generation while the old
unlinked-but-mapped segments' memory (and registered pinned bytes)
stayed resident forever.

Cached entries now record their pinned size and are evicted once the
backing /dev/shm file is gone (producer finished or restarted):
cudaHostUnregister via ctypes (GIL released), mapping dropped, pinned
accounting decremented. Eviction is gated on `pending` being empty —
an mmap must outlive any in-flight h2d copy reading from it, and the
pending futures are exactly those copies — and time-gated off the hot
path. Test: a consumer's cache entry disappears after its producer
cleans up and exits.
… gate

From a two-lens self-audit (concurrency; lifecycle/protocol) ahead of
the soak:

- Pin accounting was a torn read-modify-write: _pin releases the GIL
  (ctypes), so concurrent pins corrupted _pinned_bytes, and _peer_view's
  before/after delta could attribute another thread's bytes to the wrong
  segment (mis-eviction later). One _pin_lock now makes budget check +
  register + accounting atomic, and closes the check-then-insert race
  that let two threads map+pin the same peer segment (loser leaked).
- Eviction is now safe under threaded reads: gated on being the sole
  active reader (a second thread may have queued an async H2D whose
  future has not reached `pending` yet), and unregisters BEFORE
  unmapping — keeping the entry on unregister failure so accounting
  stays truthful.
- register_for_send can no longer orphan slots: an exception between
  reserve and the _arena_locs record frees the slot on unwind, and a
  concurrent-duplicate registration returns its slot instead of leaking
  it. The D2H copy falls back to blocking when no copy stream exists,
  so a descriptor can never ship ahead of its bytes.
- The wake watcher survives a cancelled/already-resolved future (its
  death silently downgraded every later wake to the poll tick), closes
  over only its queue (a bound method pinned the whole manager — and
  the arena's segments — for the daemon's lifetime), and close() stops
  it via sentinel.
- Double-ACK gate: a re-surfaced (retried) edge no longer re-ACKs —
  the double-decrement could free a slot while another consumer in the
  fanout still held the descriptor (silent wrong-bytes on the arena;
  loud failure on the file transport).
- Sibling arena bindings (free/stats/num_segments) release the GIL: a
  growth holds the segments mutex across an mmap, and blocking on it
  with the GIL held froze every Python thread for the duration.
- Growth warns when this entity's real segment bytes exceed 80% of
  /dev/shm (oversized dedicated segments grow past the static ceiling),
  and the periodic stats line carries live_slots/spill_files — the
  leak canary for deferred reclaim (aborts without ACKs) during soaks.
A request aborted after staging but before every consumer ACKs defers
reclaim forever (cleanup_request waits for ACKs that will never come).
MSTAR_SHM_ARENA_SLOT_TTL_S (default 0 = off) adds a bounded backstop:
slots older than the TTL are force-freed, with a loud warning carrying
the running total. Safety argument: a slot older than the REQUEST
timeout cannot have a legitimate reader — the request is dead by the
system's own contract — so a bound safely above it (recommend >= 2x the
request timeout) cannot race a real consumer. Reclaims run under
capacity pressure (retrying the reserve before spilling) and with the
periodic stats sweep.

Left off by default pending review discussion; precedent across the
ecosystem: vllm-omni's RDMA sender uses a 300 s TTL sweep as its abort
backstop (with an acknowledged TTL-vs-in-flight caveat), sglang's omni
pipeline a 600 s central watermark. The stronger long-term fix is
event-driven reclaim wired to the abort control message (the shape
sglang's LLM disaggregation uses), which touches shared
cleanup_request semantics and deserves its own change.
The removal of the stale conductor-driven-reclaim description cut
mid-sentence; the header now states the actual contract (embedder frees
on consumer ACKs), matching the SegmentedShmArena docstring.
Soak finding 1 (one leaked slot per completed request on the LLM
worker): my double-ACK gate keyed suppression by uuid alone, so a
tensor legitimately consumed by TWO nodes with staggered readiness had
its second reference's ACK suppressed — the producer's fanout refcount
(which correctly counted both) never reached zero. The gate now keys by
(uuid, edge name, destination node): a re-DELIVERED edge repeats its
triple and stays suppressed (the corruption the gate exists for), while
a distinct consumer of the same uuid ACKs normally; partial edges ACK
only their fresh infos, and the per-request triple set is dropped with
the request. Test drives both cases through the real ACK plumbing.

Soak finding 2 (worker segments survive graceful shutdown): workers
exit with the manager still referenced — no cleanup path runs, the
interpreter never collects it, and the Rust Drop that unlinks never
fires (the api entity has an explicit cleanup, which is why only its
segments disappeared). A weakref.finalize callback — capturing only
the mutable segment-path list, never the manager — now unlinks at
interpreter shutdown regardless. Test proves it from a subprocess that
exits holding a global reference to the manager.
Root cause of the soak's one-slot-per-request leak, and it is neither
the abort deferral nor the ACK gate — it is pre-existing shared
bookkeeping that the arena's live_slots canary made visible for the
first time:

`set_output_ref_counts` counts `routing.persist` in the fanout, so a
persisted tensor carries one reference PER PERSIST EDGE on top of its
persist flag. Nothing ever released those references. Clearing the flag
(unpersist, or request cleanup) left the count above zero, so `can_gc`
stayed false and the tensor was never collectable — `cleanup_request`
logged "Deferring cleanup ... awaiting TENSOR_RECEIVED ACK" and never
revisited. BAGEL's end-of-prefill token is both emitted and persisted,
which is exactly one leaked slot per request. The file transport leaks
the same way (a per-uuid /dev/shm file per persisted tensor), which is
likely the manual cleanup maintainers have been doing for a while.

Fix: the manager tracks the persist references it holds (one per
`set_persist(True)` — the same edges the fanout counted) and releases
them when the flag clears, in step with the flag that was set alongside
them. `cleanup_request` now clears via `set_persist` for the same
reason. Regression tests cover both the request-end path and the
unpersist-then-consume re-route path.

Second finding (worker segments surviving shutdown): SIGTERM — what
`Conductor.shutdown`'s `p.terminate()` sends — defaults to immediate
death, so no unwinding, no atexit, no finalizers, and the segments were
never unlinked. The main process gets this for free from SIGINT ->
KeyboardInterrupt, which is why only the workers leaked. The worker
target now turns SIGTERM into SystemExit, and shutdown escalates to
SIGKILL after the join window so a worker stuck in a C call cannot hang
teardown (its segments are then reclaimed by the next start's sweep).

Also: document the ACK gate's known limitation (a graph routing the
same tensor to the same node under the same edge name twice in one
request is indistinguishable from a re-delivery), and `ruff --fix`.

test/modular failure set is byte-identical to upstream/main before and
after (45 pre-existing GPU-dependent failures on this box); arena suite
15/15.
…guard

@NSagan271's mstar-project#183 is the canonical fix for the persisted-tensor leak,
in the conductor/worker domain: it stops counting routing.persist in
set_output_ref_counts at the source, leaving the persist flag + the
conductor's unpersist-with-K accounting to balance the references.

My previous persist-hold mechanism here (tracking references per
set_persist and releasing them on flag clear) was a second, redundant
fix for the same bug — and once mstar-project#183 removes the count, it would
over-dereference and free persisted tensors early. Reverted, so mstar-project#168
makes no changes to the shared persist / ref-count semantics and
rebases cleanly onto mstar-project#183.

Also dropping the double-ACK guard from the self-audit: as NSagan271
noted, a persisted tensor legitimately re-sent to the same node across
graph walks looks identical to a re-delivery, so the guard could
suppress a real ACK and undercount against the conductor's K — exactly
what mstar-project#183's accounting must not lose. Removing it restores the
pre-audit ACK behavior the conductor logic is designed around.

Kept: the worker SIGTERM -> SystemExit graceful-exit handler and the
SIGKILL-after-join escalation in Conductor.shutdown (matches the code
NSagan271 verified), which is what lets the arena's exit finalizer run
and unlink worker segments. The three tests tied to the reverted
shared code are removed with it; arena suite green.
@npuichigo

Copy link
Copy Markdown
Contributor Author

@NSagan271 done

@NSagan271
NSagan271 merged commit f09d662 into mstar-project:main Jul 29, 2026
2 checks passed
stephen-dwq pushed a commit that referenced this pull request Jul 31, 2026
…2) (#168)

* shm arena: tensor transport over persistent mmapped segments (Step 2)

Replaces the per-tensor file open/write/read/unlink in
SharedMemoryCommunicationManager with a Rust segmented /dev/shm arena
(rust/src/shm.rs: persistent mmaps + first-fit allocator, buffer-protocol
views for zero-copy staging).

- Producer reserves (segment, offset) per tensor and D2H-copies straight
  into the mapped segment on the dedicated copy stream; one stream sync
  covers the batch before the control message ships.
- The location rides the existing descriptors (TensorPointerInfo grows
  optional shm_segment/shm_offset) — no wire-shape change.
- Consumer opens the named segment once, views the bytes zero-copy, and
  H2D-copies on its dedicated stream; the stream is synchronized before
  ACKing, since the producer reclaims the slot on ACK (the file
  transport's f.read() made that implicit).
- Pinning: each mapped segment is cudaHostRegister-ed once per process,
  both sides (MSTAR_SHM_ARENA_PIN, default on) — copies through the side
  streams then run at page-locked bandwidth and stay asynchronous, which
  is what preserves the copy/compute overlap the side streams exist for.
  Measured on the standalone bench: 256 MiB x20 D2H = 466 ms pageable
  mmap vs 187 ms registered (== torch pinned); registration is a one-time
  ~21 ms per 256 MiB segment.
- Capacity: the arena grows by segments up to
  MSTAR_SHM_ARENA_MAX_SEGMENTS (mappings never move — registrations and
  open consumer views stay valid; oversized tensors get a dedicated
  segment). At the cap, sends backpressure until consumers ACK, failing
  loudly after MSTAR_SHM_ARENA_FULL_TIMEOUT_S.
- Reclaim is uuid-keyed on the sender (cleanup maps uuid -> free), same
  lifecycle as the file transport's unlink.
- Selection: MSTAR_SHM_ARENA = 0 (default, files) / 1 / AUTO on the SHM
  protocol, mirroring MSTAR_RUST_ZMQ; documented in
  docs/environment_variables.rst.

Tests: cargo tests for the arena/allocator; pytest covers the
producer->consumer roundtrip through two managers, descriptor stamping,
uuid reclaim, growth + backpressure-then-fail, and factory selection.

* arena: defer the consumer ACK on a CUDA event instead of a host sync

The edge's H2D copies must complete before the ACK lets the producer
reclaim the slot. A blocking h2d-stream synchronize enforced that on the
host thread — serializing the transport tick on the slowest copy, which
compounds under concurrency. get_ready_tensors already polls a future
per pending edge, so the constraint moves onto the device timeline: one
CUDA event covers the batch (all copies queue on the h2d stream in
program order) and the edge reports ready when the event fires. No host
block; the wait_stream ordering for downstream kernels is unchanged.

The event path needs CUDA, so it rides the same c16-32 gate run as the
pinning claim; on CPU (and in CI) edges stay future=None as before.

* arena: fail loudly on transport mismatch in both directions

An arena consumer already rejected file-producer descriptors explicitly;
the reverse (file consumer, arena producer) surfaced as a bare
FileNotFoundError on a path that never existed. Both directions now
raise the same explicit message: MSTAR_SHM_ARENA must match across the
deployment. Test covers both.

* ci: run the full test/rust suite (the arena tests were not exercised)

cpu torch + triton + numpy cover the tensor-transport import chain; the
job previously pinned the Step-1 communicator test only.

* docs: drop migration-step jargon from arena module docs

* arena: fragmentation gauge, spill-to-file at the cap, pinned-bytes budget

Review follow-ups on the arena's behavior under sustained heterogeneous
load:

- Fragmentation observability: the allocator (already a sorted,
  coalescing free-list) now exposes largest_free_block; SegmentedShmArena
  and the manager surface (total, free, largest, pinned) via stats().
  Growth logs the snapshot, and the exact fragmentation signature —
  a reserve failing while total free covers it — logs a warning naming
  the collapsed largest block.
- Graceful spill: at the segment cap, a send now backpressures briefly
  (MSTAR_SHM_ARENA_SPILL_AFTER_S, 0.05 s) and then stages the tensor
  through the per-uuid file protocol instead of failing — slower, never
  fails, the file transport's saturation behavior. Descriptors keep
  shm_segment=None for spilled tensors; the consumer reads those through
  a file fallback (which also makes a file-producer + arena-consumer
  deployment interoperate); reclaim unlinks spilled files.
  MSTAR_SHM_ARENA_SPILL=0 restores strict backpressure + timeout.
- Pinned budget: MSTAR_SHM_ARENA_PIN_MAX_MB (4096) caps TOTAL
  cudaHostRegister-ed bytes, distinct from the segment cap — pinned
  pages come out of the OS's pageable pool system-wide. Segments past
  the budget stay unpinned (copies work, no async overlap), as do
  oversized dedicated segments, whose one-shot transfer doesn't
  amortize the registration.
- Wake on read completion: start_read_tensors now returns a real Future
  (completed by a watcher thread when the h2d-stream event fires) so the
  worker's eventfd wakes the moment copies land, instead of an
  otherwise-idle worker discovering them on its next 10 ms poll tick.
- GIL released across the arena FFI's slow paths (reserve's
  segment-growth mmap, multi-allocation uuid frees).

Tests updated to the new contract (spill roundtrip + file reclaim +
stats gauge; strict mode still fails loudly; the mixed-transport test
now proves file->arena interop instead of a refusal). Docs: env vars
for the new knobs; installation's Rust section reframed as a running
component list; arena module docstring condensed.

* test: mixed arena+spill edge; deterministic fragmentation signature

Two saturation corners the spill tests didn't pin down: (1) a single
edge mixing arena-staged and spilled tensors — the consumer dispatches
per descriptor, one start_read_tensors call reads both; (2) the
fragmentation warning itself, constructed deterministically: fill to
the cap, free alternate slots (total free ample, largest block small),
then a reserve that fits the total but no block — asserts the warning
fires and the tensor spills while a small tensor still lands in a hole.

* docs: frame the rust/ crate as general components, not transport-only

The lib-target comment explained the rlib in terms of specific future
consumers, and the crate docs described the crate as exactly its first
two modules. Both now say what is structurally true: one crate of
independent, individually opt-in components with a Python surface and a
plain-library form — new capabilities land as new modules.

* arena: round-3 review fixes — borrow, GIL, event, race, pin policy, stats

- SegmentedShmArena gets interior mutability (segments behind a Mutex),
  so reserve takes &self: the binding releases the GIL across the
  growth path, and a &mut PyO3 borrow held there made any concurrent
  &self call (stats, free, free_uuid) raise "Already mutably borrowed".
  Growth now serializes on the segments lock instead of the PyCell
  borrow.
- cudaHostRegister goes through ctypes, which releases the GIL for the
  duration — registering a 256 MiB segment on the send path no longer
  stalls the process's other Python threads once per growth (the torch
  cudart binding holds the GIL). Torch-binding fallback kept for
  environments without a loadable libcudart.
- _CudaEventFuture uses Event(blocking=True): the default busy-waits,
  so the watcher thread turning events into wake futures burned a full
  core per wait.
- _wake_when_done guards its queue/thread creation with a lock (two
  concurrent first-callers could create two watchers and lose wakes).
- Oversized-segment pinning REVERSED per review: dedicated segments are
  reused for later large tensors, so an unpinned one degrades every
  subsequent transfer through it — they now pin like any segment,
  within the budget.
- Manager stats() renamed stats_summary() (the raw arena tuple keeps
  stats()), and under --log-stats the producer path logs the snapshot
  at most once per MSTAR_SHM_ARENA_STATS_INTERVAL_S (default 60 s), so
  a long soak leaves an occupancy/fragmentation time series in the
  logs.

* arena: per-entity ceiling checks + rebase to the new register_for_send

Ceilings are per-entity and multiply across a node (every entity —
workers + the api-server data worker — creates its own arena, and a
consumer pins peer segments beyond its own): construction now fails
fast with the sizing formula when ONE entity's
MAX_SEGMENTS x SEGMENT_MB already exceeds /dev/shm (statvfs), warns
when it exceeds current free space, and warns when the per-process pin
budget is an outsized share of physical RAM. Both x(num entities)
formulas documented in the module docstring and env-vars table.

Rebased on main: register_for_send now receives TensorPointerInfos
directly; the arena override stamps the passed info as well as the
store-tracked ones, and the tests call with infos.

* arena: instance-unique names, orphan sweep, immediate spill, dead API out

Deep-review fixes (verified by reviewer across 7 models x 9 configs with
bitwise gates):

- Segment base names are now instance-unique
  (mstar_arena_{entity}_{pid}_{token}): a fixed per-entity name in the
  global /dev/shm namespace let a second server's create() truncate the
  first's LIVE segments (silent cross-server corruption, observed on a
  shared cluster) and made cross-user startups die on Permission
  denied. Wire-compatible: consumers open whatever name the descriptor
  carries. Test: two same-entity-id managers coexist and the first's
  staged data survives the second's creation.
- Startup sweep of orphaned segments: SIGKILL never runs Drop, leaving
  up to a full arena per kill until reboot. The pid embedded in the new
  names gives the sweep its liveness check (/proc/<pid>); files it
  cannot judge or cannot remove are left with a debug note. Test: a
  dead-owner file is reclaimed, a live-owner file is untouched.
- Spill grace defaults to 0: on a worker, the TENSOR_RECEIVED ACKs that
  free slots are processed by the same thread that would sit in the
  grace wait, so waiting was pure dead time per at-cap tensor (and
  strict mode a guaranteed stall-then-fail). Docs now state plainly
  that strict backpressure is only meaningful where another thread
  drains ACKs (the threaded api-server).
- The uuid-ledger arena API (reserve_for/free_uuid) had no Python
  users — removed along with its bindings; the module docs no longer
  describe a reclaim flow this PR does not implement. The
  _infos_by_uuid side-table is gone too (register_for_send stamps the
  infos it is passed).

* arena: evict dead peer segments (instance-unique names made the cache a leak)

Self-review follow-up: the consumer's peer-segment cache never evicted,
and the instance-unique naming fix turned that from stale-reuse into an
unbounded leak — every producer restart mints NEW segment names, so a
long-lived consumer mapped and pinned each generation while the old
unlinked-but-mapped segments' memory (and registered pinned bytes)
stayed resident forever.

Cached entries now record their pinned size and are evicted once the
backing /dev/shm file is gone (producer finished or restarted):
cudaHostUnregister via ctypes (GIL released), mapping dropped, pinned
accounting decremented. Eviction is gated on `pending` being empty —
an mmap must outlive any in-flight h2d copy reading from it, and the
pending futures are exactly those copies — and time-gated off the hot
path. Test: a consumer's cache entry disappears after its producer
cleans up and exits.

* arena: adversarial-audit fixes — races, leaks, watcher lifecycle, ACK gate

From a two-lens self-audit (concurrency; lifecycle/protocol) ahead of
the soak:

- Pin accounting was a torn read-modify-write: _pin releases the GIL
  (ctypes), so concurrent pins corrupted _pinned_bytes, and _peer_view's
  before/after delta could attribute another thread's bytes to the wrong
  segment (mis-eviction later). One _pin_lock now makes budget check +
  register + accounting atomic, and closes the check-then-insert race
  that let two threads map+pin the same peer segment (loser leaked).
- Eviction is now safe under threaded reads: gated on being the sole
  active reader (a second thread may have queued an async H2D whose
  future has not reached `pending` yet), and unregisters BEFORE
  unmapping — keeping the entry on unregister failure so accounting
  stays truthful.
- register_for_send can no longer orphan slots: an exception between
  reserve and the _arena_locs record frees the slot on unwind, and a
  concurrent-duplicate registration returns its slot instead of leaking
  it. The D2H copy falls back to blocking when no copy stream exists,
  so a descriptor can never ship ahead of its bytes.
- The wake watcher survives a cancelled/already-resolved future (its
  death silently downgraded every later wake to the poll tick), closes
  over only its queue (a bound method pinned the whole manager — and
  the arena's segments — for the daemon's lifetime), and close() stops
  it via sentinel.
- Double-ACK gate: a re-surfaced (retried) edge no longer re-ACKs —
  the double-decrement could free a slot while another consumer in the
  fanout still held the descriptor (silent wrong-bytes on the arena;
  loud failure on the file transport).
- Sibling arena bindings (free/stats/num_segments) release the GIL: a
  growth holds the segments mutex across an mmap, and blocking on it
  with the GIL held froze every Python thread for the duration.
- Growth warns when this entity's real segment bytes exceed 80% of
  /dev/shm (oversized dedicated segments grow past the static ceiling),
  and the periodic stats line carries live_slots/spill_files — the
  leak canary for deferred reclaim (aborts without ACKs) during soaks.

* arena: TTL backstop for abort-orphaned slots (default off)

A request aborted after staging but before every consumer ACKs defers
reclaim forever (cleanup_request waits for ACKs that will never come).
MSTAR_SHM_ARENA_SLOT_TTL_S (default 0 = off) adds a bounded backstop:
slots older than the TTL are force-freed, with a loud warning carrying
the running total. Safety argument: a slot older than the REQUEST
timeout cannot have a legitimate reader — the request is dead by the
system's own contract — so a bound safely above it (recommend >= 2x the
request timeout) cannot race a real consumer. Reclaims run under
capacity pressure (retrying the reserve before spilling) and with the
periodic stats sweep.

Left off by default pending review discussion; precedent across the
ecosystem: vllm-omni's RDMA sender uses a 300 s TTL sweep as its abort
backstop (with an acknowledged TTL-vs-in-flight caveat), sglang's omni
pipeline a 600 s central watermark. The stronger long-term fix is
event-driven reclaim wired to the abort control message (the shape
sglang's LLM disaggregation uses), which touches shared
cleanup_request semantics and deserves its own change.

* docs: repair the garbled reclaim sentence in the shm module header

The removal of the stale conductor-driven-reclaim description cut
mid-sentence; the header now states the actual contract (embedder frees
on consumer ACKs), matching the SegmentedShmArena docstring.

* arena: fix the soak's two findings — ACK over-suppression, exit unlink

Soak finding 1 (one leaked slot per completed request on the LLM
worker): my double-ACK gate keyed suppression by uuid alone, so a
tensor legitimately consumed by TWO nodes with staggered readiness had
its second reference's ACK suppressed — the producer's fanout refcount
(which correctly counted both) never reached zero. The gate now keys by
(uuid, edge name, destination node): a re-DELIVERED edge repeats its
triple and stays suppressed (the corruption the gate exists for), while
a distinct consumer of the same uuid ACKs normally; partial edges ACK
only their fresh infos, and the per-request triple set is dropped with
the request. Test drives both cases through the real ACK plumbing.

Soak finding 2 (worker segments survive graceful shutdown): workers
exit with the manager still referenced — no cleanup path runs, the
interpreter never collects it, and the Rust Drop that unlinks never
fires (the api entity has an explicit cleanup, which is why only its
segments disappeared). A weakref.finalize callback — capturing only
the mutable segment-path list, never the manager — now unlinks at
interpreter shutdown regardless. Test proves it from a subprocess that
exits holding a global reference to the manager.

* Fix the persisted-tensor reference leak and worker SIGTERM cleanup

Root cause of the soak's one-slot-per-request leak, and it is neither
the abort deferral nor the ACK gate — it is pre-existing shared
bookkeeping that the arena's live_slots canary made visible for the
first time:

`set_output_ref_counts` counts `routing.persist` in the fanout, so a
persisted tensor carries one reference PER PERSIST EDGE on top of its
persist flag. Nothing ever released those references. Clearing the flag
(unpersist, or request cleanup) left the count above zero, so `can_gc`
stayed false and the tensor was never collectable — `cleanup_request`
logged "Deferring cleanup ... awaiting TENSOR_RECEIVED ACK" and never
revisited. BAGEL's end-of-prefill token is both emitted and persisted,
which is exactly one leaked slot per request. The file transport leaks
the same way (a per-uuid /dev/shm file per persisted tensor), which is
likely the manual cleanup maintainers have been doing for a while.

Fix: the manager tracks the persist references it holds (one per
`set_persist(True)` — the same edges the fanout counted) and releases
them when the flag clears, in step with the flag that was set alongside
them. `cleanup_request` now clears via `set_persist` for the same
reason. Regression tests cover both the request-end path and the
unpersist-then-consume re-route path.

Second finding (worker segments surviving shutdown): SIGTERM — what
`Conductor.shutdown`'s `p.terminate()` sends — defaults to immediate
death, so no unwinding, no atexit, no finalizers, and the segments were
never unlinked. The main process gets this for free from SIGINT ->
KeyboardInterrupt, which is why only the workers leaked. The worker
target now turns SIGTERM into SystemExit, and shutdown escalates to
SIGKILL after the join window so a worker stuck in a C call cannot hang
teardown (its segments are then reclaimed by the next start's sweep).

Also: document the ACK gate's known limitation (a graph routing the
same tensor to the same node under the same edge name twice in one
request is indistinguishable from a re-delivery), and `ruff --fix`.

test/modular failure set is byte-identical to upstream/main before and
after (45 pre-existing GPU-dependent failures on this box); arena suite
15/15.

* Defer the persist-leak fix to #183; drop the double-ACK guard

@NSagan271's #183 is the canonical fix for the persisted-tensor leak,
in the conductor/worker domain: it stops counting routing.persist in
set_output_ref_counts at the source, leaving the persist flag + the
conductor's unpersist-with-K accounting to balance the references.

My previous persist-hold mechanism here (tracking references per
set_persist and releasing them on flag clear) was a second, redundant
fix for the same bug — and once #183 removes the count, it would
over-dereference and free persisted tensors early. Reverted, so #168
makes no changes to the shared persist / ref-count semantics and
rebases cleanly onto #183.

Also dropping the double-ACK guard from the self-audit: as NSagan271
noted, a persisted tensor legitimately re-sent to the same node across
graph walks looks identical to a re-delivery, so the guard could
suppress a real ACK and undercount against the conductor's K — exactly
what #183's accounting must not lose. Removing it restores the
pre-audit ACK behavior the conductor logic is designed around.

Kept: the worker SIGTERM -> SystemExit graceful-exit handler and the
SIGKILL-after-join escalation in Conductor.shutdown (matches the code
NSagan271 verified), which is what lets the arena's exit finalizer run
and unlink worker segments. The three tests tied to the reverted
shared code are removed with it; arena suite green.
t-avil added a commit to t-avil/mstar that referenced this pull request Jul 31, 2026
Brings in mstar-project#168 (SHM tensor transport over Rust arena), mstar-project#195 (KeyError in
eager batched execution for packed-output submodules), mstar-project#190 (client stream
decode), mstar-project#187 (ruff pin).

Conflict in mstar/engine/kv_cache_engine.py: both sides fix the same missing
per-rid KeyError in the batched-logits sampling path. Kept our resolution,
which is a superset — it slices [padded_bs, V] logits to the real request
count, clones to break FlashInfer's reused output-buffer alias, honors
MSTAR_SLIM_SAMPLE, and re-runs unpack_packed_outputs for packed sentinels —
and grafted upstream's once-per-(node, walk) diagnostic warning for rids a
submodule gated out, which our version was handling silently.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012fTosh9fKWQuGLYcHT2h1m
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.

3 participants