fix(audio): preserve recording outcomes during cancellation and cleanup

src/helpers/audio.ts

@@ -100,7 +100,7 @@ export async function playAudio(input: NodeJS.ReadableStream | Response | File):
 
 /** Controls microphone selection and when an in-progress recording is finalized. */
 type RecordAudioOptions = {
-  /** Stops recording when aborted and resolves with the audio captured so far. */
+  /** Stops recording when aborted; successful termination returns the captured audio. */
   signal?: AbortSignal;
 
   /** Zero-based audio-input device number passed to FFmpeg; defaults to `0`. */
@@ -110,6 +110,14 @@ type RecordAudioOptions = {
   timeout?: number;
 };
 
+function removeRecordingAbortListener(signal: AbortSignal | undefined, listener: () => void): void {
+  try {
+    signal?.removeEventListener('abort', listener);
+  } catch {
+    // A cleanup callback must not replace the recording's terminal outcome.
+  }
+}
+
 function nodejsRecordAudio({ signal, device, timeout }: RecordAudioOptions = {}): Promise<File> {
   checkFileSupport();
   return new Promise((resolve, reject) => {
@@ -119,27 +127,48 @@ function nodejsRecordAudio({ signal, device, timeout }: RecordAudioOptions = {})
     let internalSignal: AbortSignal | undefined;
     let wasStopped = false;
     let settled = false;
+    let callerAbortObserved = false;
+    let timeoutAbortObserved = false;
+    let rejectRecording: (error: unknown) => void = reject;
 
     const collectData = (chunk: Buffer) => {
-      data.push(chunk);
+      if (!settled) {
+        data.push(chunk);
+      }
     };
     const stopRecording = () => {
       if (!settled && ffmpeg) {
-        wasStopped ||= ffmpeg.kill('SIGTERM');
+        try {
+          wasStopped ||= ffmpeg.kill('SIGTERM');
+        } catch (error) {
+          rejectRecording(error);
+        }
       }
     };
+    const stopCallerRecording = () => {
+      callerAbortObserved = true;
+      stopRecording();
+    };
+    const stopTimeoutRecording = () => {
+      timeoutAbortObserved = true;
+      stopRecording();
+    };
     const cleanup = () => {
       if (settled) {
         return false;
       }
 
       settled = true;
-      signal?.removeEventListener('abort', stopRecording);
-      internalSignal?.removeEventListener('abort', stopRecording);
-      ffmpeg?.stdout?.removeListener('data', collectData);
+      removeRecordingAbortListener(signal, stopCallerRecording);
+      removeRecordingAbortListener(internalSignal, stopTimeoutRecording);
+      try {
+        ffmpeg?.stdout?.removeListener('data', collectData);
+      } catch {
+        // Continue settling even when the output stream rejects cleanup.
+      }
       return true;
     };
-    const rejectRecording = (error: unknown) => {
+    rejectRecording = (error: unknown) => {
       if (cleanup()) {
         reject(error);
       }
@@ -154,6 +183,35 @@ function nodejsRecordAudio({ signal, device, timeout }: RecordAudioOptions = {})
       resolve(audioFile);
     };
 
+    const observeTimeout = () => {
+      internalSignal?.addEventListener('abort', stopTimeoutRecording, { once: true });
+      if (settled) {
+        removeRecordingAbortListener(internalSignal, stopTimeoutRecording);
+        return false;
+      }
+
+      if (internalSignal && !timeoutAbortObserved && internalSignal.aborted) {
+        stopTimeoutRecording();
+      }
+      return !settled;
+    };
+    const observeCaller = () => {
+      if (!signal) {
+        return;
+      }
+      if (signal.aborted) {
+        stopRecording();
+        return;
+      }
+
+      signal.addEventListener('abort', stopCallerRecording, { once: true });
+      if (settled) {
+        removeRecordingAbortListener(signal, stopCallerRecording);
+      } else if (!callerAbortObserved && signal.aborted) {
+        stopCallerRecording();
+      }
+    };
+
     try {
       if (typeof timeout === 'number' && (timeout > 0 || Number.isNaN(timeout))) {
         internalSignal = AbortSignal.timeout(timeout);
@@ -182,8 +240,16 @@ function nodejsRecordAudio({ signal, device, timeout }: RecordAudioOptions = {})
       ffmpeg.stdout?.on('data', collectData);
 
       ffmpeg.on('error', (error) => {
-        console.error(error);
-        rejectRecording(error);
+        if (!cleanup()) {
+          return;
+        }
+
+        try {
+          console.error(error);
+        } catch {
+          // A custom logger must not hide the original process failure.
+        }
+        reject(error);
       });
 
       ffmpeg.on('close', (code) => {
@@ -194,18 +260,23 @@ function nodejsRecordAudio({ signal, device, timeout }: RecordAudioOptions = {})
         returnData();
       });
 
-      internalSignal?.addEventListener('abort', stopRecording, { once: true });
+      if (settled || !observeTimeout()) {
+        return;
+      }
+      observeCaller();
+    } catch (error) {
+      if (!cleanup()) {
+        return;
+      }
 
-      if (signal) {
-        if (signal.aborted) {
-          stopRecording();
-        } else {
-          signal.addEventListener('abort', stopRecording, { once: true });
+      try {
+        if (!wasStopped) {
+          ffmpeg?.kill('SIGTERM');
         }
+      } catch {
+        // Preserve the original setup error when process termination also fails.
       }
-    } catch (error) {
-      stopRecording();
-      rejectRecording(error);
+      reject(error);
     }
   });
 }
@@ -216,13 +287,13 @@ function nodejsRecordAudio({ signal, device, timeout }: RecordAudioOptions = {})
  * This helper is supported only in Node.js-compatible runtimes and requires
  * the `ffmpeg` executable from FFmpeg to be available on `PATH`. Recording
  * continues until the FFmpeg process exits, the supplied signal is aborted, or
- * a positive timeout elapses. Aborting or timing out finalizes and returns the
- * audio captured so far instead of rejecting.
+ * a positive timeout elapses. Successful cancellation returns the audio captured
+ * so far; if sending `SIGTERM` throws, recording rejects with that error.
  *
  * @param options Audio-input device, optional abort signal, and recording timeout.
  * @returns The captured WAV file with MIME type `audio/wav`.
- * @throws {Error} If recording is unsupported, `ffmpeg` cannot start, or the
- * recording process exits unsuccessfully before an intentional stop.
+ * @throws {Error} If recording is unsupported, `ffmpeg` cannot start or exits
+ * unsuccessfully before an intentional stop, or sending it `SIGTERM` throws.
  */
 export async function recordAudio(options: RecordAudioOptions = {}): Promise<File> {
   if (isNode) {

tests/helpers/audio-recording.test.ts

@@ -136,6 +136,42 @@ describe('recordAudio', () => {
     },
   );
 
+  test('preserves captured audio when cleanup fails without collecting private later output', async () => {
+    const ffmpeg = mockFfmpeg();
+    const caller = new AbortController();
+    const timeout = new AbortController();
+    vi.spyOn(AbortSignal, 'timeout').mockReturnValue(timeout.signal);
+    const recording = recordAudio({ signal: caller.signal, timeout: 50 });
+    ffmpeg.stdout.write(Buffer.from('captured audio'));
+
+    const removeCaller = vi.spyOn(caller.signal, 'removeEventListener').mockImplementation(() => {
+      throw new Error('caller cleanup failed');
+    });
+    const removeTimeout = vi.spyOn(timeout.signal, 'removeEventListener');
+    const removeOutput = vi.spyOn(ffmpeg.stdout, 'removeListener').mockImplementation(() => {
+      ffmpeg.stdout.write(Buffer.from('private cleanup audio'));
+      throw new Error('stdout cleanup failed');
+    });
+    const concatenate = vi.spyOn(Buffer, 'concat');
+
+    expect(() => ffmpeg.emit('close', 0)).not.toThrow();
+    const chunks = concatenate.mock.calls[0]?.[0];
+    expect(chunks).toHaveLength(1);
+
+    const file = await recording;
+    expect(Buffer.from(await file.arrayBuffer()).toString()).toBe('captured audio');
+    expect(removeCaller).toHaveBeenCalledTimes(1);
+    expect(removeTimeout).toHaveBeenCalledTimes(1);
+    expect(removeOutput).toHaveBeenCalledTimes(1);
+    expect(getEventListeners(timeout.signal, 'abort')).toHaveLength(0);
+
+    ffmpeg.stdout.write(Buffer.from('private late audio'));
+    expect(chunks).toHaveLength(1);
+    caller.abort();
+    timeout.abort();
+    expect(ffmpeg.kill).not.toHaveBeenCalled();
+  });
+
   test('does not stop ffmpeg when an external signal aborts after recording completes', async () => {
     const ffmpeg = mockFfmpeg();
     const controller = new AbortController();
@@ -187,6 +223,30 @@ describe('recordAudio', () => {
     await expect(recording).rejects.toThrow('ffmpeg process exited with code 2');
   });
 
+  test.each(['caller', 'timeout'] as const)(
+    'rejects the original termination error when %s cancellation cannot signal ffmpeg',
+    async (target) => {
+      const ffmpeg = mockFfmpeg();
+      const caller = new AbortController();
+      const timeout = new AbortController();
+      vi.spyOn(AbortSignal, 'timeout').mockReturnValue(timeout.signal);
+      const recording = recordAudio({ signal: caller.signal, timeout: 50 });
+      const failure = new Error('signal delivery failed');
+      ffmpeg.kill.mockImplementation(() => {
+        throw failure;
+      });
+
+      (target === 'caller' ? caller : timeout).abort();
+
+      await expect(recording).rejects.toBe(failure);
+      expect(ffmpeg.kill).toHaveBeenCalledTimes(1);
+      expect(ffmpeg.kill).toHaveBeenCalledWith('SIGTERM');
+      expect(getEventListeners(caller.signal, 'abort')).toHaveLength(0);
+      expect(getEventListeners(timeout.signal, 'abort')).toHaveLength(0);
+      expect(ffmpeg.stdout.listenerCount('data')).toBe(0);
+    },
+  );
+
   test('immediately stops recording for an already-aborted signal', async () => {
     const ffmpeg = mockFfmpeg();
     const controller = new AbortController();
@@ -250,6 +310,30 @@ describe('recordAudio', () => {
     expect(consoleError).toHaveBeenCalledWith(failure);
   });
 
+  test('preserves the original process failure when cleanup and error logging throw', async () => {
+    const ffmpeg = mockFfmpeg();
+    const caller = new AbortController();
+    const timeout = new AbortController();
+    vi.spyOn(AbortSignal, 'timeout').mockReturnValue(timeout.signal);
+    const recording = recordAudio({ signal: caller.signal, timeout: 50 });
+    vi.spyOn(caller.signal, 'removeEventListener').mockImplementation(() => {
+      throw new Error('caller cleanup failed');
+    });
+    const logger = vi.spyOn(console, 'error').mockImplementation(() => {
+      throw new Error('logging failed');
+    });
+    const failure = new Error('microphone unavailable');
+
+    expect(() => ffmpeg.emit('error', failure)).not.toThrow();
+
+    await expect(recording).rejects.toBe(failure);
+    expect(logger).toHaveBeenCalledWith(failure);
+    expect(getEventListeners(timeout.signal, 'abort')).toHaveLength(0);
+    expect(ffmpeg.stdout.listenerCount('data')).toBe(0);
+    caller.abort();
+    expect(ffmpeg.kill).not.toHaveBeenCalled();
+  });
+
   test('reports synchronous ffmpeg startup failures', async () => {
     spawnMock.mockImplementation(() => {
       throw new Error('ffmpeg was not found');
@@ -270,6 +354,29 @@ describe('recordAudio', () => {
     expect(ffmpeg.stdout.listenerCount('data')).toBe(0);
   });
 
+  test('does not register cancellation listeners after process setup has already failed', async () => {
+    const ffmpeg = mockFfmpeg();
+    const caller = new AbortController();
+    const timeout = new AbortController();
+    vi.spyOn(AbortSignal, 'timeout').mockReturnValue(timeout.signal);
+    vi.spyOn(console, 'error').mockImplementation(() => {});
+    const failure = new Error('microphone failed while close observation was installed');
+
+    ffmpeg.on('newListener', (event) => {
+      if (event === 'close') {
+        ffmpeg.emit('error', failure);
+      }
+    });
+
+    await expect(recordAudio({ signal: caller.signal, timeout: 50 })).rejects.toBe(failure);
+    expect(getEventListeners(caller.signal, 'abort')).toHaveLength(0);
+    expect(getEventListeners(timeout.signal, 'abort')).toHaveLength(0);
+    expect(ffmpeg.stdout.listenerCount('data')).toBe(0);
+    caller.abort();
+    timeout.abort();
+    expect(ffmpeg.kill).not.toHaveBeenCalled();
+  });
+
   test('rejects an unexpected unsuccessful ffmpeg exit', async () => {
     const ffmpeg = mockFfmpeg();
     const recording = recordAudio();