-
Notifications
You must be signed in to change notification settings - Fork 191
Implement retry-after header handling for improved throttling in fetch requests #1636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
return delaySeconds; | ||
} | ||
} catch(e) { | ||
dbg(`failed to parse retry-after header as date: %s`, errorMessage(e)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The catch block uses errorMessage(e)
, but the caught variable is named e
, not error
. This will cause a ReferenceError at runtime.
dbg(`failed to parse retry-after header as date: %s`, errorMessage(e)); | |
dbg(`failed to parse retry-after header as date: %s`, errorMessage(e)); |
AI-generated content by pr-review-commit
undefined_variable
may be incorrect. Use reactions to eval.
if (retryAfterHeader) { | ||
const retryAfterSeconds = parseRetryAfter(retryAfterHeader); | ||
if (retryAfterSeconds !== null) { | ||
delay = Math.min(maxDelay, retryAfterSeconds * 1000); // Convert to milliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delay is set as retryAfterSeconds * 1000
, but Math.min(maxDelay, retryAfterSeconds * 1000)
expects both values in milliseconds. If maxDelay
is in milliseconds, this is correct, but if not, this could cause incorrect delay calculation. Please double-check the units for maxDelay
and retryAfterSeconds
to ensure consistency.
AI-generated content by pr-review-commit
milliseconds_seconds_confusion
may be incorrect. Use reactions to eval.
// Should be approximately 5 seconds (allow some tolerance) | ||
assert( | ||
retryAfterSeconds >= 4 && retryAfterSeconds <= 6, | ||
`Expected ~5 seconds, got ${retryAfterSeconds}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test for HTTP date parsing allows a 2-second tolerance (retryAfterSeconds >= 4 && retryAfterSeconds <= 6
), but timing-based tests can be flaky due to execution delays. Consider mocking the current time or using a more robust approach to avoid intermittent test failures.
AI-generated content by pr-review-commit
flaky_test_due_to_timing
may be incorrect. Use reactions to eval.
Investigator reportContext collection
AI Analysis
|
…nd resources tests
Currently, genaiscript handles throttling situations but does not respect the
retry-after
header returned by services. This leads to unnecessary load on throttled services and suboptimal user experience with exponential backoff delays that may be longer than needed.Changes Made
This PR implements proper
retry-after
header handling in the fetch retry logic:1. Added
parseRetryAfter()
functionretry-after: 120
retry-after: Fri, 31 Dec 1999 23:59:59 GMT
null
for invalid inputs to fallback to exponential backoff2. Enhanced retry logic in
createFetch()
retry-after
header in HTTP responses during retriesretry-after
header is presentmaxDelay
configuration to prevent excessive waits3. Improved user transparency
retry #1 in 120.0s (retry-after: 120s)
vsretry #1 in 2.0s
Example Behavior
Before:
After (with retry-after: 60):
Benefits
Testing
Added comprehensive unit tests covering:
"120"
→ 120 seconds)Fixes #1629.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
cdn.sheetjs.com
node /usr/local/bin/yarn install
(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.