Release v0.5.0 - #63
Merged
Merged
Conversation
…e 200 ms poll Two defects, each with a scenario test that fails before the fix: - A latched (TRANSIENT_LOCAL) publisher in an idle process never replayed to late joiners. The top-of-wait registry check scavenged its context from the first subscription/service/client in the wait set, so a wait set holding only guard conditions -- the shape a publish-only node's executor produces -- got a null context and skipped the check entirely. The wait set now stores its context at rmw_create_wait_set. - The 200 ms poll bound that made replay work at all broke the rmw_wait timeout contract: every wait returned RMW_RET_TIMEOUT at 200 ms regardless of the caller's deadline (a 600 ms wait returned at 200 ms, and an infinite wait, which must never time out, returned TIMEOUT), and every process woke 5x/s while idle. The poll is gone. Each context binds a doorbell socket at rmw_init, registered as ENTRY_DOORBELL before the first generation snapshot so no mutation can fall into the gap between the snapshot and the wiring. Every registry mutation sends one octet to every registered doorbell strictly AFTER bumping the generation, and rmw_wait drains its doorbell strictly BEFORE reading the generation. That ordering pair makes a lost wakeup impossible: a mutation either lands in the generation the waiter is about to read, or leaves a datagram queued on a level-triggered fd. A doorbell-only wake re-checks the registry and re-blocks for the caller's remaining time, so RMW_RET_TIMEOUT surfaces only at the caller's own deadline and an infinite wait blocks until a real event. No new threads, no registry layout change. Best-effort edge cases, each reproduced before being handled: - AF_UNIX datagrams stay charged to the sender until the receiver consumes them, so one participant that never drains could exhaust the ring socket's budget and silently mute wakeups to healthy peers. The ring fd is recreated on EAGAIN and the send retried once; on a fresh fd, EAGAIN can only mean the destination's own queue is full, i.e. a wakeup is already pending there. - The slot type is re-validated inside the seqlock window, so a slot recycled to a data endpoint mid-scan cannot receive the wake octet. - The ring socket is thread-local and closed at thread exit, so ringing takes no lock and leaks no fd. Cleanup needs no new machinery: the doorbell is a PID-owned slot holding a socket path, so graceful shutdown removes it like any endpoint and the existing stale-PID reaper reclaims it after a crash (slot teardown unlinks the file). Known limit: a build that predates this change bumps the generation but never rings, so a fleet running mixed builds can miss wakeups during the upgrade window -- upgrade together, as with the shm payload flag. The dead ctx->graph_guard_condition trigger is deliberately left untouched: wiring the per-node graph guard conditions rcl actually waits on is a separate defect with its own fix. Full suite green on Jazzy (128 tests). Replay reaches a late joiner ~20 ms after it joins; a 600 ms wait now blocks the full 600 ms.
- Wait mechanism summary and wait-sequence steps 2-4: the wait set carries its context, the doorbell fd is armed with the entity fds, and step 4 blocks with no internal poll interval and honors the caller's deadline. - New subsection: the doorbell -- why an AF_UNIX datagram socket over the alternatives, the ring-after-bump / drain-before-read ordering pair that makes lost wakeups impossible, the best-effort edge cases, crash cleanup via the existing reaper, and the two accepted limits. - Registry slot state: ENTRY_DOORBELL, and the generation bump is now followed by the ring. - Operational requirements: /tmp/ros2_uds holds live sockets and must be exempted from tmp cleaners. - Limitations: notification requires a thread inside rmw_wait.
devel is now the integration branch (contributor PRs land there before main), so it needs the same CI coverage.
…hange rmw_node_get_graph_guard_condition returns the per-node guard condition created in rmw_create_node, and that is the object rclcpp's GraphListener waits on. The only trigger in rmw_wait fired ctx->graph_guard_condition instead -- a context-level field that is declared and never assigned anywhere, so the branch was dead code and the object rcl waits on was never triggered. The loop above it had an empty body and a comment claiming the node triggers its own guard condition, which nothing does. Consequence: Node::wait_for_graph_change() never woke, so everything built on graph events was blind to endpoints appearing after it started -- wait_for_service, wait_for_publisher-style helpers, and rosbag2's topic discovery, which re-reads the topic list only when a graph event fires. A recorder started before its publishers subscribed to nothing and recorded zero messages, while plain subscribers on the same topics worked, because normal pub/sub declares interest through the registry up front and never needs a graph event. That asymmetry is why this went unnoticed: everything works except the watch-the-graph machinery. Fix: the context keeps a mutex-guarded list of every node's graph guard condition. rmw_create_node appends as its last step, so no earlier failure path has to undo it; rmw_destroy_node removes the entry under the same mutex before destroying the guard condition, so the trigger loop can never fire a freed object. When rmw_wait observes a registry generation change it triggers every guard condition in the list. The dead context-level field, now orphaned, is removed. A wait set holding only guard conditions -- GraphListener's exact shape -- is woken by the registry doorbell, re-runs the generation check, triggers its own guard condition and reports it ready, so this works with no data traffic on the domain at all. Fixes #40. Full suite green on Jazzy (129 tests). The new test blocks on the node's graph guard condition alone and asserts the wait wakes with it ready when a subscription appears: it fails against the parent commit (times out at 3 s, never triggered) and passes here, firing ~1 ms after the graph change.
- Which guard condition the graph change wakes: the old wiring-gap text is replaced by the per-context list of per-node guard conditions that is now actually triggered, and how a guard-condition-only wait set makes progress. - The graph guard condition: the per-node object rcl waits on is the object rmw_wait triggers; the known-seam paragraph is gone with the seam. - Wait sequence step 2 triggers every node's graph guard condition.
The test passed even with the doorbell wake path deleted, so it was not testing what it claimed. The fixture's own rmw_create_node leaves an unconsumed registry generation edge behind, so the waiter's first rmw_wait ran the generation check, triggered the graph guard condition, and found it already ready at the top-of-wait check -- returning before it ever blocked. That proved only that a pending edge triggers the guard condition, not that a graph change arriving while a wait is blocked wakes it, which is the property rosbag2's discovery loop actually depends on. The test now consumes pending edges until a wait genuinely blocks and times out, then asserts the blocked wait woke with the guard condition ready well before its 3 s timeout AND that it really had been blocked across the 200 ms before the subscription was created. Verified three ways: fails on the parent commit (nothing triggers the guard condition), fails with register_fd(doorbell_fd) commented out (the wait now blocks to its full 3 s timeout instead of returning early), passes here.
…ion-events fix(rmw_wait): trigger the per-node graph guard conditions on graph change (rosbag2 late joiners)
Brings in "Use system clock to fill msgs timestamps (#45)". Conflict resolution in rmw_wait.cpp: #45 split now_ns() into steady_now_ns() (monotonic, for timeouts) and wall_now_ns() (system clock, for message timestamps), and patched the epoll block loop accordingly. That loop was replaced on devel by the doorbell work (#42), so #45's two hunks against the old deadline_ns/remaining_ms code no longer applied. Resolved by keeping devel's loop and applying #45's intent to it: - msg.received_timestamp_ns keeps #45's wall_now_ns() (a wall-clock timestamp is the point of #45; a steady-clock value is meaningless to a remote reader). - All three deadline computations in the doorbell loop use steady_now_ns() (caller_deadline_ns, the per-iteration remaining time, and the doorbell-wake deadline check) — timeout arithmetic must not be affected by clock steps. Full suite green on Jazzy (129 tests).
rmw_subscription_options_t::ignore_local_publications was discarded at creation, so a subscription that asked not to receive its own context's messages received them anyway. The rmw contract defines "local" as the same rmw_context_t, not merely the same process. Each context now draws a random 64-bit context_id in rmw_init, and UdsGid::generate() embeds it in the trailing 8 bytes of every GID. The GID is copied verbatim into WireHeader::gid on every datagram, so is_same_context() can answer "did this come from my own context?" from bytes already on the wire, with no extra traffic and no change to the wire format. The id comes from the kernel's entropy source rather than from getpid(): PIDs are unique only within a PID namespace, so two containers sharing a machine can observe the same pid at the same instant. The check runs in both drain paths - drain_subscription() for the take paths and drain_socket() for rmw_wait - and only when the subscription asked for it. Services and clients leave it off. DESIGN.md documents the GID composition and the context-identity mechanism.
…ached rmw_wait returned RMW_RET_TIMEOUT any time it found nothing ready, even when the caller's deadline was still far off. A drain can legitimately yield nothing: a subscription with ignore_local_publications set drops its own context's messages, and a shm descriptor whose sender is gone is dropped too. In both cases the socket wakes the epoll, the drain throws the message away, and the wait then claimed a timeout that had not happened. That broke two things. Finite waits were cut short, so callers that read a timeout as "nothing arrived in my window" - rclcpp::wait_for_message, WaitSet::wait - failed early. Infinite waits returned RMW_RET_TIMEOUT, which is meaningless when no deadline was given; rclcpp's GraphListener treats that code as fatal and aborts the process. The timeout and deadline are now worked out at function scope so the final return can check them. RMW_RET_TIMEOUT is returned only once the caller's own deadline has passed; otherwise the wait returns RMW_RET_OK with every entry set to null, which is the normal way to report a spurious wake and is what executors already handle. The wait is honest now but still returns as soon as it wakes, so a caller that waits once and gives up can still stop early. Sitting out the full deadline needs a retry loop around the drain and is left for a separate change. Two tests cover it: a finite wait must not report a timeout before its deadline, and an infinite wait must never report one. Both fail without this change. DESIGN.md's rmw_wait sequence is corrected to match - the timeout rule now sits in step 5 where it is enforced, and step 4 no longer claims a guarantee the doorbell re-block does not give.
…re_local_publications Feature: implement ignore local publications
… queued recv_from peeks first to size the buffer. A zero-length datagram makes that peek return 0 and the function bailed out without ever dequeuing it. MSG_PEEK does not consume, so the datagram stayed queued and the socket stayed readable forever, spinning every wait that polls that fd. Consume and drop it, matching how a runt datagram is already handled.
rmw_wait made three passes over every entity in the wait set on every call: drain every socket, re-arm every fd with epoll_ctl, then drain every socket again after epoll returned. For a wait set of 719 entities that is roughly 2150 syscalls per call, nearly all returning EAGAIN or EEXIST. epoll_wait already reports exactly which fds are ready, so use that: - The wait set remembers what each fd was armed for (UdsWaitSet::armed), keyed by fd and validated by a per-entity uid, so the arming pass costs no syscall in the steady state. The uid is needed because a fd number can be reused after its owner closed and the allocator may hand the new entity the same address, so (fd, pointer) cannot tell them apart. - Both full drain passes become one walk over ready_events. - Entities holding already-queued messages are still found by checking the internal queues, then confirmed with a single non-blocking epoll_wait so a message sitting in a socket is still reported in this call rather than the next one. A wake alone no longer ends the wait, only real progress does: an entity with data, or a guard condition that fired. A doorbell ring, or a datagram that drain_socket filters out (ignored local publication, foreign message type), leaves nothing ready and re-blocks for the caller's remaining time. Syscalls per call go from ~2150 to 1 + 2 per ready entity. Closes #51
query_all ran registry_cleanup_stale before every graph query, and it has 13
call sites, so rmw_get_node_names, rmw_count_publishers, rmw_count_subscribers,
rmw_get_topic_names_and_types and the rest all paid for it. The sweep walks
every live slot, copies 1164 bytes out of each and stats /proc/<pid> to decide
whether the owner is still alive.
Measured on one registry:
live entries one sweep via rmw_get_topic_names_and_types
1335 1.4 ms 2.9 ms
9235 21.3 ms 42.6 ms
12671 27.2 ms 54.4 ms
rmw_get_topic_names_and_types calls query_all twice, so it paid twice. Anything
polling the graph paid it several times a second whether or not the graph had
changed.
Reclaiming slots owned by dead processes is garbage collection and does not
belong on a read path. Sweep at most once per second per context instead,
guarded by a CAS so concurrent callers skip rather than queue up behind each
other.
No guarantee is weakened. A graph query could already return an entity whose
owner died just after the sweep that vetted it, so the result was never a
liveness statement; this only widens a window that was always open. The
full-registry path in registry_add still sweeps unconditionally, since there it
is the last resort before entity creation fails.
test_rmw_graph now compiles registry.cpp so it can seed the registry directly.
Closes #52
Replace push-based latched replay (publisher process woken to resend) 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 — the doorbell broadcast that melted 200-node launches (every registry mutation ringing every process, each wake rescanning the registry) is gone. Mechanism: - 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, plus the full 16-byte GID embedded and verified by the puller. Sparse ftruncate + per-record posix_fallocate: an idle latched publisher costs one 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. - 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. Overlap is deduped by a per-publisher watermark keyed on the full GID (context_id bytes make it pid-reuse-safe), checked in both drains before descriptor resolve. - Pulled records resolve descriptors through a pull-local reader cache, are filtered by durability (VOLATILE late joiners no longer receive history — DDS-correct; the old push replayed to every subscriber on the topic) and by ignore_local_publications, trimmed to queue depth, and enqueued before the subscription handle returns, so history always precedes live samples. - Doorbell: socket still bound at rmw_init, but the ENTRY_DOORBELL slot is registered lazily, on the first rmw_wait holding 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); failure retries next wait. Doorbell-slot mutations no longer ring (no consumers; K lazily-registering processes would otherwise mini-storm K^2/2 datagrams). - registry: 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. - Deleted: transient_local_pubs (+mutex), known_subscriber_paths, CachedMessage / heap message_cache, transient_local_publish's replay loop, and the whole wait-side TL replay block. Launch arithmetic at N=200 (verified against the doorbell regression analysis): doorbell datagrams drop from ~100,300 (19,900 of them from doorbell self-registration alone) to ~the number of graph-event consumers; wake-side registry copies from O(mutations x processes x slots) to zero for plain pub/sub processes. Replay latency improves from ~20 ms (doorbell wake) to synchronous at subscription creation. Tests: 14/14 suites green on jazzy. New: pull with no rmw_wait anywhere in the process, exactly-once under pull/live overlap, ignore_local filtering of pulled history, subscriber-churn redelivery. Rewritten to the new contract: shm-unavailable large latched payloads are not latched (was: inline heap fallback replayed by push); known_subscriber_paths pruning tests deleted with the machinery. Container note: the full suite needs --shm-size >= 1g (the 64 MB Docker default cannot hold the 38 MB registry plus payload rings). Mixed-build window: a new publisher no longer push-replays and an old subscriber never pulls, so latched replay across that 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 path still delivers).
…wired graph waits, test join guard Four findings from PR review, all confirmed against the code: - rmw_wait: the post-epoll subscription drain did not receive the replay watermark map, so a pull/live-overlap datagram arriving while the wait was blocked bypassed dedup and was delivered twice. Both subscription drain sites now pass the watermarks. - tl_ring_latch: live_desc_out/staged_out were set before the slot's posix_fallocate; on ENOSPC the durable segment was destroyed on return while the caller had already seen staged==true and would fan out a descriptor to an unlinked segment — silently losing the live large message. Outputs are now filled only after the record commits and the ring owns the segment. - rmw_wait: a graph-GC wait whose lazy doorbell registration failed (registry full) could block unbounded with no wakeup wiring, losing graph events forever. Such waits now bound their block at 200 ms (spurious-OK wake), degrading to a coarse retry loop until a slot frees and registration succeeds; a throttled warning surfaces the state. - test: the churn stress test's fatal assertions could return with the publisher thread still joinable, turning a test failure into std::terminate. Added a stop/join guard on every exit path. 14/14 suites green on jazzy.
…aging off the lock, ordered fan-out 21 confirmed findings from the independent adversarial diff review (29 agents, every finding verified against the code before counting). The blocker and the significant ones: - BLOCKER, watermark soundness: tl_ring_pull's slot scan is not a point-in-time snapshot — a publisher latching DURING the scan could produce a max-seq watermark covering a sample the scan missed, whose in-flight datagram the drains would then drop: silent loss. The pull now detects scan overlap (re-reads every pulled slot's seq after the pass); on overlap it keeps only the contiguous sequence prefix and lets everything above the first gap arrive as datagrams. Without overlap, a gap can only be a dead writer's poisoned slot (datagrams will never come), where keeping the partial history remains correct. - CRASH (introduced by the first review round's restructure, caught by the second): an over-cap latched payload whose durable staging failed fell through to the embed path and memcpy'd past the 1 KiB slot — and the mapping. The latch now rejects over-cap payloads without a descriptor. - Staging back off the lock: mid-size (>1 KiB) latched payloads were staged (shm_open/fallocate/mmap, ms-scale) INSIDE cache_mutex, and evicted segments were destroyed under it too — the deleted push code deliberately kept both outside. Restored: stage before the lock, destroy evicted after. - Fan-out ordering: the latch released cache_mutex before sending, so two threads publishing on one TL publisher could hit the wire in inverted sequence order and the watermark would then drop the older sample as a "duplicate". Latch + fence + refresh + fan-out now run under cache_mutex, matching the deleted push path's send serialization. - cached_generation could move backward (!= guard); now monotonic (>), making the stated "only ever advances" invariant actually true. - TL_RING_MAX_BYTES 1 MiB clamped the stock rosout profile (depth 1000 > 963 slots) and warned on every node; raised to 2 MiB (sparse — free for idle publishers). tl name now carries the full 32-bit counter plus the time salt (the truncated 16-bit form could alias a live same-process ring after 65k creations and unlink it). - rmw_init failure paths: dead registry_remove(-1) calls replaced with the unlink of the never-registered doorbell socket file. Tests: +5 (forged-duplicate watermark drop via raw sendto; two publishers one topic, both histories exactly once; mid-band 4 KiB latched + live byte-equality; depth-clamp + newest-suffix at 10000; TL_EMBED_CAP boundary at 1024/1025 with descriptor resolve). Stress test gained a round-0 publish gate (flake) and the post-close pull assertion now exercises the real ENOENT path (was vacuous through the cleared name). Suite: 14/14, run twice. DESIGN.md: /dev/shm budgeting for latched rings, the exact dedup bound (pulled-or-lapped, contiguous-prefix rule under overlap), and the mixed-build reaper leak note.
- WaitDoesNotStealTriggerOfGuardConditionNotWaitedOn: a GC armed earlier but absent from this call's array must keep its trigger (fails until the armed-dispatch gate lands). - RecvFromSkipsZeroLengthDatagramToRealMessage: junk must not end a drain with a real message queued behind it (fails until the recv_from restructure lands). - RecycledFdNumberIsRearmedForNewEntity: pins the armed-cache uid check, previously untested. - QueuedBacklogStillReportsSocketDataWithoutBlocking: pins the poll-only pass contract and its non-blocking property. - BoundedWaitUnderRegistryChurnTimesOutOnSchedule: pins the doorbell-only re-block loop under registry churn. - ResponseDeliveredThroughWaitOnClient: first coverage of the client arm of the wait dispatch.
…poll ADDs to polling The armed-fd cache survives across rmw_wait calls, is insert-only, and fds were never EPOLL_CTL_DEL'd — so the epoll dispatch loop would act on any fd ever armed in the wait set, whether or not this call's arrays contain its entity. A guard condition owned by another wait set had its eventfd consumed with the trigger recorded nowhere (a permanent lost wakeup for the owner: wait_for_service and GraphListener hang), a subscription's socket could be drained by a wait set not waiting on it, and under fd duplication (fork without exec, dup, SCM_RIGHTS) a destroyed entity's stale entry was dispatched as a use-after-free. Gate dispatch on membership in armed_this_call — the fds this call's arming pass touched. A ready fd outside the set is EPOLL_CTL_DEL'd and erased without dereferencing its entity: the DEL keeps a readable level-triggered fd from spinning the loop, and the erase re-arms the entity on the next call that waits on it (this also stops the map growing unboundedly under entity churn). Steady-state cost: one hash insert per entity per call, zero syscalls. register_fd also no longer swallows non-EEXIST epoll_ctl failures: with the unconditional pre-drain gone, a silently unarmed entity was permanently invisible under e.g. epoll watch exhaustion. Log the error, drain the entity directly each call, and bound the block at 200 ms so the drain recurs — the old degraded-but-correct polling, for only the failed fds. Also drain past a full 64-event batch in the poll-only pass, and correct the UdsWaitSet::armed safety comment: close() alone does not guarantee epoll removal while another descriptor references the open file description.
The zero-length cleanup consumed with a blind 1-byte recv issued after a separate, non-atomic peek. recv_from has no per-fd serialization, and the same socket is drained concurrently by the wait-side and take-side paths under a MultiThreadedExecutor: if a racing drain consumed the zero-length datagram between the peek and the discard, the 1-byte recv dequeued whatever real message was now at the head and silently destroyed all but its first byte. Let the n == 0 case fall through to the full-buffer consuming recv, which dequeues exactly one datagram whatever it turns out to be, and loop on consumed junk (zero-length and runt) so a single recv_from call returns the next real message or a genuinely empty socket. Returning false on consumed junk ended every drain loop with deliverable messages still queued behind the junk, costing one wake per junk datagram and 'nothing taken' rmw_take results with data queued.
Pre-existing (predates this branch): the consuming recv omitted MSG_TRUNC, so when a concurrent drain dequeued the peeked datagram and a larger one stood at the head, the kernel silently truncated it to the buffer sized for the smaller peek. The runt check passed and the size-mismatch branch clamped payload_len and returned true — delivering a truncated payload as if it were valid. With MSG_TRUNC the return value is the true datagram size; anything larger than the buffer is dropped with a throttled warning instead of queued as a prefix. Kept as its own commit so it can be cherry-picked to devel independently of the recv_from restructure.
Three review findings against GraphQueryThrottlesStaleCleanup: - Its negative assertion raced the 1 s throttle interval against the wall clock; under load or a sanitizer the gap between the two queries closes and the test flakes. Re-arm the throttle by storing stamps into last_cleanup_ns instead of racing it. - It asserted a machine-global negative on domain 99, which every fixture-based test binary in the package sweeps unthrottled at rmw_init — a live CI flake under parallel ctest. Give it a private domain (93) via a fixture domain override. - 'Once per second' was not actually pinned: 'once per process lifetime' satisfied both assertions. Add a third query with an aged stamp to assert the sweep resumes, and remove ghosts unconditionally so a failed assertion cannot strand DEAD_PID slots in the persistent segment.
rmw_init runs an unconditional registry_cleanup_stale but never dated it, so the process's first graph query — typically moments later, during node discovery, exactly when queries cluster — repeated the identical full stat-every-slot sweep the init just paid for. Conversely 0 is a legitimate steady_clock reading near boot, inverting the intended 'first query after a quiet period sweeps'. Stamping last_cleanup_ns right after the init sweep fixes both. Comment corrections from the review: the throttle is per context, not per process (multiple contexts per process are legal and sweep independently); the crash-detection latency the throttle introduces (<= one interval per polling context) is now stated; and the test_rmw_graph CMake note claimed registry.cpp keeps no per-process state when ring_doorbells holds a thread_local send-fd cache — the conclusion held, the premise did not. Behaviour note kept from the review: a CAS loser proceeds straight to registry_query and may observe dead-owner slots the in-flight sweep is about to reclaim; that is the pile-up removal working as intended.
The overlap re-check only revisited slots that produced a record, so a slot skipped as never-written (any ring that has not wrapped) or given up on mid-write was invisible to it. A writer committing into such a slot after the scan passed it, then into a slot still ahead of the scan, opens a sequence gap with overlapped left false: the subscriber's contiguous- prefix trim never runs, the watermark spans the gap, and the in-flight datagrams for the missing samples are deduped away and lost. Snapshot the seq every slot showed the scan and re-check all of them. False positives are free — with no gap the trim keeps everything. Reported by Copilot on #60.
max_seq was fixed before the enqueue loop, so a record whose descriptor could no longer be resolved — its slot overwritten and its durable segment unlinked between the scan and the mmap — was skipped while its sequence stayed under the watermark. At 1-64 KiB the same sample's live datagram is inline and deliverable, and dedup dropped it: the sample was lost, not lapped, despite the comment claiming otherwise. A watermark cannot express a hole, so it stops below the unresolved sequence and the rest of the history arrives as datagrams. Reported by Copilot on #60.
TL_RING_MAX_BYTES is 2 MiB, documented as 1 MiB. And a durable segment is posix_fallocate'd to its whole payload, so a retained 5 MiB sample costs 5 MiB, not "an inode and a couple of pages" — the budgeting note understated tmpfs sizing for over-cap latched samples. Reported by Copilot on #60.
…cleanup perf(rmw_graph): throttle the stale-slot sweep off the graph query path
…ady-fds Fix/wait drain only ready fds
rmw_wait.cpp conflicted (#57's armed-fd cache vs this branch's pull-based replay); resolved with the reconciliation reviewed and tested on integration/tl-pull-launch-fixes: watermarks passed to both subscription drains, the 200 ms bounded block covering unwired graph waits and unarmed fallback entities, and the armed_this_call dispatch gate. rmw_graph.cpp's sweep comment picks up the clause about the registry_add retry path this branch introduces. The resulting tree is byte-identical to integration/tl-pull-launch-fixes @ 85022eb.
feat(tl): pull-based TRANSIENT_LOCAL replay; interest-scoped doorbell
… 0.5.0 The repository had no changelog, so this adds one covering every tag, not just the new release. Entries are reconstructed from the commit bodies and tag annotations; 0.5.0 is the wait/wakeup release (doorbell wakeup, pull-based TRANSIENT_LOCAL replay, ignore_local_publications, graph guard conditions) and carries the upgrade notes, since a mixed-build fleet loses latched replay and registry wakeups during the rollout window. package.xml had said 0.1.0 since the first commit — it was never bumped for 0.2.0 through 0.4.1, so a built package reported a version five releases stale. README's Status block said 0.1.0 for the same reason, and still listed only Jazzy/Kilted/Rolling although Lyrical joined the CI matrix in #17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A confirmed teardown bug in registry.cpp can pass a non–NUL-terminated socket_path buffer to unlink()/shm_unlink(), risking out-of-bounds reads.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Release PR for v0.5.0 that brings devel into main, delivering the “wait/wakeup” redesign and associated transport/QoS fixes plus extensive new test coverage and documentation.
Changes:
- Replace
rmw_wait’s fixed polling behavior with an epoll-based doorbell wakeup path that preserves timeout semantics and avoids lost wakeups. - Rework TRANSIENT_LOCAL durability from push-based replay to pull-based replay via a per-publisher
/dev/shmTL ring with overlap dedup. - Implement/repair graph event triggering and
ignore_local_publications, and expand CI/docs/tests to cover the new contracts.
File summaries
| File | Description |
|---|---|
| rmw_unix_socket_cpp/test/test_transport.cpp | Adds regression tests for zero-length datagram handling in recv_from(). |
| rmw_unix_socket_cpp/test/test_shm_transport.cpp | Adds extensive TL ring create/latch/pull tests including overlap/poison cases. |
| rmw_unix_socket_cpp/test/test_rmw_wait.cpp | New/expanded end-to-end tests covering doorbell wakeups, timeouts, arming rules, and ignored-local behavior. |
| rmw_unix_socket_cpp/test/test_rmw_service_client.cpp | Adds a wait-driven client readiness test for response delivery. |
| rmw_unix_socket_cpp/test/test_rmw_qos.cpp | Updates/removes push-replay assumptions and adds broad pull-replay + watermark coverage. |
| rmw_unix_socket_cpp/test/test_rmw_pub_sub.cpp | Adds coverage for ignore_local_publications filtering on subscriptions. |
| rmw_unix_socket_cpp/test/test_rmw_graph.cpp | Adds a deterministic throttle test for stale-slot cleanup on graph queries. |
| rmw_unix_socket_cpp/test/test_base.hpp | Makes test domain configurable per fixture to avoid cross-test collisions. |
| rmw_unix_socket_cpp/src/types.hpp | Adds context identity, wait-set arming UIDs, doorbell fields, and TL dedup watermark state. |
| rmw_unix_socket_cpp/src/transport.cpp | Makes recv_from() consume/skip junk datagrams and detect truncation safely. |
| rmw_unix_socket_cpp/src/shm_transport.hpp | Introduces the TL ring API/types and constants for pull-based TRANSIENT_LOCAL replay. |
| rmw_unix_socket_cpp/src/shm_transport.cpp | Implements TL ring create/latch/pull/close and extends orphan cleanup for tl_ segments. |
| rmw_unix_socket_cpp/src/rmw_wait.cpp | Implements doorbell-based wakeups, correct arming/dispatch semantics, and timeout contract fixes. |
| rmw_unix_socket_cpp/src/rmw_subscription.cpp | Adds pull-based TL replay at subscription creation + ignore-local + replay dedup. |
| rmw_unix_socket_cpp/src/rmw_service.cpp | Updates GID generation to include context identity. |
| rmw_unix_socket_cpp/src/rmw_publisher.cpp | Creates TL cache pre-registration and latches at publish time with fence ordering and refreshed subscriber cache. |
| rmw_unix_socket_cpp/src/rmw_node.cpp | Publishes per-node graph guard conditions into the context list and removes them safely on destroy. |
| rmw_unix_socket_cpp/src/rmw_init.cpp | Generates per-context IDs, binds doorbell socket at init, and cleans it up correctly on fini/error paths. |
| rmw_unix_socket_cpp/src/rmw_graph.cpp | Throttles stale-slot sweeps off hot graph-query paths. |
| rmw_unix_socket_cpp/src/rmw_client.cpp | Updates GID generation to include context identity. |
| rmw_unix_socket_cpp/src/registry.hpp | Adds ENTRY_DOORBELL registry entry type. |
| rmw_unix_socket_cpp/src/registry.cpp | Rings doorbells after registry mutations and reclaims TL shm segments on stale-slot teardown. |
| rmw_unix_socket_cpp/package.xml | Bumps package version to 0.5.0. |
| rmw_unix_socket_cpp/DESIGN.md | Documents doorbell wakeups, pull-based TL replay, graph GC wiring, and context identity. |
| rmw_unix_socket_cpp/CMakeLists.txt | Builds test_rmw_graph with src/registry.cpp to seed registry state in tests. |
| README.md | Updates distro matrix and version string. |
| CHANGELOG.md | Adds a detailed changelog including 0.5.0 upgrade notes and behavior changes. |
| .github/workflows/ci.yml | Runs CI on both main and devel for pushes and PRs. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // latched-cache shm segment here (not a socket file), so a stale-PID reap | ||
| // reclaims the dead publisher's cache promptly instead of leaking it until | ||
| // the next rmw_init sweep. | ||
| if (path_copy[0] != '\0') { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Release PR for v0.5.0: merges
develintomain. 30 commits, 25 files, +3,639 / −622.This is the wait/wakeup release. Three things change shape:
rmw_waitpoll is gone. Each context binds a doorbell socket; everyregistry mutation sends one octet to every registered doorbell strictly after
bumping the generation, and
rmw_waitdrains its doorbell strictly beforereading the generation. That ordering pair makes a lost wakeup impossible. No new
threads, no registry layout change. The poll had also been silently breaking the
rmw_waittimeout contract — every wait returnedRMW_RET_TIMEOUTat 200 msregardless of the caller's deadline, and an infinite wait, which must never time
out, returned
TIMEOUT.writes each latched sample into a per-publisher shm ring at publish time; a
late-joining subscription reads that ring itself inside
rmw_create_subscription.The idle publisher is never woken, polled, or rung. This replaces the push
approach that was reverted in v0.4.1 because its doorbell broadcast melted
200-node launches.
rclcpp'sGraphListenerwaits on arefinally triggered (fix(rmw_wait): trigger the per-node graph guard conditions on graph change (rosbag2 late joiners) #43).
rmw_waithad been firingctx->graph_guard_condition,a context-level field that is declared and never assigned anywhere — dead code, so
the object
rclactually waits on was never triggered.Plus
ignore_local_publications(#49), which was discarded at creation, and a run oftransport-layer datagram fixes, two
rmw_waitperformance passes (#57, #58), and thefirst
CHANGELOG.mdthis repo has had.Areas of concern, in order:
correctness arguments in this release. Both are store-buffering arguments
(
ring-write → seq_cst fence → generation loadon the writer;registry_add → seq_cst fence → pullon the reader), and overlap is deduped by a per-publisherwatermark keyed on the full 16-byte GID. Worth a second pair of eyes.
skipped, and a watermark that ran past an unresolvable record — which suggests the
scan/overlap interaction is the subtlest part of the change.
wakeups. See Additional Information.
Individual fixes landed via #43, #49, #57, #58 and #60; each was reviewed on its own PR.
No single issue to close here.
Is this user-facing behavior change?
Yes, four of them:
rmw_waitnow honours the caller's deadline. Previously any wait returnedRMW_RET_TIMEOUTat 200 ms — a 600 ms wait returned at 200 ms, and an infinite waitreturned
TIMEOUT. Callers that (deliberately or accidentally) depended on wakingevery 200 ms will now block for as long as they asked to.
behaviour; the old push path replayed to every subscriber on the topic regardless of
its durability QoS. A VOLATILE subscription that had been receiving history will stop.
ignore_local_publicationsis now honoured. A subscription that asked not toreceive its own context's messages was receiving them anyway. "Local" means the same
rmw_context_t, per the rmw contract — not merely the same process.wait_for_service,rclcpp'sGraphListenerand rosbag2stop hanging on graph changes. Latched-replay latency also drops from ~20 ms (doorbell
wake) to synchronous at subscription creation.
Idle CPU also drops: processes no longer wake 5×/s.
How was this tested?
On this PR: the only code change is
CHANGELOG.md, thepackage.xmlversion bumpand the README Status block, so CI on this PR is the gate — and it now runs
jazzy, kilted, rolling and lyrical (Lyrical joined the matrix in #17, which is on
mainand reachesdevelthrough this PR's merge commit).On the constituent PRs, per their commit bodies: the doorbell change reports the full
suite green on Jazzy (128 tests), and #60 reports 14/14 suites green on Jazzy.
Tests added or substantially extended across the release — each of the two
rmw_waitdefects and each transport edge case was reproduced by a test that fails before the fix:
test/test_rmw_qos.cpptest/test_rmw_wait.cpp(new)test/test_shm_transport.cpptest/test_rmw_graph.cpptest/test_transport.cpptest/test_rmw_service_client.cpptest/test_rmw_pub_sub.cppNew TL coverage specifically: pull with no
rmw_waitanywhere in the process,exactly-once under pull/live overlap,
ignore_local_publicationsfiltering of pulledhistory, and subscriber-churn redelivery. Tests deleted with the machinery they covered:
the
known_subscriber_pathspruning tests.Not run locally for this PR — no local build or test run was performed on the release-prep
commit; it is documentation and metadata only.
Additional Information
Upgrade notes — upgrade the fleet together.
push-replays and a pre-0.5.0 subscriber never pulls, so latched replay between that
pair does not work during a rolling upgrade. Old publisher + new subscriber keeps
working: an empty
socket_pathskips the pull and the old push path still delivers.Same precedent as the shm payload flag.
doorbell bumps the generation but never rings.
Scale, at N=200 nodes (the regression this release exists to fix): doorbell datagrams
drop from ~100,300 — 19,900 of them from doorbell self-registration alone — to roughly the
number of graph-event consumers, because the
ENTRY_DOORBELLslot is now registeredlazily, on the first
rmw_waitholding a graph guard condition. Wake-side registry copiesgo from O(mutations × processes × slots) to zero for plain pub/sub processes.
package.xmlwas five releases stale. It had said0.1.0since the first commit andwas never bumped for 0.2.0 → 0.4.1, so a built package reported the wrong version; the
README Status block said
0.1.0for the same reason. Both now say0.5.0. Drop that hunkif the version is deliberately pinned.
develwas merged withmain, not rebased onto it.develcontains a back-merge ofmain(e4c1ba4), so a flattened rebase re-fights thenow_ns()→steady_now_ns()rename from #45 across roughly eight commits and discards the resolution that was already
reviewed and CI-tested. The merge is conflict-free —
ci.ymlauto-merges, taking Lyricalfrom
mainand keepingdevel's[main, devel]triggers.