diff --git a/backend/__tests__/routes/adminEventsCascadeStorage.test.js b/backend/__tests__/routes/adminEventsCascadeStorage.test.js new file mode 100644 index 00000000..17368385 --- /dev/null +++ b/backend/__tests__/routes/adminEventsCascadeStorage.test.js @@ -0,0 +1,223 @@ +/** + * 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', +}; + +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 === '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 () => false }; + 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 All cache, which only fs.rm ever covered', async () => { + await deleteEventCascade(42, { id: 1, username: 'admin' }); + + const deleted = mockStorage.delete.mock.calls.map(([key]) => key); + + // Sits 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. Gallery-sized. (download_jobs is main-only, so the + // per-job archives main also sweeps have no counterpart here.) + expect(deleted).toContain( + 'events/active/other-demo-2026-01-01/.download-cache/all.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 01ef769d..3ea10f89 100644 --- a/backend/src/routes/adminEvents/helpers.js +++ b/backend/src/routes/adminEvents/helpers.js @@ -224,6 +224,117 @@ async function deleteEventCascade(eventId, adminContext) { throw err; } + // Collect this event's storage keys BEFORE the transaction removes the + // photo rows. Afterwards nothing records which objects belonged to this + // event — the DB was the only place that knew, and on an S3/R2 backend the + // objects are still sitting in the bucket, unreferenced and billable. + // + // The filesystem cleanup below (#608) only ever touched local disk: in S3 + // mode those paths don't exist, `fs.rm` succeeds against nothing, and the + // real objects are never touched. Measured on a 403-photo event: bucket + // object count unchanged, 679 referenced rows gone. + // + // 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'); + const photos = await db('photos') + .where('event_id', eventId) + .select('id', 'path', 'thumbnail_path', 'hero_path', 'preview_path', 'watermark_path', 'source_origin'); + + for (const photo of photos) { + 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. + // watermark_path included: it is storage-backed on the single-photo + // path (watermarkService.deleteWatermarkFile) and leaked here the same + // way the originals did. + 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`) — so + // the `fs.unlink` below is a no-op on S3 and the zip outlives its event. + if (event.archive_path) storageKeys.add(event.archive_path); + + // The pre-built "Download All" zip is the subtle one: it lives UNDER + // events/active/{slug}/.download-cache/ (downloadZipService.js:42), so the + // recursive fs.rm below covers it on local disk and nothing covers it on + // S3, where that prefix is not a directory. It is gallery-sized. + // downloadZipService exposes a cleanup() documented as "used on event + // deletion" that this cascade never called. + // 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 backend CI job + // exceed its 10-minute budget on this branch — its _cleanup() reaches + // getStorage() and, in a suite where the S3 backend is configured but + // unreachable, every cascade delete then pays the adapter's retry backoff. + // Left as a follow-up rather than shipped as a timeout: the race is narrow + // and costs one orphaned object, the regression cost the whole suite. + if (event.download_zip_path) storageKeys.add(event.download_zip_path); + await db.transaction(async (trx) => { // 1. Delete activity logs (audit trail) await trx('activity_logs').where('event_id', eventId).del(); @@ -284,6 +395,57 @@ async function deleteEventCascade(eventId, adminContext) { } }); + // Managed objects, deleted AFTER the commit: a rolled-back transaction must + // never leave files destroyed for an event that still exists. Failures are + // logged rather than thrown, matching the philosophy of the filesystem + // cleanup above — the database is the source of truth, an orphaned object + // is recoverable noise, a half-deleted event is not. + 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 well over a thousand 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. + 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',