fix: separate network from local subscriptions

[iduartgomez]

Problem

Issue #2075 identified that local client subscriptions (WebSocket connections) were being registered in the same data structure as network peer subscriptions (ring.seeding_manager.subscribers). This coupling created several problems:

1. The allow_self hack - The get_broadcast_targets_update() function needed special logic to detect when an UPDATE originated locally vs. from the network, because local clients appeared in the subscriber list alongside remote peers.

2. Architectural confusion - The ops/ module (network operations) had implicit knowledge of client_events/ concerns, making the code harder to reason about.

3. Potential for bugs - Mixing local and network subscribers in one structure made it easy to accidentally send network messages to local clients or vice versa.

This Solution

The key insight is that local client subscriptions already have a separate delivery path: the contract executor's update_notifications channels deliver UpdateNotification directly to WebSocket clients. There's no need to also register them in the network subscriber list.

Changes:

1. complete_local_subscription() (subscribe.rs) - Removed the call to ring.add_subscriber(). Local subscriptions now only fire the LocalSubscribeComplete event to notify the client layer.

2. get_broadcast_targets_update() (update.rs) - Removed the allow_self workaround. The function now simply filters out the sender to prevent echoing updates back.

3. Documentation - Added architecture notes to LocalSubscribeComplete, add_subscriber(), and the affected functions explaining the separation.

Architecture After Fix:

Layer	Subscription Storage	Delivery Mechanism
Network	seeding_manager.subscribers	Peer-to-peer UPDATE messages
Local	Executor's update_notifications	Direct WebSocket delivery

Testing

• Added test_subscribers_for_broadcast_targeting - validates the broadcast filtering logic with real assertions
• Verified test_get_with_subscribe_flag passes - confirms local subscription + update notification works
• The test_multiple_clients_subscription (cross-node) remains #[ignore] due to unrelated network timing flakiness (see Fix flaky test_multiple_clients_subscription test #1798)

Fixes

Closes #2075

[CLAassistant]

All committers have signed the CLA.

[sanity]

I'll have an agent work on re-enabling this test

[sanity]

@claude please review this PR

[claude]

Claude finished @sanity's task —— View job

---

Code Review Complete

I've completed a detailed review of PR #2219. This is an excellent architectural improvement that cleanly separates local client subscriptions from network peer subscriptions. The PR is well-structured with comprehensive documentation, proper testing, and clear reasoning.

---

Summary

Verdict: ✅ Approve with Minor Suggestions

This PR successfully addresses issue #2075 by removing the architectural confusion between local and network subscriptions. The changes are:

• Architecturally sound - proper separation of concerns
• Well-documented - excellent architecture notes throughout
• Properly tested - new unit test validates the broadcast filtering logic
• Minimal risk - removes code (the allow_self hack) rather than adding complexity

---

Detailed Analysis

1. Architecture Changes (subscribe.rs:280-313) ✅

The removal of ring.add_subscriber() from complete_local_subscription() is the core of this fix:

// REMOVED: op_manager.ring.add_subscriber(&key, subscriber.clone(), None)
// NOW: Just notify client layer via LocalSubscribeComplete event

Strengths:

• Clean separation: local subscriptions now only use LocalSubscribeComplete event
• Updates flow through executor's update_notifications channels (separate from network)
• Eliminates mixing of concerns between ops/ and client_events/ modules

Verification needed:

• Confirm that the executor's update_notifications mechanism handles all local subscription cases
• The PR description mentions send_update_notification in runtime.rs but this file wasn't changed - might be worth adding a comment reference there too

---

2. Broadcast Filtering (update.rs:743-806) ✅

The get_broadcast_targets_update() simplification is beautiful:

Before:

let allow_self = self_addr.as_ref().map(|me| me == sender).unwrap_or(false);
// ... complex filter checking is_sender, is_self, allow_self

After:

.filter(|pk| pk.socket_addr().as_ref() != Some(sender))

Strengths:

• Much clearer intent: "don't echo updates back to sender"
• Removes the allow_self hack that was needed when local clients were in the subscriber list
• The architecture note explains the separation clearly

Observations:

• The new logic is simpler and easier to reason about
• No edge cases for "local auto-subscribes" since they're handled separately now

---

3. Documentation Quality ✅

The architecture notes added throughout are excellent:

• complete_local_subscription() (subscribe.rs:282-295)
• get_broadcast_targets_update() (update.rs:746-752)
• add_subscriber() (seeding.rs:109-119)
• LocalSubscribeComplete event (message.rs:409-420)

All consistently reference issue #2075 and explain the separation clearly.

