refactor(storage): backend/category config and usage stats through AppSettingsRepository; fix the seed-file test race and the shared-replica migration-flip gap

This commit is contained in:
jubnl
2026-09-24 01:20:46 +02:00
parent a51aedbc62
commit 36e178c085
12 changed files with 374 additions and 119 deletions
@@ -89,4 +89,36 @@ export class AppSettingsRepository extends TrekRepository<AppSettings> {
async deleteValue(key: string): Promise<number> {
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<number> {
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<void> {
await this.upsert({ key, value }, { onConflictFields: ['key'], onConflictAction: 'merge' });
}
}
@@ -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<unknown> {
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 [];
}
+36 -1
View File
@@ -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;
}
@@ -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<void> {
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<number> {
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<void> {
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<unknown> => {
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<string, BackendConfig>;
backendSources: Map<string, BackendSource>;
categoryBackends: Map<ServedCategory, string>;
categorySources: Map<ServedCategory, 'default' | 'settings'>;
} {
// 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<string, unknown> {
}
/** 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<number> {
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<number> {
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, string | number | string[]>): 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}'`;
@@ -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<StorageUsage> {
@@ -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<StorageUsage | null> {
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;
+7 -2
View File
@@ -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,