fix(passthrough): bound connectTimeoutMs to TCP establishment; fix duplicate error log - #30
Closed
dean0x wants to merge 4 commits into
Closed
fix(passthrough): bound connectTimeoutMs to TCP establishment; fix duplicate error log#30dean0x wants to merge 4 commits into
dean0x wants to merge 4 commits into
Conversation
…ix duplicate error log Fixes #27 Defect A: upstream.setTimeout(connectTimeoutMs) was armed immediately after http.request() and never re-armed on connect, so it measured time-to-first-byte rather than connection establishment time. On a keep-alive pooled socket there is no connect phase at all, making the 10 s budget a hard cap on upstream think-time and causing spurious 504s for long-running requests. Fix: fold a rearm into the existing 'socket' listener. When socket.connecting is true (fresh connection), re-arm to streamIdleTimeoutMs on the 'connect' event. When socket.connecting is false (pooled socket), re-arm immediately — no 'connect' event fires on a reused socket. The existing re-arm inside the response callback (for mid-stream idle semantics) is left in place. Defect B: the timeout handler called upstream.destroy() before writing the 504. destroy() on an in-flight ClientRequest emits 'error' (ECONNRESET) on the next tick. The error handler's `responded` guard was inert on the pre-header path (responded was still false), so both anthropic_upstream_timeout and anthropic_upstream_error were logged and res.end() was called after res.destroy(). Fix: replace the `responded` guard in the error handler with a new `settled` flag that is set by whichever handler responds first (timeout 504 writer or the response callback). The 504 is also written before upstream.destroy() to narrow the race window. Adds three regression tests: think-time > connectTimeoutMs succeeds; exactly one warn event on genuine timeout; pooled-socket path arms streamIdleTimeoutMs immediately. Co-Authored-By: Claude <noreply@anthropic.com>
…dget Adds a three-budget design to the Anthropic passthrough: - connectTimeoutMs (10 s): TCP establishment only — unchanged. - headerTimeoutMs (600 s): connect→response-headers, re-armed on socket connect (or immediately for pooled sockets). Defaults to Anthropic's own server-side ceiling so the relay never fires before the origin does. - streamIdleTimeoutMs (300 s): headers→stream-end, reset per chunk — unchanged. Previously the socket re-armed to streamIdleTimeoutMs on connect; a non-streaming Opus completion with large max_tokens that buffers server-side for several minutes could be cut off at 300 s where a direct connection would have succeeded. With headerTimeoutMs defaulting to 600 s the relay is aligned with the origin's own timeout. Also: - CHANGELOG: version [Unreleased] → [0.2.1] - 2026-08-19; corrects the res.end/res.destroy ordering claim (it was res.destroy after res.end, not the reverse); documents the TTFB budget widening. - Tests: updated three existing timeout tests to exercise headerTimeoutMs explicitly; renamed to reflect the new knob. - Fixes missing headerTimeoutMs in server-wiring and doctor test fixtures. Co-Authored-By: Claude <noreply@anthropic.com>
…e headerTimeoutMs default to 660 s Three changes from verification follow-up on #27: 1. Add mid-stream idle test to passthrough.test.ts. The new test sends 6 SSE chunks ~50 ms apart (~300 ms of active streaming) then stalls. With streamIdleTimeoutMs=100ms the idle timer fires after the stall and res.destroy() truncates the body. A 2 s timing bound makes the test non-vacuous: with streamIdleTimeoutMs=10_000 the elapsed time reaches ~10 261 ms, failing the bound and proving the knob does the work. The active-chunk phase also pins the "reset by every received chunk" invariant. 2. Raise headerTimeoutMs default from 600_000 to 660_000 ms. The relay's clock starts at TCP connect; the origin's starts at full-request-received. Equal budgets with an earlier start lets the relay pre-empt the origin by the request-upload time plus RTT. The 60 s of headroom corrects that. Updated in src/config.ts (schema + JSDoc), subswitch.config.example.json, README.md table, and CHANGELOG.md. 3. Add assert.equal(result.value.config.anthropic.headerTimeoutMs, 660_000) to test/unit/config.test.ts alongside its connectTimeoutMs and streamIdleTimeoutMs siblings so the default is not unasserted. Co-Authored-By: Claude <noreply@anthropic.com>
ClientRequest.setTimeout() defers via an internal 'connect' listener so it
fires only after TCP connect — in the same tick as the headerTimeoutMs rearm —
meaning connectTimeoutMs was never in force for any measurable interval.
Measured before fix: blackholed IP (192.0.2.1) with connectTimeoutMs=700 failed
after 75 019 ms (macOS kernel TCP timeout). Neither budget fired.
Fix: arm the timer directly on the socket inside the 'socket' event handler,
before 'connect' fires. Node v22's internal socket-timeout handler (onTimeout)
skips req.emit('timeout') when socket.connecting is true, so we explicitly
forward socket 'timeout' → upstream.emit('timeout') to trigger the 504 handler.
On connect we cancel the connect timer and re-arm to headerTimeoutMs via the
normal upstream.setTimeout() path (which propagates correctly for connected
sockets). Measured after fix: same scenario fails at ~700 ms. ✓
On HTTPS, 'connect' fires after TCP but before the TLS handshake, so TLS
negotiation falls under headerTimeoutMs, not connectTimeoutMs. Documented in
JSDoc for both PassthroughOptions and config.ts AnthropicSchema.
New test: connectTimeoutMs fires during TCP connect to a non-routable upstream
(192.0.2.1, TEST-NET-1). Without fix the test was cancelled at the 30 s
--test-timeout limit; with fix it passes in ~261 ms. Non-vacuity confirmed
empirically. Exactly one anthropic_upstream_timeout warn is emitted — settled
de-dup holds on the connect-timeout path.
Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
Superseded by #33. All commits from this branch are contained in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two bugs in the Anthropic passthrough timeout logic that caused spurious 504s for long-running requests and duplicate
warnlog entries on timeout.Fixes #27
Changes
Defect A —
connectTimeoutMsnow bounds only TCP establishmentupstream.setTimeout(connectTimeoutMs)was armed immediately afterhttp.request()and never re-armed on connect, so it effectively measured time-to-first-byte. On a keep-alive pooled socket there is no connect phase at all, making the 10 s default a hard cap on upstream think-time and producing spurious 504s for long-runningclaude-opus-4-class requests.Fix (
src/anthropic-passthrough.ts): fold a rearm into the existing'socket'listener:streamIdleTimeoutMson the'connect'eventsocket.connectingis false, so re-arm immediately — no'connect'event fires on a reused socketThe mid-stream re-arm already in the response callback is left in place.
Config doc update (
src/config.ts): theconnectTimeoutMsJSDoc comment is updated to describe the tightened semantics explicitly.Defect B — eliminated duplicate
anthropic_upstream_errorwarn on pre-header timeoutThe timeout handler called
upstream.destroy()before writing the 504. Destroying an in-flightClientRequestemits'error'(ECONNRESET) on the next tick. The error handler's guardif (responded) returnwas inert on the pre-header path (respondedwas stillfalse), so bothanthropic_upstream_timeoutandanthropic_upstream_errorwere logged andres.end()was called afterres.destroy().Fix: replace
respondedwith asettledflag set by whichever handler wins the race (timeout 504 writer or the response callback). The 504 is also written beforeupstream.destroy()to narrow the race window.Breaking Changes
None. Default timeout values are unchanged. The behavior change (header-wait budget widened from 10 s to
streamIdleTimeoutMs= 300 s) is intentional and fixes the reported regression.Reviewer Focus Areas
src/anthropic-passthrough.tslines 85–135:settledflag placement andsocketlistener rearm logic — confirm bothsocket.connectingbranches are reachable and correctsettled→destroy()) — confirm no use-after-end path survivestest/integration/passthrough.test.ts: think-time test, single-warn test, pooled-socket test