Suggestion: Consider adding a cross-reference in runtime.rs near the send_update_notification function to point back to this architecture, since it's mentioned in multiple doc comments.

---

4. Test Coverage ✅

The new test_subscribers_for_broadcast_targeting() (seeding.rs:350-413) is well-designed:

What it validates:

• Network subscribers are correctly stored in seeding_manager
• Broadcast targeting correctly filters out the sender
• The foundation for UPDATE propagation works correctly

Strengths:

• Clear test structure with good comments
• Validates the exact filtering logic used by get_broadcast_targets_update
• Architecture note explains how this relates to Decouple local client subscriptions from network subscriptions #2075

Minor suggestion: Consider adding a negative test case that explicitly verifies local clients are not in the network subscriber list (though this is implicitly verified by the test setup).

---

5. Test Comment Update (operations.rs:639-644) ✅

The updated comment for test_multiple_clients_subscription is accurate:

// Issue #2075 decoupled local client subscriptions from network subscriptions, which
// should improve reliability. However, the test still exhibits flakiness due to
// network timing/transaction timeout issues unrelated to the subscription logic.

Observation: The comment correctly attributes the remaining flakiness to network timing issues (see #1798), not to this PR's changes. This is honest and helpful for future debugging.

---

Potential Issues & Questions

1. Backward Compatibility 🔍

Question: Are there any scenarios where code outside the files changed might still assume local subscriptions are in ring.subscribers?

• Checked: get_broadcast_targets_update is the main consumer (fixed in this PR)
• The separation appears complete, but worth confirming no other code paths check ring.subscribers_of() expecting to find local clients

2. Update Delivery Path 🔍

Question: Can you confirm the executor's update_notifications path handles all edge cases?

Specifically:

• What happens if a local client subscribes but the contract isn't cached yet?
• Does the executor notification happen for both local and remote UPDATEs?
• Are there any race conditions where an UPDATE could arrive before the subscription is fully established?

The PR assumes this path works correctly (which it likely does), but since we're removing the network-based path for local clients, it's worth explicit verification.

3. Test Gap 🤔

The test_get_with_subscribe_flag mentioned in the PR description isn't shown in the diff. Consider:

• Adding explicit assertions in that test confirming local subscription works without network registration
• Or creating a new focused test: "local subscription delivers updates without network broadcast"

---

Code Quality Notes

Positive patterns:

• Consistent architecture notes across all changed files
• Clear separation of concerns (network vs. local)
• Good use of logging (tracing::debug) to explain what's happening
• Test validates the exact logic being changed

Minor style observations:

• The upstream_addr parameter docstring in add_subscriber() (seeding.rs:115-118) is excellent - clear explanation of why it exists
• The formatting and structure are consistent with the rest of the codebase

---

Recommendations

Must-Have (Blockers):

None - the PR is ready to merge as-is.

Should-Have (Strongly Recommended):

1. Add cross-reference in runtime.rs (seeding.rs:109-119)

   • Near send_update_notification, add a comment pointing to this architecture
   • Example: "// See issue Decouple local client subscriptions from network subscriptions #2075: local subscriptions deliver via this path, separate from network subscriptions"

2. Verify update delivery path

   • Confirm (via test or code review) that executor notifications handle all cases
   • Especially: local subscribe → local update, local subscribe → remote update

Nice-to-Have (Optional Improvements):

1. Add negative test case

   • Test that explicitly verifies local clients are NOT in network subscriber list
   • Would make the separation even more explicit

2. Add integration test

   • Test: local client subscribes, receives updates, without network registration
   • Would complement the unit test in seeding.rs

3. Architecture diagram

   • Consider adding a simple ASCII diagram in ARCHITECTURE.md showing:
     • Network subscriptions → seeding_manager.subscribers → UPDATE broadcast
     • Local subscriptions → executor.update_notifications → WebSocket delivery

---

Summary Table

Aspect	Rating	Notes
Architecture	✅ Excellent	Clean separation of concerns
Code Quality	✅ Excellent	Simpler, more maintainable
Documentation	✅ Excellent	Comprehensive architecture notes
Testing	✅ Good	New unit test validates core logic
Risk	✅ Low	Removes complexity, well-documented

---

Conclusion

This is a well-executed architectural cleanup that makes the codebase more maintainable and easier to reason about. The removal of the allow_self hack and the clear separation between network and local subscriptions are significant improvements.

The PR description is also exemplary - it explains the problem, the solution, the reasoning, and the testing strategy clearly.

Recommendation: Approve and merge after considering the "Should-Have" suggestions above (especially verifying the executor notification path handles all cases).

---

[sanity]

⚠️ Do NOT merge until @iduartgomez has reviewed