fix(feature-flags): Report poll timeouts and cancel abandoned requests

src/common/interfaces/get-options.interface.ts

@@ -6,4 +6,6 @@ export interface GetOptions {
   skipApiKeyCheck?: boolean;
   /** Maximum number of retries for this request, overriding the client-wide `maxRetries`. */
   maxRetries?: number;
+  /** Aborts the request, rejecting with the signal's reason. An aborted request is not retried. */
+  signal?: AbortSignal;
 }

src/common/interfaces/http-client.interface.ts

@@ -7,6 +7,8 @@ export type RequestOptions = {
    * `maxRetries`. Set to `0` to disable retries for this request.
    */
   maxRetries?: number;
+  /** Aborts the request. An aborted request is not retried. */
+  signal?: AbortSignal;
 };
 export type ResponseHeaderValue = string | string[];
 export type ResponseHeaders = Record<string, ResponseHeaderValue>;

src/common/net/fetch-client.spec.ts

@@ -661,6 +661,77 @@ describe('FetchHttpClient with timeout', () => {
   });
 });
 
+describe('FetchHttpClient with an abort signal', () => {
+  beforeEach(() => fetch.resetMocks());
+
+  it('aborts the request in flight and fails with the signal reason instead of retrying a timeout', async () => {
+    const mockFetch = jest.fn();
+    mockFetch.mockImplementation(
+      (_, options) =>
+        new Promise((_, reject) => {
+          options.signal.addEventListener('abort', () => {
+            const error = new Error('AbortError');
+            error.name = 'AbortError';
+            reject(error);
+          });
+        }),
+    );
+    const client = new FetchHttpClient(
+      'https://api.example.com',
+      {},
+      mockFetch,
+    );
+    const controller = new AbortController();
+    const reason = new Error('Stop');
+
+    const request = client.get('/users', { signal: controller.signal });
+    controller.abort(reason);
+
+    await expect(request).rejects.toBe(reason);
+    expect(mockFetch).toHaveBeenCalledTimes(1);
+    expect(mockFetch.mock.calls[0][1].signal.aborted).toBe(true);
+  });
+
+  it('fails with a null signal reason instead of returning no response', async () => {
+    const mockFetch = jest.fn();
+    mockFetch.mockImplementation(
+      (_, options) =>
+        new Promise((_, reject) => {
+          options.signal.addEventListener('abort', () => {
+            const error = new Error('AbortError');
+            error.name = 'AbortError';
+            reject(error);
+          });
+        }),
+    );
+    const client = new FetchHttpClient(
+      'https://api.example.com',
+      {},
+      mockFetch,
+    );
+    const controller = new AbortController();
+
+    const request = client.get('/users', { signal: controller.signal });
+    controller.abort(null);
+
+    await expect(request).rejects.toBeNull();
+    expect(mockFetch).toHaveBeenCalledTimes(1);
+  });
+
+  it('sends no further attempt once the signal aborts between retries', async () => {
+    fetchOnce({}, { status: 500 });
+    const controller = new AbortController();
+    const reason = new Error('Stop');
+    const mockSleep = jest.spyOn(fetchClient, 'sleep');
+    mockSleep.mockImplementation(async () => controller.abort(reason));
+
+    await expect(
+      fetchClient.get('/organizations', { signal: controller.signal }),
+    ).rejects.toBe(reason);
+    expect(fetch.mock.calls.length).toBe(1);
+  });
+});
+
 describe('request timeout covers the response body (GH-1679)', () => {
   // `fetch` in this file is the jest-fetch-mock import.
   type FetchFn = typeof globalThis.fetch;

src/common/net/fetch-client.ts

@@ -56,6 +56,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
       null,
       options.headers,
       options.maxRetries,
+      options.signal,
     );
   }
 
@@ -79,6 +80,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
         ...options.headers,
       },
       options.maxRetries,
+      options.signal,
     );
   }
 
@@ -102,6 +104,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
         ...options.headers,
       },
       options.maxRetries,
+      options.signal,
     );
   }
 
