Skip to content

fix: settle connection.closed() when the connection is aborted - #66

Merged
nflaig merged 3 commits into
mainfrom
nflaig/settle-closed-on-abort
Aug 9, 2026
Merged

fix: settle connection.closed() when the connection is aborted#66
nflaig merged 3 commits into
mainfrom
nflaig/settle-closed-on-abort

Conversation

@nflaig

@nflaig nflaig commented Aug 8, 2026

Copy link
Copy Markdown
Member

Connection::closed() resolves only once quinn reports the connection closed, so if the connection driver stalls it never resolves - the same condition #64 works around at the endpoint level. The JS side keeps one of these pending per connection for its whole lifetime (src/connection.ts), and it is the only thing that tells the transport the connection died:

this.#connection.closed().then(() => { this.onTransportClosed() }, (err) => { this.onTransportClosed(err) })

So a stalled driver does not just leak the napi deferred behind that promise, it leaves libp2p believing a dead connection is still live.

Settle closed() on abort() as well as on the connection actually closing. watch + send_replace rather than Notify or plain send, both of which drop the signal when nothing has subscribed yet, and subscribe() + borrow_and_update() so an abort() that lands before the first closed() is still seen.

No API change, closed() is still Promise<void> and abort() still void.

Worth a close look on review: onTransportClosed() now fires when abort() is called rather than when quinn confirms the close. That is a small timing change in the connection lifecycle and I am not the right person to judge whether anything downstream depends on the old ordering.

This does not fix the Lodestar shutdown hang I originally wrote it for

I believed the pending closed() deferreds left the worker thread blocked in native code. gdb stacks from a live wedged Lodestar worker show otherwise - the thread is spinning in Node's own Environment::CleanupHandles(), which loops uv_run(UV_RUN_ONCE) until handle_wrap_queue_ drains:

Thread 73 (LWP 1524870 "WorkerThread"):     <- state R, on CPU
#2  uv_run (loop=0x7fd473dc6938, mode=UV_RUN_ONCE)
#3  node::Environment::CleanupHandles()
#4  node::Environment::RunCleanup()
#5  node::worker::Worker::Run()

A later capture walked that loop and found 6 handles left, only 2 of them plain UV_ASYNC, fewer than a healthy loop has. Not a pile of leaked napi threadsafe functions.

Measured on a mainnet node, soaking at ~200 peers with 90-160 live inbound QUIC connections for at least 5 minutes before each shutdown:

shutdowns worker failed to terminate
2.1.2 5 1
this patch 20 4

Identical rate, so the hypothesis was wrong. No regression observed over those 20 shutdowns either.

Review this as a correctness fix - a promise that can never settle, and a transport that never learns its connection died - not as a fix for anything. Lodestar side mitigation, which keeps a stuck worker from costing the node its state archive, is ChainSafe/lodestar#9790. Root cause notes and captures: https://gist.github.com/nflaig/b266d89c03cdd2c76338823afed5b2c0

AI Assistance Disclosure

Written and measured with Claude Code, validated on a mainnet node as above.

🤖 Generated with Claude Code

nflaig and others added 2 commits August 8, 2026 11:09
`Connection::closed()` only resolves once quinn reports the connection closed,
which never happens if the connection driver stalls - the same condition #64
works around at the endpoint level. The JS side keeps one of these pending per
connection for its whole lifetime, each backed by a napi deferred on the calling
thread, so a pending one keeps that thread from finishing teardown.

Settle on `abort()` as well. `watch` rather than `Notify` so an `abort()` that
lands before `closed()` subscribes is not lost.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pped

`watch::Sender::send` returns early without updating the value when there is no
receiver, and the receiver is only created inside `closed()`. An `abort()` before
the first `closed()` would be lost, which is the race this was meant to close.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@spiral-ladder spiral-ladder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm wiht optional comment

Comment thread rust/lib.rs Outdated
Co-authored-by: bing <spiralladder@fastmail.com>
@nflaig
nflaig merged commit 554deb1 into main Aug 9, 2026
18 checks passed
nflaig added a commit to ChainSafe/lodestar that referenced this pull request Aug 9, 2026
Picks up ChainSafe/js-libp2p-quic#66, released as 2.1.3.

