fix(storage): release a deleted journey's photos and a deleted list's images, and keep one bad key from stopping a category

Deleting a journey left its uploaded photos behind for good, and deleting
a list queued neither its cover nor its place images. An object with a key
the store refuses no longer fails every night's pass, and the admin view
keeps the counts of a category skipped for a running backup.

server/src/db/repositories/CollectionPlaces.repository.ts

@@ -145,6 +145,17 @@ export class CollectionPlacesRepository extends TrekRepository<CollectionPlaces>
     return row?.image_url;
   }
 
+  /** `deleteCollection`'s pre-delete read: every saved place's image, released once the list and its places are gone. */
+  async imageUrlsOf(collectionId: number): Promise<Array<string | null>> {
+    const rows = await this.db_()
+      .selectFrom('collection_places')
+      .select('image_url')
+      .where('collection_id', '=', collectionId)
+      .where('image_url', 'is not', null)
+      .execute();
+    return rows.map((row) => row.image_url);
+  }
+
   /**
    * CL48 (`updatePlace`, TX) — the allow-listed dynamic `UPDATE
    * collection_places SET ... WHERE id=?`, including the cross-list move

server/src/db/repositories/Journeys.repository.ts

@@ -52,6 +52,19 @@ type JourneyListKyselyDB = Pick<
 /** The `journeys`/`journey_contributors` tables {@link JourneysRepository.listRecipientUserIds} reads. */
 type JourneyRecipientsKyselyDB = Pick<DB, 'journeys' | 'journey_contributors'>;
 
+/** The tables {@link JourneysRepository.listGalleryPhotos} and {@link JourneysRepository.deleteUnlinkedPhotos} touch. */
+type JourneyPhotoFilesKyselyDB = Pick<DB, 'journeys' | 'journey_photos' | 'trek_photos' | 'trip_photos'>;
+
+/** A photo behind a journey's gallery, with the stored paths its row names. */
+export interface JourneyGalleryPhotoFiles {
+  photo_id: number;
+  file_path: string | null;
+  thumbnail_path: string | null;
+}
+
+/** Ids per DELETE, well under SQLite's bound-parameter limit. */
+const PHOTO_DELETE_BATCH = 500;
+
 /**
  * `journeys` — the root of the journey cluster (Plan 3g Task 1, R9's Part A).
  * Every method here is a Part-A-owned statement (JG1-JG29 group) or a shared
@@ -261,4 +274,49 @@ export class JourneysRepository extends TrekRepository<Journeys> {
   async deleteOwnedByUser(userId: number): Promise<void> {
     await this.nativeDelete({ user: userId });
   }
+
+  /**
+   * The photos in the gallery of one journey, or of every journey a user owns,
+   * with the paths their `trek_photos` rows store. Read before the journey is
+   * deleted: the cascade takes the gallery rows, and with them the only way
+   * back to the photos.
+   */
+  async listGalleryPhotos(scope: { journeyId: number } | { ownerId: number }): Promise<JourneyGalleryPhotoFiles[]> {
+    const [column, value] =
+      'journeyId' in scope ? (['j.id', scope.journeyId] as const) : (['j.user_id', scope.ownerId] as const);
+    return await this.kysely<JourneyPhotoFilesKyselyDB>()
+      .selectFrom('journey_photos as jp')
+      .innerJoin('journeys as j', 'j.id', 'jp.journey_id')
+      .innerJoin('trek_photos as tp', 'tp.id', 'jp.photo_id')
+      .select(['jp.photo_id', 'tp.file_path', 'tp.thumbnail_path'])
+      .distinct()
+      .where(column, '=', value)
+      .execute();
+  }
+
+  /**
+   * Deletes the `trek_photos` rows among `photoIds` that no trip and no journey
+   * links any more, after their journeys went: the same rule as
+   * `TrekPhotoRegistrationService.deleteIfOrphan` with `local`, for a whole
+   * gallery at once. A row left behind would keep naming its file, and the
+   * storage reclaim counts every row with a path as live (#2924). A photo still
+   * in another journey's gallery, or in a trip's, keeps its row. Answers how
+   * many rows went.
+   */
+  async deleteUnlinkedPhotos(photoIds: readonly number[]): Promise<number> {
+    const pages = Array.from({ length: Math.ceil(photoIds.length / PHOTO_DELETE_BATCH) }, (_unused, i) =>
+      photoIds.slice(i * PHOTO_DELETE_BATCH, (i + 1) * PHOTO_DELETE_BATCH),
+    );
+    // One page after the other: they share the caller's transaction.
+    return await pages.reduce<Promise<number>>(async (previous, page) => {
+      const deleted = await previous;
+      const result = await this.kysely<JourneyPhotoFilesKyselyDB>()
+        .deleteFrom('trek_photos')
+        .where('id', 'in', page)
+        .where('id', 'not in', (eb) => eb.selectFrom('trip_photos').select('photo_id'))
+        .where('id', 'not in', (eb) => eb.selectFrom('journey_photos').select('photo_id'))
+        .executeTakeFirst();
+      return deleted + Number(result.numDeletedRows);
+    }, Promise.resolve(0));
+  }
 }

