fix: batch list() calls in all selector-based update methods to prevent unbounded memory consumption#15011
Conversation
…nt unbounded memory consumption
Affected methods in product-module-service.ts:
- updateProducts()
- updateProductVariants()
- updateProductTags()
- updateProductTypes()
- updateProductOptions()
- updateProductCollections()
- updateProductCategories_()
- updateProductOptionValues()
Affected methods in cart-module.ts:
- updateCarts()
All selector-based branches previously called list(selector, {}) with an
empty config — no take, no skip. On large datasets this loads all matching
records into Node.js heap memory simultaneously, risking OOM crashes.
Each affected method now batches in chunks of 500 using take/skip pagination,
following the safe pattern already used in updateProducts_() at L1748.
Fixes medusajs#15010
🦋 Changeset detectedLatest commit: 68b439e The changes in this PR will be included in the next version bump. This PR includes changesets to release 77 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@ashif323 is attempting to deploy a commit to the medusajs Team on Vercel. A member of the Team first needs to authorize it. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3e2ced9. Configure here.
| ) | ||
| skip += BATCH_SIZE | ||
| } while (batch.length === BATCH_SIZE) | ||
| } |
There was a problem hiding this comment.
Batching code indented at wrong level across all methods
Medium Severity
All 9 batching blocks are indented at 2 spaces with the closing } at column 0, while the surrounding else block and method body use 4–6 space indentation. This makes the batching code visually appear to live outside the else block (or even outside the method), even though brace-matching is syntactically correct. A future developer modifying this code could easily misread the block structure and introduce a scoping bug.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 3e2ced9. Configure here.
| normalizedInput.push(...batch.map((variant) => ({ id: variant.id, ...data }))) | ||
| skip += BATCH_SIZE | ||
| } while (batch.length === BATCH_SIZE) | ||
| } |
There was a problem hiding this comment.
Identical batching pattern duplicated nine times across methods
Low Severity
The same do...while batching loop with BATCH_SIZE, skip, and batch variables is copy-pasted across 9 methods. A shared helper (e.g., collectBatchedIds(service, selector, context)) would consolidate this into one implementation, reducing maintenance burden and the risk of inconsistent fixes if the pattern ever needs to change.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3e2ced9. Configure here.
Addresses Cursor Bugbot feedback on PR medusajs#15011: - Extracts repeated do/while batching pattern into a shared private helper - Fixes indentation inconsistency across all affected methods - No behavior change The productCategoryService_ required a cast to IMedusaInternalService<any> as it uses a different concrete type (ProductCategoryService) than the other services in this module.
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Thank you for your contribution! After reviewing this PR, we need a few things addressed before we can move forward: Required changes:
Concerns: The |
1 participant
What
Replaces all unbounded
list(selector, {})calls with paginated batching(BATCH_SIZE=500) across 9 selector-based update methods in the product and
cart modules.
Why
When any of these methods are called with a broad selector object (e.g.
{ status: "active" },{ collection_id: "x" }), the internallist()call had an empty config — no
take, noskip. On large datasets thisloads all matching records into Node.js heap simultaneously, risking OOM
crashes and severe performance degradation.
This is the runtime consequence of the TSDoc inaccuracy noted in PR #14920
— the docs said limit was 15, reality is no limit at all.
Affected methods
packages/modules/product/src/services/product-module-service.tsupdateProducts()updateProductVariants()updateProductTags()updateProductTypes()updateProductOptions()updateProductCollections()updateProductCategories_()updateProductOptionValues()packages/modules/cart/src/services/cart-module.tsupdateCarts()How
The safe pattern already exists in the codebase at
updateProducts_()with
take: productIds.length— this PR extends it consistently to allselector-based paths.
Notes
Closes #15010
Note
Medium Risk
Touches multiple selector-based update code paths in cart/product modules; while behavior should be equivalent, pagination loops could change update coverage under concurrent writes or if service pagination semantics differ.
Overview
Prevents potential OOMs when selector-based update methods are called with broad filters by replacing unbounded
list(selector, {})calls with paginated batching (take/skip,BATCH_SIZE = 500).This batching is applied to
updateCarts()and the product module’s selector-based update methods (products, variants, tags, types, options, collections, categories, option values), accumulating IDs batch-by-batch before issuing the finalupdate()/update*_()call.Reviewed by Cursor Bugbot for commit 3e2ced9. Bugbot is set up for automated code reviews on this repo. Configure here.