Repository navigation
rewrite(phase5): udp.hpp — wire Server::Handler to Phase 4 frame-level dispatch - #151
Merged
Merged
Conversation
…l 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. ## The gap udp::Server::Handler was shaped to match mock::Server::dispatch()'s single-already-isolated-message contract, so Server::serve() decoded each datagram into individual ACF messages and called Handler once per message. A caller wiring that Handler straight to mock::Server:: dispatch() -- the obvious, natural thing to do -- never reached mock.hpp's Phase 4 batch D2 frame-level entry points (dispatch_frame()/ dispatch_frame_e2e()), silently losing Table 24 response suppression, conditional/cancellation-opcode routing, and E2E CRC verification/ fragment reassembly for every request that goes over UDP. Note: c-RCP's current src/udp.c/include/rcp/udp.h have been re-architected into a pure raw-AVTPDU transport (send/recv only) with no dispatch wiring at all -- that concern is deliberately deferred to a future layer per udp.h's own comment. This gap is purely internal to cpp-RCP's own design (where udp::Server bakes in decode+dispatch+ encode), not a delta versus a current c-RCP dispatch implementation. ## Fix Widened Server::Handler to a frame-level contract: using Handler = std::function<size_t(size_t client, avtp::StreamId stream_id, uint8_t sequence_num, const std::vector<uint8_t>& acf_frame, std::vector<FrameResponse>& out_responses)>; acf_frame is the raw ACF-region bytes -- exactly the shape mock:: Server::dispatch_frame()/dispatch_frame_e2e() accept. New FrameResponse type mirrors the response half of mock::FrameMemberResult without udp.hpp needing to #include mock.hpp. serve() now hands raw bytes straight through and treats FrameResponse::info.rsp == false as "no wire message for this member" (Table 24 suppression/queued/pending/ etc.), skipping it from the reply MultiFrame; if every member was suppressed, no reply datagram is sent at all (previously a reply was always sent). Chose widening Handler's own signature over keeping the old single-message shape as a parallel "compatibility path" -- two wiring surfaces would keep inviting the exact mistake this gap describes; one correct surface is safer. Both the real POSIX Server and the Windows stub Server were updated identically. ## Call sites tests/test_udp.cpp: every existing handler lambda ported to the new signature. include/rcp/tls.hpp needed no change (SecureServer::Handler is just `using Handler = udp::Server::Handler;`, follows automatically -- confirmed via rcp_tls ctest pass). ## Other findings No other behavioral deltas found versus c-RCP's current udp.c/udp.h -- its Annex J wrap/unwrap and control port already match cpp-RCP's existing encode_annexj_datagram/decode_annexj_datagram/ kAnnexJControlPort, left untouched. Documented (via a new test) that regmap::RequestStreamConfig::rx_resp_stream_index defaults to 1, not 0 (intentional "answer discovery before configuration" power-on default) -- the suppression test sets it explicitly to 0 to actually exercise suppression. ## Verification 4 new end-to-end tests wiring a real mock::Server through UDP: multi-member dispatch_frame, Table 24 suppression (client now genuinely times out instead of getting a default response), a Triggered conditional-opcode request (routed to Pending admission, not mistaken for Standard), and an E2E-CRC-protected request via dispatch_frame_e2e. include/rcp/udp.hpp: 747 -> 846 lines. tests/test_udp.cpp: 599 -> 923 lines, test_udp: 24 TEST_CASEs (151 assertions). 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>
SoundMatt
added a commit
that referenced
this pull request
Aug 22, 2026
…el dispatch (#152) * rewrite(phase5): l2.hpp — add FrameHandler wired to Phase 4 frame-level 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> * fix(l2): cast AvtpFrameHeader::sequence_num (uint16_t) to uint8_t for 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> --------- Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.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.The gap
udp::Server::Handlerwas shaped to matchmock::Server::dispatch()'s single-already-isolated-message contract, soserve()decoded each datagram into individual ACF messages and calledHandleronce per message. A caller wiring thatHandlerstraight tomock::Server::dispatch()— the obvious, natural thing to do — never reached Phase 4 batch D2's frame-level entry points (dispatch_frame()/dispatch_frame_e2e()), silently losing Table 24 response suppression, conditional/cancellation-opcode routing, and E2E CRC verification/fragment reassembly for every request over UDP.Note: c-RCP's current
udp.c/udp.hhave been re-architected into a pure raw-AVTPDU transport with no dispatch wiring at all — this gap is purely internal to cpp-RCP's own design, not a delta versus current c-RCP.Fix
Widened
Server::Handlerto a frame-level contract taking raw ACF-region bytes — exactly whatdispatch_frame()/dispatch_frame_e2e()accept. NewFrameResponsetype mirrorsmock::FrameMemberResult's response half withoutudp.hppneeding to#include mock.hpp. Chose widening the signature over a parallel compatibility path — two wiring surfaces would keep inviting the exact mistake this gap describes.Call sites
tests/test_udp.cppupdated;tls.hppneeded no change (SecureServer::Handleraliasesudp::Server::Handler).Verification
4 new end-to-end tests: multi-member
dispatch_frame, Table 24 suppression, a Triggered conditional-opcode request, and an E2E-CRC-protected request viadispatch_frame_e2e.udp.hpp: 747→846 lines.test_udp.cpp: 599→923 lines, 24 TEST_CASEs (151 assertions).Full tree rebuilds clean (0 errors/warnings), 58/58 ctest suites pass (100%) — independently re-verified from a fresh build.