66 67 68 69 70 71 multiple bugs #72

2026-06-22T10:42:08Z

FrauJulian commented

2026-06-22 10:42:08 +00:00

(Migrated from github.com)

This pull request significantly improves the robustness and reliability of the AudioManager class by enhancing error handling and cleanup logic, especially around voice connection and playback startup. It also introduces more comprehensive tests to cover these scenarios and tightens validation for volume control. The changes ensure that resources are properly cleaned up when failures occur and that the manager's state remains consistent, even during concurrent operations or unexpected errors.

Error Handling and Cleanup Improvements:

Improved connection startup logic in AudioManager to handle failures and concurrent connects. If connecting fails or is superseded by another connect/dispose, resources are cleaned up and the state is set appropriately. This prevents race conditions and ensures only the latest connection is kept.
Enhanced playback startup to await ffmpeg readiness and handle errors during ffmpeg/process/audio resource creation or playback. Proper cleanup is performed if any step fails, and state is updated accordingly.
The renewal timer logic now catches errors from start() and performs thorough cleanup if a renewal restart fails.

Validation and Utility Enhancements:

Tightened volume validation in setVolume to reject non-finite values (e.g., NaN, Infinity), not just out-of-range numbers.

FFmpeg Process Management:

The startFfmpeg function now returns a handle with a ready promise that resolves when ffmpeg produces output, or rejects with detailed errors (including recent stderr output) if startup fails. This prevents attempting playback before ffmpeg is ready and provides clearer diagnostics. [1] [2]

Testing Improvements:

Expanded the test suite for AudioManager to cover connection and playback startup failures, concurrent operations, cleanup on error, and edge cases like volume validation and renewal restart errors. Utility helpers were added for deferred promises and mock handles. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

This pull request significantly improves the robustness and reliability of the `AudioManager` class by enhancing error handling and cleanup logic, especially around voice connection and playback startup. It also introduces more comprehensive tests to cover these scenarios and tightens validation for volume control. The changes ensure that resources are properly cleaned up when failures occur and that the manager's state remains consistent, even during concurrent operations or unexpected errors. **Error Handling and Cleanup Improvements:** - Improved connection startup logic in `AudioManager` to handle failures and concurrent connects. If connecting fails or is superseded by another connect/dispose, resources are cleaned up and the state is set appropriately. This prevents race conditions and ensures only the latest connection is kept. - Enhanced playback startup to await ffmpeg readiness and handle errors during ffmpeg/process/audio resource creation or playback. Proper cleanup is performed if any step fails, and state is updated accordingly. - The renewal timer logic now catches errors from `start()` and performs thorough cleanup if a renewal restart fails. **Validation and Utility Enhancements:** - Tightened volume validation in `setVolume` to reject non-finite values (e.g., `NaN`, `Infinity`), not just out-of-range numbers. **FFmpeg Process Management:** - The `startFfmpeg` function now returns a handle with a `ready` promise that resolves when ffmpeg produces output, or rejects with detailed errors (including recent stderr output) if startup fails. This prevents attempting playback before ffmpeg is ready and provides clearer diagnostics. [[1]](diffhunk://#diff-cd98925a2cc62595a508ce6f147cbab6878794d7067403cdbe75d11223b24116L6-R18) [[2]](diffhunk://#diff-cd98925a2cc62595a508ce6f147cbab6878794d7067403cdbe75d11223b24116R55-R113) **Testing Improvements:** - Expanded the test suite for `AudioManager` to cover connection and playback startup failures, concurrent operations, cleanup on error, and edge cases like volume validation and renewal restart errors. Utility helpers were added for deferred promises and mock handles. [[1]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eR2-R11) [[2]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eR24-R30) [[3]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eL29-R43) [[4]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eL48-R60) [[5]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eR80-R99) [[6]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eR133-R217) [[7]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eR263-R394) [[8]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eR484-R489) [[9]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eR538-R551) [[10]](diffhunk://#diff-d1abb688b7da770c576ec69404a6218e1ded42ea38e3d0ad6a3eb40414ea815eR569-R589)

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2026-06-22 10:45:58 +00:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull request overview

