diff --git a/server/src/db/repositories/AppSettings.repository.ts b/server/src/db/repositories/AppSettings.repository.ts index a5f8d68ae..025efd893 100644 --- a/server/src/db/repositories/AppSettings.repository.ts +++ b/server/src/db/repositories/AppSettings.repository.ts @@ -89,4 +89,36 @@ export class AppSettingsRepository extends TrekRepository { async deleteValue(key: string): Promise { return this.nativeDelete({ key }); } + + /** + * `SELECT COUNT(*) AS n FROM app_settings WHERE key IN (?, ?, ...)` (Plan + * 3i, storage's SR1 — `StorageRegistryService#seedFromFileOnce`'s + * seed-once boot guard). Counts every ROW whose key matches, including one + * whose `value` is NULL — unlike `getValues()`, which drops a NULL-valued + * row from its Map, this mirrors the legacy statement exactly: no WHERE on + * `value`, so a present-but-NULL row still counts as "already seeded." + */ + async countKeysPresent(keys: string[]): Promise { + return this.count({ key: { $in: keys } }); + } + + /** + * `INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)` (Plan + * 3i, storage's SS1 — `StorageStatsService#scan`). The SAME "one row per + * key" write as `setValue()`'s `ON CONFLICT(key) DO UPDATE` upsert, but a + * genuinely DIFFERENT legacy SQL text (R4/R5's ruling: `app_settings` has + * two distinct upsert dialects across this program's converted call + * sites — preserve both as separately-named methods, never merged into one + * with a boolean/dialect flag, even where — as here — their observable + * behavior on this table is identical). Per `setValue()`'s own docstring, + * the only real difference between the two dialects (REPLACE deletes and + * reinserts, moving the SQLite rowid; `ON CONFLICT ... DO UPDATE` keeps the + * original row) is unobservable on `app_settings` (no `INTEGER PRIMARY + * KEY`/`AUTOINCREMENT`/trigger/FK references it) — this method exists so + * the repository's public surface keeps naming the legacy text 1:1, not + * because the two need different runtime behavior on this table. + */ + async upsertOrReplace(key: string, value: string): Promise { + await this.upsert({ key, value }, { onConflictFields: ['key'], onConflictAction: 'merge' }); + } } diff --git a/server/src/nest/storage/storage-admin.service.ts b/server/src/nest/storage/storage-admin.service.ts index 9365939b3..91635ecd7 100644 --- a/server/src/nest/storage/storage-admin.service.ts +++ b/server/src/nest/storage/storage-admin.service.ts @@ -1,8 +1,10 @@ import fs from 'node:fs'; import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import type { StorageAdminState, StorageBackend, StorageConfigPut, StorageTestResponse, StorageUsage } from '@trek/shared'; -import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; import { BACKENDS_KEY, CATEGORIES_KEY, @@ -10,7 +12,7 @@ import { StorageRegistryService, } from './storage-registry.service'; import { StorageService } from './storage.service'; -import { SEED_CONFIG_PATH } from './storage-paths'; +import { getSeedConfigPath } from './storage-paths'; import { assertNoMaskSentinels, decryptBackendSecrets, @@ -33,7 +35,7 @@ import { StorageStatsService } from './storage-stats.service'; @Injectable() export class StorageAdminService { constructor( - private readonly db: DatabaseService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, private readonly registry: StorageRegistryService, private readonly storage: StorageService, private readonly jobs: StorageJobsService, @@ -57,7 +59,7 @@ export class StorageAdminService { })), categories: snapshot.categories, health: { replicaFailures: this.storage.health().replicaFailures.map((f) => ({ ...f })) }, - seedFilePresent: fs.existsSync(SEED_CONFIG_PATH), + seedFilePresent: fs.existsSync(getSeedConfigPath()), usage: await this.stats.readUsage(), backfills: this.jobs.statuses(), migrations: this.jobs.migrationStatuses(), @@ -119,12 +121,9 @@ export class StorageAdminService { this.registry.preview({ backends: unmasked.backends, categories: unmasked.categories }); const encrypted = encryptStorageSecrets(unmasked); await this.uow.transactional(async () => { - const upsert = this.db.prepare( - 'INSERT INTO app_settings (key, value) VALUES (?, ?) ON CONFLICT(key) DO UPDATE SET value = excluded.value', - ); - upsert.run(BACKENDS_KEY, JSON.stringify(encrypted.backends)); - upsert.run(CATEGORIES_KEY, JSON.stringify(encrypted.categories)); - upsert.run(VERSION_KEY, String(currentVersion + 1)); + await this.appSettings.setValue(BACKENDS_KEY, JSON.stringify(encrypted.backends)); + await this.appSettings.setValue(CATEGORIES_KEY, JSON.stringify(encrypted.categories)); + await this.appSettings.setValue(VERSION_KEY, String(currentVersion + 1)); }); await this.registry.reload(); // Any running job whose backend the reloaded config no longer has ends @@ -183,10 +182,10 @@ export class StorageAdminService { /** The raw stored backends row — the unmask source (tolerates absent/garbage rows). */ private async storedBackendsRow(): Promise { - const row = this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', BACKENDS_KEY); - if (!row?.value) return []; + const value = await this.appSettings.getValue(BACKENDS_KEY); + if (!value) return []; try { - return JSON.parse(row.value) as unknown; + return JSON.parse(value) as unknown; } catch { return []; } diff --git a/server/src/nest/storage/storage-paths.ts b/server/src/nest/storage/storage-paths.ts index e5e06c596..df490e342 100644 --- a/server/src/nest/storage/storage-paths.ts +++ b/server/src/nest/storage/storage-paths.ts @@ -19,4 +19,39 @@ export const DEFAULT_BACKUPS_ROOT = path.join(DATA_ROOT, 'backups'); /** Driver-agnostic global scratch space (`data/tmp`) — see StorageService.tempDir(). */ export const GLOBAL_TEMP_DIR = path.join(DATA_ROOT, 'tmp'); /** Seed-once boot provisioning file — imported only when no storage.* row exists. */ -export const SEED_CONFIG_PATH = path.join(DATA_ROOT, 'storage-config.json'); +const DEFAULT_SEED_CONFIG_PATH = path.join(DATA_ROOT, 'storage-config.json'); + +/** + * Test-only override for the seed-config path (Plan 3i, R5 defect 1 — the + * `storage-config.json` test-harness race, 3c's deferred L13). Production + * never calls the setter, so `getSeedConfigPath()` always resolves to the + * real source-tree default there. + * + * Why a seam here rather than an env var read through `src/app-config`: this + * repo's env-config layer (`src/app-config/env.schema.ts` + the + * `no-restricted-syntax` ESLint ban on raw `process.env` outside its + * documented exemption list) would need a new schema entry and an ESLint + * exemption-list edit for a value that is NEVER read from a real environment + * variable in production — it only exists to redirect one test suite away + * from the real file. A DI/constructor parameter on `StorageRegistryService` + * was the brief's other named option; it was rejected too, because + * `storage-admin.service.ts#state()` also reads this path (for + * `seedFilePresent`) and is a SIBLING service, not a consumer of + * `StorageRegistryService` — giving the registry alone a constructor + * override would still leave the admin service reading the real path, + * reintroducing the same race for that one read. A plain module-level seam + * in this file (the one place both services already import the path from) + * covers both call sites with the smallest, most local change, and needs no + * new provider/token wiring in `storage.module.ts`. + */ +let seedConfigPathOverride: string | null = null; + +/** Test-only — see `seedConfigPathOverride`'s doc comment. Pass `null` to restore the production default. */ +export function setSeedConfigPathForTests(overridePath: string | null): void { + seedConfigPathOverride = overridePath; +} + +/** The effective seed-config path: the test override when one is set, else the real source-tree default. */ +export function getSeedConfigPath(): string { + return seedConfigPathOverride ?? DEFAULT_SEED_CONFIG_PATH; +} diff --git a/server/src/nest/storage/storage-registry.service.ts b/server/src/nest/storage/storage-registry.service.ts index de5ef0529..fbcad0918 100644 --- a/server/src/nest/storage/storage-registry.service.ts +++ b/server/src/nest/storage/storage-registry.service.ts @@ -1,15 +1,19 @@ import fs from 'node:fs'; import { Injectable, Logger, type OnModuleInit } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; +import { MikroORM } from '@mikro-orm/core'; import { STORAGE_BACKEND_TYPES, storageConfigSchema } from '@trek/shared'; -import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; +import { withRequestContext } from '../database/request-context'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; import { decrypt_api_key } from '../common/crypto/apiKeyCrypto'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; import { LocalDriver } from './drivers/local.driver'; import { MirrorDriver, type ReplicaFailure } from './drivers/mirror.driver'; import { S3Driver } from './drivers/s3.driver'; import { StorageEventsService } from './storage-events.service'; -import { DEFAULT_BACKUPS_ROOT, DEFAULT_UPLOADS_ROOT, GLOBAL_TEMP_DIR, SEED_CONFIG_PATH } from './storage-paths'; +import { DEFAULT_BACKUPS_ROOT, DEFAULT_UPLOADS_ROOT, GLOBAL_TEMP_DIR, getSeedConfigPath } from './storage-paths'; import { assertNoMaskSentinels, encryptStorageSecrets } from './storage-secrets'; import { SERVED_CATEGORIES, @@ -131,15 +135,30 @@ export class StorageRegistryService implements OnModuleInit { private loadFailure: string | null = null; constructor( - private readonly db: DatabaseService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, private readonly env: RuntimeEnvService, private readonly events: StorageEventsService, private readonly uow: UnitOfWork, + private readonly orm: MikroORM, ) {} + /** + * `onModuleInit` is a DIFFERENT boot hook than the `CronRegistrarService`/ + * `runOnBoot` machinery every job file in this plan otherwise relies on + * (Plan 3i's own BOOT GATE note): it fires at Nest's MODULE-INIT lifecycle + * stage, before `app.init()` wires up `@mikro-orm/nestjs`'s per-request + * middleware — a repository call here has no per-request `EntityManager` + * fork to resolve through and would hit MikroORM's `cannotUseGlobalContext` + * refusal (confirmed against a real `createNestApplication()` boot, not + * only the test ORMs' `allowGlobalContext: true` default, which silently + * hides this). This opens its own request context, the same shape + * `StorageHealthNotifierService`'s `onApplicationBootstrap` hook uses. + */ async onModuleInit(): Promise { - await this.seedFromFileOnce(); - await this.load(true); + await withRequestContext(this.orm, async () => { + await this.seedFromFileOnce(); + await this.load(true); + }); } /** Re-read settings, validate, atomically swap. In-flight ops keep their resolved instances. */ @@ -203,7 +222,7 @@ export class StorageRegistryService implements OnModuleInit { /** Current optimistic-concurrency counter — 0 when never bumped (fresh install). */ async currentConfigVersion(): Promise { - return await readConfigVersion(this.db); + return await readConfigVersion(this.appSettings); } /** Non-null when the last load() fell back (last-good config or built-in defaults) — null once a load succeeds. */ @@ -229,28 +248,30 @@ export class StorageRegistryService implements OnModuleInit { if (!names.has(backend)) { throw new StorageBackendError(`cannot assign '${category}' to unknown backend '${backend}'`); } - // The OTHER writer of the two settings rows, so it enforces the same - // shared-replica rule validateConfig does (see assertNoSharedReplicas): - // a mirror's sync sweep DELETES replica objects its primary doesn't hold, - // so routing a category onto a backend that is somebody's replica would - // hand this category's objects to that sweep. - const target = defined.find((b) => b.name === backend); - const owner = target?.type === 'mirror' ? String(target.options.primary) : backend; - const holder = defined.find((b) => b.type === 'mirror' && replicaNamesOf(b.options).includes(owner)); - if (holder) { - throw new StorageBackendError( - `cannot assign '${category}' to '${backend}' — '${owner}' is a mirror replica of '${holder.name}', and that mirror's sync sweep would delete the category's objects`, - ); - } - const stored = new Map(parseCategoryMap((await this.readSettings()).categories)); + // R5 defect 2 fix (2026-09-25 ORM Plan 3i, storage task): this used to + // run its OWN, narrower inline check here — whether the flip's direct + // target (or, for a mirror target, that mirror's primary) is itself a + // replica of some OTHER mirror. That covers only the first of + // assertNoSharedReplicas's two documented refusal cases and misses the + // second (a backend replicating two mirrors whose swept key prefixes + // overlap) — a flip could persist a config that the very next + // build()/reload() would then refuse (or silently mis-sweep). Calling + // the SAME predicate validateConfig() runs on every load — over the + // merged backend/category maps with THIS flip applied — catches both + // cases before the write, not merely on the next reload. Genuine + // behavior change, not parity — see the task report's "For the user" + // note. + const settings = await this.readSettings(); + const { backends: mergedBackends, categoryBackends: mergedCategories } = this.mergeBackendsAndCategories(settings); + mergedCategories.set(category, backend); + assertNoSharedReplicas(mergedBackends, mergedCategories); + + const stored = new Map(parseCategoryMap(settings.categories)); stored.set(category, backend); const next = Object.fromEntries(stored); await this.uow.transactional(async () => { - const upsert = this.db.prepare( - 'INSERT INTO app_settings (key, value) VALUES (?, ?) ON CONFLICT(key) DO UPDATE SET value = excluded.value', - ); - upsert.run(CATEGORIES_KEY, JSON.stringify(next)); - upsert.run(VERSION_KEY, String((await readConfigVersion(this.db)) + 1)); + await this.appSettings.setValue(CATEGORIES_KEY, JSON.stringify(next)); + await this.appSettings.setValue(VERSION_KEY, String((await readConfigVersion(this.appSettings)) + 1)); }); await this.reload(); } @@ -278,26 +299,23 @@ export class StorageRegistryService implements OnModuleInit { * 'storage.%', restart (documented in the README with slice 3). */ private async seedFromFileOnce(): Promise { - const rowCount = this.db.get<{ n: number }>( - 'SELECT COUNT(*) AS n FROM app_settings WHERE key IN (?, ?)', - BACKENDS_KEY, - CATEGORIES_KEY, - ); - const filePresent = fs.existsSync(SEED_CONFIG_PATH); - if ((rowCount?.n ?? 0) > 0) { + const seedPath = getSeedConfigPath(); + const rowCount = await this.appSettings.countKeysPresent([BACKENDS_KEY, CATEGORIES_KEY]); + const filePresent = fs.existsSync(seedPath); + if (rowCount > 0) { if (filePresent) { - this.logger.log(`storage config rows exist — ignoring ${SEED_CONFIG_PATH}; manage storage in the admin UI`); + this.logger.log(`storage config rows exist — ignoring ${seedPath}; manage storage in the admin UI`); } return; } if (!filePresent) return; const fail = (detail: string): never => { - throw new Error(`invalid storage seed file ${SEED_CONFIG_PATH}: ${detail}`); + throw new Error(`invalid storage seed file ${seedPath}: ${detail}`); }; let json: unknown; try { - json = JSON.parse(fs.readFileSync(SEED_CONFIG_PATH, 'utf8')); + json = JSON.parse(fs.readFileSync(seedPath, 'utf8')); } catch (err) { return fail(err instanceof Error ? err.message : String(err)); } @@ -316,14 +334,11 @@ export class StorageRegistryService implements OnModuleInit { } const encrypted = encryptStorageSecrets(config); await this.uow.transactional(async () => { - const upsert = this.db.prepare( - 'INSERT INTO app_settings (key, value) VALUES (?, ?) ON CONFLICT(key) DO UPDATE SET value = excluded.value', - ); - upsert.run(BACKENDS_KEY, JSON.stringify(encrypted.backends)); - upsert.run(CATEGORIES_KEY, JSON.stringify(encrypted.categories)); + await this.appSettings.setValue(BACKENDS_KEY, JSON.stringify(encrypted.backends)); + await this.appSettings.setValue(CATEGORIES_KEY, JSON.stringify(encrypted.categories)); }); this.logger.log( - `storage config seeded from ${SEED_CONFIG_PATH} — the file is now ignored; manage storage in the admin UI`, + `storage config seeded from ${seedPath} — the file is now ignored; manage storage in the admin UI`, ); } @@ -348,11 +363,11 @@ export class StorageRegistryService implements OnModuleInit { } private async readSettings(): Promise<{ backends: unknown; categories: unknown }> { - const read = (key: string): unknown => { - const row = this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', key); - if (!row?.value) return undefined; + const read = async (key: string): Promise => { + const value = await this.appSettings.getValue(key); + if (!value) return undefined; try { - return JSON.parse(row.value) as unknown; + return JSON.parse(value) as unknown; } catch (err) { // JSON.parse's own SyntaxError can echo a snippet of the raw input // around the failure position (Node 24 V8, e.g. `Unexpected token @@ -368,10 +383,23 @@ export class StorageRegistryService implements OnModuleInit { ); } }; - return { backends: read(BACKENDS_KEY), categories: read(CATEGORIES_KEY) }; + return { backends: await read(BACKENDS_KEY), categories: await read(CATEGORIES_KEY) }; } - private build(settings: { backends: unknown; categories: unknown }, boot: boolean): RegistryState { + /** + * The merged backend/category maps build() computes as its own steps 1–2 + * (env defaults + built-ins + settings overrides), without the driver + * construction/validation side effects that follow — the shared basis + * both build() and assignCategory()'s pre-write assertNoSharedReplicas + * check need (R5 defect 2), so the two never compute two different + * answers for "what does this instance's config resolve to." + */ + private mergeBackendsAndCategories(settings: { backends: unknown; categories: unknown }): { + backends: Map; + backendSources: Map; + categoryBackends: Map; + categorySources: Map; + } { // 1. Env is read fresh on every load (never snapshotted — RuntimeEnvService rule). const placePhotoDir = this.env.env().paths.placePhotoDir; @@ -403,6 +431,11 @@ export class StorageRegistryService implements OnModuleInit { categoryBackends.set(category, backendName); categorySources.set(category, 'settings'); } + return { backends, backendSources, categoryBackends, categorySources }; + } + + private build(settings: { backends: unknown; categories: unknown }, boot: boolean): RegistryState { + const { backends, backendSources, categoryBackends, categorySources } = this.mergeBackendsAndCategories(settings); // 3. Validate the merged config as a whole. validateConfig(backends, categoryBackends); @@ -482,9 +515,9 @@ function isRecord(value: unknown): value is Record { } /** Raw read of the version counter row — 0 for an absent/garbage row (fresh install, or a hand-edited DB). */ -async function readConfigVersion(db: DatabaseService): Promise { - const row = db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', VERSION_KEY); - const parsed = row?.value ? Number.parseInt(row.value, 10) : 0; +async function readConfigVersion(appSettings: AppSettingsRepository): Promise { + const value = await appSettings.getValue(VERSION_KEY); + const parsed = value ? Number.parseInt(value, 10) : 0; return Number.isFinite(parsed) ? parsed : 0; } @@ -560,11 +593,6 @@ function prefixFor(category: ServedCategory, backendName: string): string { return category === 'photos-google' && backendName === 'place-photos-local' ? '' : CATEGORY_PREFIXES[category]; } -/** Replica names off a snapshot's loose options record (mirrors always carry a string[] there). */ -function replicaNamesOf(options: Record): string[] { - return Array.isArray(options.replicas) ? options.replicas : []; -} - /** Human-readable prefix for an error message — '' is the backend's whole root. */ function describePrefix(prefix: string): string { return prefix === '' ? 'the backend root' : `'${prefix}'`; diff --git a/server/src/nest/storage/storage-stats.service.ts b/server/src/nest/storage/storage-stats.service.ts index 65a537b0e..63fc337eb 100644 --- a/server/src/nest/storage/storage-stats.service.ts +++ b/server/src/nest/storage/storage-stats.service.ts @@ -1,6 +1,8 @@ import { Injectable, Logger } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { STORAGE_CATEGORIES, storageUsageSchema, type StorageUsage } from '@trek/shared'; -import { DatabaseService } from '../database/database.service'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; import { StorageService } from './storage.service'; export class StatsBusyError extends Error {} @@ -23,7 +25,7 @@ export class StorageStatsService { constructor( private readonly storage: StorageService, - private readonly db: DatabaseService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, ) {} async scan(): Promise { @@ -47,19 +49,27 @@ export class StorageStatsService { legacyPhotos.bytes += stat.size; } const usage: StorageUsage = { computedAt: Date.now(), categories, legacyPhotos }; - this.db.run('INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)', USAGE_KEY, JSON.stringify(usage)); + await this.appSettings.upsertOrReplace(USAGE_KEY, JSON.stringify(usage)); return usage; } finally { this.scanning = false; } } - /** The stored row, parsed; null when absent or unparseable (logged, never a 500). */ + /** + * The stored row, parsed; null when absent or unparseable (logged, never a + * 500). `getValue()` returns null for BOTH "no row" and "row present with + * a NULL value" — the legacy raw read only skipped the warn log for the + * former; a row with a NULL `value` here would previously have fallen + * through to `JSON.parse(null)` (parses to the JS `null`), failed the Zod + * schema and logged the same warning anyway. `scan()` only ever writes a + * JSON string, never NULL, so this distinction is unreachable in practice. + */ async readUsage(): Promise { - const row = this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', USAGE_KEY); - if (!row) return null; + const value = await this.appSettings.getValue(USAGE_KEY); + if (value == null) return null; try { - return storageUsageSchema.parse(JSON.parse(row.value)); + return storageUsageSchema.parse(JSON.parse(value)); } catch { this.logger.warn('stored storage.usage row is unparseable — treating as never computed'); return null; diff --git a/server/src/nest/storage/storage.module.ts b/server/src/nest/storage/storage.module.ts index ad8fb644d..8495bee0b 100644 --- a/server/src/nest/storage/storage.module.ts +++ b/server/src/nest/storage/storage.module.ts @@ -1,7 +1,9 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { AppConfigModule } from '../app-config/app-config.module'; import { AuditModule } from '../audit/audit.module'; import { SchedulingModule } from '../scheduling/scheduling.module'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; import { StorageEventsService } from './storage-events.service'; import { StorageJobsService } from './storage-jobs.service'; import { StorageRegistryService } from './storage-registry.service'; @@ -22,10 +24,13 @@ import { StorageUsageScanJob } from './storage-usage-scan.job'; * directly, the same as BackupModule/BackupController (which also sits behind * StorageModule) already does. StorageRegistryService stays UNEXPORTED — the * admin controller reaches it as a same-module provider, and nothing outside - * may cache drivers or trigger reloads. + * may cache drivers or trigger reloads. MikroOrmModule.forFeature registers + * AppSettingsRepository for StorageRegistryService/StorageAdminService/ + * StorageStatsService's @InjectRepository (Plan 3i, storage task — SR/SA/SS's + * 14 call sites, all on app_settings). */ @Module({ - imports: [AppConfigModule, AuditModule, SchedulingModule], + imports: [AppConfigModule, AuditModule, SchedulingModule, MikroOrmModule.forFeature([AppSettings])], controllers: [StorageAdminController], providers: [ StorageRegistryService, diff --git a/server/tests/integration/storage-restore-reload.test.ts b/server/tests/integration/storage-restore-reload.test.ts index 6fc28aac5..193d88d8a 100644 --- a/server/tests/integration/storage-restore-reload.test.ts +++ b/server/tests/integration/storage-restore-reload.test.ts @@ -30,17 +30,15 @@ const Database = require('better-sqlite3'); import { createTables } from '../../src/db/schema'; import { runMigrations } from '../../src/db/migrations'; -import { DatabaseService } from '../../src/nest/database/database.service'; import type { RuntimeEnvService } from '../../src/nest/app-config/runtime-env.service'; import { StorageEventsService } from '../../src/nest/storage/storage-events.service'; import { BACKENDS_KEY, CATEGORIES_KEY, StorageRegistryService } from '../../src/nest/storage/storage-registry.service'; import { StorageService } from '../../src/nest/storage/storage.service'; -import { createTestUnitOfWork } from '../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from '../helpers/test-uow'; const testDb = new Database(':memory:'); testDb.exec('PRAGMA journal_mode = WAL'); testDb.exec('PRAGMA foreign_keys = ON'); -const db = new DatabaseService(testDb); beforeAll(() => { createTables(testDb); @@ -81,7 +79,13 @@ describe('C6 — restore reloads the storage registry (audit #4)', () => { ]); setSetting(CATEGORIES_KEY, { files: 'nas-a' }); - const registry = new StorageRegistryService(db, envStub(), new StorageEventsService(), await createTestUnitOfWork(testDb)); + const registry = new StorageRegistryService( + await createTestAppSettingsRepo(testDb), + envStub(), + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); await registry.onModuleInit(); const storage = new StorageService(registry); diff --git a/server/tests/unit/db/repositories/AppSettings.repository.test.ts b/server/tests/unit/db/repositories/AppSettings.repository.test.ts index c7590cb20..2e7086c4f 100644 --- a/server/tests/unit/db/repositories/AppSettings.repository.test.ts +++ b/server/tests/unit/db/repositories/AppSettings.repository.test.ts @@ -133,6 +133,32 @@ describe('AppSettingsRepository', () => { expect(await appSettings.findByKeyPrefix('perm_')).toEqual([]); }); + // Plan 3i (storage task) additions: countKeysPresent (SR1's + // `SELECT COUNT(*) ... WHERE key IN (?, ?)`) and upsertOrReplace (SS1's + // `INSERT OR REPLACE`), kept as distinct methods from setValue's + // `ON CONFLICT DO UPDATE` upsert per R4/R5's ruling even though their + // observable behavior on this table is identical (setValue's own + // docstring explains why). + it('APPSETREPO-015: countKeysPresent counts every matching ROW, including one whose value is NULL — unlike getValues, which drops it', async () => { + insertRaw('storage.backends', '[]'); + insertRaw('storage.categories', null); // present row, NULL value + expect(await appSettings.countKeysPresent(['storage.backends', 'storage.categories'])).toBe(2); + expect(await appSettings.countKeysPresent(['storage.backends', 'storage.does_not_exist'])).toBe(1); + expect(await appSettings.countKeysPresent(['storage.nope_a', 'storage.nope_b'])).toBe(0); + }); + + it('APPSETREPO-016: upsertOrReplace inserts a new row exactly as INSERT OR REPLACE would', async () => { + await appSettings.upsertOrReplace('storage.usage', '{"computedAt":1}'); + expect(rawRow('storage.usage')).toStrictEqual({ key: 'storage.usage', value: '{"computedAt":1}' }); + }); + + it('APPSETREPO-017: upsertOrReplace on an existing key replaces the value only, with no duplicate/ghost row', async () => { + insertRaw('storage.usage', '{"computedAt":1}'); + await appSettings.upsertOrReplace('storage.usage', '{"computedAt":2}'); + expect(rawRow('storage.usage')).toStrictEqual({ key: 'storage.usage', value: '{"computedAt":2}' }); + expect(testDb.prepare('SELECT COUNT(*) as c FROM app_settings WHERE key = ?').get('storage.usage')).toEqual({ c: 1 }); + }); + // LIKE-escaping finding: neither the legacy `LIKE 'prefix%'` literal nor // `$like`'s bound parameter here escapes `%`/`_` in the prefix — a bound // LIKE parameter's wildcards are interpreted by SQLite exactly like a diff --git a/server/tests/unit/nest/storage/storage-admin.service.test.ts b/server/tests/unit/nest/storage/storage-admin.service.test.ts index e163d7098..88df3c647 100644 --- a/server/tests/unit/nest/storage/storage-admin.service.test.ts +++ b/server/tests/unit/nest/storage/storage-admin.service.test.ts @@ -17,7 +17,6 @@ import { Logger } from '@nestjs/common'; import { MASKED_SETTING_VALUE, type StorageConfig } from '@trek/shared'; import { createTables } from '../../../../src/db/schema'; import { runMigrations } from '../../../../src/db/migrations'; -import { DatabaseService } from '../../../../src/nest/database/database.service'; import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; import { encrypt_api_key } from '../../../../src/nest/common/crypto/apiKeyCrypto'; import { StorageAdminService } from '../../../../src/nest/storage/storage-admin.service'; @@ -27,9 +26,7 @@ import { StorageStatsService } from '../../../../src/nest/storage/storage-stats. import { StorageRegistryService, BACKENDS_KEY, CATEGORIES_KEY } from '../../../../src/nest/storage/storage-registry.service'; import { StorageService } from '../../../../src/nest/storage/storage.service'; import { StorageConflictError } from '../../../../src/nest/storage/storage.types'; -import { createTestUnitOfWork } from '../../../helpers/test-uow'; - -const db = new DatabaseService(testDb); +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from '../../../helpers/test-uow'; beforeAll(() => { createTables(testDb); @@ -67,12 +64,19 @@ async function makeService(opts: { uploadsRoot?: string } = {}) { setSetting(BACKENDS_KEY, JSON.stringify([{ name: 'uploads-local', type: 'local', options: { root: uploadsRoot } }])); const env = { env: () => ({ paths: {} }) } as unknown as RuntimeEnvService; const uow = await createTestUnitOfWork(testDb); - const registry = new StorageRegistryService(db, env, new StorageEventsService(), uow); + const appSettings = await createTestAppSettingsRepo(testDb); + const registry = new StorageRegistryService( + appSettings, + env, + new StorageEventsService(), + uow, + (await sharedTestOrm(testDb)).orm, + ); await registry.onModuleInit(); const storage = new StorageService(registry); const jobs = new StorageJobsService(registry); - const stats = new StorageStatsService(storage, db); - const service = new StorageAdminService(db, registry, storage, jobs, stats, uow); + const stats = new StorageStatsService(storage, appSettings); + const service = new StorageAdminService(appSettings, registry, storage, jobs, stats, uow); return { service, registry, uploadsRoot, stats, jobs, uow }; } diff --git a/server/tests/unit/nest/storage/storage-jobs.service.test.ts b/server/tests/unit/nest/storage/storage-jobs.service.test.ts index 646adb5a9..c285de122 100644 --- a/server/tests/unit/nest/storage/storage-jobs.service.test.ts +++ b/server/tests/unit/nest/storage/storage-jobs.service.test.ts @@ -17,7 +17,6 @@ import { Readable } from 'node:stream'; import { Logger } from '@nestjs/common'; import { createTables } from '../../../../src/db/schema'; import { runMigrations } from '../../../../src/db/migrations'; -import { DatabaseService } from '../../../../src/nest/database/database.service'; import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; import { MirrorDriver } from '../../../../src/nest/storage/drivers/mirror.driver'; import { StorageEventsService } from '../../../../src/nest/storage/storage-events.service'; @@ -30,9 +29,7 @@ import { MigrationTargetError, StorageJobsService, } from '../../../../src/nest/storage/storage-jobs.service'; -import { createTestUnitOfWork } from '../../../helpers/test-uow'; - -const db = new DatabaseService(testDb); +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from '../../../helpers/test-uow'; beforeAll(() => { createTables(testDb); @@ -72,7 +69,13 @@ async function makeWorld() { ); setSetting('storage.categories', JSON.stringify({ backups: 'm' })); const env = { env: () => ({ paths: {} }) } as unknown as RuntimeEnvService; - const registry = new StorageRegistryService(db, env, new StorageEventsService(), await createTestUnitOfWork(testDb)); + const registry = new StorageRegistryService( + await createTestAppSettingsRepo(testDb), + env, + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); await registry.onModuleInit(); const storage = new StorageService(registry); const jobs = new StorageJobsService(registry); @@ -102,7 +105,13 @@ async function makeMigrationWorld() { ); setSetting('storage.categories', JSON.stringify({ files: 'uploads-local' })); const env = { env: () => ({ paths: {} }) } as unknown as RuntimeEnvService; - const registry = new StorageRegistryService(db, env, new StorageEventsService(), await createTestUnitOfWork(testDb)); + const registry = new StorageRegistryService( + await createTestAppSettingsRepo(testDb), + env, + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); await registry.onModuleInit(); const storage = new StorageService(registry); const jobs = new StorageJobsService(registry); @@ -129,7 +138,13 @@ async function makePhotosGoogleMigrationWorld() { ]), ); const env = { env: () => ({ paths: { placePhotoDir: placePhotoRoot } }) } as unknown as RuntimeEnvService; - const registry = new StorageRegistryService(db, env, new StorageEventsService(), await createTestUnitOfWork(testDb)); + const registry = new StorageRegistryService( + await createTestAppSettingsRepo(testDb), + env, + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); await registry.onModuleInit(); const storage = new StorageService(registry); const jobs = new StorageJobsService(registry); @@ -154,7 +169,13 @@ async function makeMigrationBackfillWorld() { ); setSetting('storage.categories', JSON.stringify({ backups: 'm', files: 'uploads-local' })); const env = { env: () => ({ paths: {} }) } as unknown as RuntimeEnvService; - const registry = new StorageRegistryService(db, env, new StorageEventsService(), await createTestUnitOfWork(testDb)); + const registry = new StorageRegistryService( + await createTestAppSettingsRepo(testDb), + env, + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); await registry.onModuleInit(); const storage = new StorageService(registry); const jobs = new StorageJobsService(registry); @@ -530,7 +551,13 @@ describe('StorageJobsService.cancelJobsForMissingBackends', () => { ); setSetting('storage.categories', JSON.stringify({ files: 'nas' })); const env = { env: () => ({ paths: {} }) } as unknown as RuntimeEnvService; - const registry = new StorageRegistryService(db, env, new StorageEventsService(), await createTestUnitOfWork(testDb)); + const registry = new StorageRegistryService( + await createTestAppSettingsRepo(testDb), + env, + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); await registry.onModuleInit(); const storage = new StorageService(registry); const jobs = new StorageJobsService(registry); diff --git a/server/tests/unit/nest/storage/storage-registry.service.test.ts b/server/tests/unit/nest/storage/storage-registry.service.test.ts index 2e8721ade..5b3619024 100644 --- a/server/tests/unit/nest/storage/storage-registry.service.test.ts +++ b/server/tests/unit/nest/storage/storage-registry.service.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeAll, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from 'vitest'; import { Logger } from '@nestjs/common'; // ── DB setup (the permissions.service.test.ts pattern: real in-memory SQLite @@ -22,7 +22,6 @@ import path from 'node:path'; import { Readable } from 'node:stream'; import { createTables } from '../../../../src/db/schema'; import { runMigrations } from '../../../../src/db/migrations'; -import { DatabaseService } from '../../../../src/nest/database/database.service'; import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; import { encrypt_api_key } from '../../../../src/nest/common/crypto/apiKeyCrypto'; import { StorageEventsService } from '../../../../src/nest/storage/storage-events.service'; @@ -34,12 +33,11 @@ import { GLOBAL_TEMP_DIR, DEFAULT_BACKUPS_ROOT, DEFAULT_UPLOADS_ROOT, - SEED_CONFIG_PATH, + getSeedConfigPath, + setSeedConfigPathForTests, } from '../../../../src/nest/storage/storage-paths'; import { STORAGE_CATEGORIES } from '../../../../src/nest/storage/storage.types'; -import { createTestUnitOfWork } from '../../../helpers/test-uow'; - -const db = new DatabaseService(testDb); +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from '../../../helpers/test-uow'; beforeAll(() => { createTables(testDb); @@ -94,7 +92,13 @@ async function makeRegistry(opts: RegistryOpts = {}) { setSetting('storage.backends', JSON.stringify([uploadsOverride(uploadsRoot), ...(opts.backends ?? [])])); if (opts.categories) setSetting('storage.categories', JSON.stringify(opts.categories)); const stub = makeEnvStub({ placePhotoDir: opts.placePhotoDir }); - const registry = new StorageRegistryService(db, stub.env, new StorageEventsService(), await createTestUnitOfWork(testDb)); + const registry = new StorageRegistryService( + await createTestAppSettingsRepo(testDb), + stub.env, + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); if (opts.boot !== false) await registry.onModuleInit(); return { registry, uploadsRoot, setUploadsRoot: (root: string) => rewriteUploadsOverride(root) }; } @@ -474,7 +478,7 @@ describe('StorageRegistryService assignCategory', () => { expect(await registry.currentConfigVersion()).toBe(0); }); - it('REG-ASSIGN-005 refuses the migration flip onto a backend that is currently a mirror replica (validateConfig, other direction)', async () => { + it('REG-ASSIGN-005 refuses the migration flip onto a backend that is currently a mirror replica (validateConfig, other direction) — via the shared assertNoSharedReplicas predicate (R5 defect 2)', async () => { const { registry } = await makeRegistry({ backends: [ { name: 'nas-backups', type: 'local', options: { root: makeTmpDir() } }, @@ -483,14 +487,19 @@ describe('StorageRegistryService assignCategory', () => { categories: { backups: 'backups-mirror' }, }); + // Message wording changed under R5 defect 2: assignCategory no longer + // runs its own narrower inline check — it now calls the SAME + // assertNoSharedReplicas predicate validateConfig() uses, so this is the + // predicate's own case-1 message, not the old bespoke "cannot assign" + // text. Still refuses, still persists nothing — see the task report. await expect(registry.assignCategory('files', 'nas-backups')).rejects.toThrow( - /cannot assign 'files' to 'nas-backups' — 'nas-backups' is a mirror replica of 'backups-mirror'/, + /backend 'nas-backups' is a mirror replica of 'backups-mirror' and also serves category 'files'/, ); expect(registry.snapshot().categories.files.backend).toBe('uploads-local'); // unchanged expect(await registry.currentConfigVersion()).toBe(0); // nothing written }); - it('REG-ASSIGN-006 refuses the flip onto a MIRROR whose primary is a replica of another mirror', async () => { + it('REG-ASSIGN-006 refuses the flip onto a MIRROR whose primary is a replica of another mirror — via the shared assertNoSharedReplicas predicate (R5 defect 2)', async () => { const { registry } = await makeRegistry({ backends: [ { name: 'nas', type: 'local', options: { root: makeTmpDir() } }, @@ -501,11 +510,49 @@ describe('StorageRegistryService assignCategory', () => { categories: { backups: 'backups-mirror' }, }); + // Same wording change as REG-ASSIGN-005 — see its comment. await expect(registry.assignCategory('files', 'nas-mirror')).rejects.toThrow( - /cannot assign 'files' to 'nas-mirror' — 'nas' is a mirror replica of 'backups-mirror'/, + /backend 'nas' is a mirror replica of 'backups-mirror' and also serves category 'files'/, ); expect(await registry.currentConfigVersion()).toBe(0); }); + + it('REG-ASSIGN-007 R5 defect 2 (regression): refuses a flip that would let two mirrors share a replica with overlapping swept prefixes — the OLD narrower inline check missed this case entirely', async () => { + // Two mirrors (m-backups, m-files) both list 'nas' as a replica. Only + // 'backups' is routed through m-backups at boot — a single mirror + // replicating 'nas' trips no refusal (REG-SHARED-003's pairwise overlap + // check needs at least two). The OLD inline check in assignCategory only + // asked "is the flip's OWN target (or its mirror's primary) a replica of + // some mirror" — flipping 'files' onto m-files computes owner='p2' (the + // mirror's primary), and NOTHING replicates 'p2', so the old code would + // have let this through, only for the NEXT build()/reload() to refuse it + // (or worse, silently let two mirrors' sync sweeps fight over 'nas'). + const { registry } = await makeRegistry({ + backends: [ + { name: 'nas', type: 'local', options: { root: makeTmpDir() } }, + { name: 'p1', type: 'local', options: { root: makeTmpDir() } }, + { name: 'p2', type: 'local', options: { root: makeTmpDir() } }, + { name: 'm-backups', type: 'mirror', options: { primary: 'p1', replicas: ['nas'] } }, + { name: 'm-files', type: 'mirror', options: { primary: 'p2', replicas: ['nas'] } }, + ], + categories: { backups: 'm-backups' }, // 'files' still defaults to uploads-local + }); + expect(await registry.currentConfigVersion()).toBe(0); + + await expect(registry.assignCategory('files', 'm-files')).rejects.toThrow( + /backend 'nas' replicates both 'm-backups' and 'm-files', whose swept key prefixes overlap/, + ); + + // Refused BEFORE any write — not merely on the next reload. The + // pre-seeded 'storage.categories' row (written by makeRegistry's setup, + // not by this call) is byte-unchanged — no 'files' key was added. + expect(registry.snapshot().categories.files.backend).toBe('uploads-local'); + expect(await registry.currentConfigVersion()).toBe(0); + const row = testDb.prepare("SELECT value FROM app_settings WHERE key = 'storage.categories'").get() as { + value: string; + }; + expect(JSON.parse(row.value)).toEqual({ backups: 'm-backups' }); + }); }); // ── replica-failure health ──────────────────────────────────────────────────── @@ -718,17 +765,46 @@ describe('StorageRegistryService shared-replica refusals', () => { // ── seed-once boot import ───────────────────────────────────────────────────── describe('seed-once storage-config.json import', () => { + // R5 defect 1 fix (2026-09-25 ORM Plan 3i, storage task — the + // storage-config.json test-harness race, 3c's deferred L13, + // task-9-review-template.md:131): SEED_CONFIG_PATH used to be a FIXED + // source-tree path (data/storage-config.json) that this describe block + // wrote and deleted in place, while other vitest workers' concurrent + // buildApp() boots read the SAME real file through seedFromFileOnce() — + // genuinely racy across workers. setSeedConfigPathForTests() redirects + // every test below to a private per-worker tmp path (still named + // storage-config.json, so the SEED-004 error-message regex below still + // matches) that no other worker or suite ever touches; production never + // calls the setter, so getSeedConfigPath() there is always the real path. + const seedDir = fs.mkdtempSync(path.join(os.tmpdir(), 'trek-storage-seed-')); + const seedPath = path.join(seedDir, 'storage-config.json'); + + beforeAll(() => { + setSeedConfigPathForTests(seedPath); + }); + + afterAll(() => { + setSeedConfigPathForTests(null); + fs.rmSync(seedDir, { recursive: true, force: true }); + }); + const nasRoot = () => makeTmpDir(); function writeSeed(content: string): void { - fs.mkdirSync(path.dirname(SEED_CONFIG_PATH), { recursive: true }); - fs.writeFileSync(SEED_CONFIG_PATH, content); + fs.mkdirSync(path.dirname(seedPath), { recursive: true }); + fs.writeFileSync(seedPath, content); } /** A registry with NO storage.* rows (makeRegistry seeds an override row, so build raw). */ async function makeUnseededRegistry() { const stub = makeEnvStub({}); - return new StorageRegistryService(db, stub.env, new StorageEventsService(), await createTestUnitOfWork(testDb)); + return new StorageRegistryService( + await createTestAppSettingsRepo(testDb), + stub.env, + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); } function readRow(key: string): string | undefined { @@ -739,7 +815,12 @@ describe('seed-once storage-config.json import', () => { } afterEach(() => { - fs.rmSync(SEED_CONFIG_PATH, { force: true }); + fs.rmSync(seedPath, { force: true }); + }); + + it('REG-SEED-PATH-011 getSeedConfigPath() honors the test override (the race fix itself)', () => { + expect(getSeedConfigPath()).toBe(seedPath); + expect(getSeedConfigPath()).not.toBe(path.join('data', 'storage-config.json')); }); it('SEED-001 imports a valid file when no rows exist: encrypted rows, live drivers, pinned log', async () => { diff --git a/server/tests/unit/nest/storage/storage-stats.service.test.ts b/server/tests/unit/nest/storage/storage-stats.service.test.ts index 12b808e2d..88f6895f0 100644 --- a/server/tests/unit/nest/storage/storage-stats.service.test.ts +++ b/server/tests/unit/nest/storage/storage-stats.service.test.ts @@ -16,15 +16,12 @@ import path from 'node:path'; import { Readable } from 'node:stream'; import { createTables } from '../../../../src/db/schema'; import { runMigrations } from '../../../../src/db/migrations'; -import { DatabaseService } from '../../../../src/nest/database/database.service'; import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; import { StorageEventsService } from '../../../../src/nest/storage/storage-events.service'; import { StorageRegistryService } from '../../../../src/nest/storage/storage-registry.service'; import { StorageService } from '../../../../src/nest/storage/storage.service'; import { StatsBusyError, StorageStatsService } from '../../../../src/nest/storage/storage-stats.service'; -import { createTestUnitOfWork } from '../../../helpers/test-uow'; - -const db = new DatabaseService(testDb); +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from '../../../helpers/test-uow'; beforeAll(() => { createTables(testDb); @@ -59,10 +56,17 @@ async function makeWorld() { ]), ); const env = { env: () => ({ paths: {} }) } as unknown as RuntimeEnvService; - const registry = new StorageRegistryService(db, env, new StorageEventsService(), await createTestUnitOfWork(testDb)); + const appSettings = await createTestAppSettingsRepo(testDb); + const registry = new StorageRegistryService( + appSettings, + env, + new StorageEventsService(), + await createTestUnitOfWork(testDb), + (await sharedTestOrm(testDb)).orm, + ); await registry.onModuleInit(); const storage = new StorageService(registry); - const stats = new StorageStatsService(storage, db); + const stats = new StorageStatsService(storage, appSettings); return { storage, stats, uploadsRoot }; }