Skip to content

feat(): Critical Stability & Reliability Fixes - #12

Open
Arielpetit wants to merge 1 commit into
baairon:mainfrom
Arielpetit:fix/critical-bug-fixes
Open

feat(): Critical Stability & Reliability Fixes#12
Arielpetit wants to merge 1 commit into
baairon:mainfrom
Arielpetit:fix/critical-bug-fixes

Conversation

@Arielpetit

Copy link
Copy Markdown

PR Description: Critical Stability & Reliability Fixes

Overview

This PR resolves three critical issues affecting application reliability, performance, and data consistency. The changes eliminate unnecessary Spotify token refreshes, prevent MPV connection race conditions, and ensure accurate version tracking across library and playback history mutations.


Issues Fixed

1. Fixed Spotify Token Cache Invalidation

Problem

The Spotify token cache validation logic refreshed access tokens 30 seconds before their actual expiration, resulting in unnecessary authentication requests.

Root Cause

The cache validation used a buffered expiration check:

hit.expiresAtMs - Date.now() > TOKEN_SKEW_MS

Although the buffer was intended as a safety margin, it caused valid tokens to be discarded prematurely.

Solution

Updated the validation logic to check only whether the token has actually expired:

hit.expiresAtMs > Date.now()

Behavior Change

Before

  • Tokens refreshed up to 30 seconds early
  • Increased unnecessary Spotify API requests
  • Higher chance of hitting rate limits

After

  • Tokens remain in use until they actually expire
  • Fewer authentication requests
  • More efficient cache utilization

Impact

  • Reduced unnecessary API calls
  • Improved rate-limit efficiency
  • Better token cache performance

2. Fixed MPV Connection Reset Race Condition

Problem

A race condition existed between ensureStarted() and resetConnection(), allowing the connection state to become inconsistent during startup.

Root Cause

While ensureStarted() was establishing a connection, resetConnection() could execute simultaneously and clear the active ready promise.

This resulted in startup failures and unstable MPV connections.

Solution

Added a dedicated resetting flag to synchronize connection resets.

The implementation now:

  • Prevents concurrent reset operations
  • Guards against nested resets
  • Ensures startup and reset operations are atomic

Behavior Change

Before

  • Reset operations could interrupt startup
  • Connection state could become inconsistent
  • Playback initialization occasionally failed

After

  • Startup and reset operations are synchronized
  • Stable MPV connection lifecycle
  • Improved playback reliability

Impact

  • Eliminated startup race conditions
  • Improved connection stability
  • More predictable error handling

3. Fixed Library Index Version Tracking

Problem

Version tracking across the Library and PlayHistory stores could become stale after certain mutations.

Root Cause

The version counter depended on notify() being called, but not every mutation path triggered it.

As a result, getVersion() could return outdated values after some operations.

Solution

Moved version updates directly into every mutation method.

Updated methods include:

Library

  • upsert()
  • upsertMany()
  • remove()
  • removeMany()
  • clear()

PlayHistory

  • record()
  • retain()

Behavior Change

Before

  • Version values could become outdated
  • Change detection was unreliable
  • Synchronization issues were possible

After

  • Every mutation increments the version immediately
  • Accurate version reporting
  • Reliable synchronization across consumers

Impact

  • Improved data consistency
  • Reliable change detection
  • More predictable state management

Files Changed

src/sources/spotify/token.ts

Changes

  • Updated token expiration validation
  • Removed premature refresh buffer

Why

  • Prevent unnecessary Spotify authentication requests.

src/player/mpv.ts

Changes

  • Added resetting synchronization flag
  • Prevented overlapping connection resets
  • Protected startup sequence from race conditions

Why

  • Improve MPV connection reliability and stability.

src/library/library.ts

Changes

  • Increment version directly in all mutation methods
  • Removed dependency on notification callbacks for version tracking

Why

  • Ensure version accuracy after every state change.

src/player/history.ts

Changes

  • Added direct version updates in history mutations
  • Standardized version management

Why

  • Keep playback history version tracking consistent with the library store.

Verification

  • ✅ All 36 test files pass
  • 424 tests passing
  • ✅ Type checking successful
  • ✅ No breaking API changes
  • ✅ Backward compatible
  • ✅ Follows existing project architecture
  • ✅ Changes are minimal and focused

Impact

  • Reliability: Eliminates MPV startup race conditions.
  • Performance: Reduces unnecessary Spotify token refreshes and API calls.
  • Data Integrity: Ensures accurate version tracking across all mutation paths.
  • Maintainability: Simplifies version management and improves connection handling.
  • Compatibility: Fully backward compatible with existing behavior.

@Arielpetit

Copy link
Copy Markdown
Author

@gabconcepcionph @alzithetrivialmind @baairon can you please review this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant