Fix Garmin rate-limit retry collisions and reduce Connect API pressure - #1364
Merged
Merged
Conversation
Unify cooldown retries through enqueueSyncJob with shared dedup, slow Garmin pacing, cache OAuth consumer credentials, and skip scheduled sync when step-chain jobs are already queued. Co-authored-by: Cursor <cursoragent@cursor.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughGarmin Connect requests now bypass provider rate limiting for OAuth consumer fetches, cache consumer credentials across client instances, and use updated Garmin throttle, limiter, and fallback cooldown values. Sync scheduling now uses request deduplication, provider-scoped cooldown job IDs, and queued-job checks for step-chain providers. ChangesGarmin rate limits and sync scheduling
Sequence Diagram(s)Garmin consumer fetch flow sequenceDiagram
participant GarminProvider
participant createGarminConnectFetch
participant GarminConnectClient
participant baseFetchFn
participant rateLimitedFetchFn
GarminProvider->>createGarminConnectFetch: build authFetch and sync fetchFn
GarminConnectClient->>createGarminConnectFetch: first fromTokens request for oauth_consumer.json
createGarminConnectFetch->>baseFetchFn: bypass rate limit for oauth consumer host
GarminConnectClient->>GarminConnectClient: cache oauth consumer
GarminConnectClient->>createGarminConnectFetch: later Garmin API request
createGarminConnectFetch->>rateLimitedFetchFn: use Garmin rate-limited fetch
Queued sync skip flow sequenceDiagram
participant processScheduledSyncJob
participant listProviderSyncJobsForUser
participant getProviderSyncQueue
processScheduledSyncJob->>listProviderSyncJobsForUser: check queued sync jobs for user/provider
listProviderSyncJobsForUser->>getProviderSyncQueue: getActive()
listProviderSyncJobsForUser->>getProviderSyncQueue: getWaiting()
listProviderSyncJobsForUser->>getProviderSyncQueue: getDelayed(0, 49)
listProviderSyncJobsForUser-->>processScheduledSyncJob: filtered jobs
alt jobs already queued for a step-chain provider
processScheduledSyncJob-->>processScheduledSyncJob: skip enqueue and increment skippedDueToInFlight
else no queued jobs
processScheduledSyncJob->>getProviderSyncQueue: add sync job
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
Reviewer's GuideRoutes Garmin rate-limit retries through the shared sync enqueue/dedup path, reduces overall Garmin Connect API pressure (throttling, inferred budgets, queue limiter), and hardens scheduled sync behavior by extending Garmin cooldowns, preventing duplicate delayed jobs, and skipping fan-out when step-chain jobs are already in flight. Sequence diagram for scheduled sync skipping in-flight step-chain Garmin jobssequenceDiagram
participant Scheduler as ScheduledSyncJob
participant Process as processScheduledSyncJob
participant StepCheck as isStepChainSyncProvider
participant ListJobs as listProviderSyncJobsForUser
participant Enqueue as enqueueSyncJob
Scheduler->>Process: processScheduledSyncJob(job, db)
loop users/providers
Process->>StepCheck: isStepChainSyncProvider(providerId)
alt provider is step-chain
Process->>ListJobs: listProviderSyncJobsForUser(providerId, userId)
ListJobs-->>Process: pendingJobs
alt pendingJobs.length > 0
Process-->>Process: skippedDueToInFlight++
Process-->>Scheduler: logger.info Skipping providerId for userId
else no pending jobs
Process->>Enqueue: enqueueSyncJob(providerId, jobData)
Enqueue-->>Process: job
Process-->>Process: jobCount++
end
else provider not step-chain
Process->>Enqueue: enqueueSyncJob(providerId, jobData)
Enqueue-->>Process: job
Process-->>Process: jobCount++
end
end
Process-->>Scheduler: logger.info summary with skippedDueToInFlight
Sequence diagram for Garmin rate-limit delayed retry deduplicationsequenceDiagram
actor Worker as RateLimitedSyncJob
participant Schedule as scheduleDelayedSyncJob
participant Enqueue as enqueueSyncJob
participant Dedup as enqueueSyncJobWithRequestDedup
participant Queue as getProviderSyncQueue
participant JobStore as getJob
Worker->>Schedule: scheduleDelayedSyncJob(jobData, cooldown)
Schedule-->>Schedule: providerId = jobData.providerId ?? cooldown.providerId
Schedule->>Enqueue: enqueueSyncJob(providerId, jobData with providerId)
Enqueue->>Dedup: enqueueSyncJobWithRequestDedup(providerId, jobData, retryOptions, addJob, getJob)
alt nextOptions.jobId is null and requestQuery exists
Dedup-->>Dedup: buildSyncRequestJobId(...)
end
Dedup->>JobStore: getJob(nextOptions.jobId)
alt existing job found
JobStore-->>Dedup: existing
Dedup->>existing: getState()
alt state is delayed
Dedup-->>Enqueue: return existing
Enqueue-->>Schedule: existing
else other state
Dedup->>existing: remove()
Dedup->>Queue: add(name, jobData, nextOptions)
Queue-->>Dedup: newJob
Dedup-->>Enqueue: newJob
Enqueue-->>Schedule: newJob
end
else no existing job
JobStore-->>Dedup: null
Dedup->>Queue: add(name, jobData, nextOptions)
Queue-->>Dedup: newJob
Dedup-->>Enqueue: newJob
Enqueue-->>Schedule: newJob
end
Schedule-->>Worker: cooldown.expiresAt.toISOString()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
Co-authored-by: Cursor <cursoragent@cursor.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The module-level OAuth consumer cache in
garmin-connect/client.tsis global and keyed only by URL constant, so iffromTokensis ever used with differenthostvalues or differingfetchFnbehavior (e.g., proxy, region, or test doubles), they will all share the same cached consumer; consider scoping the cache by host or by the effective consumer URL to avoid subtle cross-environment leakage. - In
createGarminConnectFetch, theisGarminOAuthConsumerRequestcheck uses a simpleincludes(OAUTH_CONSUMER_HOST)on the stringified input; if other S3 URLs can contain that substring or if protocols/ports matter, you may want to parse asnew URL(...)and comparehostname(and optionallypathname) instead to reduce the chance of accidentally bypassing adaptive rate limiting for unrelated requests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The module-level OAuth consumer cache in `garmin-connect/client.ts` is global and keyed only by URL constant, so if `fromTokens` is ever used with different `host` values or differing `fetchFn` behavior (e.g., proxy, region, or test doubles), they will all share the same cached consumer; consider scoping the cache by host or by the effective consumer URL to avoid subtle cross-environment leakage.
- In `createGarminConnectFetch`, the `isGarminOAuthConsumerRequest` check uses a simple `includes(OAUTH_CONSUMER_HOST)` on the stringified input; if other S3 URLs can contain that substring or if protocols/ports matter, you may want to parse as `new URL(...)` and compare `hostname` (and optionally `pathname`) instead to reduce the chance of accidentally bypassing adaptive rate limiting for unrelated requests.
## Individual Comments
### Comment 1
<location path="src/providers/garmin/provider.ts" line_range="53-62" />
<code_context>
+const OAUTH_CONSUMER_HOST = "thegarth.s3.amazonaws.com";
+
+function isGarminOAuthConsumerRequest(input: RequestInfo | URL): boolean {
+ const url =
+ typeof input === "string"
+ ? input
+ : input instanceof URL
+ ? input.href
+ : input instanceof Request
+ ? input.url
+ : String(input);
+ return url.includes(OAUTH_CONSUMER_HOST);
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid substring matching for URL host detection to reduce false positives.
Using `url.includes(OAUTH_CONSUMER_HOST)` can misclassify requests when the host string appears in query params, paths, or inside another hostname. Prefer parsing the URL (e.g. `const { hostname } = new URL(url)`) and comparing `hostname` (and optionally `pathname`) to the expected values so routing only triggers for the intended host.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
- Fix listProviderSyncJobsForUser perf: use bounded typed getters instead of unbounded getJobs - Fix provider-scoped cooldown jobId: strip userId so all users share one retry jobId - Fix redundant cooldown re-check in scheduleDelayedSyncJob: call enqueueSyncJobWithRequestDedup directly - Remove unused PENDING_SYNC_JOB_STATES constant - Update all test mocks and assertions
Kills 34/37 mutants in Garmin provider rate-limit code. Covers: catch-block headers guards, null/get-method headers, OAuth consumer URL/Request bypass, init forwarding, invalid URL handling, plain-object resolveRequestUrl path, quota-exceeded Retry-After parsing.
![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
🤖 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/garmin-connect/src/client.ts`:
- Around line 51-55: The module-level test helper resetGarminConnectTestCaches
is being exported from production code, which should be removed. Keep the
cache-reset logic out of the public API in client.ts by deleting the export and
isolating tests through module isolation or a non-public cache design, while
preserving the behavior of cachedOAuthConsumer and oauthConsumerLoadPromise for
runtime use.
- Around line 230-235: The OAuth consumer response in the Garmin auth flow is
being cached without validation, so a malformed S3 payload can poison later
requests. In the OAuth consumer fetch logic in client.ts, parse the JSON with
the existing Zod schema before assigning to cachedOAuthConsumer, and only cache
the parsed result after it validates successfully. Keep the validation inside
the same fetch path that currently builds the OAuthConsumer value so invalid
data throws before caching.
In `@src/lib/sync-request-queue.ts`:
- Around line 11-17: The delayed-job deduplication in sync-request-queue’s job
lookup is bounded by queue.getDelayed(0, 49), which can miss duplicates beyond
the first 50 delayed entries. Update the job collection logic in the function
that gathers active, waiting, and delayed jobs to fetch all delayed jobs by
calling queue.getDelayed() with default arguments (or 0, -1), then keep
filtering by job.data.userId as before so duplicate sync jobs are always
detected.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: caaaf4a3-9098-40a5-936a-1f5c0e6fb2ab
📒 Files selected for processing (22)
cspell.jsonpackages/garmin-connect/src/client.test.tspackages/garmin-connect/src/client.tspackages/provider-http/src/adaptive-rate-limit.test.tspackages/provider-http/src/adaptive-rate-limit.tspackages/web/src/pages/ProviderDetailPage.test.tsxsrc/jobs/enqueue-sync-job.test.tssrc/jobs/enqueue-sync-job.tssrc/jobs/process-scheduled-sync-job.test.tssrc/jobs/process-scheduled-sync-job.tssrc/jobs/process-sync-job.test.tssrc/jobs/provider-queue-config.test.tssrc/jobs/provider-queue-config.tssrc/jobs/provider-rate-limit-cooldown.test.tssrc/jobs/provider-rate-limit-cooldown.tssrc/jobs/sync-request-job.test.tssrc/jobs/sync-request-job.tssrc/lib/sync-request-queue.test.tssrc/lib/sync-request-queue.tssrc/providers/garmin-connect-internal-sync.integration.test.tssrc/providers/garmin.test.tssrc/providers/garmin/provider.ts
…elayed-jobs query - Remove module-level OAuth consumer cache and resetGarminConnectTestCaches export; use instance-level caching with type guard + validation for S3 OAuth consumer JSON (fixes no-exports-for-testability violation) - Remove explicit getDelayed(0, 49) limit so delayed-job dedup works for users with >50 pending delayed jobs - Clean up imports and afterEach/beforeEach in tests
…min OAuth consumer validation tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
enqueueSyncJoband deduplicate any preset job ID (including rate-limit retries), so scheduled sync, step-chain continuations, and delayed retries share one enqueue/throttle path.Test plan
pnpm exec vitest run src/jobs/enqueue-sync-job.test.ts src/jobs/sync-request-job.test.ts src/jobs/process-sync-job.test.ts src/jobs/process-scheduled-sync-job.test.tspnpm exec vitest run packages/garmin-connect/src/client.test.ts src/providers/garmin.test.ts src/jobs/provider-rate-limit-cooldown.test.tsMade with Cursor
Summary by Sourcery
Optimize Garmin sync behavior to reduce Connect API load and avoid redundant or colliding retry/scheduled jobs.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by cubic
Fixes Garmin rate-limit retry collisions and reduces Connect API pressure. Retries no longer pile up, pacing is slower and shared, and the OAuth consumer fetch bypass is restricted to the S3 host.
enqueueSyncJobwith provider-scoped job IDs; reuse existing delayed cooldown jobs instead of enqueueing duplicates.active/waiting/delayed) to check pending work.thegarth.s3.amazonaws.com; cache the consumer per fetch via a WeakMap and validate the S3 JSON response.Written for commit b4af350. Summary will update on new commits.
Summary by CodeRabbit