`Connection::closed()` only resolves once quinn reports the connection
closed, so a stalled connection driver leaves it pending forever, and
with it the napi deferred and threadsafe function backing that promise.
libp2p will eventually drop such a peer on its own and call `abort()`,
but before 2.1.3 the promise stayed pending even then, so each affected
connection leaked those resources for the lifetime of the process. 2.1.3
settles it on `abort()` as well, which turns a permanent leak into none.

To be clear about what it does not do: it does not make libp2p notice a
dead peer any sooner. By the time `abort()` is called libp2p has already
concluded the connection is gone and called `onTransportClosed()`
itself. This is resource hygiene, not a peer state fix.

**This is not a fix for the shutdown hang.** I originally wrote that
patch believing the pending promises were what kept the network worker
from terminating. They are not - gdb stacks from a live wedged worker
show it spinning in Node's `Environment::CleanupHandles()` on a libuv
handle that never closes, and the wedge rate was identical with and
without the patch (4 in 20 shutdowns vs 1 in 5 on 2.1.2). #9790 is the
mitigation for that, and the underlying handle is still unidentified.

On the timing of `onTransportClosed()`: `abort()` runs on every locally
initiated close, not just at shutdown, so it is worth being precise
about what changes. libp2p already calls `onTransportClosed()` itself
immediately after `sendClose()` (`abstract-multiaddr-connection.js`),
and the method guards every state transition, so it is idempotent. The
settled promise therefore produces a redundant call at a moment the
framework was transitioning anyway, rather than an genuinely earlier
notification. A remote initiated close does not call `abort()` at all
and is unaffected. The change only has an observable effect in the
stalled driver case it was written for.

**Testing**

2.1.3 is byte-identical in behaviour to the build I ran on a mainnet
node for 20 shutdowns at ~200 peers with 90-160 live inbound QUIC
connections, no regression observed. Locally, install resolves cleanly,
the native addon loads and exposes the unchanged API surface, and
typecheck passes.

**AI Assistance Disclosure**

Dependency bump and validation with Claude Code.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
nflaig added a commit to ChainSafe/lodestar that referenced this pull request Aug 9, 2026
Picks up ChainSafe/js-libp2p-quic#66, released as 2.1.3.

`Connection::closed()` only resolves once quinn reports the connection
closed, so a stalled connection driver leaves it pending forever, and
with it the napi deferred and threadsafe function backing that promise.
libp2p will eventually drop such a peer on its own and call `abort()`,
but before 2.1.3 the promise stayed pending even then, so each affected
connection leaked those resources for the lifetime of the process. 2.1.3
settles it on `abort()` as well, which turns a permanent leak into none.

To be clear about what it does not do: it does not make libp2p notice a
dead peer any sooner. By the time `abort()` is called libp2p has already
concluded the connection is gone and called `onTransportClosed()`
itself. This is resource hygiene, not a peer state fix.

**This is not a fix for the shutdown hang.** I originally wrote that
patch believing the pending promises were what kept the network worker
from terminating. They are not - gdb stacks from a live wedged worker
show it spinning in Node's `Environment::CleanupHandles()` on a libuv
handle that never closes, and the wedge rate was identical with and
without the patch (4 in 20 shutdowns vs 1 in 5 on 2.1.2). #9790 is the
mitigation for that, and the underlying handle is still unidentified.

On the timing of `onTransportClosed()`: `abort()` runs on every locally
initiated close, not just at shutdown, so it is worth being precise
about what changes. libp2p already calls `onTransportClosed()` itself
immediately after `sendClose()` (`abstract-multiaddr-connection.js`),
and the method guards every state transition, so it is idempotent. The
settled promise therefore produces a redundant call at a moment the
framework was transitioning anyway, rather than an genuinely earlier
notification. A remote initiated close does not call `abort()` at all
and is unaffected. The change only has an observable effect in the
stalled driver case it was written for.

**Testing**

2.1.3 is byte-identical in behaviour to the build I ran on a mainnet
node for 20 shutdowns at ~200 peers with 90-160 live inbound QUIC
connections, no regression observed. Locally, install resolves cleanly,
the native addon loads and exposes the unchanged API surface, and
typecheck passes.

**AI Assistance Disclosure**

Dependency bump and validation with Claude Code.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants