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 (#1051)

Browse Source 
* 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_<filename>), 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 <[email protected]>
Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
peipeimo
2026-09-01 08:17:04 +02:00
committed by GitHub
co-authored by Peifu Mo Paul Nothaft
parent bdeb5a2151
commit 202c553a08
2 changed files with 405 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/routes/adminEvents/helpers.js
+174 -1
View File
@@ -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_<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`, 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',