netneutrality/.junie/plans/queue-scheduler-improvements.md

sessionId

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<String> 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<TrackInfo> 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<String> — 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/command/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