test: fix flakes that force CI re-runs - #11413
Merged
Merged
Conversation
The DHT returns the closest peers it reached together with the context error when a lookup runs past its deadline. We dropped both, so any lookup slower than the routing server's per-request timeout came back as HTTP 500 with nothing in it, indistinguishable from a lookup that found no peers at all. Return what we have, and only error when the set is empty.
GetClosestPeers joined the public Amino DHT with real bootstrap peers, so the assertions depended on a CI runner reaching bootstrap.libp2p.io from a cold repo. When it could not, the test retried for five minutes and failed; ten such failures since v0.42.0, every one green on re-run. Bootstrap from the harness's in-process DHT peers instead, which the provider tests already use and this one predates. The window drops from five minutes to sixty seconds because there is no longer anything slow to wait for, and passing runs go from tens of seconds to under one.
NewRandPort binds port zero, notes the number, closes the socket and hands the number to the caller, which leaves a window for anything else on the machine to take it. The number also came from the ephemeral range, the same pool every outgoing connection draws from, and the CLI suite opens a lot of those. Both TestP2PForeground tunnel subtests died on "bind: address already in use" for a server the test binds itself. - NewTCPListener hands back the bound listener, closing that window for callers that listen in-process - ports for daemons we spawn now come from below the ephemeral range, so an outgoing connection cannot land on one
TestAddGCLive asserted that gc had not started yet, but the only thing it waited for was the first file's output event. Between that event and the adder reaching the next file there is a gap, and the adder hands the pin lock to a waiting gc at exactly that boundary, so on a loaded runner gc really had started and the assertion was right to fail. Wrap the pipe so the test learns when the adder is inside the hanging file, and poll GCRequested instead of sleeping 100ms to know gc is queued. TestAddMultipleGCLive gets the same treatment for its two sleeps: too short there means gc never gets the lock and the test waits out its five second timeout instead.
os.WriteFile creates the file and fills it in two steps, and ipfswatch adds whatever is on disk when the create event wakes it. Catch it between the two and it adds an empty file, so the CID the test pulls out of the log reads back as nothing. Stage the file outside the watched directory and rename it in, which the watcher sees as one event for a file that is already complete.
The test backgrounded "ipfs log tail", slept 100ms and expected the daemon to be listing the request. The daemon only sees it once the client has started up and connected, which on a loaded runner takes longer than that, and then both the active and the inactive assertion fail together because the entry never appears at all. Poll for each state instead. The extra requests that polling makes push the daemon closer to the point where it drops finished entries from the log, so keep them with "diag cmds set-time" first.
The connect case opened by re-asserting that the previous case had left zero peers connected. Disconnecting is not permanent: the DHT keeps the other node in its routing table and re-dials it on any refresh, so that count is only true for as long as nothing else runs. What this case is named for, connecting with a bare /p2p/ address, is still covered by the connect itself and the peer count after it.
Every parallel subtest does identical setup before mounting, so they all reach the mount together and around twenty setuid fusermount helpers open /dev/fuse inside the same instant. One occasionally comes back with a bare exit status 1. Take a lock for the mount call itself, which the subtests only hold for tens of milliseconds. Also report the failure instead of panicking: a panic failed all 37 tests in the package and left daemons behind, and the daemon's stderr, where fusermount says what actually went wrong, was captured and then thrown away.
The payload is 100 random bytes and the comparison ran through Trimmed(), which strips one trailing newline. Roughly one run in 256 ends in 0x0a and loses it.
The daemon writes the line before it answers the RPC, but the test reads a buffer that a goroutine fills by copying the daemon's stderr, and that copy can still be behind when the command returns. Wait for the line rather than assuming it has landed.
A minute after the daemon starts, the republisher re-signs every key and publishes it again, giving the same value a new signature and expiry. The test captured one PUT body and compared it byte for byte with what routing returned, so a run slow enough to straddle that minute compared the first record against the second. Keep every record the mock is sent and require that routing's answer is one of them, which is what the assertion was reaching for.
The example connects its two nodes by address, but left mDNS on, so local discovery could connect them first. A connection opened while a node is still being built is invisible to that node's bitswap, which only learns about connections made after it registers its notifier, and with no routing configured there is nothing to fall back on. The final fetch then waited forever and the test died on its two minute timeout with no clue why. Turning mDNS off makes the explicit dial the only way the two can meet, and keeps the example off the reader's LAN. Alongside that: - connectToPeers returns dial errors instead of logging and continuing into a fetch that cannot succeed - the example's own deadline now fits inside the test budget, so a stall names the step that hung - CommandContext so a hung child does not outlive the test
Seven failures since v0.42.0 came from this job's setup rather than from any incompatibility. It installs whatever @helia/interop published last, and upstream shipped three packages in a row whose test config does not work from inside node_modules; a GitHub blip took out the rest. - find the compiled specs and pass them to aegir, instead of patching the config upstream ships into node_modules and grepping its text - pin node to a major: setup-node resolves an lts/ alias through a GitHub manifest with no retry and no fallback, and newer node rejects a flag aegir sets unconditionally - retry the registry lookup and fail loudly, since the old one-liner could not fail and left an empty cache key behind - install the exact version the cache key names, and only save the cache once the install is known good - drop the playwright apt packages, unused since this job stopped running browser targets
lidel
added a commit
that referenced
this pull request
Jul 29, 2026
* fix(routing): keep peers found before the timeout The DHT returns the closest peers it reached together with the context error when a lookup runs past its deadline. We dropped both, so any lookup slower than the routing server's per-request timeout came back as HTTP 500 with nothing in it, indistinguishable from a lookup that found no peers at all. Return what we have, and only error when the set is empty. * test: use local dht swarm for routing v1 test GetClosestPeers joined the public Amino DHT with real bootstrap peers, so the assertions depended on a CI runner reaching bootstrap.libp2p.io from a cold repo. When it could not, the test retried for five minutes and failed; ten such failures since v0.42.0, every one green on re-run. Bootstrap from the harness's in-process DHT peers instead, which the provider tests already use and this one predates. The window drops from five minutes to sixty seconds because there is no longer anything slow to wait for, and passing runs go from tens of seconds to under one. * test: stop handing out ports the kernel reuses NewRandPort binds port zero, notes the number, closes the socket and hands the number to the caller, which leaves a window for anything else on the machine to take it. The number also came from the ephemeral range, the same pool every outgoing connection draws from, and the CLI suite opens a lot of those. Both TestP2PForeground tunnel subtests died on "bind: address already in use" for a server the test binds itself. - NewTCPListener hands back the bound listener, closing that window for callers that listen in-process - ports for daemons we spawn now come from below the ephemeral range, so an outgoing connection cannot land on one * test: sync gc tests to the adder, not the clock TestAddGCLive asserted that gc had not started yet, but the only thing it waited for was the first file's output event. Between that event and the adder reaching the next file there is a gap, and the adder hands the pin lock to a waiting gc at exactly that boundary, so on a loaded runner gc really had started and the assertion was right to fail. Wrap the pipe so the test learns when the adder is inside the hanging file, and poll GCRequested instead of sleeping 100ms to know gc is queued. TestAddMultipleGCLive gets the same treatment for its two sleeps: too short there means gc never gets the lock and the test waits out its five second timeout instead. * test: move watched file in atomically os.WriteFile creates the file and fills it in two steps, and ipfswatch adds whatever is on disk when the create event wakes it. Catch it between the two and it adds an empty file, so the CID the test pulls out of the log reads back as nothing. Stage the file outside the watched directory and rename it in, which the watcher sees as one event for a file that is already complete. * test(sharness): poll the daemon request log The test backgrounded "ipfs log tail", slept 100ms and expected the daemon to be listing the request. The daemon only sees it once the client has started up and connected, which on a loaded runner takes longer than that, and then both the active and the inactive assertion fail together because the entry never appears at all. Poll for each state instead. The extra requests that polling makes push the daemon closer to the point where it drops finished entries from the log, so keep them with "diag cmds set-time" first. * test(sharness): drop stale peer count check The connect case opened by re-asserting that the previous case had left zero peers connected. Disconnecting is not permanent: the DHT keeps the other node in its routing table and re-dials it on any refresh, so that count is only true for as long as nothing else runs. What this case is named for, connecting with a bare /p2p/ address, is still covered by the connect itself and the peer count after it. * test(fuse): mount one node at a time Every parallel subtest does identical setup before mounting, so they all reach the mount together and around twenty setuid fusermount helpers open /dev/fuse inside the same instant. One occasionally comes back with a bare exit status 1. Take a lock for the mount call itself, which the subtests only hold for tens of milliseconds. Also report the failure instead of panicking: a panic failed all 37 tests in the package and left daemons behind, and the daemon's stderr, where fusermount says what actually went wrong, was captured and then thrown away. * test: compare cat output byte for byte The payload is 100 random bytes and the comparison ran through Trimmed(), which strips one trailing newline. Roughly one run in 256 ends in 0x0a and loses it. * test: wait for the fast-provide log line The daemon writes the line before it answers the RPC, but the test reads a buffer that a goroutine fills by copying the daemon's stderr, and that copy can still be behind when the command returns. Wait for the line rather than assuming it has landed. * test: allow for ipns republish mid-test A minute after the daemon starts, the republisher re-signs every key and publishes it again, giving the same value a new signature and expiry. The test captured one PUT body and compared it byte for byte with what routing returned, so a run slow enough to straddle that minute compared the first record against the second. Keep every record the mock is sent and require that routing's answer is one of them, which is what the assertion was reaching for. * fix(examples): turn off mdns in library example The example connects its two nodes by address, but left mDNS on, so local discovery could connect them first. A connection opened while a node is still being built is invisible to that node's bitswap, which only learns about connections made after it registers its notifier, and with no routing configured there is nothing to fall back on. The final fetch then waited forever and the test died on its two minute timeout with no clue why. Turning mDNS off makes the explicit dial the only way the two can meet, and keeps the example off the reader's LAN. Alongside that: - connectToPeers returns dial errors instead of logging and continuing into a fetch that cannot succeed - the example's own deadline now fits inside the test budget, so a stall names the step that hung - CommandContext so a hung child does not outlive the test * ci: make helia-interop job resilient Seven failures since v0.42.0 came from this job's setup rather than from any incompatibility. It installs whatever @helia/interop published last, and upstream shipped three packages in a row whose test config does not work from inside node_modules; a GitHub blip took out the rest. - find the compiled specs and pass them to aegir, instead of patching the config upstream ships into node_modules and grepping its text - pin node to a major: setup-node resolves an lts/ alias through a GitHub manifest with no retry and no fallback, and newer node rejects a flag aegir sets unconditionally - retry the registry lookup and fail loudly, since the old one-liner could not fail and left an empty cache key behind - install the exact version the cache key names, and only save the cache once the install is known good - drop the playwright apt packages, unused since this job stopped running browser targets (cherry picked from commit 8ee3dcb)
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
1 participant
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
Since v0.42.0, 224 workflow runs were re-run and then went green. That is the tell for a flake: nothing was wrong with the change, someone just clicked the button and waited. Under those runs sit 49 failed jobs and about ten distinct causes. Two of them are real bugs, not test noise.
Fix
One commit per cause, so any of these can be dropped on its own:
TestAddGCLiveguessed where the adder had got to instead of waiting for itTestIPFSWatchwrote its file in place, so the watcher could add it while still emptyt0065slept 100ms where it needs to wait for the daemont0140re-asserted a peer count the DHT is free to changenode_modulesEverything here came out of the re-run data, so nothing is speculative hardening. The bitswap half of the example hang needs a companion fix in ipfs/boxo; the example stops tripping it either way.
References