Repository navigation
fix(http): raise keep-alive timeout above proxy idle timeouts - #473
Merged
Merged
Conversation
The HTTP transport kept Node's default keepAliveTimeout of 5s. Proxies that pool upstream connections (AWS ALB, API Gateway VPC links, nginx) hold them idle for 60s, so a request written into a socket Node was closing surfaced as a proxy 502/503 that never reached DBHub's logs. Set keepAliveTimeout to 65s and headersTimeout to 66s so the proxy is always the side that closes an idle connection. Hard-coded on purpose; no new flag. Fixes #472 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013piqks7dS3HsaAKC6YVWLN
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not cover the production server wiring and therefore cannot catch removal of the configuration call.
1 open finding
What changed in this PR
Raises HTTP keep-alive timeouts to avoid proxy-generated 502/503 errors.
Changes:
- Adds centralized keep-alive timeout configuration.
- Applies it to the HTTP server.
- Adds unit tests for configured values.
| File | Description |
|---|---|
src/utils/http-keep-alive.ts |
Defines and applies HTTP timeout values. |
src/server.ts |
Configures the production HTTP server. |
src/utils/__tests__/http-keep-alive.test.ts |
Tests timeout constants and helper behavior. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } from "../http-keep-alive.js"; | ||
|
|
||
| describe("configureHttpKeepAlive", () => { | ||
| it("raises keep-alive above a 60s proxy idle timeout (issue #472)", () => { |
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
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.
Fixes #472
Problem
The HTTP transport created its server with
http.createServer(app)and kept Node's defaultkeepAliveTimeoutof 5s. Proxies that pool upstream connections (AWS ALB, API Gateway VPC links, nginxkeepalive) hold them idle for 60s. When such a proxy writes a request into a socket Node is closing at that moment, the proxy answers the client with its own 502/503 and the request never reaches DBHub's logs.Fix
Set
keepAliveTimeoutto 65s andheadersTimeoutto 66s so the proxy is always the side that closes an idle connection. Node requiresheadersTimeoutto be strictly greater thankeepAliveTimeout, hence the 1s gap. The values are hard-coded on purpose; no new flag or env var.src/utils/http-keep-alive.ts: constants plusconfigureHttpKeepAlive(server)with the rationale in commentssrc/server.ts: call it right afterhttp.createServer(app)src/utils/__tests__/http-keep-alive.test.ts: asserts the values are applied and that the headers timeout stays above keep-alive, so a future refactor of the server setup can't silently drop themVerification
Ran the reproduction script from the issue against the built server. Before, the server closed the idle socket at ~6s and the second request got a fresh connection. After:
The socket survives the 10s idle and the second request reuses it.
pnpm exec vitest runon the new test plus the HTTP bind-host and JSON-RPC integration suites: 17 passed. The connector integration suites need Docker and were not run here.🤖 Generated with Claude Code
https://claude.ai/code/session_013piqks7dS3HsaAKC6YVWLN
Generated by Claude Code