feat(engine): a dropped bag is counted at its port, per link, and graph shows it - #2026
Merged
Conversation
…g was lost The consumer-side mailbox dropped its oldest entry silently: 378 blocks published reached the audio playback path as 78, indistinguishable from a healthy run. The queue element now carries the inbound link it arrived on, so an eviction is charged to the link whose bag was lost rather than the one whose push made room — exact under fan-in — and the destination's graph node shares those counts live through ProcessorMetrics, which graph already renders. frames_dropped becomes derived from the per-link counts rather than a hardcoded zero beside them. Passing over bags to reach the newest is the read policy working, not loss, and stays uncounted.
Fifteen tests named one by one into the engine-lib slice and its xtask mirror: a module name would silently take whatever is added to it later, and this slice is the only gate the counting has. The end-to-end half drives real iceoryx2 services, a real source's write_raw and a real destination's receive_pending, and reads the counts back off the graph node's rendering — the payload GET /api/graph serves.
Craftsmanship review round 1: - ProcessorMetrics::to_json took the counts lock twice, so frames_dropped and dropped_bags_by_link could disagree inside one rendered object. One snapshot now feeds both. - push_frame's pre-drain loop was subsumed by the retry loop below it (ArrayQueue::push hands the frame back only when full), and duplicated the evict-and-charge body. One eviction site now. - The counts publish moved inside wire_rust_dest, so the destination-side wiring the compiler op actually runs is what the test drives — the call site was uncovered before. - dropped_bag_count_by_inbound_link (a snapshot) and dropped_bag_counts_by_inbound_link (a live handle) differed by one letter; the snapshot is now dropped_bag_count_snapshot_by_inbound_link. - The test module's third copy of the channel-open boilerplate is now the one shared helper, and two unit tests subsumed by the render tests are gone.
Correctness review round 2: - ProcessorMetrics had no insert site anywhere in the tree, so drop counts give `metrics` its first-ever appearance on /api/graph. Rendering the four fields nothing writes would have put four permanent zeros there — a reader could not tell an idle processor from an uninstrumented one, and the wiring helper one function away already refuses exactly that for the helper case. Only frames_dropped and dropped_bags_by_link render; the tests lock the whole object so an uncomputed field cannot creep back. - The delivery-profile doc claimed the measured 78-of-378 loss 'is counted now' without qualification. Eviction at the mailbox is; loss at the iceoryx2 subscriber ring, which overflows while a consumer parked inside process() pumps no receive, is counted nowhere. Said plainly in both places. - The counter's lifetime is stated: cumulative for the life of one wiring, not of the link id — disconnect takes it, reconnect starts at zero.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds per-inbound-link Iceoryx2 mailbox eviction counters. It attributes subscriber evictions, exposes live counts through ChangesIceoryx2 drop accounting
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to Dropped-bag counts are exposed for application-process destinations, but helper-hosted processors still do not report those losses in the graph, leaving a bounded observability gap. The change is otherwise mergeable with explicit owner awareness that the helper-process case must be scoped or addressed separately. Sequence Diagram(s)sequenceDiagram
participant Iceoryx2Subscriber
participant InputMailboxesInner
participant PortMailbox
participant ProcessorMetrics
participant GraphNode
Iceoryx2Subscriber->>InputMailboxesInner: deliver frame with inbound-link counter
InputMailboxesInner->>PortMailbox: push_frame_from_inbound_link
PortMailbox->>ProcessorMetrics: record eviction for the evicted frame's link
ProcessorMetrics->>GraphNode: publish frames_dropped and dropped_bags_by_link
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 88.68% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 53 functions across 14 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…and that is locked Fable review round 3, non-gating. The absence was declared in the PR but asserted nowhere: rendering an empty map or a zero for a destination whose mailboxes live in another process would say 'this processor lost nothing', which the parent cannot know. The missing key is what makes it read as unanswered rather than healthy, so nobody closes the gap later with a zero. Also drops a stray whitespace run inside an assertion message.
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
1 participant
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.
Summary
PortMailbox::pushevicted its oldest entry silently, whatever profile the port declared — measured on the audio playback path as 378 blocks published, 78 delivered, indistinguishable from a healthy run. The mailbox's queue element now carries the inbound link it arrived on, so an eviction is charged to the link whose bag was lost, not the link whose push made room — exact under fan-in, because the tag rides the entry rather than the port. The destination's graph node shares those counts live throughProcessorMetrics, whichgraphalready renders.iceoryx2/dropped_bag_counters.rs—InboundLinkDroppedBagCounter(one link's cumulative count) andDroppedBagCountsByInboundLink(one destination's registry). A link's counter is minted when its subscriber binds, so a wired link that has lost nothing reports0rather than going missing, and is forgotten on disconnect.mailbox.rs— one eviction site;push_frame_from_inbound_linkfor the live path,push_frame_without_inbound_link_attributionfor the manual-injection path (route, SDK e2e harness + unit tests only), which has no link to name.input.rs—PortBoundSubscribercarries its link's counter;receive_pendingpushes with it;remove_channel_linkforgets it.open_iceoryx2_service_op.rs—wire_rust_destpublishes the counts onto the destination's graph node as its last step.delivery_profile.rsand two built-in comments — corrected where this change falsified them.newestis untouched. Passing over bags to reach the latest is the read policy working, not loss, andpop/read paths are unchanged.Closes
Closes #2023
Exit criteria
graph, exact per link, and the sum equals published minus delivered.test_a_declared_port_carries_no_type_key_under_any_spellingstill passes — port rendering untouched.test.yml's slice and thextaskmirror, individually.Test plan
14 new tests, all in the engine-lib CI slice (67 pass locally, 6/6 repeated runs). The
test.ymllist and thextaskmirror are identical, checked programmatically.orderedport at depth 2; A×5 then B×5 reports{L-first: 5, L-second: 3}andcounted + delivered == published. Charging the pushing link would report{3, 5}— flipping it in the tree makes the test red, verified.write_raw, the destination's ownreceive_pending; the counts are read back off the node'sserialize_components(), which is byte-exactly whatProcessorNodeOutput::fromputs on/api/graph. Deleting the publish step fromwire_rust_destmakes it red, verified.0for every wired link;pop_latestpassing over bags counts nothing; a disconnected link's count leaves with it; a helper-placed destination's node carries nometricskey at all — inserting a default one makes it red, so the gap in note 1 cannot be closed later with a zero.Gates:
cargo xtask check-all-source-gates11/11, clippy clean on every touched file, fmt clean, rustdoc clean, wheel pytest 349 passed, stubtest and pyright clean, full local battery 44 pass / 0 fail.Notes for owner
1. The plan entry is not fully met, and
/ship-changemust not fold it in as delivered. ARCHITECTURE.md's "No loss is silent" entry is unconditional, but a helper-placed processor — which is every Python processor — counts its evictions in its own process and the parent holds no handle on them (iceoryx2_input_mailboxes_innerreturnsNonefor a subprocess host). Such a node deliberately carries nometricskey rather than a false zero. Surfacing it needs a parent↔helper report path the plan does not state; escalate is child-initiated request/reply, so this is a genuine design call, not wiring. I flagged this before starting and you said proceed — recording it here so the ship doesn't quietly mark the entry done. My recommendation: either the plan gains that mechanism, or thegraphclause is scoped to app-process destinations with the helper case named as an explicit OPEN.2. A second loss site survives uncounted. A consumer parked inside
process()pumps noreceive_pending, so the iceoryx2 subscriber ring overflows instead of the mailbox —enable_safe_overflow(true), counted nowhere. TheSpeakerSinkroom-wait is exactly that shape. This change counts the site the change file names and nothing else; the docs now say so in both places rather than claiming the 378-block gap is fully accounted for. Counting it would likely need a sequence number on the wire header (an IPC-ABI change) or an iceoryx2-side statistic — plan-level either way. Not filed as a ticket.3.
frames_droppedis now derived, and the uncomputed metric fields no longer render.ProcessorMetricshad no insert site anywhere in the tree, so this change givesmetricsits first-ever appearance on the wire. Renderingthroughput_fps, the two latencies andframes_processed— which nothing writes — would have served four permanent zeros a reader cannot distinguish from an idle processor. Onlyframes_dropped(the per-link counts summed, one snapshot so the two keys cannot disagree) anddropped_bags_by_linkrender. The change file says those placeholders "make no claim"; I read declining to publish them as honoring that rather than contradicting it, but it is your call.4. Counter lifetime differs from the change file's wording. The change file says the counter is "monotonic and cumulative"; as built, a count is cumulative for the life of one wiring — disconnect takes it with the link, and reconnecting the same id starts at zero, because a count outliving its link would name something
graphno longer has. Stated in the type doc. If you meant strictly monotonic across reconnects, that is a one-line change plus a change-file reconciliation.5. Ticket's "wheel test" became an engine test.
to_jsonis the byte-exactGET /api/graphpayload, and a wheel-level dropping pipeline is GPU-gated so it would run nowhere in CI. Worth knowing the two notes interact: the wheel case the ticket asked for is precisely the Python-destination case that renders nothing today (note 1).6. Two of these tests appeared to flake in the reviewer's
cargo test --workspacerun. I could not reproduce in 14 repeated runs (8× all iceoryx2 lib tests, 6× the exact CI slice), and one of the two is pure in-memory with no threads, no I/O and no shared state, so a genuine race is not representable. The workspace run was killed on the documented DRM-probe hang in the same binary, which reports in-flight tests as failures.7. A third review (Fable) returned APPROVE with zero findings. It independently verified the two things I was least sure of: the accounting is exact-once by construction (
push_framepops only on a real push failure,ArrayQueue::popyields each frame at most once, andrecord_evictionconsumes it, so neither double-count nor undercount is representable), and the fan-in test's{5, 3}is uniquely determined rather than accidentally order-dependent. Its non-gating nits are addressed except two I left alone deliberately:inspector.rs's bottleneck branch stays dead becauseframes_processedhas no writer anywhere (pre-existing, faithfully preserved), and frames still queued when a mailbox tears down vanish uncounted (nothing claims otherwise). It also notescargo clippy --testsfails crate-wide on ~275 pre-existingprintln!/eprintln!in test code — none in touched files, and CI lints default targets only, so it is not this PR's regression, but the debt is real.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests