fix(update): check proximity cache before ring-based target selection

[sanity]

Problem

Contract UPDATE operations fail to propagate between peers because request_update() doesn't consider the proximity cache when selecting target peers.

User-visible impact: In a River chat room with two peers directly connected and both caching the same contract:

• Messages sent by one peer are never received by the other
• Each peer only sees their own messages
• Updates are sent to distant gateways (based on ring location) instead of directly to connected peers that have the contract

Root Cause

The target selection in request_update() (lines 916-950) was:

1. Check subscribers_of(&key) - explicit network subscriptions
2. Fall back to closest_potentially_caching(&key) - based on ring location distance

Missing: Check proximity_cache.neighbors_with_contract(&key), which contains peers that have announced they cached the contract.

What Happened

Timeline: 1. Peer A creates room → stores contract locally, announces to proximity cache 2. Peer B joins → stores contract locally, announces to proximity cache 3. Both are directly connected, both have contract cached 4. Peer A sends message → UPDATE goes to... Target selection: - subscribers_of(&key) = empty (no explicit subscriptions) - closest_potentially_caching(&key) = gateway (closest by ring location) - BUT: proximity_cache.neighbors_with_contract(&key) = [Peer B] ← NEVER CHECKED! 5. Gateway receives UPDATE, fails with "missing contract" (gateway doesn't have it) 6. Peer B never receives the update 

This Solution

Added proximity cache check between subscribers and ring-based selection:

let target = if let Some(remote_subscriber) = target_from_subscribers { remote_subscriber } else if let Some(proximity_neighbor) = target_from_proximity { // ← NEW proximity_neighbor } else { // Existing fallback to closest_potentially_caching ... }

New selection order:

1. Explicit subscribers (subscribers_of)
2. Proximity cache neighbors (neighbors_with_contract) ← NEW
3. Closest by ring location (closest_potentially_caching)

This aligns request_update() with get_broadcast_targets_update() (lines 577-688) which already correctly uses both subscribers AND proximity cache neighbors for broadcasting.

Testing

• All existing tests pass including update operations tests
• The fix is conceptually simple - adds one additional lookup before the fallback
• The proximity_cache module has comprehensive unit tests verifying neighbors_with_contract() behavior

Fixes

Closes #2294

[AI-assisted - Claude]

[sanity]

@claude Please do a skeptical review of this PR

[claude]

Claude finished @sanity's task in 2m 2s —— View job

---

Skeptical PR Review

Progress

• Read the modified file and understand the changes
• Examine the diff to see exact modifications
• Verify claims about get_broadcast_targets_update() alignment
• Check for potential bugs and edge cases
• Formulate skeptical review feedback

---

Review Findings

The fix is conceptually correct and addresses a real gap in target selection logic. However, I have several concerns and questions:

✅ What's Good

1. Correct diagnosis: The PR correctly identifies that request_update() was missing proximity cache checks that get_broadcast_targets_update() already has (lines 614-639).

2. Alignment achieved: The new selection order (subscribers → proximity → ring-based) matches the broadcast logic, which is the right architecture.

3. Simple implementation: The fix is minimal and follows the existing pattern - using find_map() to get the first proximity neighbor with the contract.

⚠️ Skeptical Concerns

1. Selection determinism issue

In request_update() update.rs:932-937:

let target_from_proximity = op_manager .proximity_cache .neighbors_with_contract(&key) .into_iter() .filter(|addr| addr != &sender_addr) .find_map(|addr| op_manager.ring.connection_manager.get_peer_by_addr(addr));

Issue: find_map() takes the first peer from neighbors_with_contract(), which returns a Vec<SocketAddr>. There's no guaranteed ordering here - it could vary between calls depending on DashMap iteration order (which is non-deterministic).

Contrast with broadcast: In get_broadcast_targets_update() (lines 614-639), this doesn't matter because it collects all proximity neighbors into a HashSet and broadcasts to all of them.

Impact: In a scenario with multiple proximity neighbors caching the contract:

• Peer A and Peer B both cache the contract
• Both are in proximity cache
• find_map() might return A on one call, B on another
• This inconsistency might not be a bug, but it's worth noting for debugging

Question: Should this be deterministic (e.g., pick closest by ring location, or random but stable within a time window)?

2. Race condition on cache announcements

The proximity cache is populated via CacheAnnounce messages (proximity_cache.rs:79-82) sent asynchronously when contracts are cached.

Timeline race:

T0: Peer A creates room → caches locally → sends CacheAnnounce T1: Peer B joins → caches locally → sends CacheAnnounce T2: Peer A sends UPDATE → checks proximity cache 

Question: At T2, has Peer A's proximity cache necessarily received Peer B's announcement? If network messages are delayed or reordered, the proximity cache might be empty even though both peers have the contract.

The PR description says "Both are directly connected, both have contract cached" - but being cached locally doesn't guarantee the announcement has been received and processed by the other peer.

This might explain why the bug manifested - not just because the code was missing, but because there's a race window where proximity announcements haven't propagated yet.

3. Missing test coverage

The PR says:

The fix is conceptually simple - adds one additional lookup before the fallback

But there's no new test that specifically validates this code path. The scenario described (two peers directly connected, both caching, UPDATE propagation) should have a regression test.

Recommendation: Add an integration test using freenet-test-network that:

1. Spins up 2 peers directly connected
2. Both cache the same contract (simulating River room join)
3. Peer A sends UPDATE
4. Verify Peer B receives it via proximity cache path (not ring-based fallback)
5. Add logging to confirm which code path was taken

4. Logging clarity

The new debug log update.rs:944-948:

tracing::debug!( %key, target = ?proximity_neighbor.socket_addr(), "UPDATE: Using proximity cache neighbor as target" );

This is good, but it would be helpful to also log:

• How many proximity neighbors were found (not just which one was selected)
• Whether subscribers were checked first and found empty

This would help diagnose future issues like "why did we pick proximity neighbor X when Y also had it?"

5. Consistency with subscriber selection

In the subscriber path update.rs:916-926, there's this logic:

let mut filtered_subscribers: Vec<_> = subscribers .iter() .filter(|sub| sub.socket_addr() != Some(sender_addr)) .cloned() .collect(); filtered_subscribers.pop()

Why .pop()? This takes the last subscriber (arbitrary), whereas proximity uses .find_map() which takes the first.

Question: Is there a reason for this inconsistency? Should both use the same selection strategy (e.g., first, last, random, closest)?

🤔 Philosophical Question

Why doesn't request_update() broadcast to ALL proximity neighbors like get_broadcast_targets_update() does?

The current fix picks ONE proximity neighbor to forward the UPDATE to. But get_broadcast_targets_update() sends to ALL proximity neighbors. This asymmetry is interesting:

• Initial request: send to ONE target (subscriber → proximity → ring-based)
• Broadcast phase: send to ALL targets (all subscribers + all proximity neighbors)

This makes sense for efficiency (avoid sending the same request multiple times), but it creates a window where:

1. Peer A picks proximity neighbor B to send UPDATE to
2. Proximity neighbor C also has the contract but doesn't get the initial request
3. C will only receive it if B's broadcast phase reaches C

This should work correctly, but it's worth confirming this is the intended architecture.

---

Verdict

The fix is functionally correct and should be merged, but with these recommendations:

1. Add integration test for the two-peer direct-connection UPDATE scenario
2. Consider deterministic selection (document why non-deterministic is OK, or make it deterministic)
3. Enhanced logging to show proximity neighbor count and selection reasoning
4. Document the race condition - either in code comments or issue tracking, acknowledge that proximity cache announcements are async and there may be a brief window where they haven't propagated yet

The fix solves the immediate problem (UPDATE not propagating between directly connected peers), but the underlying timing issues with proximity cache announcements might still cause intermittent failures in real-world scenarios.

---

Files reviewed:

• crates/core/src/operations/update.rs:916-950 ✓
• crates/core/src/operations/update.rs:577-688 ✓
• crates/core/src/node/proximity_cache.rs:185-204 ✓

[Copilot]

Pull request overview

This PR fixes a critical bug where UPDATE operations fail to propagate between directly connected peers in River chat rooms. The issue occurred because request_update() was only checking explicit subscribers and ring-based distance, but not the proximity cache which tracks peers that have announced they cached a contract.

Key changes:

• Added proximity cache check (neighbors_with_contract) to target selection logic in request_update()
• Positioned the check between subscriber lookup and ring-based fallback to match the priority used in get_broadcast_targets_update()
• Added comprehensive comments explaining the rationale for peer-to-peer update scenarios

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sanity]

Response to Skeptical Review

Thanks for the thorough review! I've pushed commit 5fec28d addressing several concerns. Here's my response to each point:

✅ Addressed in Latest Commit

1. Logging clarity - Added:

• Debug logging for each disconnected proximity neighbor (matches get_broadcast_targets_update pattern)
• proximity_neighbors_found count in the selection log
• Code comment documenting the async race condition

📝 Design Clarifications

2. Selection determinism (find_map vs explicit ordering)

The non-deterministic selection is acceptable here because:

• We only need ONE valid target to forward the initial request
• The target will process the update and broadcast to all other neighbors anyway
• If determinism were required, we'd need to add ring distance calculations, which adds complexity for minimal benefit

The inconsistency with pop() in subscriber selection is pre-existing - both are arbitrary and neither affects correctness since we just need any valid target.

3. Race condition on CacheAnnounce

This is documented in the new code comment. The key insight is:

• The proximity cache is a best-effort optimization, not a guarantee
• The ring-based fallback (closest_potentially_caching) handles the case where announcements haven't propagated
• This fix improves the common case where announcements have propagated (which was the scenario in fix: UPDATE target selection ignores proximity cache, causing message sync failures #2294)

4. Why ONE target vs ALL targets

This is the correct architecture:

• request_update(): Sends initial request to ONE target - that target processes the update and initiates broadcasting
• get_broadcast_targets_update(): Used by the target to broadcast the confirmed update to ALL interested peers

Sending the initial request to multiple targets would cause duplicate processing and potential conflicts.

🔮 Future Work

5. Integration test - Agree this would be valuable. A proper test would use freenet-test-network to:

1. Set up 2 peers directly connected (no gateway in path)
2. Both cache same contract
3. Verify UPDATE propagates via proximity path

This is out of scope for this bug fix but would be good follow-up work.

[AI-assisted - Claude]

[sanity]

Update: Added regression test for issue #2294

Following the review feedback, I've extended test_three_node_network_connectivity to validate UPDATE propagation via proximity cache:

Test scenario:

1. Peer1 PUTs contract → caches it, sends CacheAnnounce to neighbors
2. Peer2 GETs contract with subscribe → caches it, sends CacheAnnounce
3. Both peers now have each other in proximity_cache.neighbors_with_contract()
4. Peer1 UPDATEs contract
5. Verify Peer2 receives UpdateNotification

This ensures the proximity cache path is exercised and would catch any regression of issue #2294.

Also fixed a flaky test issue where the mesh formation retry loop could consume the entire test timeout (90 × 2s = 180s), leaving no time for actual test operations. Changed to deadline-based approach with 90s max for mesh formation.

[AI-assisted - Claude]