From 8f0ccd0bfd85786ef65fa7487cbad31781c923d9 Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Tue, 4 Aug 2026 20:07:28 +0200
Subject: [PATCH 1/8] feat(uploads): idempotent remove + age-graced multi-path
 sweep of unreferenced GridFS blobs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 .../repositories/uploads.repository.js        | 155 +++++++++++++-
 .../tests/uploads.repository.unit.tests.js    | 202 ++++++++++++++++++
 2 files changed, 353 insertions(+), 4 deletions(-)
 create mode 100644 modules/uploads/tests/uploads.repository.unit.tests.js

diff --git a/modules/uploads/repositories/uploads.repository.js b/modules/uploads/repositories/uploads.repository.js
index e86c91af6..d3f1fe3b4 100644
--- a/modules/uploads/repositories/uploads.repository.js
+++ b/modules/uploads/repositories/uploads.repository.js
@@ -4,6 +4,7 @@
 import mongoose from 'mongoose';
 
 import AppError from '../../../lib/helpers/AppError.js';
+import logger from '../../../lib/services/logger.js';
 
 const Uploads = mongoose.model('Uploads');
 
@@ -47,12 +48,27 @@ const getStream = (upload) => {
 const update = (id, update) => Uploads.findOneAndUpdate({ _id: id }, update, { returnDocument: 'after' }).exec();
 
 /**
- * @desc Function to remove an upload from db
- * @param {Object} upload
- * @return {Object} confirmation of delete
+ * @desc Function to remove an upload from db. A missing file — no record
+ * matches the lookup, e.g. a filename already deleted by a previous pass —
+ * is a NO-OP (logged at debug), not a thrown error. A retention job that
+ * re-runs over the same window can call this with the SAME filename across
+ * passes when a persist fails after a successful delete; treating "already
+ * gone" as success (instead of throwing) prevents that resurfacing as
+ * per-pass error-log noise. A genuine GridFS failure (bucket error deleting
+ * a file that DOES exist) still throws — only the "nothing to delete" case
+ * is swallowed.
+ * @param {Object} upload - an Upload doc (with `_id`), or a bare lookup key
+ *   (e.g. `{ filename }`)
+ * @return {Object} confirmation of delete, or a no-op marker when no
+ *   matching file was found
  */
 const remove = async (upload) => {
-  if (!upload || !upload._id) upload = await Uploads.findOne({ filename: upload.filename }).exec();
+  const filename = upload?.filename;
+  if (!upload || !upload._id) upload = await Uploads.findOne({ filename }).exec();
+  if (!upload) {
+    logger.debug('Upload: remove - no matching file, treating as already removed', { filename });
+    return { deletedCount: 0, notFound: true };
+  }
   try {
     const unlinked = await bucket.delete(upload._id);
     return unlinked;
@@ -116,6 +132,136 @@ const purge = async (kind, collection, key) => {
   return { deletedCount };
 };
 
+/**
+ * @desc Sweep GridFS blobs of a given `kind` that are unreferenced by ANY of
+ * several possible reference paths on another collection, respecting a
+ * minimum age (grace window) so a blob written moments ago — before the
+ * document that will reference it is persisted — is never swept.
+ *
+ * Kept as a separate function rather than a `purge()` extension — not merely
+ * to avoid touching `purge()`'s existing callers, but because the two use
+ * genuinely different, non-interchangeable query strategies: `purge()`'s
+ * native `$lookup` localField/foreignField join lets MongoDB use an index on
+ * the foreign collection for an equality match — appropriate when there's
+ * exactly one reference key. That strategy has no way to express "referenced
+ * by path A OR path B OR ..." (a blob referenced only via a path it doesn't
+ * check would look unreferenced and be deleted — data loss, worse than the
+ * leak this fixes), which is why this function instead does a full streaming
+ * scan (see Implementation below) — the only way to check several paths,
+ * some of them arrays, without N correlated sub-queries. Forcing `purge()`'s
+ * single-key callers through that broader scan would trade an indexed join
+ * for a full collection scan for no benefit; forcing this function's
+ * multi-path callers through a `$lookup` would reintroduce the N-sub-queries
+ * problem. One function selecting between two internal algorithms by
+ * argument shape would not actually be simpler than two named functions.
+ * `purge()` also has no age floor, and bolting one on would change behaviour
+ * for its existing caller. A reference path may point at a scalar field or
+ * an array-of-subdocuments field (e.g. `snapshots.html` where `snapshots` is
+ * an array) — both are normalised to an array before the OR check.
+ *
+ * Implementation: rather than a correlated `$lookup` pipeline per candidate
+ * upload (a nested-loop join — one sub-query per row against `collection`),
+ * this makes ONE streaming pass over `collection` to build an in-memory Set
+ * of every filename referenced from any `paths` entry, then a single
+ * streaming pass over the candidate uploads checking Set membership:
+ * O(collection) + O(uploads) instead of O(uploads × collection). The
+ * collection-side pass never accumulates into a single Mongo document
+ * (would risk the 16MB BSON limit at scale) — dedup happens client-side.
+ *
+ * Fails loudly (throws) if `collection` does not exist, rather than treating
+ * a mistyped name as "nothing is referenced" — the latter would silently
+ * delete every candidate blob once past the grace window: a wrong
+ * collection name must never look like a clean, empty result.
+ *
+ * @param {String} kind - metadata.kind to sweep (e.g. 'htmlSnapshot')
+ * @param {String} collection - name of the collection to check references against
+ * @param {String[]} paths - dot-paths on `collection` docs that may reference an upload's filename
+ * @param {Number} minAgeMs - minimum age (ms, from GridFS `uploadDate`) before an unreferenced blob is eligible for deletion
+ * @return {Object} counters — { scanned, referenced, orphaned, deleted, skippedTooYoung }
+ */
+const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
+  if (!Array.isArray(paths) || paths.length === 0) {
+    throw new AppError('Upload: sweepUnreferenced requires at least one reference path', { code: 'REPOSITORY_ERROR' });
+  }
+  if (!Number.isFinite(minAgeMs) || minAgeMs < 0) {
+    throw new AppError('Upload: sweepUnreferenced requires a non-negative minAgeMs', { code: 'REPOSITORY_ERROR' });
+  }
+
+  /* A mistyped `collection` name would make the aggregation below return an
+   * EMPTY cursor — every candidate would then look unreferenced and get
+   * deleted once past the grace window. That failure mode is silent DATA
+   * LOSS. Fail loudly instead. */
+  const collectionExists = await mongoose.connection.db
+    .listCollections({ name: collection }, { nameOnly: true })
+    .hasNext();
+  if (!collectionExists) {
+    throw new AppError(`Upload: sweepUnreferenced target collection "${collection}" does not exist`, { code: 'REPOSITORY_ERROR' });
+  }
+
+  // Normalises a reference path's value to an array: missing/null -> [],
+  // an array field (e.g. across subdocuments) -> itself, a scalar -> [value].
+  const toArrayExpr = (path) => {
+    const field = `$${path}`;
+    return {
+      $let: {
+        vars: { v: { $ifNull: [field, null] } },
+        in: {
+          $cond: [{ $eq: ['$$v', null] }, [], { $cond: [{ $isArray: '$$v' }, '$$v', ['$$v']] }],
+        },
+      },
+    };
+  };
+
+  const referenced = new Set();
+  const referenceCursor = mongoose.connection.db.collection(collection).aggregate([
+    { $project: { _id: 0, refs: { $concatArrays: paths.map(toArrayExpr) } } },
+    { $match: { 'refs.0': { $exists: true } } },
+  ]);
+  for await (const doc of referenceCursor) {
+    for (const filename of doc.refs) {
+      if (typeof filename === 'string') referenced.add(filename);
+    }
+  }
+
+  const now = Date.now();
+  let scanned = 0;
+  let referencedCount = 0;
+  let orphaned = 0;
+  let deleted = 0;
+  let skippedTooYoung = 0;
+
+  const candidateCursor = Uploads.find({ 'metadata.kind': kind }).select('filename uploadDate').lean().cursor();
+  for await (const candidate of candidateCursor) {
+    scanned += 1;
+    if (referenced.has(candidate.filename)) {
+      referencedCount += 1;
+      continue;
+    }
+    orphaned += 1;
+    // Missing uploadDate is treated as "unknown age" -> never eligible for
+    // deletion (fail closed, not open) rather than as "very old".
+    const ageMs = candidate.uploadDate ? now - new Date(candidate.uploadDate).getTime() : -1;
+    if (ageMs < minAgeMs) {
+      skippedTooYoung += 1;
+      continue;
+    }
+    try {
+      await bucket.delete(candidate._id);
+      deleted += 1;
+    } catch (err) {
+      logger.error('Upload: sweepUnreferenced - delete failed', {
+        filename: candidate.filename,
+        kind,
+        error: err?.message,
+      });
+    }
+  }
+
+  const counters = { scanned, referenced: referencedCount, orphaned, deleted, skippedTooYoung };
+  logger.info('Upload: sweepUnreferenced complete', { kind, collection, minAgeMs, ...counters });
+  return counters;
+};
+
 export default {
   list,
   get,
@@ -124,4 +270,5 @@ export default {
   remove,
   deleteMany,
   purge,
+  sweepUnreferenced,
 };
diff --git a/modules/uploads/tests/uploads.repository.unit.tests.js b/modules/uploads/tests/uploads.repository.unit.tests.js
new file mode 100644
index 000000000..4e8399ec3
--- /dev/null
+++ b/modules/uploads/tests/uploads.repository.unit.tests.js
@@ -0,0 +1,202 @@
+/**
+ * Module dependencies.
+ */
+import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals';
+
+/**
+ * Unit tests for uploads.repository.js — remove() no-op semantics and
+ * sweepUnreferenced() multi-path unreferenced-blob sweep.
+ */
+describe('UploadRepository unit tests:', () => {
+  let UploadRepository;
+  let mockUploadsModel;
+  let mockBucket;
+  let mockDb;
+  let mockLogger;
+
+  /** Builds an async-iterable cursor stub from a plain array of docs. */
+  const asCursor = (docs) => ({
+    [Symbol.asyncIterator]: async function* iterate() {
+      for (const doc of docs) yield doc;
+    },
+  });
+
+  beforeEach(async () => {
+    jest.resetModules();
+
+    mockBucket = { delete: jest.fn().mockResolvedValue(undefined), openDownloadStream: jest.fn() };
+
+    mockLogger = { debug: jest.fn(), info: jest.fn(), error: jest.fn(), warn: jest.fn() };
+
+    mockDb = {
+      listCollections: jest.fn(() => ({ hasNext: jest.fn().mockResolvedValue(true) })),
+      collection: jest.fn(() => ({ aggregate: jest.fn(() => asCursor([])) })),
+    };
+
+    mockUploadsModel = {
+      findOne: jest.fn(() => ({ exec: jest.fn().mockResolvedValue(null) })),
+      find: jest.fn(() => ({
+        select: jest.fn().mockReturnThis(),
+        lean: jest.fn().mockReturnThis(),
+        cursor: jest.fn(() => asCursor([])),
+      })),
+      aggregate: jest.fn(),
+    };
+
+    jest.unstable_mockModule('mongoose', () => ({
+      default: {
+        model: jest.fn(() => mockUploadsModel),
+        connection: { db: mockDb },
+        mongo: { GridFSBucket: jest.fn(() => mockBucket) },
+      },
+    }));
+
+    jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ default: mockLogger }));
+
+    const mod = await import('../repositories/uploads.repository.js');
+    UploadRepository = mod.default;
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
+  describe('remove', () => {
+    test('is a no-op when no record matches the lookup (already removed)', async () => {
+      mockUploadsModel.findOne.mockReturnValue({ exec: jest.fn().mockResolvedValue(null) });
+
+      const result = await UploadRepository.remove({ filename: 'gone.png' });
+
+      expect(result).toEqual({ deletedCount: 0, notFound: true });
+      expect(mockBucket.delete).not.toHaveBeenCalled();
+      expect(mockLogger.debug).toHaveBeenCalledWith(
+        'Upload: remove - no matching file, treating as already removed',
+        { filename: 'gone.png' },
+      );
+    });
+
+    test('deletes the GridFS file when the doc already carries an _id', async () => {
+      const upload = { _id: 'abc123', filename: 'present.png' };
+
+      const result = await UploadRepository.remove(upload);
+
+      expect(mockUploadsModel.findOne).not.toHaveBeenCalled();
+      expect(mockBucket.delete).toHaveBeenCalledWith('abc123');
+      expect(result).toBeUndefined();
+    });
+
+    test('throws on a genuine GridFS bucket failure for a file that DOES exist', async () => {
+      const upload = { _id: 'abc123', filename: 'present.png' };
+      mockBucket.delete.mockRejectedValueOnce(new Error('bucket unreachable'));
+
+      await expect(UploadRepository.remove(upload)).rejects.toThrow('Upload: delete error');
+      expect(mockBucket.delete).toHaveBeenCalledWith('abc123');
+    });
+  });
+
+  describe('sweepUnreferenced', () => {
+    const kind = 'htmlSnapshot';
+    const collection = 'histories';
+
+    test('throws when paths is missing/empty', async () => {
+      await expect(UploadRepository.sweepUnreferenced(kind, collection, [], 1000)).rejects.toThrow(
+        'sweepUnreferenced requires at least one reference path',
+      );
+    });
+
+    test('throws when the target collection does not exist (fails loudly, not a silent empty result)', async () => {
+      mockDb.listCollections.mockReturnValue({ hasNext: jest.fn().mockResolvedValue(false) });
+
+      await expect(UploadRepository.sweepUnreferenced(kind, 'typo_collection', ['snapshot'], 1000)).rejects.toThrow(
+        'target collection "typo_collection" does not exist',
+      );
+    });
+
+    test('respects the grace window: an unreferenced-but-young blob is skipped, not deleted', async () => {
+      const now = Date.now();
+      mockUploadsModel.find.mockReturnValue({
+        select: jest.fn().mockReturnThis(),
+        lean: jest.fn().mockReturnThis(),
+        cursor: jest.fn(() => asCursor([{ _id: 'young1', filename: 'young.png', uploadDate: new Date(now - 1000) }])),
+      });
+
+      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
+
+      expect(mockBucket.delete).not.toHaveBeenCalled();
+      expect(counters).toMatchObject({ scanned: 1, orphaned: 1, deleted: 0, skippedTooYoung: 1 });
+    });
+
+    test('sweeps an unreferenced blob past the grace window on a scalar reference path', async () => {
+      const now = Date.now();
+      mockDb.collection.mockReturnValue({ aggregate: jest.fn(() => asCursor([])) }); // nothing references it
+      mockUploadsModel.find.mockReturnValue({
+        select: jest.fn().mockReturnThis(),
+        lean: jest.fn().mockReturnThis(),
+        cursor: jest.fn(() => asCursor([{ _id: 'old1', filename: 'orphan.png', uploadDate: new Date(now - 120_000) }])),
+      });
+
+      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
+
+      expect(mockBucket.delete).toHaveBeenCalledWith('old1');
+      expect(counters).toMatchObject({ scanned: 1, orphaned: 1, deleted: 1, skippedTooYoung: 0 });
+    });
+
+    test('keeps a blob referenced via a scalar path', async () => {
+      const now = Date.now();
+      mockDb.collection.mockReturnValue({ aggregate: jest.fn(() => asCursor([{ refs: ['referenced.png'] }])) });
+      mockUploadsModel.find.mockReturnValue({
+        select: jest.fn().mockReturnThis(),
+        lean: jest.fn().mockReturnThis(),
+        cursor: jest.fn(() => asCursor([{ _id: 'ref1', filename: 'referenced.png', uploadDate: new Date(now - 120_000) }])),
+      });
+
+      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
+
+      expect(mockBucket.delete).not.toHaveBeenCalled();
+      expect(counters).toMatchObject({ scanned: 1, referenced: 1, orphaned: 0, deleted: 0 });
+    });
+
+    test('keeps a blob referenced via an array-of-subdocuments path', async () => {
+      const now = Date.now();
+      // Simulates the aggregation already flattening an array-of-subdocuments
+      // path (e.g. `snapshots.html`) into the referenced-filename set.
+      mockDb.collection.mockReturnValue({
+        aggregate: jest.fn(() => asCursor([{ refs: ['sub1.png', 'sub2.png'] }])),
+      });
+      mockUploadsModel.find.mockReturnValue({
+        select: jest.fn().mockReturnThis(),
+        lean: jest.fn().mockReturnThis(),
+        cursor: jest.fn(() =>
+          asCursor([
+            { _id: 'sub1', filename: 'sub1.png', uploadDate: new Date(now - 120_000) },
+            { _id: 'sub2', filename: 'sub2.png', uploadDate: new Date(now - 120_000) },
+          ]),
+        ),
+      });
+
+      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshots.html'], 60_000);
+
+      expect(mockBucket.delete).not.toHaveBeenCalled();
+      expect(counters).toMatchObject({ scanned: 2, referenced: 2, orphaned: 0, deleted: 0 });
+    });
+
+    test('keeps a blob referenced by only ONE of several paths (data-loss guard)', async () => {
+      const now = Date.now();
+      // Only the second path (`snapshots.html`) references it — a
+      // single-path check would have missed this and deleted a live blob.
+      mockDb.collection.mockReturnValue({
+        aggregate: jest.fn(() => asCursor([{ refs: ['multi-ref.png'] }])),
+      });
+      mockUploadsModel.find.mockReturnValue({
+        select: jest.fn().mockReturnThis(),
+        lean: jest.fn().mockReturnThis(),
+        cursor: jest.fn(() => asCursor([{ _id: 'multi1', filename: 'multi-ref.png', uploadDate: new Date(now - 120_000) }])),
+      });
+
+      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['avatar', 'snapshots.html'], 60_000);
+
+      expect(mockBucket.delete).not.toHaveBeenCalled();
+      expect(counters).toMatchObject({ scanned: 1, referenced: 1, orphaned: 0, deleted: 0 });
+    });
+  });
+});

From 4020e46e2ed30c5513ac3502531c795529898320 Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Tue, 4 Aug 2026 20:10:59 +0200
Subject: [PATCH 2/8] refactor(uploads): simplify sweepUnreferenced counters
 and test scaffolding
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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.
---
 .../repositories/uploads.repository.js        | 14 ++--
 .../tests/uploads.repository.unit.tests.js    | 64 ++++++++-----------
 2 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/modules/uploads/repositories/uploads.repository.js b/modules/uploads/repositories/uploads.repository.js
index d3f1fe3b4..63d3d0289 100644
--- a/modules/uploads/repositories/uploads.repository.js
+++ b/modules/uploads/repositories/uploads.repository.js
@@ -226,7 +226,6 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
   const now = Date.now();
   let scanned = 0;
   let referencedCount = 0;
-  let orphaned = 0;
   let deleted = 0;
   let skippedTooYoung = 0;
 
@@ -237,7 +236,6 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
       referencedCount += 1;
       continue;
     }
