fix(test): convert multi-gateway test to use freenet_test macro with partial connectivity

[sanity]

Summary

This PR converts the test_ping_partially_connected_network test to use the #[freenet_test] macro and extends the macro with support for partial connectivity networks.

Key Changes

1. Extended #[freenet_test] macro with peer_connectivity_ratio attribute

   • Added new peer_connectivity_ratio parameter (0.0-1.0) to control network topology
   • Implemented deterministic blocking formula: (a * 17 + b * 31 + a * b * 7) % 100
   • Uses relative peer indices (not absolute node indices) for consistent blocking patterns
   • Documented in macro README and testing guide

2. Converted test from manual setup to macro-based

   • Reduced test code from ~376 lines to ~179 lines (52% reduction)
   • Macro now handles: socket binding, configuration, node startup, lifecycle management
   • Test focuses purely on business logic: contract operations and subscription propagation
   • Improved readability and maintainability

3. Fixed critical bugs

   • Peer indexing bug: Was using absolute node indices (0-9) instead of relative peer indices (0-6), causing incorrect blocking patterns
   • Stack overflow: Changed from large select! block with boxed futures to tokio::task::spawn_local with LocalSet
   • Contract compilation: Fixed to use correct build directory based on debug/release mode
   • Chrono version conflict: Use re-exported chrono from freenet_ping_types

4. Core fixes from original PR

   • Treat handshake enqueue timeouts as recoverable so gateways keep running
   • Ensure test harness compiles the ping contract with the contract feature
   • Use the same contract-loading flow in multi-gateway test as the single-gateway variant

Test Results

The test now passes consistently in ~129s with the correct network topology:

• 3 gateways (gw-0, gw-1, gw-2)
• 7 peer nodes (node-0 through node-6)
• 30% connectivity ratio between peers (matches original test)
• All peers auto-connect to all gateways

Testing

cargo test -p freenet-ping-app --test run_app test_ping_partially_connected_network -- --nocapture

Closes #2029

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

[sanity]

Review of PR #2031

Reviewed the changes - this addresses multiple issues discovered during #2022 investigation. Here's my analysis:

✅ Strong Points

1. Handshake Timeout Handling (p2p_protoc.rs)

• Converts handshake timeout from fatal error to graceful recovery
• Critical fix: Previously timeout(...).await? would propagate Elapsed error and crash the gateway
• Now treats timeout as connection failure, notifies callback, but keeps gateway running
• This directly addresses the "channel closed" crashes seen in CI

2. Contract Compilation (common/mod.rs)

• Adds contract feature to compilation pipeline via extra_features()
• Ensures freenet_stdlib::time is available when compiling ping contract
• Includes ensure_target_dir_env() fallback for when CARGO_TARGET_DIR isn't set

3. Test Consistency (run_app.rs)

• Changes from std::fs::read() + ContractContainer::try_from() to common::load_contract()
• Now uses same contract loading path as single-gateway test
• Ensures contract compiles with proper features at test time

🔍 Observations

Handshake Error Handling
The timeout handling change is interesting:

Err(elapsed) => {
    // ...logs warning...
    // ...notifies callback...
    Ok(())  // <-- Returns Ok, gateway continues
}

This makes timeouts non-fatal, which is appropriate for CI environments where resource contention can cause temporary delays during multi-gateway initialization.

Contract Loading Pattern
The test now compiles the contract twice:

1. First with temp params to get code hash
2. Second with real params to get final container

This matches the single-gateway test pattern and ensures consistent behavior.

⚠️ Potential Concerns

1. Timeout Semantics
Treating handshake queue timeout as Ok(()) means:

