feat(uploads): idempotent remove + age-graced sweep of unreferenced GridFS blobs - #4017
Merged
Merged
Conversation
…eferenced GridFS blobs
remove() now treats a lookup that matches no record as a debug-logged
no-op ({ deletedCount: 0, notFound: true }) instead of throwing — a
retention job re-running over the same window should see "already
gone" as success, not per-pass error noise. A genuine GridFS bucket
failure on a file that DOES exist still throws.
Adds sweepUnreferenced(kind, collection, paths, minAgeMs): sweeps
GridFS blobs unreferenced by ANY of several reference paths (scalar
or array-of-subdocuments, normalised before the OR check) on another
collection, respecting a minimum-age grace window so a blob written
just before its referencing document persists is never swept. Kept
separate from purge() because purge()'s indexed $lookup join can only
express a single reference key; multi-path OR needs a full streaming
scan instead (one pass to build the referenced-filename set, one pass
over age-eligible candidates) — see JSDoc for the full rationale.
Closes #4013
…olding - orphaned is derivable (scanned - referenced), drop the redundant mutable counter. - Drop the repository-level "sweep complete" summary log — this repository has no other function that logs on success, and the counters already go back to the caller; kept the per-item logger.error on a caught delete failure (would otherwise be silently swallowed) and remove()'s logger.debug (explicit no-op contract). - Test file: factor the repeated find()/aggregate() cursor mock shapes into setCandidates()/setReferences() helpers instead of re-typing them in five tests. No behavior change; full uploads suite (unit + integration) still green.
PRF Phase 0 pre-push review (kimi) flagged two real gaps:
- sweepUnreferenced() caught a per-item bucket.delete() failure and
logged it, but the returned counters looked identical to a clean
sweep — a caller had no way to detect a partial sweep. Adds a
deleteFailed counter alongside the existing deleted/skippedTooYoung.
- remove()'s no-op path (null/undefined/{} input) had no direct test
coverage. Adds explicit coverage asserting the lookup query is
{ filename: undefined } — not a stripped-key match-all — and that
the no-op path never touches the bucket.
The gate's third finding (a hypothetical "Mongoose strips undefined
and matches the first document" bug) was checked against this repo's
actual Mongoose version end-to-end (a real GridFS file survives
remove(null)/remove(undefined)/remove({}) untouched) and did not
reproduce — recorded as a false positive, not applied.
… TOCTOU trade-off
PRF Phase 0 gate iteration 2 (kimi) findings addressed:
- [medium] no test asserted sweepUnreferenced rejects negative/NaN/Infinity
minAgeMs — the guard already existed, coverage was the gap. Added.
- [low] the scalar/array-of-subdocuments normalisation ($concatArrays,
$isArray) was only ever exercised against a stubbed aggregate() in unit
tests. Adds a real integration test seeding actual GridFS blobs and a raw
referencing collection, backdating one blob's uploadDate to exercise the
grace window in the same run — verifies scanned/referenced/orphaned/
deleted/skippedTooYoung end to end against live Mongo.
- [nit] remove()'s debug log on the no-op path showed `filename: undefined`
with no other context when called with null/undefined/{}; now also logs
the original lookup argument (captured before the internal `upload`
reassignment, which was overwriting it).
Not applied — [critical] a TOCTOU race in the two-pass streaming design
(a blob referenced for the first time by a brand-new document during the
sweep's execution window can still look unreferenced and get deleted).
This is inherent to the ported, production-proven reference algorithm, not
introduced by this change — purge()'s existing $lookup approach has the
same class of race, and closing it would need transactional/causal-
consistency machinery well beyond this issue's scope. Documented explicitly
in sweepUnreferenced's JSDoc as an accepted trade-off rather than silently
fixed or silently ignored.
Also not applied — [medium] remove()'s return value shape differs between
the delete-success path (whatever the GridFS bucket resolves with) and the
no-op path ({ deletedCount, notFound }). Pre-existing before this change
(ported from the same reference), no current caller destructures the
delete-success return value, and redesigning it is outside this issue's
scope.
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (3)
WalkthroughChangesThe upload repository now treats missing removals as logged no-ops. It also exposes Uploads cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant UploadRepository
participant ReferencingCollection
participant GridFS
Caller->>UploadRepository: purgeUnreferenced(kind, collection, paths, graceMs)
UploadRepository->>ReferencingCollection: stream reference documents
ReferencingCollection-->>UploadRepository: scalar and nested array references
UploadRepository->>GridFS: stream age-eligible uploads
GridFS-->>UploadRepository: upload candidates
UploadRepository->>GridFS: delete unreferenced uploads
GridFS-->>UploadRepository: deletion result or failure
UploadRepository-->>Caller: return sweep counters
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4017 +/- ##
==========================================
+ Coverage 93.54% 93.80% +0.26%
==========================================
Files 170 170
Lines 5759 5810 +51
Branches 1846 1860 +14
==========================================
+ Hits 5387 5450 +63
+ Misses 302 290 -12
Partials 70 70
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
PierreBrisorgueil
marked this pull request as ready for review
August 4, 2026 18:57
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 17 minutes. |
…lers CodeRabbit was rate-limited on PR #4017 (17 min cooldown); fell back to an independent Claude reviewer per the documented protocol. Two medium findings: - sweepUnreferenced(kind, ...) had no guard on `kind`, unlike its other three params which all fail loudly on a bad value. A missing/empty kind would silently match every upload kind via Uploads.find({ 'metadata.kind': kind }) instead of failing. Added the same guard, plus test coverage. - remove()'s no-op path is a real behavior change for one existing caller: modules/users/controllers/users.images.controller.js calls UploadsService.remove({ filename: req.user.avatar }) with a bare filename lookup (no _id) when updating/removing a profile avatar. Verified: no test or code path depends on the old throw-on-not-found behavior here — the full user.account.integration.tests.js avatar suite (change/remove avatar, with and without an existing one) still passes. If anything this is a latent bug fix: a stale/already-gone avatar reference previously blocked a legitimate avatar update with a 422; it no longer does. Also cleaned up two nits from the same review: removed an unused `aggregate` mock left over in the repository unit test's Uploads model stub (sweepUnreferenced uses the raw driver's aggregate, not Uploads.aggregate — that mock was dead), and made the integration test's grace-window young orphan deterministic (explicit uploadDate = now, matching the already deterministic backdated old orphan) instead of relying on real elapsed time staying under the 5s window.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@modules/uploads/repositories/uploads.repository.js`:
- Around line 250-268: Update the referenceCursor and candidateCursor creation
in the sweep to prevent cursor expiration during per-document processing, using
the supported noCursorTimeout cursor option and/or appropriate batch sizing.
Preserve the existing iteration, deletion behavior, and counters while ensuring
both long-running passes remain usable for large collections.
- Around line 238-248: Add a JSDoc header directly above the toArrayExpr
function, documenting its path parameter with `@param` and its
aggregation-expression result with `@returns`; preserve the existing
implementation and behavior.
- Around line 69-81: Update remove() in
modules/uploads/repositories/uploads.repository.js (lines 69-81) to return {
deletedCount: 0, notFound: true } before calling Uploads.findOne when the
argument has neither an _id nor a string filename; preserve lookup behavior for
valid identifiers. In modules/uploads/tests/uploads.repository.unit.tests.js
(lines 95-107), update the tests for null, undefined, and {} to assert that
findOne() is not called.
In `@modules/uploads/tests/uploads.integration.tests.js`:
- Around line 429-438: Increase the sweep grace window in the test around
UploadRepository.sweepUnreferenced by using a substantially larger minAgeMs
value, and backdate oldOrphanUpload by the same expanded interval so it remains
eligible. Keep youngOrphanUpload stamped at the current time and preserve the
skipped-too-young assertion.
- Around line 456-463: Move the database and upload cleanup currently following
the assertions into a finally block associated with the surrounding try/catch,
ensuring it runs whether assertions pass or throw. Keep the existing error
assertion and logging in catch, and preserve cleanup of the referencing
collection plus scalarRefUpload, arrayRefUpload, multiPathUpload, and
youngOrphanUpload.
In `@modules/uploads/tests/uploads.repository.unit.tests.js`:
- Around line 223-234: Update the test helper setReferences and the multi-path
sweepUnreferenced test to capture the generated aggregation pipeline, then
assert that the projection’s $concatArrays includes both avatar and
snapshots.html. Ensure the assertion verifies every requested path is present,
so the test fails when only the first path is used.
🪄 Autofix
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: 17f7c1a8-ae1b-4332-9c33-9596c52847be
📒 Files selected for processing (3)
modules/uploads/repositories/uploads.repository.jsmodules/uploads/tests/uploads.integration.tests.jsmodules/uploads/tests/uploads.repository.unit.tests.js
- remove(): short-circuit to the no-op marker BEFORE querying when the
lookup argument carries neither an _id nor a string filename, instead of
still calling findOne({ filename: undefined }). This repo's Mongoose
version was already verified end-to-end to handle that filter safely, but
exact handling of an undefined-valued filter key has shifted across
Mongoose/MongoDB-driver versions across several linked upstream issues —
skipping the query removes any dependence on that behavior for a lookup
that was never going to resolve to anything. Updated the corresponding
unit test to assert findOne is not called at all for null/undefined/{}.
- toArrayExpr: proper JSDoc header (was a line comment).
- Both sweepUnreferenced cursors now request noCursorTimeout — this is a
batch/cron-scale streaming pass, and MongoDB closes an idle cursor after
10 minutes by default; a slow pass over a large collection could
otherwise fail mid-run with the counters accumulated so far lost.
- Integration test: widened the grace-window margin (10-minute backdate /
5-minute minAgeMs, was 10s/5s) so the sweep's own runtime under CI load
can't tip the young-orphan fixture over the threshold; moved fixture
cleanup into a finally block so a failed assertion no longer leaks
GridFS blobs into the next run's scan count.
- Unit test: the multi-path data-loss-guard test now captures the actual
aggregation pipeline and asserts both paths reached it, instead of a
stub that returned the same result regardless of which paths were
passed in (would not have caught a regression that only used the first
path).
Reviewer: CodeRabbit (Pro plan, pierreb-devkit/Node PR #4017) — reset from
its earlier same-session rate limit and posted a real review with 6
actionable comments; all 6 addressed here.
…stency with purge() Renames sweepUnreferenced -> purgeUnreferenced and its minAgeMs param -> graceMs (positional). No behavior change. Function, exports, tests, and JSDoc identifier mentions all updated together.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@modules/uploads/repositories/uploads.repository.js`:
- Around line 77-80: Update the three functions noOp, purgeUnreferenced, and
toArrayExpr with JSDoc headers; change their return tags to `@returns`, and
document purgeUnreferenced’s async resolved value as Promise<Object>. Preserve
each function’s existing behavior and implementation.
- Around line 231-233: Update the validation in uploads.repository.js at lines
231-233, within purgeUnreferenced, to require a non-empty string for every
reference path before any database operation; retain the existing AppError
contract for invalid input. In
modules/uploads/tests/uploads.repository.unit.tests.js lines 136-140, add
rejection cases for [null], [undefined], [42], and [''] and assert aggregation
and deletion are not started.
In `@modules/uploads/tests/uploads.integration.tests.js`:
- Around line 470-471: Remove the catch-and-log handler from the
UploadRepository.purgeUnreferenced call in the fixture cleanup, allowing its
rejection to propagate and fail the test. Keep the existing finally-based
cleanup flow unchanged so cleanup is still attempted.
In `@modules/uploads/tests/uploads.repository.unit.tests.js`:
- Around line 136-140: Add test cases for UploadRepository.purgeUnreferenced
covering reference-path arrays containing null, undefined, 42, and an empty
string. For each invalid element, assert rejection and verify aggregate() and
bucket.delete() are not invoked.
🪄 Autofix
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: cacc77b5-0fdb-4db0-8db9-3d7deb5808b4
📒 Files selected for processing (3)
modules/uploads/repositories/uploads.repository.jsmodules/uploads/tests/uploads.integration.tests.jsmodules/uploads/tests/uploads.repository.unit.tests.js
- JSDoc: added a header to the noOp() closure inside remove(), and switched @return -> @returns on purgeUnreferenced (documenting the Promise<Object> resolved value) and toArrayExpr, per this repo's coding guideline for new/modified functions. Pre-existing sibling functions' @return tags are untouched (out of scope for this PR). - purgeUnreferenced now validates every `paths` element is a non-empty string before any database call, same "fail loudly" bar already applied to kind/collection/graceMs — a non-string/empty path would otherwise build a nonsensical aggregation field reference instead of erroring clearly. Added rejection tests for [null], [undefined], [42], ['']. - Integration test: the finally-block cleanup call no longer swallows its own rejection via .catch(console.log) — a real cleanup failure now fails the test instead of silently leaking fixture blobs into the next run's scanned count. Reviewer: CodeRabbit (Pro plan) — 4 actionable comments on the rename push, all addressed here.
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
1 participant
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
remove()is now idempotent: a lookup that matches no record is a debug-logged no-op ({ deletedCount: 0, notFound: true }) instead of a thrown error; a genuine GridFS bucket failure on a file that DOES exist still throws. (2) newsweepUnreferenced(kind, collection, paths, minAgeMs): sweeps GridFS blobs of a givenkindthat are unreferenced by ANY of several reference paths on another collection (scalar or array-of-subdocuments, normalised before the OR check), respecting a minimum-age grace window so a blob written just before its referencing document persists is never swept.remove()to treat "already gone" as success, not per-pass error-log noise.purge()'s indexed$lookupjoin can only express a single reference key — extending it to "referenced by path A OR B" would either lose the index for existing callers or require per-row sub-queries.sweepUnreferencedis a separate function using a full streaming scan instead: one pass over the referencing collection to build a referenced-filename Set, one pass over age-eligible candidates checking membership.Scope
modules/uploads(repository layer only; not wired through the service layer, mirroring howpurge()is already exposed — repository-only, no current callers)nonelow(net-new function with a defensive design;remove()'s no-op branch is additive and does not change the existing delete-success path)Validation
npm run lintnpm test(test:unitfull suite: 169 suites / 2349 tests green;test:integrationfull suite: 43 suites / 548 tests green, including a new end-to-endsweepUnreferencedtest against a live Mongo aggregation pipeline)remove(null)/remove(undefined)/remove({})against a live seeded GridFS document to confirm none of them touch an unrelated fileGuardrails check
Notes for reviewers
sweepUnreferencedtakes a caller-suppliedcollectionname andpathsarray used inside a MongoDB aggregation pipeline as field-path references (never as document keys, never via$where/$function) — not an injection vector, and the function currently has no callers wiring untrusted input into it. Full detail in the JSDoc.remove()'s no-op path (additive).sweepUnreferenced's two-pass streaming design has a known, documented trade-off — a blob referenced for the first time by a brand-new document during the sweep's execution window can still look unreferenced and get deleted (same class of racepurge()'s$lookupapproach already has). Documented explicitly in the function's JSDoc as an accepted trade-off; closing it would need transactional/causal-consistency machinery outside this issue's scope.Summary by CodeRabbit
New Features
Bug Fixes
Tests