Repository navigation
fix(bot): unlink dead Last.fm sessions on error 9 and notify once - #1946
Conversation
The player scrobble path only warn-logged invalid-session failures, so a dead lastfm_links row produced 2 failing API calls per track forever (~2.5k error lines/60d in prod for one dead key). A shared deadSessionHandler now serves both scrobble paths: - DB-row dead key: unlink + one info log + one guarded DM (in-memory set; unlink's P2025-true is not an idempotency signal) - env-fallback dead key (no row): distinct config warning, never unlink, never DM - detection via isLastFmInvalidSessionError instead of substring 403 - player path now passes allowEnvFallback: false, matching the external scrobbler, so unlinked requesters no longer scrobble to the env account ADR: decisions/2026-08-03-lastfm-dead-session-handling.md
📝 WalkthroughWalkthroughThe PR centralizes invalid Last.fm session handling. It conditionally unlinks matching database sessions, sends deduplicated relinking DMs, protects environment fallback keys, restricts player fallback, and routes player and external scrobbling errors through the shared handler. ChangesLast.fm dead-session handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerOrExternalScrobbler
participant handleDeadLastFmSession
participant LastFmLinkService
participant DiscordClient
PlayerOrExternalScrobbler->>handleDeadLastFmSession: report invalid Last.fm session
handleDeadLastFmSession->>LastFmLinkService: unlinkIfKeyMatches with Discord ID and failed session key
LastFmLinkService-->>handleDeadLastFmSession: matching deletion result
handleDeadLastFmSession->>DiscordClient: send at most one relinking DM
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
Failed to generate code suggestions for PR |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR extracts Last.fm dead-session handling (invalid session key / error 9) into a new shared deadSessionHandler module and routes both the externalScrobbler and trackNowPlaying (main player) scrobble paths through it. Per the accompanying ADR, the intended behavior is to unlink dead lastfm_links rows, send a one-time DM prompting the user to relink (guarded by an in-memory set), skip unlink/DM when the failing key is the env key (logging a config warning instead), and disable env-fallback in the player path. The surface area covers the new ADR document, the new handler and its tests, and refactors to externalScrobbler.ts (removing its inline unlink logic in favor of the shared handler) and trackNowPlaying.ts, along with updates to the affected spec files and their mocks.
No blocking issues surfaced. 5 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 89 functions depend on the 70 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 89 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 84 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bot/src/lastfm/deadSessionHandler.spec.ts (1)
49-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the DM-dedup test with a concurrent invocation.
This test calls
handleDeadLastFmSessiontwice sequentially withawait. The scenario this guard protects against, per the docstring indeadSessionHandler.ts, isupdateNowPlayingandscrobbleracing for one track — that is, two in-flight calls before either resolves. Add aPromise.allvariant to directly exercise that interleaving.♻️ Proposed additional test
it('unlinks, logs, and DMs once when the dead key is a DB row', async () => { getByDiscordIdMock.mockResolvedValue({ discordId: 'user-1' }) const { client, sendMock } = makeClient() await handleDeadLastFmSession('user-1', client, 'scrobble') await handleDeadLastFmSession('user-1', client, 'updateNowPlaying') expect(unlinkMock).toHaveBeenCalledTimes(2) expect(infoLogMock).toHaveBeenCalledWith( expect.objectContaining({ message: 'Removed invalid Last.fm session', data: expect.objectContaining({ discordId: 'user-1' }), }), ) expect(sendMock).toHaveBeenCalledTimes(1) }) + + it('DMs once when scrobble and updateNowPlaying race concurrently', async () => { + getByDiscordIdMock.mockResolvedValue({ discordId: 'user-1' }) + const { client, sendMock } = makeClient() + + await Promise.all([ + handleDeadLastFmSession('user-1', client, 'scrobble'), + handleDeadLastFmSession('user-1', client, 'updateNowPlaying'), + ]) + + expect(sendMock).toHaveBeenCalledTimes(1) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/lastfm/deadSessionHandler.spec.ts` around lines 49 - 64, Update the test “unlinks, logs, and DMs once when the dead key is a DB row” to invoke both handleDeadLastFmSession calls concurrently via Promise.all instead of awaiting them sequentially, while preserving the existing assertions for unlinking, logging, and a single DM.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/handlers/player/trackNowPlaying.ts`:
- Around line 400-405: Update the session-key lookup in the track-now-playing
handler to preserve the environment Last.fm fallback when getLastFmRequesterId
returns undefined for autoplay or radio tracks. Allow getSessionKeyForUser to
use its default env-fallback behavior, while retaining the existing candidate
filtering that skips requesters without IDs.
---
Nitpick comments:
In `@packages/bot/src/lastfm/deadSessionHandler.spec.ts`:
- Around line 49-64: Update the test “unlinks, logs, and DMs once when the dead
key is a DB row” to invoke both handleDeadLastFmSession calls concurrently via
Promise.all instead of awaiting them sequentially, while preserving the existing
assertions for unlinking, logging, and a single DM.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b216344c-9e4c-4119-8da0-d9c9e745719a
📒 Files selected for processing (8)
decisions/2026-08-03-lastfm-dead-session-handling.mdpackages/bot/src/handlers/externalScrobbler.spec.tspackages/bot/src/handlers/externalScrobbler.tspackages/bot/src/handlers/player/trackNowPlaying.spec.tspackages/bot/src/handlers/player/trackNowPlaying.tspackages/bot/src/lastfm/deadSessionHandler.spec.tspackages/bot/src/lastfm/deadSessionHandler.tspackages/bot/src/lastfm/index.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- compare failed key with the stored row before unlinking — a stale error 9 must never delete a freshly relinked session - only log/DM on removed === true; unlink failure keeps the guard - DM guard keyed by session key, not user — a relinked-then-expired session still notifies - lookup failure bails with a warning instead of reading as no-link - player path env fallback is now requester-scoped: requester-less autoplay tracks keep env scrobbling; identified-but-unlinked requesters no longer misattribute to the env account
|
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces a shared "dead Last.fm session" handling path for scrobbling. It adds a new deadSessionHandler module in packages/bot/src/lastfm/ and an accompanying ADR documenting the decision to unlink dead sessions and send a single DM on error 9, with requester-scoped env-key fallback rules. It refactors externalScrobbler.ts and trackNowPlaying.ts to route invalid-session errors through the new handleDeadLastFmSession function instead of calling lastFmLinkService.unlink directly. The existing and new specs are updated/added to mock and assert against the shared handler rather than the raw unlink service. Surface area spans the two scrobbler handlers, the new dead-session handler and its tests, related test mocks, and a decisions/ADR markdown file.
Worth a look
- Non-atomic check-then-act on per-session DM guard Set allows double-DM under concurrency —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 94 functions depend on the 75 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 94 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 89 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- unlinkIfKeyMatches(discordId, sessionKey) via deleteMany: closes the check-then-act window — a relink landing mid-cleanup changes the key, so the delete no-ops and the fresh link survives - dm guard moved to a bounded LRU (max 500) so long-lived processes do not retain every expired session key forever - spec: lookup-failure test now asserts exactly one warning
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This pull request introduces a shared deadSessionHandler module in the bot package to centralize handling of dead/invalid Last.fm sessions (error 9), and routes both the externalScrobbler and trackNowPlaying scrobble paths through it. It also adjusts env-fallback behavior to be requester-scoped in the player path and adds an ADR documenting the rationale and alternatives considered. The changes touch the two scrobble handlers, their specs, the shared lastFmLinkService, and add new dead-session handler code plus tests. The spec updates replace direct unlink mocking with assertions against the new handleDeadLastFmSession entry point.
Worth a look
- Non-atomic check-then-act on notifiedSessionKeys Set allows double-DM under concurrency —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 102 functions depend on the 83 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 102 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 97 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
decisions/2026-08-03-lastfm-dead-session-handling.md (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winADR says
Set<string>, but the shipped guard is a boundedLRUCache.Line 32 describes the DM guard as an in-memory
Set<string>. The implementation indeadSessionHandler.tsusesnew LRUCache<string, true>({ max: 500 }), not a plainSet. Update the wording so the ADR matches the actual bounded-memory behavior, since this document is the reference for future readers evaluating memory growth of the guard.Proposed wording fix
-6. **DM guarded per session key** (in-memory `Set<string>`), not per user and not by the unlink result: updateNowPlaying/scrobble races can't double-DM, and a relinked-then-expired session (new key) still notifies. +6. **DM guarded per session key** (bounded in-memory LRU cache, max 500 entries), not per user and not by the unlink result: updateNowPlaying/scrobble races can't double-DM, and a relinked-then-expired session (new key) still notifies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@decisions/2026-08-03-lastfm-dead-session-handling.md` at line 32, Update the “DM guarded per session key” statement in the ADR to describe the shipped bounded LRUCache<string, true> guard with its max-500 capacity instead of an in-memory Set<string>. Preserve the existing per-session-key, race-prevention, and relinked-session behavior details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/lastfm/deadSessionHandler.ts`:
- Around line 81-91: Update the failure branch in deadSessionHandler around
unlinkIfKeyMatches so a false result is logged at a non-error level, avoiding
false-positive or duplicate error reporting because database failures are
already logged internally. Preserve the early return and update the existing
deadSessionHandler test that currently expects an error log.
---
Nitpick comments:
In `@decisions/2026-08-03-lastfm-dead-session-handling.md`:
- Line 32: Update the “DM guarded per session key” statement in the ADR to
describe the shipped bounded LRUCache<string, true> guard with its max-500
capacity instead of an in-memory Set<string>. Preserve the existing
per-session-key, race-prevention, and relinked-session behavior details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fcd595a-2aca-489f-a0be-a99f49b68fe8
📒 Files selected for processing (7)
decisions/2026-08-03-lastfm-dead-session-handling.mdpackages/bot/src/handlers/externalScrobbler.spec.tspackages/bot/src/handlers/externalScrobbler.tspackages/bot/src/handlers/player/trackNowPlaying.tspackages/bot/src/lastfm/deadSessionHandler.spec.tspackages/bot/src/lastfm/deadSessionHandler.tspackages/shared/src/services/LastFmLinkService/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bot/src/handlers/externalScrobbler.ts
- packages/bot/src/handlers/externalScrobbler.spec.ts
- packages/bot/src/handlers/player/trackNowPlaying.ts
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
New production advisories since the audit gate last ran green: - undici (5 GHSAs, fixed in 7.29.0) — override was pinned at 7.28.0 - fast-uri GHSA-7p8r-x3mc-p8w7 — via npm audit fix - ip-address (3 GHSAs, SSRF/trust-boundary) — via npm audit fix
github-actions
Bot
added
the
dependencies
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces a Last.fm dead-session handling feature for the bot. It adds a new Architecture Decision Record (decisions/2026-08-03-lastfm-dead-session-handling.md) documenting the rationale, and appears to add a shared dead-session handler module (packages/bot/src/lastfm/deadSessionHandler.ts) plus associated tests, with changes touching the trackNowPlaying player path and externalScrobbler handlers to route through it. The pull request also updates numerous package-lock.json dependency entries (reclassifying several from devOptional to dev, bumping versions like fast-uri and ip-address), and adjusts various package.json scripts, overrides, and devDependencies. The surface area spans documentation, bot Last.fm/scrobbler logic, and dependency/tooling configuration.
Worth a look
- In-memory DM guard Set has non-atomic check-then-add allowing double-DM race —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 271 functions depend on the 252 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 271 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 266 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
- unlinkIfKeyMatches returns 'removed' | 'stale' | 'error': a racing cleanup is an expected no-op, not a failed removal; the service logs real database failures itself - dm dedup recorded only after a successful send, so a transient dm failure no longer consumes the one notification for that session key - service-level tests for matching deletion, stale no-op, db rejection
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces a new architecture decision record (decisions/2026-08-03-lastfm-dead-session-handling.md) documenting the plan to add a shared "dead session handler" for Last.fm error-9 (invalid session key) cases across both scrobble paths, along with related decisions about unlink behavior, DM notifications, and env-fallback scoping. The changed symbols suggest accompanying implementation and test changes in the bot's externalScrobbler and trackNowPlaying handlers plus a new deadSessionHandler module and its specs. The diff also includes routine package-lock.json dependency metadata updates (e.g. dev/devOptional flag changes and minor version bumps) and package.json metadata edits.
Worth a look
- In-memory DM guard Set may not be concurrency-safe under async races —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 277 functions depend on the 258 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 277 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 272 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
concurrent handlers could double-send while the reservation was only recorded after the async fetch/send completed; a transient failure could also suppress the notification forever. reserve before the await, delete the reservation in the catch. spec now models the real removed-then-stale race.
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR adds a new architecture decision record (decisions/2026-08-03-lastfm-dead-session-handling.md) documenting a plan to handle Last.fm "dead session" errors by unlinking dead session keys and sending a one-time DM to affected users, with env-fallback and double-DM guards. Based on the changed symbols, it appears to introduce a shared deadSessionHandler module in packages/bot, wire it into the externalScrobbler and trackNowPlaying player paths, and add related helpers/tests (e.g., isLastFmInvalidSessionError, LastFmLinkService usage, and various spec mocks). The package-lock.json diff also reshuffles dependency metadata (several devOptional → dev flags and a few version bumps like fast-uri and ip-address). The reviewable surface spans the bot's scrobbling/Last.fm session logic, shared services, associated test files, and dependency lockfile changes.
No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 277 functions depend on the 258 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 277 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 272 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
cubic-dev-ai flagged that a transient DM-send failure after a successful unlink appears to permanently suppress the relink notification. Verified: the guard-release on failure is a defensive no-op under the current call graph, not a retry path, since the lastfm_links row is already deleted by that point and no later call for that session key can re-enter this function. No behavior change; corrects the ADR and code comment to state what's actually achievable instead of implying an active retry that can't fire.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/services/LastFmLinkService/index.ts (1)
125-152: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake lookup failures distinguishable from an absent link.
getByDiscordIdcatches database failures and returnsnullat Line 25-31. The supplied dead-session handler therefore cannot enter its lookup-errorcatch. IfenvFallbackUsedis true, a database outage enters the!rowbranch and can emit an invalid-environment-key warning.Propagate the lookup failure, or add a distinct lookup result for this handler. Preserve
nullonly for an actual missing row.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/services/LastFmLinkService/index.ts` around lines 125 - 152, Update getByDiscordId so database lookup failures propagate to the dead-session handler instead of being converted to null; reserve null exclusively for an absent link. Ensure the handler’s existing lookup-error catch runs on database outages, preventing the !row/envFallbackUsed warning path from treating failures as missing rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/shared/src/services/LastFmLinkService/index.ts`:
- Around line 125-152: Update getByDiscordId so database lookup failures
propagate to the dead-session handler instead of being converted to null;
reserve null exclusively for an absent link. Ensure the handler’s existing
lookup-error catch runs on database outages, preventing the !row/envFallbackUsed
warning path from treating failures as missing rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2260c2b2-aaba-4e7a-a341-84be069fd1aa
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
decisions/2026-08-03-lastfm-dead-session-handling.mdpackage.jsonpackages/bot/src/lastfm/deadSessionHandler.spec.tspackages/bot/src/lastfm/deadSessionHandler.tspackages/shared/src/services/LastFmLinkService/index.spec.tspackages/shared/src/services/LastFmLinkService/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bot/src/lastfm/deadSessionHandler.spec.ts
- decisions/2026-08-03-lastfm-dead-session-handling.md
- packages/bot/src/lastfm/deadSessionHandler.ts
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Auto-approved: Focused bug fix: unlinks dead Last.fm sessions on error 9 with atomic conditional delete, DM once per key, and env-fallback scrobble scoping. All behavior changes are visible in the diff and confined to the described incorrect behavior.
Re-trigger cubic
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This pull request adds a new architecture decision record (decisions/2026-08-03-lastfm-dead-session-handling.md) documenting an approach to handling dead Last.fm sessions, and appears to introduce a shared dead-session handler used by the bot's scrobble paths (external scrobbler and player now-playing). The symbol list also references related handler logic, session-key lookup/unlink behavior, DM notification guarding, and associated test mocks/specs, along with various package-lock.json dependency metadata changes (e.g. dev/devOptional reclassifications and version bumps). Reviewers should focus on the new/changed handler code, its call sites in the scrobble flows, and the accompanying tests.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 280 functions depend on the 258 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 280 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 274 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
…on-handling # Conflicts: # package-lock.json # package.json # packages/bot/src/lastfm/index.ts
github-actions
Bot
removed
the
dependencies
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Graphify review — findings
This PR introduces centralized handling for dead Last.fm sessions (invalid session key / error 9). It adds a new shared deadSessionHandler in the bot's lastfm module and routes both the externalScrobbler and trackNowPlaying scrobble paths through it, replacing the previous inline unlink logic. It also adds an ADR documenting the decision, updates the lastFmLinkService (including new unlinkIfKeyMatches/getSessionKey surface area), and rewrites the associated spec files to mock and assert against the new handler. The reviewable surface spans the bot handlers (externalScrobbler, trackNowPlaying and its state helpers), the new deadSessionHandler and its tests, lastfm/index exports, and the shared lastFmLinkService implementation plus specs.
Worth a look
- In-memory per-session DM guard Set is unguarded shared state under concurrent scrobble/nowPlaying paths —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- lastFmLinkService now exposes unlinkIfKeyMatches / getSessionKey with allowEnvFallback default true —
packages/shared/src/services/lastfmLinkService/index.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 111 functions depend on the 89 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 111 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 105 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
This pull request adds a shared Last.fm "dead session" handler (packages/bot/src/lastfm/deadSessionHandler.ts) and routes both scrobble paths—externalScrobbler.ts and the player path in trackNowPlaying.ts—through it instead of handling invalid-session errors inline. It also introduces a getByDiscordId/session-key lookup on lastFmLinkService, adjusts the env-fallback behavior in the player path to be requester-scoped, and updates the associated unit tests to assert calls into the new handler rather than direct unlink calls. An accompanying ADR (decisions/2026-08-03-...) documents the rationale, alternatives, and revisit conditions. The surface area spans the two handler files and their spec files, the new dead-session handler and its spec, the shared lastFmLinkService and its spec, and the new decision record.
Worth a look
- Non-atomic check-then-act on per-session DM guard Set allows double-DM under concurrency —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 111 functions depend on the 89 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 111 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 105 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
This PR introduces a shared "dead Last.fm session" handler and routes both scrobble paths (externalScrobbler and the player's trackNowPlaying) through it, replacing the previous inline unlink logic in externalScrobbler. It also adds an ADR documenting the decision (unlink dead sessions on error 9, send a best-effort DM, guard notifications per session key, and scope env-key fallback to requester-less tracks) and a new unlinkIfKeyMatches/getByDiscordId surface on lastFmLinkService. The changes span the two handlers, the new deadSessionHandler, the shared link service, and their accompanying spec files. The surface area is: bot handlers (external scrobbler, track-now-playing), a new bot lastfm/deadSessionHandler module, the shared lastFmLinkService, associated test mocks/specs, and one new decision document.
Worth a look
- Non-atomic check-then-act in per-session DM guard allows double-DM under concurrent scrobble paths —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 111 functions depend on the 89 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 111 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 105 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
This PR introduces a shared Last.fm "dead session" handler (packages/bot/src/lastfm/deadSessionHandler.ts) and routes both scrobble paths—externalScrobbler and the main player path (trackNowPlaying)—through it when an invalid-session (error 9) is detected, replacing the previous direct lastFmLinkService.unlink call in externalScrobbler. It also adds a unlinkIfKeyMatches method to the shared lastFmLinkService and adjusts env-fallback behavior in the player path to be requester-scoped. An accompanying ADR document (decisions/2026-08-03-lastfm-dead-session-handling.md) records the rationale, and the associated spec files are updated to mock/assert against the new handler instead of the old unlink flow. Surface area: bot handlers (externalScrobbler, trackNowPlaying), a new bot lastfm module, shared lastFmLinkService, plus a new decision doc and multiple test files.
Worth a look
- Non-atomic check-then-act on per-session DM guard Set allows double-DM under concurrent scrobble+nowPlaying —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 111 functions depend on the 89 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 111 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 105 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Graphify review — findings
This PR adds a Last.fm "dead session" handling path. It introduces a shared deadSessionHandler module in the bot package that both the external scrobbler and the trackNowPlaying player path route to when a Last.fm invalid-session (error 9) is detected, replacing the previous inline unlink logic in externalScrobbler. It also touches the shared lastFmLinkService (adding key-match-aware unlink helpers), the player now-playing handler, related test mocks/specs, and includes a new ADR documenting the decision. The surface area covers the bot's two scrobbling paths, the shared link service, their spec files, and a decision record; changes center on where and how invalid-session errors trigger unlinking and DM notification.
Worth a look
- envFallbackUsed hardcoded false in player path where env fallback is requester-scoped —
packages/bot/src/handlers/player/trackNowPlaying.ts· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- In-memory DM guard Set is non-atomic check-then-act, allowing double-DM under concurrency —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 111 functions depend on the 89 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 111 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 105 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Graphify review — findings
This pull request adds a shared deadSessionHandler module for Last.fm dead-session ("error 9" / invalid session key) handling and routes both the externalScrobbler and trackNowPlaying scrobble paths through it, replacing the previous direct lastFmLinkService.unlink call in externalScrobbler. The handler introduces logic around checking existing links, distinguishing env-fallback keys, guarding DM notifications per session key, and scoping env fallback to requester-less tracks. It also adds a new unlinkIfKeyMatches method area in the shared lastFmLinkService, updates the associated specs, and documents the reasoning in a new ADR (decisions/2026-08-03-lastfm-dead-session-handling.md). Surface area touched: bot scrobbler/player handlers, the shared Last.fm link service, related test files, and decision documentation.
Worth a look
- envFallbackUsed hardcoded false in player path breaks env-key detection —
packages/bot/src/handlers/player/trackNowPlaying.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Non-atomic check-then-act on notifiedSessionKeys Set allows double-DM under concurrent scrobble/nowPlaying —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 111 functions depend on the 89 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 111 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 105 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
|
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
This pull request adds a shared deadSessionHandler module in the bot package that centralizes handling of expired/invalid Last.fm session keys ("error 9"), and routes both the external scrobbler path and the trackNowPlaying player path through it instead of handling unlinking inline. It introduces a new unlinkIfKeyMatches method on the shared lastFmLinkService and changes the player path's env-fallback behavior to be requester-scoped. An ADR documenting the decision and its considered alternatives is also added, and the affected test suites are updated to mock and assert against the new handler. The touched surface spans packages/bot/src/lastfm/deadSessionHandler.ts, packages/bot/src/handlers/externalScrobbler.ts, packages/bot/src/handlers/player/trackNowPlaying.ts, packages/shared/src/services/lastFmLinkService, their spec files, and a new decisions document.
Worth a look
- Non-atomic check-then-act on notifiedSessionKeys guard allows double-DM under concurrent scrobble/nowplaying —
packages/bot/src/lastfm/deadSessionHandler.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 111 functions depend on the 89 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
scrobbleCurrentTrackIfLastFm()— 3 callers, 8 callees - new:
handleDeadLastFmSession()— 6 callers, 3 callees - worse:
updateLastFmNowPlaying()— 2 callers, 7 callees
Verification — 111 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 105 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
🤖 I have created a release *beep* *boop* --- <details><summary>2.39.3</summary> ## [2.39.3](v2.39.2...v2.39.3) (2026-08-12) ### Bug Fixes * **bot:** expose degraded-extractor signal and honest play error ([#1999](#1999)) ([8348bd6](8348bd6)) * **bot:** unlink dead Last.fm sessions on error 9 and notify once ([#1946](#1946)) ([d22b8bd](d22b8bd)) * **ci:** add a second retry for docker-build ([#2007](#2007)) ([9347c55](9347c55)) * **ci:** extract node_modules before per-workspace build steps run ([#2006](#2006)) ([ae577d1](ae577d1)) * **ci:** free disk space before Docker builds to prevent BuildKit GC race ([#2003](#2003)) ([5bcd388](5bcd388)) * **ci:** isolate docker cache scope by branch ([#2012](#2012)) ([5d25ec8](5d25ec8)) * **ci:** pin node:24-alpine base image by digest ([#2004](#2004)) ([f154bfe](f154bfe)) * **ci:** skip gha cache-from on docker-build retries ([#2008](#2008)) ([89dbb90](89dbb90)) * **ci:** split deps-production per production target ([#2005](#2005)) ([43aacce](43aacce)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
1 participant
Why
Prod: 2,501 Last.fm "403 Invalid session key (error 9)" lines in 60d (~42/day), surfaced during the Lavalink re-evaluation. Root cause: the player scrobble path () only warn-logged the failure — the dead row stayed, so every track produced 2 failing API calls forever. (The external scrobbler already unlinked correctly.)
What
New shared () used by both scrobble paths, per the critic-reviewed ADR ():
Tests
Summary by cubic
Unlinks invalid Last.fm sessions (error 9) and sends one relink DM, stopping repeated 403s and preventing mis-attributed scrobbles. Uses a shared dead-session handler across the player and external scrobbler.
Bug Fixes
handleDeadLastFmSession: atomicunlinkIfKeyMatches(discordId, sessionKey)with tri-state'removed' | 'stale' | 'error'; compare failed key to stored key to protect fresh relinks; log + DM only on'removed'.isLastFmInvalidSessionError;externalScrobblernow routes invalid-session errors through the shared handler.LASTFM_SESSION_KEY; never unlink or DM; DB lookup failures warn and skip. Player path fallback is requester-scoped (allowEnvFallback: requesterId === undefined) to prevent env-account misattribution.Dependencies
undicito7.29.0.fast-uriandip-address.Written for commit 391db89. Summary will update on new commits.
Summary by CodeRabbit