server/src/nest/auth/user-cleanup.service.ts

@@ -191,14 +191,23 @@ export class UserCleanupService {
    */
   async deleteUserCompletely(userId: number): Promise<void> {
     const avatar = await this.usersRepo.getAvatar(userId);
-    await this.uow.transactional(async () => {
+    const photos = await this.uow.transactional(async () => {
+      // The galleries of the journeys the user owns go with the cascade below, so
+      // their photos are read first. A photo row none of the remaining trips and
+      // journeys links goes too, as when one journey is deleted: left behind, it
+      // would keep naming its file for the storage reclaim forever (#2924).
+      const gallery = await this.journeysRepo.listGalleryPhotos({ ownerId: userId });
       await this.cleanupUserReferences(userId);
+      await this.journeysRepo.deleteUnlinkedPhotos(gallery.map((photo) => photo.photo_id));
       await this.erasePluginUserData(userId);
       await this.usersRepo.deleteById(userId);
+      return gallery;
     });
-    // After the commit, the uploaded avatar goes unless another row names it.
-    // What the user's trips and journeys named is the nightly reclaim pass's:
-    // those rows are other domains' and went with the cascade.
+    // After the commit, the uploaded avatar and those photos' files go unless
+    // another row names them. What the user's trips named is the nightly
+    // reclaim pass's: those rows are other domains' and went with the cascade.
     await this.release.releaseValues([avatar], { bare: 'avatars' });
+    const photoFiles = photos.flatMap((photo) => [photo.file_path, photo.thumbnail_path]);
+    await this.release.releaseValues(photoFiles, { bare: 'journey' });
   }
 }

server/src/nest/collections/collections.service.ts

@@ -614,8 +614,13 @@ export class CollectionsService {
     // Snapshot recipients BEFORE the cascade wipes collection_members.
     const accepted = await this.members.acceptedUserIds(id);
     const pending = await this.members.pendingUserIds(id);
+    // The cover and the saved places' images, read before the cascade takes their rows:
+    // they go like a single place's do, unless a trip place or a copy still names them (#2924).
+    const cover = (await this.collectionsRepo.coverImage(id)) ?? null;
+    const images = await this.collectionPlaces.imageUrlsOf(id);
 
     await this.collectionsRepo.deleteById(id); // CASCADE drops members + places + tags
+    await this.release.releaseValues([cover, ...images]);
 
     [...new Set([userId, ...accepted, ...pending])] // the owner's tabs too, all but the one that asked
       .forEach(uid => this.realtime.broadcastToUser(uid, { type: 'collections:deleted', collectionId: id }, socketId));

server/src/nest/journey/journey-domain.service.ts

@@ -8,6 +8,7 @@ import { RealtimeService } from '../realtime/realtime.service';
 import type { JourneyStats, JourneyTrack, TrekWsUserEventName } from '@trek/shared';
 import { todayUtc } from '@trek/shared';
 import { TrekPhotoRegistrationService } from '../photos/trek-photo-registration.service';
+import { StorageReleaseService } from '../storage/storage-release.service';
 import { getCountryFromCoords } from '../atlas/atlas-geo';
 import { computeJourneyStats, countryNamesFor, type StatsInputPoint } from './journey-stats';
 import { newEntrySortOrder, placeMovedEntry } from './journey-entry-order';
@@ -101,6 +102,8 @@ export class JourneyDomainService {
     // finished via `PlacesRepository.findRaw` (this task's ONE append to
     // `Places.repository.ts`, per the task brief — nobody else holds it).
     @InjectRepository(Places) private readonly placesRepo: PlacesRepository,
+    // Deleting a journey releases the files of the photos only it held.
+    private readonly release: StorageReleaseService,
   ) {}
 
   private ts(): number {
@@ -368,7 +371,19 @@ export class JourneyDomainService {
     if (!(await this.isOwner(journeyId, userId))) return false;
     // Read first: the delete takes the contributor rows with it.
     const audience = await this.journeysRepo.listRecipientUserIds(journeyId);
-    await this.journeysRepo.deleteById(journeyId);
+    // The gallery goes with the cascade, so its photos are read first. A photo
+    // row only this journey linked goes too: left behind, it would keep naming
+    // its file and the storage reclaim would never free it (#2924). One still
+    // in a trip or another journey keeps its row, and with it its file.
+    const photos = await this.uow.transactional(async () => {
+      const gallery = await this.journeysRepo.listGalleryPhotos({ journeyId });
+      await this.journeysRepo.deleteById(journeyId);
+      await this.journeysRepo.deleteUnlinkedPhotos(gallery.map((photo) => photo.photo_id));
+      return gallery;
+    });
+    // After the commit; the release asks every source again, so a file another row names stays.
+    const photoFiles = photos.flatMap((photo) => [photo.file_path, photo.thumbnail_path]);
+    await this.release.releaseValues(photoFiles, { bare: 'journey' });
     await this.broadcastJourneyEvent(journeyId, 'journey:deleted', {}, sid, audience);
     return true;
   }

server/src/nest/storage/storage-reclaim.service.ts

@@ -14,6 +14,7 @@ import { DatabaseLifecycle } from '../database/database-lifecycle.service';
 import { withRequestContext } from '../database/request-context';
 import { StorageInstallService } from './storage-install.service';
 import { StorageJobsService } from './storage-jobs.service';
+import { isValidKey } from './storage-keys';
 import { StorageReclaimGuardService } from './storage-reclaim-guard.service';
 import { StorageReclaimRegistryService } from './storage-reclaim-registry.service';
 import { inReclaimScope, RECLAIM_LEDGER, reclaimableCategories, sharedLocationsOf } from './storage-reclaim.ledger';
@@ -435,7 +436,10 @@ export class StorageReclaimService implements OnModuleInit {
     for await (const stat of this.storage.list(category, '', { shallow: true })) {
       seen += 1;
       if (seen % LIST_CHECK_EVERY === 0) this.assertSameDatabase(ctx);
-      if (!inReclaimScope(stat.key)) continue;
+      // A name the storage layer refuses (a backslash or a control character in a
+      // local file put there by hand) can never be stat'ed or deleted: judging it
+      // would fail its whole delete batch every night, so it is left alone.
+      if (!inReclaimScope(stat.key) || !isValidKey(stat.key)) continue;
       const owner = storageOwnerMarkerInstall(stat.key);
       if (owner !== null) {
         owners.add(owner);

server/src/nest/storage/storage-release.service.ts

@@ -78,7 +78,9 @@ class ReleaseStopped extends Error {}
 
 /**
  * What a user's change left unnamed: a replaced or removed cover, place image
- * or avatar, a deleted journey photo or trip file. The owner hands the values
+ * or avatar, a deleted journey photo or trip file, and what a deleted list
+ * (its cover and saved places' images) or journey (its gallery's photos) held;
+ * a deleted trip hands on only its cover. The owner hands the values
  * its rows held to `releaseValues` once its transaction committed, and every
  * object they name is queued, which is all the request pays. Nothing goes
  * before RELEASE_DELAY_MS, so an undo or a late write that brings the value