Skip to content

fix(tcp): close socket when reset races shutdown - #3597

Merged
tabcat merged 5 commits into
libp2p:mainfrom
GrapeBaBa:experiment/minimal-reset-shutdown
Aug 20, 2026
Merged

fix(tcp): close socket when reset races shutdown#3597
tabcat merged 5 commits into
libp2p:mainfrom
GrapeBaBa:experiment/minimal-reset-shutdown

Conversation

@GrapeBaBa

@GrapeBaBa GrapeBaBa commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Description

Avoid calling resetAndDestroy() after a graceful socket shutdown has already started.

socket.end() sets writableEnded and starts the native shutdown path. If connection abort then races the close timeout, resetAndDestroy() attempts a reset while that shutdown request is pending. On Node.js 24.19.0 this can leave the native TCP handle referenced during process teardown. Use destroy() for that state so the pending shutdown is closed normally; sockets that have not started graceful shutdown still use resetAndDestroy().

This was isolated while investigating the Lodestar shutdown hang reported here:
https://gist.github.com/nflaig/5f41cfc50f38baf5046a034162943dc3

Validation

The mainnet A/B candidate used:

  • unmodified Lodestar commit de5f89eae1cc6bd80d62124de5defeb8e2182297
  • @libp2p/tcp based on tcp-v11.0.13 (3565a2ec5e233c717929580002c554db36c667e0)
  • only the five-line sendReset() guard in this PR
  • Node.js 24.19.0

An independent rootfs comparison confirmed that the candidate image differed from the stock Lodestar image only in the compiled socket-to-conn.js and its source map. It did not include the other dial queue, listener, socket tracking, or Lodestar shutdown changes from the earlier investigation.

The candidate completed 50 effective mainnet shutdown cycles with 50 clean exits and 0 hangs. Each cycle reused the same container, waited for the node to report synced, soaked for about six minutes, then used docker stop -t 60 followed by docker start for the next cycle.

  • npm run build
  • 50-cycle Lodestar mainnet shutdown soak: 50 clean, 0 hangs
  • Repository unit tests were not rerun for this draft

Notes & open questions

This draft supersedes #3596, which contained additional shutdown and dial-draining changes that were not required by the isolated 50-cycle result.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@GrapeBaBa
GrapeBaBa marked this pull request as ready for review August 14, 2026 02:42
@GrapeBaBa
GrapeBaBa requested a review from a team as a code owner August 14, 2026 02:42

@nflaig nflaig left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM, we have tested this fix extensively and it resolves our shutdown hang issues in Lodestar

@seetadev seetadev 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.

Awesome 💯 Ccing @dozyio and @tabcat before merging it.

@matthewkeil matthewkeil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤩

matthewkeil pushed a commit to ChainSafe/lodestar that referenced this pull request Aug 14, 2026
Applies libp2p/js-libp2p#3597 as a local `pnpm patch` until it is
released. This fixes the network worker shutdown hang, the underlying
handle that #9790 mitigated but did not identify.

## Root cause

`TCPSocketMultiaddrConnection.sendReset()` calls
`socket.resetAndDestroy()` unconditionally. When the writable side has
already ended that does not tear the handle down, so the socket stays
alive as an active `TCPSocketWrap` while libp2p considers it closed.
Nothing else holds a reference, so nothing ever closes it.

Node's worker teardown then spins forever, because
`Environment::CleanupHandles()` runs until every handle is closed:

```cpp
while (handle_cleanup_waiting_ != 0 || request_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) {
  uv_run(event_loop(), UV_RUN_ONCE);
}
```

The thread never exits, so `Worker.terminate()` never resolves, so the
main thread blocks in `uv_thread_join` from `process.exit()` until the
process manager kills it.

The patch guards the case the reset cannot handle:

```js
sendReset (): void {
  if (this.socket.writableEnded) {
    this.socket.destroy()
    return
  }
  this.socket.resetAndDestroy()
}
```

## Why the exact-version pin

