paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(events): delete stored objects when cascading an event delete (#1245)

Browse Source 
* fix(events): delete stored objects when cascading an event delete

Stable twin of #1051. Deleting an event removed its database rows but left
every stored file behind: deleteEventCascade() cleaned up with fs.rm over
{STORAGE_PATH}/events/{active,archived}/{slug}, and on an S3-compatible
backend those paths don't exist locally — the call succeeds against nothing
and the real objects stay in the bucket, unreferenced by any row, invisible
in the UI, and billed every month.

Measured on a v3.45.16 install against Cloudflare R2, deleting one
403-photo event: bucket object count 5,400 before and 5,400 after, while
referenced rows dropped from 3,425 to 2,746.

Keys are collected BEFORE the transaction removes the photo rows — once
they are gone nothing records which objects belonged to the event, and only
a full-bucket audit against the whole database could find them again — and
deleted AFTER the commit, so a rolled-back delete can never destroy files
for an event that still exists.

Includes photo.watermark_path and event.archive_path, both storage-backed
and both previously fs.unlink-only. event.hero_logo_path is deliberately
excluded: multer writes logos to local disk with diskStorage regardless of
backend, so they are never bucket objects.

Reference/external photos are left alone — resolvePhotoStorageKey returns
null for them and PicPeak does not own those bytes.

This branch carries the higher priority of the pair: unlike main, stable's
deleteEventCascade never calls getStorage() at all, and the leak costs real
money for every month it goes unfixed.

Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>

* fix(events): sweep the Download All cache, and delete objects concurrently

Both from an external review round on #1051.

The pre-built "Download All" zip (events.download_zip_path) lives under
events/active/{slug}/.download-cache/. On local disk the recursive fs.rm
already covered it, which is exactly why it was easy to miss — on S3 that
prefix is not a directory, nothing covered it, and it is gallery-sized.
downloadZipService exposes a cleanup() documented as "used on event
deletion" that the cascade never called.

download_jobs (main's #173) does not exist on this branch, so the
per-job archives main also sweeps have no counterpart here.

Deletes now run through a bounded pool instead of one await per key: a
400-photo gallery owns well over a thousand objects, 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.

* fix(events): never delete a derivative another gallery still uses

Round-2 findings from the external reviewer on #1051, ported.

Canonical thumbnail/hero/preview keys are not event-scoped: the basename is
the photo's filename, and filenames are not unique across events. A legacy
gallery can 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, via downloadZipService.cleanup() — the service's own entry point for
event deletion. 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.

* revert(events): drop the Download All build cancellation

It broke CI on this branch: the backend job went from ~2 minutes to
exceeding its 10-minute budget, twice, reproducibly.

downloadZipService.cleanup() reaches getStorage() through _cleanup(), and in
a suite where the S3 backend is configured but unreachable every cascade
delete then pays the adapter's retry backoff. The full suite passes locally
against SQLite, which is why this only showed up in CI.

The race it addressed is real but narrow — a builder that started before the
delete uploads its zip after the sweep and writes the path onto a row that
no longer exists, orphaning one object. That is a cheaper problem than an
unrunnable test suite, so it goes back to being a documented follow-up
rather than shipping behind a timeout.

The shared-derivative guard from the same review round stays: that one
prevented deleting a surviving gallery's thumbnail.

---------

Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
This commit is contained in:
Paul Nothaft
2026-09-01 08:16:51 +02:00
committed by GitHub
parent 5b69e3ec4c
commit 7102687ee8
2 changed files with 385 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/__tests__/routes/adminEventsCascadeStorage.test.js
+223
View File
@@ -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);
});
});
backend/src/routes/adminEvents/helpers.js
+162
View File
@@ -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_<filename>` 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',