• Gateway continues running even if it couldn't queue the handshake
• Connection attempt is silently abandoned (callback gets notified but gateway doesn't know about failure at return site)

Question: Should we log at ERROR level instead of WARN since this represents a failed connection attempt?

2. CARGO_TARGET_DIR Fallback
The ensure_target_dir_env() adds fallback logic:

if std::env::var(TARGET_DIR_VAR).is_err() {
    // ...find workspace and set target dir...
}

Question: When would CARGO_TARGET_DIR not be set during testing? Is this for IDE/manual runs?

📋 Testing Recommendation

Before merge, verify:

1. Multi-gateway CI test: Does it pass consistently now? (most important)
2. Single-gateway test: Still passes (regression check)
3. Local testing: Both variants pass locally (sanity check)

The PR description says:

RUST_LOG=info cargo test -p freenet-ping-app --test run_app test_ping_partially_connected_network -- --ignored --nocapture

This should be run in CI to confirm the fix works in the resource-constrained environment where the original failure occurred.

🎯 Verdict

Changes look reasonable and address root causes identified in #2029:

• ✅ Fixes gateway crash on handshake timeout
• ✅ Ensures contract compiles with proper features
• ✅ Aligns test with working single-gateway pattern

Recommend: Approve pending successful CI run, particularly the Test job which includes the ignored tests.

---

[AI-assisted debugging and comment]

[sanity]

Thanks for the thorough read-through!

On the timeout logging level – I kept it at warn because the timeout here usually means “we never heard back from the handshake task in time” rather than a hard failure of the node. The connect op still receives the failure via the callback and will retry using its existing backoff, so the warning serves as a heads-up without spamming error when CI is under load. If we find it gets too chatty we can always tune the level in follow-up.

On CARGO_TARGET_DIR – integration tests launched via cargo test don’t inherit that env var (it’s only set when you call cargo yourself). In the multi-gateway test we fork a fresh cargo build to compile the contract inline; without explicitly seeding the env var that inner build fell back to $PWD/target, which conflicted with other runs. The helper just replicates what our build scripts do by default so the path is stable whether you run from CI, IDE, or a developer shell.

CI’s still running – I’ll keep an eye on the workflow to make sure the multi-gateway job makes it across the finish line.

[sanity]

CI Test Failed

The multi-gateway test still fails in CI with the same error pattern:

error: anyhow::Error - Gateway node failed: deadline has elapsed

The logs show the familiar "channel closed" errors during gateway initialization, despite the handshake timeout handling improvements.

Analysis

The handshake timeout handling change (Err(elapsed) => Ok(())) makes timeouts non-fatal for the P2P connection manager, but the test is still hitting a different deadline - likely the test harness's gateway startup timeout.

The "channel closed" errors appear to be a symptom of a deeper coordination issue during multi-gateway initialization in resource-constrained CI environments. The changes in this PR:

1. ✅ Prevent gateway crashes - Handshake timeouts no longer crash gateways
2. ❌ Don't fix the underlying coordination issue - Gateways still fail to establish connections within the test deadline

Observations from Logs

ERROR freenet::operations::connect: Failed while attempting connection to gateway,
  gateway: v6MWKgqK4rkpqzJL, error: failed notifying, channel closed

This suggests channels are still closing during the initial connection phase, which indicates:

• Gateways may be shutting down prematurely
• Or there's a race condition in channel lifecycle management
• Or the 10s timeout in establish_conn is being exceeded

Next Steps

Option 1: Increase the handshake timeout from 10s to something higher for CI
Option 2: Investigate why channels are closing - is it related to the rate limiter or connection handler?
Option 3: Add more detailed logging around channel lifecycle to understand the close sequence
Option 4: Check if the test needs sequential gateway startup rather than parallel

This is still a CI-specific issue (test passed locally for the other agent). The test may inherently be timing-sensitive and require a different approach for resource-constrained environments.

[AI-assisted debugging and comment]

[sanity]

@claude Is this PR still relevant given other PRs merged over the past 3 days?

[claude]

Claude finished @sanity's task —— View job

---

PR Relevance Analysis

• Check current branch commits and changes
• Compare with origin/main to identify differences
• Review recent merges to main
• Analyze changed files for overlap
• Provide recommendation on PR relevance

---

Answer: Yes, this PR is still very relevant

Here's the situation:

What's in this PR branch

This branch contains 4 commits that are NOT yet on main:

1. 80a7a03 - fix(tests): stabilize multi-gateway integration
2. 31ac9eb - test(tests): enable multi-gateway integration
3. 4094f82 - fix(network): prevent handshake enqueue error from crashing node
4. 4da57bd - test(ping): increase client response timeouts for integration

What's changed on main in the past 3 days

Main has moved forward by only 1 commit:

• 538d0ef - chore(deps): bump tracing-subscriber from 0.3.18 to 0.3.20 in /modules/identity-management (chore(deps): bump tracing-subscriber from 0.3.18 to 0.3.20 in /modules/identity-management #2040)

This is just a dependency update and has nothing to do with the multi-gateway test fixes.

Current Status

• The changes in this PR have NOT been merged to main
• The multi-gateway test fix is still needed
• However, the CI test still failed on Oct 29 with the same "deadline has elapsed" error

Files Changed in This PR

• apps/freenet-ping/app/src/ping_client.rs - Client response timeout changes
• apps/freenet-ping/app/tests/common/mod.rs - Contract compilation with features
• apps/freenet-ping/app/tests/run_app.rs - Test harness improvements
• crates/core/src/node/network_bridge/p2p_protoc.rs - Handshake timeout handling

Recommendation

This PR should remain open, but you need to decide:

1. If the fixes are working: Despite the changes, the CI test still failed. You may need to:

   • Investigate why the test is still timing out
   • Consider increasing timeouts further
   • Or close this PR if you determine a different approach is needed

2. Merge conflict with main: You'll need to rebase or merge main into this branch to pick up the dependency update before merging

3. Alternative: If these fixes aren't working, you might want to close this PR and pursue a different solution to the multi-gateway test instability

The PR is definitely still relevant (no other PR has addressed these issues), but it needs either more work or a decision on whether to pursue a different approach.

---

[iduartgomez]

@claude rebase on main and see if we wtill run into issues qith this test

[claude]

Claude finished @iduartgomez's task —— View job

---

Rebasing on main and testing

• Check current git status and fetch latest changes
• Analyze rebase requirements
• Rebase branch on origin/main (requires permission)
• Push rebased branch
• Run multi-gateway integration test

Analysis

I've analyzed the current state of the branch and unfortunately I don't have permission to perform git rebase operations. However, I can provide you with the information you need:

Current Branch State:

• The branch fix/multi-gateway-ci already includes all changes from main via merge commit c04a8ca
• Main is at commit 538d0ef (the tracing-subscriber dependency update)
• The branch has 6 commits total, with 4 unique commits not on main:
  1. 80a7a03 - fix(tests): stabilize multi-gateway integration
  2. 31ac9eb - test(tests): enable multi-gateway integration
  3. 4094f82 - fix(network): prevent handshake enqueue error from crashing node
  4. 4da57bd - test(ping): increase client response timeouts for integration
  5. c04a8ca - Merge branch 'main' into fix/multi-gateway-ci
  6. 4d6ca92 - fix: keep handshake hiccups from killing gateway

Files Changed:

• apps/freenet-ping/app/src/ping_client.rs (+3/-3)
• apps/freenet-ping/app/tests/common/mod.rs (+40/-3)
• apps/freenet-ping/app/tests/run_app.rs (+11/-9)
• crates/core/src/node/network_bridge/p2p_protoc.rs (+98/-19)

What You Can Do

To rebase this branch on main yourself, run:

git checkout fix/multi-gateway-ci
git rebase origin/main
git push --force-with-lease origin fix/multi-gateway-ci

This will create a cleaner linear history by replaying your commits on top of the latest main.

Alternative: The Branch is Already Up-to-Date

Since the branch already includes all changes from main (via the merge commit), rebasing is optional from a functionality standpoint. The main benefit would be a cleaner git history. If you're primarily concerned about whether the test passes, you could:

1. Trigger CI on the current branch state, or
2. Rebase locally as shown above and push

Would you like me to help with anything else regarding this PR?

---

[iduartgomez]

Latest Update: Fixed Blocking Formula and Peer Indexing

The most recent commits fix a critical issue where the test was not functionally identical to the original:

Issues Found

1. Wrong connectivity ratio: Test was using 0.5 instead of 0.3
2. Simplified blocking formula: Using (i * j) % 100 instead of the original's better distribution
3. Incorrect peer indices: Using absolute node indices (0-9 including gateways) instead of relative peer indices (0-6 for peers only)

Fixes Applied

• Updated blocking formula to use ordered pairs: (a * 17 + b * 31 + a * b * 7) % 100
• Fixed peer indexing to use relative indices (0-6) instead of absolute (0-9)
• Changed connectivity ratio from 0.5 to 0.3
• Fixed chrono version conflict

Test Status

✅ Test now passes consistently in ~129s with the same topology as the original manual test

The peer indexing fix was the most critical - without it, the blocking pattern was completely different from the original test because we were using indices 0-9 (all nodes) instead of 0-6 (peers only) in the hash formula.