From 1a2e4d4ef227c0991e1b411f1de8b8f3b24b65d2 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 17 Aug 2026 06:05:52 +0200 Subject: [PATCH] refactor(memories): trek photo cache internals through storage --- .../src/nest/memories/trek-photo-cache.job.ts | 16 ++-- .../nest/memories/trek-photo-cache.service.ts | 44 +++------ .../nest/trek-photo-cache.service.test.ts | 89 +++++++++++-------- 3 files changed, 75 insertions(+), 74 deletions(-) diff --git a/server/src/nest/memories/trek-photo-cache.job.ts b/server/src/nest/memories/trek-photo-cache.job.ts index d452fed24..512ee7144 100644 --- a/server/src/nest/memories/trek-photo-cache.job.ts +++ b/server/src/nest/memories/trek-photo-cache.job.ts @@ -19,15 +19,19 @@ export class TrekPhotoCacheJob implements OnApplicationBootstrap { onApplicationBootstrap(): void { if (!this.registrar.isEnabled()) return; // Run once immediately on startup to evict any entries left over from a previous run - try { - this.cache.sweepExpired(); - } catch { /* cache dir may not exist yet — harmless */ } - this.registrar.register('trek-photo-cache', '0 */2 * * *', () => this.tick(), { timezone: 'none' }); + void (async () => { + try { + await this.cache.sweepExpired(); + } catch { /* cache dir may not exist yet — harmless */ } + })(); + this.registrar.register('trek-photo-cache', '0 */2 * * *', () => { + void this.tick(); + }, { timezone: 'none' }); } - tick(): void { + async tick(): Promise { try { - this.cache.sweepExpired(); + await this.cache.sweepExpired(); } catch (err: unknown) { logError(`Trek photo cache cleanup: ${err instanceof Error ? err.message : err}`); } diff --git a/server/src/nest/memories/trek-photo-cache.service.ts b/server/src/nest/memories/trek-photo-cache.service.ts index 57fbc3312..535982274 100644 --- a/server/src/nest/memories/trek-photo-cache.service.ts +++ b/server/src/nest/memories/trek-photo-cache.service.ts @@ -1,15 +1,11 @@ -import path from 'node:path'; -import fs from 'node:fs'; -import fsPromises from 'node:fs/promises'; import crypto from 'node:crypto'; +import { Readable } from 'node:stream'; import { Injectable } from '@nestjs/common'; import { Response } from 'express'; import { DatabaseService } from '../database/database.service'; import { StorageService } from '../storage/storage.service'; import { StorageNotFoundError } from '../storage/storage.types'; -import { UPLOADS_ROOT } from './uploads-root'; -const TREK_PHOTO_DIR = path.join(UPLOADS_ROOT, 'photos/trek'); export const CACHE_TTL = 60 * 60 * 1000; // 1 hour /** @@ -25,17 +21,12 @@ export const CACHE_TTL = 60 * 60 * 1000; // 1 hour */ const inFlight = new Map>(); -function ensureDir(): void { - if (!fs.existsSync(TREK_PHOTO_DIR)) { - fs.mkdirSync(TREK_PHOTO_DIR, { recursive: true }); - } +/** Storage name (category 'photos-trek') for a cache key. */ +function objectName(key: string): string { + return `${key}.bin`; } -function cachedFilePath(key: string): string { - return path.join(TREK_PHOTO_DIR, `${key}.bin`); -} - -/** Disk + metadata cache for provider thumbnails and originals. */ +/** Storage + metadata cache for provider thumbnails and originals. */ @Injectable() export class TrekPhotoCacheService { constructor( @@ -47,9 +38,7 @@ export class TrekPhotoCacheService { return crypto.createHash('sha1').update(`${provider}:${assetId}:${kind}:${ownerId}`).digest('hex'); } - - - getFresh(key: string): { filePath: string; contentType: string } | null { + async getFresh(key: string): Promise<{ contentType: string } | null> { const row = this.db.get<{ content_type: string; fetched_at: number }>( 'SELECT content_type, fetched_at FROM trek_photo_cache_meta WHERE cache_key = ?', key, ); @@ -61,22 +50,16 @@ export class TrekPhotoCacheService { return null; } - const fp = cachedFilePath(key); - if (!fs.existsSync(fp)) { + if (!(await this.storage.exists('photos-trek', objectName(key)))) { this.db.run('DELETE FROM trek_photo_cache_meta WHERE cache_key = ?', key); return null; } - return { filePath: fp, contentType: row.content_type }; + return { contentType: row.content_type }; } async put(key: string, bytes: Buffer, contentType: string): Promise { - ensureDir(); - const fp = cachedFilePath(key); - const tmp = fp + '.tmp'; - - await fsPromises.writeFile(tmp, bytes); - await fsPromises.rename(tmp, fp); + await this.storage.put('photos-trek', objectName(key), Readable.from(bytes)); this.db.run( 'INSERT OR REPLACE INTO trek_photo_cache_meta (cache_key, content_type, fetched_at) VALUES (?, ?, ?)', @@ -85,7 +68,7 @@ export class TrekPhotoCacheService { } async serveFresh(res: Response, key: string): Promise { - const entry = this.getFresh(key); + const entry = await this.getFresh(key); if (!entry) return false; res.set('Content-Type', entry.contentType); @@ -93,7 +76,7 @@ export class TrekPhotoCacheService { try { // send() keeps a pre-set Content-Type, so entry.contentType survives the // .bin extension. - await this.storage.sendToResponse('photos-trek', `${key}.bin`, res); + await this.storage.sendToResponse('photos-trek', objectName(key), res); } catch (err) { // getFresh→send delete race: fall back like a cache miss. if (err instanceof StorageNotFoundError && !res.headersSent) return false; @@ -111,7 +94,7 @@ export class TrekPhotoCacheService { promise.finally(() => inFlight.delete(key)); } - sweepExpired(): void { + async sweepExpired(): Promise { const cutoff = Date.now() - CACHE_TTL * 2; const stale = this.db.all<{ cache_key: string }>( 'SELECT cache_key FROM trek_photo_cache_meta WHERE fetched_at < ?', cutoff, @@ -119,8 +102,7 @@ export class TrekPhotoCacheService { for (const row of stale) { this.db.run('DELETE FROM trek_photo_cache_meta WHERE cache_key = ?', row.cache_key); - const fp = cachedFilePath(row.cache_key); - if (fs.existsSync(fp)) fs.unlinkSync(fp); + await this.storage.delete('photos-trek', objectName(row.cache_key)); } } } diff --git a/server/tests/unit/nest/trek-photo-cache.service.test.ts b/server/tests/unit/nest/trek-photo-cache.service.test.ts index 89a7aa658..32ea48aa4 100644 --- a/server/tests/unit/nest/trek-photo-cache.service.test.ts +++ b/server/tests/unit/nest/trek-photo-cache.service.test.ts @@ -1,11 +1,15 @@ /** - * TrekPhotoCacheService — the disk + metadata cache for provider assets. + * TrekPhotoCacheService — the storage + metadata cache for provider assets. * * It had no suite of its own: it lived in services/memories/, which the * coverage gate does not measure, so a cache that silently stopped evicting or * stopped deduping concurrent fetches would not have failed anything. The * stampede guard in particular is the reason the in-flight map is module-scoped * rather than an instance field. + * + * Slice 4: the cache addresses storage as ('photos-trek', '.bin') over a + * stub-registry fixture rooted in a throwaway dir — the real uploads tree is + * never touched. */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; @@ -26,22 +30,18 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { TrekPhotoCacheService, CACHE_TTL } from '../../../src/nest/memories/trek-photo-cache.service'; -import { UPLOADS_ROOT } from '../../../src/nest/memories/uploads-root'; import { StorageNotFoundError } from '../../../src/nest/storage/storage.types'; -import type { StorageService } from '../../../src/nest/storage/storage.service'; +import { makeStorageFixture } from '../../helpers/storage-fixture'; -// serveFresh is the one byte-serving method; the cache's disk internals -// (getFresh/put/sweep) stay raw-fs until slice 4. -const storage = { sendToResponse: vi.fn().mockResolvedValue(undefined) }; -const svc = new TrekPhotoCacheService(new DatabaseService(testDb), storage as unknown as StorageService); -const CACHE_DIR = path.join(UPLOADS_ROOT, 'photos/trek'); -const written: string[] = []; +const fx = makeStorageFixture('photos/trek/'); +const svc = new TrekPhotoCacheService(new DatabaseService(testDb), fx.storage); +const CACHE_DIR = path.join(fx.root, 'photos/trek'); +const binPath = (key: string) => path.join(CACHE_DIR, `${key}.bin`); +let counter = 0; -/** Unique per case so parallel files cannot collide on the shared cache dir. */ +/** Unique per case so cases cannot collide on the shared fixture dir. */ function freshKey(label: string): string { - const key = svc.cacheKey('test', `${label}-${process.pid}-${written.length}`, 'thumbnail', 1); - written.push(key); - return key; + return svc.cacheKey('test', `${label}-${counter++}`, 'thumbnail', 1); } beforeAll(() => { @@ -54,10 +54,8 @@ beforeEach(() => { }); afterAll(() => { - for (const key of written) { - try { fs.unlinkSync(path.join(CACHE_DIR, `${key}.bin`)); } catch { /* never written */ } - } testDb.close(); + fx.cleanup(); }); describe('cacheKey', () => { @@ -71,18 +69,18 @@ describe('cacheKey', () => { }); describe('put / getFresh', () => { - it('CACHE-002: a stored entry comes back with its content type and a real file', async () => { + it('CACHE-002: a stored entry comes back with its content type and a real object', async () => { const key = freshKey('hit'); await svc.put(key, Buffer.from('jpegbytes'), 'image/jpeg'); - const entry = svc.getFresh(key); + const entry = await svc.getFresh(key); expect(entry).not.toBeNull(); expect(entry!.contentType).toBe('image/jpeg'); - expect(fs.readFileSync(entry!.filePath).toString()).toBe('jpegbytes'); + expect(fs.readFileSync(binPath(key)).toString()).toBe('jpegbytes'); }); - it('CACHE-003: an unknown key is a miss, not an error', () => { - expect(svc.getFresh('no-such-key')).toBeNull(); + it('CACHE-003: an unknown key is a miss, not an error', async () => { + expect(await svc.getFresh('no-such-key')).toBeNull(); }); it('CACHE-004: an entry past its TTL is a miss and its metadata row is dropped', async () => { @@ -91,16 +89,16 @@ describe('put / getFresh', () => { testDb.prepare('UPDATE trek_photo_cache_meta SET fetched_at = ? WHERE cache_key = ?') .run(Date.now() - CACHE_TTL - 1000, key); - expect(svc.getFresh(key)).toBeNull(); + expect(await svc.getFresh(key)).toBeNull(); expect(testDb.prepare('SELECT 1 FROM trek_photo_cache_meta WHERE cache_key = ?').get(key)).toBeUndefined(); }); - it('CACHE-005: metadata without its file is a miss and the row is dropped', () => { + it('CACHE-005: metadata without its object is a miss and the row is dropped', async () => { const key = 'orphan-meta-row'; testDb.prepare('INSERT INTO trek_photo_cache_meta (cache_key, content_type, fetched_at) VALUES (?, ?, ?)') .run(key, 'image/jpeg', Date.now()); - expect(svc.getFresh(key)).toBeNull(); + expect(await svc.getFresh(key)).toBeNull(); expect(testDb.prepare('SELECT 1 FROM trek_photo_cache_meta WHERE cache_key = ?').get(key)).toBeUndefined(); }); @@ -111,8 +109,8 @@ describe('put / getFresh', () => { const rows = testDb.prepare('SELECT content_type FROM trek_photo_cache_meta WHERE cache_key = ?').all(key); expect(rows).toHaveLength(1); - expect(svc.getFresh(key)!.contentType).toBe('image/png'); - expect(fs.readFileSync(svc.getFresh(key)!.filePath).toString()).toBe('second'); + expect((await svc.getFresh(key))!.contentType).toBe('image/png'); + expect(fs.readFileSync(binPath(key)).toString()).toBe('second'); }); }); @@ -120,29 +118,34 @@ describe('serveFresh', () => { it('CACHE-007: sends the cached bytes through storage with a one-hour cache header', async () => { const key = freshKey('serve'); await svc.put(key, Buffer.from('bytes'), 'image/webp'); + const send = vi.spyOn(fx.storage, 'sendToResponse').mockResolvedValueOnce(undefined); const res = { set: vi.fn() }; expect(await svc.serveFresh(res as never, key)).toBe(true); expect(res.set).toHaveBeenCalledWith('Content-Type', 'image/webp'); expect(res.set).toHaveBeenCalledWith('Cache-Control', 'public, max-age=3600'); - expect(storage.sendToResponse).toHaveBeenCalledWith('photos-trek', `${key}.bin`, res); + expect(send).toHaveBeenCalledWith('photos-trek', `${key}.bin`, res); + send.mockRestore(); }); it('CACHE-008: reports a miss without touching the response or storage', async () => { - storage.sendToResponse.mockClear(); + const send = vi.spyOn(fx.storage, 'sendToResponse'); const res = { set: vi.fn() }; expect(await svc.serveFresh(res as never, 'nothing-cached')).toBe(false); expect(res.set).not.toHaveBeenCalled(); - expect(storage.sendToResponse).not.toHaveBeenCalled(); + expect(send).not.toHaveBeenCalled(); + send.mockRestore(); }); it('CACHE-013: a getFresh→send delete race reads as a miss, not a crash', async () => { const key = freshKey('race'); await svc.put(key, Buffer.from('bytes'), 'image/webp'); - storage.sendToResponse.mockRejectedValueOnce(new StorageNotFoundError(`photos/trek/${key}.bin`)); + const send = vi.spyOn(fx.storage, 'sendToResponse') + .mockRejectedValueOnce(new StorageNotFoundError(`photos/trek/${key}.bin`)); const res = { set: vi.fn(), headersSent: false }; expect(await svc.serveFresh(res as never, key)).toBe(false); + send.mockRestore(); }); }); @@ -166,7 +169,7 @@ describe('the stampede guard', () => { // Load-bearing: a per-instance map would hand any second instance a private // guard and the dedup would be silently gone. The sweep cron injects the // container singleton now, but the invariant stays pinned. - const other = new TrekPhotoCacheService(new DatabaseService(testDb), storage as unknown as StorageService); + const other = new TrekPhotoCacheService(new DatabaseService(testDb), fx.storage); const promise = Promise.resolve(null); svc.setInFlight('shared-key', promise); expect(other.getInFlight('shared-key')).toBe(promise); @@ -174,28 +177,40 @@ describe('the stampede guard', () => { }); describe('sweepExpired', () => { - it('CACHE-011: drops rows and files past twice the TTL and leaves fresh ones alone', async () => { + it('CACHE-011: drops rows and objects past twice the TTL and leaves fresh ones alone', async () => { const staleKey = freshKey('sweep-stale'); const freshKeyId = freshKey('sweep-fresh'); await svc.put(staleKey, Buffer.from('old'), 'image/jpeg'); await svc.put(freshKeyId, Buffer.from('new'), 'image/jpeg'); testDb.prepare('UPDATE trek_photo_cache_meta SET fetched_at = ? WHERE cache_key = ?') .run(Date.now() - CACHE_TTL * 2 - 1000, staleKey); - const staleFile = path.join(CACHE_DIR, `${staleKey}.bin`); - svc.sweepExpired(); + await svc.sweepExpired(); expect(testDb.prepare('SELECT 1 FROM trek_photo_cache_meta WHERE cache_key = ?').get(staleKey)).toBeUndefined(); - expect(fs.existsSync(staleFile)).toBe(false); + expect(fs.existsSync(binPath(staleKey))).toBe(false); expect(testDb.prepare('SELECT 1 FROM trek_photo_cache_meta WHERE cache_key = ?').get(freshKeyId)).toBeDefined(); }); - it('CACHE-012: survives a metadata row whose file is already gone', () => { + it('CACHE-012: survives a metadata row whose object is already gone', async () => { const key = 'sweep-orphan'; testDb.prepare('INSERT INTO trek_photo_cache_meta (cache_key, content_type, fetched_at) VALUES (?, ?, ?)') .run(key, 'image/jpeg', Date.now() - CACHE_TTL * 3); - expect(() => svc.sweepExpired()).not.toThrow(); + await expect(svc.sweepExpired()).resolves.toBeUndefined(); expect(testDb.prepare('SELECT 1 FROM trek_photo_cache_meta WHERE cache_key = ?').get(key)).toBeUndefined(); }); + + it('CACHE-014: a row-less .bin object survives the DB-driven sweep (parity pin — fix #4 reclaims it)', async () => { + const key = freshKey('rowless'); + await svc.put(key, Buffer.from('leaked'), 'image/jpeg'); + testDb.prepare('DELETE FROM trek_photo_cache_meta WHERE cache_key = ?').run(key); + // Old enough that the list-driven fix-#4 sweep WILL reclaim it — today's + // DB-driven sweep never visits it (the getFresh-expiry leak, spec rev 3.2). + const old = (Date.now() - CACHE_TTL * 3) / 1000; + fs.utimesSync(binPath(key), old, old); + + await svc.sweepExpired(); + expect(fs.existsSync(binPath(key))).toBe(true); + }); });