@@ -125,6 +128,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
         ...options.headers,
       },
       options.maxRetries,
+      options.signal,
     );
   }
 
@@ -144,6 +148,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
       null,
       options.headers,
       options.maxRetries,
+      options.signal,
     );
   }
 
@@ -167,6 +172,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
         ...options.headers,
       },
       options.maxRetries,
+      options.signal,
     );
   }
 
@@ -175,7 +181,10 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
     method: string,
     body?: any,
     headers?: RequestHeaders,
+    signal?: AbortSignal,
   ): Promise<HttpClientResponseInterface> {
+    signal?.throwIfAborted();
+
     // For methods which expect payloads, we should always pass a body value
     // even when it is empty. Without this, some JS runtimes (eg. Deno) will
     // inject a second Content-Length header.
@@ -193,17 +202,26 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
     const timeoutId = setTimeout(() => {
       abortController.abort();
     }, timeout);
+    const abortRequest = () => abortController.abort();
+    signal?.addEventListener('abort', abortRequest, { once: true });
+    const release = () => {
+      clearTimeout(timeoutId);
+      signal?.removeEventListener('abort', abortRequest);
+    };
     // Pass the response headers once they are known so a timeout while
-    // reading the body still carries the request ID and Retry-After.
-    const timeoutError = (responseHeaders?: Headers) =>
-      new HttpClientError({
-        message: `Request timeout after ${timeout}ms`,
-        response: {
-          status: 408,
-          headers: responseHeaders ?? new Headers(),
-          data: { error: 'Request timeout' },
-        },
-      });
+    // reading the body still carries the request ID and Retry-After. A
+    // request the caller aborted fails with the caller's reason instead.
+    const abortError = (responseHeaders?: Headers) =>
+      signal?.aborted
+        ? signal.reason
+        : new HttpClientError({
+            message: `Request timeout after ${timeout}ms`,
+            response: {
+              status: 408,
+              headers: responseHeaders ?? new Headers(),
+              data: { error: 'Request timeout' },
+            },
+          });
     // Set once the headers arrive.
     let res: Response | undefined;
 
@@ -226,7 +244,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
         // Read the error body under the same deadline, inside the attempt,
         // so a stalled error response is retried like any other timeout.
         const rawBody = await res.text();
-        clearTimeout(timeoutId);
+        release();
 
         let responseJson: any;
 
@@ -269,22 +287,22 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
           // AbortError a fetch implementation raises for an aborted body may
           // come from another realm.
           throw abortController.signal.aborted
-            ? timeoutError(response.headers)
+            ? abortError(response.headers)
             : error;
         } finally {
-          clearTimeout(timeoutId);
+          release();
         }
       })();
       rawBody.catch(() => undefined);
 
       return new FetchHttpClientResponse(res, rawBody);
     } catch (error) {
       // Clear timeout if request failed
-      clearTimeout(timeoutId);
+      release();
 
-      // Handle timeout errors
+      // Handle timeouts and aborts
       if (abortController.signal.aborted) {
-        throw timeoutError(res?.headers);
+        throw abortError(res?.headers);
       }
 
       throw error;
@@ -297,6 +315,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
     body?: any,
     headers?: RequestHeaders,
     maxRetries?: number,
+    signal?: AbortSignal,
   ): Promise<HttpClientResponseInterface> {
     const maxRetryAttempts = maxRetries ?? this.MAX_RETRY_ATTEMPTS;
 
@@ -316,14 +335,24 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
 
     const makeRequest = async (): Promise<HttpClientResponseInterface> => {
       let requestError: any = null;
+      // Tracked apart from the error: an abort can reject with a null reason.
+      let failed = false;
 
       try {
-        response = await this.fetchRequest(url, method, body, requestHeaders);
+        response = await this.fetchRequest(
+          url,
+          method,
+          body,
+          requestHeaders,
+          signal,
+        );
       } catch (e) {
         requestError = e;
+        failed = true;
       }
 
       if (
+        !signal?.aborted &&
         this.shouldRetryRequest(requestError, retryAttempts, maxRetryAttempts)
       ) {
         retryAttempts++;
@@ -334,7 +363,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface {
         return makeRequest();
       }
 
-      if (requestError != null) {
+      if (failed) {
         throw requestError;
       }
 

src/feature-flags/runtime-client.spec.ts

@@ -148,6 +148,53 @@ describe('FeatureFlagsRuntimeClient', () => {
       // Only the initial poll should have happened
       expect(fetch.mock.calls.length).toBe(1);
     });
+
+    it('aborts the in-flight poll request', async () => {
+      fetch.mockResponseOnce(() => new Promise(() => {}));
+      const client = workos.featureFlags.createRuntimeClient();
+      await jest.advanceTimersByTimeAsync(0);
+      const signal = fetch.mock.calls[0][1]?.signal;
+      expect(signal?.aborted).toBe(false);
+
+      client.close();
+
+      expect(signal?.aborted).toBe(true);
+    });
+  });
+
+  describe('requestTimeoutMs', () => {
+    it('emits a timeout error when a poll takes too long', async () => {
+      fetch.mockResponseOnce(() => new Promise(() => {}));
+      const client = workos.featureFlags.createRuntimeClient({
+        requestTimeoutMs: 50,
+      });
+      const errors: Error[] = [];
+      client.on('error', (err) => errors.push(err));
+
+      await jest.advanceTimersByTimeAsync(50);
+
+      expect(errors).toHaveLength(1);
+      expect(errors[0].message).toBe('Request timed out');
+
+      client.close();
+    });
+
+    it('aborts the request of a poll that takes too long', async () => {
+      fetch.mockResponseOnce(() => new Promise(() => {}));
+      const client = workos.featureFlags.createRuntimeClient({
+        requestTimeoutMs: 50,
+      });
+      client.on('error', () => {});
+
+      await jest.advanceTimersByTimeAsync(49);
+      const signal = fetch.mock.calls[0][1]?.signal;
+      expect(signal?.aborted).toBe(false);
+
+      await jest.advanceTimersByTimeAsync(1);
+      expect(signal?.aborted).toBe(true);
+
+      client.close();
+    });
   });
 
   describe('isEnabled', () => {

src/feature-flags/runtime-client.ts

@@ -233,38 +233,29 @@ export class FeatureFlagsRuntimeClient extends EventEmitter<RuntimeClientEvents>
     this.pollAbortController = new AbortController();
     const { signal } = this.pollAbortController;
 
-    let timeoutId: ReturnType<typeof setTimeout>;
+    const timeoutId = setTimeout(() => {
+      this.pollAbortController?.abort(new Error('Request timed out'));
+    }, this.requestTimeoutMs);
 
     const fetchPromise = this.workos
-      .get<unknown>('/sdk/feature-flags?payload_version=2')
+      .get<unknown>('/sdk/feature-flags?payload_version=2', { signal })
       .then(({ data }) => data);
 
-    const timeoutPromise = new Promise<never>((_, reject) => {
-      timeoutId = setTimeout(() => {
-        this.pollAbortController?.abort();
-        reject(new Error('Request timed out'));
-      }, this.requestTimeoutMs);
-    });
-
+    // Settles the poll as soon as it is aborted, even while the HTTP client
+    // is between retries.
     const abortPromise = new Promise<never>((_, reject) => {
       if (signal.aborted) {
-        reject(new Error('Poll aborted'));
+        reject(signal.reason);
         return;
       }
-      signal.addEventListener(
-        'abort',
-        () => reject(new Error('Poll aborted')),
-        {
-          once: true,
-        },
-      );
+      signal.addEventListener('abort', () => reject(signal.reason), {
+        once: true,
+      });
     });
 
-    return Promise.race([fetchPromise, timeoutPromise, abortPromise]).finally(
-      () => {
-        clearTimeout(timeoutId);
-      },
-    );
+    return Promise.race([fetchPromise, abortPromise]).finally(() => {
+      clearTimeout(timeoutId);
+    });
   }
 
   private scheduleNextPoll(): void {