-    orphaned += 1;
     // Missing uploadDate is treated as "unknown age" -> never eligible for
     // deletion (fail closed, not open) rather than as "very old".
     const ageMs = candidate.uploadDate ? now - new Date(candidate.uploadDate).getTime() : -1;
@@ -257,9 +255,15 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
     }
   }
 
-  const counters = { scanned, referenced: referencedCount, orphaned, deleted, skippedTooYoung };
-  logger.info('Upload: sweepUnreferenced complete', { kind, collection, minAgeMs, ...counters });
-  return counters;
+  // orphaned is derivable — every scanned candidate is either referenced or
+  // orphaned, no third state — so it's computed once here rather than
+  // tracked as its own mutable counter through the loop.
+  const orphaned = scanned - referencedCount;
+  // Only the counters are returned — summary observability is the caller's
+  // concern (this repository has no other function that logs on success;
+  // the per-item `logger.error` above is the one exception, kept because a
+  // caught delete failure here is otherwise swallowed with no record of it).
+  return { scanned, referenced: referencedCount, orphaned, deleted, skippedTooYoung };
 };
 
 export default {
diff --git a/modules/uploads/tests/uploads.repository.unit.tests.js b/modules/uploads/tests/uploads.repository.unit.tests.js
index 4e8399ec3..c831ac3de 100644
--- a/modules/uploads/tests/uploads.repository.unit.tests.js
+++ b/modules/uploads/tests/uploads.repository.unit.tests.js
@@ -21,6 +21,20 @@ describe('UploadRepository unit tests:', () => {
     },
   });
 
