fix external signal listener leak

Author: dlarocque

packages/ai/src/constants.ts

@@ -32,7 +32,7 @@ export const PACKAGE_VERSION = version;
 
 export const LANGUAGE_TAG = 'gl-js';
 
-export const DEFAULT_FETCH_TIMEOUT_MS = 180 * 1000;
+export const DEFAULT_FETCH_TIMEOUT_MS = 180 * 1000; // TODO: Extend default timeout to accommodate for longer generation requests with pro models.
 
 /**
  * Defines the name of the default in-cloud model to use for hybrid inference.

packages/ai/src/requests/request.test.ts

@@ -706,5 +706,124 @@ describe('request methods', () => {
       expect(fetchOptions.signal).to.be.instanceOf(AbortSignal);
       expect(fetchOptions.signal?.aborted).to.be.false;
     });
+
+    it('should remove abort listener on successful completion to prevent memory leaks', async () => {
+      const controller = new AbortController();
+      const addSpy = Sinon.spy(controller.signal, 'addEventListener');
+      const removeSpy = Sinon.spy(controller.signal, 'removeEventListener');
+
+      const mockResponse = new Response('{}', {
+        status: 200,
+        statusText: 'OK'
+      });
+      fetchStub.resolves(mockResponse);
+
+      await makeRequest(
+        {
+          model: 'models/model-name',
+          task: Task.GENERATE_CONTENT,
+          apiSettings: fakeApiSettings,
+          stream: false,
+          singleRequestOptions: { signal: controller.signal }
+        },
+        '{}'
+      );
+
+      expect(addSpy).to.have.been.calledOnceWith('abort');
+      expect(removeSpy).to.have.been.calledOnceWith('abort');
+    });
+
+    it('should remove listener if fetch itself rejects', async () => {
+      const controller = new AbortController();
+      const removeSpy = Sinon.spy(controller.signal, 'removeEventListener');
+      const error = new Error('Network failure');
+      fetchStub.rejects(error);
+
+      const requestPromise = makeRequest(
+        {
+          model: 'models/model-name',
+          task: Task.GENERATE_CONTENT,
+          apiSettings: fakeApiSettings,
+          stream: false,
+          singleRequestOptions: { signal: controller.signal }
+        },
+        '{}'
+      );
+
+      await expect(requestPromise).to.be.rejectedWith(AIError, /Network failure/);
+      expect(removeSpy).to.have.been.calledOnce;
+    });
+
+    it('should remove listener if response is not ok', async () => {
+      const controller = new AbortController();
+      const removeSpy = Sinon.spy(controller.signal, 'removeEventListener');
+      const mockResponse = new Response('{}', {
+        status: 500,
+        statusText: 'Internal Server Error'
+      });
+      fetchStub.resolves(mockResponse);
+
+      const requestPromise = makeRequest(
+        {
+          model: 'models/model-name',
+          task: Task.GENERATE_CONTENT,
+          apiSettings: fakeApiSettings,
+          stream: false,
+          singleRequestOptions: { signal: controller.signal }
+        },
+        '{}'
+      );
+
+      await expect(requestPromise).to.be.rejectedWith(AIError, /500/);
+      expect(removeSpy).to.have.been.calledOnce;
+    });
+
+    it('should abort immediately if timeout is 0', async () => {
+      fetchStub.callsFake(fetchAborter);
+      const requestPromise = makeRequest(
+        {
+          model: 'models/model-name',
+          task: Task.GENERATE_CONTENT,
+          apiSettings: fakeApiSettings,
+          stream: false,
+          singleRequestOptions: { timeout: 0 }
+        },
+        '{}'
+      );
+
+      // Tick the clock just enough to trigger a timeout(0)
+      await clock.tickAsync(1);
+
+      await expect(requestPromise).to.be.rejectedWith(
+        AIError,
+        /Timeout has expired/
+      );
+    });
+
+    it('should not error if signal is aborted after completion', async () => {
+      const controller = new AbortController();
+      const removeSpy = Sinon.spy(controller.signal, 'removeEventListener');
+      const mockResponse = new Response('{}', {
+        status: 200,
+        statusText: 'OK'
+      });
+      fetchStub.resolves(mockResponse);
+
+      await makeRequest(
+        {
+          model: 'models/model-name',
+          task: Task.GENERATE_CONTENT,
+          apiSettings: fakeApiSettings,
+          stream: false,
+          singleRequestOptions: { signal: controller.signal }
+        },
+        '{}'
+      );
+
+      // Listener should be removed, so this abort should do nothing.
+      controller.abort('Too late');
+
+      expect(removeSpy).to.have.been.calledOnce;
+    });
   });
 });

packages/ai/src/requests/request.ts

@@ -195,6 +195,12 @@ export async function makeRequest(
     );
   }, timeoutMillis);
 
+  const externalAbortListener = (): void => {
+    logger.debug(`Aborting request to ${url} due to external abort signal.`);
+    // If this listener was invoked, an external signal was aborted, so externalSignal must be defined.
+    internalAbortController.abort(externalSignal!.reason);
+  };
+
   if (externalSignal) {
     if (externalSignal.aborted) {
       clearTimeout(fetchTimeoutId);
@@ -204,14 +210,7 @@ export async function makeRequest(
       );
     }
 
-    const externalAbortListener = (): void => {
-      logger.debug(`Aborting request to ${url} due to external abort signal.`);
-      internalAbortController.abort(externalSignal.reason);
-    };
-
-    externalSignal.addEventListener('abort', externalAbortListener, {
-      once: true
-    });
+    externalSignal.addEventListener('abort', externalAbortListener);
   }
 
   try {
@@ -292,8 +291,9 @@ export async function makeRequest(
 
     throw err;
   } finally {
-    if (fetchTimeoutId) {
-      clearTimeout(fetchTimeoutId);
+    clearTimeout(fetchTimeoutId);
+    if (externalSignal) {
+      externalSignal.removeEventListener('abort', externalAbortListener);
     }
   }
   return response;