Repository navigation
fix(autonat-v2): apply timeout to verification requests - #3658
Merged
Merged
Conversation
Signed-off-by: kediyalua <kediyalua@outlook.com>
Wait for the queue to go idle instead of a fixed delay, assert the stream is aborted and the nonce cleaned up, use the short timeout only in this test and remove the vacuous timeout test.
An unresponsive remote is an expected network condition.
tabcat
approved these changes
Oct 11, 2026
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
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.
Title
fix(autonat-v2): apply timeout to verification requests
Description
AutoNATv2Client.verifyExternalAddresses()creates a combined abort signal forevery queued verification job:
...but then passes the original
optionsobject toaskPeerToVerify()insteadof that signal, so the only thing the combined signal is used for is
signal.clear()in thefinallyblock. As a result thetimeoutoption(
TIMEOUT= 30s by default) never applied to the client side of the AutoNAT v2exchange:
connection.newStream(),messages.write()andmessages.read()could all block forever if the remote accepted the stream but never answered.
This looks like an oversight rather than a design decision - the surrounding
anySignal(...)/signal.clear()scaffolding only makes sense if the signal isactually used, and the rest of the module does use its timeout signals
(
handleDialBackStream()appliesAbortSignal.timeout(this.timeout)to itsreads/writes,
findRandomPeers()passes its combined signal torandomWalk.walk()andopenConnection()).The fix forwards the combined signal to
askPeerToVerify():setMaxListeners(Infinity, signal)is added for consistency withkad-dht/src/query/manager.ts- the signal is now listened to by every streamoperation performed by the job.
A regression test is included: it configures the service with the existing 100ms
timeoutand uses a remote that accepts the dial-request stream but never sendsa
DialResponse. Before the fix the stream staysopenforever and the testfails; after the fix the stream is aborted shortly after the timeout elapses.
Fixes https://github.com/libp2p/js-libp2p/issues/ISSUE_NUMBER
Notes & open questions
should time out when verifying an observed addresscase assertsaddressManager.addObservedAddrwas not called, but the AutoNAT v2 client onlyever calls
confirmObservedAddr/removeObservedAddr, so that assertion istrivially true and the case does not actually exercise any timeout behaviour.
I left it untouched to keep this PR focused - happy to fix or remove it in a
follow-up if you prefer.
findRandomPeers()builds ananySignalthat is neverclear()ed, which leaves its 10sAbortSignal.timeout()timer alive. Notaddressed here; say the word and I'll include it.
Change checklist