+  /** Stubs Uploads.find(...).select().lean().cursor() to yield `docs`. */
+  const setCandidates = (docs) => {
+    mockUploadsModel.find.mockReturnValue({
+      select: jest.fn().mockReturnThis(),
+      lean: jest.fn().mockReturnThis(),
+      cursor: jest.fn(() => asCursor(docs)),
+    });
+  };
+
+  /** Stubs db.collection(...).aggregate(...) to yield `docs` (reference scan). */
+  const setReferences = (docs) => {
+    mockDb.collection.mockReturnValue({ aggregate: jest.fn(() => asCursor(docs)) });
+  };
+
   beforeEach(async () => {
     jest.resetModules();
 
@@ -114,11 +128,7 @@ describe('UploadRepository unit tests:', () => {
 
     test('respects the grace window: an unreferenced-but-young blob is skipped, not deleted', async () => {
       const now = Date.now();
-      mockUploadsModel.find.mockReturnValue({
-        select: jest.fn().mockReturnThis(),
-        lean: jest.fn().mockReturnThis(),
-        cursor: jest.fn(() => asCursor([{ _id: 'young1', filename: 'young.png', uploadDate: new Date(now - 1000) }])),
-      });
+      setCandidates([{ _id: 'young1', filename: 'young.png', uploadDate: new Date(now - 1000) }]);
 
       const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
 
@@ -128,12 +138,8 @@ describe('UploadRepository unit tests:', () => {
 
     test('sweeps an unreferenced blob past the grace window on a scalar reference path', async () => {
       const now = Date.now();
-      mockDb.collection.mockReturnValue({ aggregate: jest.fn(() => asCursor([])) }); // nothing references it
-      mockUploadsModel.find.mockReturnValue({
-        select: jest.fn().mockReturnThis(),
-        lean: jest.fn().mockReturnThis(),
-        cursor: jest.fn(() => asCursor([{ _id: 'old1', filename: 'orphan.png', uploadDate: new Date(now - 120_000) }])),
-      });
+      setReferences([]); // nothing references it
+      setCandidates([{ _id: 'old1', filename: 'orphan.png', uploadDate: new Date(now - 120_000) }]);
 
       const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
 
@@ -143,12 +149,8 @@ describe('UploadRepository unit tests:', () => {
 
     test('keeps a blob referenced via a scalar path', async () => {
       const now = Date.now();
-      mockDb.collection.mockReturnValue({ aggregate: jest.fn(() => asCursor([{ refs: ['referenced.png'] }])) });
-      mockUploadsModel.find.mockReturnValue({
-        select: jest.fn().mockReturnThis(),
-        lean: jest.fn().mockReturnThis(),
-        cursor: jest.fn(() => asCursor([{ _id: 'ref1', filename: 'referenced.png', uploadDate: new Date(now - 120_000) }])),
-      });
+      setReferences([{ refs: ['referenced.png'] }]);
+      setCandidates([{ _id: 'ref1', filename: 'referenced.png', uploadDate: new Date(now - 120_000) }]);
 
       const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
 
@@ -160,19 +162,11 @@ describe('UploadRepository unit tests:', () => {
       const now = Date.now();
       // Simulates the aggregation already flattening an array-of-subdocuments
       // path (e.g. `snapshots.html`) into the referenced-filename set.
-      mockDb.collection.mockReturnValue({
-        aggregate: jest.fn(() => asCursor([{ refs: ['sub1.png', 'sub2.png'] }])),
-      });
-      mockUploadsModel.find.mockReturnValue({
-        select: jest.fn().mockReturnThis(),
-        lean: jest.fn().mockReturnThis(),
-        cursor: jest.fn(() =>
-          asCursor([
-            { _id: 'sub1', filename: 'sub1.png', uploadDate: new Date(now - 120_000) },
-            { _id: 'sub2', filename: 'sub2.png', uploadDate: new Date(now - 120_000) },
-          ]),
-        ),
-      });
+      setReferences([{ refs: ['sub1.png', 'sub2.png'] }]);
+      setCandidates([
+        { _id: 'sub1', filename: 'sub1.png', uploadDate: new Date(now - 120_000) },
+        { _id: 'sub2', filename: 'sub2.png', uploadDate: new Date(now - 120_000) },
+      ]);
 
       const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshots.html'], 60_000);
 
@@ -184,14 +178,8 @@ describe('UploadRepository unit tests:', () => {
       const now = Date.now();
       // Only the second path (`snapshots.html`) references it — a
       // single-path check would have missed this and deleted a live blob.
-      mockDb.collection.mockReturnValue({
-        aggregate: jest.fn(() => asCursor([{ refs: ['multi-ref.png'] }])),
-      });
-      mockUploadsModel.find.mockReturnValue({
-        select: jest.fn().mockReturnThis(),
-        lean: jest.fn().mockReturnThis(),
-        cursor: jest.fn(() => asCursor([{ _id: 'multi1', filename: 'multi-ref.png', uploadDate: new Date(now - 120_000) }])),
-      });
+      setReferences([{ refs: ['multi-ref.png'] }]);
+      setCandidates([{ _id: 'multi1', filename: 'multi-ref.png', uploadDate: new Date(now - 120_000) }]);
 
       const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['avatar', 'snapshots.html'], 60_000);
 

From 4330099eb1fb9e2b94bfa2c4d621f98b7c39da7a Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Tue, 4 Aug 2026 20:24:20 +0200
Subject: [PATCH 3/8] fix(uploads): surface delete failures in
 sweepUnreferenced counters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../repositories/uploads.repository.js        | 16 ++++++++--
 .../tests/uploads.repository.unit.tests.js    | 29 +++++++++++++++++++
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/modules/uploads/repositories/uploads.repository.js b/modules/uploads/repositories/uploads.repository.js
index 63d3d0289..81611de50 100644
--- a/modules/uploads/repositories/uploads.repository.js
+++ b/modules/uploads/repositories/uploads.repository.js
@@ -177,7 +177,10 @@ const purge = async (kind, collection, key) => {
  * @param {String} collection - name of the collection to check references against
  * @param {String[]} paths - dot-paths on `collection` docs that may reference an upload's filename
  * @param {Number} minAgeMs - minimum age (ms, from GridFS `uploadDate`) before an unreferenced blob is eligible for deletion
- * @return {Object} counters — { scanned, referenced, orphaned, deleted, skippedTooYoung }
+ * @return {Object} counters — { scanned, referenced, orphaned, deleted, deleteFailed, skippedTooYoung }.
+ *   `deleteFailed` lets a caller detect a partial sweep (some eligible blobs
+ *   left undeleted after a transient bucket error) instead of a `deleted`
+ *   count that silently looks complete.
  */
 const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
   if (!Array.isArray(paths) || paths.length === 0) {
@@ -227,6 +230,7 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
   let scanned = 0;
   let referencedCount = 0;
   let deleted = 0;
+  let deleteFailed = 0;
   let skippedTooYoung = 0;
 
   const candidateCursor = Uploads.find({ 'metadata.kind': kind }).select('filename uploadDate').lean().cursor();
@@ -247,6 +251,11 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
       await bucket.delete(candidate._id);
       deleted += 1;
     } catch (err) {
+      // Logged (not just counted): a caught delete failure that a caller
+      // never inspects the counters for would otherwise vanish with no
+      // record of which file failed. `deleteFailed` lets a caller detect a
+      // partial sweep programmatically; the log gives the "which one".
+      deleteFailed += 1;
       logger.error('Upload: sweepUnreferenced - delete failed', {
         filename: candidate.filename,
         kind,
@@ -257,13 +266,14 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
 
   // orphaned is derivable — every scanned candidate is either referenced or
   // orphaned, no third state — so it's computed once here rather than
-  // tracked as its own mutable counter through the loop.
+  // tracked as its own mutable counter through the loop. Within "orphaned",
+  // deleted + deleteFailed + skippedTooYoung together account for the total.
   const orphaned = scanned - referencedCount;
   // Only the counters are returned — summary observability is the caller's
   // concern (this repository has no other function that logs on success;
   // the per-item `logger.error` above is the one exception, kept because a
   // caught delete failure here is otherwise swallowed with no record of it).
-  return { scanned, referenced: referencedCount, orphaned, deleted, skippedTooYoung };
+  return { scanned, referenced: referencedCount, orphaned, deleted, deleteFailed, skippedTooYoung };
 };
 
 export default {
diff --git a/modules/uploads/tests/uploads.repository.unit.tests.js b/modules/uploads/tests/uploads.repository.unit.tests.js
index c831ac3de..b05ef0259 100644
--- a/modules/uploads/tests/uploads.repository.unit.tests.js
+++ b/modules/uploads/tests/uploads.repository.unit.tests.js
@@ -89,6 +89,20 @@ describe('UploadRepository unit tests:', () => {
       );
     });
 
+    test.each([[null], [undefined], [{}]])(
+      'does not query with an unbound filter when called with %p (findOne receives filename: undefined, not a stripped-key match-all)',
+      async (arg) => {
+        const findOneExec = jest.fn().mockResolvedValue(null);
+        mockUploadsModel.findOne.mockReturnValue({ exec: findOneExec });
+
+        const result = await UploadRepository.remove(arg);
+
+        expect(mockUploadsModel.findOne).toHaveBeenCalledWith({ filename: undefined });
+        expect(result).toEqual({ deletedCount: 0, notFound: true });
+        expect(mockBucket.delete).not.toHaveBeenCalled();
+      },
+    );
+
     test('deletes the GridFS file when the doc already carries an _id', async () => {
       const upload = { _id: 'abc123', filename: 'present.png' };
 
@@ -126,6 +140,21 @@ describe('UploadRepository unit tests:', () => {
       );
     });
 
+    test('a partial sweep (one delete fails) is visible in the counters, not silently reported as a clean sweep', async () => {
+      const now = Date.now();
+      setReferences([]);
+      setCandidates([{ _id: 'flaky1', filename: 'flaky.png', uploadDate: new Date(now - 120_000) }]);
+      mockBucket.delete.mockRejectedValueOnce(new Error('bucket unreachable'));
+
+      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
+
+      expect(counters).toMatchObject({ scanned: 1, orphaned: 1, deleted: 0, deleteFailed: 1 });
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        'Upload: sweepUnreferenced - delete failed',
+        expect.objectContaining({ filename: 'flaky.png', kind }),
+      );
+    });
+
     test('respects the grace window: an unreferenced-but-young blob is skipped, not deleted', async () => {
       const now = Date.now();
       setCandidates([{ _id: 'young1', filename: 'young.png', uploadDate: new Date(now - 1000) }]);

From 940031cbf79ee22fac0a89089e3984814dc36a43 Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Tue, 4 Aug 2026 20:51:10 +0200
Subject: [PATCH 4/8] test(uploads): cover invalid minAgeMs + real-pipeline
 sweep, document TOCTOU trade-off
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../repositories/uploads.repository.js        | 33 ++++++++--
 .../tests/uploads.integration.tests.js        | 62 +++++++++++++++++++
 .../tests/uploads.repository.unit.tests.js    |  9 ++-
 3 files changed, 99 insertions(+), 5 deletions(-)

diff --git a/modules/uploads/repositories/uploads.repository.js b/modules/uploads/repositories/uploads.repository.js
index 81611de50..6757d61ba 100644
--- a/modules/uploads/repositories/uploads.repository.js
+++ b/modules/uploads/repositories/uploads.repository.js
@@ -58,15 +58,25 @@ const update = (id, update) => Uploads.findOneAndUpdate({ _id: id }, update, { r
  * a file that DOES exist) still throws — only the "nothing to delete" case
  * is swallowed.
  * @param {Object} upload - an Upload doc (with `_id`), or a bare lookup key
- *   (e.g. `{ filename }`)
- * @return {Object} confirmation of delete, or a no-op marker when no
- *   matching file was found
+ *   (e.g. `{ filename }`); `null`/`undefined`/`{}` are also valid — all
+ *   resolve to a lookup that matches nothing and hit the no-op path below
+ * @return {Object} a no-op marker ({ deletedCount: 0, notFound: true }) when
+ *   no matching file was found, or otherwise whatever the underlying GridFS
+ *   bucket delete resolves with (unchanged from before this no-op path was
+ *   added — callers on the delete-success path never relied on a specific
+ *   shape here)
  */
 const remove = async (upload) => {
+  const lookup = upload ?? null;
   const filename = upload?.filename;
   if (!upload || !upload._id) upload = await Uploads.findOne({ filename }).exec();
   if (!upload) {
-    logger.debug('Upload: remove - no matching file, treating as already removed', { filename });
+    // `filename` is undefined (not just falsy-but-present) whenever `upload`
+    // itself was null/undefined/{} — logging the ORIGINAL lookup argument
+    // (captured before the findOne reassignment above overwrites `upload`)
+    // keeps this debug line useful in that case instead of showing
+    // `filename: undefined` with no other context.
+    logger.debug('Upload: remove - no matching file, treating as already removed', { filename: filename ?? null, lookup });
     return { deletedCount: 0, notFound: true };
   }
   try {
@@ -173,6 +183,21 @@ const purge = async (kind, collection, key) => {
  * delete every candidate blob once past the grace window: a wrong
  * collection name must never look like a clean, empty result.
  *
+ * Known trade-off (accepted, not fixed here): the reference-Set snapshot is
+ * taken at the START of the run, then read against for the whole candidate
+ * scan. A blob whose age already exceeds `minAgeMs` — i.e. NOT protected by
+ * the grace window, which only covers the gap between a blob's own write and
+ * its first reference — that gets referenced by a brand-new document for the
+ * FIRST TIME after the snapshot but before the scan reaches it will still
+ * look unreferenced and get deleted. Closing this would need either a
+ * point-in-time consistent read across both passes (session/causal
+ * consistency) or re-checking each candidate's reference status
+ * transactionally right before delete — real complexity for a scenario this
+ * sweep's intended use (referencing a blob at write time, not reattaching a
+ * reference to an already-old, already-orphaned one later) does not
+ * exercise. Not addressed without a product decision to actually protect
+ * against it.
+ *
  * @param {String} kind - metadata.kind to sweep (e.g. 'htmlSnapshot')
  * @param {String} collection - name of the collection to check references against
  * @param {String[]} paths - dot-paths on `collection` docs that may reference an upload's filename
diff --git a/modules/uploads/tests/uploads.integration.tests.js b/modules/uploads/tests/uploads.integration.tests.js
index e7230cba1..e9e716fa3 100644
--- a/modules/uploads/tests/uploads.integration.tests.js
+++ b/modules/uploads/tests/uploads.integration.tests.js
@@ -16,6 +16,8 @@ describe('Uploads integration tests:', () => {
   let UploadsService;
   let UploadsDataService;
   let UploadRepository;
+  let mongoose;
+  let gridfs;
   let agent;
   let credentials;
   let user;
@@ -30,6 +32,8 @@ describe('Uploads integration tests:', () => {
       UploadsService = (await import(path.resolve('./modules/uploads/services/uploads.service.js'))).default;
       UploadsDataService = (await import(path.resolve('./modules/uploads/services/uploads.data.service.js'))).default;
       UploadRepository = (await import(path.resolve('./modules/uploads/repositories/uploads.repository.js'))).default;
+      mongoose = (await import('mongoose')).default;
+      gridfs = (await import(path.resolve('./lib/services/gridfs.js'))).default;
       agent = request.agent(init.app);
     } catch (err) {
       console.log(err);
@@ -392,6 +396,64 @@ describe('Uploads integration tests:', () => {
   });
 
   describe('Cron', () => {
+    test('sweepUnreferenced sweeps multi-path-unreferenced blobs past the grace window, against a real aggregation pipeline', async () => {
+      try {
+        const kind = 'sweepIntegrationTest';
+        const referencingCollection = 'sweep_test_docs';
+
+        const [scalarRefUpload, arrayRefUpload, multiPathUpload, oldOrphanUpload, youngOrphanUpload] = await Promise.all([
+          gridfs.createFromBuffer(Buffer.from('scalar'), 'sweep-scalar-ref.bin', 'application/octet-stream', { kind }),
+          gridfs.createFromBuffer(Buffer.from('array'), 'sweep-array-ref.bin', 'application/octet-stream', { kind }),
+          gridfs.createFromBuffer(Buffer.from('multi'), 'sweep-multi-path-ref.bin', 'application/octet-stream', { kind }),
+          gridfs.createFromBuffer(Buffer.from('old-orphan'), 'sweep-old-orphan.bin', 'application/octet-stream', { kind }),
+          gridfs.createFromBuffer(Buffer.from('young-orphan'), 'sweep-young-orphan.bin', 'application/octet-stream', { kind }),
+        ]);
+
+        // Referenced via the scalar path `refA`.
+        // Referenced via the array-of-subdocuments path `refs.file`.
+        // Referenced ONLY via `refs.file`, not `refA` — the data-loss guard:
+        // a single-path check would have missed this and deleted it.
+        await mongoose.connection.db.collection(referencingCollection).insertMany([
+          { refA: scalarRefUpload.filename },
+          { refs: [{ file: arrayRefUpload.filename }] },
+          { refs: [{ file: multiPathUpload.filename }] },
+        ]);
+
+        // Backdate the old orphan past the grace window; leave the young
+        // orphan fresh (both unreferenced) — one call exercises both the
+        // "past grace -> deleted" and "within grace -> kept" branches.
+        await mongoose.connection.db
+          .collection('uploads.files')
+          .updateOne({ _id: oldOrphanUpload._id }, { $set: { uploadDate: new Date(Date.now() - 10_000) } });
+
+        const counters = await UploadRepository.sweepUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 5_000);
+
+        expect(counters).toMatchObject({ scanned: 5, referenced: 3, orphaned: 2, deleted: 1, deleteFailed: 0, skippedTooYoung: 1 });
+
+        const [scalarStillThere, arrayStillThere, multiPathStillThere, oldOrphanGone, youngOrphanStillThere] = await Promise.all([
+          UploadRepository.get(scalarRefUpload.filename),
+          UploadRepository.get(arrayRefUpload.filename),
+          UploadRepository.get(multiPathUpload.filename),
+          UploadRepository.get(oldOrphanUpload.filename),
+          UploadRepository.get(youngOrphanUpload.filename),
+        ]);
+
+        expect(scalarStillThere).toBeTruthy();
+        expect(arrayStillThere).toBeTruthy();
+        expect(multiPathStillThere).toBeTruthy();
+        expect(oldOrphanGone).toBeFalsy();
+        expect(youngOrphanStillThere).toBeTruthy();
+
+        await mongoose.connection.db.collection(referencingCollection).deleteMany({});
+        await Promise.all(
+          [scalarRefUpload, arrayRefUpload, multiPathUpload, youngOrphanUpload].map((u) => UploadRepository.remove(u)),
+        );
+      } catch (err) {
+        expect(err).toBeFalsy();
+        console.log(err);
+      }
+    });
+
     test('should be able to purge data not linked to another entity', async () => {
       try {
         const _user2 = { ..._user };
diff --git a/modules/uploads/tests/uploads.repository.unit.tests.js b/modules/uploads/tests/uploads.repository.unit.tests.js
index b05ef0259..d6d89ad41 100644
--- a/modules/uploads/tests/uploads.repository.unit.tests.js
+++ b/modules/uploads/tests/uploads.repository.unit.tests.js
@@ -85,7 +85,7 @@ describe('UploadRepository unit tests:', () => {
       expect(mockBucket.delete).not.toHaveBeenCalled();
       expect(mockLogger.debug).toHaveBeenCalledWith(
         'Upload: remove - no matching file, treating as already removed',
-        { filename: 'gone.png' },
+        { filename: 'gone.png', lookup: { filename: 'gone.png' } },
       );
     });
 
@@ -140,6 +140,13 @@ describe('UploadRepository unit tests:', () => {
       );
     });
 
+    test.each([[-1], [NaN], [Infinity]])('throws for an invalid minAgeMs (%p)', async (minAgeMs) => {
+      await expect(UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], minAgeMs)).rejects.toThrow(
+        'sweepUnreferenced requires a non-negative minAgeMs',
+      );
+      expect(mockBucket.delete).not.toHaveBeenCalled();
+    });
+
     test('a partial sweep (one delete fails) is visible in the counters, not silently reported as a clean sweep', async () => {
       const now = Date.now();
       setReferences([]);

From 72efa2df52b67442553c18e8ef7e974fca671f5f Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Tue, 4 Aug 2026 21:31:50 +0200
Subject: [PATCH 5/8] fix(uploads): validate kind in sweepUnreferenced, verify
 remove() callers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../repositories/uploads.repository.js        |  7 +++++++
 .../tests/uploads.integration.tests.js        | 21 +++++++++++++------
 .../tests/uploads.repository.unit.tests.js    | 12 ++++++++++-
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/modules/uploads/repositories/uploads.repository.js b/modules/uploads/repositories/uploads.repository.js
index 6757d61ba..8bf90fde0 100644
--- a/modules/uploads/repositories/uploads.repository.js
+++ b/modules/uploads/repositories/uploads.repository.js
@@ -208,6 +208,13 @@ const purge = async (kind, collection, key) => {
  *   count that silently looks complete.
  */
 const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
+  // A missing/empty `kind` would make `Uploads.find({ 'metadata.kind': kind })`
+  // below match every upload of every kind instead of quietly matching none —
+  // the same "must fail loudly, not silently look like a clean/empty result"
+  // requirement already applied to `collection` and `paths`.
+  if (typeof kind !== 'string' || kind.length === 0) {
+    throw new AppError('Upload: sweepUnreferenced requires a non-empty kind', { code: 'REPOSITORY_ERROR' });
+  }
   if (!Array.isArray(paths) || paths.length === 0) {
     throw new AppError('Upload: sweepUnreferenced requires at least one reference path', { code: 'REPOSITORY_ERROR' });
   }
diff --git a/modules/uploads/tests/uploads.integration.tests.js b/modules/uploads/tests/uploads.integration.tests.js
index e9e716fa3..76cdb7aa1 100644
--- a/modules/uploads/tests/uploads.integration.tests.js
+++ b/modules/uploads/tests/uploads.integration.tests.js
@@ -419,12 +419,21 @@ describe('Uploads integration tests:', () => {
           { refs: [{ file: multiPathUpload.filename }] },
         ]);
 
-        // Backdate the old orphan past the grace window; leave the young
-        // orphan fresh (both unreferenced) — one call exercises both the
-        // "past grace -> deleted" and "within grace -> kept" branches.
-        await mongoose.connection.db
-          .collection('uploads.files')
-          .updateOne({ _id: oldOrphanUpload._id }, { $set: { uploadDate: new Date(Date.now() - 10_000) } });
+        // Backdate the old orphan past the grace window and pin the young
+        // orphan's uploadDate to right now — both explicit, neither
+        // dependent on how much real wall-clock time elapses between
+        // creating the fixtures above and the sweep call below (a source of
+        // flake under CI load if left to the ambient `createFromBuffer`
+        // timestamp instead). One call exercises both the "past grace ->
+        // deleted" and "within grace -> kept" branches deterministically.
+        await Promise.all([
+          mongoose.connection.db
+            .collection('uploads.files')
+            .updateOne({ _id: oldOrphanUpload._id }, { $set: { uploadDate: new Date(Date.now() - 10_000) } }),
+          mongoose.connection.db
+            .collection('uploads.files')
+            .updateOne({ _id: youngOrphanUpload._id }, { $set: { uploadDate: new Date() } }),
+        ]);
 
         const counters = await UploadRepository.sweepUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 5_000);
 
diff --git a/modules/uploads/tests/uploads.repository.unit.tests.js b/modules/uploads/tests/uploads.repository.unit.tests.js
index d6d89ad41..b2a4c40d9 100644
--- a/modules/uploads/tests/uploads.repository.unit.tests.js
+++ b/modules/uploads/tests/uploads.repository.unit.tests.js
@@ -54,7 +54,10 @@ describe('UploadRepository unit tests:', () => {
         lean: jest.fn().mockReturnThis(),
         cursor: jest.fn(() => asCursor([])),
       })),
-      aggregate: jest.fn(),
+      // No `aggregate` mock here — this file only exercises remove() and
+      // sweepUnreferenced(); the latter uses the raw driver's
+      // db.collection().aggregate(), not Uploads.aggregate() (that's
+      // purge()'s path, untested in this file).
     };
 
     jest.unstable_mockModule('mongoose', () => ({
@@ -126,6 +129,13 @@ describe('UploadRepository unit tests:', () => {
     const kind = 'htmlSnapshot';
     const collection = 'histories';
 
+    test.each([[undefined], [null], ['']])('throws for a missing/empty kind (%p) instead of silently matching every kind', async (badKind) => {
+      await expect(UploadRepository.sweepUnreferenced(badKind, collection, ['snapshot'], 1000)).rejects.toThrow(
+        'sweepUnreferenced requires a non-empty kind',
+      );
+      expect(mockBucket.delete).not.toHaveBeenCalled();
+    });
+
     test('throws when paths is missing/empty', async () => {
       await expect(UploadRepository.sweepUnreferenced(kind, collection, [], 1000)).rejects.toThrow(
         'sweepUnreferenced requires at least one reference path',

From 5e0054bd83da1558f85df6085e1015ac1bbef933 Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Tue, 4 Aug 2026 21:43:51 +0200
Subject: [PATCH 6/8] fix(uploads): address CodeRabbit review on PR #4017
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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.
---
 .../repositories/uploads.repository.js        | 56 ++++++++++++++-----
 .../tests/uploads.integration.tests.js        | 43 ++++++++------
 .../tests/uploads.repository.unit.tests.js    | 17 ++++--
 3 files changed, 79 insertions(+), 37 deletions(-)

diff --git a/modules/uploads/repositories/uploads.repository.js b/modules/uploads/repositories/uploads.repository.js
index 8bf90fde0..e3e3827b8 100644
--- a/modules/uploads/repositories/uploads.repository.js
+++ b/modules/uploads/repositories/uploads.repository.js
@@ -69,16 +69,29 @@ const update = (id, update) => Uploads.findOneAndUpdate({ _id: id }, update, { r
 const remove = async (upload) => {
   const lookup = upload ?? null;
   const filename = upload?.filename;
-  if (!upload || !upload._id) upload = await Uploads.findOne({ filename }).exec();
-  if (!upload) {
-    // `filename` is undefined (not just falsy-but-present) whenever `upload`
-    // itself was null/undefined/{} — logging the ORIGINAL lookup argument
-    // (captured before the findOne reassignment above overwrites `upload`)
-    // keeps this debug line useful in that case instead of showing
-    // `filename: undefined` with no other context.
+  // `filename` is undefined (not just falsy-but-present) whenever `upload`
+  // itself was null/undefined/{} — logging the ORIGINAL lookup argument
+  // (captured above, before it may get reassigned below) keeps this debug
+  // line useful in that case instead of showing `filename: undefined` with
+  // no other context.
+  const noOp = () => {
     logger.debug('Upload: remove - no matching file, treating as already removed', { filename: filename ?? null, lookup });
     return { deletedCount: 0, notFound: true };
+  };
+  if (!upload || !upload._id) {
+    // A lookup key that carries neither `_id` nor a string `filename` can
+    // never resolve to a real record — short-circuit BEFORE querying rather
+    // than asking Mongoose to filter on `{ filename: undefined }`. This
+    // repo's own Mongoose version was verified end-to-end to handle that
+    // filter safely (returns no match, never an arbitrary document), but
+    // exact handling of an undefined-valued filter key has shifted across
+    // Mongoose/MongoDB-driver versions — skipping the query entirely removes
+    // any dependence on that behavior for a lookup that was never going to
+    // resolve to anything anyway.
+    if (typeof filename !== 'string' || filename.length === 0) return noOp();
+    upload = await Uploads.findOne({ filename }).exec();
   }
+  if (!upload) return noOp();
   try {
     const unlinked = await bucket.delete(upload._id);
     return unlinked;
@@ -233,8 +246,13 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
     throw new AppError(`Upload: sweepUnreferenced target collection "${collection}" does not exist`, { code: 'REPOSITORY_ERROR' });
   }
 
-  // Normalises a reference path's value to an array: missing/null -> [],
-  // an array field (e.g. across subdocuments) -> itself, a scalar -> [value].
+  /**
+   * @desc Normalises a reference path's value to an array: missing/null ->
+   *  [], an array field (e.g. across subdocuments) -> itself, a scalar ->
+   *  [value].
+   * @param {String} path - dot-path on a `collection` document
+   * @return {Object} an aggregation expression resolving to an array of reference values
+   */
   const toArrayExpr = (path) => {
     const field = `$${path}`;
     return {
@@ -248,10 +266,17 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
   };
 
   const referenced = new Set();
-  const referenceCursor = mongoose.connection.db.collection(collection).aggregate([
-    { $project: { _id: 0, refs: { $concatArrays: paths.map(toArrayExpr) } } },
-    { $match: { 'refs.0': { $exists: true } } },
-  ]);
+  // `noCursorTimeout` — this streams the WHOLE collection at cron/batch
+  // scale, not request-path scale; without it, MongoDB closes an idle
+  // cursor after 10 minutes and a slow pass over a large `collection` fails
+  // mid-run with the counters accumulated so far lost.
+  const referenceCursor = mongoose.connection.db.collection(collection).aggregate(
+    [
+      { $project: { _id: 0, refs: { $concatArrays: paths.map(toArrayExpr) } } },
+      { $match: { 'refs.0': { $exists: true } } },
+    ],
+    { noCursorTimeout: true },
+  );
   for await (const doc of referenceCursor) {
     for (const filename of doc.refs) {
       if (typeof filename === 'string') referenced.add(filename);
@@ -265,7 +290,10 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
   let deleteFailed = 0;
   let skippedTooYoung = 0;
 
-  const candidateCursor = Uploads.find({ 'metadata.kind': kind }).select('filename uploadDate').lean().cursor();
+  const candidateCursor = Uploads.find({ 'metadata.kind': kind }, null, { noCursorTimeout: true })
+    .select('filename uploadDate')
+    .lean()
+    .cursor();
   for await (const candidate of candidateCursor) {
     scanned += 1;
     if (referenced.has(candidate.filename)) {
diff --git a/modules/uploads/tests/uploads.integration.tests.js b/modules/uploads/tests/uploads.integration.tests.js
index 76cdb7aa1..51b99747a 100644
--- a/modules/uploads/tests/uploads.integration.tests.js
+++ b/modules/uploads/tests/uploads.integration.tests.js
@@ -397,10 +397,11 @@ describe('Uploads integration tests:', () => {
 
   describe('Cron', () => {
     test('sweepUnreferenced sweeps multi-path-unreferenced blobs past the grace window, against a real aggregation pipeline', async () => {
+      // Declared outside the try block — the `finally` cleanup below needs
+      // them too, and a `const` scoped to `try` is not visible in `finally`.
+      const kind = 'sweepIntegrationTest';
+      const referencingCollection = 'sweep_test_docs';
       try {
-        const kind = 'sweepIntegrationTest';
-        const referencingCollection = 'sweep_test_docs';
-
         const [scalarRefUpload, arrayRefUpload, multiPathUpload, oldOrphanUpload, youngOrphanUpload] = await Promise.all([
           gridfs.createFromBuffer(Buffer.from('scalar'), 'sweep-scalar-ref.bin', 'application/octet-stream', { kind }),
           gridfs.createFromBuffer(Buffer.from('array'), 'sweep-array-ref.bin', 'application/octet-stream', { kind }),
@@ -419,23 +420,27 @@ describe('Uploads integration tests:', () => {
           { refs: [{ file: multiPathUpload.filename }] },
         ]);
 
-        // Backdate the old orphan past the grace window and pin the young
-        // orphan's uploadDate to right now — both explicit, neither
-        // dependent on how much real wall-clock time elapses between
-        // creating the fixtures above and the sweep call below (a source of
-        // flake under CI load if left to the ambient `createFromBuffer`
-        // timestamp instead). One call exercises both the "past grace ->
-        // deleted" and "within grace -> kept" branches deterministically.
+        // Backdate the old orphan well past the grace window and pin the
+        // young orphan's uploadDate to right now — explicit, not dependent
+        // on how much real wall-clock time elapses between creating the
+        // fixtures above and the sweep call below (a source of flake under
+        // CI load if left to the ambient `createFromBuffer` timestamp
+        // instead). The 10-minute backdate / 5-minute grace-window margin
+        // (rather than a tight few-second gap) absorbs the sweep's own
+        // runtime (listCollections + aggregation + candidate streaming)
+        // without the young orphan risking tipping over the threshold.
+        // One call exercises both the "past grace -> deleted" and "within
+        // grace -> kept" branches deterministically.
         await Promise.all([
           mongoose.connection.db
             .collection('uploads.files')
-            .updateOne({ _id: oldOrphanUpload._id }, { $set: { uploadDate: new Date(Date.now() - 10_000) } }),
+            .updateOne({ _id: oldOrphanUpload._id }, { $set: { uploadDate: new Date(Date.now() - 600_000) } }),
           mongoose.connection.db
             .collection('uploads.files')
             .updateOne({ _id: youngOrphanUpload._id }, { $set: { uploadDate: new Date() } }),
         ]);
 
-        const counters = await UploadRepository.sweepUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 5_000);
+        const counters = await UploadRepository.sweepUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 300_000);
 
         expect(counters).toMatchObject({ scanned: 5, referenced: 3, orphaned: 2, deleted: 1, deleteFailed: 0, skippedTooYoung: 1 });
 
@@ -452,14 +457,18 @@ describe('Uploads integration tests:', () => {
         expect(multiPathStillThere).toBeTruthy();
         expect(oldOrphanGone).toBeFalsy();
         expect(youngOrphanStillThere).toBeTruthy();
-
-        await mongoose.connection.db.collection(referencingCollection).deleteMany({});
-        await Promise.all(
-          [scalarRefUpload, arrayRefUpload, multiPathUpload, youngOrphanUpload].map((u) => UploadRepository.remove(u)),
-        );
       } catch (err) {
         expect(err).toBeFalsy();
         console.log(err);
+      } finally {
+        // Cleanup must run even when an assertion above fails — leftover
+        // blobs of this `kind` would make the next run's `scanned` count
+        // wrong and fail it permanently. Dropping the referencing docs first
+        // then sweeping with minAgeMs=0 removes every remaining fixture
+        // blob regardless of which assertions passed, without needing to
+        // track individual upload docs here.
+        await mongoose.connection.db.collection(referencingCollection).deleteMany({});
+        await UploadRepository.sweepUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 0).catch((err) => console.log(err));
       }
     });
 
diff --git a/modules/uploads/tests/uploads.repository.unit.tests.js b/modules/uploads/tests/uploads.repository.unit.tests.js
index b2a4c40d9..45fe243b5 100644
--- a/modules/uploads/tests/uploads.repository.unit.tests.js
+++ b/modules/uploads/tests/uploads.repository.unit.tests.js
@@ -93,14 +93,11 @@ describe('UploadRepository unit tests:', () => {
     });
 
     test.each([[null], [undefined], [{}]])(
-      'does not query with an unbound filter when called with %p (findOne receives filename: undefined, not a stripped-key match-all)',
+      'short-circuits to the no-op WITHOUT querying when called with %p (a lookup key with neither _id nor a string filename can never resolve)',
       async (arg) => {
-        const findOneExec = jest.fn().mockResolvedValue(null);
-        mockUploadsModel.findOne.mockReturnValue({ exec: findOneExec });
-
         const result = await UploadRepository.remove(arg);
 
-        expect(mockUploadsModel.findOne).toHaveBeenCalledWith({ filename: undefined });
+        expect(mockUploadsModel.findOne).not.toHaveBeenCalled();
         expect(result).toEqual({ deletedCount: 0, notFound: true });
         expect(mockBucket.delete).not.toHaveBeenCalled();
       },
@@ -224,11 +221,19 @@ describe('UploadRepository unit tests:', () => {
       const now = Date.now();
       // Only the second path (`snapshots.html`) references it — a
       // single-path check would have missed this and deleted a live blob.
-      setReferences([{ refs: ['multi-ref.png'] }]);
+      // Capture the aggregate call directly (instead of setReferences(),
+      // which ignores its pipeline argument) so this test can also assert
+      // BOTH paths actually reached the pipeline — otherwise a regression
+      // that built the pipeline from only the first path would still pass.
+      const aggregate = jest.fn(() => asCursor([{ refs: ['multi-ref.png'] }]));
+      mockDb.collection.mockReturnValue({ aggregate });
       setCandidates([{ _id: 'multi1', filename: 'multi-ref.png', uploadDate: new Date(now - 120_000) }]);
 
       const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['avatar', 'snapshots.html'], 60_000);
 
+      const pipeline = JSON.stringify(aggregate.mock.calls[0][0]);
+      expect(pipeline).toContain('$avatar');
+      expect(pipeline).toContain('$snapshots.html');
       expect(mockBucket.delete).not.toHaveBeenCalled();
       expect(counters).toMatchObject({ scanned: 1, referenced: 1, orphaned: 0, deleted: 0 });
     });

From 8a3e246c7eeeb891982c798490e08668deffabcc Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Tue, 4 Aug 2026 23:43:11 +0200
Subject: [PATCH 7/8] refactor(uploads): rename sweep to purgeUnreferenced for
 naming consistency with purge()

Renames sweepUnreferenced -> purgeUnreferenced and its minAgeMs param ->
graceMs (positional). No behavior change. Function, exports, tests, and
JSDoc identifier mentions all updated together.
---
 .../repositories/uploads.repository.js        | 22 ++++++------
 .../tests/uploads.integration.tests.js        |  8 ++---
 .../tests/uploads.repository.unit.tests.js    | 36 +++++++++----------
 3 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/modules/uploads/repositories/uploads.repository.js b/modules/uploads/repositories/uploads.repository.js
index e3e3827b8..0b78aaa24 100644
--- a/modules/uploads/repositories/uploads.repository.js
+++ b/modules/uploads/repositories/uploads.repository.js
@@ -198,7 +198,7 @@ const purge = async (kind, collection, key) => {
  *
  * Known trade-off (accepted, not fixed here): the reference-Set snapshot is
  * taken at the START of the run, then read against for the whole candidate
- * scan. A blob whose age already exceeds `minAgeMs` — i.e. NOT protected by
+ * scan. A blob whose age already exceeds `graceMs` — i.e. NOT protected by
  * the grace window, which only covers the gap between a blob's own write and
  * its first reference — that gets referenced by a brand-new document for the
  * FIRST TIME after the snapshot but before the scan reaches it will still
@@ -214,25 +214,25 @@ const purge = async (kind, collection, key) => {
  * @param {String} kind - metadata.kind to sweep (e.g. 'htmlSnapshot')
  * @param {String} collection - name of the collection to check references against
  * @param {String[]} paths - dot-paths on `collection` docs that may reference an upload's filename
- * @param {Number} minAgeMs - minimum age (ms, from GridFS `uploadDate`) before an unreferenced blob is eligible for deletion
+ * @param {Number} graceMs - minimum age (ms, from GridFS `uploadDate`) before an unreferenced blob is eligible for deletion
  * @return {Object} counters — { scanned, referenced, orphaned, deleted, deleteFailed, skippedTooYoung }.
  *   `deleteFailed` lets a caller detect a partial sweep (some eligible blobs
  *   left undeleted after a transient bucket error) instead of a `deleted`
  *   count that silently looks complete.
  */
-const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
+const purgeUnreferenced = async (kind, collection, paths, graceMs) => {
   // A missing/empty `kind` would make `Uploads.find({ 'metadata.kind': kind })`
   // below match every upload of every kind instead of quietly matching none —
   // the same "must fail loudly, not silently look like a clean/empty result"
   // requirement already applied to `collection` and `paths`.
   if (typeof kind !== 'string' || kind.length === 0) {
-    throw new AppError('Upload: sweepUnreferenced requires a non-empty kind', { code: 'REPOSITORY_ERROR' });
+    throw new AppError('Upload: purgeUnreferenced requires a non-empty kind', { code: 'REPOSITORY_ERROR' });
   }
   if (!Array.isArray(paths) || paths.length === 0) {
-    throw new AppError('Upload: sweepUnreferenced requires at least one reference path', { code: 'REPOSITORY_ERROR' });
+    throw new AppError('Upload: purgeUnreferenced requires at least one reference path', { code: 'REPOSITORY_ERROR' });
   }
-  if (!Number.isFinite(minAgeMs) || minAgeMs < 0) {
-    throw new AppError('Upload: sweepUnreferenced requires a non-negative minAgeMs', { code: 'REPOSITORY_ERROR' });
+  if (!Number.isFinite(graceMs) || graceMs < 0) {
+    throw new AppError('Upload: purgeUnreferenced requires a non-negative graceMs', { code: 'REPOSITORY_ERROR' });
   }
 
   /* A mistyped `collection` name would make the aggregation below return an
@@ -243,7 +243,7 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
     .listCollections({ name: collection }, { nameOnly: true })
     .hasNext();
   if (!collectionExists) {
-    throw new AppError(`Upload: sweepUnreferenced target collection "${collection}" does not exist`, { code: 'REPOSITORY_ERROR' });
+    throw new AppError(`Upload: purgeUnreferenced target collection "${collection}" does not exist`, { code: 'REPOSITORY_ERROR' });
   }
 
   /**
@@ -303,7 +303,7 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
     // Missing uploadDate is treated as "unknown age" -> never eligible for
     // deletion (fail closed, not open) rather than as "very old".
     const ageMs = candidate.uploadDate ? now - new Date(candidate.uploadDate).getTime() : -1;
-    if (ageMs < minAgeMs) {
+    if (ageMs < graceMs) {
       skippedTooYoung += 1;
       continue;
     }
@@ -316,7 +316,7 @@ const sweepUnreferenced = async (kind, collection, paths, minAgeMs) => {
       // record of which file failed. `deleteFailed` lets a caller detect a
       // partial sweep programmatically; the log gives the "which one".
       deleteFailed += 1;
-      logger.error('Upload: sweepUnreferenced - delete failed', {
+      logger.error('Upload: purgeUnreferenced - delete failed', {
         filename: candidate.filename,
         kind,
         error: err?.message,
@@ -344,5 +344,5 @@ export default {
   remove,
   deleteMany,
   purge,
-  sweepUnreferenced,
+  purgeUnreferenced,
 };
diff --git a/modules/uploads/tests/uploads.integration.tests.js b/modules/uploads/tests/uploads.integration.tests.js
index 51b99747a..61253db85 100644
--- a/modules/uploads/tests/uploads.integration.tests.js
+++ b/modules/uploads/tests/uploads.integration.tests.js
@@ -396,7 +396,7 @@ describe('Uploads integration tests:', () => {
   });
 
   describe('Cron', () => {
-    test('sweepUnreferenced sweeps multi-path-unreferenced blobs past the grace window, against a real aggregation pipeline', async () => {
+    test('purgeUnreferenced sweeps multi-path-unreferenced blobs past the grace window, against a real aggregation pipeline', async () => {
       // Declared outside the try block — the `finally` cleanup below needs
       // them too, and a `const` scoped to `try` is not visible in `finally`.
       const kind = 'sweepIntegrationTest';
@@ -440,7 +440,7 @@ describe('Uploads integration tests:', () => {
             .updateOne({ _id: youngOrphanUpload._id }, { $set: { uploadDate: new Date() } }),
         ]);
 
-        const counters = await UploadRepository.sweepUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 300_000);
+        const counters = await UploadRepository.purgeUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 300_000);
 
         expect(counters).toMatchObject({ scanned: 5, referenced: 3, orphaned: 2, deleted: 1, deleteFailed: 0, skippedTooYoung: 1 });
 
@@ -464,11 +464,11 @@ describe('Uploads integration tests:', () => {
         // Cleanup must run even when an assertion above fails — leftover
         // blobs of this `kind` would make the next run's `scanned` count
         // wrong and fail it permanently. Dropping the referencing docs first
-        // then sweeping with minAgeMs=0 removes every remaining fixture
+        // then sweeping with graceMs=0 removes every remaining fixture
         // blob regardless of which assertions passed, without needing to
         // track individual upload docs here.
         await mongoose.connection.db.collection(referencingCollection).deleteMany({});
-        await UploadRepository.sweepUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 0).catch((err) => console.log(err));
+        await UploadRepository.purgeUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 0).catch((err) => console.log(err));
       }
     });
 
diff --git a/modules/uploads/tests/uploads.repository.unit.tests.js b/modules/uploads/tests/uploads.repository.unit.tests.js
index 45fe243b5..290d79644 100644
--- a/modules/uploads/tests/uploads.repository.unit.tests.js
+++ b/modules/uploads/tests/uploads.repository.unit.tests.js
@@ -5,7 +5,7 @@ import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globa
 
 /**
  * Unit tests for uploads.repository.js — remove() no-op semantics and
- * sweepUnreferenced() multi-path unreferenced-blob sweep.
+ * purgeUnreferenced() multi-path unreferenced-blob sweep.
  */
 describe('UploadRepository unit tests:', () => {
   let UploadRepository;
@@ -55,7 +55,7 @@ describe('UploadRepository unit tests:', () => {
         cursor: jest.fn(() => asCursor([])),
       })),
       // No `aggregate` mock here — this file only exercises remove() and
-      // sweepUnreferenced(); the latter uses the raw driver's
+      // purgeUnreferenced(); the latter uses the raw driver's
       // db.collection().aggregate(), not Uploads.aggregate() (that's
       // purge()'s path, untested in this file).
     };
@@ -122,34 +122,34 @@ describe('UploadRepository unit tests:', () => {
     });
   });
 
-  describe('sweepUnreferenced', () => {
+  describe('purgeUnreferenced', () => {
     const kind = 'htmlSnapshot';
     const collection = 'histories';
 
     test.each([[undefined], [null], ['']])('throws for a missing/empty kind (%p) instead of silently matching every kind', async (badKind) => {
-      await expect(UploadRepository.sweepUnreferenced(badKind, collection, ['snapshot'], 1000)).rejects.toThrow(
-        'sweepUnreferenced requires a non-empty kind',
+      await expect(UploadRepository.purgeUnreferenced(badKind, collection, ['snapshot'], 1000)).rejects.toThrow(
+        'purgeUnreferenced requires a non-empty kind',
       );
       expect(mockBucket.delete).not.toHaveBeenCalled();
     });
 
     test('throws when paths is missing/empty', async () => {
-      await expect(UploadRepository.sweepUnreferenced(kind, collection, [], 1000)).rejects.toThrow(
-        'sweepUnreferenced requires at least one reference path',
+      await expect(UploadRepository.purgeUnreferenced(kind, collection, [], 1000)).rejects.toThrow(
+        'purgeUnreferenced requires at least one reference path',
       );
     });
 
     test('throws when the target collection does not exist (fails loudly, not a silent empty result)', async () => {
       mockDb.listCollections.mockReturnValue({ hasNext: jest.fn().mockResolvedValue(false) });
 
-      await expect(UploadRepository.sweepUnreferenced(kind, 'typo_collection', ['snapshot'], 1000)).rejects.toThrow(
+      await expect(UploadRepository.purgeUnreferenced(kind, 'typo_collection', ['snapshot'], 1000)).rejects.toThrow(
         'target collection "typo_collection" does not exist',
       );
     });
 
-    test.each([[-1], [NaN], [Infinity]])('throws for an invalid minAgeMs (%p)', async (minAgeMs) => {
-      await expect(UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], minAgeMs)).rejects.toThrow(
-        'sweepUnreferenced requires a non-negative minAgeMs',
+    test.each([[-1], [NaN], [Infinity]])('throws for an invalid graceMs (%p)', async (graceMs) => {
+      await expect(UploadRepository.purgeUnreferenced(kind, collection, ['snapshot'], graceMs)).rejects.toThrow(
+        'purgeUnreferenced requires a non-negative graceMs',
       );
       expect(mockBucket.delete).not.toHaveBeenCalled();
     });
@@ -160,11 +160,11 @@ describe('UploadRepository unit tests:', () => {
       setCandidates([{ _id: 'flaky1', filename: 'flaky.png', uploadDate: new Date(now - 120_000) }]);
       mockBucket.delete.mockRejectedValueOnce(new Error('bucket unreachable'));
 
-      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
+      const counters = await UploadRepository.purgeUnreferenced(kind, collection, ['snapshot'], 60_000);
 
       expect(counters).toMatchObject({ scanned: 1, orphaned: 1, deleted: 0, deleteFailed: 1 });
       expect(mockLogger.error).toHaveBeenCalledWith(
-        'Upload: sweepUnreferenced - delete failed',
+        'Upload: purgeUnreferenced - delete failed',
         expect.objectContaining({ filename: 'flaky.png', kind }),
       );
     });
@@ -173,7 +173,7 @@ describe('UploadRepository unit tests:', () => {
       const now = Date.now();
       setCandidates([{ _id: 'young1', filename: 'young.png', uploadDate: new Date(now - 1000) }]);
 
-      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
+      const counters = await UploadRepository.purgeUnreferenced(kind, collection, ['snapshot'], 60_000);
 
       expect(mockBucket.delete).not.toHaveBeenCalled();
       expect(counters).toMatchObject({ scanned: 1, orphaned: 1, deleted: 0, skippedTooYoung: 1 });
@@ -184,7 +184,7 @@ describe('UploadRepository unit tests:', () => {
       setReferences([]); // nothing references it
       setCandidates([{ _id: 'old1', filename: 'orphan.png', uploadDate: new Date(now - 120_000) }]);
 
-      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
+      const counters = await UploadRepository.purgeUnreferenced(kind, collection, ['snapshot'], 60_000);
 
       expect(mockBucket.delete).toHaveBeenCalledWith('old1');
       expect(counters).toMatchObject({ scanned: 1, orphaned: 1, deleted: 1, skippedTooYoung: 0 });
@@ -195,7 +195,7 @@ describe('UploadRepository unit tests:', () => {
       setReferences([{ refs: ['referenced.png'] }]);
       setCandidates([{ _id: 'ref1', filename: 'referenced.png', uploadDate: new Date(now - 120_000) }]);
 
-      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshot'], 60_000);
+      const counters = await UploadRepository.purgeUnreferenced(kind, collection, ['snapshot'], 60_000);
 
       expect(mockBucket.delete).not.toHaveBeenCalled();
       expect(counters).toMatchObject({ scanned: 1, referenced: 1, orphaned: 0, deleted: 0 });
@@ -211,7 +211,7 @@ describe('UploadRepository unit tests:', () => {
         { _id: 'sub2', filename: 'sub2.png', uploadDate: new Date(now - 120_000) },
       ]);
 
-      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['snapshots.html'], 60_000);
+      const counters = await UploadRepository.purgeUnreferenced(kind, collection, ['snapshots.html'], 60_000);
 
       expect(mockBucket.delete).not.toHaveBeenCalled();
       expect(counters).toMatchObject({ scanned: 2, referenced: 2, orphaned: 0, deleted: 0 });
@@ -229,7 +229,7 @@ describe('UploadRepository unit tests:', () => {
       mockDb.collection.mockReturnValue({ aggregate });
       setCandidates([{ _id: 'multi1', filename: 'multi-ref.png', uploadDate: new Date(now - 120_000) }]);
 
-      const counters = await UploadRepository.sweepUnreferenced(kind, collection, ['avatar', 'snapshots.html'], 60_000);
+      const counters = await UploadRepository.purgeUnreferenced(kind, collection, ['avatar', 'snapshots.html'], 60_000);
 
       const pipeline = JSON.stringify(aggregate.mock.calls[0][0]);
       expect(pipeline).toContain('$avatar');

From b7606b207c9ecfb48785297df851cac9ea88afa7 Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Tue, 4 Aug 2026 23:58:50 +0200
Subject: [PATCH 8/8] fix(uploads): address second CodeRabbit pass on PR #4017
 (post-rename)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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.
---
 .../uploads/repositories/uploads.repository.js    | 15 +++++++++++++--
 .../uploads/tests/uploads.integration.tests.js    |  2 +-
 .../tests/uploads.repository.unit.tests.js        | 11 +++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/modules/uploads/repositories/uploads.repository.js b/modules/uploads/repositories/uploads.repository.js
index 0b78aaa24..e3ee5b938 100644
--- a/modules/uploads/repositories/uploads.repository.js
+++ b/modules/uploads/repositories/uploads.repository.js
@@ -74,6 +74,11 @@ const remove = async (upload) => {
   // (captured above, before it may get reassigned below) keeps this debug
   // line useful in that case instead of showing `filename: undefined` with
   // no other context.
+  /**
+   * @desc Builds the no-op response for `remove()` when no matching file was
+   *  found, logging the original lookup argument at debug first.
+   * @returns {Object} the no-op marker { deletedCount: 0, notFound: true }
+   */
   const noOp = () => {
     logger.debug('Upload: remove - no matching file, treating as already removed', { filename: filename ?? null, lookup });
     return { deletedCount: 0, notFound: true };
@@ -215,7 +220,7 @@ const purge = async (kind, collection, key) => {
  * @param {String} collection - name of the collection to check references against
  * @param {String[]} paths - dot-paths on `collection` docs that may reference an upload's filename
  * @param {Number} graceMs - minimum age (ms, from GridFS `uploadDate`) before an unreferenced blob is eligible for deletion
- * @return {Object} counters — { scanned, referenced, orphaned, deleted, deleteFailed, skippedTooYoung }.
+ * @returns {Promise<Object>} counters — { scanned, referenced, orphaned, deleted, deleteFailed, skippedTooYoung }.
  *   `deleteFailed` lets a caller detect a partial sweep (some eligible blobs
  *   left undeleted after a transient bucket error) instead of a `deleted`
  *   count that silently looks complete.
@@ -231,6 +236,12 @@ const purgeUnreferenced = async (kind, collection, paths, graceMs) => {
   if (!Array.isArray(paths) || paths.length === 0) {
     throw new AppError('Upload: purgeUnreferenced requires at least one reference path', { code: 'REPOSITORY_ERROR' });
   }
+  // A non-string or empty path would build a nonsensical field reference
+  // (`toArrayExpr` does `` `$${path}` `` unconditionally) rather than failing
+  // clearly — same "fail loudly" requirement as `kind`/`collection`/`graceMs`.
+  if (!paths.every((path) => typeof path === 'string' && path.length > 0)) {
+    throw new AppError('Upload: purgeUnreferenced requires every reference path to be a non-empty string', { code: 'REPOSITORY_ERROR' });
+  }
   if (!Number.isFinite(graceMs) || graceMs < 0) {
     throw new AppError('Upload: purgeUnreferenced requires a non-negative graceMs', { code: 'REPOSITORY_ERROR' });
   }
@@ -251,7 +262,7 @@ const purgeUnreferenced = async (kind, collection, paths, graceMs) => {
    *  [], an array field (e.g. across subdocuments) -> itself, a scalar ->
    *  [value].
    * @param {String} path - dot-path on a `collection` document
-   * @return {Object} an aggregation expression resolving to an array of reference values
+   * @returns {Object} an aggregation expression resolving to an array of reference values
    */
   const toArrayExpr = (path) => {
     const field = `$${path}`;
diff --git a/modules/uploads/tests/uploads.integration.tests.js b/modules/uploads/tests/uploads.integration.tests.js
index 61253db85..8c04869bf 100644
--- a/modules/uploads/tests/uploads.integration.tests.js
+++ b/modules/uploads/tests/uploads.integration.tests.js
@@ -468,7 +468,7 @@ describe('Uploads integration tests:', () => {
         // blob regardless of which assertions passed, without needing to
         // track individual upload docs here.
         await mongoose.connection.db.collection(referencingCollection).deleteMany({});
-        await UploadRepository.purgeUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 0).catch((err) => console.log(err));
+        await UploadRepository.purgeUnreferenced(kind, referencingCollection, ['refA', 'refs.file'], 0);
       }
     });
 
diff --git a/modules/uploads/tests/uploads.repository.unit.tests.js b/modules/uploads/tests/uploads.repository.unit.tests.js
index 290d79644..449f3ff1d 100644
--- a/modules/uploads/tests/uploads.repository.unit.tests.js
+++ b/modules/uploads/tests/uploads.repository.unit.tests.js
@@ -139,6 +139,17 @@ describe('UploadRepository unit tests:', () => {
       );
     });
 
+    test.each([[[null]], [[undefined]], [[42]], [['']]])(
+      'throws when paths contains a non-empty-string element (%p) instead of building a nonsensical field reference',
+      async (badPaths) => {
+        await expect(UploadRepository.purgeUnreferenced(kind, collection, badPaths, 1000)).rejects.toThrow(
+          'purgeUnreferenced requires every reference path to be a non-empty string',
+        );
+        expect(mockDb.collection).not.toHaveBeenCalled();
+        expect(mockBucket.delete).not.toHaveBeenCalled();
+      },
+    );
+
     test('throws when the target collection does not exist (fails loudly, not a silent empty result)', async () => {
       mockDb.listCollections.mockReturnValue({ hasNext: jest.fn().mockResolvedValue(false) });