From db0fbc1ccdd0763e6d9d3173d46b7ced244ee0e3 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 1 Nov 2026 13:59:28 +0100 Subject: [PATCH] fix(storage): stop deleting covers, place images and journey photos another row may still show Replacing a trip or list cover, changing or deleting a place or saved-place image and deleting a journey photo removed the bytes at once, without asking whether a trip copy, a journey, a saved place or another photo row still named them (#2924). Those paths now leave the object in place and the reclaim job removes it once no row names it. A journey photo the user deletes takes its local photo row with it when nothing else links it, so its bytes become reclaimable instead of being kept by a row nobody sees. A place image copied from a trip file is still dropped at once when the update that would name it fails: that copy was never anyone else's. Size and warning baselines follow the shrunk files. --- server/scripts/eslint-baseline.json | 2 +- server/scripts/size-baseline.json | 5 +- server/scripts/test-size-baseline.json | 2 +- .../db/repositories/TrekPhotos.repository.ts | 5 ++ .../nest/collections/collections.service.ts | 67 ++-------------- .../nest/journey/journey-domain.service.ts | 4 +- server/src/nest/journey/journey.controller.ts | 36 +-------- .../photos/trek-photo-registration.service.ts | 11 ++- server/src/nest/places/place-image.ts | 23 +----- server/src/nest/places/places.service.ts | 69 ++++------------- server/src/nest/trips/trips.controller.ts | 10 +-- server/src/nest/trips/trips.service.ts | 12 --- server/tests/integration/trips.test.ts | 5 +- .../unit/nest/collections.service.test.ts | 11 ++- .../unit/nest/journey.controller.test.ts | 76 ++++--------------- server/tests/unit/nest/places.service.test.ts | 16 ++-- .../trek-photo-registration.service.test.ts | 16 ++++ .../tests/unit/nest/trips.controller.test.ts | 7 +- server/tests/unit/nest/trips.service.test.ts | 41 ---------- 19 files changed, 101 insertions(+), 317 deletions(-) diff --git a/server/scripts/eslint-baseline.json b/server/scripts/eslint-baseline.json index 9d7a7e618..9b9bed3e5 100644 --- a/server/scripts/eslint-baseline.json +++ b/server/scripts/eslint-baseline.json @@ -5,7 +5,7 @@ "@typescript-eslint/no-require-imports": 18, "@typescript-eslint/no-unsafe-function-type": 1, "@typescript-eslint/no-unused-vars": 36, - "no-await-in-loop": 561, + "no-await-in-loop": 557, "no-console": 234, "no-empty": 6 }, diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index 5462aae78..e1e7eb0ae 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -8,14 +8,13 @@ "src/nest/atlas/atlas.service.ts": 1177, "src/nest/auth/auth.service.ts": 1143, "src/nest/budget/budget.service.ts": 1617, - "src/nest/collections/collections.service.ts": 1478, + "src/nest/collections/collections.service.ts": 1427, "src/nest/doc-sync/doc-sync.service.ts": 1060, "src/nest/doc-sync/providers/synology-drive.client.ts": 1160, "src/nest/journey/journey-domain.service.ts": 1659, - "src/nest/places/places.service.ts": 1592, + "src/nest/places/places.service.ts": 1549, "src/nest/plugins/plugin-runtime.service.ts": 1460, "src/nest/plugins/runtime/plugin-sdk.ts": 1093, "src/nest/reservations/reservations.service.ts": 1237, - "src/nest/trips/trips.service.ts": 1006, "src/nest/vacay/vacay.service.ts": 1409 } diff --git a/server/scripts/test-size-baseline.json b/server/scripts/test-size-baseline.json index dd0b2b6fe..f36afcab0 100644 --- a/server/scripts/test-size-baseline.json +++ b/server/scripts/test-size-baseline.json @@ -5,5 +5,5 @@ "tests/unit/nest/journey-domain.service.test.ts": 3876, "tests/unit/nest/maps.service.test.ts": 3680, "tests/unit/nest/places.service.test.ts": 2561, - "tests/unit/nest/trips.service.test.ts": 2404 + "tests/unit/nest/trips.service.test.ts": 2363 } diff --git a/server/src/db/repositories/TrekPhotos.repository.ts b/server/src/db/repositories/TrekPhotos.repository.ts index c37e95681..233f2d47d 100644 --- a/server/src/db/repositories/TrekPhotos.repository.ts +++ b/server/src/db/repositories/TrekPhotos.repository.ts @@ -147,6 +147,11 @@ export class TrekPhotosRepository extends TrekRepository { return await this.nativeDelete({ id, provider: { $ne: 'local' } }); } + /** `DELETE FROM trek_photos WHERE id = ?`, whatever the provider: a photo the user deleted. */ + async deleteById(id: number): Promise { + return await this.nativeDelete({ id }); + } + /** * TH1 — `SELECT file_path FROM trek_photos WHERE file_path LIKE 'journey/%'`. * The caller (`ThumbnailService.sweepOrphanThumbs`) always passes the diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index 42bc1412b..9b8bd7fbe 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/server/src/nest/collections/collections.service.ts @@ -1,11 +1,9 @@ import { DomainError } from '../common/domain-error'; import { restTripEvents, type TripBroadcast, type TripEventSink } from '../common/trip-writer'; -import path from 'path'; import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; import { PermissionsService } from '../permissions/permissions.service'; import { RealtimeService } from '../realtime/realtime.service'; -import { isUploadedPlaceImage } from '../places/place-image'; import { PlacesService } from '../places/places.service'; import { StorageService } from '../storage/storage.service'; import { Collections } from '../../db/entities/Collections.entity'; @@ -132,21 +130,6 @@ export class CollectionsService { private readonly places: PlacesService, // the journey hook of every place copyToTrip adds ) {} - /** - * Reclaim a replaced cover object (mirrors tripService.deleteOldCover — kept - * local so this service doesn't pull the trips import graph). Collection + - * trip covers share the 'covers' category; basename() tolerates the stored - * /uploads/covers/ URL form and any external URL the client saved - * (central key validation rejects hostile values; the catch swallows them - * like the old containment guard did). - */ - private async deleteOldCollectionCover(coverImage: string | null | undefined): Promise { - if (!coverImage) return; - await this.storage.delete('covers', path.basename(coverImage)).catch(() => { - /* external URL or already gone */ - }); - } - // ------------------------------------------------------------------------- // Visibility — a user may see/edit a collection if they own it OR are an // accepted member. Every read/write goes through assertAccess. @@ -618,12 +601,13 @@ export class CollectionsService { return { ...col, is_owner: col.owner_id === userId }; } - /** Set (or clear) a list's cover image, reclaiming the previous file. */ + /** + * Set (or clear) a list's cover image. The previous file stays where another + * row may name it; the storage reclaim job removes it once nothing does. + */ async setCollectionCover(userId: number, id: number, coverUrl: string | null, socketId?: string): Promise { await this.assertCanEdit(userId, id); - const prev = (await this.collectionsRepo.coverImage(id)) ?? null; await this.collectionsRepo.setCoverImage(id, coverUrl); - if (prev && prev !== coverUrl) await this.deleteOldCollectionCover(prev); await this.notifyCollectionUsers(id, socketId, 'collections:updated'); const col = await this.getCollectionRow(id); return { ...col, is_owner: col.owner_id === userId }; @@ -893,10 +877,6 @@ export class CollectionsService { const currentCollection = await this.collectionIdOfPlace(placeId); await this.assertCanEdit(userId, currentCollection); - // Capture the previous thumbnail so a replaced/cleared custom upload (#1136) - // can be reclaimed once nothing references it any more. - const prevImage = body.image_url !== undefined ? (await this.collectionPlaces.imageUrl(placeId)) ?? null : null; - const write: Parameters[1] = {}; if (body.name !== undefined) write.name = body.name; if (body.description !== undefined) write.description = body.description ?? null; @@ -934,10 +914,6 @@ export class CollectionsService { if (body.label_ids !== undefined) await this.setPlaceLabels(placeId, movedTo ?? currentCollection, body.label_ids); }); - if (body.image_url !== undefined && prevImage !== (body.image_url ?? null)) { - await this.reclaimPlaceImage(prevImage); - } - await this.notifyCollectionUsers(currentCollection, socketId, 'collections:updated'); if (movedTo) await this.notifyCollectionUsers(movedTo, socketId, 'collections:updated'); return this.getPlaceById(placeId); @@ -968,35 +944,13 @@ export class CollectionsService { return this.getPlaceById(placeId); } - /** - * Delete a custom saved-place-image object once nothing references it any - * more (Plan 3h Task 6, replacing `place-image.ts`'s free function — see - * that file's own doc comment). A trip place and a collection saved-place - * can share the same uploaded file — save-to-collection and copy-to-trip - * copy `image_url` by reference — so this ref-counts across both tables - * before deleting, mirroring `PlacesService.reclaimPlaceImage`'s shape - * exactly: `places` first via `PlacesRepository.existsByImageUrl` - * (`tripPlaces`, already injected), `collection_places` second via - * `CollectionPlacesRepository.existsByImageUrl` (`collectionPlaces`, - * already injected), JS-level `||` short-circuit preserved by early - * return. `path.basename()` keeps the storage name confined to the - * 'places' category. Best-effort: never throws. - */ - private async reclaimPlaceImage(url: string | null | undefined): Promise { - if (!isUploadedPlaceImage(url)) return; - if (await this.tripPlaces.existsByImageUrl(url)) return; - if (await this.collectionPlaces.existsByImageUrl(url)) return; - await this.storage.delete('places', path.basename(url)).catch(() => { - /* best-effort */ - }); - } - + // A saved place's uploaded image is never deleted here: copy-to-trip and + // save-to-collection share it by reference, and other columns can name it + // too. The storage reclaim job removes it once no row does (#2924). async deletePlace(userId: number, placeId: number, socketId?: string): Promise { const collectionId = await this.collectionIdOfPlace(placeId); await this.assertCanDelete(userId, collectionId); - const image = (await this.collectionPlaces.imageUrl(placeId)) ?? null; await this.collectionPlaces.deleteById(placeId); // CASCADE drops tags. NO photo-cache reclaim. - await this.reclaimPlaceImage(image); await this.notifyCollectionUsers(collectionId, socketId, 'collections:updated'); } @@ -1007,11 +961,9 @@ export class CollectionsService { // and the deletes then run in one transaction. const deleted: number[] = []; const touched = new Set(); - const images: (string | null)[] = []; for (const id of ids) { const collectionId = await this.collectionIdOfPlace(id); await this.assertCanDelete(userId, collectionId); - images.push((await this.collectionPlaces.imageUrl(id)) ?? null); touched.add(collectionId); } await this.uow.transactional(async () => { @@ -1020,7 +972,6 @@ export class CollectionsService { deleted.push(id); } }); - for (const image of images) await this.reclaimPlaceImage(image); for (const cid of touched) await this.notifyCollectionUsers(cid, socketId, 'collections:updated'); return deleted; } @@ -1108,13 +1059,11 @@ export class CollectionsService { return this.collectionPlaces.matchingByTripSource(collectionIds, tripId, place, COORD_DEDUP_TOLERANCE); } - /** Set (or clear) a saved place's custom thumbnail, reclaiming the previous upload. */ + /** Set (or clear) a saved place's custom thumbnail; the storage reclaim job takes the replaced upload. */ async setPlaceImage(userId: number, placeId: number, imageUrl: string | null, socketId?: string): Promise { const collectionId = await this.collectionIdOfPlace(placeId); await this.assertCanEdit(userId, collectionId); - const prev = (await this.collectionPlaces.imageUrl(placeId)) ?? null; await this.collectionPlaces.setImageUrl(placeId, imageUrl); - if (prev !== imageUrl) await this.reclaimPlaceImage(prev); await this.notifyCollectionUsers(collectionId, socketId, 'collections:updated'); return this.getPlaceById(placeId); } diff --git a/server/src/nest/journey/journey-domain.service.ts b/server/src/nest/journey/journey-domain.service.ts index d01364c3d..b2dfa4faf 100644 --- a/server/src/nest/journey/journey-domain.service.ts +++ b/server/src/nest/journey/journey-domain.service.ts @@ -1496,7 +1496,7 @@ export class JourneyDomainService { // JG107 — cascade on journey_entry_photos.journey_photo_id handles junction cleanup. await this.uow.transactional(async () => { await this.photosRepo.deleteById(journeyPhotoId); - await this.photos.deleteIfOrphan(row.photo_id); + await this.photos.deleteIfOrphan(row.photo_id, { local: true }); }); return { photo_id: row.photo_id, file_path: trekRow?.file_path ?? null, thumbnail_path: trekRow?.thumbnail_path ?? null }; @@ -1558,7 +1558,7 @@ export class JourneyDomainService { // JG116 — same text as JG107. await this.uow.transactional(async () => { await this.photosRepo.deleteById(photoId); - await this.photos.deleteIfOrphan(row.photo_id); + await this.photos.deleteIfOrphan(row.photo_id, { local: true }); }); return { id: row.id, photo_id: row.photo_id, file_path: trekRow?.file_path ?? null, thumbnail_path: trekRow?.thumbnail_path ?? null, journey_id: row.journey_id }; diff --git a/server/src/nest/journey/journey.controller.ts b/server/src/nest/journey/journey.controller.ts index cdbd15b2b..cf40a6820 100644 --- a/server/src/nest/journey/journey.controller.ts +++ b/server/src/nest/journey/journey.controller.ts @@ -25,7 +25,6 @@ import type { User } from '../../types'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; import { isDemoWriteBlocked, DEMO_WRITE_ERROR } from '../common/demo-write'; import { StorageService } from '../storage/storage.service'; -import { journeyThumbName } from '../memories/thumbnail.service'; import { JourneyService } from './journey.service'; import { JourneyBookService } from './journey-book.service'; import { JourneyPhotoCaptureService, type ProviderPhotoBatch, type ProviderPhotoTarget } from './journey-photo-capture.service'; @@ -460,42 +459,16 @@ export class JourneyController { @Delete('photos/:photoId') @ResponseContract(successResponseSchema) async deletePhoto(@CurrentUser() user: User, @Param('photoId') photoId: string) { + // The bytes stay: another journey, a trip or a cover may name the same + // object. Once the photo row is gone the storage reclaim job takes the file + // and its poster, and the thumbnail sweep its thumbnail (#2924). const photo = await this.journey.deletePhoto(Number(photoId), user.id); if (!photo) { throw new HttpException({ error: 'Photo not found' }, 404); } - await this.deletePhotoObject(photo.file_path); - await this.reclaimDerived(photo.file_path, photo.thumbnail_path); return { success: true }; } - /** - * Best-effort removal of a deleted photo's bytes. file_path rows are the - * uploads-relative 'journey/' form every writer stores; anything else - * (provider rows have null) has no local object. - */ - private async deletePhotoObject(filePath: string | null | undefined): Promise { - if (!filePath) return; - const name = filePath.startsWith('journey/') ? filePath.slice('journey/'.length) : null; - if (!name) return; - await this.storage.delete('journey', name).catch(() => { /* file already gone */ }); - } - - /** - * Delete a photo's derived artifacts with it (spec In-scope fix #2): the - * hash-named thumbnail derivable from file_path, and whatever - * thumbnail_path recorded — a generated thumb (usually the same object) or - * a video's uploaded poster. Both best-effort, both idempotent. - */ - private async reclaimDerived(filePath: string | null | undefined, thumbnailPath: string | null | undefined): Promise { - if (filePath?.startsWith('journey/')) { - await this.storage.delete('journey', journeyThumbName(filePath)).catch(() => { /* already gone */ }); - } - if (thumbnailPath?.startsWith('journey/')) { - await this.storage.delete('journey', thumbnailPath.slice('journey/'.length)).catch(() => { /* already gone */ }); - } - } - // ── Gallery (prefix /:id/gallery — before /:id) ───────────────────────── @Post(':id/gallery/photos') @UseInterceptors(FilesInterceptor('photos')) @@ -573,12 +546,11 @@ export class JourneyController { @HttpCode(204) @ResponseContract(emptyResponseSchema) async deleteGalleryPhoto(@CurrentUser() user: User, @Param('journeyPhotoId') journeyPhotoId: string): Promise { + // The bytes stay, as for deletePhoto above. const photo = await this.journey.deleteGalleryPhoto(Number(journeyPhotoId), user.id); if (!photo) { throw new HttpException({ error: 'Photo not found or not allowed' }, 404); } - await this.deletePhotoObject(photo.file_path); - await this.reclaimDerived(photo.file_path, photo.thumbnail_path); } // ── Journeys /:id ─────────────────────────────────────────────────────── diff --git a/server/src/nest/photos/trek-photo-registration.service.ts b/server/src/nest/photos/trek-photo-registration.service.ts index b31e9d549..6d24a563b 100644 --- a/server/src/nest/photos/trek-photo-registration.service.ts +++ b/server/src/nest/photos/trek-photo-registration.service.ts @@ -145,8 +145,10 @@ export class TrekPhotoRegistrationService { /** * PH10/PH11 — drop the row once no trip and no journey references it. - * Local photos are kept: their bytes are ours, and the file would outlive - * the row. + * A local photo is kept unless the caller deletes it on purpose (`local`, + * a journey photo the user deleted): its row is what keeps its bytes, and + * once it is gone the storage reclaim job removes them when no other row + * names the file (#2924). An unlink alone keeps a local photo. * * PH10's original statement was one `UNION ALL` existence check across * `trip_photos` and `journey_photos`; split into its two halves here @@ -156,9 +158,10 @@ export class TrekPhotoRegistrationService { * to run if the other already found a row), never a transaction (both are * reads). */ - async deleteIfOrphan(photoId: number): Promise { + async deleteIfOrphan(photoId: number, options: { local?: boolean } = {}): Promise { if (await this.tripPhotos.existsForPhoto(photoId)) return; if (await this.journeyPhotos.existsForPhoto(photoId)) return; // PH10 — converted (Plan 3g Task 4) - await this.trekPhotos.deleteNonLocal(photoId); + if (options.local) await this.trekPhotos.deleteById(photoId); + else await this.trekPhotos.deleteNonLocal(photoId); } } diff --git a/server/src/nest/places/place-image.ts b/server/src/nest/places/place-image.ts index f2ba2942b..016e77db9 100644 --- a/server/src/nest/places/place-image.ts +++ b/server/src/nest/places/place-image.ts @@ -4,22 +4,7 @@ export function placeImageUrl(filename: string): string { return `${URL_PREFIX}${filename}`; } -export function isUploadedPlaceImage(url: string | null | undefined): url is string { - return typeof url === 'string' && url.startsWith(URL_PREFIX); -} - -// `reclaimPlaceImage` (the free function this file used to export) is gone -// (Plan 3h Task 6, closing out a 3-plan-old carve-out): `PlacesService` had -// its own converted private `reclaimPlaceImage` since Plan 3c Task 4 -// (`places.service.ts`, PI1/PI2 through `PlacesRepository`/ -// `CollectionPlacesRepository.existsByImageUrl`); `collections.service.ts`'s -// four call sites (`updatePlace`, `deletePlace`, `deletePlacesMany`, -// `setPlaceImage`) now call a mirroring private `CollectionsService -// .reclaimPlaceImage` method instead, through `PlacesRepository -// .existsByImageUrl` (already injected there, `tripPlaces`) and -// `CollectionPlacesRepository.existsByImageUrl` (already injected, -// `collectionPlaces`) — a whole-tree grep for `from '.*place-image'` (run -// before this deletion) confirmed zero remaining importers of this function; -// `placeImageUrl`/`isUploadedPlaceImage` above still have real importers -// (`collections.controller.ts`, `places.controller.ts`, `places.service.ts`) -// and stay. +// No place or saved-place write deletes a replaced image any more: copies +// share the object by reference and other columns can name it, so the storage +// reclaim job removes it once no row does (#2924, places-storage-refs.service.ts +// and collections-storage-refs.service.ts answer for these columns). diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 0ac0c1678..1ccc8b0c5 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -20,7 +20,7 @@ import { ratingAggregate } from '../common/rowShape'; import { UnsplashService } from '../unsplash/unsplash.service'; import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service'; import { type UpdateConflict, isUpdateConflict } from '../common/conflictResult'; -import { isUploadedPlaceImage, placeImageUrl } from './place-image'; +import { placeImageUrl } from './place-image'; import { MAX_PLACE_IMAGE_SIZE, PLACE_IMAGE_EXTENSIONS } from '../common/place-image-upload'; import { randomUUID } from 'node:crypto'; import { JourneyDomainService } from '../journey/journey-domain.service'; @@ -165,7 +165,8 @@ export class PlacesService { @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, // Plan 3e Task 2 (budget) — additive, PL15/PL18/PL22 only. @InjectRepository(BudgetItems) private readonly budgetItemsRepo: BudgetItemsRepository, - // Plan 3h Task 6 (survivors) — additive, SV-PI2's `reclaimPlaceImage` only. + // Plan 3h Task 6 (survivors): read by nothing since the storage reclaim job + // took over the image ref-count (#2924); kept so the constructor stays put. @InjectRepository(CollectionPlaces) private readonly collectionPlacesRepo: CollectionPlacesRepository, // Reading files and shared lists; this service only persists what it reads. private readonly placeImport: PlaceImportService, @@ -244,37 +245,6 @@ export class PlacesService { this.realtime.broadcast(tripId, event, payload, socketId); } - /** - * Delete a custom place-image object once nothing references it any more - * (moved in from `place-image.ts`'s free function `reclaimPlaceImage`, - * option (a) — see `place-image.ts`'s own docstring for why that free - * function stays, unconverted, for `collections.service.ts`'s own call - * sites). A trip place and a collection saved-place can share the same - * uploaded file — save-to-collection and copy-to-trip copy `image_url` by - * reference — so this ref-counts across both tables before deleting. - * - * PI1 (`SELECT 1 FROM places WHERE image_url = ? LIMIT 1`) through - * `PlacesRepository.existsByImageUrl`; PI2 (`SELECT 1 FROM collection_places - * WHERE image_url = ? LIMIT 1`) through `CollectionPlacesRepository - * .existsByImageUrl` (Plan 3h Task 6, closing out the carve-out), evaluated - * only when PI1 is false — reproducing the legacy `UNION ALL … LIMIT 1`'s - * short-circuit by evaluation order, the same pattern - * `PlacePhotoCacheService.isReferenced` uses for the same two tables - * (Plan 3c Task 1's PP6 ruling). `path.basename()` keeps the storage name - * confined to the 'places' category. Best-effort: never throws (central - * key validation rejects a hostile stored value; the catch swallows it - * exactly like the legacy unlink guard). - */ - private async reclaimPlaceImage(url: string | null | undefined): Promise { - if (!isUploadedPlaceImage(url)) return; - if (await this.placesRepo.existsByImageUrl(url)) return; - // PI2 (Plan 3h Task 6) — `CollectionPlacesRepository.existsByImageUrl`. - if (await this.collectionPlacesRepo.existsByImageUrl(url)) return; - await this.storage.delete('places', path.basename(url)).catch(() => { - /* best-effort */ - }); - } - // ------------------------------------------------------------------------- // List places // ------------------------------------------------------------------------- @@ -319,7 +289,7 @@ export class PlacesService { /** * Makes a picture already attached in the trip the place's own image (#1242). The file * is copied, not pointed at: the place keeps its picture when the attachment is deleted, - * and the update path reclaims the copy like any other uploaded image. Returns a reason + * and the storage reclaim job takes the copy like any other uploaded image. Returns a reason * string when the file cannot serve, the updated place otherwise. */ async setImageFromFile(tripId: string, placeId: string, fileId: number): Promise<'not_found' | 'not_image' | 'too_large' | Awaited>> { @@ -335,7 +305,8 @@ export class PlacesService { const { stream } = await this.storage.getStream('files', path.basename(file.filename)); await this.storage.put('places', name, stream, { contentType: mime }); const updated = await this.update(tripId, placeId, { image_url: placeImageUrl(name) } as never); - if (!updated || isUpdateConflict(updated)) await this.reclaimPlaceImage(placeImageUrl(name)); + // The copy is this request's own until the update names it, so a failed update drops it here. + if (!updated || isUpdateConflict(updated)) await this.storage.delete('places', name).catch(() => undefined); return updated; } @@ -447,22 +418,20 @@ export class PlacesService { body: PlaceUpdateInput, ifMatch?: string, ): Promise { - const { result, reclaim } = await this.uow.transactional(() => this.applyUpdate(tripId, placeId, body, ifMatch)); - if (reclaim !== undefined) await this.reclaimPlaceImage(reclaim); + const { result } = await this.uow.transactional(() => this.applyUpdate(tripId, placeId, body, ifMatch)); return result; } /** * The DB half of update(). Split out so updateMany() can run it inside the - * same UnitOfWork transaction and settle the storage reclaims after the - * transaction commits. + * same UnitOfWork transaction. */ private async applyUpdate( tripId: string, placeId: string, body: PlaceUpdateInput, ifMatch?: string, - ): Promise<{ result: PlaceWithTags | UpdateConflict | null; reclaim?: string | null }> { + ): Promise<{ result: PlaceWithTags | UpdateConflict | null }> { // PL9 — the pre-image read every `!== undefined` fallback below needs. // `toRowId` first (Task 3 review H1, absorbed here): every write in // this method (`updatePlace`, `tagsRepo.deleteForPlace`/`insertIgnore`) @@ -555,14 +524,9 @@ export class PlacesService { if (tags.length > 0) await this.tagsRepo.insertIgnore(id, await this.tagsOnTrip(tid, tags)); // PL13 } - // A custom uploaded thumbnail (#1136) that was just replaced or cleared leaves - // an orphan file behind — reclaim it (in the caller, once any enclosing - // transaction committed) if nothing references it any more. - const reclaim = image_url !== undefined && image_url !== existingPlace.image_url - ? existingPlace.image_url - : undefined; - - return { result: await this.placesRepo.findWithTagsAndRatings(id), reclaim }; // PL14 + // A replaced or cleared custom upload (#1136) stays: a copy of this place or + // a cover may name it, and the storage reclaim job removes it once none does. + return { result: await this.placesRepo.findWithTagsAndRatings(id) }; // PL14 } // ------------------------------------------------------------------------- @@ -654,7 +618,6 @@ export class PlacesService { await this.placesRepo.deleteById(id); }); await reclaimPhotoCache(this.photoCache, place.google_place_id, place.image_url); - await this.reclaimPlaceImage(place.image_url); return { deleted: true, deletedTourPlaceIds: wasTour ? [id] : [], cancelled }; } @@ -699,7 +662,6 @@ export class PlacesService { // Reclaim after the transaction commits so isReferenced() sees the final place set. for (const row of reclaimable) { await reclaimPhotoCache(this.photoCache, row.google_place_id, row.image_url); - await this.reclaimPlaceImage(row.image_url); } return { deleted, deletedTourPlaceIds, cancelled }; } @@ -728,19 +690,14 @@ export class PlacesService { async updateMany(tripId: string, ids: number[], body: PlaceUpdateInput): Promise { if (ids.length === 0) return []; const updated: PlaceWithTags[] = []; - const reclaims: (string | null)[] = []; await this.uow.transactional(async () => { for (const id of ids) { // Bulk update sends no If-Match, so applyUpdate() never returns a // conflict here; the guard keeps the types honest. - const { result: place, reclaim } = await this.applyUpdate(tripId, String(id), body); + const { result: place } = await this.applyUpdate(tripId, String(id), body); if (place && !isUpdateConflict(place)) updated.push(place); - if (reclaim !== undefined) reclaims.push(reclaim); } }); - // Settle reclaims after the transaction commits, so the refcount sees the - // final image_url state. - for (const reclaim of reclaims) await this.reclaimPlaceImage(reclaim); return updated; } diff --git a/server/src/nest/trips/trips.controller.ts b/server/src/nest/trips/trips.controller.ts index 75d9f510a..9dabccb57 100644 --- a/server/src/nest/trips/trips.controller.ts +++ b/server/src/nest/trips/trips.controller.ts @@ -200,15 +200,8 @@ export class TripsController { throw new HttpException({ error: 'Could not save the selected cover image' }, 502); } } - const oldCover = - body.cover_image !== undefined - ? ((await this.trips.getRaw(id)) as { cover_image: string | null } | undefined)?.cover_image - : undefined; try { const result = await this.trips.update(id, user.id, body, user.role); - if (body.cover_image !== undefined && body.cover_image !== oldCover) { - await this.trips.deleteOldCover(oldCover); - } if (Object.keys(result.changes).length > 0) { await this.audit.writeAudit({ userId: user.id, @@ -264,8 +257,9 @@ export class TripsController { } // Commit the spooled upload to its final storage location (atomic // same-volume rename) before anything references the final path. + // The replaced cover stays: a trip copy or a journey may show the same + // object, and the storage reclaim job removes it once nothing names it. await this.storage.put('covers', file.filename, { tmpPath: file.path }); - await this.trips.deleteOldCover(trip.cover_image); const coverUrl = `/uploads/covers/${file.filename}`; await this.trips.updateCoverImage(id, coverUrl); return { cover_image: coverUrl }; diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index f4df10137..53223f1d0 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -1,5 +1,4 @@ import { Injectable } from '@nestjs/common'; -import path from 'path'; import { EntityManager } from '@mikro-orm/core'; import { Trips } from '../../db/entities/Trips.entity'; import { Days } from '../../db/entities/Days.entity'; @@ -624,17 +623,6 @@ export class TripsService { // ── Cover image ─────────────────────────────────────────────────────────── - async deleteOldCover(coverImage: string | null | undefined): Promise { - if (!coverImage) return; - // cover_image is client-supplied, so treat it as untrusted: covers are flat - // filenames in the 'covers' category — basename() confines the delete to - // it, and central key validation rejects anything hostile (swallowed like - // the old containment guard; an external https URL is likewise a no-op). - await this.storage.delete('covers', path.basename(coverImage)).catch(() => { - /* external URL or already gone */ - }); - } - /** TP35 — `TripsRepository.setCoverImage`. */ async updateCoverImage(tripId: string | number, coverUrl: string): Promise { await this.tripsRepo.setCoverImage(Number(tripId), coverUrl); // safe: only called after `getRaw`/`getOwner` matched the same id through the raw-bind seam diff --git a/server/tests/integration/trips.test.ts b/server/tests/integration/trips.test.ts index fd2ff73f7..722c8050f 100644 --- a/server/tests/integration/trips.test.ts +++ b/server/tests/integration/trips.test.ts @@ -1301,7 +1301,7 @@ describe('Trip cover upload parity', () => { expect(fsMod.existsSync(pathMod.join(coversDir, diskName))).toBe(true); }); - it('TRIP-P02 — re-upload deletes the previous cover file', async () => { + it('TRIP-P02 — re-upload keeps the previous cover file for the storage reclaim job (#2924)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -1317,7 +1317,8 @@ describe('Trip cover upload parity', () => { .set('Cookie', authCookie(user.id)) .attach('cover', FIXTURE_IMG, 'two.jpg'); expect(second.status).toBe(201); - expect(fsMod.existsSync(pathMod.join(coversDir, firstName))).toBe(false); + // A trip copy or a journey may show the same object, so the replace leaves it. + expect(fsMod.existsSync(pathMod.join(coversDir, firstName))).toBe(true); }); it('TRIP-P03 — no file → 400 "No image uploaded"', async () => { diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index 34ff57c63..3acd7586d 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -846,7 +846,7 @@ describe('custom saved-place image', () => { expect(updated.image_url).toBe('/uploads/places/col-set.jpg'); }); - it('COLLECTIONS-SVC-061: setPlaceImage stores the url and reclaims a replaced upload', async () => { + it('COLLECTIONS-SVC-061: setPlaceImage stores the url and leaves a replaced upload to the reclaim job', async () => { const u = createUser(testDb).user; const col = await svc.createCollection(u.id, { name: 'Photos' }); const p = (await svc.savePlace(u.id, { collection_id: col.id, name: 'Pic' })).place!; @@ -856,10 +856,12 @@ describe('custom saved-place image', () => { const res = await svc.setPlaceImage(u.id, p.id, '/uploads/places/col-replace-b.jpg'); expect(res.image_url).toBe('/uploads/places/col-replace-b.jpg'); - expect(fs.existsSync(fileA)).toBe(false); + // A trip copy may still show it; the storage reclaim job decides (#2924). + expect(fs.existsSync(fileA)).toBe(true); + fs.unlinkSync(fileA); }); - it('COLLECTIONS-SVC-062: deletePlace reclaims the uploaded image when unreferenced', async () => { + it('COLLECTIONS-SVC-062: deletePlace leaves the uploaded image to the reclaim job', async () => { const u = createUser(testDb).user; const col = await svc.createCollection(u.id, { name: 'Photos' }); const p = (await svc.savePlace(u.id, { collection_id: col.id, name: 'Pic', image_url: '/uploads/places/col-delete.jpg' })).place!; @@ -867,7 +869,8 @@ describe('custom saved-place image', () => { expect(fs.existsSync(fileA)).toBe(true); await svc.deletePlace(u.id, p.id); - expect(fs.existsSync(fileA)).toBe(false); + expect(fs.existsSync(fileA)).toBe(true); + fs.unlinkSync(fileA); }); }); diff --git a/server/tests/unit/nest/journey.controller.test.ts b/server/tests/unit/nest/journey.controller.test.ts index c151f43a3..3bd6c3d82 100644 --- a/server/tests/unit/nest/journey.controller.test.ts +++ b/server/tests/unit/nest/journey.controller.test.ts @@ -8,7 +8,6 @@ import type { JourneyPhotoLinksService } from '../../../src/nest/journey/journey import { JourneyPublicController } from '../../../src/nest/journey/journey-public.controller'; import { JourneyController } from '../../../src/nest/journey/journey.controller'; import type { JourneyService } from '../../../src/nest/journey/journey.service'; -import { journeyThumbName } from '../../../src/nest/memories/thumbnail.service'; import type { StorageService } from '../../../src/nest/storage/storage.service'; import type { User } from '../../../src/types'; import { HttpException } from '@nestjs/common'; @@ -706,31 +705,14 @@ describe('JourneyController', () => { expect(updatePhoto).toHaveBeenCalledWith(7, 1, { caption: 'x' }, 'sock-1'); }); - it('DELETE photo removes the storage object when a path exists', async () => { - expect( - await ctl( - svc({ deletePhoto: vi.fn().mockReturnValue({ id: 7, file_path: 'journey/a.jpg' }) } as Partial), - ).deletePhoto(user, '7'), - ).toEqual({ success: true }); - expect(storageStub.delete).toHaveBeenCalledWith('journey', 'a.jpg'); - // ...and the derived thumb goes with it (spec fix #2). - expect(storageStub.delete).toHaveBeenCalledWith('journey', journeyThumbName('journey/a.jpg')); - // a vanished object is swallowed - (storageStub.delete as ReturnType).mockRejectedValueOnce(new Error('ENOENT')); - expect( - await ctl( - svc({ deletePhoto: vi.fn().mockReturnValue({ id: 8, file_path: 'journey/b.jpg' }) } as Partial), - ).deletePhoto(user, '8'), - ).toEqual({ success: true }); - // a file_path outside the journey category never reaches storage + it('DELETE photo leaves the bytes to the storage reclaim job (#2924)', async () => { (storageStub.delete as ReturnType).mockClear(); - expect( - await ctl( - svc({ - deletePhoto: vi.fn().mockReturnValue({ id: 9, file_path: 'elsewhere/x.jpg' }), - } as Partial), - ).deletePhoto(user, '9'), - ).toEqual({ success: true }); + const deletePhoto = vi.fn().mockReturnValue({ id: 7, file_path: 'journey/a.jpg', thumbnail_path: 'journey/p.jpg' }); + expect(await ctl(svc({ deletePhoto } as Partial)).deletePhoto(user, '7')).toEqual({ + success: true, + }); + expect(deletePhoto).toHaveBeenCalledWith(7, 1); + // Another row may name the same object: the photo row going is what frees it. expect(storageStub.delete).not.toHaveBeenCalled(); }); @@ -761,7 +743,7 @@ describe('JourneyController', () => { expect(await ctl(svc()).galleryProviderPhotos(user, '9', { provider: 'immich', asset_id: 'a' })).toEqual({ id: 3 }); }); - it('DELETE gallery photo: 404, then removes the object when present', async () => { + it('DELETE gallery photo: 404, then leaves the bytes and the poster to the reclaim job', async () => { expect( await thrownAsync(() => ctl(svc({ deleteGalleryPhoto: vi.fn().mockReturnValue(null) } as Partial)).deleteGalleryPhoto( @@ -770,41 +752,15 @@ describe('JourneyController', () => { ), ), ).toEqual({ status: 404, body: { error: 'Photo not found or not allowed' } }); - // no file_path → nothing to delete, returns void - expect( - await ctl( - svc({ deleteGalleryPhoto: vi.fn().mockReturnValue({ id: 7, file_path: null }) } as Partial), - ).deleteGalleryPhoto(user, '7'), - ).toBeUndefined(); - await ctl( - svc({ - deleteGalleryPhoto: vi.fn().mockReturnValue({ id: 8, file_path: 'journey/g.jpg' }), - } as Partial), - ).deleteGalleryPhoto(user, '8'); - expect(storageStub.delete).toHaveBeenCalledWith('journey', 'g.jpg'); - expect(storageStub.delete).toHaveBeenCalledWith('journey', journeyThumbName('journey/g.jpg')); - (storageStub.delete as ReturnType).mockRejectedValueOnce(new Error('ENOENT')); - expect( - await ctl( - svc({ - deleteGalleryPhoto: vi.fn().mockReturnValue({ id: 9, file_path: 'journey/h.jpg' }), - } as Partial), - ).deleteGalleryPhoto(user, '9'), - ).toBeUndefined(); - }); - - it('DELETE gallery video also reclaims its uploaded poster (spec fix #2 extension)', async () => { (storageStub.delete as ReturnType).mockClear(); - await ctl( - svc({ - deleteGalleryPhoto: vi - .fn() - .mockReturnValue({ id: 10, file_path: 'journey/clip.mp4', thumbnail_path: 'journey/poster.jpg' }), - } as Partial), - ).deleteGalleryPhoto(user, '10'); - expect(storageStub.delete).toHaveBeenCalledWith('journey', 'clip.mp4'); - expect(storageStub.delete).toHaveBeenCalledWith('journey', journeyThumbName('journey/clip.mp4')); - expect(storageStub.delete).toHaveBeenCalledWith('journey', 'poster.jpg'); + const deleteGalleryPhoto = vi + .fn() + .mockReturnValue({ id: 10, file_path: 'journey/clip.mp4', thumbnail_path: 'journey/poster.jpg' }); + expect( + await ctl(svc({ deleteGalleryPhoto } as Partial)).deleteGalleryPhoto(user, '10'), + ).toBeUndefined(); + expect(deleteGalleryPhoto).toHaveBeenCalledWith(10, 1); + expect(storageStub.delete).not.toHaveBeenCalled(); }); it('PATCH /:id returns the updated journey on success', async () => { diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index c2a4a9f4e..8ea325040 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -1498,16 +1498,16 @@ describe('importKmlPlaces — full stored row and atomicity (M2/L2)', () => { }); }); -// ── Custom place image reclaim (#1136) ────────────────────────────────────────── +// ── Custom place images stay for the storage reclaim job (#1136, #2924) ────────────────────────────────────────── -describe('custom place image reclaim', () => { +describe('custom place images', () => { function writePlaceImage(name: string): string { const filePath = path.join(placesStorageFx.root, name); fs.writeFileSync(filePath, 'jpeg-bytes'); return filePath; } - it('PLACE-SVC-046 — replacing image_url unlinks the previous upload once unreferenced', async () => { + it('PLACE-SVC-046 — replacing image_url keeps the previous upload for the reclaim job', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'Photo' }) as any; @@ -1516,10 +1516,10 @@ describe('custom place image reclaim', () => { expect(fs.existsSync(fileA)).toBe(true); await svc.update(String(trip.id), String(place.id), { image_url: '/uploads/places/svc-replace-b.jpg' }); - expect(fs.existsSync(fileA)).toBe(false); + expect(fs.existsSync(fileA)).toBe(true); }); - it('PLACE-SVC-047 — clearing image_url to null unlinks the previous upload', async () => { + it('PLACE-SVC-047 — clearing image_url to null keeps the previous upload', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'Photo' }) as any; @@ -1528,10 +1528,10 @@ describe('custom place image reclaim', () => { expect(fs.existsSync(fileA)).toBe(true); await svc.update(String(trip.id), String(place.id), { image_url: null } as any); - expect(fs.existsSync(fileA)).toBe(false); + expect(fs.existsSync(fileA)).toBe(true); }); - it('PLACE-SVC-048 — remove unlinks the uploaded image', async () => { + it('PLACE-SVC-048 — remove keeps the uploaded image', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'Photo' }) as any; @@ -1540,7 +1540,7 @@ describe('custom place image reclaim', () => { expect(fs.existsSync(fileA)).toBe(true); await svc.remove(String(trip.id), String(place.id)); - expect(fs.existsSync(fileA)).toBe(false); + expect(fs.existsSync(fileA)).toBe(true); }); it('PLACE-SVC-049 — a collection_places reference keeps the file when the trip place is deleted', async () => { diff --git a/server/tests/unit/nest/trek-photo-registration.service.test.ts b/server/tests/unit/nest/trek-photo-registration.service.test.ts index f8f10b457..0bc1545de 100644 --- a/server/tests/unit/nest/trek-photo-registration.service.test.ts +++ b/server/tests/unit/nest/trek-photo-registration.service.test.ts @@ -272,6 +272,22 @@ describe('TrekPhotoRegistrationService.deleteIfOrphan (PH10-11)', () => { await repo.deleteIfOrphan(id); expect(await rawRow(id)).not.toBeNull(); }); + + it('TREKPHOTO-064: a LOCAL photo the user deleted goes once nothing links it, so its bytes can be reclaimed', async () => { + const id = await repo.getOrCreateLocal('journey/deleted-by-user.jpg'); + await repo.deleteIfOrphan(id, { local: true }); + expect(await rawRow(id)).toBeNull(); + }); + + it('TREKPHOTO-065: a deleted LOCAL photo another trip still links stays', async () => { + const { user } = createUser(testDb); + const id = await repo.getOrCreateLocal('journey/still-on-a-trip.jpg'); + const trip = createTrip(testDb, user.id); + await insertRow(t, TripPhotos, { trip: trip.id, user: user.id, photo: id, shared: 1 }); + + await repo.deleteIfOrphan(id, { local: true }); + expect(await rawRow(id)).not.toBeNull(); + }); }); describe('M1: TripAlbumLinksRepository/TripPhotosRepository onConflict + empty-array branches', () => { diff --git a/server/tests/unit/nest/trips.controller.test.ts b/server/tests/unit/nest/trips.controller.test.ts index cace9ba70..2e5ac3946 100644 --- a/server/tests/unit/nest/trips.controller.test.ts +++ b/server/tests/unit/nest/trips.controller.test.ts @@ -448,10 +448,8 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = const update = vi .fn() .mockReturnValue({ updatedTrip: { id: 9 }, changes: {}, newTitle: 'b', newReminder: 0, oldReminder: 0 }); - const deleteOldCover = vi.fn(); const s = svc({ update, - deleteOldCover, getRaw: vi.fn().mockReturnValue({ cover_image: null }), } as Partial); await tc(s).update(user, '9', { cover_image: 'https://images.unsplash.com/photo-123?w=1080' }, req); @@ -591,16 +589,15 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = ), ), ).toEqual({ status: 400, body: { error: 'No image uploaded' } }); - const deleteOldCover = vi.fn(); const updateCoverImage = vi.fn(); const s = svc({ getRaw: vi.fn().mockReturnValue({ cover_image: '/old.jpg' }), - deleteOldCover, updateCoverImage, } as Partial); expect(await tc(s).cover(user, '9', file)).toEqual({ cover_image: '/uploads/covers/abc.jpg' }); expect(storageStub.put).toHaveBeenCalledWith('covers', 'abc.jpg', { tmpPath: undefined }); - expect(deleteOldCover).toHaveBeenCalledWith('/old.jpg'); + // The replaced cover is left to the storage reclaim job: a copy may share it (#2924). + expect(storageStub.put).toHaveBeenCalledTimes(1); expect(updateCoverImage).toHaveBeenCalledWith('9', '/uploads/covers/abc.jpg'); }); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index ffd5c078d..db4c598af 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -68,8 +68,6 @@ import { JourneyEntries } from '../../../src/db/entities/JourneyEntries.entity'; import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; import { makeStorageFixture } from '../../helpers/storage-fixture'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; -import fs from 'fs'; -import path from 'path'; import { notificationsStub } from '../../helpers/notifications'; import { EphemeralTokenService } from '../../../src/nest/auth-core/ephemeral-token.service'; import { @@ -855,45 +853,6 @@ describe('generateDays', () => { }); }); -// ── deleteOldCover — path containment ────────────────────────────────────────── - -describe('deleteOldCover', () => { - it('TRIP-SVC-COVER-001: never deletes outside the covers category for a crafted cover_image', async () => { - // Attacker-controlled values aimed at auth-gated sibling upload dirs — the - // basename + category addressing keeps every delete inside covers/. - const filesDir = path.join(coversFx.root, 'files'); - const avatarsDir = path.join(coversFx.root, 'avatars'); - fs.mkdirSync(filesDir, { recursive: true }); - fs.mkdirSync(avatarsDir, { recursive: true }); - const secret = path.join(filesDir, 'secret.pdf'); - const someone = path.join(avatarsDir, 'someone.png'); - fs.writeFileSync(secret, 'pdf'); - fs.writeFileSync(someone, 'png'); - - await svc.deleteOldCover('/uploads/files/secret.pdf'); - await svc.deleteOldCover('/uploads/covers/../files/secret.pdf'); - await svc.deleteOldCover('/uploads/avatars/someone.png'); - - expect(fs.existsSync(secret)).toBe(true); - expect(fs.existsSync(someone)).toBe(true); - }); - - it('TRIP-SVC-COVER-002: deletes a legitimate cover file', async () => { - const coversDir = path.join(coversFx.root, 'covers'); - fs.mkdirSync(coversDir, { recursive: true }); - const cover = path.join(coversDir, 'abc123.jpg'); - fs.writeFileSync(cover, 'jpeg'); - - await svc.deleteOldCover('/uploads/covers/abc123.jpg'); - expect(fs.existsSync(cover)).toBe(false); - }); - - it('TRIP-SVC-COVER-003: an external https cover URL is tolerated (no throw)', async () => { - await expect(svc.deleteOldCover('https://example.com/some/pic.jpg')).resolves.toBeUndefined(); - await expect(svc.deleteOldCover(null)).resolves.toBeUndefined(); - }); -}); - describe('resyncReservationDays (#1288)', () => { const dayFor = async (tripId: number, date: string) => (await storedFields(Days, { trip: tripId, date }, ['id']) as { id: number }).id;