Repository navigation
feat(bot): add Spotify API 429 retry with Retry-After header - #808
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds centralized HTTP 429 rate-limit retry handling to five Spotify API functions. A new ChangesSpotify API 429 Retry Implementation
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 review |
✅ Actions performedReview triggered.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The for loop always returns or throws on every path, making the post-loop throw dead code. Removing it improves SonarCloud coverage metrics and eliminates the unreachable branch warning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
… correctness The for-loop exit was unreachable but TypeScript required a throw/return after it (TS2366). Using while(true) makes TypeScript's flow analysis see that the loop never exits normally, satisfying the return-type constraint without adding dead code that SonarCloud would flag. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…stPopularity, and getArtistGenres Covers new withSpotifyRetry wrapper paths in all three functions to push new code coverage above the 80% SonarCloud quality gate threshold. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
The retry refactor renamed the debug-log field from 'retryAfter' to 'retryAfterHeader' (storing the raw header string for diagnostics rather than the parsed seconds value). Update the legacy spec assertion to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![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/spotify/spotifyApi.ts (1)
77-90: 💤 Low valueOptional: add a small random jitter to reduce thundering-herd on retries.
When multiple Spotify calls fan out in parallel (e.g., several
getArtistPopularitylookups during a single recommendation flow) and all receive 429 with the sameRetry-After, they will wake up and retry at the exact same instant. A small jitter (e.g., ±10–20%) on top of the parsed delay reduces the chance of immediately re-tripping the rate limit. Capped at 2 retries the blast radius is small, so this is purely a polish item.♻️ Example jitter
- const delayMs = Math.min( - parsedDelayMs ?? DEFAULT_RETRY_AFTER_MS, - MAX_RETRY_AFTER_MS, - ) + const baseDelayMs = Math.min( + parsedDelayMs ?? DEFAULT_RETRY_AFTER_MS, + MAX_RETRY_AFTER_MS, + ) + // ±20% jitter to avoid synchronized retries across concurrent calls + const jitter = baseDelayMs * (Math.random() * 0.4 - 0.2) + const delayMs = Math.max(0, Math.round(baseDelayMs + jitter))🤖 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/spotify/spotifyApi.ts` around lines 77 - 90, The retry delay calculation should add a small random jitter to avoid thundering-herd; when computing delayMs (currently derived from parsedDelayMs, DEFAULT_RETRY_AFTER_MS, MAX_RETRY_AFTER_MS) add a random multiplier (e.g., 0.9–1.2) to the chosen delay, clamp the result back to MAX_RETRY_AFTER_MS, and use that jittered value in the debugLog and the await sleep call; ensure the jitter logic is deterministic enough for tests (seed or injectable RNG if needed) and keep references to parsedDelayMs, DEFAULT_RETRY_AFTER_MS, MAX_RETRY_AFTER_MS, debugLog, attempt, and sleep when implementing the change.
🤖 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/spotify/spotifyApi.ts`:
- Around line 63-105: Wrap the direct fetch calls in searchSpotifyTrack,
getUserTopArtistsAndTracks, and getUserSavedTracks with the existing
withSpotifyRetry helper so 429s are retried; for each wrapped call, call
throwIfRetryable(res) immediately after the fetch and before any existing if
(!res.ok) early returns to preserve the established error flow; for
getUserSavedTracks, move the withSpotifyRetry wrapper inside the pagination loop
so each page fetch is retried individually (i.e., wrap the per-page fetch call
inside the while (offset < maxTracks) body), and apply the same
throwIfRetryable(res) check per-page.
---
Nitpick comments:
In `@packages/bot/src/spotify/spotifyApi.ts`:
- Around line 77-90: The retry delay calculation should add a small random
jitter to avoid thundering-herd; when computing delayMs (currently derived from
parsedDelayMs, DEFAULT_RETRY_AFTER_MS, MAX_RETRY_AFTER_MS) add a random
multiplier (e.g., 0.9–1.2) to the chosen delay, clamp the result back to
MAX_RETRY_AFTER_MS, and use that jittered value in the debugLog and the await
sleep call; ensure the jitter logic is deterministic enough for tests (seed or
injectable RNG if needed) and keep references to parsedDelayMs,
DEFAULT_RETRY_AFTER_MS, MAX_RETRY_AFTER_MS, debugLog, attempt, and sleep when
implementing the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dfa71fb-648c-4dcf-b2d6-8d26bfd1879d
📒 Files selected for processing (2)
packages/bot/src/spotify/spotifyApi.tspackages/bot/src/spotify/spotifyApiRetry.spec.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 (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use theisPrisma*Error()helper functions to check for specific Prisma error types (e.g.,isPrismaForeignKeyError,isPrismaUniqueConstraintError) instead of manually checking error codes
Use Prisma's$transaction()method to ensure database operations are atomic and avoid partial updates when multiple related tables are modified
Always useselectorincludein Prisma queries to explicitly specify which fields to return, avoiding unnecessary data transfer
For Redis operations, use connection pooling and implement exponential backoff retry logic for transient failures
Always uselogAndRethrow()orlogAndSwallow()utilities when handling errors to ensure errors are logged with context before propagating or suppressing
Files:
packages/bot/src/spotify/spotifyApiRetry.spec.tspackages/bot/src/spotify/spotifyApi.ts
packages/{bot,backend,shared}/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/{bot,backend,shared}/src/**/*.ts: For feature toggles, check both global and guild-specific toggles usingFeatureToggleService.isEnabledForGuild()rather than checking them separately
Use branded types (e.g.,GuildId,UserId,ChannelId) for Discord IDs throughout the codebase to prevent type-level ID confusion
Files:
packages/bot/src/spotify/spotifyApiRetry.spec.tspackages/bot/src/spotify/spotifyApi.ts
packages/bot/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building Discord embeds, use
EmbedBuilderService.createTemplate()orEmbedBuilderService.getTemplate()instead of constructing embeds directly
Files:
packages/bot/src/spotify/spotifyApiRetry.spec.tspackages/bot/src/spotify/spotifyApi.ts
packages/bot/src/{spotify,utils/music}/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Extract Spotify track IDs using
extractSpotifyTrackId()before passing to Spotify API calls to prevent malformed requests
Files:
packages/bot/src/spotify/spotifyApiRetry.spec.tspackages/bot/src/spotify/spotifyApi.ts
🔇 Additional comments (3)
packages/bot/src/spotify/spotifyApi.ts (1)
17-61: LGTM — clean separation of helpers.
parseRetryAfterMscorrectly distinguishes delta-seconds vs HTTP-date and returnsnull(notNaN) for unparseable inputs, so the caller's?? DEFAULT_RETRY_AFTER_MSfallback works as intended. The clamp to0for past HTTP-dates is the right call.throwIfRetryabledocuments the rationale (fetch()doesn't throw on HTTP errors) which is exactly the issue the earlier review caught.packages/bot/src/spotify/spotifyApiRetry.spec.ts (2)
8-20: LGTM — solid setup with proper isolation between tests.
beforeEachresetsfetchMockand clears mock state, and the regression block at the bottom (lines 287-373) properly exercises the production-realistic case wherefetchresolves with a 429Responserather than throwing. Together with the throw-based tests above, both thethrowIfRetryablepath and thewithSpotifyRetrycatch handler are well covered.
284-373: LGTM — these regression tests are exactly what was missing.The fake-timer-based tests properly exercise the production code path (
fetchresolving with a 429Response,throwIfRetryablethrowing,withSpotifyRetrycatching, parsingRetry-After, sleeping, retrying). The HTTP-date test correctly pins system time so the parsed delta is deterministic, which would have caught the originalparseIntNaN bug. Good safety net.
Per Greptile feedback (PR #808): three Spotify endpoints still bypassed the 429 retry path because they called fetch directly. Wrap them in withSpotifyRetry + throwIfRetryable for consistency: - searchSpotifyTrack - getUserTopArtistsAndTracks (parallel artists + tracks fetches) - getUserSavedTracks (per-page wrap inside the pagination loop — particularly exposed since it can issue up to 4 sequential requests) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
* feat(bot): add Spotify 429 retry with Retry-After header * test(spotify): add non-429 and Retry-After header coverage * fix(spotify): remove unreachable throw after withSpotifyRetry loop The for loop always returns or throws on every path, making the post-loop throw dead code. Removing it improves SonarCloud coverage metrics and eliminates the unreachable branch warning. * refactor(spotify): use while(true) in withSpotifyRetry for TypeScript correctness The for-loop exit was unreachable but TypeScript required a throw/return after it (TS2366). Using while(true) makes TypeScript's flow analysis see that the loop never exits normally, satisfying the return-type constraint without adding dead code that SonarCloud would flag. * test(spotify): add 429 retry tests for getBatchAudioFeatures, getArtistPopularity, and getArtistGenres Covers new withSpotifyRetry wrapper paths in all three functions to push new code coverage above the 80% SonarCloud quality gate threshold. * fix(spotify): retry actually fires on 429 + handle Retry-After HTTP-date The previous retry implementation never triggered in production because fetch() does NOT throw on HTTP error statuses — it resolves with a Response where ok=false. Each wrapped callback's 'if (!res.ok) return null' short-circuited before withSpotifyRetry's catch could see anything, so 429 responses silently became null/[]. Fix: - Add throwIfRetryable(res) helper that throws the Response on 429. - Call it inside every wrapped callback BEFORE the !res.ok early return, so a resolved 429 becomes a thrown Response that the retry catch can intercept. - 5 sites covered: getSpotifyRecommendations, getAudioFeatures, getBatchAudioFeatures, getArtistPopularity, getArtistGenres. Also fix Retry-After parsing (Greptile concern 2): - parseInt of an HTTP-date returns NaN, which becomes NaN ms sleep (0ms in practice — busy-loop retry, not the requested back-off). - New parseRetryAfterMs helper handles both delta-seconds and RFC 7231 IMF-fixdate, returns null when unparseable so we fall back to a sane default (1s) instead of NaN. - Cap the delay at MAX_RETRY_AFTER_MS (60s) to prevent abuse. Tests: - New 'fetch resolves (not throws)' suite covering the realistic path. - Retry-After delta-seconds via fake timers. - Retry-After HTTP-date doesn't produce NaN. Addresses Greptile feedback on PR #808. * test(spotify): align Retry-After log assertion with new field name The retry refactor renamed the debug-log field from 'retryAfter' to 'retryAfterHeader' (storing the raw header string for diagnostics rather than the parsed seconds value). Update the legacy spec assertion to match. * fix(spotify): apply withSpotifyRetry to remaining fetch sites Per Greptile feedback (PR #808): three Spotify endpoints still bypassed the 429 retry path because they called fetch directly. Wrap them in withSpotifyRetry + throwIfRetryable for consistency: - searchSpotifyTrack - getUserTopArtistsAndTracks (parallel artists + tracks fetches) - getUserSavedTracks (per-page wrap inside the pagination loop — particularly exposed since it can issue up to 4 sequential requests) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Promote [Unreleased] entries into the v2.10.0 section, bump root + workspace versions from 2.9.0 to 2.10.0, and update the lockfile. Release highlights: - Spotify 429 retry hardening (#808) - Last.fm canonical metadata + multi-artist scrobble fix (#821) - Autoplay Spanish-gospel-block + sertanejo prioritization series (#817-#820, #827, #829, #830) - Review-tools revamp: Claude review + Danger + chilled CodeRabbit via org-level reusable workflows (#838) - Coverage threshold pinned for phase-2 test cleanup (#835) - CI extended to release/** branches (#816)
…2074) Closes #2072. Replaces #2073, which GitHub auto-closed when #2071 merged with `--delete-branch` and took its base branch with it. Same content, rebuilt on current `main`. ## Measured, with Lucky own credentials from inside lucky-bot ``` v1/search / artists/{id} / albums/{id} / tracks/{id} HTTP 200 v1/audio-features HTTP 403 ``` So `currentFeatures` was always `null` and `enrichWithAudioFeatures` returned its input unchanged on the first line. The 5000-entry LRU cache behind it, with a 24h TTL, could only ever hold `null`. ## The Last.fm replacement already exists The plan was to substitute the Spotify signal with Last.fm tags. That work was already done: `candidateScorer` scores genre-family overlap in-pass via `calculateGenreFamilyPenalty` on Last.fm tags, and the code says so at the old call site: > Genre-family scoring moved to in-pass `calculateRecommendationScore` via Last.fm tags so it works without a Spotify token. This pass is now limited to audio-feature (energy/valence) deltas, which the Last.fm-tag path cant substitute for. What remained is energy, valence, tempo and acousticness. Last.fm does not expose those, so there is nothing to substitute with. ## Extended access does not bring them back Ruled out with sources on #2072. Since 2025-05-15 it requires a registered business entity and 250k MAUs, and the deprecation notice says extended mode **preserved** these endpoints for apps that already had it on 2024-11-27. It is not a door that opens later. ## Removed `enrichWithAudioFeatures` existed **twice**, in `candidateScorer.ts` and in `candidateFallback.ts`. Both are gone, along with `audioFeatures.ts`, `getAudioFeatures`, `getBatchAudioFeatures`, the `SpotifyAudioFeatures` / `SpotifyAudioFeatureConstraints` types, and the `currentFeatures` parameter threaded through the collector chain. ``` 15 files changed, 29 insertions(+), 1164 deletions(-) ``` ## Tests retargeted, not deleted Two regression tests used the removed fetchers only as a **vehicle** to exercise `spotifyFetch` shared `AbortSignal` (#1279) and its 429 retry (PR #808 / Greptile). Those behaviours still matter, so both now run against `searchSpotifyTrack` and carry a comment saying why. ## Verification ``` jest 3122 passed, 1 skipped, 243 suites tsc clean ``` <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Removes Spotify audio-features–based scoring because the endpoint now returns 403. Previously we boosted/penalized candidates using energy/valence/tempo/acousticness; in practice this path was a no-op (always null), so behavior does not change and scoring continues to rely on Last.fm tag–based genre-family checks. - Delete the enrichment path: remove `enrichWithAudioFeatures` (from candidate scorer and fallback) and `audioFeatures.ts`; drop `getAudioFeatures`, `getBatchAudioFeatures`, and the `SpotifyAudioFeatures`/`SpotifyAudioFeatureConstraints` types. - Simplify call sites: remove the `currentFeatures` parameter from candidate collection and replenisher; delete the related cache and the export in queue manipulation. - Keep infra tests: retarget AbortSignal deadline and 429-retry regressions to `searchSpotifyTrack`; update Retry-After tests to return a valid search payload and assert non-null results. Remove an unused `getArtistGenres` import flagged by CodeQL. - No rollout or user migration needed; compile-time errors will surface any stale imports. <sup>Written for commit 7bb2eff. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2074?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
This branch was successfully deployed
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Greptile Summary
withSpotifyRetry(up to 2 retries, honoursRetry-After) wrapping 5 of 7 Spotifyfetchcall sites;searchSpotifyTrackandgetUserTopArtistsAndTracksare still called directly without retry coverage.withSpotifyRetry'scatchis never reached; the tests pass only because they mockfetchto throw aResponse, which is not how the real API behaves.Retry-Afterheader parsing usesparseInton the raw header value; if Spotify sends an HTTP-date string (valid per RFC 7231) rather than a delta-seconds integer,parseIntreturnsNaN, makingdelayMs = NaNandsleep(NaN)collapses to a 0 ms delay.Confidence Score: 3/5
Not safe to merge — the core retry feature is dead code at runtime and the tests validate an unrealistic mock path.
A P1 defect (retry catch is unreachable because fetch resolves on 429) means the entire feature ships as non-functional code. The NaN delay on HTTP-date Retry-After is an additional P1 edge case. Multiple P1s on the central changed path push the score to 3/5.
packages/bot/src/spotify/spotifyApi.ts — every withSpotifyRetry callback must explicitly throw the response on 429 before the generic !res.ok guard; packages/bot/src/spotify/spotifyApiRetry.spec.ts — tests must be rewritten to mock fetch resolving with a 429 Response rather than throwing one.
Important Files Changed
withSpotifyRetrywrapper applied to 5 of 7 fetch call sites; retry catch block is unreachable in production becausefetch()resolves (not throws) on 429, andparseIntof an HTTP-dateRetry-Aftervalue producesNaNdelay.fetchto throw aResponseobject, which does not match real Fetch API behaviour — realfetchalways resolves for HTTP error responses, so the retry path is never exercised against production semantics.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Caller: e.g. getAudioFeatures] --> B[withSpotifyRetry wraps callback] B --> C[attempt = 0..maxRetries loop] C --> D[await fn - inner fetch callback] D --> E{fetch resolves} E -- "res.ok === false (incl. 429)" --> F[callback returns null/empty\nNO throw — retry catch NEVER reached] E -- "res.ok === true" --> G[callback returns parsed data] F --> H[withSpotifyRetry returns null/empty\nretry logic bypassed] G --> I[withSpotifyRetry returns data] D --> J{fetch throws — network error only} J -- "error.status === 429\ntest mock only" --> K{attempt < maxRetries?} K -- yes --> L[parse Retry-After header\ninteger OK, HTTP-date → NaN delay] L --> M[sleep delayMs] M --> C K -- no --> N[warnLog + rethrow] N --> O[outer catch: logAndSwallow] J -- "non-429 error" --> OComments Outside Diff (1)
packages/bot/src/spotify/spotifyApi.ts, line 435 (link)searchSpotifyTrackandgetUserTopArtistsAndTracksnot covered by retryBoth
searchSpotifyTrack(line ~435) andgetUserTopArtistsAndTracks(~475) still callfetchdirectly withoutwithSpotifyRetry. Any 429 from those endpoints won't be retried, which is inconsistent with the intent of this PR.Reviews (3): Last reviewed commit: "Merge branch 'release/v2.10.0' into feat..." | Re-trigger Greptile
Summary by CodeRabbit
Bug Fixes
Tests