fix(client): reconnect policy — terminal 4xxx closes, jittered backoff (ADR-0016)#32
Conversation
|
Post-PR codex (gpt-5.5) adversarial pass on the final diff found one HIGH: Fixed in 3e8f731 with a The same pass explicitly cleared the previously-fixed paths: no stale-timer resurrection in the transient-drop → demand-driven-connect → terminal-close path, resubscribe invariant preserved, attempt counter grows/resets correctly. 🤖 Generated with Claude Code |
…f (ADR-0016) The transport's auto-reconnect was blind to CloseEvent.code and used a fixed delay from both reconnect sites: an accept-then-close(4403) auth rejection retried forever (#25) and a DO outage produced a fixed-interval thundering herd (#26). The delay is now an injectable policy — reconnectDelay?: (attempt, closeCode?, closeReason?) => number | null — with a default of capped exponential backoff + full jitter (base = reconnectDelayMs, cap 30s, attempt reset on successful open) that treats application close codes 4000-4999 as terminal. Terminal stops surface via the new onClosed(code, reason) hook. Both sites route through one scheduleReconnect that still sets `reconnecting` at scheduling time (the demand-driven connect resubscribe invariant). Fixes #25, #26. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…al/close (codex review) A transient drop armed an untracked setTimeout; if a demand-driven connect() then hit a terminal 4xxx close, the stale timer still fired later — reconnecting past "stop" and double-firing onClosed. The timer also survived an intentional close(). Track the single pending handle and clear it on successful open, terminal stop, close(), and supersession. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
close() followed by an immediate connect() installs a fresh socket; the old socket's close event could then land and null the LIVE connection, forcing the next demand-driven connect() to open a needless third socket. Only the current socket's close may detach or schedule a reconnect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…post-PR codex review) close() cancels the pending retry timer but cannot cancel a connect() body already parked on a slow open() (TLS / cold start); that body installed the socket unconditionally, resurrecting a live, resubscribed connection after teardown. A closeEpoch captured before the await and checked after it lets close() invalidate in-flight opens — the orphan socket is closed, not installed — while an explicit connect() AFTER close() (pinned behavior) still works, which a plain intentionallyClosed check would break. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…| fn One option per concern (review feedback on ADR-0016): a number is the default policy's base delay (the old reconnectDelayMs meaning), a function is the full policy. Pre-1.0 clean break, no shim — ADR-0014 precedent. The reconnect-window fixture moves to a fixed `() => 60_000` policy: it needs the timer to NEVER fire, and a jittered 60s base can land near 0. ADR-0016 also now records why closeCode/closeReason stay open types: 4xxx is app-defined private-use vocabulary this library never emits, and the closed set in the signature is the return (`number | null`). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ferral Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
32fcad6 to
d10df99
Compare
Fixes #25
Fixes #26
Problem
The transport auto-reconnected from two sites (socket close handler, connect-failure catch) on a fixed
reconnectDelayMs, blind toCloseEvent.code:Design (ADR-0016)
reconnectDelay?: (attempt, closeCode?, closeReason?) => number | null(null= stop). Both reconnect sites route through a singlescheduleReconnect.defaultReconnectDelaygives full-jitter exponential backoff (base =reconnectDelayMs, default 250 ms; cap 30 s, never below the base; attempt counter resets on a successful open) and returnsnullfor application close codes 4000-4999 — the server closed on purpose, retrying cannot succeed.onClosed(code, reason)hook fires exactly when the policy returnsnull(connect failures reportundefined— no close frame), so auth closes are distinguishable from transient drops. Replaces the open()-listener workaround.reconnectingis still set at SCHEDULING time (demand-driven connect resubscribe window, ADR-0011 grill), and also on a terminal stop so an app-drivenconnect()after re-auth still resubscribes from the cursor.reconnectDelayMskeeps working as the default policy's base. Pre-existing reconnect suites (reconnect,reconnect-recovery,reconnect-window) pass unchanged.Adversarial review (codex gpt-5.5)
onClosed; it also survivedclose(). The single pending timer handle is now tracked and cancelled on successful open, terminal stop,close(), and supersession — regression tests added.close()+ immediateconnect()) could null the live socket; close handling is now socket-instance guarded.close()); they fail loud via their existingtimeoutMs. Unchanged pre-existing semantics, noted as a possible follow-up.Tests
npx vitest run: 46 files, 209 tests, all green. New suites:tests/reconnect-policy.test.ts(policy unit tests + workerd terminal/transient/attempt-counter/stale-timer coverage),tests/auth-path.test.ts(upgrade-status vs application-close auth surfaces, incl. transport-vs-4403 end to end).🤖 Generated with Claude Code