[benaliabderrahmane]

What

Replaces push-based TRANSIENT_LOCAL replay with a pull: the publisher writes each latched sample into a per-publisher shm cache at publish time, and a late-joining subscription reads that cache itself inside rmw_create_subscription. The idle publisher is never woken, polled, or rung.

This removes the doorbell broadcast that regressed 200-node launches (#42's every-mutation-rings-every-process, ~100k wake datagrams and the wake-side registry rescans), while keeping — and improving — the late-joiner guarantee #37/#42 fought for: replay is now synchronous at subscription creation, and works even for a publisher process that never enters rmw_wait (previously impossible).

How

• tl_ring (shm_transport): fixed ring of qos.depth per-record-seqlocked slots, created strictly before registry_add and named in the publisher slot's previously-unused socket_path field. Unique per-incarnation name (counter + time salt) so a recycled PID can never alias a dead publisher's cache; the full 16-byte GID is embedded in the header and verified by the puller. Sparse ftruncate + per-record posix_fallocate: an idle latched publisher costs an inode and a page; ENOSPC degrades to latch-less with a logged error (live delivery unaffected). Payloads > TL_EMBED_CAP (1 KiB) ride the existing durable segments as 32-byte descriptors; ring bytes capped at TL_RING_MAX_BYTES (2 MiB).
• Losslessness is a store-buffering fence pair: publisher does ring-write → seq_cst fence → fresh generation load → refresh → fan-out (sequence numbers assigned inside the latch critical section, so ring order == seq order); subscriber does registry_add → seq_cst fence → pull. A sample is either in a cache the pull observes, or its publisher observed the registration and datagram-delivers it; the overlap is deduped by a per-publisher watermark keyed on the full GID (context_id bytes make it pid-reuse-safe).
• Crash containment: per-record seqlocks — a publisher SIGKILLed mid-latch poisons exactly one slot, which the puller skips after bounded retries; subscription creation can never hang. teardown_slot shm-unlinks tl_-prefixed slot paths so the stale reaper reclaims a dead publisher's cache promptly; the orphan sweep gains a ros2_uds_tl_ prefix pass.
• Doorbell, interest-scoped: the socket is still bound at rmw_init, but the ENTRY_DOORBELL slot registers lazily, on the first rmw_wait whose set holds a graph guard condition — only graph-event consumers (wait_for_service, GraphListener, rosbag2) need registry wakeups now. Registration precedes that wait's generation check (register-before-snapshot, as rmw_init did). Doorbell-slot mutations no longer ring (no consumers; K lazily-registering processes would otherwise send K²/2 datagrams).
• Deleted: transient_local_pubs (+mutex), known_subscriber_paths, CachedMessage/heap message_cache, and the entire wait-side replay block — including the transient_local_pubs_mutex that was held across queries and replay sends, serializing publisher creation during storms.

Launch arithmetic (N=200, from the doorbell regression analysis)

Doorbell datagrams: ~100,300 → ~number of graph-event consumers (19,900 of the old count came from doorbell self-registration alone). Wake-side registry copies for plain pub/sub processes: → zero. Replay latency: ~20 ms after wake → synchronous at create.

Deliberate behaviour changes (release-note these)

1. VOLATILE late joiners no longer receive latched history. The old push replayed to every subscriber on the topic (no durability filter). The pull is durability-gated — the DDS-correct behaviour. Grep the fleet for VOLATILE subs on latched topics before deploying.
2. A latched payload published while /dev/shm is unavailable is not replayable (was: inline heap fallback that the push path could still replay). Live delivery is unaffected; the degradation is logged.
3. Mixed-build window: a new publisher no longer push-replays and an old subscriber never pulls, so latched replay across that exact pair is lost during a rolling upgrade — upgrade together (same precedent as the shm payload flag). Old publisher + new subscriber keeps working (empty socket_path skips the pull; the old push still delivers, and new binaries still ring old binaries' doorbells).

Testing

14/14 suites green on jazzy. New tests: pull with no rmw_wait anywhere in the process; exactly-once under pull/live overlap; ignore_local_publications filtering of pulled history; subscriber-churn redelivery; concurrent publish + churn stress (20 rounds, exactly-once + in-order + newest-depth suffix); poisoned-slot (odd seqlock) bounded pull. Rewritten to the new contract: shm-unavailable large latched payloads are not latched; known_subscriber_paths pruning tests deleted with the machinery.

CI/container note: the full suite needs --shm-size ≥ 1 GB (the 64 MB Docker default cannot hold the 38 MB registry plus the payload rings the large-message tests create).

Not yet done — recommended before deploying to the vessel: run the 200-node launch perf test against this branch to validate the storm elimination end-to-end.

Related

• Root-cause analysis of the doorbell launch regression (mutations × doorbells broadcast + wake-side rescans + cleanup-in-graph-query feedback).
• With this merged: Fix/wait drain only ready fds #57 (drain-only-ready) and perf(rmw_graph): throttle the stale-slot sweep off the graph query path #58 (throttle cleanup) remain valid and worth merging; perf(registry): ring doorbells from a cache instead of scanning every… #59 (cheap-doorbell-ring) should be closed — the ring set is now ~empty, and its REGISTRY_VERSION bump bought nothing.

[Copilot]

Pull request overview

Replaces publisher-driven TRANSIENT_LOCAL replay with synchronous subscriber-side shared-memory pulls and scopes doorbell registration to graph consumers.

Changes:

• Adds per-publisher shared-memory history rings and replay deduplication.
• Lazily registers graph doorbells to reduce launch-time broadcasts.
• Expands replay, failure, churn, and concurrency tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
DESIGN.md	Documents pull replay and scoped doorbells.
src/types.hpp	Adds replay-ring and watermark state.
src/shm_transport.hpp	Defines the latched-ring API and layout.
src/shm_transport.cpp	Implements ring creation, latching, pulling, and cleanup.
src/rmw_publisher.cpp	Latches samples before live fan-out.
src/rmw_subscription.cpp	Pulls history during subscription creation.
src/rmw_wait.cpp	Removes wait-side replay and lazily registers doorbells.
src/rmw_init.cpp	Defers doorbell registry registration.
src/registry.cpp	Scopes ringing and cleans stale replay caches.
test/test_shm_transport.cpp	Tests ring behavior and poisoned slots.
test/test_rmw_qos.cpp	Tests pull replay, deduplication, failure, and churn.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[benaliabderrahmane]

Confirmed and fixed in 0898d48 — the post-epoll drain now passes &sub->replayed_watermarks like the pre-block drain. Good catch: this was a real duplicate-delivery window for a datagram landing while the wait was blocked.

[benaliabderrahmane]

Confirmed and fixed in 0898d48 — live_desc_out/staged_out are now filled only after the record commits and durable_segs[slot] owns the segment, so an ENOSPC on the slot fallocate can no longer hand the caller a descriptor to an unlinked segment.

[benaliabderrahmane]

Confirmed; fixed in 0898d48 with a different remedy than an error return: RMW_RET_ERROR from rmw_wait is executor-fatal in rclcpp, so an unwired graph wait now bounds its block at 200 ms (spurious-OK wake) and retries registration on each re-entry until a registry slot frees, with a throttled warning. Liveness is preserved without killing the executor over a transient registry-full.

[benaliabderrahmane]

Confirmed and fixed in 0898d48 — added a stop/join RAII guard so a fatal assertion reports the failure instead of std::terminate-ing the test process.

[benaliabderrahmane]

Independent adversarial review completed (29 agents over the diff, four lenses, every finding verified against the code before counting): 21 confirmed findings, all addressed in ba57e60.

The two that mattered most:

• Blocker — watermark unsound under scan overlap: a publisher latching during the pull's slot scan could produce a watermark covering a sample the scan missed, whose in-flight datagram would then be dropped — silent loss. The pull now detects overlap (re-reads pulled slots' seq after the pass) and, on overlap, keeps only the contiguous sequence prefix, letting everything above the first gap arrive as datagrams. A gap without overlap can only be a dead writer's poisoned slot, where keeping the partial history stays correct.
• Crash introduced by the previous fix round: an over-cap latched payload whose staging failed fell through to the embed path and wrote ~100 KB past a 1 KiB slot (SIGSEGV under ctest, silent corruption when lucky). The latch now rejects over-cap payloads without a descriptor. This one is a direct lesson in why the review rounds run until clean — the fix for finding fix: replay TRANSIENT_LOCAL cache to late-joining subscribers #2 of round one created it.

Also in ba57e60: staging moved back off cache_mutex and evicted-segment destruction moved after it (the deleted push code's discipline, lost in the rewrite); fan-out now serialized under cache_mutex so concurrent publishes cannot invert sequence order into the watermark; monotonic cached_generation; TL_RING_MAX_BYTES→2 MiB so the stock rosout depth-1000 profile no longer clamps+warns on every node; full-width tl name counter; init failure-path unlink. +5 targeted tests (forged-duplicate watermark drop, two-publisher histories, mid-band 4 KiB, depth clamp, embed-cap boundary), suite 14/14 twice.

Remaining before merge, in my view: the 200-node launch benchmark against this branch — the storm-elimination claim is arithmetic until measured.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (4)

rmw_unix_socket_cpp/src/shm_transport.cpp:904

• Overlap detection rechecks only slots that produced an accepted record. A writer can finish a slot after it was skipped as empty/odd and then commit a later slot before the scan reaches it; no accepted slot changes, so overlapped stays false despite a gap. The subscription then installs max_seq and drops the missing sample's live datagram. Use a scan-wide writer epoch or snapshot/recheck every slot, including skipped slots, and ensure the watermark cannot span leading or internal gaps.

  if (overlapped_out) {
    for (const auto & [slot_i, s1] : accepted_at) {
      const auto * rec_hdr =
        reinterpret_cast<const ShmRecordHeader *>(area + slot_i * stride);
      if (rec_hdr->seq.load(std::memory_order_acquire) != s1) {

rmw_unix_socket_cpp/src/rmw_subscription.cpp:318

• max_seq is installed even when descriptor resolution skips a record here. If an over-cap slot is copied and then overwritten/unlinked before this mmap, the sample is not enqueued, but its sequence is still covered by the watermark; for a 1–64 KiB payload, the concurrently sent live datagram is inline and otherwise deliverable, so it is incorrectly dropped. Track only successfully delivered sequences without spanning resolution failures (likely exact sequences/ranges rather than one maximum).

          if (!rmw_uds::shm_resolve_incoming(
              pull_cache, ctx->domain_id, msg.header, msg.payload))
          {
            continue;  // large payload's segment already evicted: lapped

rmw_unix_socket_cpp/DESIGN.md:606

• The documented cap is 1 MiB here and in the PR description, but TL_RING_MAX_BYTES is implemented as 2 MiB in src/shm_transport.hpp:287. This understates the per-publisher /dev/shm budget; update the documentation and release note to the implemented 2 MiB value (or change the constant if 1 MiB was intended).

Losslessness rests on a store-buffering fence pair: the publisher does *ring-write → `seq_cst` fence → fresh generation load → refresh subscriber cache → fan out*, the subscriber does *registry_add → `seq_cst` fence → pull*. A sample therefore either lands in a cache the pull observes, or its publisher observes the subscriber's registration and delivers it as a datagram; the overlap is deduplicated by a per-publisher sequence watermark recorded at pull time (sequence numbers are assigned inside the latch critical section, so ring order equals sequence order and "at or below the watermark" exactly means "pulled or lapped"). Payloads above a small embed cap (`TL_EMBED_CAP`, 1 KiB) are staged once into the existing durable segments and the slot carries the 32-byte descriptor; the ring's byte footprint is capped (`TL_RING_MAX_BYTES`, 1 MiB), so an extreme `depth` replays the newest samples that fit — a stated, accepted limit. If the cache cannot be created or a slot cannot be committed (`/dev/shm` unavailable or full), the publisher runs latch-less with a logged error: live delivery is unaffected, late joiners get nothing — there is no heap fallback to replay from anymore. `depth` still controls this one publisher-side structure. Two consequences are deliberate: a VOLATILE late joiner no longer receives latched history (the pull is durability-gated — the DDS-correct behaviour, where the old push replayed to every subscriber on the topic), and replay from an already-dead publisher's still-mapped cache remains possible until its slot is reaped, a mild extension of DDS writer-lifetime semantics.

rmw_unix_socket_cpp/DESIGN.md:608

• The /dev/shm estimate for over-cap samples is incorrect: shm_stage_durable rounds and posix_fallocates the full payload size (src/shm_transport.cpp:267-300), so a retained 5 MiB sample consumes about 5 MiB, not “an inode and a couple of pages.” Budgeting guidance should include the sum of retained over-cap payload sizes (up to each publisher's effective depth), or deployments can substantially undersize tmpfs.

Three operational notes. **/dev/shm budget:** latched history that used to live on the heap now lives in tmpfs — budget roughly one latched ring per TRANSIENT_LOCAL publisher (sparse file up to `TL_RING_MAX_BYTES`; committed pages only for slots actually latched, so a chatty depth-1000 topic like `/rosout` converges toward ~1 MiB) plus one small durable segment (an inode and a couple of pages) per over-cap latched sample currently live in a ring. Docker's default 64 MB `--shm-size` is not enough at fleet scale: the registry alone is 38 MB — size the mount explicitly. **Dedup bound:** the per-publisher watermark claims only "pulled or already lapped out of the publisher's ring at pull time"; a subscriber that matches mid-burst may therefore not receive samples older than the newest ring-depth — exactly the history DDS would not owe it either — and when a publisher latches concurrently with the pull's scan, the pull keeps only the contiguous prefix of what it saw, letting the rest arrive as datagrams rather than risk the watermark swallowing a missed sample. **Mixed-build reaping:** an old binary's stale-slot reaper does not know the tl_ prefix, so it zeroes a dead new-binary publisher's slot without unlinking the cache segment; the leak is bounded and reclaimed by any new-binary process's `rmw_init` sweep.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

rmw_unix_socket_cpp/DESIGN.md:606

• The documented 1 MiB cap contradicts TL_RING_MAX_BYTES = 2 * 1024 * 1024 in src/shm_transport.hpp:287 (and the source comment explicitly says 2 MiB is required for depth-1000 rosout). Update this budget and the PR release note to 2 MiB, or restore the implementation to the advertised 1 MiB limit.

Losslessness rests on a store-buffering fence pair: the publisher does *ring-write → `seq_cst` fence → fresh generation load → refresh subscriber cache → fan out*, the subscriber does *registry_add → `seq_cst` fence → pull*. A sample therefore either lands in a cache the pull observes, or its publisher observes the subscriber's registration and delivers it as a datagram; the overlap is deduplicated by a per-publisher sequence watermark recorded at pull time (sequence numbers are assigned inside the latch critical section, so ring order equals sequence order and "at or below the watermark" exactly means "pulled or lapped"). Payloads above a small embed cap (`TL_EMBED_CAP`, 1 KiB) are staged once into the existing durable segments and the slot carries the 32-byte descriptor; the ring's byte footprint is capped (`TL_RING_MAX_BYTES`, 1 MiB), so an extreme `depth` replays the newest samples that fit — a stated, accepted limit. If the cache cannot be created or a slot cannot be committed (`/dev/shm` unavailable or full), the publisher runs latch-less with a logged error: live delivery is unaffected, late joiners get nothing — there is no heap fallback to replay from anymore. `depth` still controls this one publisher-side structure. Two consequences are deliberate: a VOLATILE late joiner no longer receives latched history (the pull is durability-gated — the DDS-correct behaviour, where the old push replayed to every subscriber on the topic), and replay from an already-dead publisher's still-mapped cache remains possible until its slot is reaped, a mild extension of DDS writer-lifetime semantics.

[benaliabderrahmane]

Confirmed and fixed in 9fa4bdc — this was a real losslessness hole, and the reproduction is worse than "an empty slot": any slot the scan produces no record for is invisible to the re-check, which includes both the never-written slots of a ring that hasn't wrapped and the ones abandoned after the bounded odd-seqlock retries.

Concrete failure, depth-8 ring: samples 1–2 sit in slots 0–1, the scan passes slots 2–5 as never-written, the writer then fills slots 2–7 with samples 3–8, and the scan accepts slots 6–7. The pull returns {1,2,7,8} — no accepted slot moved, so overlapped stays false, the subscriber's contiguous-prefix trim never runs, the watermark installs at 8, and the in-flight datagrams for samples 3–6 are all deduped away. Four samples lost.

Fix is your second suggestion: the scan now records the last seq it observed in every slot, skipped ones included, and re-checks all of them. False positives are free — with no gap the trim keeps everything, so over-reporting only ever costs replay depth, never a sample. The header contract was also stating the weaker invariant ("any pulled slot's seq moved"), which is exactly what let this survive review; it now states the one the watermark actually needs.

TlRingPullFlagsOverlapWhenASkippedSlotIsFilled pins it and fails on ba57e60. It forces the interleaving rather than racing for it: both threads share one CPU, so the latching thread can only run when the scan yields — which it does at a poisoned slot, after it has already passed the empty ones.

[benaliabderrahmane]

Confirmed and fixed in 5a730f3. The comment on that continue asserted the sample was "lapped", and that premise is what made the skip look free — it isn't. A resolution failure means the slot was overwritten and its segment unlinked between the scan and the mmap, which says nothing about the wire: at 1–64 KiB the live datagram carries the payload inline and is fully deliverable, so covering that sequence with the watermark loses the sample outright rather than deduplicating it.

I did not go to exact sequence sets. A scalar watermark cannot express a hole, so it now stops below the unresolved sequence and the rest of the pulled history arrives as datagrams instead — the same tradeoff the overlap trim already makes, and it keeps the dedup structure a single integer per publisher. The cost is bounded: with the sibling fix in 9fa4bdc, a resolution failure implies a concurrent overwrite, so the truncated history is history the publisher is actively lapping anyway.

ReplayWatermarkStopsAtAnUnresolvableRecord pins it — three over-cap latched samples, their durable segments unlinked before the subscription's pull, asserting the watermark lands at 0 and not 3. Fails on ba57e60.

[benaliabderrahmane]

Third review round — Copilot's findings on ba57e60, all four fixed

Both posted comments were real, and both were losslessness defects rather than style points. Fixed in 9fa4bdc (overlap detection blind to skipped slots) and 5a730f3 (watermark covering a record the resolve skipped), each with a regression test that fails on ba57e60. Details in the inline replies.

Also fixed here, from Copilot's suppressed comments on the same head — these never appeared as posted comments, so they went unanswered for a day:

• DESIGN.md / PR description: the ring cap was documented as 1 MiB, implemented as 2 MiB (TL_RING_MAX_BYTES, shm_transport.hpp:287). Corrected in both (1c0e527 + this description).
• The /dev/shm budgeting note understated over-cap samples. It called each durable segment "an inode and a couple of pages", but shm_stage_durable posix_fallocates the whole page-rounded payload — a retained 5 MiB sample costs 5 MiB. At fleet scale that is the difference between a correctly and a badly sized tmpfs mount, so the note now says to add the sum of retained over-cap payloads.

Why the review process missed these

Worth recording, because two of the four are the kind of defect a normal review reliably misses:

1. A "no new comments" review header hid four findings. Copilot's first pass on ba57e60 reported zero new comments with four in a collapsed Suppressed comments block. Suppressed findings exist only in the review body — they never reach pulls/{n}/comments — so polling that endpoint gives a false all-clear. They have to be read out of pulls/{n}/reviews.
2. The prior review verified the invariant as written, not the one the code needs. The overlap contract said "if any pulled slot's seq moved, a writer latched during the scan" — true, and confirmed as true. The property the watermark actually rests on is "a gap in the pulled records implies overlapped", which is strictly stronger and was false. The contract comment now states the latter.
3. The overlap detector's true-branch had no test at all. test_shm_transport.cpp contained no threads; both existing assertions were EXPECT_FALSE(overlapped). A mechanism whose entire purpose is to fire during a concurrent write had only ever been exercised with no concurrent writer. The new test forces the interleaving deterministically (shared CPU, so the writer runs only inside the scan's yield at a poisoned slot) rather than racing for it.

Full 14-suite ctest green on this branch and on integration/tl-pull-launch-fixes, which carries the same three commits.