Fixes #1034: Connection Timeout enquanto usuário navega em www.miess.com.br/lingerie #1035
Conversation
Automated implementation by Ralph Bot. This commit addresses the requirements specified in #1034.
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughThe PR introduces dual-level timeout mechanisms: a request-wide timeout in the runtime preparation layer and a per-fetch timeout in the patched fetch utility. Both mechanisms leverage AbortController and AbortSignal.any to combine timeout signals with existing abort signals, ensuring coordinated cancellation across the request lifecycle and individual fetch operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utils/patched_fetch.ts (1)
22-52: SanitizeFETCH_TIMEOUT_MSand clear timers on early abort.Line 22 can parse to
NaN, which makes timeouts fire immediately. Also,signal.throwIfAborted()can throw beforefetchPromiseis created, leaving the timeout pending. Consider guarding the timeout value and clearing the timer in acatchpath.🔧 Proposed fix
-const FETCH_TIMEOUT_MS = parseInt(Deno.env.get("FETCH_TIMEOUT_MS") ?? "30000"); // 30 seconds default +const parsedFetchTimeout = Number(Deno.env.get("FETCH_TIMEOUT_MS")); +const FETCH_TIMEOUT_MS = Number.isFinite(parsedFetchTimeout) ? parsedFetchTimeout : 30000; // 30 seconds default @@ - signal?.throwIfAborted?.(); - - // Clean up timeout when fetch completes - const fetchPromise = fetcher(input, { signal, ...init }); - fetchPromise.finally(() => clearTimeout(fetchTimeoutId)); - - return fetchPromise; + try { + signal?.throwIfAborted?.(); + + // Clean up timeout when fetch completes + const fetchPromise = fetcher(input, { signal, ...init }); + fetchPromise.finally(() => clearTimeout(fetchTimeoutId)); + + return fetchPromise; + } catch (err) { + clearTimeout(fetchTimeoutId); + throw err; + }
🤖 Fix all issues with AI agents
In `@runtime/mod.ts`:
- Around line 237-239: REQUEST_TIMEOUT_MS currently uses parseInt which can
return NaN and make setTimeout treat the delay as 0, causing immediate aborts;
change the parsing logic for REQUEST_TIMEOUT_MS so it produces a safe positive
integer fallback (e.g., parse then if Number.isNaN or <=0 use 30000) before
passing it to setTimeout/AbortController; update the symbol usages
REQUEST_TIMEOUT_MS, abortController, and timeoutId accordingly so the timeoutId
is set with a validated numeric timeout value.
In `@utils/patched_fetch.ts`:
- Around line 51-52: The current call to fetcher creates the request with `{
signal, ...init }` which lets init.signal overwrite the timeout `signal`,
disabling the timeout when a caller passes their own signal; change this to
create a combined AbortController (e.g., combinedController) that forwards both
the timeout abort and the caller's `init.signal` (attach an `abort` listener on
`init.signal` to call `combinedController.abort()`), pass
`combinedController.signal` to `fetcher` (instead of `{ signal, ...init }`), and
ensure you remove the `init.signal` listener and clear `fetchTimeoutId` in the
finally/cleanup so no listeners or timers leak (refer to symbols: `fetcher`,
`input`, `init`, `signal`, `fetchPromise`, `fetchTimeoutId`).
🧹 Nitpick comments (1)
runtime/mod.ts (1)
255-262: Verify Context.setDefault per request is safe for concurrent traffic.Line 256 mutates the global default context for each request. If any work runs outside AsyncLocalStorage, this risks cross-request context bleed (including abort signals). Please confirm this is safe, or switch to a request-scoped bind (e.g.,
Context.bind/RequestContext.bind) or restore the prior default once the request finishes.
| const REQUEST_TIMEOUT_MS = parseInt(Deno.env.get("REQUEST_TIMEOUT_MS") ?? "30000"); // 30 seconds default | ||
| const abortController = new AbortController(); | ||
| const timeoutId = setTimeout(() => abortController.abort(), REQUEST_TIMEOUT_MS); |
There was a problem hiding this comment.
Harden REQUEST_TIMEOUT_MS parsing to avoid accidental immediate aborts.
Line 237 uses parseInt, which can yield NaN if the env var is mis-set. setTimeout(NaN) effectively becomes 0, causing immediate aborts. Consider a safe numeric parse with a fallback.
🔧 Proposed fix
- const REQUEST_TIMEOUT_MS = parseInt(Deno.env.get("REQUEST_TIMEOUT_MS") ?? "30000"); // 30 seconds default
+ const parsedTimeout = Number(Deno.env.get("REQUEST_TIMEOUT_MS"));
+ const REQUEST_TIMEOUT_MS = Number.isFinite(parsedTimeout) ? parsedTimeout : 30000; // 30 seconds default📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const REQUEST_TIMEOUT_MS = parseInt(Deno.env.get("REQUEST_TIMEOUT_MS") ?? "30000"); // 30 seconds default | |
| const abortController = new AbortController(); | |
| const timeoutId = setTimeout(() => abortController.abort(), REQUEST_TIMEOUT_MS); | |
| const parsedTimeout = Number(Deno.env.get("REQUEST_TIMEOUT_MS") ?? "30000"); | |
| const REQUEST_TIMEOUT_MS = Number.isFinite(parsedTimeout) ? parsedTimeout : 30000; // 30 seconds default | |
| const abortController = new AbortController(); | |
| const timeoutId = setTimeout(() => abortController.abort(), REQUEST_TIMEOUT_MS); |
🤖 Prompt for AI Agents
In `@runtime/mod.ts` around lines 237 - 239, REQUEST_TIMEOUT_MS currently uses
parseInt which can return NaN and make setTimeout treat the delay as 0, causing
immediate aborts; change the parsing logic for REQUEST_TIMEOUT_MS so it produces
a safe positive integer fallback (e.g., parse then if Number.isNaN or <=0 use
30000) before passing it to setTimeout/AbortController; update the symbol usages
REQUEST_TIMEOUT_MS, abortController, and timeoutId accordingly so the timeoutId
is set with a validated numeric timeout value.
| const fetchPromise = fetcher(input, { signal, ...init }); | ||
| fetchPromise.finally(() => clearTimeout(fetchTimeoutId)); |
There was a problem hiding this comment.
Combined timeout signal is lost when init.signal is present.
Line 51 uses { signal, ...init }, so init.signal overwrites the combined signal. That bypasses the timeout for callers who pass a signal, defeating the feature.
🐛 Proposed fix
- const fetchPromise = fetcher(input, { signal, ...init });
+ const fetchPromise = fetcher(input, { ...init, signal });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchPromise = fetcher(input, { signal, ...init }); | |
| fetchPromise.finally(() => clearTimeout(fetchTimeoutId)); | |
| const fetchPromise = fetcher(input, { ...init, signal }); | |
| fetchPromise.finally(() => clearTimeout(fetchTimeoutId)); |
🤖 Prompt for AI Agents
In `@utils/patched_fetch.ts` around lines 51 - 52, The current call to fetcher
creates the request with `{ signal, ...init }` which lets init.signal overwrite
the timeout `signal`, disabling the timeout when a caller passes their own
signal; change this to create a combined AbortController (e.g.,
combinedController) that forwards both the timeout abort and the caller's
`init.signal` (attach an `abort` listener on `init.signal` to call
`combinedController.abort()`), pass `combinedController.signal` to `fetcher`
(instead of `{ signal, ...init }`), and ensure you remove the `init.signal`
listener and clear `fetchTimeoutId` in the finally/cleanup so no listeners or
timers leak (refer to symbols: `fetcher`, `input`, `init`, `signal`,
`fetchPromise`, `fetchTimeoutId`).
Summary
This pull request implements a solution for #1034.
Issue Reference
#1034
Implementation
Automated implementation by Ralph Bot using Claude Code.
This PR was created automatically by Ralph.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.