Add remove/unbridge services (remove_subscribe, remove_advertise)#31
Merged
Conversation
udp_bridge could add a runtime bridge (remote_subscribe / remote_advertise) but not remove one. Disconnecting required tearing down the node or silencing a topic with a negative rate period (left the forwarding in place). This adds real teardown, needed for marine_control's dynamic device control (ADR-0003 D7-dyn). - RemoteSubscribeInternal.msg: add OPERATION_SUBSCRIBE=0 / OPERATION_UNSUBSCRIBE=1 + uint8 operation (mirrors the ConnectionInternal.operation idiom). - removeSubscriberConnectionFrom (new testable header subscriber_registry.h): pure map logic — drop a (remote, connection) forwarding, drop the remote when it has no connections, drop the source entry (handing back its subscription for off-lock teardown) when it has no remotes. Idempotent. - removeSubscriberConnection: runs the map logic under subscribers_mutex_, resets the freed subscription off-lock (hygiene; an in-flight forwarding callback keeps its own executor ref so the reset is always safe), then sendBridgeInfo. - ~/remove_advertise: local removal of a local->remote push (mirror of remote_advertise). ~/remove_subscribe: sends OPERATION_UNSUBSCRIBE so the remote drops its push to us (mirror of remote_subscribe); decodeSubscribeRequest branches on operation. - callback(): use subscribers_.find() instead of operator[] (both the forward and the stats paths) so an in-flight message during teardown can't re-insert an empty entry that updateLocalSubscriptions would resurrect as a zombie sub. - 7 unit tests for the removal cascade + idempotency. 102 tests pass. Reviewed by two Deep adversarial passes (protocol/logic + concurrency): logic, key symmetry, direction, erase cascade, and teardown safety confirmed sound. Wire-format note: appending `operation` is not backward-compatible across versions; a new receiver throws on an old-format subscribe and decode()'s existing catch-all logs+drops it (recoverable). Deploy the fleet together. Closes #30 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Add remove/unbridge services:
remove_subscribe,remove_advertiseCloses #30. Prereq for marine_control dynamic device control (ADR-0003 D7-dyn,
rolker/unh_marine_autonomy#160).udp_bridgecould add a runtime bridge (remote_subscribe/remote_advertise) but not remove one — disconnect meant tearing down the node or silencing a topic with a negative rateperiod(which left the forwarding in place). This adds real teardown.Changes
RemoteSubscribeInternal.msg: addOPERATION_SUBSCRIBE=0/OPERATION_UNSUBSCRIBE=1+uint8 operation(mirrors the existingConnectionInternal.operationidiom).subscriber_registry.h(new, testable):removeSubscriberConnectionFrom— pure map logic. Drop a(remote, connection)forwarding; drop the remote when it has no connections; drop the source entry (handing back its subscription for off-lock teardown) when it has no remotes. Idempotent.removeSubscriberConnection: runs that undersubscribers_mutex_, resets the freed subscription off-lock, thensendBridgeInfo().~/remove_advertise: local removal of a local→remote push.~/remove_subscribe: sendsOPERATION_UNSUBSCRIBEso the remote drops its push to us;decodeSubscribeRequestbranches onoperation.callback(): usesubscribers_.find()instead ofoperator[](forward + stats paths) so an in-flight message during teardown can't re-insert an empty entry thatupdateLocalSubscriptionswould resurrect as a zombie subscription.Review (two Deep adversarial passes — protocol/logic + concurrency)
No must-fix defects: key symmetry (add/remove use the identical
(source, remote, connection)triple), direction (remove_subscribetargets the remote,remove_advertiseis local), erase cascade, lock ordering, and subscription-teardown safety all confirmed sound — the off-lockreset()is safe because an in-flight callback keeps its own executor ref. Review-driven fixes applied: corrected the teardown-rationale comments and the message's wire-compat note.Wire-format note: appending
operationis not backward-compatible across versions. A new receiver deserializing an old-format subscribe throws on the missing byte;decode()'s existing catch-all logs + drops it (recoverable, no crash, not misread). Deploy the bridge fleet together rather than mixing versions across a link.Authored-By:
Claude Code AgentModel:
Claude Fable 5