This PR aims to harden AudioManager and ffmpeg startup behavior by making connection/playback transitions more failure-safe and by expanding tests around these error paths.

Changes:

Added ffmpeg “readiness” tracking (handle.ready) and improved startup diagnostics via FfmpegProcessError (including stderr tail).
Improved AudioManager connection/playback startup cleanup and tightened volume validation (Number.isFinite).
Expanded Jest coverage for connection/playback failures, concurrent operations, renewal restart failures, and volume edge cases.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`src/ffmpeg.ts`	Introduces `ready` promise for ffmpeg startup and richer error reporting.
`src/audio-manager.ts`	Adds more defensive state handling for connect/play, strengthens volume validation, and adds renewal failure cleanup.
`tests/ffmpeg.test.ts`	Adds tests for ffmpeg readiness success/failure and stderr-tail behavior.
`tests/audio-manager.test.ts`	Adds tests covering connect/play failure cleanup, concurrency, renewal failures, and volume validation edge cases.

Comments suppressed due to low confidence (1)

src/ffmpeg.ts:118

stopProcess calls childProcess.removeAllListeners(), which can prevent startFfmpeg().ready from ever settling if stop() is invoked before ffmpeg produces output. The readiness promise relies on error/exit/stdout.readable listeners; removing them mid-flight can leave callers awaiting forever (e.g., stopping playback while await handle.ready is in progress). Consider removing removeAllListeners() (or deferring it until after the ready promise has settled / process has exited) so readiness can deterministically resolve/reject.

