Fix race condition between fetch timeout and Express response timeout#25
Merged
Conversation
…eout FETCH_TIMEOUT and RESPONSE_TIMEOUT were both 10s, causing them to fire simultaneously when an upstream host was unreachable. The AbortSignal would throw (triggering sendError 404) at the same moment res.setTimeout sent a 504, resulting in ERR_HTTP_HEADERS_SENT errors in production. Reduce FETCH_TIMEOUT to 5s so the 404 path always completes well before the 10s response timeout fires. Also replace new Date().getTime() with Date.now() in getCacheHeaders. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaced hardcoded timeout values with a new shared Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Express
participant TimeoutConfig
participant ResponseHelper
participant UpstreamAPI
Client->>Express: GET /thumb/...
Note right of Express: reads REQUEST_TIMEOUT_MS from TimeoutConfig
Express->>ResponseHelper: fetch image (uses FETCH_TIMEOUT_MS)
ResponseHelper->>UpstreamAPI: HTTP fetch (abortable)
UpstreamAPI-->>ResponseHelper: image / error / timeout
alt fetch succeeds before FETCH_TIMEOUT_MS
ResponseHelper-->>Express: image + headers (Expires uses Date.now())
Express-->>Client: 200 OK + body
else fetch fails or FETCH_TIMEOUT_MS reached
ResponseHelper-->>Express: error
Express->>Express: res.setTimeout(RESPONSE_TIMEOUT_MS) handler checks writableEnded/headersSent
alt response still writable
Express-->>Client: 504 Gateway Timeout
else headers already sent
Express->>Client: connection destroyed
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ResponseHelper.ts (1)
9-11: Decouple timeout coordination from comments-only couplingLine 9 documents the dependency on Express
RESPONSE_TIMEOUT, but it’s still enforced manually. If that timeout changes later, this race fix can silently regress. Consider sourcing both values from one shared config constant/module instead of duplicating the relationship in comments.Suggested refactor
+// src/timeoutConfig.ts +export const RESPONSE_TIMEOUT_MS = 10_000; +export const FETCH_TIMEOUT_MS = 5_000; // keep safely below RESPONSE_TIMEOUT_MS- FETCH_TIMEOUT = 5 * 1000; // 5 seconds + FETCH_TIMEOUT = FETCH_TIMEOUT_MS;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ResponseHelper.ts` around lines 9 - 11, The FETCH_TIMEOUT constant in ResponseHelper.ts is hardcoded and comments-only coupled to Express's RESPONSE_TIMEOUT; create or use a shared configuration constant (e.g., RESPONSE_TIMEOUT in a central config module) and derive FETCH_TIMEOUT from it instead of duplicating the value in comments. Update ResponseHelper to import the shared RESPONSE_TIMEOUT and set FETCH_TIMEOUT relative to it (for example a safe margin or percentage), and update ExpressSetup to also import the same shared RESPONSE_TIMEOUT so both modules read the single source of truth; ensure the identifiers ResponseHelper.FETCH_TIMEOUT and ExpressSetup.RESPONSE_TIMEOUT (or the chosen shared name) are used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ResponseHelper.ts`:
- Around line 9-11: The FETCH_TIMEOUT constant in ResponseHelper.ts is hardcoded
and comments-only coupled to Express's RESPONSE_TIMEOUT; create or use a shared
configuration constant (e.g., RESPONSE_TIMEOUT in a central config module) and
derive FETCH_TIMEOUT from it instead of duplicating the value in comments.
Update ResponseHelper to import the shared RESPONSE_TIMEOUT and set
FETCH_TIMEOUT relative to it (for example a safe margin or percentage), and
update ExpressSetup to also import the same shared RESPONSE_TIMEOUT so both
modules read the single source of truth; ensure the identifiers
ResponseHelper.FETCH_TIMEOUT and ExpressSetup.RESPONSE_TIMEOUT (or the chosen
shared name) are used consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef456e5b-f561-4f91-9bab-659156b89c91
📒 Files selected for processing (1)
src/ResponseHelper.ts
Addresses CodeRabbit nitpick: FETCH_TIMEOUT and RESPONSE_TIMEOUT were only loosely coupled via comments, so a future change to one could silently reintroduce the race condition. Both values now live in timeoutConfig.ts as the single source of truth, with the constraint documented there. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/timeoutConfig.ts (1)
7-11: Enforce timeout ordering as a runtime invariantThe safety condition is currently comment-only. Add a startup guard so future config edits can’t silently reintroduce the race.
Proposed diff
export const FETCH_TIMEOUT_MS = 5_000; + +if (FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS) { + throw new Error( + "FETCH_TIMEOUT_MS must be strictly less than RESPONSE_TIMEOUT_MS", + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/timeoutConfig.ts` around lines 7 - 11, Add a runtime assertion at module initialization to enforce the safety ordering between FETCH_TIMEOUT_MS and RESPONSE_TIMEOUT_MS: verify that FETCH_TIMEOUT_MS is "well under" RESPONSE_TIMEOUT_MS (e.g., FETCH_TIMEOUT_MS < RESPONSE_TIMEOUT_MS - some margin or just FETCH_TIMEOUT_MS < RESPONSE_TIMEOUT_MS) and if the check fails, log an explicit error and terminate startup (throw an Error or call process.exit(1)). Place this guard in the same module near the FETCH_TIMEOUT_MS/RESPONSE_TIMEOUT_MS definitions so the check runs on import and prevents silent regressions; reference the constants FETCH_TIMEOUT_MS and RESPONSE_TIMEOUT_MS in the check and error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ExpressSetup.ts`:
- Around line 88-91: The timeout handler registered on res via
res.setTimeout(RESPONSE_TIMEOUT_MS, ...) must guard against sending headers/body
after the main handler already responded; modify the callback in ExpressSetup.ts
to check res.headersSent (or res.writableEnded) and only call res.status(504)
and res.send("Gateway Timeout") when the response has not already been sent — if
headers are sent, simply return without attempting to write the response. Ensure
you update the callback associated with RESPONSE_TIMEOUT_MS accordingly.
---
Nitpick comments:
In `@src/timeoutConfig.ts`:
- Around line 7-11: Add a runtime assertion at module initialization to enforce
the safety ordering between FETCH_TIMEOUT_MS and RESPONSE_TIMEOUT_MS: verify
that FETCH_TIMEOUT_MS is "well under" RESPONSE_TIMEOUT_MS (e.g.,
FETCH_TIMEOUT_MS < RESPONSE_TIMEOUT_MS - some margin or just FETCH_TIMEOUT_MS <
RESPONSE_TIMEOUT_MS) and if the check fails, log an explicit error and terminate
startup (throw an Error or call process.exit(1)). Place this guard in the same
module near the FETCH_TIMEOUT_MS/RESPONSE_TIMEOUT_MS definitions so the check
runs on import and prevents silent regressions; reference the constants
FETCH_TIMEOUT_MS and RESPONSE_TIMEOUT_MS in the check and error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db604e62-6ee2-4cee-aa41-eedc5fcc440d
📒 Files selected for processing (3)
src/ExpressSetup.tssrc/ResponseHelper.tssrc/timeoutConfig.ts
Check res.headersSent and res.writableEnded before sending 504 in the res.setTimeout callback to prevent double-write errors when the main handler completes before the timeout fires. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/timeoutConfig.ts (1)
7-11: Enforce the timeout invariant in code.Lines 8-10 describe the key safety guarantee, but nothing fails fast if a later edit makes
FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS. A small module-level assertion would keep this race from coming back silently.Possible guard
export const FETCH_TIMEOUT_MS = 5_000; + +if (FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS) { + throw new Error("FETCH_TIMEOUT_MS must be less than RESPONSE_TIMEOUT_MS"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/timeoutConfig.ts` around lines 7 - 11, Add a module-level guard that enforces the invariant FETCH_TIMEOUT_MS < RESPONSE_TIMEOUT_MS by checking these constants at import time and throwing a clear Error if the condition is violated; place the check immediately after the constants (e.g., referencing FETCH_TIMEOUT_MS and RESPONSE_TIMEOUT_MS) and include both values in the error message so any future change that makes FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS fails fast during startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ExpressSetup.ts`:
- Around line 88-92: The timeout handler registered in
res.setTimeout(RESPONSE_TIMEOUT_MS, ...) currently returns when res.headersSent
is true, leaving stalled connections open; update the handler so that if headers
have been sent but the response is not writableEnded, you explicitly destroy the
underlying socket (e.g., call res.socket.destroy() or res.destroy() if
available) before returning, while keeping the existing writableEnded guard and
still sending the 504 when headers have not been sent; modify the callback in
res.setTimeout to perform this socket destruction for stalled-stream cases.
---
Nitpick comments:
In `@src/timeoutConfig.ts`:
- Around line 7-11: Add a module-level guard that enforces the invariant
FETCH_TIMEOUT_MS < RESPONSE_TIMEOUT_MS by checking these constants at import
time and throwing a clear Error if the condition is violated; place the check
immediately after the constants (e.g., referencing FETCH_TIMEOUT_MS and
RESPONSE_TIMEOUT_MS) and include both values in the error message so any future
change that makes FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS fails fast during
startup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29378d5b-763f-4797-afa6-c550507f3b1b
📒 Files selected for processing (3)
src/ExpressSetup.tssrc/ResponseHelper.tssrc/timeoutConfig.ts
When headersSent is true but writableEnded is false (response started but body stream stalled), returning from the timeout callback leaves the socket open indefinitely. Explicitly call res.destroy() to close the connection in that case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/timeoutConfig.ts (1)
8-11: Enforce the fetch-vs-response timeout invariant in code, not only comments.The critical race-prevention rule is documented here but currently not guarded. Add a startup-time assertion so invalid future edits fail fast.
Proposed diff
export const RESPONSE_TIMEOUT_MS = 10_000; @@ export const FETCH_TIMEOUT_MS = 5_000; + +if (FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS) { + throw new Error( + "Invalid timeout config: FETCH_TIMEOUT_MS must be less than RESPONSE_TIMEOUT_MS", + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/timeoutConfig.ts` around lines 8 - 11, Add a startup-time assertion that enforces FETCH_TIMEOUT_MS is strictly less than RESPONSE_TIMEOUT_MS so future edits fail-fast; locate FETCH_TIMEOUT_MS in timeoutConfig.ts and import or reference RESPONSE_TIMEOUT_MS (wherever it’s defined) and throw an Error (or use assert) during module initialization if FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS, so the process will crash on invalid configuration rather than relying on the comment alone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/timeoutConfig.ts`:
- Around line 8-11: Add a startup-time assertion that enforces FETCH_TIMEOUT_MS
is strictly less than RESPONSE_TIMEOUT_MS so future edits fail-fast; locate
FETCH_TIMEOUT_MS in timeoutConfig.ts and import or reference RESPONSE_TIMEOUT_MS
(wherever it’s defined) and throw an Error (or use assert) during module
initialization if FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS, so the process will
crash on invalid configuration rather than relying on the comment alone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bf5f83e-cf0a-4854-95ed-b00cc4f38673
📒 Files selected for processing (3)
src/ExpressSetup.tssrc/ResponseHelper.tssrc/timeoutConfig.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
FETCH_TIMEOUTandRESPONSE_TIMEOUTwere both 10s, causing them to fire simultaneously when an upstream host is unreachable (e.g. a stale IP in item metadata). The AbortSignal would throw andsendError(404)would run at the same instantres.setTimeoutsent a 504, producingERR_HTTP_HEADERS_SENTerrors and 504 responses visible to users.FETCH_TIMEOUTfrom 10s to 5s so the 404 error path always completes well before the 10s Express response timeout fires.new Date().getTime()withDate.now()ingetCacheHeaders.Context
Currently producing ~1,700 504s/day from items whose
objectURLs point to a stale IP (67.111.179.146) that no longer responds — these hang for the full timeout before failing. This fix ensures they fail fast with a 404 rather than racing into a 504.Test plan
ERR_HTTP_HEADERS_SENTerrors; hung-upstream items return 404 within ~5s instead of 504 after 10s🤖 Generated with Claude Code
Summary by CodeRabbit
Performance & Infrastructure
Bug Fixes