--- sessionId: session-260906-073726-sbxb --- # Requirements ### Overview & Goals Improve the code quality, correctness, and maintainability of `QueueManager`, `TrackScheduler`, `TrackInfo`, and `VoteSkipService` — the core music queue and playback scheduling subsystem. ### Scope **In Scope:** - Fix thread-safety issues (check-then-act race conditions in `QueueManager`) - Simplify confusing queue insertion logic in `QueueManager.queue()` - Separate concerns: extract command-result formatting from queue data operations - Fix `TrackInfo` using wrong logger (`QueueManager.class` instead of `TrackInfo.class`) - Clean up `TrackScheduler.onTrackStuck()` complexity - Add unit tests for `QueueManager`, `TrackScheduler`, `TrackInfo`, and `VoteSkipService` - Expose skip-vote state through `TrackInfo` more cleanly (encapsulate the `Set` skips) **Out of Scope:** - Changes to `MusicManager`, `MusicServiceImpl`, or audio source/loading classes - New user-facing features or commands - Changes to `MusicManagerCallback` interface # Technical Design ### Current Implementation **`QueueManager`** (`src/main/java/me/bobert/netneutrality/music/QueueManager.java`, 286 lines): - Manages a `LinkedBlockingDeque` with manual `synchronized` blocks - Multiple methods check state outside the synchronized block, then re-check inside (e.g., `clearQueue()` L81 vs L86, `skip()` L197 vs L203, `shuffle()` L229 vs L233, `removeFromQueue()` L254 vs L259) — the outer checks are racy and misleading - `queue()` method (L135-158) has confusing branching: `addFirst` + `add` can result in the same track being added twice when `player.getPlayingTrack() == null && !queue.isEmpty()` - `clearQueue()` calls `callback.onQueueEmpty()` before checking if queue is empty (L79) - `removeTracksFromUser()` iterates the queue outside synchronized (L97), then again inside (L102) - `skip(int)` silently resets `nbTrack` to 1 if it exceeds queue size (L209) — surprising behavior - Mixes queue data operations with `CommandExecutionResult` formatting **`TrackScheduler`** (`src/main/java/me/bobert/netneutrality/music/TrackScheduler.java`, 175 lines): - `onTrackStuck()` (L129-173) has deeply nested logic with inline async recovery — hard to follow and test - Circular dependency: creates `QueueManager` in constructor, which takes `this` back - `context` field is accessed from multiple threads but only `attachContext`/`detachContext` are synchronized — `getContext()`, `getDj()` are not **`TrackInfo`** (`src/main/java/me/bobert/netneutrality/music/TrackInfo.java`, 83 lines): - Logger incorrectly uses `QueueManager.class` (L26) instead of `TrackInfo.class` - `getSkips()` returns mutable `Set` — breaks encapsulation - Duplicate copyright block (L1-5 and L7-11) **`VoteSkipService`** (`src/main/java/me/bobert/netneutrality/music/VoteSkipService.java`, 57 lines): - Relatively clean, but directly calls `queueManager.skip()` mixing vote logic with queue mutation - `info.addSkip()` uses `context.getGuildUser()` but `info.getSkips().size()` check uses raw set — no race protection ### Proposed Changes #### QueueManager 1. **Remove all outer unsynchronized checks** — move all logic inside a single `synchronized` block per method to eliminate check-then-act races 2. **Rewrite `queue()` method** — simplify to: add to queue, if nothing playing start the head track. Remove the confusing `addFirst`/`add` dual-path 3. **Fix `clearQueue()`** — move `callback.onQueueEmpty()` call to after the queue is confirmed non-empty and cleared 4. **Fix `skip(int)`** — when `nbTrack > queue.size()`, skip all remaining tracks instead of silently resetting to 1 5. **Fix `removeTracksFromUser()`** — do all filtering inside synchronized block 6. **Return queue operation results as data** (e.g., number removed, track name) and let callers build `CommandExecutionResult` — or at minimum, document that this class returns command results #### TrackScheduler 1. **Extract stuck-track recovery** into a dedicated `StuckTrackRecoveryHandler` class to reduce `onTrackStuck()` complexity 2. **Make `context` access thread-safe** — synchronize `getContext()` and `getDj()`, or use `volatile` 3. **Add `lastStuckTrack` reset** on track start or successful recovery to prevent stale state #### TrackInfo 1. **Fix logger** — change to `TrackInfo.class` 2. **Remove duplicate copyright block** 3. **Encapsulate skips** — return `Collections.unmodifiableSet(skips)` from `getSkips()`, add `getSkipCount()` method #### VoteSkipService 1. **Synchronize vote-skip check** — ensure `addSkip` + threshold check is atomic (or accept eventual consistency and document it) ### File Structure All changes in `src/main/java/me/bobert/netneutrality/music/`: - **Modified:** `QueueManager.java`, `TrackScheduler.java`, `TrackInfo.java`, `VoteSkipService.java` - **New:** `StuckTrackRecoveryHandler.java` - **New tests:** `src/test/java/me/bobert/netneutrality/feature/music/QueueManagerTest.java`, `TrackSchedulerTest.java`, `TrackInfoTest.java`, `VoteSkipServiceTest.java` # Testing ### Validation Approach Add unit tests for all four classes using JUnit 5 and Mockito (already in project dependencies via the existing test classes). ### Key Scenarios - **QueueManager:** queue/dequeue ordering, skip single/multiple, shuffle preserves current track, removeFromQueue bounds, clearQueue idempotency, removeTracksFromUser correctness - **TrackInfo:** skip vote tracking, duplicate skip prevention, unmodifiable skips set, equals/hashCode - **VoteSkipService:** threshold calculation, skip when below min members, vote counting, duplicate vote handling - **TrackScheduler:** onTrackEnd triggers correct queue action per end reason, onTrackStuck recovery flow ### Edge Cases - Queue with single track: skip, shuffle, remove - Skip count exceeding queue size - Empty queue operations - Concurrent vote-skip from same user # Delivery Steps ### ✓ Step 1: Fix TrackInfo issues TrackInfo has correct logger, clean copyright, and encapsulated skip state. - Fix logger from `QueueManager.class` to `TrackInfo.class` (line 26) - Remove duplicate copyright block (lines 7-11) - Change `getSkips()` to return `Collections.unmodifiableSet(skips)` - Add `getSkipCount()` convenience method - Add `TrackInfoTest.java` covering skip tracking, duplicate prevention, equals/hashCode, and unmodifiable set enforcement ### ✓ Step 2: Fix QueueManager thread-safety and queue logic QueueManager has correct synchronization and simplified queue insertion. - Remove all outer unsynchronized pre-checks in `clearQueue()`, `skip()`, `shuffle()`, `removeFromQueue()`, `removeTracksFromUser()` — consolidate into single synchronized blocks - Rewrite `queue(AudioTrack, Member)` to eliminate the confusing `addFirst`/`add` dual-path: simply add to tail, then start playback if nothing is playing - Fix `clearQueue()` to call `callback.onQueueEmpty()` only after confirming queue was non-empty - Fix `skip(int)` to skip all remaining when `nbTrack > queue.size()` instead of silently resetting to 1 - Fix `removeTracksFromUser()` to do all filtering inside synchronized block - Add `QueueManagerTest.java` covering queue ordering, skip, shuffle, remove, clear, and edge cases ### ✓ Step 3: Extract stuck-track recovery and fix TrackScheduler thread-safety TrackScheduler is simplified with extracted recovery logic and thread-safe context access. - Create `StuckTrackRecoveryHandler.java` encapsulating the retry-or-skip logic currently in `onTrackStuck()` (lines 129-173) - Simplify `onTrackStuck()` to delegate to the new handler - Reset `lastStuckTrack` in `onTrackStart()` to prevent stale state - Synchronize `getContext()` and `getDj()` methods (or make `context` volatile) - Add `TrackSchedulerTest.java` covering onTrackEnd behavior per end reason and stuck-track recovery delegation ### ✓ Step 4: Improve VoteSkipService robustness and add tests VoteSkipService has documented concurrency behavior and full test coverage. - Add documentation comment about the vote-then-check atomicity assumption - Add `VoteSkipServiceTest.java` covering: threshold math, auto-skip below min members, vote accumulation, duplicate vote from same user, skip trigger when threshold exceeded