fix: Phase 1 - Allow nodes at optimal location to subscribe (#1848)

[sanity]

Summary

Phase 1 fix for the update propagation architecture issue identified in #1848.

This PR addresses the "optimal location paradox" where nodes at the optimal network position for a contract become isolated because they skip subscription attempts entirely.

Changes

• Remove early return when node is at optimal location with no other peers available
• Properly exclude self when finding next-best peer for subscription
• Allow subscription attempts even when temporarily isolated (for future peer joins)
• Add warning logs to identify when nodes may become isolated

Problem Fixed

Previously, when a node was at the optimal location for a contract and had no other peers available, it would skip subscription entirely:

if closest.is_none() {
    if op_manager.ring.should_seed(&key) {
        // Node would return here, becoming isolated
        return;
    }
}

Now it attempts subscription anyway, which allows it to connect when new peers join the network later.

Test Results

Tested with multi-machine-test River integration:

• ✅ Nodes now attempt subscriptions even when at optimal location
• ✅ Warning logs correctly identify isolation risk
• ⚠️ WebSocket response routing still needs Phase 2 fix (tracked separately)

Related Issues

• Fixes part of Fix Update Propagation Architecture: Implement Proximity-Based Forwarding and Repair Subscription Topology #1848 (Phase 1 of 3)
• Related to UPDATE operations not propagated between peers - breaks collaborative applications like River #1846, Nodes at optimal location cannot subscribe to contracts, becoming isolated from update network #1847, WebSocket Subscribe Response Not Received in River Integration Tests river#33, Integration test hangs on second node subscription (Bob) after Subscribe fix river#34

Next Steps

• Phase 2: Fix WebSocket response routing for SubscribeResponse messages
• Phase 3: Implement proximity propagation if needed after Phase 1&2

[AI-assisted debugging and comment]

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

[sanity]

[Codex] Thank you for picking this up! Unfortunately I do not think the behaviour change in start_subscription_request achieves the stated goal yet. After these changes we still end up calling request_subscribe, which immediately returns RingError::NoCachingPeers when there is no remote candidate. In other words, a node that is sitting at the optimal location still exits with the same error so it never gains a downstream subscription. To actually allow the "next-best" peer, we need to fall back to the rest of the routing candidates (for example by walking k_closest_potentially_caching and keeping state across retries) instead of short-circuiting on the first miss.

On a smaller note: closest_potentially_caching already excludes the local node because ConnectionManager::add_connection never stores self, so the new skip_self array does not change the result set. The real gap is that we stop after the single closest lookup rather than iterating the next-best peers.

Could we restructure this so that the subscribe path keeps trying additional candidates (and/or records the current peer as a seeding source) before it surfaces NoCachingPeers back to the caller?

[iduartgomez]

Minor changes , good otherwise

[iduartgomez]

remove the comment Phase 1 fix for issue #1848:

[iduartgomez]

Remove this explicit comment: "// Phase 1 fix for issue #1848: Properly find next-best peer for subscription"

[sanity]

Closing this PR as it's been superseded by PR #1854 which has already been merged.

Why PR #1850 is no longer needed:

As @codex correctly pointed out, this PR's approach had fundamental issues:

1. closest_potentially_caching already excludes the local node internally, so the skip_self array doesn't change behavior
2. It still immediately returns NoCachingPeers error instead of trying alternative peers

What PR #1854 implemented instead:

PR #1854 properly fixed both the subscription routing issue AND the optimal location problem by:

• Using k_closest_potentially_caching(k=3) to get multiple candidate peers
• Implementing proper retry logic with skip lists for failed peers
• Ensuring nodes at optimal locations can still subscribe via alternative peers

The fix in PR #1854 is more comprehensive and addresses all the issues that PR #1850 was trying to solve, plus additional problems with subscription response routing.

Thanks @codex for the insightful review - your analysis was spot on about the limitations of this approach!

[AI-assisted debugging and comment]