Skip to content

fix(core): report WebSocket bind failures instead of panicking - #775

Open
cds-amal wants to merge 4 commits into
solana-foundation:mainfrom
cds-rs:fix/musical-ports
Open

fix(core): report WebSocket bind failures instead of panicking#775
cds-amal wants to merge 4 commits into
solana-foundation:mainfrom
cds-rs:fix/musical-ports

Conversation

@cds-amal

@cds-amal cds-amal commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

#774's CI run hit a WebSocket port race, but the bind failure surfaced as a misleading runtime panic:

parity_ws bind error
Cannot drop a runtime in a context where blocking is not allowed
Failed to receive WebSocket RPC server startup result

The port race is a separate issue. This PR fixes the failure path so a lost WebSocket port reports the bind error that caused it.

How

jsonrpc-ws-server creates a private Tokio runtime for its event loop and drops it when start() fails.

Our WebSocket starter called start() inside runtime.block_on. On a bind failure, that private runtime was therefore dropped from an async context, which panics. The handler thread died before it could publish the startup error, hiding the useful failure: which port failed to bind and why.

Fix

  • Give the WebSocket server the executor already used by the pubsub tasks (event_loop_executor), eliminating its private runtime and worker threads.
  • Share one server-thread scaffold between the HTTP and WebSocket starters instead of maintaining two copies of the same lifecycle logic.
  • Publish a startup error on the handshake channel before emitting the aborted event (aborted delivery blocks under backpressure, and the caller waiting on the handshake may be the thread that drains events), and emit shutdown when a server exits abnormally.
  • Close the already-running HTTP server when WebSocket startup fails instead of leaving its port bound.
  • Make both starters plain functions; neither awaited anything.

Server behavior is otherwise unchanged: same servers, ports, and close handles.

How to test

The commits are ordered so both states reproduce. The regression test ws_bind_failure_reports_the_bind_error lands first: it occupies the WebSocket port and expects the bind error through the startup channel plus the aborted event. It is red at its own commit against the unfixed code, and the fix commit turns it green.

Baseline, from the PR branch (shows the CI failure pair: the runtime-drop panic and the masked channel error):

git checkout HEAD^
cargo test -p surfpool-core --lib ws_bind_failure_reports -- --nocapture

Fix (the bind error reaches the caller, no panic):

git checkout -
cargo test -p surfpool-core --lib ws_bind_failure_reports
  • All 90 WebSocket integration tests pass on the shared executor.
  • test_simnet_ticks passes end to end.
  • Clippy clean on changed lines.

NOTE:

The port race itself remains: get_free_port binds :0, reads the assigned port, releases it, and hopes the port remains available; the runloop preflight check has the same time-of-check-time-of-use (TOCTOU) shape.

- Occupy the WebSocket port, call the WS starter, and expect the bind
  error through the startup channel plus the aborted event.
- Red at this commit: the handler thread panics dropping a runtime in an
  async context, and the starter reports only the generic channel error.
  The next commit makes it green.
- jsonrpc-ws-server builds a private tokio runtime and drops it when a
  bind fails; dropped inside an async context, that panics the handler
  thread before the error report runs, masking the bind error as a
  channel failure. Seen in CI when a test port was claimed between the
  preflight check and the server bind.
- Hand the ws server the runtime the pubsub tasks already use
  (event_loop_executor), removing the private runtime and its threads.
- Extract one server-thread scaffold shared by the HTTP and WebSocket
  starters; both become plain fns (neither awaits). Emit the aborted
  event before publishing the startup error, and emit shutdown even when
  the server exits abnormally.
- Close the HTTP server before propagating a WebSocket startup error, so
  a failed startup does not orphan it with its port bound.
- The regression test from the previous commit goes green; its call
  site sheds the block_on wrapper along with the API.
@greptile-apps

greptile-apps Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR restructures HTTP and WebSocket server startup around a shared thread scaffold and ensures WebSocket bind failures reach the startup caller without a Tokio runtime-drop panic.

  • Supplies the WebSocket server with an explicitly owned Tokio runtime and executor.
  • Sends startup errors before potentially blocking aborted events.
  • Closes the already-running HTTP server when WebSocket startup fails.
  • Adds regression coverage for occupied WebSocket ports.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
crates/core/src/runloops/mod.rs The shared RPC startup scaffold correctly publishes handshake errors before blocking event delivery, resolving the previously reported deadlock path.

Reviews (4): Last reviewed commit: "docs(core): clean up docs" | Re-trigger Greptile

Comment thread crates/core/src/runloops/mod.rs
- aborted() blocks on a full events buffer, and the caller waiting on
  the startup handshake may be the thread that drains events, so
  emitting the event first can deadlock startup: caller waits on
  handler, handler waits on the drain the caller runs.
- Publish the handshake error first; the aborted event follows once the
  drain runs. Verified with a Spin model across both orders and both
  buffer states.
- Drain events blocking with a deadline in the regression test; a
  nonblocking poll races the aborted send even in the fixed order.
- Name the servers-handle tuple; dropping async exposed a
  type_complexity lint on the old inline signature.
@cds-amal
cds-amal marked this pull request as draft August 23, 2026 00:05
@cds-amal
cds-amal marked this pull request as ready for review August 25, 2026 19:26
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