From 824bb43c058e0925791d5ae578a1432b27d2e4ea Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Thu, 20 Aug 2026 09:05:46 +0300 Subject: [PATCH] fix(collab): sweepBlobs must not overstate reclaimed bytes on delete() failure sweepBlobs computed how many deletes succeeded but discarded that count and returned the full planned reclaimBytes regardless of whether any store.delete() call reported failure, overstating freed storage while the undeleted blob kept consuming space. planBlobSweep now records each dropped hash's byte length and sweepBlobs sums only the bytes for hashes that were actually deleted. --- .changeset/blob-gc-overstated-reclaim.md | 7 ++++++ packages/collab/src/geometry/gc.ts | 26 ++++++++++++++++---- packages/collab/test/blob-gc.test.ts | 30 ++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 .changeset/blob-gc-overstated-reclaim.md diff --git a/.changeset/blob-gc-overstated-reclaim.md b/.changeset/blob-gc-overstated-reclaim.md new file mode 100644 index 0000000000..2cdaad7c80 --- /dev/null +++ b/.changeset/blob-gc-overstated-reclaim.md @@ -0,0 +1,7 @@ +--- +"@ifc-lite/collab": patch +--- + +Fix `sweepBlobs` reporting a blob as reclaimed even when the underlying `store.delete()` call failed. + +`sweepBlobs` computed how many deletes actually succeeded but then discarded that count and returned `decision.reclaimBytes` unconditionally — the full byte total `planBlobSweep` had planned to free, regardless of whether any individual `delete()` call reported failure (a remote backend 404, a race with another sweep, a transient error). A caller using the return value for storage-capacity accounting would believe more space was freed than actually was, while the undeleted blob kept consuming storage. `planBlobSweep` now records each dropped hash's byte length on the `SweepDecision`, and `sweepBlobs` sums only the bytes for hashes whose `delete()` actually returned `true`. diff --git a/packages/collab/src/geometry/gc.ts b/packages/collab/src/geometry/gc.ts index 4f9eb7cdf3..59a8ee1e40 100644 --- a/packages/collab/src/geometry/gc.ts +++ b/packages/collab/src/geometry/gc.ts @@ -114,6 +114,12 @@ export interface SweepDecision { drop: BlobHash[]; /** Bytes that will be reclaimed once `drop` is processed. May be undefined when the store doesn't expose sizes. */ reclaimBytes: number; + /** + * Per-hash byte size for every entry in `drop`, so `sweepBlobs` can + * report the bytes actually reclaimed rather than the bytes merely + * planned — a `store.delete()` failure must not be counted. + */ + dropByteLengths: Record; } /** @@ -134,6 +140,7 @@ export async function planBlobSweep( const now = options.now ? options.now() : Date.now(); const all = await store.list(); const drop: BlobHash[] = []; + const dropByteLengths: Record = {}; let reclaim = 0; for (const hash of all) { if (referenced.has(hash)) continue; @@ -147,16 +154,18 @@ export async function planBlobSweep( // so the grace window keeps protecting in-flight uploads. if (!options.sweepUnknownAge) continue; drop.push(hash); + dropByteLengths[hash] = meta.byteLength; reclaim += meta.byteLength; continue; } const ageMs = Math.max(0, now - new Date(meta.uploadedAt).getTime()); if (ageMs >= epochMs) { drop.push(hash); + dropByteLengths[hash] = meta.byteLength; reclaim += meta.byteLength; } } - return { drop, reclaimBytes: reclaim }; + return { drop, reclaimBytes: reclaim, dropByteLengths }; } async function metaFromGet(store: BlobStore, hash: BlobHash): Promise { @@ -177,13 +186,20 @@ async function metaFromGet(store: BlobStore, hash: BlobHash): Promise { let freed = 0; for (const hash of decision.drop) { const ok = await store.delete(hash); - if (ok) freed += 1; + if (ok) freed += decision.dropByteLengths[hash] ?? 0; } - void freed; - return decision.reclaimBytes; + return freed; } diff --git a/packages/collab/test/blob-gc.test.ts b/packages/collab/test/blob-gc.test.ts index 664ab66a08..1c2fbc6f1b 100644 --- a/packages/collab/test/blob-gc.test.ts +++ b/packages/collab/test/blob-gc.test.ts @@ -91,4 +91,34 @@ describe('blob GC', () => { expect(decision.drop).toEqual([orphan.hash]); expect(decision.reclaimBytes).toBe(3); }); + + it('does not report bytes as reclaimed when the underlying delete() fails', async () => { + const inner = new MemoryBlobStore(); + const a = await inner.put(new Uint8Array([1, 1, 1])); + const b = await inner.put(new Uint8Array([2, 2, 2, 2])); + + // A store whose delete() reports failure for one of the two blobs — + // e.g. a remote backend that 404s or races with another sweep. + const flaky = { + put: inner.put.bind(inner), + get: inner.get.bind(inner), + has: inner.has.bind(inner), + list: inner.list.bind(inner), + stat: inner.stat.bind(inner), + delete: async (hash: string) => (hash === a.hash ? false : inner.delete(hash)), + }; + + const referenced = new Set(); + const decision = await planBlobSweep(flaky, referenced, { epochMs: 0 }); + expect(decision.drop.sort()).toEqual([a.hash, b.hash].sort()); + expect(decision.reclaimBytes).toBe(7); + + const freed = await sweepBlobs(flaky, decision); + + // Only `b` (4 bytes) was actually deleted; `a` (3 bytes) failed to + // delete and must still be present in the store. + expect(await inner.has(a.hash)).toBe(true); + expect(await inner.has(b.hash)).toBe(false); + expect(freed).toBe(4); + }); });