Skip to content

Preview: forward the HMR subprotocol, and never reset a connecting upstream - #90

Merged
bharathm03 merged 1 commit into
developmentfrom
fix/preview-ws-subprotocol
Sep 3, 2026
Merged

Preview: forward the HMR subprotocol, and never reset a connecting upstream#90
bharathm03 merged 1 commit into
developmentfrom
fix/preview-ws-subprotocol

Conversation

@bharathm03

Copy link
Copy Markdown
Contributor

Vite/Astro HMR never connected through the previewer proxy. The browser console showed ws://localhost:4321/?token=... failed and [vite] failed to connect to websocket, and probing the proxy also took the dev server down.

Three distinct faults, all verified against a live Astro dev server before the fix:

1. The app's 101 answered with no subprotocol echo

Chromium fails a handshake outright when it sent Sec-WebSocket-Protocol and the response carries none ("Sent non-empty 'Sec-WebSocket-Protocol' header but no response was received"). PreviewProxyServer now hands the browser's own list to webSocketHandler, so the 101 echoes the browser's first choice.

2. The bridge stripped the subprotocol before dialing the dev server

Sec-WebSocket-Protocol was in WS_HOP_BY_HOP_HEADERS, so the upstream request never named vite-hmr. Every Vite-family dev server (Astro, Nuxt, SvelteKit, plain Vite) upgrades an HMR request only when it names that protocol, and one without it is left unanswered rather than refused — which is why the failure had no error anywhere to point at. The list is now forwarded to Bun as protocols.

3. Closing a tunnel mid-handshake reset the socket, and that killed the dev server

This is what crashed Astro during diagnosis:

node:events:487
      throw er; // Unhandled 'error' event
Error: read ECONNRESET at TCP.onStreamRead

Bun aborts a socket that never finished its handshake with a RESET, and Node hands an upgrade request to its listeners with its own error handler already removed — so a dev server that ignored the upgrade (which fault 2 guaranteed) holds that socket with no error listener at all, and the reset exits the process. Measured on Node 26.

releaseUpstream now parks a still-connecting socket and closes it once it opens, bounded by WS_ABANDONED_MAX. A parked socket is marked abandoned, so nothing it later says is routed under a tunnel id that may already name a newer tunnel.

Also, from review

openUpstream wraps the WebSocket constructor. Bun throws SyntaxError for a subprotocol that is not an RFC 6455 token (the common bearer.<base64>= auth-smuggling pattern is enough — = is not a token character), nothing between the relay's message listener and onWsOpen catches, and index.ts answers an uncaught exception by shutting the host down. One previewed page would have killed every agent on the machine. The one tunnel is refused instead.

Testing

  • bridge/tests/tunnel-manager-ws-subprotocol.test.ts — 7 new tests: the subprotocol reaches a Vite-shaped upstream, multi-entry order is preserved, none is sent when none was asked for, a mid-handshake close sends no reset and closes gracefully after a late 101, a parked socket never speaks for a reused tunnel id, the park cap evicts the oldest, and a constructor-refused subprotocol ends one tunnel rather than the host.
  • app/test/services/preview_proxy_server_ws_test.dart — 4 new tests over the raw bytes of the 101 (dart:io's client tolerates a missing echo, Chromium does not, so only the wire proves it).
  • Gates: full bridge suite green, tsc --noEmit clean, flutter test 19/19 on both proxy files, flutter analyze clean.

Known limits (documented in bridge/CLAUDE.md, not fixed here)

  • The park has no TTL and its cap is per-TunnelManager. Bun raises no handshake timeout (measured: readyState stayed 0 past 45s), and each checkout teardown discards the manager and builds a fresh budget, so parked sockets can accumulate across manager lifetimes.
  • Past the cap, eviction performs the very mid-handshake reset the park exists to avoid. It delays the crash rather than preventing it, and it logs when it happens.
  • The echo stays optimistic. A server that picks a later entry than the browser's first is logged but not corrected; one that echoes nothing cannot be detected at all, since Bun reports ws.protocol as the first entry offered either way. Closing that needs a new wire field and a reordered handshake.
  • PreviewService.dispose() cancels each tunnel's subscription before closing its sink, so onDone never fires and the bridge keeps an open upstream. Pre-existing, in a file this PR does not touch.

https://claude.ai/code/session_01Ca7qjBqVbkUGQ71TewJZN5

…stream

A Vite-family dev server (Astro, Nuxt, SvelteKit, plain Vite) upgrades an HMR request only when it names `vite-hmr`, and the previewer proxy dropped `Sec-WebSocket-Protocol` in BOTH directions: the app's 101 answered the browser with no echo, which Chromium fails outright, and the bridge stripped the header before dialing the dev server, which left the upgrade unanswered rather than refused. The app now echoes the browser's first choice and the bridge forwards the whole list to Bun as `protocols`.

The bridge also closed an upstream socket whose tunnel ended while its handshake was still in flight. Bun aborts an unfinished handshake with a RESET, and Node hands an `upgrade` to its listeners with its own error handler already removed — so a dev server that ignored the upgrade holds that socket with no error listener at all and the reset lands as an unhandled `read ECONNRESET` that exits the process. Measured on Node 26 against Astro. Such a socket is now parked and closed once it opens, bounded by `WS_ABANDONED_MAX`.

`openUpstream` wraps the WebSocket constructor, which throws `SyntaxError` for a subprotocol that is not an RFC 6455 token. Nothing between the relay's message listener and there catches, and an uncaught exception shuts the whole host down — so the one tunnel is refused instead.

Known limits, recorded in bridge/CLAUDE.md: the park has no TTL and its cap is per-manager, past the cap eviction performs the very reset the park exists to avoid, and the echo stays optimistic — a server that picks a later entry is logged, and one that echoes nothing cannot be detected at all (Bun reports `ws.protocol` as the first entry offered).

Claude-Session: https://claude.ai/code/session_01Ca7qjBqVbkUGQ71TewJZN5
@bharathm03
bharathm03 enabled auto-merge (squash) September 3, 2026 10:44
@bharathm03
bharathm03 merged commit f76825d into development Sep 3, 2026
4 checks passed
@bharathm03
bharathm03 deleted the fix/preview-ws-subprotocol branch September 3, 2026 10:56
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