Revert "feat(pkg/p2p): reconnect on disconnected peers (#3212)"

[julienrbrt]

This reverts commit 04c9cad.

Overview

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ee6d78ca-031f-44b5-a35c-96eb894fabaf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch julien/debug-p2p-1

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Apr 16, 2026, 8:16 AM

[claude]

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

---

Code Review

• Gather context and read changed files
• Review the revert for completeness and correctness
• Assess impact on p2p reconnection behavior
• Provide final review summary

---

Summary

This PR reverts #3212 (feat(pkg/p2p): reconnect on disconnected peers), removing the custom seed peer reconnect loop from the P2P client. The revert is clean and complete — no residual references to the removed code remain anywhere in the package.

---

Revert Completeness

All code from #3212 has been fully removed:

Removed	Location
seedReconnectBackoff / seedReconnectMaxBackoff constants	client.go:35-38
ctx, cancel, seedPeers struct fields	client.go:58-61
context.WithCancel in startWithHost	client.go:142
c.host.Network().Notify(c.newDisconnectNotifee()) registration	client.go:175
c.cancel() in Close	client.go:178
disconnectNotifee type + all 6 interface methods	client.go:245-320
reconnectSeedPeer, newDisconnectNotifee, isConnected	client.go:295-340
c.seedPeers = peers in setupDHT	client.go:258
TestSeedPeerReconnect + TestSeedPeerReconnectStopsOnClose	client_test.go:278-375

No orphaned imports, no dangling references. The waitForCondition helper was retained correctly — it's still used by TestClientInfoMethods (lines 327, 345).

---

Design Observations on the Reverted Code

A few issues in the original PR #3212 that may justify this revert:

1. Context stored in struct (c.ctx context.Context) — this is a well-known Go anti-pattern (per the context package docs). Storing the startup context on the struct ties the reconnect goroutine's lifetime to the Start() caller's context, rather than the node's own lifecycle.

2. Goroutine accumulation — Disconnected() spawns go n.c.reconnectSeedPeer(sp) every time a disconnect event fires. If a peer flaps or disconnects rapidly (e.g. a node restart), multiple concurrent reconnect goroutines accumulate before any of them can establish a connection.

3. Redundant reconnection with DHT — dht.IpfsDHT initialized with dht.BootstrapPeers(peers...) already handles periodic refresh and reconnection to bootstrap peers via dht.Bootstrap(). The explicit reconnect loop may create duplicate or racing connection attempts against the DHT's internal retry logic.

4. Connected() method is a no-op — The notifee's Connected() handler only logs; it provides no functional value and consumes a network event listener slot.

---

Suggestions

• Document the reason for revert — The PR description is empty. A brief note (e.g. "Reverted due to goroutine accumulation on peer flap; DHT handles bootstrap peer reconnection natively") would help future contributors avoid re-introducing the same issue.

• Follow-up issue — If seed peer reconnection is a real operational need (e.g., observed in production where nodes lose bootstrap connectivity and never recover), it's worth opening a tracking issue for a corrected implementation — one that uses context.Background() with explicit lifecycle management, deduplicates reconnect goroutines (e.g. via sync.Map of in-flight reconnects), and accounts for the DHT's built-in behaviour.

---

Verdict: LGTM as a clean, complete revert. The main ask is a sentence in the PR description explaining the motivation.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.28%. Comparing base (2865d6d) to head (549e0f4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3258      +/-   ##
==========================================
- Coverage   62.33%   62.28%   -0.05%     
==========================================
  Files         122      122              
  Lines       12873    12826      -47     
==========================================
- Hits         8024     7989      -35     
+ Misses       3968     3958      -10     
+ Partials      881      879       -2     

Flag	Coverage Δ
combined	62.28% <ø> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[julienrbrt]

unnecessary