fix: separate network from local subscriptions #2229
Merged
Conversation
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
Address review comments from PR freenet#2219: 1. Add comprehensive documentation to send_update_notification() in runtime.rs explaining the separation between local and network subscription delivery mechanisms. 2. Add explicit assertions and documentation in test_get_with_subscribe_flag confirming that local subscriptions work without network registration. 3. Update test_multiple_clients_subscription comment to document the remote update scenario (local subscribe → remote update) and its relationship to the local subscription test. The architecture uses two distinct paths: - Local subscriptions: executor notification channels (send_update_notification) - Network subscriptions: ring.seeding_manager.subscribers (broadcast targeting) This completes the documentation requested in the PR review.
- Analyzed test_small_network_get_failure failure modes - Identified gateway crash root cause (fixed in a283e23) - Documented PUT operation timeout issues - Provided recommendations for re-enabling the test - Suggested modernization using #[freenet_test] macro Related: freenet#2023, freenet#2043, freenet#2036, freenet#2011
Co-authored-by: nacho.d.g <iduartgomez@users.noreply.github.com>
- Update Cargo.lock to include test-log dependency changes - Fix formatting (remove extra blank line in test function) - Resolves CI build failure with --locked flag This addresses the CI error: "the lock file needs to be updated but --locked was passed"
- Remove local subscriber registration from seeding_manager in
complete_local_subscription() - local clients no longer pollute the
network peer subscription list
- Remove the allow_self hack from get_broadcast_targets_update() - this
workaround was only needed because local clients were mixed with
network peers
- Add architecture documentation explaining the separation:
- Network subscriptions: seeding_manager.subscribers for peer-to-peer
UPDATE propagation
- Local subscriptions: executor's update_notifications channels for
direct WebSocket client delivery
This clean separation eliminates cross-module awareness between ops/
and client_events/, making the code easier to reason about and debug.
Fixes freenet#2075
…et#2075) Add tests that document and validate the separation between local client subscriptions and network peer subscriptions: - test_local_subscription_does_not_register_in_seeding_manager_architecture: Documents that complete_local_subscription does NOT register in seeding_manager - test_subscribers_for_broadcast_targeting: Validates the broadcast filtering logic used by get_broadcast_targets_update These tests serve as architectural documentation and regression prevention for the decoupling implemented in Issue freenet#2075.
- Add test_subscribers_for_broadcast_targeting to validate the filtering logic used by get_broadcast_targets_update after Issue freenet#2075 refactor - Update test_multiple_clients_subscription comment to note that Issue freenet#2075 fix should help but test still has unrelated network timing flakiness
iduartgomez
deleted the
claude/address-pr-comments-01DunYUzm91E2LCaGQFGi4nm
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Superseeds #2219
Fixes #2075