Skip to content

fix(request-response): debug_assert panic on last connection close - #6601

Open
procdump wants to merge 1 commit into
libp2p:masterfrom
procdump:fix/panic-request-response-connection-tracking-assert_main
Open

procdump wants to merge 1 commit into
libp2p:masterfrom
procdump:fix/panic-request-response-connection-tracking-assert_main

Conversation

@procdump

@procdump procdump commented Sep 3, 2026

Copy link
Copy Markdown

Description

The problem

I've been experiencing panics with debug builds in some tests around the
rayls blockchain, where rust-libp2p v0.56.0 is used.

After some investigation it turns out request-response records a connection in self.connected
from handle_established_inbound_connection / handle_established_outbound_connection. Those run at
handler-creation time, before the swarm has committed to the connection. In a composed
NetworkBehaviour the derive calls each field's handle_established_* in order and ?-propagates
the result, so a sibling ordered after request-response returning ConnectionDenied aborts the
connection once we have already recorded it. Neither ConnectionEstablished nor ConnectionClosed
ever follows for that id, and the entry is left behind as a phantom.

That has two consequences:

  • self.connected desynchronises from remaining_established, so closing the peer's last real
    connection reports remaining_established == 0 against a non-empty list and trips the
    debug_assert_eq! in on_connection_closed. This is the panic I was seeing.
  • For as long as the phantom is there the peer looks connected, so send_request notifies a
    connection_id that has no handler behind it instead of dialing the peer. This one is silent and
    happens in release builds too.

What's changed

A self.connected entry is now created on FromSwarm::ConnectionEstablished, the first point at
which the swarm has committed to the connection, so connected mirrors the swarm's established set
by construction. handle_established_* become pure handler factories with no side effects.

Queued requests can no longer be preloaded into the handler at construction, so they are emitted as
ToSwarm::NotifyHandler { handler: NotifyHandler::One(connection_id), .. } instead. Same queue, same
connection — they are just dispatched on the next poll rather than baked into the handler.

AI Assistance Disclosure

Tools used (required — write none if no AI was used): Claude Code

Claude Code wrote the two regression tests and helped explore alternative fixes, including checking
whether the change could affect consumers negatively. I reviewed and verified the result.

Attestation (required):

  • I have read every line of this diff, understand what it does, and can explain it in review.

Notes & open questions

Reproducing

Keep the two new tests and restore the old recording logic:

git checkout master -- protocols/request-response/src/lib.rs
cargo test -p libp2p-request-response --test connection_tracking

Further steps

I also have this backported to 0.56.0 here where I no longer observe the panic after a few nights of testing. In this regard if this PR gets merged is it possible that 0.56.1 gets published with this fix in?

I'd be happy to have this resolved so share your feedback.

@procdump
procdump marked this pull request as draft September 3, 2026 12:00
@procdump
procdump marked this pull request as ready for review September 3, 2026 12:03
@procdump
procdump force-pushed the fix/panic-request-response-connection-tracking-assert_main branch 5 times, most recently from e6562bb to 0eb5e30 Compare September 15, 2026 06:27
@procdump
procdump force-pushed the fix/panic-request-response-connection-tracking-assert_main branch 2 times, most recently from 245d69f to 5f0ad36 Compare September 17, 2026 14:23
@procdump
procdump force-pushed the fix/panic-request-response-connection-tracking-assert_main branch from 5f0ad36 to a1b453d Compare September 23, 2026 10:39
Recording at handle_established_* left a phantom entry when a sibling
behaviour denied the connection, tripping the on_connection_closed
debug_assert and mis-routing requests to a dead connection. Record on
FromSwarm::ConnectionEstablished instead, so `connected` mirrors the
swarm's committed set.
@procdump
procdump force-pushed the fix/panic-request-response-connection-tracking-assert_main branch from a1b453d to da8c9e4 Compare September 29, 2026 13:35
MegaRedHand added a commit to lambdaclass/ethlambda that referenced this pull request Oct 1, 2026
## 🗒️ Description / Motivation

A Platåberget follower built with `release-fast` (which keeps
`debug-assertions` on) lost its P2P task to a panic inside libp2p
request-response. The process, the API and the chain actor kept running,
so the head froze for about five hours with nothing but this in the log:

