fix(bitswap/httpnet): bound probe traffic - #1205
Merged
Merged
Conversation
Every connected HTTP peer was probed with GET/HEAD /ipfs/bafkqaaa every 5 seconds for the lifetime of the process, with results discarded and no backoff; at fleet scale this generated tens of thousands of requests per second of background load on public gateways. Latency, whose only scheduled consumer is the DONT_HAVE timeout manager, is now measured without dedicated traffic: - seed the EWMA from the Connect probe round trip - update it from time-to-headers of server-understood retrieval responses (200 and the 404 family); throttle and server errors are excluded so they cannot shrink DONT_HAVE timeouts - keep on-demand Ping for interface compliance - the pinger struct remains as the connection registry backing IsConnectedToPeer; latency is still wiped on disconnect
Connect re-probed failing endpoints on every call with no memory, and a message sender created during a cooldown treated it as permanent. Backoff now works end to end: - Connect and Ping honor per-host cooldowns; a failed probe starts one from Retry-After (past dates rejected) or the new exported DefaultConnectFailureBackoff - endpoints skipped only for an active cooldown stay in the peerstore as failover targets instead of being lost for the connection's lifetime; HEAD support is not assumed for them - cooldowns live in a process-wide SharedCooldownTracker by default so they survive short-lived Network instances; WithCooldownTracker opts a Network into a private registry; Network.Stop leaves the registry alone; the tracker no longer runs a cleaner goroutine (expired entries are swept during writes) - expired sender cooldown snapshots are cleared (CompareAndSwap, so a concurrently stored fresh deadline survives) instead of poisoning the sender for its lifetime
410 on the connection probe counted as success to accommodate a provider that has since shut down. A 410 probe now fails Connect and starts a cooldown; 410 on real block requests still counts as a valid content-unavailable reply.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1205 +/- ##
==========================================
+ Coverage 63.85% 64.20% +0.34%
==========================================
Files 269 269
Lines 27148 27193 +45
==========================================
+ Hits 17335 17458 +123
+ Misses 8096 8018 -78
Partials 1717 1717
... and 11 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This was referenced Aug 16, 2026
lidel
marked this pull request as ready for review
August 16, 2026 22:52
gammazero
approved these changes
Aug 17, 2026
gammazero
left a comment
gammazero
left a comment
Contributor
There was a problem hiding this comment.
Looks good. Only a few nits, mostly not even part of your PR, that changing is optional.
jsamol
pushed a commit
to Acurast/ipfs-mobile
that referenced
this pull request
Aug 17, 2026
pseudo-version of ipfs/boxo#1205; removes the 5s ping loop, adds probe backoff with Retry-After, and keeps cooldowns in a process-wide registry that survives node-per-download recycling
acurastgitlab
pushed a commit
to Acurast/ipfs-mobile
that referenced
this pull request
Aug 17, 2026
pseudo-version of ipfs/boxo#1205; removes the 5s ping loop, adds probe backoff with Retry-After, and keeps cooldowns in a process-wide registry that survives node-per-download recycling (cherry picked from commit b43e2dc)
Co-authored-by: Andrew Gillis <11790789+gammazero@users.noreply.github.com>
TestDisconnectFreezesProbes failed on windows runners: their monotonic clock ticks at ~0.5ms, so a probe over a pooled connection measures as zero and the reconnect latency seed was skipped, making a measured peer look unmeasured. Latency samples are now floored at 1ms in recordLatencyIfConnected and Connect seeds unconditionally, since seeding is only reached after a successful probe. The test covers the zero-sample case.
lidel
force-pushed
the
fix/httpnet-bound-probe-traffic
branch
from
August 17, 2026 10:41
5bec5a5 to
8d1234e
Compare
Member
Author
|
@gammazero fwiw staging test looks ok: https://github.com/ipshipyard/waterworks-infra/pull/1037#issuecomment-5313945021 -- if no concerns, feel free to merge and ship as a patch release (fine if its the only fix, we want this out asap, just so LLMs stop propagating older version) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
For legacy reasons, every connected HTTP peer gets probed with
GET/HEAD /ipfs/bafkqaaaevery 5 seconds for the lifetime of the process. Results are discarded, failures never slow the loop, andConnectre-probes failing endpoints with no memory. A single idle daemon emits ~17k requests per day per HTTP provider. The probe target and wire format are spec-defined and unchanged1; the defect is frequency and lifecycle, which the spec does not constrain.Fix
Connectprobe round trip and updated from time-to-headers of server-understood retrieval responses; on-demandPingstays, matching bsnetConnectandPinghonor per-host cooldowns; a failed probe starts one fromRetry-After23 (past dates rejected) or the newDefaultConnectFailureBackoff; endpoints skipped for an active cooldown stay in the peerstore as failover targetsSharedCooldownTrackerso backoff survives applications that build a short-livedNetworkper retrieval;WithCooldownTrackeropts a Network out; the tracker holds no goroutine and needs no shutdownHistoric conxtex: the 5s interval landed in #747 without a stated rationale, nothing consumes it on a schedule, and #918 already records that idle peers should not be pinged. An opt-in periodic pinger can come back later without API breakage, but i'm unsure we need it..
Testing
New synctest and integration suites cover the behavior changes;
TestNoBackgroundProbesis the regression guard for the removed loop.go test -race -count=3 ./bitswap/network/httpnet/...andgo test -race ./bitswap/...pass. Companion kubo PR (branch pinned, green CI) plus ipfs-check and rainbow validation branches: to follow, linked here before merge.Before this ships as a patch release, we need to make sure success rate remains the same:
Footnotes
Trustless Gateway spec, dedicated probe paths: defines the probe target and response; says nothing about client request frequency. ↩
Path Gateway spec, Retry-After response header: gateways SHOULD return Retry-After with 429, 503 and 504. ↩
RFC 9110, section 10.2.3: Retry-After indicates how long the user agent ought to wait before making a follow-up request. ↩