function stopProcess(childProcess: ChildProcessByStdio<null, Readable, Readable>): void {
    childProcess.stdout.destroy();
    childProcess.stderr.destroy();
    childProcess.removeAllListeners();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview This PR aims to harden `AudioManager` and ffmpeg startup behavior by making connection/playback transitions more failure-safe and by expanding tests around these error paths. **Changes:** - Added ffmpeg “readiness” tracking (`handle.ready`) and improved startup diagnostics via `FfmpegProcessError` (including stderr tail). - Improved `AudioManager` connection/playback startup cleanup and tightened volume validation (`Number.isFinite`). - Expanded Jest coverage for connection/playback failures, concurrent operations, renewal restart failures, and volume edge cases. ### Reviewed changes Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments. | File | Description | | ---- | ----------- | | `src/ffmpeg.ts` | Introduces `ready` promise for ffmpeg startup and richer error reporting. | | `src/audio-manager.ts` | Adds more defensive state handling for connect/play, strengthens volume validation, and adds renewal failure cleanup. | | `tests/ffmpeg.test.ts` | Adds tests for ffmpeg readiness success/failure and stderr-tail behavior. | | `tests/audio-manager.test.ts` | Adds tests covering connect/play failure cleanup, concurrency, renewal failures, and volume validation edge cases. | <details> <summary>Comments suppressed due to low confidence (1)</summary> **src/ffmpeg.ts:118** * `stopProcess` calls `childProcess.removeAllListeners()`, which can prevent `startFfmpeg().ready` from ever settling if `stop()` is invoked before ffmpeg produces output. The readiness promise relies on `error/exit/stdout.readable` listeners; removing them mid-flight can leave callers awaiting forever (e.g., stopping playback while `await handle.ready` is in progress). Consider removing `removeAllListeners()` (or deferring it until after the ready promise has settled / process has exited) so readiness can deterministically resolve/reject. ``` function stopProcess(childProcess: ChildProcessByStdio<null, Readable, Readable>): void { childProcess.stdout.destroy(); childProcess.stderr.destroy(); childProcess.removeAllListeners(); ``` </details> --- 💡 <a href="/FrauJulian/Discord-Audio-Stream/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

src/audio-manager.ts

						
				@@ -93,1 +103,4 @@

				        if (this.connection !== connection) {

				            throw new AudioManagerStateError('Voice connection was stopped before it became ready.');

				        }

				        this.playbackState = 'ready';

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2026-06-22 10:45:57 +00:00

In connect(), the catch handler always sets this.playbackState = 'stopped' even if this connection attempt is no longer the active one (e.g., a newer concurrent connect() has already replaced this.connection, or the manager was disposed). This can incorrectly overwrite the state of a newer connection attempt or a disposed manager. Only transition to 'stopped' when this.connection === connection (i.e., when the failing attempt is still current).

In `connect()`, the `catch` handler always sets `this.playbackState = 'stopped'` even if this connection attempt is no longer the active one (e.g., a newer concurrent `connect()` has already replaced `this.connection`, or the manager was disposed). This can incorrectly overwrite the state of a newer connection attempt or a disposed manager. Only transition to `'stopped'` when `this.connection === connection` (i.e., when the failing attempt is still current).

src/audio-manager.ts

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2026-06-22 10:45:57 +00:00

If ffmpeg.ready rejects because playback was stopped/replaced while waiting (e.g., the process was terminated), this code will currently rethrow the underlying FfmpegProcessError. That makes cancellation look like a startup failure and can break the intended contract/tests that expect an AudioManagerStateError when playback is superseded. In the catch, if this.ffmpeg !== ffmpeg, treat it as a cancellation and throw AudioManagerStateError instead of propagating the ffmpeg error.

If `ffmpeg.ready` rejects because playback was stopped/replaced while waiting (e.g., the process was terminated), this code will currently rethrow the underlying `FfmpegProcessError`. That makes cancellation look like a startup failure and can break the intended contract/tests that expect an `AudioManagerStateError` when playback is superseded. In the `catch`, if `this.ffmpeg !== ffmpeg`, treat it as a cancellation and throw `AudioManagerStateError` instead of propagating the ffmpeg error.

src/ffmpeg.ts

						
				@@ -65,0 +101,4 @@

				        childProcess.once('error', onError);

				        childProcess.once('exit', onExit);

				        childProcess.stdout.once('readable', onReadable);

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2026-06-22 10:45:57 +00:00

waitForFfmpegOutput registers a stderr.on('data', …) listener but never unregisters it. When readiness resolves/rejects, that listener will remain attached for the lifetime of the process/stream, which can leak listeners across repeated starts and keep buffering stderr unnecessarily. Add stderr.off('data', appendStderr) to the cleanup() function so it is always removed on settle.

`waitForFfmpegOutput` registers a `stderr.on('data', …)` listener but never unregisters it. When readiness resolves/rejects, that listener will remain attached for the lifetime of the process/stream, which can leak listeners across repeated starts and keep buffering stderr unnecessarily. Add `stderr.off('data', appendStderr)` to the `cleanup()` function so it is always removed on settle.

chatgpt-codex-connector[bot] (Migrated from github.com) reviewed

2026-06-22 10:51:50 +00:00

chatgpt-codex-connector[bot] (Migrated from github.com) left a comment

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b4e92b9b7b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

### 💡 Codex Review Here are some automated review suggestions for this pull request. **Reviewed commit:** `b4e92b9b7b` <details> <summary>ℹ️ About Codex in GitHub</summary> Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you - Open a pull request for review - Mark a draft as ready - Comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. When you [sign up for Codex through ChatGPT](https://openai.com/codex), Codex can also answer questions or update the PR, like "@codex address that feedback". </details>

src/audio-manager.ts

						
				@@ -93,0 +101,4 @@

				            throw error;

				        }

				        if (this.connection !== connection) {

				            throw new AudioManagerStateError('Voice connection was stopped before it became ready.');

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-06-22 10:51:50 +00:00

Preserve state when stale connects fail

When overlapping connect() calls are made, the newer call destroys the previous connection; if the older entersState() later rejects after the newer connection has become ready, this catch still sets playbackState to stopped even though this.connection points at the newer live connection. That leaves the manager reporting stopped/not playing while it is connected and may already have scheduled renewal for the newer connection.

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Preserve state when stale connects fail** When overlapping `connect()` calls are made, the newer call destroys the previous connection; if the older `entersState()` later rejects after the newer connection has become ready, this catch still sets `playbackState` to `stopped` even though `this.connection` points at the newer live connection. That leaves the manager reporting `stopped`/not playing while it is connected and may already have scheduled renewal for the newer connection. Useful? React with 👍 / 👎.

src/audio-manager.ts

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-06-22 10:51:50 +00:00

Settle ffmpeg readiness when playback is stopped

If stop(), dispose(), or a replacement play() happens while ffmpeg has not produced output yet, stopCurrentPlayback() calls handle.stop(), and stopProcess() removes the child exit/error listeners without rejecting this ready promise. In that real startup/stall case this await never resumes, leaving the original play() promise pending forever even though playback was stopped.

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Settle ffmpeg readiness when playback is stopped** If `stop()`, `dispose()`, or a replacement `play()` happens while ffmpeg has not produced output yet, `stopCurrentPlayback()` calls `handle.stop()`, and `stopProcess()` removes the child `exit`/`error` listeners without rejecting this `ready` promise. In that real startup/stall case this `await` never resumes, leaving the original `play()` promise pending forever even though playback was stopped. Useful? React with 👍 / 👎.

src/audio-manager.ts

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-06-22 10:51:50 +00:00

Reset state when replacement startup fails

When play() is called while already playing or paused, stopCurrentPlayback() stops the old stream but leaves playbackState as playing/paused. If the new ffmpeg ready promise or createAudioResource() then throws, this catch cleans up only the handle and rethrows, so isPlaying/state can still report active playback even though no resource or ffmpeg remains.

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Reset state when replacement startup fails** When `play()` is called while already playing or paused, `stopCurrentPlayback()` stops the old stream but leaves `playbackState` as `playing`/`paused`. If the new ffmpeg `ready` promise or `createAudioResource()` then throws, this catch cleans up only the handle and rethrows, so `isPlaying`/`state` can still report active playback even though no resource or ffmpeg remains. Useful? React with 👍 / 👎.

src/audio-manager.ts

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-06-22 10:51:50 +00:00

Clean up after rejecting non-finite initial volume

With volume.enabled and volume.initialPercent set to NaN or Infinity, play() has already created this.ffmpeg and this.resource before calling setVolume(). This new validation now throws outside either playback cleanup block, so start()/play() rejects while leaving the ffmpeg process/resource retained; validate the initial volume before spawning or wrap that call in cleanup.

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Clean up after rejecting non-finite initial volume** With `volume.enabled` and `volume.initialPercent` set to `NaN` or `Infinity`, `play()` has already created `this.ffmpeg` and `this.resource` before calling `setVolume()`. This new validation now throws outside either playback cleanup block, so `start()`/`play()` rejects while leaving the ffmpeg process/resource retained; validate the initial volume before spawning or wrap that call in cleanup. Useful? React with 👍 / 👎.

src/audio-manager.ts

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-06-22 10:51:50 +00:00

Guard renewal cleanup against stale starts

After the renewal timer fires, its async start() can overlap with a user dispose() or a newer manual start(). If that older renewal later rejects, this catch unconditionally destroys whatever connection is currently stored and sets a disposed manager back to stopped, so a stale renewal failure can tear down a newer session or re-enable a disposed instance.

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Guard renewal cleanup against stale starts** After the renewal timer fires, its async `start()` can overlap with a user `dispose()` or a newer manual `start()`. If that older renewal later rejects, this catch unconditionally destroys whatever connection is currently stored and sets a disposed manager back to `stopped`, so a stale renewal failure can tear down a newer session or re-enable a disposed instance. Useful? React with 👍 / 👎.

src/ffmpeg.ts

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-06-22 10:51:50 +00:00

Don't resolve readiness on stdout EOF

When ffmpeg exits before writing any audio, stdout can still emit readable at EOF with read() returning null before the child exit event. This handler resolves ready unconditionally and removes the exit/error listeners, so invalid inputs or ffmpeg startup failures can proceed into AudioManager.play() as if audio is available instead of rejecting with FfmpegProcessError.

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Don't resolve readiness on stdout EOF** When ffmpeg exits before writing any audio, stdout can still emit `readable` at EOF with `read()` returning `null` before the child `exit` event. This handler resolves `ready` unconditionally and removes the exit/error listeners, so invalid inputs or ffmpeg startup failures can proceed into `AudioManager.play()` as if audio is available instead of rejecting with `FfmpegProcessError`. Useful? React with 👍 / 👎.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fraujulian/Discord-Audio-Stream#72