fix(ws): process pending handshakes concurrently

[richard-ramos]

Summary

This PR fixes WebSocket accept head-of-line blocking caused by slow or malformed HTTP upgrade requests.

WsTransport now accepts raw HTTP streams separately from WebSocket handshake parsing. A dispatcher owns HttpServer.acceptStream(), while bounded handshake workers parse headers with readHttpRequest() and pass valid requests to WSServer.handleRequest(). Successfully upgraded connections are queued for WsTransport.accept().

This prevents one incomplete WebSocket handshake from blocking later valid WebSocket connections until the header timeout expires.

Requires:

• chore: expose HTTP accept primitives status-im/nim-websock#193

Should fix:

• bug: WSS handler socket leak logos-messaging/logos-delivery#3634

Affected Areas

• Gossipsub
• Transports
  WebSocket transport accept/handshake path, dependency bump to nim-websock PR 193. Do not merge until then
• Peer Management / Discovery
• Protocol Logic
• Build / Tooling
• Other

Compatibility & Downstream Validation

Reference PRs / branches / commits demonstrating successful integration:

• Nimbus:
  N/A

• Waku:
  N/A

• Codex:
  N/A

Impact on Library Users

This changes the public WsTransport.new timeout parameter from handshakeTimeout to headersTimeout, with the former being deprecated in nim-websocks.

Behaviorally, malformed or slow WebSocket handshakes are now handled inside the transport and should no longer cause the switch accept loop to stop or block valid WebSocket accepts behind header timeouts.

The WebSocket transport also gains a configurable concurrentAccepts limit, defaulting to 200, to bound concurrent handshake work and accepted-connection queueing.

Risk Assessment

Risk is moderate because this changes the WebSocket accept path and task lifecycle.

Main risks:

• Accept ordering is no longer deterministic when multiple WebSocket handshakes are in flight.
• Downstreams using the old named constructor argument handshakeTimeout must rename it to headersTimeout.
• Very large malformed-connection floods can still consume bounded accept worker capacity, sockets, TLS resources, or OS backlog, but they should no longer serialize all WebSocket accepts one header timeout at a time.

Mitigations:

• Handshake concurrency is bounded.
• Successful accepts are queued through a bounded internal queue.
• Malformed and slow handshakes are logged and closed inside the transport.
• WebSocket transport tests include a regression for slow incomplete headers not blocking a valid accept.

References

• bug: WSS handler socket leak logos-messaging/logos-delivery#3634
• chore: expose HTTP accept primitives status-im/nim-websock#193
• Consumed nim-websock commit: 1c12189667ff5586cb81a2e43fd4a19190b7060d

Additional Notes

Verified locally with:

nim c --path:. tests/libp2p/transports/test_ws.nim
./tests/libp2p/transports/test_ws --output-level=VERBOSE

[Copilot]

Pull request overview

Updates WsTransport’s inbound accept path to avoid head-of-line blocking from slow/malformed HTTP upgrade requests by splitting raw stream acceptance from WebSocket handshake parsing and bounding handshake concurrency.

Changes:

• Refactors WebSocket accept flow to use an accept dispatcher + bounded handshake workers and a bounded queue of successfully-upgraded connections.
• Renames the public constructor argument from handshakeTimeout to headersTimeout and adds concurrentAccepts to bound in-flight handshake work.
• Adds a regression test for “slow headers don’t block valid accepts” and adjusts stream transport tests to tolerate nondeterministic accept order.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
libp2p/transports/wstransport.nim	Implements concurrent accept/handshake pipeline and new configuration knobs (headersTimeout, concurrentAccepts).
tests/libp2p/transports/test_ws.nim	Adds regression test to ensure slow/incomplete headers don’t block a valid accept.
tests/libp2p/transports/stream_tests.nim	Updates connection-parallelism test to be robust to nondeterministic accept order in concurrent transports.

[gmelodie]

This proc is only here so you can wait(self.headersTimeout)? If so, maybe create a template so you can

withTimeout(self.headersTimeout):
  your_code

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 71.59091% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.81%. Comparing base (c6ac669) to head (6467d4f).

Files with missing lines	Patch %	Lines
libp2p/transports/wstransport.nim	71.59%	50 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2359      +/-   ##
==========================================
+ Coverage   73.77%   73.81%   +0.03%     
==========================================
  Files         150      150              
  Lines       19810    19910     +100     
  Branches       19       19              
==========================================
+ Hits        14615    14696      +81     
- Misses       5195     5214      +19     

Files with missing lines	Coverage Δ
libp2p/transports/wstransport.nim	81.79% <71.59%> (+0.06%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

slow.close() is used after the accept/dial succeeds, but it does not wait for the underlying transport to actually close. Since this test is asserting timing behavior and then immediately tears down connections/transports, prefer await slow.closeWait() here (with appropriate error handling) to reduce flakiness from lingering sockets during teardown.

[vladopajic]

does it make sense to do this in deffer? would avoid same pattern in CancelledError as well

[vladopajic]

could this be done in defer? it would avoid this gigantic try block and indentation

[vladopajic]

self.connections.values().mapIt(it.close()) ?

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

libp2p/transports/wstransport.nim:389

• stop() enqueues the nil sentinel via notifyAcceptClosed() before cancelling acceptLoop / handshakeFuts. A handshake worker can still successfully enqueue a real Connection after this sentinel, causing accept() to pop nil and raise TransportClosedError even though there are valid connections still queued behind it (leaving them unaccepted/leaked).

Consider only signalling closure after all handshake workers are stopped (or preventing workers from enqueueing once shutdown begins), so the sentinel cannot be inserted ahead of real connections.

  self.running = false # mark stopped as soon as possible
  self.notifyAcceptClosed()

  try:
    trace "Stopping WS transport"
    await procCall Transport(self).stop() # call base

    var toWait: seq[Future[void]]
    if not isNil(self.acceptLoop) and not self.acceptLoop.finished:
      toWait.add(self.acceptLoop.cancelAndWait())

    for fut in self.handshakeFuts:
      if not fut.finished:
        toWait.add(fut.cancelAndWait())

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.