Repository navigation
rewrite(phase5): admin.hpp — fixed-capacity subscriber/counter bounds, port deadlock fix - #154
Merged
Merged
Conversation
…te-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>
# Conflicts: # tests/test_shmem.cpp Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
…, 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>
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). Phase 5 (transport layer), final module — closes out Phase 5's "port from c-RCP" scope (udp #151, l2 #152, shmem #153, now admin).
Ported (c-RCP src/admin.c 247 lines / include/rcp/admin.h 177 lines)
std::array<..., kMaxSubscribers=16>/std::array<..., kMaxCounters=256>replacing unboundedstd::vector/std::map, matchingRCP_ADMIN_MAX_SUBSCRIBERS/RCP_ADMIN_MAX_COUNTERS(admin.h:109-110) and this rewrite's fixed-capacity architecture.Server::subscribe()/ counter recording now fail closed (AdminErrc::subscriber_capacity_exceeded/counter_capacity_exceeded) at the bound, matching c-RCP's admin.c:133-198 behavior.TEST_CASEs.Bug found and fixed (same class as shmem.hpp/#153)
emit()held the subscriber-list mutex across every subscriber callback invocation — a real deadlock risk if a callback calls back intoServer. Fixed by copying the subscriber list under the lock, then invoking unlocked.Deliberate non-port (documented in-file)
c-RCP retired
rcp_registry_tproject-wide and went caller-driven; cpp-RCP'sadmin.hppinstead holds a live reference toshmem::Registry, which is still current in this rewrite. KEPT the registry-reference design — c-RCP's rationale for going caller-driven doesn't apply here. Reasoning recorded inadmin.hpp's own header comment.Verification
include/rcp/admin.hpp: 128 → 338 lines.tests/test_admin.cpp: 123 → 200 lines. Full tree rebuilt from scratch against the fully-merged base (picked up #151/#152/#153 via merge-forward), 58/58 ctest suites pass (100%) — independently re-verified.🤖 Generated with Claude Code