Repository navigation
fix(feature-flags): Report poll timeouts and cancel abandoned requests - #1754
Open
stanleyphu wants to merge 2 commits into
Open
stanleyphu wants to merge 2 commits into
stanleyphu wants to merge 2 commits into
Conversation
A poll that hit requestTimeoutMs emitted 'error' with "Poll aborted" instead of the timeout: the timeout aborted the poll's controller before rejecting, and the abort listener's rejection won the race. The poll also never handed its abort signal to the HTTP request, so a timed-out poll, or one in flight at close(), left its request open until the HTTP client's own 60-second timeout. That timeout surfaces as a 408, which the client retries, so abandoned requests were re-sent after a minute, including after close(). The timeout now aborts the poll with a timeout error as the reason, and the poll passes its signal to the request. To carry it, workos.get and the HTTP client accept a `signal` option: aborting cancels the request, rejects with the signal's reason, and is never retried. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
[Medium impact] The PR appears safe to merge; the null-cancellation fix preserves successful requests and existing retry behavior. SummaryThe PR reports feature-flag poll timeouts correctly and cancels abandoned HTTP requests.
No new actionable issues were found in the changes since the previous review. Tests were inspected, not run. Reviews (2) · Last reviewed commit: "fix(feature-flags): Reject requests abor..." · Reviewed by Greptile |
`controller.abort(null)` sets the signal's reason to null, which the retry loop mistook for success, so `workos.get` failed with a TypeError instead of rejecting with the caller's reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@greptileai review |
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.
Description
A feature flags runtime client poll that runs past
requestTimeoutMswas reported wrong and never cleaned up:errorevent said "Poll aborted", not that the poll timed out. The timeout aborted the poll's controller and then rejected, but the abort listener's rejection settled the race first. Every timeout reachederrorlisteners and theloggerasError('Poll aborted').workos.get, so after a timeout, or afterclose(), the HTTP request stayed open until the HTTP client's own 60-second timeout. That timeout is a 408, which the client retries, so the abandoned request was sent again about a minute later, even afterclose(). Against an API that stops answering, each poll holds a connection for a minute and then opens another.The timeout now aborts the poll with a
Request timed outerror as the reason, and the poll passes its signal to the request. To carry it,workos.getand the HTTP client'sRequestOptionstake asignal, the same waymaxRetriesflows through. Aborting cancels the request in flight and rejects with the signal's reason (not a 408, and notUnexpected error: …). An aborted request is not retried.Proof
New tests, each failing on
mainand passing here:runtime-client.spec.ts›requestTimeoutMs› emits a timeout error: onmainthe error message is"Poll aborted"; here it is"Request timed out".runtime-client.spec.ts› aborts the request of a poll that takes too long, and ›close› aborts the in-flight poll request: onmainthe signalfetchreceived is stillaborted: falseafter the timeout orclose(); here it istrue.fetch-client.spec.ts› with an abort signal: onmainthe signal is ignored. The request hangs until Jest's 5 s test timeout, and an abort between retries still sends the retry. Here the request is aborted, rejects with the signal's reason, and no further attempt goes out.workos.spec.ts›get› when the signal aborts: same throughworkos.get. Onmainit hangs; here it rejects with the reason.End to end, I ran builds of
mainand this branch against a local server that accepts/sdk/feature-flagsand never answers, using realfetch. The server recorded when each request's connection closed.requestTimeoutMs: 500,pollingIntervalMs: 5000,close()at 12 s, observed for 70 s:Default
requestTimeoutMs,close()at 2 s while the first poll is in flight, observed for 66 s:Not covered
workos.getrejects only once that sleep ends (the computed backoff, or aRetry-Afterof up to 60 s). No request is in flight then, and the runtime client's poll still settles at its deadline. Making the sleep abortable would mean changingHttpClient.sleep, so I left it out.WorkOSlevel onlygettakessignal, since that is all the runtime client needs. The HTTP client honors it for every method.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
🤖 Generated with Claude Code