Repository navigation
feat(bot): harden queue snapshot restore with staleness guard and double-restore prevention (#243) - #256
Conversation
…ble-restore prevention - Add staleness guard to restoreSnapshot() (default 30min maxAge) to skip snapshots that are too old to be meaningful - Prepend currentTrack to restore list so the track playing at disconnect time is also recovered, not only upcoming tracks - Delete snapshot from Redis after a successful restore to prevent the same session being re-applied on the next connection event - Wire MusicSessionSnapshotService constructor to ENVIRONMENT_CONFIG.SESSIONS .QUEUE_SESSION_TTL so the env var actually controls the singleton TTL - Use ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED in lifecycleHandlers instead of raw process.env for consistent config sourcing - Expand test coverage: staleness rejection, currentTrack restore, snapshot cleared on success, no-delete when no tracks restored, early return on non-empty queue Closes #243
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for regal-bunny-0c8efe ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR enhances the music session snapshot restoration feature by adding staleness guards (30-minute default threshold), explicit Redis deletion after successful restore, proper recovery of the currently playing track, and alignment of configuration sourcing to use environment config instead of direct process.env access. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Bot Lifecycle
participant Service as MusicSessionSnapshotService
participant Redis as Redis Store
participant Queue as GuildQueue
Client->>Service: restoreSnapshot(queue, requestedBy, {maxAgeMs})
alt Snapshot not found
Service-->>Client: Return early, no restore
else Snapshot stale (> maxAgeMs)
Service-->>Client: Skip restoration, do not delete
else Snapshot valid
Service->>Queue: Build track list (currentTrack + upcomingTracks)
Service->>Queue: Add tracks to queue
alt Queue was not playing
Service->>Queue: Start playback
end
Service->>Redis: DEL guild snapshot key
Redis-->>Service: Deletion confirmed
Service-->>Client: Return restore success with sessionSnapshotId
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bot/src/utils/music/sessionSnapshots.ts (1)
173-202:⚠️ Potential issue | 🟠 MajorReject malformed snapshots before the new staleness logic uses them.
The restore path still trusts the Redis payload shape, so non-numeric
savedAtor non-arrayupcomingTrackswill fall into the catch path and keep failing on every reconnect until TTL expiry instead of being rejected once as invalid.As per coding guidelines "For external data sources (HTTP, database), always validate and sanitize input using type guards or schema validators"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/sessionSnapshots.ts` around lines 173 - 202, The restore flow trusts Redis payload shape; add explicit validation immediately after retrieving snapshot from getSnapshot (before the staleness check) to ensure snapshot.savedAt is a finite number and snapshot.upcomingTracks is an array (and that snapshot.currentTrack is either null/undefined or an object that matches SnapshotTrack shape). If validation fails, log a debug/error message (similar to the existing debugLog) and return { restoredCount: 0, sessionSnapshotId: null }; optionally call this.deleteSnapshot(queue.guild.id) if available to purge the bad payload so it doesn't repeatedly fail on reconnect. Ensure the checks reference getSnapshot, snapshot.savedAt, snapshot.upcomingTracks, and snapshot.currentTrack so the guard runs before using those fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-28: Update the RELEASE notes to add traceability references:
append the issue and PR links `#243` and `#256` and parent work `#223` to the relevant
bullets about MusicSessionSnapshotService.deleteSnapshot(), restoreSnapshot(),
lifecycleHandlers.ts (ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED), and the
constructor change using ENVIRONMENT_CONFIG.SESSIONS.QUEUE_SESSION_TTL so each
added/changed line includes the corresponding “(fixes `#243`, PR `#256`, parent
`#223`)” style reference.
In `@packages/bot/src/utils/music/sessionSnapshots.spec.ts`:
- Around line 130-185: The test must assert the restore order so currentTrack is
prepended before upcomingTracks: after calling
MusicSessionSnapshotService.restoreSnapshot(queue) verify the first
addTrack/search call corresponds to snapshot.currentTrack (e.g., compare the
first call's query/url to snapshot.currentTrack.url or title) and the second
call corresponds to the upcomingTracks entry; use the jest mock call history on
addTrack or on player.search (e.g., addTrack.mock.calls[0],
addTrack.mock.calls[1] or player.search.mock.calls) to assert the exact sequence
rather than only checking restoredCount or call count.
In `@packages/bot/src/utils/music/sessionSnapshots.ts`:
- Around line 152-160: The deleteSnapshot path currently does a
non-authoritative DEL and swallows failures, and deletion happens before
verifying play() succeeded; fix by making restore-and-delete atomic and only
removing the snapshot after successful playback: replace the separate GET →
restore → DEL flow with an atomic fetch-and-delete (use Redis GETDEL or a small
EVAL/Lua script via redisClient.getDel/getdel or eval) in the restore code so
only one consumer can load the snapshot, and move any explicit
deleteSnapshot(guildId) call to run only after play() returns success; also stop
silently swallowing DEL failures in deleteSnapshot — surface/log and retry/raise
so deletion errors are not ignored. Ensure you update references to
deleteSnapshot, getKey, and the code that calls play() to follow this new atomic
restore-then-confirm-delete flow.
---
Outside diff comments:
In `@packages/bot/src/utils/music/sessionSnapshots.ts`:
- Around line 173-202: The restore flow trusts Redis payload shape; add explicit
validation immediately after retrieving snapshot from getSnapshot (before the
staleness check) to ensure snapshot.savedAt is a finite number and
snapshot.upcomingTracks is an array (and that snapshot.currentTrack is either
null/undefined or an object that matches SnapshotTrack shape). If validation
fails, log a debug/error message (similar to the existing debugLog) and return {
restoredCount: 0, sessionSnapshotId: null }; optionally call
this.deleteSnapshot(queue.guild.id) if available to purge the bad payload so it
doesn't repeatedly fail on reconnect. Ensure the checks reference getSnapshot,
snapshot.savedAt, snapshot.upcomingTracks, and snapshot.currentTrack so the
guard runs before using those fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5b7d9ab-a9fe-4312-94c2-43d66e5de22e
📒 Files selected for processing (4)
CHANGELOG.mdpackages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quality Gates
- GitHub Check: SonarCloud Scan
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{js,jsx,ts,tsx,vue,html}
📄 CodeRabbit inference engine (.cursor/rules/accessibility-openness.mdc)
Provide accessible UI components using semantic HTML and ARIA attributes where necessary
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/dependency-injection.mdc)
**/*.{ts,tsx,js,jsx}: Prefer constructor injection for classes that require dependencies
Avoid global mutable singletons unless necessary
Use explicit interfaces for external dependencies to make testing easier
**/*.{ts,tsx,js,jsx}: Include required references in PRs/code for non-trivial logic: TypeScript (official docs), MDN (JavaScript reference), and official docs for any runtime/framework/libraries used (e.g., Node.js, React) as applicable.
Before assuming behavior of an API, include the doc link and a ≤25-word quote when the change relies on it.
**/*.{ts,tsx,js,jsx}: Prefer named exports for clear usage and easier refactors in TypeScript/JavaScript
Keep import order consistent: external first, then internal modules
Remove dead code and unused imports
**/*.{ts,tsx,js,jsx}: Use PascalCase naming convention for React/UI components
Use camelCase naming convention for variables and functions
Use UPPER_SNAKE_CASE naming convention for constants
Maintain consistent import grouping and ordering within the project, keeping third-party imports separate from local imports
For external data sources (HTTP, database), always validate and sanitize input using type guards or schema validators
**/*.{ts,tsx,js,jsx}: Use Prettier with no semicolons, single quotes, 4-space indent, 80 character width
Files must not exceed 250 lines and this is enforcedImplement TypeScript typecheck and linter in CI quality checks
**/*.{ts,tsx,js,jsx}: Use TypeScript for enhanced type safety
Implement error handling and error logging
Avoid commenting code unless extremely necessary - code should explain itself with descriptive names
Leave NO todos, placeholders or missing pieces in the code
Variables and functions must use camelCase
Constants must use UPPER_SNAKE_CASE
Use arrow functions for methods and computed properties
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements
Maintain consistent import grouping/order: external imports first, then...
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{js,jsx,ts,tsx}: Never throw strings. ThrowError(or typed subclasses) with descriptive messages
Include causal error ascausewhen available for better debugging
Define clear, stable error codes (e.g.,ERR_AUTH_EXPIRED,ERR_NETWORK_TIMEOUT)
Provide optional metadata (e.g.,details,retryable,status,correlationId) in error objects
Use domain error classes per area (e.g.,AuthenticationError,ValidationError,NetworkError)
Log errors with structure (message, code, stack, cause, correlationId, user context where appropriate)
MarkretryablevsnonRetryableerrors where helpful for operations
Set timeouts and handle aborts/cancellations; avoid dangling requests in API/network code
Implement backoff for transient failures; avoid infinite retries
Map HTTP status → domain errors; 4xx vs 5xx behave differently (e.g., retry for 5xx/network)
**/*.{js,jsx,ts,tsx}: Use functional components with hooks in React/React Native. Avoid class components.
Keep components focused on a single responsibility; extract complex logic into custom hooks.
Keep state local when possible. Use Context/Zustand/Redux only when necessary for state management.
If props or state traverse more than 3 levels, consider using context or a feature-scoped store instead of prop drilling.
Use performance optimization techniques:React.memo,useMemo,useCallback,Suspense(web), and virtualization for long lists; avoid unnecessary re-renders.
Web accessibility: use semantic HTML, labels, focus management, keyboard navigation, andaria-*attributes as needed.
React Native accessibility: use accessibility props (accessible,accessibilityLabel), proper roles and labels.
Identify and extract repetitive UI components proactively tocomponents/with clear props and minimal coupling.
Web styles: prefer co-located styles or design system tokens; avoid global style leakage.
React Native styles: preferStyleSheet.create, design tokens, and theme providers; avoid in...
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/pattern.mdc)
Introduce interfaces at module boundaries to enable testing and substitutions
**/*.{ts,tsx}: Avoid usinganytype in TypeScript. If unavoidable, useunknownwith type guards and justify with a code comment
Preferinterfacefor defining public object shapes in TypeScript, usetypefor unions and utility types
Use TypeScript utility types such asPartial,Pick,Omit,Readonly, andRecordwhen appropriate
UseI{Name}naming convention for interfaces in TypeScript
UseT{Name}naming convention for type aliases and utility types in TypeScript
**/*.{ts,tsx}: Functions must be less than 50 lines with cyclomatic complexity less than 10
Do not useanytypes - ESLint enforces this at error level
**/*.{ts,tsx}: Prefer types over interfaces for most cases
Don't ever useany- type safety always
Avoid enums; use const objects instead
For complex types, create a separate file to declare them and import them
Avoid usinganytype; if unavoidable, useunknownwith type guards and justify with code comment
Preferinterfacefor public API shapes; usetypefor unions and utility types
Use TypeScript utility types (Partial, Pick, Omit, Readonly, Record)
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/documentation.mdc)
**/*.{js,ts,tsx,jsx}: Minimize comments in code; explain the 'why' when non-obvious, let code express the 'what' through clear naming
Document trade-offs briefly when deviating from ideal patterns
**/*.{js,ts,tsx,jsx}: Store secrets, ports, and hosts in environment variables (.env,.env.example) and never hardcode them
Avoid redundant or decorative AI comments; code should be self-explanatory and only commented when logic is non-obvious; prefer refactoring over lengthy comments
**/*.{js,ts,tsx,jsx}: No hardcoded secrets, IPs, or ports in code; use.envanddocs/for required environment variables
Avoid redundant or decorative AI comments; code should be clear from names and structure; comment only when logic is non-obvious
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
packages/bot/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/lucky-discord-bot.mdc)
packages/bot/**/*.{ts,tsx}: UseuseMainPlayer()fromdiscord-playerto access the player instance; do not instantiate player directly
Do not duplicate queue or player state outside Discord Player; use shared services from@lucky/sharedfor persistent data like track history and session information
UseerrorLoganddebugLogfrom@lucky/shared/utilsfor logging throughout the bot package
Use embed and reply utilities from@lucky/sharedfor consistent message formatting and error sanitization across the bot
Use services from@lucky/shared(DatabaseService, Redis client) for database and cache access; do not instantiate Prisma or Redis directly in the bot package
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
packages/bot/src/handlers/player/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/lucky-discord-bot.mdc)
Track handling, errors, and lifecycle must be managed through dedicated handlers in
packages/bot/src/handlers/player/(trackHandlers, errorHandlers, lifecycleHandlers)
Files:
packages/bot/src/handlers/player/lifecycleHandlers.ts
packages/bot/**
📄 CodeRabbit inference engine (.cursor/rules/lucky-project.mdc)
The
botpackage depends onsharedand contains Discord bot commands and player handlers using Discord.js and Discord Player
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
**/*.{js,mjs,ts,mts}
📄 CodeRabbit inference engine (.cursor/rules/lucky-project.mdc)
Use Node.js version ≥22 with ESM (ECMAScript modules) only; no CommonJS
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
packages/bot/src/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/subagent-discord.mdc)
Use
@lucky/sharedfor database, Redis, logging, and embed utilities instead of implementing them locally
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
packages/bot/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use Discord.js + Discord Player for Discord bot commands, events, and music features in
packages/bot
Files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{test,spec}.{js,jsx,ts,tsx}: Test behavior, not implementation. Prefer Testing Library utilities for testing React/React Native components.
For React Native tests: mock native modules and test component interactions and accessibility labels.
Files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/pattern.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Test behavior, not implementation details
Prefer unit tests for core logic; add integration tests at meaningful boundaries
Files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/testing-quality.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Use Jest + a React testing library for unit and component tests as applicable
Test behavior, not implementation details
Files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
**/*.{spec,test}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/quality.mdc)
**/*.{spec,test}.{ts,tsx,js,jsx}: Use Jest for unit and integration tests
Test behavior, not implementation details
Run unit, integration tests, and coverage report in CI quality checks
Files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/quality.mdc)
Unit tests must use naming convention
*.spec.ts
Files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
{CHANGELOG.md,README.md}
📄 CodeRabbit inference engine (.cursor/rules/agent-rules.mdc)
ALWAYS update CHANGELOG.md and README.md as changes are made.
Files:
CHANGELOG.md
CHANGELOG.md
📄 CodeRabbit inference engine (.cursor/rules/templates-examples.mdc)
CHANGELOG.md must be updated with all changes in pull requests
Always update CHANGELOG.md with all code changes
Update CHANGELOG.md with all changes, include breaking changes documentation, and reference issues and PRs
Files:
CHANGELOG.md
{CHANGELOG.md,docs/**}
📄 CodeRabbit inference engine (.cursor/rules/lucky-project.mdc)
Update
CHANGELOG.mdand relevantdocs/files when behavior or setup changes
Files:
CHANGELOG.md
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-discord-bot.mdc:0-0
Timestamp: 2026-03-09T20:20:47.877Z
Learning: Applies to packages/bot/**/*.{ts,tsx} : Do not duplicate queue or player state outside Discord Player; use shared services from `lucky/shared` for persistent data like track history and session information
📚 Learning: 2026-03-09T20:20:47.877Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-discord-bot.mdc:0-0
Timestamp: 2026-03-09T20:20:47.877Z
Learning: Applies to packages/bot/src/handlers/player/**/*.{ts,tsx} : Track handling, errors, and lifecycle must be managed through dedicated handlers in `packages/bot/src/handlers/player/` (trackHandlers, errorHandlers, lifecycleHandlers)
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.ts
📚 Learning: 2026-03-09T20:20:47.877Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-discord-bot.mdc:0-0
Timestamp: 2026-03-09T20:20:47.877Z
Learning: Applies to packages/bot/**/*.{ts,tsx} : Do not duplicate queue or player state outside Discord Player; use shared services from `lucky/shared` for persistent data like track history and session information
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
📚 Learning: 2026-03-09T20:21:52.065Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-discord.mdc:0-0
Timestamp: 2026-03-09T20:21:52.065Z
Learning: Applies to packages/bot/src/functions/music/**/*.ts : Use existing voice/queue/guild validators before manipulating player or queue state
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
📚 Learning: 2026-03-09T20:20:38.694Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-backend-api.mdc:0-0
Timestamp: 2026-03-09T20:20:38.694Z
Learning: Applies to packages/backend/src/**/*.{ts,tsx} : Use shared config and env from `lucky/shared` when needed; avoid duplicating env parsing in backend code
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
📚 Learning: 2026-03-09T20:21:52.065Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-discord.mdc:0-0
Timestamp: 2026-03-09T20:21:52.065Z
Learning: Applies to packages/bot/src/functions/music/commands/**/*.ts : Use `.cursor/skills/music-queue-player/SKILL.md` for play, queue, skip, volume commands and player lifecycle management
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
📚 Learning: 2026-03-09T20:20:47.877Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-discord-bot.mdc:0-0
Timestamp: 2026-03-09T20:20:47.877Z
Learning: Applies to packages/bot/**/*.{ts,tsx} : Use `useMainPlayer()` from `discord-player` to access the player instance; do not instantiate player directly
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.ts
📚 Learning: 2026-03-09T20:20:38.694Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-backend-api.mdc:0-0
Timestamp: 2026-03-09T20:20:38.694Z
Learning: Applies to packages/backend/src/**/*.{ts,tsx} : Keep tokens and secrets in environment variables only; never hardcode or expose in code
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.ts
📚 Learning: 2026-03-09T20:20:38.694Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-backend-api.mdc:0-0
Timestamp: 2026-03-09T20:20:38.694Z
Learning: Applies to packages/backend/src/{services,middleware}/**/*.{ts,tsx} : Use SessionService in middleware for session handling; keep session and auth logic centralized
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.ts
📚 Learning: 2026-03-09T20:21:52.065Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-discord.mdc:0-0
Timestamp: 2026-03-09T20:21:52.065Z
Learning: Applies to packages/bot/src/functions/{general,music,download}/commands/**/*.ts : Apply `.cursor/rules/lucky-discord-bot.mdc` rules for Discord bot commands and player implementation
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.ts
📚 Learning: 2026-03-14T23:37:46.576Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-14T23:37:46.576Z
Learning: Applies to packages/bot/**/*.{js,ts} : Use Discord.js + Discord Player for Discord bot commands, events, and music features in `packages/bot`
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.ts
📚 Learning: 2026-03-09T20:21:15.595Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-shared.mdc:0-0
Timestamp: 2026-03-09T20:21:15.595Z
Learning: Applies to packages/shared/**/*.ts : Organize the Lucky Shared Package with the following directory structure: Config in `packages/shared/src/config/` (environment, constants, feature toggles, YouTube config); Services in `packages/shared/src/services/` (DatabaseService, Redis client/operations, FeatureToggleService, ReactionRoles, RoleManagement); Types in `packages/shared/src/types/` (errors, commands, common, discord, music); Utils in `packages/shared/src/utils/` (error handling, retry, embeds, log, monitoring, composables, prismaClient)
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
📚 Learning: 2026-03-09T20:21:38.098Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-backend.mdc:0-0
Timestamp: 2026-03-09T20:21:38.098Z
Learning: Applies to packages/backend/**/*.ts : Use `lucky/shared` for config and DB/Redis when needed in backend code
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
📚 Learning: 2026-03-09T20:21:08.612Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-project.mdc:0-0
Timestamp: 2026-03-09T20:21:08.612Z
Learning: Applies to packages/bot/** : The `bot` package depends on `shared` and contains Discord bot commands and player handlers using Discord.js and Discord Player
Applied to files:
packages/bot/src/handlers/player/lifecycleHandlers.ts
📚 Learning: 2026-03-09T20:21:15.595Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-shared.mdc:0-0
Timestamp: 2026-03-09T20:21:15.595Z
Learning: Applies to packages/shared/**/*.ts : Use Redis client and operations located in `packages/shared/src/services/redis/`; use for cache, sessions, and rate limits as defined by existing keys and types
Applied to files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
📚 Learning: 2026-03-09T20:21:08.612Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-project.mdc:0-0
Timestamp: 2026-03-09T20:21:08.612Z
Learning: Applies to {packages/*/tests/**/*.test.{js,ts},tests/**/*.test.{js,ts}} : Add or adjust unit and integration tests when changing behavior; follow existing patterns in `packages/*/tests` and root `tests/` directories
Applied to files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
📚 Learning: 2026-03-14T23:37:46.576Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-14T23:37:46.576Z
Learning: Use the `testing-lucky` skill for unit tests, Jest ESM mocks, and fixing disabled tests
Applied to files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
📚 Learning: 2026-03-09T20:21:52.065Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-discord.mdc:0-0
Timestamp: 2026-03-09T20:21:52.065Z
Learning: Applies to packages/bot/src/**/*.ts : Use `lucky/shared` for database, Redis, logging, and embed utilities instead of implementing them locally
Applied to files:
packages/bot/src/utils/music/sessionSnapshots.spec.tspackages/bot/src/utils/music/sessionSnapshots.ts
📚 Learning: 2026-03-09T20:20:47.877Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-discord-bot.mdc:0-0
Timestamp: 2026-03-09T20:20:47.877Z
Learning: Applies to packages/bot/**/*.{ts,tsx} : Use services from `lucky/shared` (DatabaseService, Redis client) for database and cache access; do not instantiate Prisma or Redis directly in the bot package
Applied to files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
📚 Learning: 2026-03-09T20:21:08.612Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-project.mdc:0-0
Timestamp: 2026-03-09T20:21:08.612Z
Learning: Applies to {packages/shared/**,prisma/**}/**/*.{ts,js} : Use Prisma for PostgreSQL database and Redis for caching with shared client configuration
Applied to files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
📚 Learning: 2026-03-14T23:37:46.576Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-14T23:37:46.576Z
Learning: Use the `prisma-redis-lucky` skill for schema, migrations, and DB/Redis operations in shared package
Applied to files:
packages/bot/src/utils/music/sessionSnapshots.spec.ts
| ### Added | ||
|
|
||
| - `MusicSessionSnapshotService.deleteSnapshot()` — explicit Redis delete after a | ||
| successful restore so the same snapshot is not re-applied on subsequent | ||
| connections (double-restore prevention). | ||
|
|
||
| ### Changed | ||
|
|
||
| - Session snapshot restore now prepends `currentTrack` to the restored queue so | ||
| the track that was playing at crash/disconnect time is also recovered, not | ||
| only upcoming tracks. | ||
| - `restoreSnapshot()` now enforces a configurable `maxAgeMs` staleness guard | ||
| (default 30 minutes) and skips snapshots older than the threshold. | ||
| - `lifecycleHandlers.ts` now reads the session-restore gate from | ||
| `ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED` instead of raw | ||
| `process.env`, aligning with the shared config source of truth. | ||
| - `MusicSessionSnapshotService` constructor now uses | ||
| `ENVIRONMENT_CONFIG.SESSIONS.QUEUE_SESSION_TTL` as the default TTL so the | ||
| `QUEUE_SESSION_TTL` env var is respected at the singleton level. |
There was a problem hiding this comment.
Add the linked work references to this unreleased entry.
The new bullets don't mention issue #243, PR #256, or parent work #223, so this changelog entry still misses the traceability the repo expects for release notes.
As per coding guidelines "Update CHANGELOG.md with all changes, include breaking changes documentation, and reference issues and PRs"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 10 - 28, Update the RELEASE notes to add
traceability references: append the issue and PR links `#243` and `#256` and parent
work `#223` to the relevant bullets about
MusicSessionSnapshotService.deleteSnapshot(), restoreSnapshot(),
lifecycleHandlers.ts (ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED), and the
constructor change using ENVIRONMENT_CONFIG.SESSIONS.QUEUE_SESSION_TTL so each
added/changed line includes the corresponding “(fixes `#243`, PR `#256`, parent
`#223`)” style reference.
| it('also restores currentTrack prepended to the queue (Gap 4 fix)', async () => { | ||
| const service = new MusicSessionSnapshotService(300) | ||
| const snapshot = { | ||
| sessionSnapshotId: 'snap-ct', | ||
| guildId: 'guild-ct', | ||
| savedAt: Date.now(), | ||
| currentTrack: { | ||
| title: 'Was Playing', | ||
| author: 'Artist A', | ||
| url: 'https://example.com/current', | ||
| duration: '3:00', | ||
| source: 'youtube', | ||
| }, | ||
| upcomingTracks: [ | ||
| { | ||
| title: 'Next Up', | ||
| author: 'Artist B', | ||
| url: 'https://example.com/next', | ||
| duration: '2:30', | ||
| source: 'youtube', | ||
| }, | ||
| ], | ||
| } | ||
| getMock.mockResolvedValue(JSON.stringify(snapshot)) | ||
| delMock.mockResolvedValue(1) | ||
|
|
||
| const addTrack = jest.fn() | ||
| const queue = { | ||
| guild: { id: 'guild-ct' }, | ||
| currentTrack: null, | ||
| tracks: { size: 0 }, | ||
| addTrack, | ||
| node: { | ||
| isPlaying: () => false, | ||
| play: jest.fn().mockResolvedValue(undefined), | ||
| }, | ||
| player: { | ||
| search: jest.fn().mockImplementation(async (query: unknown) => ({ | ||
| tracks: [ | ||
| { | ||
| title: String(query).split(' ')[0], | ||
| author: 'resolved', | ||
| url: query, | ||
| metadata: {}, | ||
| }, | ||
| ], | ||
| })), | ||
| }, | ||
| } as unknown as GuildQueue | ||
|
|
||
| const result = await service.restoreSnapshot(queue) | ||
|
|
||
| // currentTrack + 1 upcoming = 2 tracks | ||
| expect(result.restoredCount).toBe(2) | ||
| expect(addTrack).toHaveBeenCalledTimes(2) | ||
| }) |
There was a problem hiding this comment.
Assert prepend order here, not just the number of restored tracks.
This test still passes if currentTrack is restored after upcomingTracks, because it only checks restoredCount and total calls. Please assert the first and second search/addTrack calls so the prepend contract is actually covered.
Suggested assertion update
- const addTrack = jest.fn()
+ const addTrack = jest.fn()
+ const search = jest.fn().mockImplementation(async (query: unknown) => ({
+ tracks: [
+ {
+ title: String(query),
+ author: 'resolved',
+ url: query,
+ metadata: {},
+ },
+ ],
+ }))
const queue = {
guild: { id: 'guild-ct' },
currentTrack: null,
tracks: { size: 0 },
addTrack,
@@
- player: {
- search: jest.fn().mockImplementation(async (query: unknown) => ({
- tracks: [
- {
- title: String(query).split(' ')[0],
- author: 'resolved',
- url: query,
- metadata: {},
- },
- ],
- })),
- },
+ player: { search },
} as unknown as GuildQueue
const result = await service.restoreSnapshot(queue)
// currentTrack + 1 upcoming = 2 tracks
expect(result.restoredCount).toBe(2)
expect(addTrack).toHaveBeenCalledTimes(2)
+ expect(search).toHaveBeenNthCalledWith(
+ 1,
+ 'https://example.com/current',
+ expect.any(Object),
+ )
+ expect(search).toHaveBeenNthCalledWith(
+ 2,
+ 'https://example.com/next',
+ expect.any(Object),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('also restores currentTrack prepended to the queue (Gap 4 fix)', async () => { | |
| const service = new MusicSessionSnapshotService(300) | |
| const snapshot = { | |
| sessionSnapshotId: 'snap-ct', | |
| guildId: 'guild-ct', | |
| savedAt: Date.now(), | |
| currentTrack: { | |
| title: 'Was Playing', | |
| author: 'Artist A', | |
| url: 'https://example.com/current', | |
| duration: '3:00', | |
| source: 'youtube', | |
| }, | |
| upcomingTracks: [ | |
| { | |
| title: 'Next Up', | |
| author: 'Artist B', | |
| url: 'https://example.com/next', | |
| duration: '2:30', | |
| source: 'youtube', | |
| }, | |
| ], | |
| } | |
| getMock.mockResolvedValue(JSON.stringify(snapshot)) | |
| delMock.mockResolvedValue(1) | |
| const addTrack = jest.fn() | |
| const queue = { | |
| guild: { id: 'guild-ct' }, | |
| currentTrack: null, | |
| tracks: { size: 0 }, | |
| addTrack, | |
| node: { | |
| isPlaying: () => false, | |
| play: jest.fn().mockResolvedValue(undefined), | |
| }, | |
| player: { | |
| search: jest.fn().mockImplementation(async (query: unknown) => ({ | |
| tracks: [ | |
| { | |
| title: String(query).split(' ')[0], | |
| author: 'resolved', | |
| url: query, | |
| metadata: {}, | |
| }, | |
| ], | |
| })), | |
| }, | |
| } as unknown as GuildQueue | |
| const result = await service.restoreSnapshot(queue) | |
| // currentTrack + 1 upcoming = 2 tracks | |
| expect(result.restoredCount).toBe(2) | |
| expect(addTrack).toHaveBeenCalledTimes(2) | |
| }) | |
| it('also restores currentTrack prepended to the queue (Gap 4 fix)', async () => { | |
| const service = new MusicSessionSnapshotService(300) | |
| const snapshot = { | |
| sessionSnapshotId: 'snap-ct', | |
| guildId: 'guild-ct', | |
| savedAt: Date.now(), | |
| currentTrack: { | |
| title: 'Was Playing', | |
| author: 'Artist A', | |
| url: 'https://example.com/current', | |
| duration: '3:00', | |
| source: 'youtube', | |
| }, | |
| upcomingTracks: [ | |
| { | |
| title: 'Next Up', | |
| author: 'Artist B', | |
| url: 'https://example.com/next', | |
| duration: '2:30', | |
| source: 'youtube', | |
| }, | |
| ], | |
| } | |
| getMock.mockResolvedValue(JSON.stringify(snapshot)) | |
| delMock.mockResolvedValue(1) | |
| const addTrack = jest.fn() | |
| const search = jest.fn().mockImplementation(async (query: unknown) => ({ | |
| tracks: [ | |
| { | |
| title: String(query), | |
| author: 'resolved', | |
| url: query, | |
| metadata: {}, | |
| }, | |
| ], | |
| })) | |
| const queue = { | |
| guild: { id: 'guild-ct' }, | |
| currentTrack: null, | |
| tracks: { size: 0 }, | |
| addTrack, | |
| node: { | |
| isPlaying: () => false, | |
| play: jest.fn().mockResolvedValue(undefined), | |
| }, | |
| player: { search }, | |
| } as unknown as GuildQueue | |
| const result = await service.restoreSnapshot(queue) | |
| // currentTrack + 1 upcoming = 2 tracks | |
| expect(result.restoredCount).toBe(2) | |
| expect(addTrack).toHaveBeenCalledTimes(2) | |
| expect(search).toHaveBeenNthCalledWith( | |
| 1, | |
| 'https://example.com/current', | |
| expect.any(Object), | |
| ) | |
| expect(search).toHaveBeenNthCalledWith( | |
| 2, | |
| 'https://example.com/next', | |
| expect.any(Object), | |
| ) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/utils/music/sessionSnapshots.spec.ts` around lines 130 -
185, The test must assert the restore order so currentTrack is prepended before
upcomingTracks: after calling MusicSessionSnapshotService.restoreSnapshot(queue)
verify the first addTrack/search call corresponds to snapshot.currentTrack
(e.g., compare the first call's query/url to snapshot.currentTrack.url or title)
and the second call corresponds to the upcomingTracks entry; use the jest mock
call history on addTrack or on player.search (e.g., addTrack.mock.calls[0],
addTrack.mock.calls[1] or player.search.mock.calls) to assert the exact sequence
rather than only checking restoredCount or call count.
| async deleteSnapshot(guildId: string): Promise<void> { | ||
| try { | ||
| await redisClient.del(this.getKey(guildId)) | ||
| } catch (error) { | ||
| errorLog({ | ||
| message: 'Failed to delete music session snapshot', | ||
| error, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Snapshot deletion is still not an authoritative restore claim.
This path is still GET → restore → best-effort DEL. Overlapping restore attempts can both load the same snapshot before either delete runs, and deleteSnapshot() swallows DEL failures, so the payload can still be replayed. Also, Line 224 deletes the snapshot before Line 227 proves play() succeeded, which can drop the only recovery copy on a restart-path failure.
Also applies to: 173-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/utils/music/sessionSnapshots.ts` around lines 152 - 160, The
deleteSnapshot path currently does a non-authoritative DEL and swallows
failures, and deletion happens before verifying play() succeeded; fix by making
restore-and-delete atomic and only removing the snapshot after successful
playback: replace the separate GET → restore → DEL flow with an atomic
fetch-and-delete (use Redis GETDEL or a small EVAL/Lua script via
redisClient.getDel/getdel or eval) in the restore code so only one consumer can
load the snapshot, and move any explicit deleteSnapshot(guildId) call to run
only after play() returns success; also stop silently swallowing DEL failures in
deleteSnapshot — surface/log and retry/raise so deletion errors are not ignored.
Ensure you update references to deleteSnapshot, getKey, and the code that calls
play() to follow this new atomic restore-then-confirm-delete flow.
Bumps version to 2.6.17 and promotes CHANGELOG entries for PRs #253-#256:\n- fix(deploy): restore executable bit on deploy.sh\n- feat(bot): harden queue snapshot restore with staleness guard\n- feat(bot): enforce provider health cooldown ordering in search fallback\n- fix(deploy): recover webhook rollouts from checkout drift
…ble-restore prevention (#256) - Add staleness guard to restoreSnapshot() (default 30min maxAge) to skip snapshots that are too old to be meaningful - Prepend currentTrack to restore list so the track playing at disconnect time is also recovered, not only upcoming tracks - Delete snapshot from Redis after a successful restore to prevent the same session being re-applied on the next connection event - Wire MusicSessionSnapshotService constructor to ENVIRONMENT_CONFIG.SESSIONS .QUEUE_SESSION_TTL so the env var actually controls the singleton TTL - Use ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED in lifecycleHandlers instead of raw process.env for consistent config sourcing - Expand test coverage: staleness rejection, currentTrack restore, snapshot cleared on success, no-delete when no tracks restored, early return on non-empty queue Closes #243
Bumps version to 2.6.17 and promotes CHANGELOG entries for PRs #253-#256:\n- fix(deploy): restore executable bit on deploy.sh\n- feat(bot): harden queue snapshot restore with staleness guard\n- feat(bot): enforce provider health cooldown ordering in search fallback\n- fix(deploy): recover webhook rollouts from checkout drift
This branch was successfully deployed
1 participant
Summary
Closes #243 (wave1 slice 4 — queue snapshot restore guardrails).
Addresses 5 gaps in the existing
MusicSessionSnapshotService:Gap 2):restoreSnapshot()now enforces a configurablemaxAgeMsthreshold (default 30 min). Snapshots older than the threshold are silently skipped rather than replaying stale state.Gap 3): The snapshot is deleted from Redis after a successful restore via the newdeleteSnapshot()method. A second crash before a newsaveSnapshot()call will no longer re-apply the same stale session.Gap 4): The track that was actively playing at disconnect time is now prepended to the restore list so it is re-enqueued first, not dropped.Gap 6):lifecycleHandlers.tsnow readsENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLEDinstead of rawprocess.env.MUSIC_SESSION_RESTORE_ENABLED.Gap 7):MusicSessionSnapshotServiceconstructor now defaults toENVIRONMENT_CONFIG.SESSIONS.QUEUE_SESSION_TTLsoQUEUE_SESSION_TTLenv var actually applies at the singleton level.Test coverage added
6 new tests in
sessionSnapshots.spec.ts:currentTrackprepended and restored (Gap 4)maxAgeMsguard (Gap 2)All 59 bot test suites pass (347 tests).
Summary by CodeRabbit
New Features
Bug Fixes