Repository navigation
rewrite(phase5): l2.hpp — add FrameHandler wired to Phase 4 frame-level dispatch - #152
Merged
Merged
Conversation
…el dispatch Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Phase 5 (transport layer), Wave 1 (parallel with udp.hpp's equivalent fix in #151). ## The gap Server::serve() decoded every inbound frame straight into a MultiFrame (parsed acf::AcfEntry list) and called Handler once per already-decoded member -- the same shape as mock::Server::dispatch(), which never receives a stream_id at all. A caller wiring Handler to mock::Server::dispatch() can never reach Table 24 suppression (only reachable via the ten dispatch_<type>() wrappers' stream_id parameter), never reaches peek_conditional_request_type()/decode_and_dispatch() so conditional/cancellation-opcode requests get misread as ordinary Standard requests and executed immediately instead of queued, and never reaches dispatch_frame()/dispatch_frame_e2e() so E2E/fragment dispatch is unreachable too. ## Fix Added a namespace-scope FrameMemberResult (deliberately a parallel type to mock::Server::FrameMemberResult, not a shared one -- matching this file's own documented "parallel concrete types, no shared interface" convention) and a FrameHandler type receiving one whole frame's raw, unparsed ACF bytes plus its stream_id/sequence_num -- the exact shape mock::Server::dispatch_frame()/dispatch_frame_e2e() expect. Two new pure, socket-free helpers, decode_avtp_frame_header()/ decode_l2_frame_header(), split the AVTP envelope off without parsing the ACF payload, so serve() can hand raw bytes to a FrameHandler instead of re-encoding an already-parsed MultiFrame. set_frame_handler() takes priority over the pre-existing Handler when both are set; a member whose response.rsp == false (Table 24 suppression, a Queued/ Pending outcome, or a frame-level sequence-gate rejection) is dropped from the outgoing frame, and if every member was dropped, nothing is sent at all. The old Handler path is untouched byte-for-byte for backward compatibility with tests/l2_veth_roundtrip.cpp (a real-frames-over- real-veth-interfaces CI gate) -- this was a deliberate additive choice rather than replacing Handler's signature (the sibling udp.hpp fix in #151 widened Handler directly instead; both are valid shapes for the same underlying gap, no reconciliation needed since they're independent files). ## Bug found and fixed (c-RCP delta) rcp_l2_mac_is_unicast() (c-RCP src/l2.c:79-82, include/rcp/l2.h:121-137, REQ-L2-011) had no cpp-RCP equivalent anywhere -- added is_unicast_mac(). Confirmed via repo-wide search that regmap::writer_ctx()'s via_unicast parameter has no production caller yet (lifecycle.hpp's REQ-LIFECYCLE-027 unicast gate is data-model-only today) -- a real content gap worth closing now, but wiring it into a live dispatch path is a separate, broader gap, documented as out-of-scope for this batch. ## Verification Verified in a Docker/Ubuntu 24.04 container with CAP_NET_RAW/ CAP_NET_ADMIN and a real veth pair, replicating ci.yml's exact l2-veth job flags: clean build (0 errors, 0 warnings), full ctest 58/58, and l2_veth_roundtrip actually EXECUTED (not just compiled) against real AF_PACKET sockets, both before and after the change. A scratch program exercising the new FrameHandler path over the same real veth pair (multi-member dispatch plus a suppressed member producing no wire response) also passed. macOS build clean, 0/0 warnings, 58/58 ctest (l2_veth_roundtrip is Linux-only, gated separately -- confirmed absent from the macOS ctest registration as expected, re-confirmed by the "l2 veth round trip" CI job itself on this PR). include/rcp/l2.hpp: 808 -> 1069 lines. tests/test_l2.cpp: 244 -> 596 lines, 11 -> 21 TEST_CASEs (7 new for FrameHandler/Table-24/ conditional-opcode/multi-member/E2E wiring, 3 new for is_unicast_mac). .fusa-reqs.json: +2 entries (REQ-L2-009, REQ-L2-010). tests/l2_veth_roundtrip.cpp: unchanged by design (still uses Handler, exercising raw-socket round-trip fidelity, not dispatch semantics). No production call site wires l2::Server anywhere else in the tree (repo-wide grep confirmed), so no other call sites needed updating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
… FrameHandler calls in tests Fixes an MSVC /W4 /WX narrowing-conversion error (C2220/C4244) at two new test call sites (test_l2.cpp:525,572) that call FrameHandler's uint8_t sequence_num parameter directly with hdr.sequence_num (uint16_t) -- not caught by clang/gcc's warning sets on the other CI platforms. Test-only, no production code affected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
SoundMatt
added a commit
that referenced
this pull request
Aug 22, 2026
…, port deadlock fix (#154) * rewrite(phase5): shmem.hpp — rebuild Channel around a real bounded byte-level buffer Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Phase 5 (transport layer), Wave 2. A dedicated investigation (this session) found cpp-RCP's shmem.hpp was a decoded-object pass-through with no byte encoding at all and no capacity bound -- Channel::request() called a std::function handler directly and synchronously in the same process. c-RCP's rcp_shmem_avtp_pair_new() is a real byte-level bounded ring-buffer transport, real memcpy framing, RCP_ERR_BUSY backpressure when full -- but is ALSO in-process only under the hood (plain heap, non-pshared mutex/condvar). Neither codebase has ever had genuine cross-process shared memory; "shmem" means "a fast paired in-process queue" in both trees. Decision: rebuild Channel around a real byte-level bounded buffer (closing the two real gaps -- no capacity bound, codec never exercised) while keeping the existing decoded-object Handler signature at the boundary (matching udp::Server's own internal-codec/external- decoded-callback shape), so admin.hpp/config.hpp need zero changes. ## Ported (c-RCP src/shmem.c 266 lines / include/rcp/shmem.h 99 lines) - shmem.c:68-71 (RCP_ERR_BUSY on full queue) -> Channel's new detail::FrameSlots<kMaxQueueCapacity> bounded pool + ErrBusy return, REQ-SHMEM-006. - shmem.c:64-67/109-113 (closed-side send/recv RCP_ERR_CLOSED) -> Channel::request()'s existing closed_ check, now cited REQ-SHMEM-005/010. - The codec-never-exercised gap: Channel::request() now genuinely calls acf::encode_acf_abb/encode_acf_gbb and decode_acf_abb/ decode_acf_gbb (selection mirrors udp.hpp's encode_frame/decode_frame) on both the request and response legs before/after the Handler runs. - shmem.h:91-93's runtime queue_capacity parameter (clamped to min 1, shmem.c:204) -> Channel(stream_key, queue_capacity = kDefaultQueueCapacity), clamped into [1, kMaxQueueCapacity]. ## New shape Channel keeps its old Handler signature and synchronous, no-thread- of-its-own dispatch model exactly. Internally, request() now: encodes -> acquires a fixed-capacity slot (ErrBusy if none free) -> decodes back out of that slot -> calls Handler with decoded objects (handler copied out from under a separate mutex and invoked UNLOCKED, so concurrent callers' handlers genuinely run in parallel) -> encodes/ decodes the response -> releases the slot via RAII. Registry's public surface is byte-for-byte unchanged -- confirmed by reading all of admin.hpp/config.hpp/test_admin.cpp/test_config.cpp in full: they only use Registry::add_channel/deregister/lookup/channels/close and Channel::stream_key()/new_channel(), never Channel::request()'s internals. No changes needed to any of the four. ## Bug found and fixed mid-task (real concurrency bug) The original code held the handler mutex ACROSS the handler call itself, which would have serialized every concurrent request and deadlocked under the new capacity tests -- fixed by copying the handler out under the lock, then invoking it unlocked. ## Architecture note (documented in the header) Unlike c-RCP's genuinely two-party, strict-FIFO ring (two independent threads holding *out_a/*out_b), Channel::request() is both producer and consumer in one synchronous call, so the bounded buffer is a per-caller indexed slot pool, not a literal head/tail FIFO -- same capacity bound and BUSY-without-enqueuing contract, no independent- reader ordering to preserve since there's only ever one reader. ## REQ-SHMEM mapping / c-RCP citation drift found (not propagated) .fusa-reqs.json was split 2026-08-18 (issue #533) into REQ-SHMEM-001.. 010 (not 001..009). Found three req-id mis-citations in c-RCP's own src/shmem.c inline comments (shmem_side_send citing 007 instead of 006; shmem_side_recv wrongly also citing 006; shmem_side_destroy citing 007 instead of 008) -- c-RCP's own test_shmem.c citations are correct, only the .c file's comments drifted; not propagated here. REQ-SHMEM-004 (recv timeout) and REQ-SHMEM-007 (recv buffer-too-small) have no analog in the new cpp shape, documented as such rather than force-fit. ## Verification include/rcp/shmem.hpp: 173 -> 458 lines. tests/test_shmem.cpp: 170 -> 526 lines, test_shmem: 20 TEST_CASEs (68 assertions), re-run 5x back-to-back with no flakiness in the new thread/condition-variable- based backpressure tests. Full tree rebuilds clean (0 errors, 0 warnings), 58/58 ctest suites pass (100%) -- independently re-verified from a fresh build, not just the porting agent's own report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> * rewrite(phase5): admin.hpp — fixed-capacity subscriber/counter bounds, port deadlock fix Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Phase 5 (transport layer), final module. ## Ported (c-RCP src/admin.c 247 lines / include/rcp/admin.h 177 lines) - admin.c:109-110 (RCP_ADMIN_MAX_SUBSCRIBERS=16, RCP_ADMIN_MAX_COUNTERS=256) -> std::array<..., kMaxSubscribers>/std::array<..., kMaxCounters> bounds on Server's subscriber list and counter table, replacing cpp-RCP's unbounded std::vector/std::map. Matches this rewrite's fixed-capacity, no-dynamic-allocation-on-safety-relevant-paths architecture (already applied to server.hpp's admission queue and shmem.hpp's FrameSlots). - admin.c:133-147 (rcp_admin_server_subscribe, capacity check before add) -> Server::subscribe() returns AdminErrc::subscriber_capacity_exceeded once kMaxSubscribers is reached, matching c-RCP's fail-closed "cb not added" behavior (admin.h:145) rather than growing unbounded. - admin.c:173-198 (rcp_admin_server_record_counter, capacity check on a genuinely new counter name+labels pair) -> counter_capacity_exceeded once kMaxCounters distinct counters exist (admin.h:158), existing counters still update past the bound. - Ported c-RCP's two boundary tests (subscribe-at-capacity, record_counter-at-capacity) into Catch2 TEST_CASEs. ## Bug found and fixed (real concurrency bug, same class as shmem.hpp/#153) emit() (ported from rcp_admin_server_emit(), admin.c:149-171) held the subscriber-list mutex across every subscriber callback invocation -- a subscriber callback that itself calls back into Server (a realistic pattern for a metrics/event sink) would deadlock. Fixed by copying the subscriber list out under the lock, then invoking callbacks unlocked -- same fix shape as shmem.hpp's Channel::request() handler-invocation fix in #153. ## Deliberate non-port, documented in-file c-RCP's admin.c is caller-driven: rcp_registry_t was retired project- wide (per c-RCP's own history) and every admin_server_t call site now passes state in directly rather than looking it up from a live registry. cpp-RCP's admin.hpp instead holds a reference to shmem::Registry and looks up endpoint/channel state through it live. NOT migrated to c-RCP's caller-driven pattern: c-RCP's rationale for going caller-driven was retiring rcp_registry_t entirely, which doesn't apply here -- shmem::Registry is live, current, and still the single source of truth for channel state in this rewrite (see shmem.hpp, #153). Reasoning recorded in admin.hpp's own header comment, not just here. ## Verification include/rcp/admin.hpp: 128 -> 338 lines. tests/test_admin.cpp: 123 -> 200 lines. Full tree rebuilds clean (0 errors, 0 warnings) from a from-scratch rebuild against the fully-merged rewrite/v3-from-c-rcp base (picked up #151/#152/#153 via merge-forward first), 58/58 ctest suites pass (100%) -- independently re-verified, not just the porting agent's own report. ## Phase 5 status udp (#151), l2 (#152), shmem (#153) already merged; this closes out admin, the fourth and final "port from c-RCP" module in Phase 5's scope. tls/capi remain as re-verify-only per ROADMAP.md Phase 17 — confirmed clean against the new core, no content changes needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --------- Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
10 tasks
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.
Part of the v3.0.0 ground-up rewrite (#129, ROADMAP.md Phase 17), branch
rewrite/v3-from-c-rcp. Phase 5 (transport layer), Wave 1 (parallel with udp.hpp's equivalent fix in #151).The gap
Same class as udp.hpp:
Server::serve()decoded every frame into a parsedMultiFrameand calledHandleronce per already-decoded member — never receiving astream_id, so a caller wiring it tomock::Server::dispatch()could never reach Table 24 suppression, conditional/cancellation-opcode routing, or E2E/fragment dispatch.Fix (additive, not a signature replacement)
Added a new
FrameHandlertype receiving one whole frame's raw, unparsed ACF bytes plusstream_id/sequence_num— the exact shapedispatch_frame()/dispatch_frame_e2e()expect, via two new pure helpers that split the AVTP envelope without parsing the ACF payload.set_frame_handler()takes priority over the pre-existingHandlerwhen both are set. The oldHandlerpath is untouched byte-for-byte to preservetests/l2_veth_roundtrip.cpp's real-frames-over-real-veth CI gate.Bug found and fixed (c-RCP delta)
rcp_l2_mac_is_unicast()(REQ-L2-011) had no cpp-RCP equivalent anywhere — addedis_unicast_mac(). Confirmedregmap::writer_ctx()'svia_unicastparameter has no production caller yet; wiring it into a live dispatch path is a separate, broader gap, documented as out-of-scope here.Verification
Verified in a Docker/Ubuntu 24.04 container with real veth interfaces, replicating CI's exact flags:
l2_veth_roundtripactually executed (not just compiled) against real AF_PACKET sockets, both before and after — plus a scratch program exercising the newFrameHandlerpath over the same real veth pair. macOS build clean, 58/58 ctest.l2.hpp: 808→1069 lines.test_l2.cpp: 244→596 lines, 11→21 TEST_CASEs..fusa-reqs.json: +2 entries.