diff --git a/client/src/components/Admin/storage/reclaimModel.test.ts b/client/src/components/Admin/storage/reclaimModel.test.ts index 52aa53475..ff6c3cf86 100644 --- a/client/src/components/Admin/storage/reclaimModel.test.ts +++ b/client/src/components/Admin/storage/reclaimModel.test.ts @@ -1,4 +1,4 @@ -// FE-ADMIN-STOR-RECLAIM-001 to -007: the reclaim report as both storage panels show it. +// FE-ADMIN-STOR-RECLAIM-001 to -009: the reclaim report as both storage panels show it. import { STORAGE_RECLAIM_CATEGORIES, type StorageReclaimCategoryReport, type StorageReclaimReport } from '@trek/shared'; import { reclaimView } from './reclaimModel'; @@ -137,4 +137,84 @@ describe('reclaimView', () => { it('FE-ADMIN-STOR-RECLAIM-007: a running check reads as running', () => { expect(reclaimView({ deleteEnabled: false, running: true, lastRun: null, lastDeleteRun: null }).running).toBe(true); }); + + it('FE-ADMIN-STOR-RECLAIM-008: a category held back by a backup or a failed delete still counts what was judged and not deleted', () => { + const view = reclaimView({ + deleteEnabled: true, + running: false, + lastDeleteRun: null, + lastRun: report( + { + covers: category({ + skipped: 'backup_running', + unreferenced: { objects: 3_000, bytes: 3_000 }, + eligible: { objects: 3_000, bytes: 3_000 }, + }), + places: category({ + skipped: 'backup_running', + unreferenced: { objects: 1_500, bytes: 150 }, + eligible: { objects: 1_200, bytes: 120 }, + deleted: { objects: 1_000, bytes: 100 }, + }), + files: category({ + skipped: 'delete_failed', + unreferenced: { objects: 5, bytes: 50 }, + eligible: { objects: 4, bytes: 40 }, + deleted: { objects: 1, bytes: 10 }, + }), + }, + 'delete' + ), + }); + const byCategory = Object.fromEntries(view.lines.map((line) => [line.category, line])); + expect(byCategory.covers).toMatchObject({ + freeable: { objects: 3_000, bytes: 3_000 }, + waiting: none, + skipped: 'backup_running', + }); + expect(byCategory.places).toMatchObject({ + freeable: { objects: 200, bytes: 20 }, + waiting: { objects: 300, bytes: 30 }, + skipped: 'backup_running', + }); + expect(byCategory.files).toMatchObject({ + freeable: { objects: 3, bytes: 30 }, + waiting: { objects: 1, bytes: 10 }, + skipped: 'delete_failed', + }); + expect(view.freeable).toEqual({ objects: 3_203, bytes: 3_050 }); + expect(view.waiting).toEqual({ objects: 301, bytes: 40 }); + }); + + it('FE-ADMIN-STOR-RECLAIM-009: a category whose sources or listing failed shows its reason and counts nothing', () => { + const view = reclaimView({ + deleteEnabled: true, + running: false, + lastDeleteRun: null, + lastRun: report({ + covers: category({ skipped: 'source_failed', objects: 12 }), + files: category({ skipped: 'list_failed' }), + }), + }); + expect(view.lines).toEqual([ + { + category: 'files', + freeable: none, + waiting: none, + confirmationRequired: false, + unreferenced: 0, + skipped: 'list_failed', + }, + { + category: 'covers', + freeable: none, + waiting: none, + confirmationRequired: false, + unreferenced: 0, + skipped: 'source_failed', + }, + ]); + expect(view.freeable).toEqual(none); + expect(view.waiting).toEqual(none); + }); }); diff --git a/client/src/components/Admin/storage/reclaimModel.ts b/client/src/components/Admin/storage/reclaimModel.ts index 19a9fb1d8..4dc5b2063 100644 --- a/client/src/components/Admin/storage/reclaimModel.ts +++ b/client/src/components/Admin/storage/reclaimModel.ts @@ -67,11 +67,23 @@ function freedBy(run: StorageReclaimReport | null): ReclaimCount { ); } +type SkipReason = StorageReclaimCategoryReport['skipped']; + /** - * The reclaim state as the storage panels show it, desktop and phone alike. - * `null` (a server from before the reclaim pass, or a state not loaded) reads - * as deletion off, not running and never checked. + * The reasons the pass gives when it left a category before judging it, so its + * counts mean nothing. Every other reason (a backup running, a failed delete, + * an interrupted pass) comes after the judgement and keeps its figures; a new + * reason counts by default rather than hiding what the pass found. */ +const NEVER_JUDGED: ReadonlySet = new Set([ + 'sources_missing', + 'shared_location', + 'foreign_install', + 'owner_unknown', + 'list_failed', + 'source_failed', +]); + /** * One category of the last check as a line, or null when it has nothing to * say. A category the pass stopped in still counts what it found; one it @@ -82,7 +94,7 @@ function lineOf( report: StorageReclaimCategoryReport | undefined ): ReclaimCategoryLine | null { if (!report || report.skipped === 'not_reclaimed') return null; - const judged = report.skipped === null || report.skipped === 'interrupted' || report.skipped === 'delete_failed'; + const judged = !NEVER_JUDGED.has(report.skipped); // What was deleted on this run no longer waits to be freed. const freeable = judged ? minus(report.eligible, report.deleted) : NONE; const waiting = judged ? minus(report.unreferenced, report.eligible) : NONE; diff --git a/server/src/db/repositories/CollectionPlaces.repository.ts b/server/src/db/repositories/CollectionPlaces.repository.ts index 73685b7fd..22d0c802e 100644 --- a/server/src/db/repositories/CollectionPlaces.repository.ts +++ b/server/src/db/repositories/CollectionPlaces.repository.ts @@ -145,6 +145,17 @@ export class CollectionPlacesRepository extends TrekRepository 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> { + 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 diff --git a/server/src/db/repositories/Journeys.repository.ts b/server/src/db/repositories/Journeys.repository.ts index db1d7ebba..b4f696440 100644 --- a/server/src/db/repositories/Journeys.repository.ts +++ b/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; +/** The tables {@link JourneysRepository.listGalleryPhotos} and {@link JourneysRepository.deleteUnlinkedPhotos} touch. */ +type JourneyPhotoFilesKyselyDB = Pick; + +/** 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 { async deleteOwnedByUser(userId: number): Promise { 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 { + const [column, value] = + 'journeyId' in scope ? (['j.id', scope.journeyId] as const) : (['j.user_id', scope.ownerId] as const); + return await this.kysely() + .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 { + 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>(async (previous, page) => { + const deleted = await previous; + const result = await this.kysely() + .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)); + } } diff --git a/server/src/nest/auth/user-cleanup.service.ts b/server/src/nest/auth/user-cleanup.service.ts index 5b64e5076..4159c55b1 100644 --- a/server/src/nest/auth/user-cleanup.service.ts +++ b/server/src/nest/auth/user-cleanup.service.ts @@ -191,14 +191,23 @@ export class UserCleanupService { */ async deleteUserCompletely(userId: number): Promise { 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' }); } } diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index c6133411f..3e29da937 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/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)); diff --git a/server/src/nest/journey/journey-domain.service.ts b/server/src/nest/journey/journey-domain.service.ts index b2dfa4faf..8f37aea82 100644 --- a/server/src/nest/journey/journey-domain.service.ts +++ b/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; } diff --git a/server/src/nest/storage/storage-reclaim.service.ts b/server/src/nest/storage/storage-reclaim.service.ts index 53a96e0e0..62efbc95d 100644 --- a/server/src/nest/storage/storage-reclaim.service.ts +++ b/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); diff --git a/server/src/nest/storage/storage-release.service.ts b/server/src/nest/storage/storage-release.service.ts index 2424fafc2..12aaa0aae 100644 --- a/server/src/nest/storage/storage-release.service.ts +++ b/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 diff --git a/server/tests/helpers/accommodations-service.ts b/server/tests/helpers/accommodations-service.ts index 0c6928069..2aa6aec61 100644 --- a/server/tests/helpers/accommodations-service.ts +++ b/server/tests/helpers/accommodations-service.ts @@ -93,6 +93,7 @@ export async function accommodationsOver( await createTestJourneyPhotosRepo(conn), await createTestJourneyEntryPhotosRepo(conn), await createTestPlacesRepo(conn), + { releaseValues: async () => 0 } as never, // StorageReleaseService ), await createTestUnitOfWork(conn), await createTestDayAssignmentsRepo(conn), diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 751e81132..e22926b82 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -361,6 +361,7 @@ export async function createPluginRpcHostParts( await createTestJourneyPhotosRepo(db), await createTestJourneyEntryPhotosRepo(db), await createTestPlacesRepo(db), + { releaseValues: async () => 0 } as never, // StorageReleaseService ); const atlas = new AtlasService( await createTestBucketListRepo(db), diff --git a/server/tests/unit/nest/assignments.service.test.ts b/server/tests/unit/nest/assignments.service.test.ts index 40fd88ae7..8b83d1aec 100644 --- a/server/tests/unit/nest/assignments.service.test.ts +++ b/server/tests/unit/nest/assignments.service.test.ts @@ -109,6 +109,7 @@ beforeAll(async () => { await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + { releaseValues: async () => 0 } as never, // StorageReleaseService ), await createTestUnitOfWork(testDb), await createTestDayAssignmentsRepo(testDb), diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index 27d1f9010..ef5035172 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -700,6 +700,24 @@ describe('deleteCollection', () => { expect(await placesIn(col.id)).toEqual({ n: 0 }); expect(await membersOf(col.id)).toEqual({ n: 0 }); }); + + it('COLLECTIONS-SVC-040b: deleting a list releases its cover and the images of its saved places (#2924)', async () => { + const owner = createUser(testDb).user; + const col = await svc.createCollection(owner.id, { name: 'Doomed' }); + const files = ['list-cover.jpg', 'list-place-a.jpg', 'list-place-b.jpg'].map((name) => { + const filePath = path.join(storageFx.root, name); + fs.writeFileSync(filePath, 'jpeg-bytes'); + return filePath; + }); + await svc.setCollectionCover(owner.id, col.id, '/uploads/covers/list-cover.jpg'); + await svc.savePlace(owner.id, { collection_id: col.id, name: 'A', image_url: '/uploads/places/list-place-a.jpg' }); + await svc.savePlace(owner.id, { collection_id: col.id, name: 'B', image_url: '/uploads/places/list-place-b.jpg' }); + await svc.savePlace(owner.id, { collection_id: col.id, name: 'No image' }); + + await svc.deleteCollection(owner.id, col.id); + + expect(files.map((file) => fs.existsSync(file))).toEqual([false, false, false]); + }); }); // ── owner_id semantics: member account deletion keeps shared content ───────── diff --git a/server/tests/unit/nest/common/conflictUpdate.test.ts b/server/tests/unit/nest/common/conflictUpdate.test.ts index 2e34b797c..8838629b2 100644 --- a/server/tests/unit/nest/common/conflictUpdate.test.ts +++ b/server/tests/unit/nest/common/conflictUpdate.test.ts @@ -153,6 +153,7 @@ beforeAll(async () => { await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + { releaseValues: async () => 0 } as never, // StorageReleaseService ), makeStorageFixture('').storage, await accommodationsOver(testDb), diff --git a/server/tests/unit/nest/journey-book.service.test.ts b/server/tests/unit/nest/journey-book.service.test.ts index a501d1f92..acaf65fe5 100644 --- a/server/tests/unit/nest/journey-book.service.test.ts +++ b/server/tests/unit/nest/journey-book.service.test.ts @@ -77,6 +77,7 @@ beforeAll(async () => { await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + { releaseValues: async () => 0 } as never, // StorageReleaseService ); // Plan 3g Task 3: JourneyBooksRepository (JB1-JB7), not `dbs` any more. // task-5-fix-brief constructor-ripple (M1): `UnitOfWork`, so `saveBook`'s diff --git a/server/tests/unit/nest/journey-domain.service.test.ts b/server/tests/unit/nest/journey-domain.service.test.ts index 59bd82867..ff6d52300 100644 --- a/server/tests/unit/nest/journey-domain.service.test.ts +++ b/server/tests/unit/nest/journey-domain.service.test.ts @@ -52,6 +52,7 @@ import { // ported from the reviewer's probe (scratchpad r3g/probe/zz-r3g-parity-probe.test.ts). import { todayUtc } from '@trek/shared'; import { GALLERY_CHRONOLOGICAL_ORDER } from '../../helpers/legacy-gallery-order'; +import { releaseNothing } from '../../helpers/storage-release'; import type { EntityClass, EntityDTO, FilterQuery, FindOptions, RequiredEntityData } from '@mikro-orm/core'; import { countRows, deleteRows, findRows, insertRow, updateRows } from '../../helpers/factories/rows'; import { DayAssignments } from '../../../src/db/entities/DayAssignments.entity'; @@ -98,6 +99,7 @@ async function pickAll>( } let svc: JourneyDomainService; +const release = releaseNothing(); // Plan 3g Task 1's own additions (below, "repositories (R9's parity + mutation // proofs)" describe block) reach the repositories directly — held here so // that block doesn't need its own second construction. @@ -145,6 +147,7 @@ beforeAll(async () => { // R9, Task 2 only appends to what Task 1's construction already // established, never re-touches the wrapping mechanism itself. photosRepoDirect, entryPhotosRepoDirect, placesRepoDirect, + release, ); }); @@ -540,6 +543,49 @@ describe('deleteJourney', () => { const row = await one(Journeys, { id: journey.id }); expect(row).toBeDefined(); }); + + it('JOURNEY-SVC-022b: deleting a journey drops the photo rows only it held and releases their files', async () => { + const { user } = createUser(testDb); + const journey = await svc.createJourney(user.id, { title: 'J' }); + const [photo, video] = await svc.uploadGalleryPhotos(journey.id, user.id, [ + { path: 'journey/a.jpg' }, + { path: 'journey/b.mp4', thumbnail: 'journey/b-poster.jpg', mediaType: 'video' }, + ]); + release.releaseValues.mockClear(); + + expect(await svc.deleteJourney(journey.id, user.id)).toBe(true); + + expect(await one(TrekPhotos, { id: { $in: [photo.photo_id, video.photo_id] } })).toBeUndefined(); + expect(release.releaseValues).toHaveBeenCalledTimes(1); + const [values, options] = release.releaseValues.mock.calls[0] as unknown as [Array, unknown]; + expect(values.filter(Boolean).sort()).toEqual(['journey/a.jpg', 'journey/b-poster.jpg', 'journey/b.mp4']); + expect(options).toEqual({ bare: 'journey' }); + }); + + it('JOURNEY-SVC-022c: a photo another journey or a trip still links keeps its row through the delete', async () => { + const { user } = createUser(testDb); + const journey = await svc.createJourney(user.id, { title: 'J' }); + const other = await svc.createJourney(user.id, { title: 'Other' }); + const trip = createTrip(testDb, user.id); + const [shared, inTrip, alone] = await svc.uploadGalleryPhotos(journey.id, user.id, [ + { path: 'journey/shared.jpg' }, + { path: 'journey/trip.jpg' }, + { path: 'journey/alone.jpg' }, + ]); + // The same file uploaded to a second journey reuses its photo row. + const [again] = await svc.uploadGalleryPhotos(other.id, user.id, [{ path: 'journey/shared.jpg' }]); + expect(again.photo_id).toBe(shared.photo_id); + testDb + .prepare('INSERT INTO trip_photos (trip_id, user_id, photo_id) VALUES (?, ?, ?)') + .run(trip.id, user.id, inTrip.photo_id); + + expect(await svc.deleteJourney(journey.id, user.id)).toBe(true); + + expect(await one(TrekPhotos, { id: shared.photo_id })).toBeDefined(); + expect(await one(TrekPhotos, { id: inTrip.photo_id })).toBeDefined(); + expect(await one(TrekPhotos, { id: alone.photo_id })).toBeUndefined(); + expect(await one(JourneyPhotos, { journey: other.id, photo: shared.photo_id })).toBeDefined(); + }); }); // -- Trip management ---------------------------------------------------------- diff --git a/server/tests/unit/nest/journey-share.service.test.ts b/server/tests/unit/nest/journey-share.service.test.ts index 7631d6a62..fb3ff7a1c 100644 --- a/server/tests/unit/nest/journey-share.service.test.ts +++ b/server/tests/unit/nest/journey-share.service.test.ts @@ -101,6 +101,7 @@ beforeAll(async () => { await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + { releaseValues: async () => 0 } as never, // StorageReleaseService ), new SettingsService( uow, diff --git a/server/tests/unit/nest/journey-stats.service.test.ts b/server/tests/unit/nest/journey-stats.service.test.ts index 13bcf2914..ea4a59eff 100644 --- a/server/tests/unit/nest/journey-stats.service.test.ts +++ b/server/tests/unit/nest/journey-stats.service.test.ts @@ -108,6 +108,7 @@ beforeAll(async () => { await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + { releaseValues: async () => 0 } as never, // StorageReleaseService ); }); diff --git a/server/tests/unit/nest/places.directions.test.ts b/server/tests/unit/nest/places.directions.test.ts index bde6f60c9..8dbfdaab1 100644 --- a/server/tests/unit/nest/places.directions.test.ts +++ b/server/tests/unit/nest/places.directions.test.ts @@ -137,6 +137,7 @@ async function svc(searchNominatim: OsmClient['searchNominatim']): Promise 0 } as never, // StorageReleaseService ), storageFx.storage, await accommodationsOver(testDb), diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index 34212e323..cb02192a4 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -145,7 +145,7 @@ async function makePlacesService( await createTestJourneysRepo(testDb), await createTestJourneyContributorsRepo(testDb), await createTestJourneyTripsRepo(testDb), await createTestJourneyEntriesRepo(testDb), await createTestTripsRepo(testDb), // Plan 3g Task 2 constructor-ripple: JourneyPhotosRepository/JourneyEntryPhotosRepository/PlacesRepository. - await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), { releaseValues: async () => 0 } as never, ), placesStorageFx.storage, await accommodationsOver(testDb), await createTestUnitOfWork(testDb), diff --git a/server/tests/unit/nest/storage/storage-reclaim.service.test.ts b/server/tests/unit/nest/storage/storage-reclaim.service.test.ts index 04e34b522..f80216a94 100644 --- a/server/tests/unit/nest/storage/storage-reclaim.service.test.ts +++ b/server/tests/unit/nest/storage/storage-reclaim.service.test.ts @@ -10,7 +10,7 @@ * an owner marker of another install keeps its hands off; a restore, or a * database naming another install, resets the switch, the marks, the queue * and the reports but keeps this install's id, and so does a restore's reset - * file whatever id the database names. RCLS-001 to RCLS-046. + * file whatever id the database names. RCLS-001 to RCLS-047. */ import { AppSettings } from '../../../../src/db/entities/AppSettings.entity'; import { AuditLog } from '../../../../src/db/entities/AuditLog.entity'; @@ -342,6 +342,22 @@ describe('StorageReclaimService: mark, then sweep', () => { expect(again.categories.covers.objects).toBe(5); expect((await marksOf('covers')).map((mark) => mark.key)).toEqual(['orphan.jpg']); }); + + it('RCLS-047: a listed name the storage layer refuses is never judged, so it cannot fail the batch it would sit in', async () => { + // A file put into the folder by hand with a backslash in its name: a Linux + // local backend lists it, but stat and delete refuse the key. + const listOf = storage.list.bind(storage); + vi.spyOn(storage, 'list').mockImplementation(async function* (category, prefix, opts) { + yield* listOf(category, prefix, opts); + if (category === 'covers') yield { key: 'trip\\photo.jpg', size: 5, mtimeMs: 1 }; + }); + await markedWeekOld(['orphan.jpg']); + expect((await marksOf('covers')).map((mark) => mark.key)).toEqual(['orphan.jpg']); + + const report = await reclaim.run({ now: WEEK_LATER }); + expect(report.categories.covers).toMatchObject({ objects: 5, deleted: { objects: 1, bytes: 5 }, skipped: null }); + expect(fs.existsSync(objectPath('covers', 'orphan.jpg'))).toBe(false); + }); }); describe('StorageReclaimService: a large unreferenced share waits for the admin', () => { diff --git a/server/tests/unit/nest/tours.gpx.atomic.test.ts b/server/tests/unit/nest/tours.gpx.atomic.test.ts index 48c37f351..bd146dd48 100644 --- a/server/tests/unit/nest/tours.gpx.atomic.test.ts +++ b/server/tests/unit/nest/tours.gpx.atomic.test.ts @@ -126,6 +126,7 @@ async function makePlacesService(): Promise { await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + { releaseValues: async () => 0 } as never, // StorageReleaseService ), storage, await accommodationsOver(testDb), diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index cad6a0e1d..162ad42f3 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -222,6 +222,7 @@ beforeAll(async () => { await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + { releaseValues: async () => 0 } as never, // StorageReleaseService ), makeStorageFixture('').storage, await accommodationsOver(testDb, realtime), diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index bf0a8d66e..767dafa93 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -265,6 +265,7 @@ beforeAll(async () => { await createTestJourneyTripsRepo(testDb), await createTestJourneyEntriesRepo(testDb), await createTestTripsRepo(testDb), // Plan 3g Task 2 constructor-ripple: JourneyPhotosRepository/JourneyEntryPhotosRepository/PlacesRepository. await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), + { releaseValues: async () => 0 } as never, // StorageReleaseService ), makeStorageFixture('').storage, await accommodationsOver(testDb, realtime), await createTestUnitOfWork(testDb), diff --git a/server/tests/unit/nest/user-cleanup.service.test.ts b/server/tests/unit/nest/user-cleanup.service.test.ts index 07e756848..1f323b5d1 100644 --- a/server/tests/unit/nest/user-cleanup.service.test.ts +++ b/server/tests/unit/nest/user-cleanup.service.test.ts @@ -23,6 +23,7 @@ import { Plugins } from '../../../src/db/entities/Plugins.entity'; import { ShareTokens } from '../../../src/db/entities/ShareTokens.entity'; import { TrekPhotos } from '../../../src/db/entities/TrekPhotos.entity'; import { TripMembers } from '../../../src/db/entities/TripMembers.entity'; +import { TripPhotos } from '../../../src/db/entities/TripPhotos.entity'; import { Users } from '../../../src/db/entities/Users.entity'; import type { BudgetItemsRepository } from '../../../src/db/repositories/BudgetItems.repository'; import type { BudgetSettlementsRepository } from '../../../src/db/repositories/BudgetSettlements.repository'; @@ -459,4 +460,39 @@ describe('deleteUserCompletely', () => { await svc.deleteUserCompletely(user.id); expect(cleanupRelease.releaseValues).toHaveBeenCalledWith(['face.png'], { bare: 'avatars' }); }); + + it('USER-CLEANUP-012: drops the photo rows only their own journeys held and releases those files', async () => { + const { user: victim } = createUser(testDb, { username: 'photo-victim' }); + const { user: other } = createUser(testDb, { username: 'photo-other' }); + const ownJourney = await createJourney(victim.id, 'Mine'); + const otherJourney = await createJourney(other.id, 'Theirs'); + const trip = createTrip(testDb, other.id); + const photo = async (file: string, thumb: string | null = null) => { + const row = { provider: 'local', owner: victim.id, file_path: file, thumbnail_path: thumb }; + const id = await insertRow(orm, TrekPhotos, row); + await insertRow(orm, JourneyPhotos, { journey: ownJourney, photo: id, created_at: 0 }); + return id; + }; + const alone = await photo('journey/alone.jpg', 'journey/alone-poster.jpg'); + const inOtherJourney = await photo('journey/shared.jpg'); + const inTrip = await photo('journey/trip.jpg'); + await insertRow(orm, JourneyPhotos, { journey: otherJourney, photo: inOtherJourney, created_at: 0 }); + await insertRow(orm, TripPhotos, { trip: trip.id, user: other.id, photo: inTrip }); + + await svc.deleteUserCompletely(victim.id); + + expect(await findRow(orm, TrekPhotos, { id: alone })).toBeNull(); + expect(await findRow(orm, TrekPhotos, { id: inOtherJourney })).not.toBeNull(); + expect(await findRow(orm, TrekPhotos, { id: inTrip })).not.toBeNull(); + // Every file the gallery named is handed on: the release keeps what another row still names. + type Released = [Array, { bare?: string }]; + const calls = cleanupRelease.releaseValues.mock.calls as unknown as Released[]; + const handed = calls.find(([, options]) => options.bare === 'journey')?.[0] ?? []; + expect(handed.filter(Boolean).sort()).toEqual([ + 'journey/alone-poster.jpg', + 'journey/alone.jpg', + 'journey/shared.jpg', + 'journey/trip.jpg', + ]); + }); }); diff --git a/wiki/Admin-Storage.md b/wiki/Admin-Storage.md index 1649b37df..7be2233b3 100644 --- a/wiki/Admin-Storage.md +++ b/wiki/Admin-Storage.md @@ -141,9 +141,19 @@ already wait, is no longer waited for: the log says so with the number of files, and such files are then only removed by the nightly cleanup, once deletion is switched on. -Whatever is left over after that (files of deleted trips and users, files from -before this check existed) is found by a check every night at 04:05 server -time. It looks at the covers, place images, avatars, trip files and journal +Deleting a whole list or journey works the same way for what it held: a list +releases its cover and the images of its saved places, a journey the photos +of its gallery that no trip or other journey shows, and deleting a user does +this for every journey they own. Deleting a whole trip releases only its +cover: the trip's files and its places' images are left to the nightly +cleanup below. Journal photos of a journey deleted before your instance ran a +version with this behaviour are the exception: their photo entries were kept, +so TREK still counts those files as used and neither the release nor the +nightly check removes them. + +Whatever is left over after that (files of deleted trips and users, files +from before this check existed) is found by a check every night at 04:05 +server time. It looks at the covers, place images, avatars, trip files and journal photos for files that nothing in TREK names any more, and the **Unreferenced files** section shows what it found: