fix: reduce worker memory for large Garmin dump imports - #1635
Conversation
Batch FlowProducer.add() calls into chunks of 500 instead of serializing all children in one tree, drop raw Garmin summary payloads from job entries, and release parsed dump data before flow creation. Prevents OOM on dumps with 15K+ FIT files.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
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? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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.
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughGarmin FIT imports are split into 500-entry batch flows with deterministic IDs. Checkpoints track and aggregate results across batches, while generated activity-summary data omits the raw Garmin summary. ChangesGarmin FIT batching
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GarminDump
participant ImportJob
participant BatchFlows
participant ChildJobs
GarminDump->>ImportJob: prepare FIT job entries
ImportJob->>BatchFlows: enqueue 500-entry batch flows
BatchFlows->>ChildJobs: create FIT and extraction jobs
ChildJobs-->>ImportJob: return results and failures
ImportJob->>ImportJob: aggregate results by batch ID
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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 |
![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
🤖 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-garmin-dump-import-job.ts`:
- Around line 190-198: Update the batch result handling around
batchFailureFromChildren and fitResult: allow a zero-file import (batchIds
empty) to produce the existing zero-record result without throwing, while still
throwing for a missing result when batches were expected and no failures exist.
Always merge batchFailures into the final fitResult.errors, including when
batchResult contains successful partial results, preserving all existing result
data and failure messages.
🪄 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: d441388f-f155-4576-a036-161e92c972b9
📒 Files selected for processing (6)
src/jobs/garmin-dump-flow.test.tssrc/jobs/garmin-dump-flow.tssrc/jobs/process-garmin-dump-import-job.test.tssrc/jobs/process-garmin-dump-import-job.tssrc/jobs/queues.tssrc/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 SummaryThe changes in this PR are generally well-structured and introduce a great optimization by batching Garmin FIT file imports and clearing large arrays/maps to free up memory before processing. However, there are two critical logic bugs in
I have posted an inline comment with a committable suggestion to fix both issues. Once that is applied, the PR is good to go! 🤖 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}
- Guard missing-batch-result error with totalFitFiles > 0 check - Always push batchFailures into fitResult.errors instead of dropping them via ?? when batchResult is non-null - Add tests for zero-file imports and partial failure merging
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. |
…tisfy Stryker The PR-added array-clear lines (parsedDump.summaries/fitFiles/weightFitFiles = []) were untestable memory-release mutations: nothing reads those properties afterward, so Stryker's ArrayDeclaration mutants survived (score 0% < threshold 75, CI fail). Spread-copy parsedDump.tempDirectories into preparedImport so parsedDump becomes fully unreferenced after prepareGarminDumpImport returns. The local summaryByExternalId map and the entire parsedDump object (including summaries/fitFiles/weightFitFiles and their contents) are then GC'd together once the function returns, before the long FIT import runs. This frees equal-or-more memory than the early clears while removing the untestable mutants. No production test-only complexity or Stryker disable.
|
🤖 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.
|
@CodeRabbit full rereview |
|
✅ Action performedFull review finished. |
Summary
Fix worker OOM when importing large Garmin dumps (15K+ FIT files).
Three root causes for memory consumption:
activitySummary.rawembedded the entire Garmin summary object via.passthrough()for every FIT file job entry — hundreds of MB for large dumpsFlowProducer.add()built all 15K+ child jobs in one.map()then serialized the entire tree to JSON at onceparsedDumpdata (summaries, fitFiles) stayed in memory after buildingfitJobEntriesbut before flow creationChanges
src/providers/garmin-dump.ts: DroprawfromgarminSummaryToFitJobSummary; releaseparsedDumparrays after buildingfitJobEntriessrc/jobs/garmin-dump-flow.ts: BatchFlowProducer.add()into chunks of 500 (FLOW_BATCH_SIZE), each with a unique batch job ID viacreateBatchIdsrc/jobs/process-garmin-dump-import-job.ts: PersistbatchIdsarray in checkpoint; aggregate results/failures across multiple batch childrensrc/jobs/queues.ts: Makerawoptional infitFileImportActivitySummarySchemaSummary by cubic
Prevents worker OOM when importing large Garmin dumps (15K+ FIT files) by batching flow creation, trimming job payloads, and letting parsed dump memory be collected sooner. Also handles zero-file imports and merges partial batch failures into the final result.
Bug Fixes
FlowProducer.add()into chunks of 500 viaFLOW_BATCH_SIZE, with unique batch IDs fromcreateBatchId.batchIdsand aggregate results across batches; always append partial batch failures toerrors.totalFitFilesis 0; improve waiting logs to include batch count.fitFileImportActivitySummarySchema.rawoptional.parsedDumpby spread-copyingtempDirectoriesinto the prepared import so parsed data is GC’d before batch flows run.Migration
activitySummary.raw, handle it being absent (now optional).Written for commit 4d7448b. Summary will update on new commits.
Summary by CodeRabbit