From 786b1f5c07a7b663ff7fea17d24c1b9553648ab3 Mon Sep 17 00:00:00 2001 From: jubnl Date: Sun, 16 Aug 2026 11:04:04 +0200 Subject: [PATCH] refactor(maps): stream place-photo bytes through storage --- server/src/nest/maps/maps.controller.ts | 34 ++++++---- server/src/nest/maps/maps.module.ts | 3 +- server/src/nest/maps/maps.service.ts | 4 +- .../place-photos/place-photo-cache.service.ts | 18 ++++- .../tests/unit/nest/maps.controller.test.ts | 66 ++++++++++++------- server/tests/unit/nest/maps.service.test.ts | 12 ++-- .../place-photo-cache.service.test.ts | 12 ++++ 7 files changed, 104 insertions(+), 45 deletions(-) diff --git a/server/src/nest/maps/maps.controller.ts b/server/src/nest/maps/maps.controller.ts index 88d7096a8..770f3c9c5 100644 --- a/server/src/nest/maps/maps.controller.ts +++ b/server/src/nest/maps/maps.controller.ts @@ -11,7 +11,7 @@ import { UseGuards, } from '@nestjs/common'; import type { Response } from 'express'; -import { createReadStream } from 'node:fs'; +import type { Readable } from 'node:stream'; import type { MapsAutocompleteResult, MapsPlaceDetailsResult, @@ -22,6 +22,7 @@ import type { } from '@trek/shared'; import type { User } from '../../types'; import { MapsService } from './maps.service'; +import { StorageService } from '../storage/storage.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; import { MapsSearchDto, MapsAutocompleteDto, MapsResolveUrlDto } from './maps.dto'; @@ -60,7 +61,10 @@ function toHttpException(err: unknown, fallbackMessage: string, defaultStatus: n @Controller('api/maps') @UseGuards(JwtAuthGuard) export class MapsController { - constructor(private readonly maps: MapsService) {} + constructor( + private readonly maps: MapsService, + private readonly storage: StorageService, + ) {} @Post('search') @HttpCode(200) // Express answers with res.json (200); Nest would otherwise default POST to 201. @@ -166,9 +170,9 @@ export class MapsController { } @Get('place-photo/:placeId/bytes') - placePhotoBytes(@Param('placeId') placeId: string, @Res() res: Response): void { - const fp = this.maps.photoBytesPath(placeId); - if (!fp) { + async placePhotoBytes(@Param('placeId') placeId: string, @Res() res: Response): Promise { + const key = this.maps.photoBytesKey(placeId); + if (!key) { // Same reasoning as the JSON endpoint above, and the bigger half of #1727: // places keep this URL in image_url, so a trip render asks for one photo per // place and every entry the cache sweep dropped answered 404 — a whole map @@ -177,15 +181,21 @@ export class MapsController { this.emptyPhoto(res); return; } - // Stream the cached file directly instead of res.sendFile(): the send library - // bundled under @nestjs/platform-express rejects absolute Windows paths (drive - // letter, no `root`) with a NotFoundError that surfaced as an unhandled 500, - // even though the file exists. A plain read stream serves the bytes - // cross-platform. Cached photos are always JPEG (placePhotoCache writes - // `.jpg`). + // Bytes are stream-piped (spec §Serving pins this route's stream contract); + // headers go on before the stream attempt, matching the old createReadStream + // ordering — an early failure overrides them via emptyPhoto exactly as the + // old error event did. Cached photos are always JPEG (placePhotoCache + // writes `.jpg`). res.set('Cache-Control', 'public, max-age=2592000, immutable'); res.type('image/jpeg'); - const stream = createReadStream(fp); + let stream: Readable; + try { + ({ stream } = await this.storage.getStream('photos-google', key)); + } catch { + // Cache-delete race — same terminal state as the old stream error path. + if (!res.headersSent) this.emptyPhoto(res); + return; + } stream.on('error', () => { if (!res.headersSent) this.emptyPhoto(res); }); diff --git a/server/src/nest/maps/maps.module.ts b/server/src/nest/maps/maps.module.ts index 8a26d0e3b..d0056199b 100644 --- a/server/src/nest/maps/maps.module.ts +++ b/server/src/nest/maps/maps.module.ts @@ -3,6 +3,7 @@ import { MapsController } from './maps.controller'; import { MapsService } from './maps.service'; import { MapsMcp } from './maps.mcp'; import { PlacePhotosModule } from '../place-photos/place-photos.module'; +import { StorageModule } from '../storage/storage.module'; /** * Maps / geo domain (L3 leaf module). Registered in AppModule. Exports @@ -11,7 +12,7 @@ import { PlacePhotosModule } from '../place-photos/place-photos.module'; * Nothing outside the container consumes this domain, so there is no bridge. */ @Module({ - imports: [PlacePhotosModule], + imports: [PlacePhotosModule, StorageModule], controllers: [MapsController], providers: [MapsService, MapsMcp], exports: [MapsService], diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 764071cf0..057a08160 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -529,8 +529,8 @@ export class MapsService { return this.getPlacePhoto(userId, placeId, lat, lng, name) as Promise; } - photoBytesPath(placeId: string): string | null { - return this.photoCache.serveFilePath(placeId); + photoBytesKey(placeId: string): string | null { + return this.photoCache.serveKey(placeId); } reverse(lat: string, lng: string, lang?: string): Promise { diff --git a/server/src/nest/place-photos/place-photo-cache.service.ts b/server/src/nest/place-photos/place-photo-cache.service.ts index 790154aac..de2139489 100644 --- a/server/src/nest/place-photos/place-photo-cache.service.ts +++ b/server/src/nest/place-photos/place-photo-cache.service.ts @@ -77,11 +77,15 @@ export class PlacePhotoCacheService implements OnModuleInit { } } - private filePath(placeId: string): string { + private fileName(placeId: string): string { // Hash to avoid filename collisions — coords:lat:lng pseudo-IDs contain characters that // collapse identically under sanitization (e.g. ':' and '.' both → '_') const hash = crypto.createHash('sha1').update(placeId).digest('hex'); - return path.join(this.photoDir, `${hash}.jpg`); + return `${hash}.jpg`; + } + + private filePath(placeId: string): string { + return path.join(this.photoDir, this.fileName(placeId)); } private proxyUrl(placeId: string): string { @@ -214,6 +218,16 @@ export class PlacePhotoCacheService implements OnModuleInit { return fp; } + /** + * Storage name (category 'photos-google') for a cached photo, or null when + * not on disk. The registry's mode-aware prefix reproduces photoDir in both + * TREK_PLACE_PHOTO_DIR modes; the cache's own disk internals move onto + * storage in slice 4. + */ + serveKey(placeId: string): string | null { + return this.serveFilePath(placeId) ? this.fileName(placeId) : null; + } + /** * A cache entry is "referenced" while any place still points at it — either by the * Google place_id (the dedup key) or by the stable proxy URL stored in image_url diff --git a/server/tests/unit/nest/maps.controller.test.ts b/server/tests/unit/nest/maps.controller.test.ts index a058db614..0de4f5996 100644 --- a/server/tests/unit/nest/maps.controller.test.ts +++ b/server/tests/unit/nest/maps.controller.test.ts @@ -2,17 +2,19 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { HttpException } from '@nestjs/common'; import type { Response } from 'express'; -const { createReadStream } = vi.hoisted(() => ({ createReadStream: vi.fn() })); -vi.mock('node:fs', () => ({ createReadStream })); - import { MapsController } from '../../../src/nest/maps/maps.controller'; import type { MapsService } from '../../../src/nest/maps/maps.service'; +import type { StorageService } from '../../../src/nest/storage/storage.service'; import type { User } from '../../../src/types'; const user = { id: 3 } as User; +// The bytes route is the only storage consumer; everything else ignores it. +const getStream = vi.fn(); +const storageStub = { getStream } as unknown as StorageService; + function makeController(svc: Partial) { - return new MapsController(svc as MapsService); + return new MapsController(svc as MapsService, storageStub); } /** Run an async handler, expecting an HttpException; return its { status, body }. */ @@ -243,38 +245,46 @@ describe('MapsController (parity with the legacy /api/maps route)', () => { return res as unknown as Response & { status: ReturnType; json: ReturnType; set: ReturnType; type: ReturnType; end: ReturnType }; } - beforeEach(() => createReadStream.mockReset()); + // Braced body on purpose: mockReset() returns the mock, and a function + // returned from beforeEach is invoked as a teardown callback — which would + // call getStream() bare and turn a mockRejectedValue impl into an + // unhandled rejection. + beforeEach(() => { + getStream.mockReset(); + }); + + function makeStream() { + const stream = { on: vi.fn().mockReturnThis(), pipe: vi.fn() }; + return stream; + } // Places persist this URL in image_url, so an evicted cache entry means one // request per place on a trip render. 404 for each of them is the ban vector // from #1727 — an uncached photo answers 204 with an empty body instead. - it('204 without a body when the photo is not cached', () => { + it('204 without a body when the photo is not cached', async () => { const res = makeRes(); - makeController({ photoBytesPath: () => null }).placePhotoBytes('p1', res); + await makeController({ photoBytesKey: () => null }).placePhotoBytes('p1', res); expect(res.status).toHaveBeenCalledWith(204); expect(res.end).toHaveBeenCalled(); expect(res.json).not.toHaveBeenCalled(); - expect(createReadStream).not.toHaveBeenCalled(); + expect(getStream).not.toHaveBeenCalled(); }); - it('streams the cached file with image/jpeg + an immutable cache header on a hit', () => { - const stream = { on: vi.fn().mockReturnThis(), pipe: vi.fn() }; - createReadStream.mockReturnValue(stream); + it('streams the cached bytes with image/jpeg + an immutable cache header on a hit', async () => { + const stream = makeStream(); + getStream.mockResolvedValue({ stream, stat: { key: 'photos/google/abc.jpg', size: 5, mtimeMs: 0 } }); const res = makeRes(); - makeController({ photoBytesPath: () => '/cache/p1.jpg' }).placePhotoBytes('p1', res); + await makeController({ photoBytesKey: () => 'abc.jpg' }).placePhotoBytes('p1', res); expect(res.set).toHaveBeenCalledWith('Cache-Control', 'public, max-age=2592000, immutable'); expect(res.type).toHaveBeenCalledWith('image/jpeg'); - expect(createReadStream).toHaveBeenCalledWith('/cache/p1.jpg'); + expect(getStream).toHaveBeenCalledWith('photos-google', 'abc.jpg'); expect(stream.pipe).toHaveBeenCalledWith(res); }); - it('falls back to an empty 204 when the read stream errors', () => { - let onError: () => void = () => {}; - const stream = { on: vi.fn((ev: string, cb: () => void) => { if (ev === 'error') onError = cb; return stream; }), pipe: vi.fn() }; - createReadStream.mockReturnValue(stream); + it('falls back to an empty 204 when the stream cannot be opened (cache-delete race)', async () => { + getStream.mockRejectedValue(new Error('storage object not found')); const res = makeRes(); - makeController({ photoBytesPath: () => '/cache/p1.jpg' }).placePhotoBytes('p1', res); - onError(); + await makeController({ photoBytesKey: () => 'abc.jpg' }).placePhotoBytes('p1', res); expect(res.status).toHaveBeenCalledWith(204); expect(res.end).toHaveBeenCalled(); // The hit path already asked for a month of immutable caching — that header @@ -283,13 +293,25 @@ describe('MapsController (parity with the legacy /api/maps route)', () => { expect(res.set).toHaveBeenLastCalledWith('Cache-Control', 'no-store'); }); - it('does not re-send a 204 when the stream errors after headers were flushed', () => { + it('falls back to an empty 204 when the stream errors before headers were flushed', async () => { let onError: () => void = () => {}; const stream = { on: vi.fn((ev: string, cb: () => void) => { if (ev === 'error') onError = cb; return stream; }), pipe: vi.fn() }; - createReadStream.mockReturnValue(stream); + getStream.mockResolvedValue({ stream, stat: { key: 'photos/google/abc.jpg', size: 5, mtimeMs: 0 } }); + const res = makeRes(); + await makeController({ photoBytesKey: () => 'abc.jpg' }).placePhotoBytes('p1', res); + onError(); + expect(res.status).toHaveBeenCalledWith(204); + expect(res.end).toHaveBeenCalled(); + expect(res.set).toHaveBeenLastCalledWith('Cache-Control', 'no-store'); + }); + + it('does not re-send a 204 when the stream errors after headers were flushed', async () => { + let onError: () => void = () => {}; + const stream = { on: vi.fn((ev: string, cb: () => void) => { if (ev === 'error') onError = cb; return stream; }), pipe: vi.fn() }; + getStream.mockResolvedValue({ stream, stat: { key: 'photos/google/abc.jpg', size: 5, mtimeMs: 0 } }); const res = makeRes(); (res as { headersSent: boolean }).headersSent = true; - makeController({ photoBytesPath: () => '/cache/p1.jpg' }).placePhotoBytes('p1', res); + await makeController({ photoBytesKey: () => 'abc.jpg' }).placePhotoBytes('p1', res); onError(); expect(res.status).not.toHaveBeenCalled(); expect(res.end).not.toHaveBeenCalled(); diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index 6920b189f..533a3cf61 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -112,7 +112,7 @@ const photoCacheStub = { markError: (placeId: string, kind?: string) => mockCacheMarkError(placeId, kind), getInFlight: (placeId: string) => mockCacheGetInFlight(placeId), setInFlight: (placeId: string, p: Promise) => mockCacheSetInFlight(placeId, p), - serveFilePath: (placeId: string) => mockServeFilePath(placeId), + serveKey: (placeId: string) => mockServeFilePath(placeId), } as unknown as PlacePhotoCacheService; import { db } from '../../../src/db/database'; @@ -2329,16 +2329,16 @@ describe('kill-switch settings reads', () => { }); }); -describe('photoBytesPath', () => { - it('returns the cached file path from placePhotoCache', () => { - mockServeFilePath.mockReturnValue('/cache/p1.jpg'); - expect(svc.photoBytesPath('p1')).toBe('/cache/p1.jpg'); +describe('photoBytesKey', () => { + it('returns the cached storage name from placePhotoCache', () => { + mockServeFilePath.mockReturnValue('abc.jpg'); + expect(svc.photoBytesKey('p1')).toBe('abc.jpg'); expect(mockServeFilePath).toHaveBeenCalledWith('p1'); }); it('returns null when nothing is cached', () => { mockServeFilePath.mockReturnValue(null); - expect(svc.photoBytesPath('p1')).toBeNull(); + expect(svc.photoBytesKey('p1')).toBeNull(); }); }); diff --git a/server/tests/unit/nest/place-photos/place-photo-cache.service.test.ts b/server/tests/unit/nest/place-photos/place-photo-cache.service.test.ts index 850a58b95..bc65ad5c9 100644 --- a/server/tests/unit/nest/place-photos/place-photo-cache.service.test.ts +++ b/server/tests/unit/nest/place-photos/place-photo-cache.service.test.ts @@ -109,6 +109,18 @@ describe('placePhotoCache.put() downscale guard', () => { }); }); +describe('placePhotoCache.serveKey()', () => { + it('returns the bare .jpg storage name when the photo is on disk', async () => { + fs.writeFileSync(filePathFor('p-key'), await makeJpeg(10, 10)); + const hash = crypto.createHash('sha1').update('p-key').digest('hex'); + expect(cache.serveKey('p-key')).toBe(`${hash}.jpg`); + }); + + it('returns null when nothing is cached', () => { + expect(cache.serveKey('p-missing')).toBeNull(); + }); +}); + describe('placePhotoCache.removeIfUnreferenced()', () => { it('PPC-004: removes a cache entry that no place references', async () => { await cache.put('orphan', await makeJpeg(50, 50), null);