```
panicked at .../rust-libp2p-da8daccbaa8a6b4a/2f14d0e/protocols/request-response/src/lib.rs:708:9:
assertion `left == right` failed
  left: false
 right: true
```

That line is `debug_assert_eq!(connections.is_empty(),
remaining_established == 0)` in `on_connection_closed`: request-response
still counted a connection to a peer the swarm said had none left.

The cause is the field order of `Behaviour`, where `connection_limits`
came last:

| Step | What happens |
|---|---|
| 1 | The swarm calls `handle_established_*_connection` on `Behaviour`;
the derive calls each field in declaration order with `?` |
| 2 | Every `request_response::Behaviour` in `ReqResp` records the
connection in its `connected` map (`preload_new_handler`) |
| 3 | `connection_limits` refuses it (per-peer, total, inbound or
outbound ceiling) |
| 4 | The swarm reports a `ListenFailure`/`DialFailure`, and never a
`ConnectionEstablished` or `ConnectionClosed` for it. Request-response
ignores both, so the entry stays |
| 5 | The peer's last real connection closes with `remaining_established
== 0`, while request-response still holds the phantom: the assert fires
|

Without debug assertions the phantom is worse than a leak.
`try_send_request` picks a connection by `request_id %
connections.len()`, so some requests to that peer go to
`NotifyHandler::One(phantom)`. The swarm drops an event for an unknown
connection silently (`swarm/src/lib.rs:1214` in the pinned fork), and
the request timeout lives in the handler that was never spawned, so no
`OutboundFailure` ever comes back. ethlambda retires in-flight requests
on `OutboundFailure`.

## What Changed

- `crates/net/p2p/src/lib.rs`: `connection_limits` is now the first
field of `Behaviour` (and of its struct literal in `build_swarm`), with
a doc comment saying why it has to stay first. A refusal now happens
before any other behaviour sees the connection.
- `crates/net/p2p/src/lib.rs` (tests): a regression test, below.

## Correctness / Behavior Guarantees

- Which connections are refused is unchanged: same limits, same counts.
What changes is that no other behaviour sees a refused connection.
- `connection_limits` is safe to put first. It records established
connections only on `FromSwarm::ConnectionEstablished`, emits no events
(`poll` is always `Pending`), and its handler is `dummy`, so the
protocols offered and the event dispatch are unchanged.
- No field after it refuses connections, so no behaviour can be left
holding a phantom.
- Lean runs `unlimited_connections()`, which never refuses, so the lean
network is unaffected.

## Tests Added / Run

-
`tests::a_connection_the_limits_refuse_leaves_no_request_response_state`
builds the real beacon swarm with `build_swarm` and drives its
`Behaviour` the way the swarm does: two connections from one peer
(`MAX_CONNECTIONS_PER_PEER`), a third refused and reported as a
`ListenFailure`, then both held connections close. Before the fix it
fails in both build modes:
- with debug assertions (`release-fast`, as CI runs it): the production
panic at `request-response/src/lib.rs:708:9`
- without (`CARGO_PROFILE_RELEASE_FAST_DEBUG_ASSERTIONS=false`): its own
assertion, since the field still reports the peer as connected

  After the fix it passes in both.
- `cargo test -p ethlambda-p2p --lib --profile release-fast`: 208
passed, 1 ignored.

## Related Issues / PRs

- libp2p/rust-libp2p#4773: the same assert, open. The maintainers'
advice there is to put connection-management behaviours first, which is
what this does.
- libp2p/rust-libp2p#4870: the underlying design issue (the `handle_*`
callbacks take `&mut self`), open.
- libp2p/rust-libp2p#6601: fixes request-response itself by recording
connections on `ConnectionEstablished`. Open and unreviewed; it could be
cherry-picked into `lambdaclass/rust-libp2p` later, and this PR does not
depend on it.

## ✅ Verification Checklist

- [x] Ran `make fmt` — clean for this change. `cargo fmt --all --
--check` also flags the `mod` order in `bin/ethlambda/src/main.rs`,
which is already on `beacon-chain-integration` (from c635524) and left
out of this PR
- [x] Ran `make lint` (clippy with `-D warnings`) — clean
- [ ] Ran `make test` (`test-consensus` plus `test-node`, at
`release-fast`) — left to CI; ran the p2p unit tests above

This branch has not been deployed

No deployments
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.

1 participant