`@libp2p/tcp` is declared as `^11.0.13` and `patchedDependencies` keys
are version exact, so an 11.0.14 release would resolve past the patch
and silently drop the fix. The `overrides` pin prevents that, same as
the existing `sigstore` patch. It does mean no `@libp2p/tcp` bump until
the patch is dropped.

## Testing

Validated on a mainnet node against a baseline hang rate of 4 in 36
shutdowns (11.1%).

70 consecutive shutdowns, synced with >=200 peers and a 5 minute soak
each:

| criterion | result |
| --- | --- |
| hangs | 0 / 70 |
| `TCPSocketWrap` present after `libp2p.stop()` | 0 / 70 |
| worker terminate | 0.027-0.072s, hang signature is 3.000s |
| exit code | 0 on all 70, never SIGKILLed |
| shutdown duration | mean 7.20s, max 9.8s |
| state archived | 70 / 70 |

Plus one shutdown at 9.07h uptime, since orphaned socket counts grew
with uptime: terminate 0.180s, no `TCPSocketWrap`, exit 0.

Counted independently from the raw node logs as well as from the test
harness. The last unpatched shutdown on the same machine, three minutes
before deploying the patched build, hung at 3.001s with
`activeResources=MessagePort=1,TCPSocketWrap=6,Timeout=1`.

Observing 70 consecutive clean shutdowns if the bug were still present
has probability 0.00026. Fisher exact against the measured baseline
gives p = 0.012.

The `activeResources` logging added by #9790 is what made this
diagnosable, the presence of `TCPSocketWrap` separated 5 hangs from 32
clean shutdowns perfectly (Fisher p = 0.0000023).

Longest uptime tested was 9.07h. The worst case observed, 28 orphaned
sockets, took ~38h to accumulate, so this validates the mechanism rather
than proving an upper bound.

Full write-up:
https://gist.github.com/nflaig/5f41cfc50f38baf5046a034162943dc3

## When to remove

Drop the patch and the `overrides` pin together once js-libp2p#3597 is
released and `@libp2p/tcp` is bumped to a version containing it.

## AI Assistance Disclosure

Investigation, patch 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>
@GrapeBaBa

Copy link
Copy Markdown
Contributor Author

Open the PR to fix the CI failed, not sure if that repo is still alive?

@Faolain

Faolain commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Open the PR to fix the CI failed, not sure if that repo is still alive?

it got merged (libp2p might also need the nodedatachannel dependency update as well)

@tabcat
tabcat self-requested a review August 19, 2026 23:39
Adds a regression test that fails without the sendReset() guard, and a
companion test pinning the reset path for sockets that have not started
a graceful shutdown. Both assert the socket was torn down rather than
that a method was called, so a no-op or widened guard is caught.
@tabcat
tabcat merged commit 55d7a53 into libp2p:main Aug 20, 2026
34 of 35 checks passed
@tabcat

tabcat commented Aug 20, 2026

Copy link
Copy Markdown
Member

@GrapeBaBa Thanks!

@tabcat tabcat mentioned this pull request Aug 20, 2026
wemeetagain added a commit to ChainSafe/lodestar that referenced this pull request Aug 24, 2026
## Summary

Bump libp2p and related dependencies to the latest compatible published
versions, including the released TCP shutdown fix from
libp2p/js-libp2p#3597.

### Updated

- `libp2p`: `3.1.6` -> `3.3.9`
- `@libp2p/tcp`: `^11.0.13` -> `^11.0.27`
- `@libp2p/interface`: `^3.1.0` -> `^3.3.0`
- align the remaining direct `@libp2p/*`, reqresp, and datastore
dependencies with the compatible release set
- remove the temporary `@libp2p/tcp@11.0.13` patch and exact override
added in #9825
- refresh `pnpm-lock.yaml`

Co-authored-by: Cayman <caymannava@gmail.com>
Co-authored-by: bing <spiralladder@fastmail.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.

6 participants