iframe-proxy

TroyCoombs · 2026-08-04T12:52:20Z

Summary

Improves hookdeck listen stability and UX against the new server-side websocket proxy (Core #4477), which moved CLI sessions to Redis with a TTL:

Session recreation on reconnect: send X-Webhook-Ids (comma-separated connection IDs) and X-Session-Filters (base64-encoded JSON) headers on every connect/reconnect, so the server can recreate an expired session instead of rejecting the connection with 4001 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 with CLI_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.
Clean close on shutdown: Ctrl+C / quit now sends a WebSocket close 1000 before teardown, letting the server tombstone the session immediately instead of holding it through the reconnect grace window (prevents events routing to a CLI that's gone).
Quiet reconnects on intentional server closes: close codes 1001 (pod restart during a deploy) and 4001 (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 where ws.IsCloseError(err) was called with no codes (always false), which made the error-level close branches unreachable — genuinely unexpected close codes now surface properly.
Backoff counter reset: the reconnect attempt counter resets after a successful connection, so backoff reflects consecutive failures rather than lifetime reconnects (long-running CLIs no longer drift toward the 10s max delay after routine deploys).
Race-safety: websocket client connection state and the proxy's client reference are now guarded for cross-goroutine access (signal handler vs. reconnect loop), verified under go test -race.

Test plan

go build ./..., go vet, gofmt clean
go 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/4001
Manual: run hookdeck listen against prod, verify session recreation after Redis expiry and quiet reconnect through a ws-proxy deploy

Made with Cursor

- 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

leggetter · 2026-08-04T13:06:14Z

Copilot

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-encoded X-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 (1001 going away, 4001 session 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 on notifyClose after shutdown. Use Run() + 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 no Run() select loop to drain notifyClose, so a clean close can still leave blocked goroutines. Switch to Run() + 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>

leggetter · 2026-08-04T14:10:12Z

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 ws.IsCloseError(err) bug (nice catch — with no codes it always returns false, so every close error went to debug and both error branches were dead) revived the error path for close codes outside the handled set. 1006 falls in that gap, and it's the most common real-world drop: network blip, laptop sleep, LB idle timeout, pod killed without a graceful close. Verified against a server that drops TCP with no close handshake:

ERROR close error: websocket: close 1006 (abnormal closure): unexpected EOF
ERROR If you run into issues, please re-run with `--log-level debug` and share the output ... on GitHub.

That's the opposite of this PR's "quiet reconnects" goal, so 1006 now logs at debug alongside 1001/4001. Added TestAbruptDisconnectReconnectsQuietly, which fails without the fix.

2. Ctrl+C often printed "Connection lost, reconnecting…" on the way out. New behavior from calling Stop() in the signal handler: Stop() closes done, Client.Run takes its <-c.done branch, and that branch closes NotifyExpired. Since Stop() runs before cancel(), the proxy's select sees NotifyExpired ready before (or alongside) signalCtx.Done() and can take the disconnect path, starting the reconnect spinner as the process exits — and that path returns without renderer.Cleanup(), so the spinner is never stopped. A shuttingDown flag set before Stop() disambiguates it. (Checking signalCtx.Err() alone doesn't work — the loop can get there before cancel() executes.)

3. hasConnectedOnce is now an atomic.Bool. Pre-existing, but adjacent to this PR's race-safety work and invisible to -race because pkg/listen/proxy has no tests: it's written by the per-attempt connection-monitor goroutine and read by canConnect() on the Run goroutine. CompareAndSwap also makes the "spawn the health monitor exactly once" guarantee in the comment actually true, now that each reconnect attempt spawns its own monitor goroutine.

go build ./..., go vet, gofmt, and go test -race ./... all clean (the pkg/listen/healthcheck port-443 test fails in my sandbox, but does so on clean main too).

One thing I deliberately left alone: p.currentWebSocketClient().SendMessage(...) at proxy.go:434 and :461 is unchecked while the call site in processEndpointResponse nil-checks. Pre-existing inconsistency and not reachable in practice (the handler only runs once a client exists), so it seemed out of scope here.

Generated by Claude Code

leggetter

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.

TroyCoombs and others added 3 commits July 16, 2026 12:22

TroyCoombs requested a review from leggetter August 4, 2026 12:54

leggetter mentioned this pull request Aug 4, 2026

fix(listen): recover when the CLI session expires server-side #321

Closed

leggetter requested a lite review from Copilot August 4, 2026 13:23

Copilot started reviewing on behalf of leggetter August 4, 2026 13:24 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

Comment thread pkg/websocket/client.go

Comment thread pkg/websocket/client_test.go Outdated

claude added 2 commits August 4, 2026 13:35

leggetter approved these changes Aug 4, 2026

View reviewed changes

leggetter merged commit 50b89a1 into main Aug 4, 2026
15 of 16 checks passed

leggetter deleted the feat/include_meta_session_data branch August 4, 2026 19:18

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Uh oh!

Improve listen reliability with the new websocket proxy (session recreation, clean close, quiet reconnects) - #322

Improve listen reliability with the new websocket proxy (session recreation, clean close, quiet reconnects)#322
leggetter merged 6 commits into
mainfrom
feat/include_meta_session_data

TroyCoombs commented Aug 4, 2026

Uh oh!

leggetter commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

leggetter commented Aug 4, 2026

Uh oh!

leggetter left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

TroyCoombs commented Aug 4, 2026

Summary

Test plan

Uh oh!

leggetter commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

leggetter commented Aug 4, 2026

Uh oh!

leggetter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants