Skip to content

Commit ef59c0a

Browse files
committed
fix: Address Codex review feedback
- Remove early return in start_subscription_request that prevented optimal location nodes from subscribing - Remove empty test that provided no actual coverage The early return issue was a critical bug that prevented the k_closest fix from working in the auto-subscribe path (PUT/GET operations). When a node was at optimal location, it would skip subscription entirely rather than trying alternative peers. These issues made it past testing because: 1. The integration tests were removed due to CI environment constraints 2. The empty test gave false confidence about coverage 3. The auto-subscribe path wasn't explicitly tested
1 parent 0d1f829 commit ef59c0a

File tree

2 files changed

+3
-40
lines changed

2 files changed

+3
-40
lines changed

crates/core/src/operations/mod.rs

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -308,26 +308,9 @@ impl<T> From<SendError<T>> for OpError {
308308

309309
/// If the contract is not found, it will try to get it first if the `try_get` parameter is set.
310310
async fn start_subscription_request(op_manager: &OpManager, key: ContractKey) {
311-
// Check if we are the optimal location for this contract
312-
// If we are and no other peers are suitable, skip subscription for now
313-
// TODO: In the future, still find next-best peers for redundancy (issue #1793)
314-
let own_location = op_manager.ring.connection_manager.own_location();
315-
let closest = op_manager
316-
.ring
317-
.closest_potentially_caching(&key, [&own_location.peer].as_slice());
318-
319-
if closest.is_none() {
320-
// No other peers available for caching
321-
// Check if we should be caching this locally
322-
if op_manager.ring.should_seed(&key) {
323-
tracing::debug!(
324-
%key,
325-
"Skipping subscription - node is optimal location and no other peers available"
326-
);
327-
return;
328-
}
329-
}
330-
311+
// Always try to subscribe, even if we're at optimal location
312+
// The k_closest_potentially_caching logic in subscribe::request_subscribe
313+
// will find alternative peers if needed
331314
let sub_op = subscribe::start_op(key);
332315
if let Err(error) = subscribe::request_subscribe(op_manager, sub_op).await {
333316
tracing::warn!(%error, "Error subscribing to contract");

crates/core/src/operations/subscribe/tests.rs

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,26 +31,6 @@ fn test_subscription_response_generates_host_result() {
3131
}
3232
}
3333

34-
/// Test that k_closest_potentially_caching is used for finding peers
35-
/// This verifies the fix for nodes at optimal location not being able to subscribe
36-
#[test]
37-
fn test_uses_multiple_peer_candidates() {
38-
// This test would require mocking the OpManager and ring structures
39-
// For now, we verify the code structure uses k_closest_potentially_caching
40-
// with k=3 in the actual implementation
41-
42-
// The actual implementation in request_subscribe should use:
43-
// op_manager.ring.k_closest_potentially_caching(key, EMPTY, 3)
44-
// instead of closest_potentially_caching
45-
46-
// Similarly, in the message handler for ReturnSub with subscribed: false,
47-
// it should use:
48-
// op_manager.ring.k_closest_potentially_caching(key, &skip_list, 3)
49-
50-
// This is a compile-time verification that the method exists
51-
// Actual runtime testing requires the full integration test
52-
}
53-
5434
/// Test that subscription state transitions correctly
5535
#[test]
5636
fn test_subscription_state_transitions() {

0 commit comments

Comments
 (0)