From 202c553a08fe81f0e3413051a19bf3f4390392f1 Mon Sep 17 00:00:00 2001 From: peipeimo <150317697+peipeimo@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:17:04 -0400 Subject: [PATCH] fix(events): delete stored objects when cascading an event delete (#1051) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(events): delete stored objects when cascading an event delete * fix(events): sweep watermarks and the archive zip on cascade delete too Two more objects in the same class as the originals: both are written through the storage backend, both were only ever removed with fs.unlink, so both outlive the event on S3. - photo.watermark_path — a canonical key, deleted via getStorage() on the single-photo path (watermarkService.deleteWatermarkFile) and on archive (archiveService.js:227). The cascade neither selected nor removed it. - event.archive_path — written by storage.putFromFile (archiveService.js:160) and typically the largest single object an event owns. event.hero_logo_path is deliberately NOT included: multer writes logos to local disk with diskStorage regardless of backend (adminEvents/logo.js:19-28), so they are never bucket objects and the existing fs.unlink is correct. Collect into a Set — an unresized gallery can carry one object in both hero_path and preview_path, and the second delete would log a spurious failure. * fix(events): sweep the download caches, and delete objects concurrently Both from an external review round on this PR. The download caches are the subtle case: the pre-built "Download All" zip (events.download_zip_path) and one zip per custom-resolution download job (download_jobs.zip_path) both live under events/active/{slug}/.download-cache/. On local disk the recursive fs.rm already covered them, which is exactly why they were easy to miss — on S3 that prefix is not a directory, nothing covered them, and both are gallery-sized. downloadZipService exposes a cleanup() documented as "used on event deletion" that the cascade never called. The job rows are read before the transaction for the same reason the photo rows are: download_jobs.event_id is ON DELETE CASCADE, so on Postgres they vanish with the event and take their keys with them. Guarded with hasTable so a pre-#173 install doesn't abort the delete. Deletes now run through a bounded pool instead of one await per key. A 400-photo gallery owns ~1600 objects once derived tiers are counted, and that many sequential DeleteObject round trips runs to minutes — long enough for a proxy to time the request out AFTER the commit, leaving the event deleted and the sweep half-finished. A pool rather than Promise.all over every key, so the fan-out can't exhaust the S3 client's connection pool. * fix(events): never delete a derivative another gallery still uses Round-2 findings from the external reviewer. Canonical thumbnail/hero/preview keys are not event-scoped: the basename is the photo's filename (imageProcessor passes no outputBasename for managed photos, so the key is thumbnails/thumb_w300_), and filenames are not unique across events — the responsive-tier code says so in as many words, which is why THOSE keys carry a p{id}_ prefix. A legacy gallery can therefore share a canonical derivative with a photo in another event, and deleting it here blanked a surviving gallery's tile. Derived keys are now checked against photos outside this event and anything still referenced is left alone; if the check itself fails, every derivative is kept. An orphan costs storage, a deleted derivative costs someone else's gallery. Originals need no check — their keys embed the slug. Also cancel any in-flight or debounced Download All build before snapshotting paths. A builder that started before the delete would otherwise upload a gallery-sized zip after the sweep and write its path onto a row that no longer exists, orphaning it permanently. downloadZipService.cleanup() is the service's own entry point for this and does all three things: bumps the version so an in-flight build discards its result, clears the debounce so nothing rebuilds for a deleted event, and removes the current object. * revert(events): drop the Download All build cancellation Reverted for the same reason as on the stable twin, where it was caught: downloadZipService.cleanup() reaches getStorage() through _cleanup(), so where the S3 backend is configured but unreachable every cascade delete pays the adapter's retry backoff. On stable that took the backend CI job from ~2 minutes to past its 10-minute budget, twice, reproducibly. This branch's suite happened not to trip it, but the same cost lands in the request path of a real delete — and the twins have to carry the same code. The race it addressed is narrow and costs one orphaned zip; documented as a follow-up instead. The shared-derivative guard from the same review round stays — that one prevented deleting a surviving gallery's thumbnail. --------- Co-authored-by: Peifu Mo Co-authored-by: Paul Nothaft --- .../routes/adminEventsCascadeStorage.test.js | 231 ++++++++++++++++++ backend/src/routes/adminEvents/helpers.js | 175 ++++++++++++- 2 files changed, 405 insertions(+), 1 deletion(-) create mode 100644 backend/__tests__/routes/adminEventsCascadeStorage.test.js diff --git a/backend/__tests__/routes/adminEventsCascadeStorage.test.js b/backend/__tests__/routes/adminEventsCascadeStorage.test.js new file mode 100644 index 00000000..5727d0fc --- /dev/null +++ b/backend/__tests__/routes/adminEventsCascadeStorage.test.js @@ -0,0 +1,231 @@ +/** + * Regression test: deleting an event must remove its stored objects. + * + * deleteEventCascade() cleaned up the local filesystem only (#608). On an + * S3/R2 storage backend that cleanup is a no-op, so every deleted gallery + * left its originals and derived tiers in the bucket — unreferenced, + * invisible in the UI, and billed forever. Measured on a v3.45.16 install + * against Cloudflare R2: deleting a 403-photo event changed the bucket + * object count by exactly zero. + * + * The keys must be collected BEFORE the transaction deletes the photo + * rows, because afterwards nothing knows which objects were this event's. + */ + +const os = require('os'); +const path = require('path'); + +// The cascade runs a real `fs.rm(..., { recursive: true })` over +// {STORAGE_PATH}/events/{active,archived}/{slug}. Point that at a throwaway +// directory before requiring the module under test — the default resolves +// into the working tree. +process.env.STORAGE_PATH = path.join(os.tmpdir(), 'picpeak-cascade-storage-test'); + +const mockStorage = { delete: jest.fn().mockResolvedValue(undefined) }; + +const mockEvent = { + id: 42, + slug: 'other-demo-2026-01-01', + event_name: 'Demo', + source_mode: 'managed', + // Written through the backend by archiveService, so it is a bucket object + // and the fs.unlink in the cascade never touched it on S3. + archive_path: 'archives/other-demo-2026-01-01.zip', + // The pre-built "Download All" zip. Lives under the event prefix, so the + // recursive fs.rm covers it on local disk and nothing covers it on S3. + download_zip_path: 'events/active/other-demo-2026-01-01/.download-cache/all.zip', +}; + +// One zip per custom-resolution download job, same prefix. The rows are +// ON DELETE CASCADE, so they must be read before the transaction. +const mockDownloadJobs = [ + { zip_path: 'events/active/other-demo-2026-01-01/.download-cache/job-abc123.zip' }, +]; +const mockPhotos = [ + { + id: 1, + path: 'other-demo-2026-01-01/photo_one.jpg', + thumbnail_path: 'thumbnails/thumb_aaa_photo_one.jpg', + hero_path: null, + preview_path: 'previews/prev_aaa_photo_one.jpg', + watermark_path: 'watermarked/wm_aaa_photo_one.jpg', + source_origin: 'managed', + }, + { + id: 2, + path: 'other-demo-2026-01-01/photo_two.jpg', + thumbnail_path: 'thumbnails/thumb_bbb_photo_two.jpg', + hero_path: null, + preview_path: null, + watermark_path: null, + source_origin: 'managed', + }, + { + // External photos live outside the managed backend and must be left alone. + id: 3, + path: 'ignored.jpg', + thumbnail_path: null, + hero_path: null, + preview_path: null, + watermark_path: null, + source_origin: 'external', + }, +]; + +let mockPhotoRowsDeleted = false; +let mockJobRowsDeleted = false; + +// Photos in OTHER events that share a canonical derivative key with this one. +let mockSharedDerivatives = []; + +// The shared-derivative probe: db('photos').whereNot(...).where(cb).select(...) +const sharedProbe = { + where: () => sharedProbe, + whereIn: () => sharedProbe, + orWhereIn: () => sharedProbe, + select: async () => mockSharedDerivatives, +}; + +function mockMakeDb() { + const table = (name) => { + const chain = { + where: () => chain, + first: async () => (name === 'events' ? mockEvent : undefined), + whereNotNull: () => chain, + whereNot: () => sharedProbe, + orWhereIn: () => chain, + whereIn: () => chain, + select: async () => { + if (name === 'download_jobs') { + return mockJobRowsDeleted ? [] : mockDownloadJobs; + } + if (name === 'photos') { + // The whole point: if this runs after the transaction, the rows + // are gone and we would collect nothing. + return mockPhotoRowsDeleted ? [] : mockPhotos; + } + return []; + }, + del: async () => { + if (name === 'photos') mockPhotoRowsDeleted = true; + if (name === 'download_jobs') mockJobRowsDeleted = true; + return 1; + }, + }; + return chain; + }; + // #1132 guards the merge-dismissals delete behind a hasTable check. + table.schema = { hasTable: async (t) => t === 'download_jobs' }; + table.transaction = async (cb) => cb(table); + return table; +} + +jest.mock('../../src/database/db', () => ({ + db: mockMakeDb(), + logActivity: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock('../../src/services/storage', () => ({ + getStorage: () => mockStorage, +})); + +const { deleteEventCascade } = require('../../src/routes/adminEvents/helpers'); + +describe('deleteEventCascade — storage cleanup', () => { + beforeEach(() => { + mockStorage.delete.mockClear(); + mockPhotoRowsDeleted = false; + mockJobRowsDeleted = false; + mockSharedDerivatives = []; + }); + + it('deletes originals and every derived tier from the storage backend', async () => { + await deleteEventCascade(42, { id: 1, username: 'admin' }); + + const deleted = mockStorage.delete.mock.calls.map(([key]) => key); + + expect(deleted).toEqual(expect.arrayContaining([ + 'events/active/other-demo-2026-01-01/photo_one.jpg', + 'events/active/other-demo-2026-01-01/photo_two.jpg', + 'thumbnails/thumb_aaa_photo_one.jpg', + 'thumbnails/thumb_bbb_photo_two.jpg', + 'previews/prev_aaa_photo_one.jpg', + ])); + }); + + it('deletes pre-generated watermarks and the archive zip', async () => { + await deleteEventCascade(42, { id: 1, username: 'admin' }); + + const deleted = mockStorage.delete.mock.calls.map(([key]) => key); + + // Both are storage-backend objects that only fs.unlink ever touched, so + // both survived an event delete on S3. + expect(deleted).toEqual(expect.arrayContaining([ + 'watermarked/wm_aaa_photo_one.jpg', + 'archives/other-demo-2026-01-01.zip', + ])); + }); + + it('deletes the download caches, which only fs.rm ever covered', async () => { + await deleteEventCascade(42, { id: 1, username: 'admin' }); + + const deleted = mockStorage.delete.mock.calls.map(([key]) => key); + + // Both sit under events/active/{slug}/.download-cache/ — swept by the + // recursive fs.rm on local disk, invisible to it on S3 where the prefix + // is not a directory. Both are gallery-sized. + expect(deleted).toEqual(expect.arrayContaining([ + 'events/active/other-demo-2026-01-01/.download-cache/all.zip', + 'events/active/other-demo-2026-01-01/.download-cache/job-abc123.zip', + ])); + }); + + it('leaves a derivative alone when another event still points at it', async () => { + // Canonical thumbnail/hero/preview keys are not event-scoped — the + // basename is the photo's filename, and filenames are not unique across + // events. Deleting one a surviving gallery still references would blank + // its tile. + mockSharedDerivatives = [{ + thumbnail_path: 'thumbnails/thumb_aaa_photo_one.jpg', + hero_path: null, + preview_path: null, + watermark_path: null, + }]; + + await deleteEventCascade(42, { id: 1, username: 'admin' }); + + const deleted = mockStorage.delete.mock.calls.map(([key]) => key); + expect(deleted).not.toContain('thumbnails/thumb_aaa_photo_one.jpg'); + // The originals are slug-scoped and must still go. + expect(deleted).toContain('events/active/other-demo-2026-01-01/photo_one.jpg'); + // So must a derivative nobody else claims. + expect(deleted).toContain('thumbnails/thumb_bbb_photo_two.jpg'); + }); + + it('never asks the backend to delete the same key twice', async () => { + await deleteEventCascade(42, { id: 1, username: 'admin' }); + + const managed = mockStorage.delete.mock.calls + .map(([key]) => key) + .filter((key) => !key.startsWith('thumbnails/thumb_w') && !key.startsWith('previews/preview_w')); + + expect(managed).toEqual([...new Set(managed)]); + }); + + it('leaves external/reference photos in place', async () => { + await deleteEventCascade(42, { id: 1, username: 'admin' }); + + const deleted = mockStorage.delete.mock.calls.map(([key]) => key); + expect(deleted).not.toEqual(expect.arrayContaining(['ignored.jpg'])); + expect(deleted).not.toEqual(expect.arrayContaining(['events/active/ignored.jpg'])); + }); + + it('still completes the delete when the storage backend throws', async () => { + mockStorage.delete.mockRejectedValue(new Error('bucket unreachable')); + + await expect(deleteEventCascade(42, { id: 1, username: 'admin' })) + .resolves.toEqual({ id: 42, name: 'Demo' }); + + mockStorage.delete.mockResolvedValue(undefined); + }); +}); diff --git a/backend/src/routes/adminEvents/helpers.js b/backend/src/routes/adminEvents/helpers.js index 532aeb97..b84d8d01 100644 --- a/backend/src/routes/adminEvents/helpers.js +++ b/backend/src/routes/adminEvents/helpers.js @@ -230,9 +230,131 @@ async function deleteEventCascade(eventId, adminContext) { // transaction is about to delete. So they are read here, while the rows // still exist, and swept after the commit; miss that window and every tier // this event generated is orphaned with nothing left to derive its key from. + // + // The managed objects themselves need exactly the same window (#1051), so + // one query serves both. On an S3/R2 backend the filesystem sweep below + // removes nothing at all — those paths don't exist locally, `fs.rm` happily + // succeeds against them, and every originally-uploaded photo stays in the + // bucket unreferenced and billable. Measured on a 403-photo event: object + // count unchanged, 679 rows gone. const tieredPhotos = await db('photos') .where('event_id', eventId) - .select('id', 'path', 'filename', 'source_origin', 'external_relpath'); + .select('id', 'path', 'filename', 'source_origin', 'external_relpath', + 'thumbnail_path', 'hero_path', 'preview_path', 'watermark_path'); + + // A Set because a photo can carry the same key in two columns (an unresized + // gallery's hero and preview can resolve to one object) and deleting it + // twice would log a spurious failure for the second attempt. + const storageKeys = new Set(); + // Derived keys separately: unlike the originals, whose keys embed the event + // slug, these are not event-scoped and need a shared-ownership check below. + const derivedKeys = new Set(); + try { + const { resolvePhotoStorageKey } = require('../../services/photoResolver'); + for (const photo of tieredPhotos) { + try { + // Returns null for reference/external photos, which live on a mount + // outside the managed backend and must NOT be deleted — PicPeak does + // not own those bytes. + const originalKey = resolvePhotoStorageKey(event, photo); + if (originalKey) storageKeys.add(originalKey); + } catch (keyErr) { + logger.warn('Could not resolve storage key during cascade delete', { + eventId, photoId: photo.id, error: keyErr.message + }); + } + // Derived tiers are stored as canonical keys and pass through verbatim, + // the same list adminPhotoDimensions.js:801 sweeps on a re-render. + for (const derived of [photo.thumbnail_path, photo.hero_path, photo.preview_path, photo.watermark_path]) { + if (derived) { + storageKeys.add(derived); + derivedKeys.add(derived); + } + } + } + } catch (collectErr) { + logger.warn('Could not enumerate stored objects before cascade delete', { + eventId, error: collectErr.message + }); + } + + // A canonical derivative can belong to more than one gallery. Its basename + // comes from the photo's filename — imageProcessor passes no outputBasename + // for managed photos, so the key is `thumbnails/thumb_w300_` with + // nothing event-scoped in it — and filenames are not unique across events. + // The responsive-tier code says exactly that, which is why THOSE keys carry + // a p{id}_ prefix; the canonical ones predate it. Deleting a shared key here + // would blank a surviving gallery's tile until something regenerated it, so + // anything another event still points at is left alone. Originals need no + // such check: their keys embed the slug. + const derived = Array.from(derivedKeys); + try { + // Chunked: SQLite caps bind variables at 999 and this is four columns wide. + for (let i = 0; i < derived.length; i += 200) { + const chunk = derived.slice(i, i + 200); + const shared = await db('photos') + .whereNot('event_id', eventId) + .where((qb) => qb + .whereIn('thumbnail_path', chunk) + .orWhereIn('hero_path', chunk) + .orWhereIn('preview_path', chunk) + .orWhereIn('watermark_path', chunk)) + .select('thumbnail_path', 'hero_path', 'preview_path', 'watermark_path'); + for (const row of shared) { + for (const key of [row.thumbnail_path, row.hero_path, row.preview_path, row.watermark_path]) { + if (key && derivedKeys.has(key)) storageKeys.delete(key); + } + } + } + } catch (sharedErr) { + // Can't prove ownership — keep the objects. An orphan costs storage; a + // deleted derivative costs someone else's gallery. + logger.warn('Could not check for shared derivatives; leaving them in place', { + eventId, error: sharedErr.message + }); + for (const key of derivedKeys) storageKeys.delete(key); + } + + // The archive zip is typically the largest single object an event owns, and + // archiveService writes it through the backend (`storage.putFromFile`, see + // archiveService.js:160) — so the `fs.unlink` below is a no-op on S3 and the + // zip outlives the event it belongs to. + if (event.archive_path) storageKeys.add(event.archive_path); + + // The download caches are the subtle ones: they live UNDER + // events/active/{slug}/.download-cache/, so the recursive fs.rm below covers + // them on local disk and nothing covers them on S3, where the prefix is not + // a directory. Both are gallery-sized. + // + // - the pre-built "Download All" zip (downloadZipService.js:44) + // - one zip per custom-resolution download job (downloadJobService.js:77) + // + // The job rows must be read BEFORE the transaction for the same reason the + // photo rows are: download_jobs.event_id is ON DELETE CASCADE, so on + // Postgres the rows vanish with the event and their keys with them. + // NOTE: an in-flight "Download All" build that started before this delete + // can still upload its zip after the sweep and write the path onto a row + // that no longer exists, orphaning it. downloadZipService.cleanup() is the + // service's cancel primitive, but calling it here made the stable twin's + // backend CI job exceed its 10-minute budget: _cleanup() reaches + // getStorage(), and where the S3 backend is configured but unreachable + // every cascade delete pays the adapter's retry backoff — in the request + // path, not just in tests. Left as a follow-up rather than shipped behind a + // timeout: the race costs one orphaned object, this cost the whole suite. + if (event.download_zip_path) storageKeys.add(event.download_zip_path); + try { + if (await db.schema.hasTable('download_jobs')) { + const jobs = await db('download_jobs') + .where('event_id', eventId) + .whereNotNull('zip_path') + .select('zip_path'); + for (const job of jobs) storageKeys.add(job.zip_path); + } + } catch (jobErr) { + logger.warn('Could not enumerate download job archives before cascade delete', { + eventId, error: jobErr.message + }); + } await db.transaction(async (trx) => { // 1. Delete activity logs (audit trail) @@ -323,6 +445,57 @@ async function deleteEventCascade(eventId, adminContext) { logger.warn('Failed to delete responsive tiers during cascade delete', { eventId, error: tierErr.message }); } + // Managed objects, post-commit for the same reason: a rolled-back + // transaction must never leave files destroyed for an event that still + // exists. Every key here goes through the backend on both engines — on + // local disk the folder sweep above already covers the originals, but the + // tiers, watermarks and the archive live outside the event folder and would + // otherwise leak there too. + if (storageKeys.size > 0) { + const { getStorage } = require('../../services/storage'); + let removed = 0; + try { + const storage = getStorage(); + const keys = Array.from(storageKeys); + + // Bounded concurrency rather than one await per key. A 400-photo gallery + // owns ~1600 objects once the derived tiers are counted, and on S3 that + // many sequential DeleteObject round trips runs to minutes — long enough + // for a proxy to time the request out AFTER the commit, leaving the + // event deleted and the sweep half-finished. Deleting is idempotent and + // order-independent, so there is nothing to serialise for. + // + // A pool, not Promise.all over every key: an unbounded fan-out would + // open one socket per object and exhaust the S3 client's connection + // pool, which is what #1049 just finished making failures survivable. + const CONCURRENCY = 16; + let cursor = 0; + const worker = async () => { + while (cursor < keys.length) { + const key = keys[cursor++]; + try { + await storage.delete(key); + removed++; + } catch (delErr) { + logger.warn('Failed to delete stored object during cascade delete', { + eventId, key, error: delErr.message + }); + } + } + }; + await Promise.all( + Array.from({ length: Math.min(CONCURRENCY, keys.length) }, worker) + ); + } catch (storageErr) { + logger.warn('Storage backend unavailable during cascade delete', { + eventId, error: storageErr.message + }); + } + logger.info('Cascade delete removed stored objects', { + eventId, removed, total: storageKeys.size + }); + } + // Audit trail (outside the transaction so a logging failure can't undo // the actual delete). await logActivity('event_deleted',