fix: complete parent PUT when child subscribe finishes #2109
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses parent operation completion when child subscribe operations finish, improves transient connection tracking concurrency, and guards against counter overflow in connection management.
- Completes parent PUT operations when auto-subscribe child operations finish
- Migrates transient connection tracking from
HashMapwithRwLocktoDashMapfor better concurrent access - Adds overflow protection for the reserved connections counter
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/core/src/ring/connection_manager.rs | Migrates transient connections to DashMap, removes unused opened_at field, adds overflow guard for reserved connections counter, and implements is_transient_addr() method |
| crates/core/src/operations/mod.rs | Adds clarifying comment and tracing for root operations awaiting sub-operation completion |
| crates/core/src/node/network_bridge/p2p_protoc.rs | Adds logic to complete child subscribe operations and delegate result delivery to parent flow |
| apps/freenet-ping/app/tests/run_app_blocked_peers.rs | Updates ignore attribute and adds detailed TODO comments explaining WebSocket reset issue during teardown |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| continue; | ||
| } | ||
|
|
||
| if !op_manager.is_sub_operation(tx) { |
There was a problem hiding this comment.
This check is redundant because the same condition !op_manager.is_sub_operation(tx) is already checked on line 980 (immediately above). The inner if block on line 986 will always evaluate to true because if we reached this point, the condition on line 980 must have been false.
Consider removing this redundant check and simplifying the code:
// Already checked above, no need to check again
let response = Ok(HostResponse::ContractResponse(
ContractResponse::SubscribeResponse { key, subscribed },
));| match self.reserved_connections.compare_exchange( | ||
| current, | ||
| current + 1, | ||
| current.saturating_add(1), |
There was a problem hiding this comment.
The overflow guard on line 174 checks current == usize::MAX and rejects the connection, but then line 183 uses current.saturating_add(1) in the compare-exchange. If current == usize::MAX, we return early, so the saturating_add at line 183 will never actually saturate - it will always just be current + 1.
Either the check on line 174 should be current >= usize::MAX (which is impossible), or the saturating_add is unnecessary. Since we explicitly check and reject at MAX, the saturating operation provides no additional protection. Consider using plain addition:
current + 1,| current.saturating_add(1), | |
| current + 1, |
There was a problem hiding this comment.
is true this ius unlikely to happen unless there is a bug, but if there is such bug, is safer to just saturate instead of overflow.
maybe it should be handled at the check as suggested... this is just extra safety, and what we may need to compare against is the max reservations allowed instead of usize::MAX (again, to prevent bugs)
| let current = self | ||
| .reserved_connections | ||
| .load(std::sync::atomic::Ordering::SeqCst); | ||
| // Do not allow underflow (agreement with #2092) or overflow |
There was a problem hiding this comment.
The comment mentions "Do not allow underflow (agreement with #2092) or overflow" but the code below only guards against overflow (when current == usize::MAX). There is no underflow guard in this function since we're only incrementing the counter here, not decrementing it.
The comment should be clarified to accurately reflect what the code does:
// Guard against overflow when incrementingUnderflow guards would be needed in the fetch_sub operations elsewhere (e.g., lines 212, 234, 278, 415, 498, 515), but not here.
| // Do not allow underflow (agreement with #2092) or overflow | |
| // Guard against overflow when incrementing |
sanity
force-pushed
the
fix/put-subscribe-completion
branch
from
4b78283 to
c9cd355
Compare
- Add handle_aborted_op call when connection establishment fails - Inject empty ReturnGet to trigger existing retry logic for GET ops - Fix contract loading in run_app_blocked_peers test - Test remains #[ignore] until PUT/subscribe bug fixed separately Implements Option A from issue #2021
- Treat LocalSubscribeComplete for sub-ops as completion to unblock parent PUTs - Add logging when root ops await sub-ops for clarity during debugging
- Switch transient_connections to DashMap and use direct DashMap APIs - Saturate reserved_connections increment to avoid overflow/underflow and log a warn instead of looping at usize::MAX
The test passes functionally - all PUT/GET/Subscribe operations and state propagation work correctly. However, it fails with a WebSocket connection reset during teardown. This appears to be a cleanup/teardown issue rather than a functional bug. Marked ignored with TODO-MUST-FIX to unblock PR merges for the functional fixes (PUT completion and counter overflow). See issue #2108 for investigation details.
sanity
force-pushed
the
fix/put-subscribe-completion
branch
from
c9cd355 to
efb886b
Compare
iduartgomez
left a comment
iduartgomez
left a comment
There was a problem hiding this comment.
Not blocked by the comments
Scope (stacked on PR #2105)
Testing