Fix Garmin dump import fan-out - #1599
Conversation
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughGarmin dump processing now streams selected ZIP content to temporary files, supports nested archive extraction, and orchestrates FIT imports through BullMQ flows with batch aggregation, lock renewal, progress handling, and cleanup. Queue infrastructure, workers, tests, and incident documentation are updated. ChangesGarmin dump import pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant importGarminDumpFile
participant FlowProducer
participant processZipEntryExtractJob
participant processFitFileImportJob
participant processFitFileImportBatchJob
importGarminDumpFile->>FlowProducer: create FIT import flow
FlowProducer->>processZipEntryExtractJob: extract nested FIT entry
processZipEntryExtractJob-->>processFitFileImportJob: provide extracted filePath
FlowProducer->>processFitFileImportJob: run FIT import child
processFitFileImportJob-->>processFitFileImportBatchJob: return import result
processFitFileImportBatchJob-->>importGarminDumpFile: aggregate child results
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
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 |
![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 13 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Addressed the Cubic review summary from #1599 (review).
|
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
…tall-rca # Conflicts: # src/jobs/process-fit-file-import-job.ts # src/providers/garmin-dump.test.ts # src/providers/garmin-dump.ts
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Pull Request Review SummaryThis is an exceptionally well-designed and high-quality Pull Request! It introduces a highly scalable, distributed architecture for importing Garmin dumps using BullMQ flows. By offloading ZIP extraction to a dedicated Key Findings & Recommendations:
Excellent work on this PR! Once the error handling behavior is addressed, this is ready to merge. 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
|
@CodeRabbit full rereview |
✅ Action performedReview finished.
|
Pull Request Review SummaryThe changes in this PR are exceptionally well-designed, robust, and highly optimized. Key Highlights:
No issues or bugs were found. LGTM! 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit 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: 7
🤖 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 `@docs/production-incident-baseline.md`:
- Around line 12884-12929: The incident report’s RCA uses present tense for the
pre-fix behavior and lacks primary-source citations for its operational claims.
Update the Garmin root-cause wording to say it “previously read,” and add
citations linking the referenced Redis stream and Docker inspection artifacts
plus official BullMQ documentation supporting lock renewal, stalled jobs, and
flow behavior. Keep the documented timeline and mitigation details unchanged.
In `@src/jobs/process-fit-file-import-job.ts`:
- Around line 277-288: Update the cleanup flow in processFitFileImportJob so
validation failures from resolveFitFileImportJobData do not leave child
extraction files behind. In the finally block, safely discover all relevant
child extraction paths when cleanupFilePath was not set, then remove them using
the existing cleanup behavior while preserving normal cleanup for resolved
filePath values.
In `@src/jobs/process-zip-entry-extract-job.ts`:
- Around line 172-190: The extraction flow around extractEntryChain currently
writes directly to the deterministic outputPath, making BullMQ replays fail when
that file already exists. First add a regression test that reproduces replaying
an existing output, then extract into a unique temporary file and atomically
move or replace it at the final publish step only after successful extraction;
ensure cleanup and existing success/failure behavior remain intact.
In `@src/providers/garmin-dump.test.ts`:
- Around line 813-851: Update the test around importGarminDumpFile and
flowMock.waitUntilFinished to defer the third extendLock rejection until after
the FIT flow resolves, using a controllable promise or deferred signal. Have the
test release that rejection after waitUntilFinished returns, then assert the
final result still reports the “lost lock” processing error and clears the
renewal interval.
In `@src/providers/garmin-dump.ts`:
- Around line 617-630: Update src/providers/garmin-dump.ts lines 617-630 to
track the active renewal promise, serialize interval-triggered renewals, and
make throwIfFailed() and stop() await that promise before completing while
preserving renewal errors. In src/providers/garmin-dump.test.ts lines 813-851,
add coverage using a deferred renewal that rejects after the FIT flow resolves,
verifying the rejection is awaited and reported.
- Around line 366-374: The summarized-activity extraction path in the Garmin
dump handler must avoid buffering the entire entry. Replace streamToBuffer usage
with direct streaming from openReadStream to the hashed JSON file, while
enforcing an appropriate JSON-specific size limit or incremental parsing and
preserving byte-count accounting via countExtractedBytes.
- Around line 751-758: Update the unexpected-error catch block in the Garmin FIT
import job processing flow to call Sentry.captureException(error) before
converting the failure into the queued SyncError. Preserve the existing
errors.push behavior and error 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: ASSERTIVE
Plan: Pro Plus
Run ID: 704bcc11-a736-4636-915b-6ad68b0336c7
📒 Files selected for processing (13)
docs/production-incident-baseline.mdsrc/jobs/process-fit-file-import-batch-job.test.tssrc/jobs/process-fit-file-import-batch-job.tssrc/jobs/process-fit-file-import-job.test.tssrc/jobs/process-fit-file-import-job.tssrc/jobs/process-zip-entry-extract-job.test.tssrc/jobs/process-zip-entry-extract-job.tssrc/jobs/queues.test.tssrc/jobs/queues.tssrc/jobs/worker.test.tssrc/jobs/worker.tssrc/providers/garmin-dump.test.tssrc/providers/garmin-dump.ts
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/production-incident-baseline.md (1)
12883-12937: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRoot Cause paragraph mixes past and present tense describing already-fixed behavior.
"previously read the full uploaded ZIP into memory, recursively buffers every extracted entry into arrays, ... and only extends the BullMQ import-job lock" —
buffers/extendsshould also be past tense (buffered/extended) since the Fix/mitigation bullet confirms this behavior is no longer current. This file already applies the "previously read" convention correctly elsewhere for RCA sections describing fixed code; keep it consistent here.✏️ Proposed fix
- into memory, recursively buffers every extracted entry into arrays, including - unrelated Garmin account-export payloads and both large nested uploaded-file - ZIPs, and only extends the BullMQ import-job lock with a one-shot 10-minute + into memory, recursively buffered every extracted entry into arrays, including + unrelated Garmin account-export payloads and both large nested uploaded-file + ZIPs, and only extended the BullMQ import-job lock with a one-shot 10-minute extension.🤖 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 `@docs/production-incident-baseline.md` around lines 12883 - 12937, Update the Root cause paragraph in the Garmin Dump Import incident entry so the historical behavior uses past tense consistently: change the descriptions associated with full ZIP processing to say it “buffered” extracted entries and “extended” the BullMQ import-job lock only once, while preserving the existing explanation and Fix / mitigation content.Source: Coding guidelines
🤖 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 `@src/jobs/process-fit-file-import-job.ts`:
- Around line 376-380: Update the unlink failure handler in the cleanup loop of
processFitFileImportJob to call Sentry.captureException(error) in addition to
the existing logger.warn, matching the sibling cleanup-discovery catch and
ensuring the caught error is reported without changing the cleanup flow.
- Around line 84-89: Replace the deprecated .passthrough() call in
fitFileImportErrorPathSchema with Zod 4’s non-deprecated loose-object API,
preserving the optional originalPath field and acceptance of unknown properties.
---
Outside diff comments:
In `@docs/production-incident-baseline.md`:
- Around line 12883-12937: Update the Root cause paragraph in the Garmin Dump
Import incident entry so the historical behavior uses past tense consistently:
change the descriptions associated with full ZIP processing to say it “buffered”
extracted entries and “extended” the BullMQ import-job lock only once, while
preserving the existing explanation and Fix / mitigation content.
🪄 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: 5c0be368-8a61-4df5-84ec-e15dda7c16f1
📒 Files selected for processing (8)
docs/production-incident-baseline.mdsrc/jobs/process-fit-file-import-job.test.tssrc/jobs/process-fit-file-import-job.tssrc/jobs/process-zip-entry-extract-job.test.tssrc/jobs/process-zip-entry-extract-job.tssrc/jobs/queues.test.tssrc/providers/garmin-dump.test.tssrc/providers/garmin-dump.ts
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Addressed CodeRabbit outside-diff review #1599 (review). Fixed.
|
Summary
Validation
pnpm lintwith repo ClickHouse envpnpm tsc --noEmitpackages/server:pnpm tsc --noEmitpackages/web:pnpm tsc --noEmitpnpm exec stryker run stryker.ci.config.json --mutate src/providers/garmin-dump.tspassed with 82.87 mutation scorepnpm exec stryker run stryker.ci.config.json --mutate src/jobs/process-zip-entry-extract-job.tspassed with 78.16 mutation scorepnpm testfailed locally in unrelatedpackages/server/src/routers/predictions.integration.test.tsbecause the ClickHouse test container hit its 3 GiB memory limit while rebuildingv_sleep; 771 files passed before the failure.Summary by CodeRabbit