Improve listen reliability with the new websocket proxy (session recreation, clean close, quiet reconnects) - #322
Conversation
- Send X-Webhook-Ids and X-Session-Filters headers on every websocket connect/reconnect so the server can recreate the session in Redis if it expired between reconnects. - Send a clean websocket close (code 1000) on shutdown (Ctrl+C) so the server can tombstone the session immediately instead of holding it for the reconnect grace window. Stop() is now idempotent via sync.Once. Pairs with the server-side session tombstone in hookdeck/core#4477. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…64 filters header - Guard conn/isConnected with a mutex: Stop() runs on the signal-handler goroutine while the connect goroutine writes them, and the clean-close frame depends on reading a consistent snapshot. - Route all webSocketClient access through locked accessors: the reconnect loop reassigns it while the signal handler and event handlers read it. - Drop the Stop() call in the signalCtx.Done() branch — the signal callback always runs Stop() before cancelling the context. - Base64-encode the X-Session-Filters header: raw UTF-8 header bytes are decoded as latin-1 by the Node server and silently corrupt non-ASCII filter values on session recreation. - Add websocket client tests: recreation headers (incl. UTF-8 round-trip), clean close 1000 on Stop, Stop idempotency, and a race-detector exercise for concurrent Stop/connect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… after successful connections Server deploys close CLI sockets with 1001 and session expiry closes with 4001 — both are routine and recovered by reconnecting (4001 recreation via the session headers), so stop logging them as errors. Also fixes the close dispatch bug where ws.IsCloseError(err) with no codes always returned false, leaving the error branches unreachable. Reset the attempt counter after a successful connection so backoff reflects consecutive failures instead of lifetime reconnects. Co-authored-by: Cursor <cursoragent@cursor.com>
Brings the branch up to date with main (v2.3.1) — it was cut from January's main (v1.6 era), before the v2.0.0 Go SDK removal, so the PR was un-mergeable. Resolutions: - pkg/listen/proxy/proxy.go: kept main's current base (pkg/hookdeck types, shared APIClient, NoHealthcheck/Insecure health checks) and re-applied this branch's changes on top; connection.Id -> connection.ID to match the post-SDK hookdeck.Connection type; gofmt. - pkg/websocket/client.go and client_test.go merged cleanly. Verified: go build ./..., go vet, gofmt clean, and go test -race ./pkg/websocket ./pkg/listen/... passes (except the pre-existing sandbox-only healthcheck port-443 test, which also fails on clean main in this environment). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MH9LQENoLJSawdD7X4yy2h
|
Pushed a merge commit (d9b6638) bringing this branch up to date with Resolution notes:
Verified locally: Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability and user experience of hookdeck listen against the new server-side websocket proxy by making reconnects resilient to server-side session expiry, ensuring intentional shutdowns close cleanly, and reducing noisy logs for expected server closes.
Changes:
- Send session recreation metadata on every WebSocket connect/reconnect (
X-Webhook-Ids, base64-encodedX-Session-Filters) to allow server-side session recreation after Redis TTL expiry. - Implement clean shutdown behavior (send close code
1000) and reduce log severity for expected close codes (1001going away,4001session expired). - Add concurrency guards around WebSocket client state and add tests for the new reconnect/close behaviors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/websocket/client.go | Adds reconnect headers, clean close on Stop, quieter expected-close logging, and some connection-state synchronization. |
| pkg/websocket/client_test.go | Adds new unit tests covering session recreation headers, clean close, reconnect quietness, and Stop races. |
| pkg/listen/proxy/proxy.go | Plumbs connection IDs + filters into the WebSocket client, adds mutex-guarded access for the proxy’s active client pointer, and resets reconnect backoff after successful connects. |
Suppressed comments (2)
pkg/websocket/client_test.go:96
- Same issue here: calling
client.connect()directly can leave goroutines blocked onnotifyCloseafter shutdown. UseRun()+Connected()instead so the client tears down cleanly.
if err := client.connect(context.Background()); err != nil {
t.Fatalf("connect failed: %v", err)
}
defer client.Stop()
pkg/websocket/client_test.go:130
- Same here:
client.connect()starts pumps but there’s noRun()select loop to drainnotifyClose, so a clean close can still leave blocked goroutines. Switch toRun()+Connected()before asserting the close code.
if err := client.connect(context.Background()); err != nil {
t.Fatalf("connect failed: %v", err)
}
client.Stop()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e leaks - Stop(): key the clean-close frame on conn != nil alone. conn is only assigned after a successful upgrade, and also requiring isConnected skipped the close in the window between changeConnection() and setConnected(true). - Tests: go through Run() + Connected() instead of calling the unexported connect() directly, so readPump can't block forever on notifyClose after Stop() (goroutine leak). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MH9LQENoLJSawdD7X4yy2h
…e connect flag Three follow-ups from review of this branch: - Abnormal closure (1006) now logs at debug like 1001/4001. Fixing the latent ws.IsCloseError(err) bug revived the error-level branch, which meant an ordinary network blip, laptop sleep, LB idle timeout, or an ungracefully killed pod printed 'close error' plus an invitation to file a bug report. 1006 is never sent on the wire — gorilla synthesizes it for unexpected EOF — and the reconnect loop handles it. Covered by a regression test that drops the TCP connection with no close handshake. - Ctrl+C no longer announces 'Connection lost, reconnecting...' on the way out. Stopping the client closes its NotifyExpired channel, and because Stop runs before the context is cancelled, that case could win the select against signalCtx.Done() and start the reconnect spinner as the process exited (also leaving it running, since that path skips renderer.Cleanup). A shutdown flag set before Stop makes the intent unambiguous. - hasConnectedOnce is now an atomic.Bool. It was written by the per-attempt connection monitor goroutine and read by canConnect on the Run goroutine — a data race that -race never caught because this package has no tests. CompareAndSwap also makes the 'spawn the health monitor exactly once' guarantee real. Co-Authored-By: Claude <noreply@anthropic.com>
|
Pushed cf9f0ff with three fixes found while reviewing this branch — two of them user-visible regressions that came in as side effects of otherwise-correct changes. 1. Abnormal closure (1006) was logging at error level. Fixing the latent That's the opposite of this PR's "quiet reconnects" goal, so 1006 now logs at debug alongside 1001/4001. Added 2. Ctrl+C often printed "Connection lost, reconnecting…" on the way out. New behavior from calling 3.
One thing I deliberately left alone: Generated by Claude Code |
leggetter
left a comment
There was a problem hiding this comment.
Reviewed locally: go build, go vet, and gofmt are clean, and go test -race ./pkg/websocket/ ./pkg/listen/... passes (including the new client tests). Verified a live connect + clean Ctrl+C shutdown against the current server. LGTM.
Summary
Improves
hookdeck listenstability and UX against the new server-side websocket proxy (Core #4477), which moved CLI sessions to Redis with a TTL:X-Webhook-Ids(comma-separated connection IDs) andX-Session-Filters(base64-encoded JSON) headers on every connect/reconnect, so the server can recreate an expired session instead of rejecting the connection with4001 SESSION_EXPIRED. This fixes the permanent "connected-but-broken" flap loop where a CLI would retry a dead session ID forever while all events failed withCLI_UNAVAILABLE. Filters are base64-encoded because raw UTF-8 header bytes are decoded as latin-1 by Node and would silently corrupt non-ASCII values.1001(pod restart during a deploy) and4001(session expired, recreated on reconnect) now log at debug level instead of printing an alarming ERROR + "share the output with the Hookdeck team" message on every deploy. This also fixes a latent bug wherews.IsCloseError(err)was called with no codes (always false), which made the error-level close branches unreachable — genuinely unexpected close codes now surface properly.go test -race.Test plan
go build ./...,go vet,gofmtcleango test -race ./pkg/websocket/ ./pkg/listen/...passes, including new tests for: session-recreation headers (content, base64 ASCII-safety, omission when unset), clean close 1000 on Stop, Stop idempotency, Stop/connect race, and quiet reconnect on server closes 1001/4001hookdeck listenagainst prod, verify session recreation after Redis expiry and quiet reconnect through a ws-proxy deployMade with Cursor