From 761cd14b2b6d58df2d2eee719cda8e738d3ab147 Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 22 Sep 2026 16:54:45 +0200 Subject: [PATCH] =?UTF-8?q?fix(server):=20plan=203a=20review=20follow-ups?= =?UTF-8?q?=20=E2=80=94=20fail=20closed=20without=20an=20ORM=20at=20every?= =?UTF-8?q?=20choke=20point;=20seam=20and=20D5=20ratchet=20tests;=20dead?= =?UTF-8?q?=20method=20and=20DDL=20copies=20removed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/db/repositories/Addons.repository.ts | 22 +-- .../db/repositories/AppSettings.repository.ts | 2 +- server/src/nest/audit/audit.service.ts | 32 ++++- server/src/nest/categories/categories.mcp.ts | 9 ++ server/src/nest/doc-sync/doc-sync.job.ts | 7 +- .../nest/integrations/airtrail-sync.job.ts | 17 ++- .../nest/integrations/dawarich-sync.job.ts | 23 ++- .../src/nest/memories/journey-thumbs.job.ts | 8 +- .../src/nest/memories/trek-photo-cache.job.ts | 9 +- .../notifications/reminder-jobs.service.ts | 40 +++--- .../place-photos/place-photo-cache.job.ts | 8 +- .../nest/plugins/plugin-runtime.service.ts | 15 +- .../plugins/supervisor/plugin-supervisor.ts | 28 +++- server/src/nest/realtime/trek-ws.adapter.ts | 21 ++- .../nest/scheduling/cron-registrar.service.ts | 48 ++++++- server/tests/e2e/addons.e2e.test.ts | 55 +++---- server/tests/helpers/test-uow.ts | 25 ++-- .../boot-sweeps-request-context.test.ts | 93 ++++++++++++ .../orm-request-context-seams.test.ts | 66 +++++++++ .../plugins/boot-registry-order.test.ts | 7 +- .../integration/plugins/supervisor.test.ts | 17 ++- .../db/repositories/Addons.repository.test.ts | 33 ----- .../tests/unit/nest/airtrail-sync.job.test.ts | 136 ++++++++++++++++++ server/tests/unit/nest/audit.service.test.ts | 22 ++- .../tests/unit/nest/cache-sweep-jobs.test.ts | 6 + .../unit/nest/cron-registrar.service.test.ts | 63 ++++++-- .../nest/database/unit-of-work.d5.test.ts | 54 +++++++ .../tests/unit/nest/dawarich-sync.job.test.ts | 16 +++ .../unit/nest/doc-sync/doc-sync.job.test.ts | 11 ++ .../nest/memories/journey-thumbs.job.test.ts | 83 +++++++++++ .../memories/trek-photo-cache.job.test.ts | 67 +++++++++ .../unit/nest/permissions.service.test.ts | 4 +- .../place-photo-cache.job.test.ts | 61 ++++++++ .../unit/nest/reminder-jobs.service.test.ts | 11 ++ .../tests/unit/nest/trek-ws-adapter.test.ts | 62 ++++---- .../unit/plugins/request-context.test.ts | 28 ++-- .../unit/plugins/supervisor-lifecycle.test.ts | 15 +- 37 files changed, 1020 insertions(+), 204 deletions(-) create mode 100644 server/tests/integration/boot-sweeps-request-context.test.ts create mode 100644 server/tests/integration/orm-request-context-seams.test.ts create mode 100644 server/tests/unit/nest/airtrail-sync.job.test.ts create mode 100644 server/tests/unit/nest/database/unit-of-work.d5.test.ts create mode 100644 server/tests/unit/nest/memories/journey-thumbs.job.test.ts create mode 100644 server/tests/unit/nest/memories/trek-photo-cache.job.test.ts create mode 100644 server/tests/unit/nest/place-photos/place-photo-cache.job.test.ts diff --git a/server/src/db/repositories/Addons.repository.ts b/server/src/db/repositories/Addons.repository.ts index 0987ac1d7..0c90af796 100644 --- a/server/src/db/repositories/Addons.repository.ts +++ b/server/src/db/repositories/Addons.repository.ts @@ -23,8 +23,8 @@ export class AddonsRepository extends EntityRepository { * `refresh: true` (Task 0 review, I1 — carried forward for every PK read in * this plan): a primary-key `findOne` answers a repeat call from the * identity map, invisible to a write on the same id inside the same - * request (`setEnabled`, below, or `admin.service.ts`'s still-raw - * `UPDATE addons SET enabled = ?` on the same connection). + * request (`admin.service.ts`'s still-raw `UPDATE addons SET enabled = ?` + * on the same connection). */ async isEnabled(id: string): Promise { const row = await this.findOne({ id }, { fields: ['enabled'], refresh: true }); @@ -43,22 +43,4 @@ export class AddonsRepository extends EntityRepository { const rows = await this.find({ enabled: true }, { orderBy: { sort_order: 'asc' } }); return rows.map((row) => toRow(row) as AddonRow); } - - /** - * `UPDATE addons SET enabled = ? WHERE id = ?` — the write half of the - * enablement toggle `admin.service.ts` still issues as raw SQL - * (`updateAddon`, outside this plan's six domains). Declared here, on the - * repository that owns the `addons` table, so that conversion has a - * ready-made method rather than inventing its own; nothing in this domain - * calls it yet. - * - * `enabled` is `p.boolean()` on the entity, so `nativeUpdate` stores the - * legacy `1`/`0` through MikroORM's own boolean type mapper — the - * repository test asserts the raw stored value directly, and that a - * following read (with `refresh: true`) comes back as a JS boolean, not - * the raw int. - */ - async setEnabled(id: string, enabled: boolean): Promise { - await this.nativeUpdate({ id }, { enabled }); - } } diff --git a/server/src/db/repositories/AppSettings.repository.ts b/server/src/db/repositories/AppSettings.repository.ts index ce7cfa2b6..06e4315a4 100644 --- a/server/src/db/repositories/AppSettings.repository.ts +++ b/server/src/db/repositories/AppSettings.repository.ts @@ -20,10 +20,10 @@ export class AppSettingsRepository extends EntityRepository { * the same key inside the same request, which is exactly the coexistence * case this plan is built on (raw SQL and the ORM sharing one connection * while callers convert one at a time). `refresh` keeps this a single + * query either way; only a stale identity-map hit is avoided. * Side effect of `refresh`: an UNFLUSHED in-memory change to the selected * field on that entity is discarded (the entity reverts to the row). No * caller mutates these entities before reading, by design (D4: rows out). - * query either way; only a stale identity-map hit is avoided. */ async getValue(key: string): Promise { const row = await this.findOne({ key }, { fields: ['value'], refresh: true }); diff --git a/server/src/nest/audit/audit.service.ts b/server/src/nest/audit/audit.service.ts index 378b2c24d..ac063c766 100644 --- a/server/src/nest/audit/audit.service.ts +++ b/server/src/nest/audit/audit.service.ts @@ -1,5 +1,6 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; +import { ValidationError } from '@mikro-orm/core'; import { AuditLog } from '../../db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../db/repositories/AuditLog.repository'; import { Users } from '../../db/entities/Users.entity'; @@ -80,10 +81,31 @@ export class AuditService { try { const email = await this.users.getEmail(userId); return email || `uid:${userId}`; - } catch { return `uid:${userId}`; } + } catch (e) { + // RULING (task-6-fix-brief.md item 4): an audit write must never block + // the request it is auditing, so this stays a swallow — the documented + // exception to "fail closed on a MikroORM ValidationError" every other + // domain in this plan follows. But a ValidationError here (typically + // cannotUseGlobalContext) is a wiring bug, not an ordinary DB failure, + // and folding it into the generic `uid:${userId}` fallback with no log + // line would hide a future unwrapped entrypoint. Log it distinctly so + // it is never confused with (or lost among) an actual missing user. + if (e instanceof ValidationError) { + logError(`Audit email lookup ran with no request context: ${e.message}`); + } + return `uid:${userId}`; + } } - /** Best-effort; never throws — failures are logged only. */ + /** + * Best-effort; never throws — failures are logged only. RULING + * (task-6-fix-brief.md item 4): this is the one documented exception to + * "fail closed on a MikroORM ValidationError" — an audit row must never + * become a 500 for the mutation it is recording. A ValidationError still + * gets a message distinct from every other write failure (below), so an + * unwrapped entrypoint stays visible in the log even though it never + * throws. + */ async writeAudit(entry: { userId: number | null; action: string; @@ -113,7 +135,11 @@ export class AuditService { logDebug(oneLine(`AUDIT ${entry.action} userId=${entry.userId} ${detailsJson}`)); } } catch (e) { - logError(`Audit write failed: ${e instanceof Error ? e.message : e}`); + if (e instanceof ValidationError) { + logError(`Audit write ran with no request context (row not written): ${e.message}`); + } else { + logError(`Audit write failed: ${e instanceof Error ? e.message : e}`); + } } } } diff --git a/server/src/nest/categories/categories.mcp.ts b/server/src/nest/categories/categories.mcp.ts index 0b08815f9..cf9128277 100644 --- a/server/src/nest/categories/categories.mcp.ts +++ b/server/src/nest/categories/categories.mcp.ts @@ -33,6 +33,15 @@ import { CategoriesService } from './categories.service'; export class CategoriesMcp { constructor( private readonly categories: CategoriesService, + // Carve-out (task-6-review-parity.md M3, mirroring addons.service.ts's own + // — see that file's docstring): this domain's own reads/writes are all + // repository-backed below; `DatabaseService` stays injected purely to + // feed `isDemoUser`'s `isDemoUserId(env, db, userId)`, whose + // `SELECT email FROM users WHERE id = ?` lives in + // `common/demo-write.ts` (nest/auth/nest/users, not this domain's plan). + // `UsersRepository.getEmail` already issues that identical statement — + // converting `demo-write.ts` to it is a deferred follow-up + // (`deferred.md`), not this plan's. private readonly db: DatabaseService, private readonly env: RuntimeEnvService, private readonly guards: McpToolGuardsService, diff --git a/server/src/nest/doc-sync/doc-sync.job.ts b/server/src/nest/doc-sync/doc-sync.job.ts index 54adf5a8a..4dd24e3d5 100644 --- a/server/src/nest/doc-sync/doc-sync.job.ts +++ b/server/src/nest/doc-sync/doc-sync.job.ts @@ -52,7 +52,12 @@ export class DocSyncJob implements OnApplicationBootstrap { async onApplicationBootstrap(): Promise { if (!this.registrar.isEnabled()) return; - logInfo(`Document sync: polling every ${await this.intervalSeconds()}s`); + // Through runOnBoot (task-6-review-parity.md C1: raw SQL today, but the + // wrap belongs at the entrypoint so it stays safe if this dependency + // graph goes repository-backed later). + await this.registrar.runOnBoot('docsync-boot', async () => { + logInfo(`Document sync: polling every ${await this.intervalSeconds()}s`); + }); this.registrar.register('docsync', '* * * * *', () => this.tick()); } diff --git a/server/src/nest/integrations/airtrail-sync.job.ts b/server/src/nest/integrations/airtrail-sync.job.ts index 915a2dded..36e0a3468 100644 --- a/server/src/nest/integrations/airtrail-sync.job.ts +++ b/server/src/nest/integrations/airtrail-sync.job.ts @@ -21,10 +21,19 @@ export class AirtrailSyncJob implements OnApplicationBootstrap { async onApplicationBootstrap(): Promise { if (!this.registrar.isEnabled()) return; - const value = this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', 'airtrail_poll_interval_minutes')?.value; - const raw = Number.parseInt(value || '5', 10); - const minutes = Number.isFinite(raw) && raw >= 1 && raw <= 59 ? raw : 5; - logInfo(`AirTrail sync: scheduled every ${minutes}m`); + // Through runOnBoot (task-6-review-parity.md C1: raw SQL today, but the + // wrap belongs at the entrypoint so it stays safe if this dependency + // graph goes repository-backed later). minutes defaults to the same + // fallback the interval read itself falls back to, so an absent MikroORM + // (logged distinctly by runOnBoot, never silently) still registers the + // job at its default cadence rather than not registering at all. + let minutes = 5; + await this.registrar.runOnBoot('airtrail-sync-boot', async () => { + const value = this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', 'airtrail_poll_interval_minutes')?.value; + const raw = Number.parseInt(value || '5', 10); + minutes = Number.isFinite(raw) && raw >= 1 && raw <= 59 ? raw : 5; + logInfo(`AirTrail sync: scheduled every ${minutes}m`); + }); this.registrar.register('airtrail-sync', `*/${minutes} * * * *`, () => this.tick()); } diff --git a/server/src/nest/integrations/dawarich-sync.job.ts b/server/src/nest/integrations/dawarich-sync.job.ts index 500d2882f..74741897a 100644 --- a/server/src/nest/integrations/dawarich-sync.job.ts +++ b/server/src/nest/integrations/dawarich-sync.job.ts @@ -27,13 +27,22 @@ export class DawarichSyncJob implements OnApplicationBootstrap { async onApplicationBootstrap(): Promise { if (!this.registrar.isEnabled()) return; - const value = this.db.get<{ value: string }>( - 'SELECT value FROM app_settings WHERE key = ?', - 'dawarich_poll_interval_minutes', - )?.value; - const raw = Number.parseInt(value || '15', 10); - const minutes = Number.isFinite(raw) && raw >= 5 && raw <= 59 ? raw : 15; - logInfo(`Dawarich sync: scheduled every ${minutes}m`); + // Through runOnBoot (task-6-review-parity.md C1: raw SQL today, but the + // wrap belongs at the entrypoint so it stays safe if this dependency + // graph goes repository-backed later). minutes defaults to the same + // fallback the interval read itself falls back to, so an absent MikroORM + // (logged distinctly by runOnBoot, never silently) still registers the + // job at its default cadence rather than not registering at all. + let minutes = 15; + await this.registrar.runOnBoot('dawarich-sync-boot', async () => { + const value = this.db.get<{ value: string }>( + 'SELECT value FROM app_settings WHERE key = ?', + 'dawarich_poll_interval_minutes', + )?.value; + const raw = Number.parseInt(value || '15', 10); + minutes = Number.isFinite(raw) && raw >= 5 && raw <= 59 ? raw : 15; + logInfo(`Dawarich sync: scheduled every ${minutes}m`); + }); this.registrar.register('dawarich-sync', `*/${minutes} * * * *`, () => this.tick()); } diff --git a/server/src/nest/memories/journey-thumbs.job.ts b/server/src/nest/memories/journey-thumbs.job.ts index c354facda..1c135e5e7 100644 --- a/server/src/nest/memories/journey-thumbs.job.ts +++ b/server/src/nest/memories/journey-thumbs.job.ts @@ -19,8 +19,12 @@ export class JourneyThumbsJob implements OnApplicationBootstrap { onApplicationBootstrap(): void { if (!this.registrar.isEnabled()) return; - // Run once on startup to reclaim orphans left over from before this sweeper existed. - void this.sweep(); + // Run once on startup to reclaim orphans left over from before this + // sweeper existed — through runOnBoot (task-6-review-parity.md C1), not + // the bare `void this.sweep()` this used to be: sweepOrphanThumbs reaches + // AddonsService.isAddonEnabled, which is repository-backed, and this call + // has no HTTP request (or registered-tick wrapper) behind it. + void this.registrar.runOnBoot('journey-thumbs-boot', () => this.sweep()); this.registrar.register('journey-thumbs', '0 4 * * *', () => { void this.sweep(); }); diff --git a/server/src/nest/memories/trek-photo-cache.job.ts b/server/src/nest/memories/trek-photo-cache.job.ts index 512ee7144..a00d54fc5 100644 --- a/server/src/nest/memories/trek-photo-cache.job.ts +++ b/server/src/nest/memories/trek-photo-cache.job.ts @@ -18,12 +18,15 @@ 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 - void (async () => { + // Run once immediately on startup to evict any entries left over from a + // previous run — through runOnBoot (task-6-review-parity.md C1: raw SQL + // today, but the wrap belongs at the entrypoint so it stays safe if this + // dependency graph goes repository-backed later). + void this.registrar.runOnBoot('trek-photo-cache-boot', 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' }); diff --git a/server/src/nest/notifications/reminder-jobs.service.ts b/server/src/nest/notifications/reminder-jobs.service.ts index 052c22190..c5dd3fa3c 100644 --- a/server/src/nest/notifications/reminder-jobs.service.ts +++ b/server/src/nest/notifications/reminder-jobs.service.ts @@ -34,26 +34,30 @@ export class ReminderJobsService implements OnApplicationBootstrap { if (!this.registrar.isEnabled()) return; // Boot banners only — the enable gates are read per tick below; these - // reflect the state at boot. - try { - const reminderEnabled = (await this.getSetting('notify_trip_reminder')) !== 'false'; - const channelsRaw = (await this.getSetting('notification_channels')) || (await this.getSetting('notification_channel')) || 'none'; - const activeChannels = channelsRaw === 'none' ? [] : channelsRaw.split(',').map(c => c.trim()); - if (!reminderEnabled) { - logInfo('Trip reminders: disabled in settings'); - } else { - const tripCount = this.db.get<{ c: number }>('SELECT COUNT(*) as c FROM trips WHERE reminder_days > 0 AND start_date IS NOT NULL')?.c ?? 0; - logInfo(`Trip reminders: enabled via [${activeChannels.join(',')}]${tripCount > 0 ? `, ${tripCount} trip(s) with active reminders` : ''}`); - } + // reflect the state at boot. Through runOnBoot (task-6-review-parity.md + // C1: raw SQL today, but the wrap belongs at the entrypoint so it stays + // safe if this dependency graph goes repository-backed later). + await this.registrar.runOnBoot('reminder-jobs-boot', async () => { + try { + const reminderEnabled = (await this.getSetting('notify_trip_reminder')) !== 'false'; + const channelsRaw = (await this.getSetting('notification_channels')) || (await this.getSetting('notification_channel')) || 'none'; + const activeChannels = channelsRaw === 'none' ? [] : channelsRaw.split(',').map(c => c.trim()); + if (!reminderEnabled) { + logInfo('Trip reminders: disabled in settings'); + } else { + const tripCount = this.db.get<{ c: number }>('SELECT COUNT(*) as c FROM trips WHERE reminder_days > 0 AND start_date IS NOT NULL')?.c ?? 0; + logInfo(`Trip reminders: enabled via [${activeChannels.join(',')}]${tripCount > 0 ? `, ${tripCount} trip(s) with active reminders` : ''}`); + } - if ((await this.getSetting('notify_todo_due')) !== 'false') { - logInfo(`Todo due reminders: enabled (lead ${TODO_REMINDER_LEAD_DAYS}d)`); - } else { - logInfo('Todo due reminders: disabled in settings'); + if ((await this.getSetting('notify_todo_due')) !== 'false') { + logInfo(`Todo due reminders: enabled (lead ${TODO_REMINDER_LEAD_DAYS}d)`); + } else { + logInfo('Todo due reminders: disabled in settings'); + } + } catch { + /* banners are best-effort */ } - } catch { - /* banners are best-effort */ - } + }); this.registrar.register('trip-reminders', '0 9 * * *', () => this.tripTick()); this.registrar.register('todo-reminders', '0 9 * * *', () => this.todoTick()); diff --git a/server/src/nest/place-photos/place-photo-cache.job.ts b/server/src/nest/place-photos/place-photo-cache.job.ts index b15d5123d..b097c7dec 100644 --- a/server/src/nest/place-photos/place-photo-cache.job.ts +++ b/server/src/nest/place-photos/place-photo-cache.job.ts @@ -19,8 +19,12 @@ export class PlacePhotoCacheJob implements OnApplicationBootstrap { onApplicationBootstrap(): void { if (!this.registrar.isEnabled()) return; - // Run once on startup to reclaim orphans left over from before this sweeper existed. - void this.sweep(); + // Run once on startup to reclaim orphans left over from before this + // sweeper existed — through runOnBoot (task-6-review-parity.md C1's + // "scanned for siblings" list: sweepOrphans is raw SQL today, safe, but + // the wrap belongs at the entrypoint so it stays safe once this + // dependency graph goes repository-backed too, same as journey-thumbs). + void this.registrar.runOnBoot('place-photo-cache-boot', () => this.sweep()); this.registrar.register('place-photo-cache', '30 3 * * *', () => { void this.sweep(); }); diff --git a/server/src/nest/plugins/plugin-runtime.service.ts b/server/src/nest/plugins/plugin-runtime.service.ts index 5b26bb6a1..97f1b1fb5 100644 --- a/server/src/nest/plugins/plugin-runtime.service.ts +++ b/server/src/nest/plugins/plugin-runtime.service.ts @@ -1,4 +1,4 @@ -import { Injectable, type OnApplicationBootstrap, type OnModuleDestroy } from '@nestjs/common'; +import { Injectable, Optional, type OnApplicationBootstrap, type OnModuleDestroy } from '@nestjs/common'; import semver from 'semver'; import { MikroORM } from '@mikro-orm/core'; import { DatabaseService } from '../database/database.service'; @@ -195,8 +195,17 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // request context built from this. Nest always injects it (MikroOrmCoreModule is // global); a hand-built test instance that dispatches through a real // PermissionsService/repository must pass one or those reads fail closed with - // cannotUseGlobalContext instead of running. - private readonly orm?: MikroORM, + // cannotUseGlobalContext instead of running (task-6-fix-brief.md item 1: the + // supervisor now THROWS rather than dispatching unwrapped when it is absent). + // `@Optional()` (task-6-review-template.md Minor 3 — harmonised with + // `CronRegistrarService`'s own `@Optional()` orm param; a plain `?` is + // TS-only and does not tell Nest's DI a provider may be absent): safe + // today because nothing constructs this service through a partial + // `Test.createTestingModule` graph, only `new PluginRuntimeService(...)` + // by hand or the full `buildApp()` — but the first partial harness that + // does needs this, the same lesson `CronRegistrarService` already learned + // (task-2-fix-report.md's "Concerns"). + @Optional() private readonly orm?: MikroORM, ) {} private get db() { diff --git a/server/src/nest/plugins/supervisor/plugin-supervisor.ts b/server/src/nest/plugins/supervisor/plugin-supervisor.ts index 32a096935..6f510ec04 100644 --- a/server/src/nest/plugins/supervisor/plugin-supervisor.ts +++ b/server/src/nest/plugins/supervisor/plugin-supervisor.ts @@ -120,9 +120,13 @@ export class PluginSupervisor { // PluginRuntimeService's own `orm` field is not assigned yet when its `supervisor` // field initializer runs (constructor params are assigned after field // initializers — see that file's comment), so this is read lazily, once per - // dispatch. Optional only so the hand-built doubles in - // supervisor-lifecycle.test.ts / provider-hook-grant.test.ts / - // event-subscriptions.test.ts (none of which reach a repository) don't need one; + // dispatch. Optional at the type level only for the hand-built doubles in + // provider-hook-grant.test.ts / event-subscriptions.test.ts, neither of + // which ever dispatches a `'req'` message through onMessage — a double + // that DOES dispatch (supervisor-lifecycle.test.ts's rate-limit case, + // tests/integration/plugins/supervisor.test.ts's real child IPC) must + // pass one, because onMessage now THROWS when this returns undefined + // (task-6-fix-brief.md item 1 — no more silent unwrapped dispatch). // PluginRuntimeService always passes the real MikroORM (OrmModule is global). private readonly resolveOrm?: () => { em: EntityManager } | undefined, ) { @@ -568,10 +572,22 @@ export class PluginSupervisor { // dispatch's own audit write. Without this, PermissionsService.checkPermission // ran outside any context, threw on a cold cache, and the catch served // defaults — fail-open on a tightened flag (task-2-review.md C3). + // + // Fail closed (task-6-fix-brief.md item 1, task-6-review-template.md + // Important 1 / M-B): an absent `resolveOrm` used to fall through to + // an UNWRAPPED dispatch — silent degrade, the exact shape D6 forbids + // at a choke point. It only failed closed downstream, and only + // because two OTHER switches happened to hold (production's + // `allowGlobalContext: false` and `loadPermissions` rethrowing a + // MikroORM `ValidationError`) — nothing here said so. Throw instead: + // the dispatch never runs without a context, full stop, regardless of + // what those other switches do. The three hand-built test doubles + // that dispatch through this path now pass a thunk (`sharedTestOrm`). const orm = this.resolveOrm?.(); - const res = orm - ? await withRequestContext(orm, () => sup.rpcHost.dispatch(req, actingUserId)) - : await sup.rpcHost.dispatch(req, actingUserId); + if (!orm) { + throw new Error('PluginSupervisor: no ORM available to build a request context for this RPC dispatch'); + } + const res = await withRequestContext(orm, () => sup.rpcHost.dispatch(req, actingUserId)); sup.child?.send(res); } finally { sup.rpcLimiter.release(); diff --git a/server/src/nest/realtime/trek-ws.adapter.ts b/server/src/nest/realtime/trek-ws.adapter.ts index 5899b5f1e..a6bfabeeb 100644 --- a/server/src/nest/realtime/trek-ws.adapter.ts +++ b/server/src/nest/realtime/trek-ws.adapter.ts @@ -104,9 +104,11 @@ export class TrekWsAdapter extends WsAdapter { httpServer: HttpServer, // D6 (task-2-review.md's controller ruling): a WS message handler has no HTTP // request behind it, so every `@SubscribeMessage` dispatch is wrapped here — - // ONE wrapper at bindMessageHandlers below, not one per handler. Optional only - // for trek-ws-adapter.test.ts, which never reaches a repository; bootstrap.ts - // always passes the real MikroORM. + // ONE wrapper at bindMessageHandlers below, not one per handler. Optional at + // the type level only for hand-built test doubles — bootstrap.ts always + // passes the real MikroORM, and any double that dispatches a matched + // message through bindMessageHandlers (task-6-fix-brief.md item 1) must + // pass one too, or the dispatch throws rather than running unwrapped. private readonly orm?: { em: EntityManager }, ) { super(httpServer as unknown as INestApplicationContext); @@ -219,9 +221,16 @@ export class TrekWsAdapter extends WsAdapter { // invoked (before transform()/subscribe() ever run), and RequestContext.create // is AsyncLocalStorage.run — it only covers what executes inside this // synchronous frame, after which the async chain carries it on its own. - const result = this.orm - ? withRequestContext(this.orm, () => handler.callback(message, socket)) - : handler.callback(message, socket); + // + // Fail closed (task-6-fix-brief.md item 1): a missing `orm` used to fall + // through to an unwrapped call — silent degrade. `bootstrap.ts` always + // passes the real MikroORM, so this only ever throws in a hand-built + // test double that dispatches through bindMessageHandlers without one; + // that double now passes an ORM from `sharedTestOrm`. + if (!this.orm) { + throw new Error('TrekWsAdapter: no MikroORM available to build a request context for this message handler'); + } + const result = withRequestContext(this.orm, () => handler.callback(message, socket)); transform(result).subscribe({ next: (response) => { if (response !== undefined && socket.readyState === 1) { diff --git a/server/src/nest/scheduling/cron-registrar.service.ts b/server/src/nest/scheduling/cron-registrar.service.ts index 0b18e59cd..1e23ab618 100644 --- a/server/src/nest/scheduling/cron-registrar.service.ts +++ b/server/src/nest/scheduling/cron-registrar.service.ts @@ -5,6 +5,7 @@ import { MikroORM } from '@mikro-orm/core'; import { readEnv } from '../../app-config'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; import { withRequestContext } from '../database/request-context'; +import { logError } from '../audit/audit-log.logger'; /** * The one way TREK code schedules a cron. Job providers register here from @@ -71,13 +72,58 @@ export class CronRegistrarService implements OnApplicationShutdown { if (!this.isEnabled()) return false; const timeZone = opts?.timezone === 'none' ? undefined : readEnv().app.tz || 'UTC'; const orm = this.orm; - const wrappedTick = orm ? () => withRequestContext(orm, () => onTick()) : onTick; + // Fail closed (task-6-fix-brief.md item 1): registration itself never + // requires an ORM (CRONREG-002..009 register jobs whose onTick never + // touches a repository, and the e2e partial harnesses that construct this + // service via @Optional() with no MikroORM never tick at all — isEnabled() + // is false under NODE_ENV=test). But once a tick DOES fire, it must run + // inside a request context or not at all: dispatching it unwrapped used to + // let a repository read inside onTick either silently succeed against the + // wrong (global) EntityManager or throw MikroORM's own generic + // "global EntityManager" error deep inside the job's own try/catch. Throw + // OUR OWN clear error instead, so a hand-built double that registers a + // job without an ORM but whose tick reaches a repository fails loudly and + // distinctly rather than silently degrading. + const wrappedTick = async () => { + if (!orm) { + throw new Error(`CronRegistrarService: no MikroORM available to build a request context for job "${name}"`); + } + return withRequestContext(orm, () => onTick()); + }; const job = CronJob.from({ cronTime: expression, onTick: wrappedTick, start: true, timeZone }); this.registry.addCronJob(name, job); this.names.add(name); return true; } + /** + * Runs a boot-time (`onApplicationBootstrap`) one-off sweep inside the same + * request context a registered tick gets from `register()` above — the one + * choke point for the whole class of bug task-6-review-parity.md's C1 + * found: a job's "run once at startup" call is not a scheduled tick, so it + * bypassed `register()`'s wrapper entirely; once its dependency graph went + * repository-backed (`AddonsService.isAddonEnabled`), the read threw + * `cannotUseGlobalContext` on EVERY production boot — silently, because the + * job's own try/catch swallowed it into a log line about the sweep's own + * domain, never mentioning the missing context. Every + * `onApplicationBootstrap` boot sweep goes through this one method now, + * whether or not it reaches a repository TODAY, so a job's dependency graph + * can grow one later without silently regressing to the same bug. + * + * Fails closed like every other D6 choke point: no MikroORM means `fn` + * never runs at all, and the failure is logged with a message naming THIS + * method — never MikroORM's own "global EntityManager" wording — so a + * missing-context boot failure is never confused with (or swallowed by) an + * ordinary sweep failure the job's own catch already logs. + */ + async runOnBoot(name: string, fn: () => Promise | void): Promise { + if (!this.orm) { + logError(`CronRegistrarService.runOnBoot: no MikroORM available — boot sweep "${name}" did not run`); + return; + } + await withRequestContext(this.orm, fn); + } + /** Stop and drop a job by name. A name that was never registered is a no-op. */ unregister(name: string): void { if (!this.names.has(name)) return; diff --git a/server/tests/e2e/addons.e2e.test.ts b/server/tests/e2e/addons.e2e.test.ts index badc2b476..8a626d1e2 100644 --- a/server/tests/e2e/addons.e2e.test.ts +++ b/server/tests/e2e/addons.e2e.test.ts @@ -1,9 +1,13 @@ /** * GET /api/addons e2e — exercises the AddonsController through the real - * JwtAuthGuard against a temp SQLite db. getPhotoProviderConfig is - * mocked; the addons/photo_providers/photo_provider_fields/app_settings reads + * JwtAuthGuard against a real migrated-and-seeded temp SQLite db + * (createSnapshotTestDb(), task-6-fix-brief.md item 5 — this used to hand-roll + * four CREATE TABLEs, a second hand-maintained schema copy whose NOT + * NULL/defaults/FK/UNIQUE drifted from the real migrations). getPhotoProviderConfig + * is mocked; the addons/photo_providers/photo_provider_fields/app_settings reads * run against the temp db (the collab/bag-tracking flags are real AddonsService - * reads since the admin-1 extraction). Asserts the byte-identical body the legacy inline handler produced. + * reads since the admin-1 extraction). Asserts the byte-identical body the legacy + * inline handler produced. */ import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import request from 'supertest'; @@ -11,32 +15,23 @@ import cookieParser from 'cookie-parser'; import type { Server } from 'http'; import { DatabaseModule } from '../../src/nest/database/database.module'; import { Test } from '@nestjs/testing'; -import { seedUser, sessionCookie } from './harness'; +import { sessionCookie } from './harness'; -const { db } = vi.hoisted(() => { - // eslint-disable-next-line @typescript-eslint/no-require-imports - const Database = require('better-sqlite3'); - const tmp = new Database(':memory:'); - tmp.exec('PRAGMA journal_mode = WAL'); - tmp.exec(`CREATE TABLE users (id INTEGER PRIMARY KEY AUTOINCREMENT, username TEXT NOT NULL, - email TEXT NOT NULL UNIQUE, role TEXT NOT NULL DEFAULT 'user', password_version INTEGER NOT NULL DEFAULT 0);`); - // description/config (addons) and description (photo_providers) are absent - // from the legacy SELECTs this suite exercises, but the Addons/PhotoProviders - // entities declare them as columns (Plan 3a Task 4) — MikroORM's generated - // SELECT names every scalar column, so they must exist on this hand-rolled - // schema even though no test here reads them off addons/photo_providers rows. - tmp.exec(`CREATE TABLE addons (id TEXT PRIMARY KEY, name TEXT, description TEXT, type TEXT, icon TEXT, enabled INTEGER, config TEXT DEFAULT '{}', sort_order INTEGER);`); - tmp.exec(`CREATE TABLE photo_providers (id TEXT PRIMARY KEY, name TEXT, description TEXT, icon TEXT, enabled INTEGER, sort_order INTEGER);`); - tmp.exec(`CREATE TABLE photo_provider_fields (id INTEGER PRIMARY KEY AUTOINCREMENT, provider_id TEXT, field_key TEXT, - label TEXT, input_type TEXT, placeholder TEXT, hint TEXT, required INTEGER, secret INTEGER, - settings_key TEXT, payload_key TEXT, sort_order INTEGER);`); - tmp.exec(`CREATE TABLE app_settings (key TEXT PRIMARY KEY, value TEXT);`); - return { db: tmp }; +vi.mock('../../src/db/database', async () => { + const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); + return buildDbMock(createSnapshotTestDb()); }); -vi.mock('../../src/db/database', () => ({ - db, canAccessTrip: vi.fn(), isOwner: vi.fn(), getPlaceWithTags: vi.fn(), closeDb: () => {}, reinitialize: () => {}, -})); +import { db } from '../../src/db/database'; + +// The snapshot ships the real seeded catalogue (production default addons + +// photo providers) — this suite wants a known, empty set of each so every +// case controls its own fixture rows exactly like the legacy hand-rolled +// tables did. Child table first for the FK on photo_provider_fields. +db.exec('DELETE FROM photo_provider_fields'); +db.exec('DELETE FROM photo_providers'); +db.exec('DELETE FROM addons'); +db.exec('DELETE FROM app_settings'); const { getPhotoProviderConfig } = vi.hoisted(() => ({ getPhotoProviderConfig: vi.fn(() => ({ url: 'https://immich.example' })), @@ -62,7 +57,13 @@ describe('GET /api/addons e2e (real auth guard + temp SQLite)', () => { } beforeAll(async () => { - seedUser(db as never, { id: 1 }); + // harness.ts's seedUser() omits password_hash, which the real migrated + // schema requires NOT NULL (share.e2e.test.ts hit the same thing) — a + // raw insert here instead, matching the SeededUser shape id/role/ + // password_version=0 that sessionCookie(1) needs. + db.prepare( + "INSERT INTO users (id, username, email, password_hash, role, password_version) VALUES (1, 'e2e-user', 'e2e@example.test', 'x', 'user', 0)", + ).run(); // bag tracking is opt-in (=== 'true'); collab flags default ON with no rows db.prepare("INSERT INTO app_settings (key, value) VALUES ('bag_tracking_enabled', 'true')").run(); db.prepare("INSERT INTO addons (id, name, type, icon, enabled, sort_order) VALUES ('packing','Packing','trip','Backpack',1,1)").run(); diff --git a/server/tests/helpers/test-uow.ts b/server/tests/helpers/test-uow.ts index e70975aee..35d08a6e9 100644 --- a/server/tests/helpers/test-uow.ts +++ b/server/tests/helpers/test-uow.ts @@ -20,15 +20,22 @@ const tagsPerHandle = new WeakMap>(); const settingsPerHandle = new WeakMap>(); const usersPerHandle = new WeakMap>(); // ONE MikroORM per handle, shared by createTestUnitOfWork and -// createTestAppSettingsRepo (task-2-review.md I2): each used to call -// createTestOrm(db) independently, which opened a SECOND MikroORM.init over the -// same better-sqlite3 handle — two identity maps, two Kysely clients, and -// (the functional bug) `UnitOfWork.transactional` opening its transaction on -// ORM A's EntityManager while a repository built from ORM B resolved a -// DIFFERENT EntityManager, so `TransactionContext` never saw it: a repository -// write inside `uow.transactional(...)` silently ran outside the transaction. -// Both functions below now derive from this single `t.em`, so a repository -// resolved from `t.repo(X)` and a `UnitOfWork` built from `t.em` share the same +// createTestAppSettingsRepo (task-2-review.md I2; reworded per +// task-1-2-rereview.md's I-A — the original wording here claimed a +// functional bug that measurement disproved): each used to call +// createTestOrm(db) independently, which opened a SECOND MikroORM.init over +// the same better-sqlite3 handle — two identity maps and two Kysely clients, +// neither ever closed. That is NOT the same as a repository write silently +// running outside a transaction: `TransactionContext.getEntityManager(name)` +// keys on the EntityManager's CONTEXT NAME ('default'), not on which ORM +// instance created it, so a repository built from ORM B still resolved ORM +// A's transactional fork correctly even under the old split wiring — +// transaction resolution worked either way. The reason to share one ORM per +// handle is simpler: it is the same EntityManager Nest's real DI graph would +// inject (one MikroORM per app, not one per consumer), and it stops leaking +// a second identity map and Kysely client per test file. Both functions below +// now derive from this single `t.em`, so a repository resolved from +// `t.repo(X)` and a `UnitOfWork` built from `t.em` share the same // context-resolving EntityManager, exactly like Nest's real DI graph. const ormPerHandle = new WeakMap>(); /** diff --git a/server/tests/integration/boot-sweeps-request-context.test.ts b/server/tests/integration/boot-sweeps-request-context.test.ts new file mode 100644 index 000000000..786b1b852 --- /dev/null +++ b/server/tests/integration/boot-sweeps-request-context.test.ts @@ -0,0 +1,93 @@ +/** + * Boot-sweep request-context regression guard (task-6-review-parity.md + * Critical C1; task-6-fix-brief.md item 7). + * + * `journey-thumbs.job.ts`'s `onApplicationBootstrap` used to call + * `void this.sweep()` directly, bypassing `CronRegistrarService`'s + * request-context wrapper entirely — the one place every SCHEDULED tick + * gets it. Once `sweepOrphanThumbs` went repository-backed + * (`AddonsService.isAddonEnabled`), that boot-time call threw + * `cannotUseGlobalContext` on EVERY production boot, silently, because the + * job's own try/catch swallowed it into a log line about the sweep itself. + * The smoke boot reproduced it twice. Every `onApplicationBootstrap` boot + * sweep across the seven job providers listed in the parity review now + * routes through `CronRegistrarService.runOnBoot` instead. + * + * Under `NODE_ENV=test`, `CronRegistrarService.isEnabled()` is false + * (SCHED-GATE), so `onApplicationBootstrap` never fires these sweeps + * automatically in this suite — deliberately, so tests stay timer- and + * network-free. This test instead triggers each job's real boot-sweep + * entrypoint directly through the SAME, production-wired + * `CronRegistrarService.runOnBoot` (its `orm` is `app.get(MikroORM)` — + * pinned by orm-request-context-seams.test.ts), which is exactly what + * `onApplicationBootstrap` itself would call in production. It then asserts + * the captured log contains zero `cannotUseGlobalContext` / "global + * EntityManager" lines — the regression guard the smoke boot would + * otherwise be the only thing catching. + */ +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import type { INestApplication } from '@nestjs/common'; + +vi.mock('../../src/db/database', async () => { + const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); + return buildDbMock(createSnapshotTestDb()); +}); +vi.mock('../../src/config', () => ({ + JWT_SECRET: 'test-jwt-secret-for-trek-testing-only', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, + SESSION_DURATION: '24h', + SESSION_DURATION_MS: 86400000, + SESSION_DURATION_SECONDS: 86400, + DEFAULT_LANGUAGE: 'en', +})); +vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn(), getOnlineUserIds: vi.fn(() => []) })); + +import { db as testDb } from '../../src/db/database'; +import { buildApp } from '../../src/bootstrap'; +import { CronRegistrarService } from '../../src/nest/scheduling/cron-registrar.service'; +import { JourneyThumbsJob } from '../../src/nest/memories/journey-thumbs.job'; +import { PlacePhotoCacheJob } from '../../src/nest/place-photos/place-photo-cache.job'; +import { TrekPhotoCacheJob } from '../../src/nest/memories/trek-photo-cache.job'; +import { ReminderJobsService } from '../../src/nest/notifications/reminder-jobs.service'; +import { DocSyncJob } from '../../src/nest/doc-sync/doc-sync.job'; +import { AirtrailSyncJob } from '../../src/nest/integrations/airtrail-sync.job'; +import { DawarichSyncJob } from '../../src/nest/integrations/dawarich-sync.job'; + +describe('Every onApplicationBootstrap boot sweep runs inside a request context', () => { + let app: INestApplication; + + beforeAll(async () => { + app = await buildApp(); + }); + + afterAll(async () => { + await app.close(); + testDb.close(); + }); + + it('BOOT-SWEEP-001: the seven boot-sweep entrypoints, invoked through the real production-wired CronRegistrarService.runOnBoot, never log cannotUseGlobalContext / "global EntityManager"', async () => { + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const registrar = app.get(CronRegistrarService); + + await registrar.runOnBoot('journey-thumbs-boot', () => app.get(JourneyThumbsJob).sweep()); + await registrar.runOnBoot('place-photo-cache-boot', () => app.get(PlacePhotoCacheJob).sweep()); + await registrar.runOnBoot('trek-photo-cache-boot', () => app.get(TrekPhotoCacheJob).tick()); + await registrar.runOnBoot('reminder-jobs-boot', async () => { + await app.get(ReminderJobsService).tripTick(); + await app.get(ReminderJobsService).todoTick(); + }); + await registrar.runOnBoot('docsync-boot', () => app.get(DocSyncJob).tick()); + await registrar.runOnBoot('airtrail-sync-boot', () => app.get(AirtrailSyncJob).tick()); + await registrar.runOnBoot('dawarich-sync-boot', () => app.get(DawarichSyncJob).tick()); + + const suspicious = errSpy.mock.calls + .map((args) => args.map(String).join(' ')) + .filter((line) => /cannotUseGlobalContext|global EntityManager/i.test(line)); + expect(suspicious).toEqual([]); + } finally { + errSpy.mockRestore(); + } + }); +}); diff --git a/server/tests/integration/orm-request-context-seams.test.ts b/server/tests/integration/orm-request-context-seams.test.ts new file mode 100644 index 000000000..f54123829 --- /dev/null +++ b/server/tests/integration/orm-request-context-seams.test.ts @@ -0,0 +1,66 @@ +/** + * ORM request-context seam ratchet (task-6-fix-brief.md item 2; + * task-6-review-template.md's Important 4 / M-E). task-6-fix-brief.md item 1 + * made the three D6 choke points — CronRegistrarService.register/runOnBoot, + * PluginSupervisor.onMessage's dispatch, TrekWsAdapter.bindMessageHandlers — + * THROW when their ORM/thunk is absent, and every unit suite (WSAD-040, + * CRONREG-010, CTX-PLUGIN-001) asserts that throw. None of those tests can + * tell a genuinely wired production seam apart from a hand-built double that + * happens to pass no ORM — they'd pass either way. This is the one test that + * pins production wiring itself: that `buildApp()` actually populates + * `CronRegistrarService`'s and `PluginRuntimeService`'s `@Optional()` ORM + * params and `PluginSupervisor`'s `resolveOrm` thunk with the SAME MikroORM + * instance `app.get(MikroORM)` resolves — so a future refactor that quietly + * drops one of those three constructor arguments fails here, not silently in + * production. + */ +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import type { INestApplication } from '@nestjs/common'; + +vi.mock('../../src/db/database', async () => { + const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); + return buildDbMock(createSnapshotTestDb()); +}); +vi.mock('../../src/config', () => ({ + JWT_SECRET: 'test-jwt-secret-for-trek-testing-only', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, + SESSION_DURATION: '24h', + SESSION_DURATION_MS: 86400000, + SESSION_DURATION_SECONDS: 86400, + DEFAULT_LANGUAGE: 'en', +})); +vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn(), getOnlineUserIds: vi.fn(() => []) })); + +import { db as testDb } from '../../src/db/database'; +import { MikroORM } from '@mikro-orm/core'; +import { buildApp } from '../../src/bootstrap'; +import { CronRegistrarService } from '../../src/nest/scheduling/cron-registrar.service'; +import { PluginRuntimeService } from '../../src/nest/plugins/plugin-runtime.service'; + +describe('ORM request-context seams populated in production', () => { + let app: INestApplication; + + beforeAll(async () => { + app = await buildApp(); + }); + + afterAll(async () => { + await app.close(); + testDb.close(); + }); + + it('SEAM-001: CronRegistrarService, PluginRuntimeService and PluginSupervisor.resolveOrm() all resolve the SAME MikroORM app.get(MikroORM) does', () => { + const orm = app.get(MikroORM); + + const cronRegistrar = app.get(CronRegistrarService) as unknown as { orm?: MikroORM }; + expect(cronRegistrar.orm).toBe(orm); + + const runtime = app.get(PluginRuntimeService) as unknown as { + orm?: MikroORM; + supervisor: { resolveOrm?: () => unknown }; + }; + expect(runtime.orm).toBe(orm); + expect(runtime.supervisor.resolveOrm?.()).toBe(orm); + }); +}); diff --git a/server/tests/integration/plugins/boot-registry-order.test.ts b/server/tests/integration/plugins/boot-registry-order.test.ts index c7dd38e6a..d42a0332a 100644 --- a/server/tests/integration/plugins/boot-registry-order.test.ts +++ b/server/tests/integration/plugins/boot-registry-order.test.ts @@ -123,7 +123,12 @@ describe('plugin boot vs registry scan ordering', () => { // (if it had one) first. The boot reconcile must therefore not live there. { provide: PluginRuntimeService, - useFactory: () => new PluginRuntimeService(dbs, new AuditService(auditLogRepo, usersRepo), addonsService, userSettings, undefined, hostFactory), + // 8th arg (orm): task-6-fix-brief.md item 1 — PluginSupervisor.onMessage + // now THROWS on a 'req' dispatch (e.g. this fixture's ctx.db.migrate in + // onLoad) when resolveOrm returns undefined, instead of running it + // unwrapped, so this hand-built double needs the same real ORM the + // repositories above (auditLogRepo/usersRepo) already share. + useFactory: () => new PluginRuntimeService(dbs, new AuditService(auditLogRepo, usersRepo), addonsService, userSettings, undefined, hostFactory, undefined, t?.orm), }, { provide: 'REGISTRY_SCAN', diff --git a/server/tests/integration/plugins/supervisor.test.ts b/server/tests/integration/plugins/supervisor.test.ts index a62b9f401..a3630d0cc 100644 --- a/server/tests/integration/plugins/supervisor.test.ts +++ b/server/tests/integration/plugins/supervisor.test.ts @@ -11,12 +11,14 @@ import { describe, it, expect, beforeAll, afterAll, afterEach } from 'vitest'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; +import Database from 'better-sqlite3'; import { PluginSupervisor, type SupervisorHooks, type SupervisorTuning } from '../../../src/nest/plugins/supervisor/plugin-supervisor'; import { PluginRpcHost, type HostDeps } from '../../../src/nest/plugins/host/rpc-host'; import { PluginDataDb } from '../../../src/nest/plugins/host/plugin-data.service'; import { createTestPluginRegistry } from '../../../src/nest/plugins/host/rpc-kit/testing'; import { DbRpc } from '../../../src/nest/plugins/host/rpc/db.rpc'; import type { PluginUserSettingsService } from '../../../src/nest/plugins/plugin-user-settings.service'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; /** * DbRpc also carries `settings.get`, which needs the per-user settings store. No @@ -53,7 +55,12 @@ function makeSupervisor(events: Array<{ topic: string; data: unknown }>, tuning: onEvent: (_id, topic, data) => events.push({ topic, data }), onLog: (_id, level, msg, meta) => events.push({ topic: '__log', data: { level, msg, meta } }), }; - return new PluginSupervisor(createRpcHost, hooks, tuning); + // task-6-fix-brief.md item 1: every real `ctx.*` round-trip below dispatches + // through PluginSupervisor.onMessage's 'req' branch over real child-process + // IPC, which now THROWS without a resolveOrm thunk rather than dispatching + // unwrapped — so this double needs a real (if otherwise unused) ORM, even + // though PluginDataDb/DbRpc here are raw better-sqlite3, not MikroORM. + return new PluginSupervisor(createRpcHost, hooks, tuning, () => ormHandle.orm); } /** @@ -66,17 +73,23 @@ function logMeta>(events: Array<{ topic: string; dat return (hit && (hit.data as { meta?: unknown }).meta) as T; } -beforeAll(() => { +const ormDb = new Database(':memory:'); +let ormHandle: TestOrm; + +beforeAll(async () => { codeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'trekplug-code-')); dataRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'trekplug-pdata-')); process.env.TREK_PLUGINS_DIR = codeRoot; process.env.TREK_PLUGINS_DATA_DIR = dataRoot; + ormHandle = await createTestOrm(ormDb); }); afterAll(async () => { delete process.env.TREK_PLUGINS_DIR; delete process.env.TREK_PLUGINS_DATA_DIR; fs.rmSync(codeRoot, { recursive: true, force: true }); fs.rmSync(dataRoot, { recursive: true, force: true }); + await ormHandle.close(); + ormDb.close(); }); afterEach(async () => { await sup?.shutdownAll(); diff --git a/server/tests/unit/db/repositories/Addons.repository.test.ts b/server/tests/unit/db/repositories/Addons.repository.test.ts index 2f997f0fd..4c6b05bdb 100644 --- a/server/tests/unit/db/repositories/Addons.repository.test.ts +++ b/server/tests/unit/db/repositories/Addons.repository.test.ts @@ -64,13 +64,6 @@ describe('AddonsRepository', () => { // repeat call from the identity map unless refresh: true is set — // invisible to a write on the same id in the same request. describe('sees a write on the same id in the same request (I1, identity-map regression)', () => { - it('ADDONSREPO-004: setEnabled then isEnabled reads the new value, not the stale one', async () => { - insertAddon({ id: 'budget', name: 'Budget', enabled: 0 }); - expect(await addons.isEnabled('budget')).toBe(false); // populate the identity map - await addons.setEnabled('budget', true); - expect(await addons.isEnabled('budget')).toBe(true); - }); - it('ADDONSREPO-005: a raw UPDATE on the same id then isEnabled reads the new value', async () => { insertAddon({ id: 'budget', name: 'Budget', enabled: 0 }); expect(await addons.isEnabled('budget')).toBe(false); // populate the identity map @@ -116,30 +109,4 @@ describe('AddonsRepository', () => { }); }); }); - - describe('setEnabled', () => { - it('ADDONSREPO-010: stores the boolean as the legacy 0/1', async () => { - insertAddon({ id: 'budget', name: 'Budget', enabled: 0 }); - await addons.setEnabled('budget', true); - expect(rawRow('budget')).toMatchObject({ enabled: 1 }); - - await addons.setEnabled('budget', false); - expect(rawRow('budget')).toMatchObject({ enabled: 0 }); - }); - - it('ADDONSREPO-011: does not touch any other column', async () => { - insertAddon({ id: 'budget', name: 'Costs', description: 'Track spend', type: 'trip', icon: 'wallet', enabled: 0, sort_order: 5 }); - await addons.setEnabled('budget', true); - expect(rawRow('budget')).toEqual({ - id: 'budget', - name: 'Costs', - description: 'Track spend', - type: 'trip', - icon: 'wallet', - enabled: 1, - config: '{}', - sort_order: 5, - }); - }); - }); }); diff --git a/server/tests/unit/nest/airtrail-sync.job.test.ts b/server/tests/unit/nest/airtrail-sync.job.test.ts new file mode 100644 index 000000000..c3746f892 --- /dev/null +++ b/server/tests/unit/nest/airtrail-sync.job.test.ts @@ -0,0 +1,136 @@ +/** + * AirtrailSyncJob (AIRTRAIL-JOB-001..010): the scheduling shell around the + * AirTrail poller, built with `new`, collaborators as plain mocks, no Nest + * container and no timer. Mirrors dawarich-sync.job.test.ts's shape — the + * two jobs are structurally identical siblings with different numbers (floor + * 1 vs 5, default 5 vs 15) and a different setting key. + * + * task-6-review-parity.md C1 / task-6-fix-brief.md item 7: the boot-time + * interval read now goes through CronRegistrarService.runOnBoot instead of + * running inline in onApplicationBootstrap — AIRTRAIL-JOB-009/010 pin that. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const logMock = vi.hoisted(() => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logError: vi.fn(), logWarn: vi.fn(), logDebug: vi.fn() })); +vi.mock('../../../src/nest/audit/audit-log.logger', () => logMock); + +import { AirtrailSyncJob } from '../../../src/nest/integrations/airtrail-sync.job'; +import type { AirtrailSyncService } from '../../../src/nest/integrations/airtrail-sync.service'; +import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { CronRegistrarService } from '../../../src/nest/scheduling/cron-registrar.service'; + +const SETTING_KEY = 'airtrail_poll_interval_minutes'; + +function makeJob(intervalSetting?: string, enabled = true) { + let onTick: (() => void | Promise) | undefined; + const registrar = { + isEnabled: vi.fn(() => enabled), + register: vi.fn((_name: string, _expression: string, cb: () => void | Promise) => { + onTick = cb; + return enabled; + }), + unregister: vi.fn(), + runOnBoot: vi.fn(async (_name: string, fn: () => void | Promise) => { await fn(); }), + }; + const db = { + get: vi.fn((_sql: string, ..._params: unknown[]) => (intervalSetting === undefined ? undefined : { value: intervalSetting })), + }; + const airtrail = { + runAirtrailSync: vi.fn<() => Promise>().mockResolvedValue(undefined), + }; + const job = new AirtrailSyncJob( + db as unknown as DatabaseService, + airtrail as unknown as AirtrailSyncService, + registrar as unknown as CronRegistrarService, + ); + return { job, registrar, db, airtrail, takeTick: () => onTick }; +} + +beforeEach(() => vi.clearAllMocks()); + +describe('AirtrailSyncJob bootstrap', () => { + it('AIRTRAIL-JOB-001: registers */N under the job name the scheduler reports, and logs the banner', async () => { + const { job, registrar } = makeJob('20'); + await job.onApplicationBootstrap(); + expect(registrar.register).toHaveBeenCalledWith('airtrail-sync', '*/20 * * * *', expect.any(Function)); + expect(logMock.logInfo).toHaveBeenCalledWith('AirTrail sync: scheduled every 20m'); + }); + + it('AIRTRAIL-JOB-002: reads its own setting key, not the Dawarich one', async () => { + const { job, db } = makeJob('10'); + await job.onApplicationBootstrap(); + expect(db.get).toHaveBeenCalledWith('SELECT value FROM app_settings WHERE key = ?', SETTING_KEY); + }); + + it('AIRTRAIL-JOB-003: clamps the interval to 1-59 minutes and falls back to 5 on anything else', async () => { + for (const [setting, minutes] of [ + [undefined, 5], + ['', 5], + ['not-a-number', 5], + ['0', 5], + ['60', 5], + ['1', 1], // the floor itself is allowed + ['59', 59], // and so is the ceiling + ] as const) { + vi.clearAllMocks(); + const { job, registrar } = makeJob(setting); + await job.onApplicationBootstrap(); + expect(registrar.register).toHaveBeenCalledWith('airtrail-sync', `*/${minutes} * * * *`, expect.any(Function)); + } + }); + + it('AIRTRAIL-JOB-004: the test gate stops the job before it registers, reads or logs anything', async () => { + const { job, registrar, db } = makeJob('5', false); + await job.onApplicationBootstrap(); + expect(registrar.register).not.toHaveBeenCalled(); + expect(db.get).not.toHaveBeenCalled(); + expect(logMock.logInfo).not.toHaveBeenCalled(); + expect(registrar.runOnBoot).not.toHaveBeenCalled(); + }); + + it('AIRTRAIL-JOB-005: the callback handed to the registrar is the tick, so a fired cron reaches runAirtrailSync', async () => { + const { job, airtrail, takeTick } = makeJob('5'); + await job.onApplicationBootstrap(); + const onTick = takeTick(); + expect(onTick).toBeTypeOf('function'); + await onTick?.(); + expect(airtrail.runAirtrailSync).toHaveBeenCalledTimes(1); + }); + + it('AIRTRAIL-JOB-009: the boot-time interval read goes through CronRegistrarService.runOnBoot (task-6-review-parity.md C1 — the boot-sweep choke point)', async () => { + const { job, registrar } = makeJob('5'); + await job.onApplicationBootstrap(); + expect(registrar.runOnBoot).toHaveBeenCalledWith('airtrail-sync-boot', expect.any(Function)); + }); + + it('AIRTRAIL-JOB-010: registers at the default 5m cadence if runOnBoot declines to run fn (no ORM available)', async () => { + const { job, registrar, db } = makeJob('30'); + registrar.runOnBoot.mockImplementationOnce(async () => { /* simulates no ORM available — fn never runs */ }); + await job.onApplicationBootstrap(); + expect(db.get).not.toHaveBeenCalled(); + expect(registrar.register).toHaveBeenCalledWith('airtrail-sync', '*/5 * * * *', expect.any(Function)); + }); +}); + +describe('AirtrailSyncJob tick', () => { + it('AIRTRAIL-JOB-006: delegates to runAirtrailSync and stays quiet on success', async () => { + const { job, airtrail } = makeJob('5'); + await expect(job.tick()).resolves.toBeUndefined(); + expect(airtrail.runAirtrailSync).toHaveBeenCalledTimes(1); + expect(logMock.logError).not.toHaveBeenCalled(); + }); + + it('AIRTRAIL-JOB-007: an unreachable instance becomes one log line and a resolved promise, never an unhandled rejection in the scheduler', async () => { + const { job, airtrail } = makeJob('5'); + airtrail.runAirtrailSync.mockRejectedValue(new Error('fetch failed')); + await expect(job.tick()).resolves.toBeUndefined(); + expect(logMock.logError).toHaveBeenCalledWith('AirTrail sync tick failed: fetch failed'); + }); + + it('AIRTRAIL-JOB-008: a non-Error rejection is logged by value instead of as an empty message', async () => { + const { job, airtrail } = makeJob('5'); + airtrail.runAirtrailSync.mockRejectedValue('ECONNREFUSED'); + await expect(job.tick()).resolves.toBeUndefined(); + expect(logMock.logError).toHaveBeenCalledWith('AirTrail sync tick failed: ECONNREFUSED'); + }); +}); diff --git a/server/tests/unit/nest/audit.service.test.ts b/server/tests/unit/nest/audit.service.test.ts index 0788e484d..f60ff7f2d 100644 --- a/server/tests/unit/nest/audit.service.test.ts +++ b/server/tests/unit/nest/audit.service.test.ts @@ -19,6 +19,7 @@ vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ })); import type { Request } from 'express'; +import { ValidationError } from '@mikro-orm/core'; import Database from 'better-sqlite3'; import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; @@ -38,6 +39,7 @@ testDb.exec('PRAGMA busy_timeout = 5000'); let t: TestOrm; let auditLogRepo: AuditLogRepository; +let usersRepo: UsersRepository; let svc: AuditService; beforeAll(async () => { @@ -45,7 +47,8 @@ beforeAll(async () => { runMigrations(testDb); t = await createTestOrm(testDb); auditLogRepo = t.repo(AuditLog) as AuditLogRepository; - svc = new AuditService(auditLogRepo, t.repo(Users) as UsersRepository); + usersRepo = t.repo(Users) as UsersRepository; + svc = new AuditService(auditLogRepo, usersRepo); }); beforeEach(() => { @@ -186,6 +189,23 @@ describe('writeAudit', () => { spy.mockRestore(); }); + it('AUDIT-SVC-020: a MikroORM ValidationError from insertEntry still never throws, but is logged with a message distinct from a normal write failure (task-6-fix-brief.md item 4)', async () => { + const spy = vi.spyOn(auditLogRepo, 'insertEntry').mockRejectedValueOnce(ValidationError.cannotUseGlobalContext()); + await expect(svc.writeAudit({ userId: 1, action: 'user.login' })).resolves.not.toThrow(); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('Audit write ran with no request context')); + expect(logError).not.toHaveBeenCalledWith(expect.stringContaining('Audit write failed:')); + spy.mockRestore(); + }); + + it('AUDIT-SVC-021: a MikroORM ValidationError from the email lookup still resolves to uid:, but is logged with a distinct message', async () => { + seedUser(1, 'a@b.c'); + const spy = vi.spyOn(usersRepo, 'getEmail').mockRejectedValueOnce(ValidationError.cannotUseGlobalContext()); + await svc.writeAudit({ userId: 1, action: 'user.login' }); + expect(logInfo).toHaveBeenCalledWith(expect.stringContaining('uid:1 logged in')); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('Audit email lookup ran with no request context')); + spy.mockRestore(); + }); + it('AUDIT-SVC-018: settings.api_keys_update names the changed keys and nothing else (#1939)', async () => { seedUser(1, 'admin@b.c'); await svc.writeAudit({ diff --git a/server/tests/unit/nest/cache-sweep-jobs.test.ts b/server/tests/unit/nest/cache-sweep-jobs.test.ts index 0ca02f93f..ad0d9d9b9 100644 --- a/server/tests/unit/nest/cache-sweep-jobs.test.ts +++ b/server/tests/unit/nest/cache-sweep-jobs.test.ts @@ -26,6 +26,12 @@ function registrarStub(enabled = true) { isEnabled: vi.fn(() => enabled), register: vi.fn(() => enabled), unregister: vi.fn(), + // task-6-fix-brief.md item 7: every boot-time one-off sweep now goes + // through CronRegistrarService.runOnBoot instead of running inline — + // this double just runs fn immediately (synchronously reaching fn's own + // first call, exactly the pre-fix `void this.sweep()` shape), so every + // existing assertion below is unaffected. + runOnBoot: vi.fn(async (_name: string, fn: () => void | Promise) => { await fn(); }), }; } diff --git a/server/tests/unit/nest/cron-registrar.service.test.ts b/server/tests/unit/nest/cron-registrar.service.test.ts index 707a921aa..cec6fc3e2 100644 --- a/server/tests/unit/nest/cron-registrar.service.test.ts +++ b/server/tests/unit/nest/cron-registrar.service.test.ts @@ -33,6 +33,14 @@ vi.mock('cron', () => ({ }, })); +const logErrorMock = vi.hoisted(() => vi.fn()); +vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ + logInfo: vi.fn(), + logDebug: vi.fn(), + logError: logErrorMock, + logWarn: vi.fn(), +})); + import { SchedulerRegistry } from '@nestjs/schedule'; import { CronRegistrarService } from '../../../src/nest/scheduling/cron-registrar.service'; import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; @@ -170,20 +178,17 @@ describe('CronRegistrarService', () => { await t.close(); }); - it('CRONREG-010 — without MikroORM injected, a repository read inside onTick throws cannotUseGlobalContext', async () => { + it('CRONREG-010 — without MikroORM injected, a fired tick throws rather than running the onTick body unwrapped (task-6-fix-brief.md item 1: fail closed at the choke point)', async () => { const registry = new SchedulerRegistry(); const registrar = new CronRegistrarService(registry, { isTest: () => false } as RuntimeEnvService); // no orm arg - let caught: unknown; - registrar.register('job', '0 2 * * *', async () => { - try { - await t.orm.em.find(Users, {}); - } catch (e) { - caught = e; - } + let bodyRan = false; + registrar.register('job', '0 2 * * *', () => { + bodyRan = true; }); - await h.jobs[0].onTick(); - expect(caught).toBeInstanceOf(Error); - expect(String((caught as Error).message)).toMatch(/global (EntityManager|context)/i); + await expect(h.jobs[0].onTick()).rejects.toThrow(/no MikroORM available/i); + // The wrapper throws BEFORE calling onTick at all — the whole point is + // that a repository read inside the body never runs unwrapped. + expect(bodyRan).toBe(false); }); it('CRONREG-011 — with MikroORM injected, the SAME repository read inside onTick succeeds — the wrapper is load-bearing', async () => { @@ -203,4 +208,40 @@ describe('CronRegistrarService', () => { expect(Array.isArray(result)).toBe(true); }); }); + + describe('runOnBoot (task-6-fix-brief.md item 7 — the boot-sweep choke point)', () => { + const testDb = createSnapshotTestDb(); + let t: TestOrm; + + beforeEach(async () => { + t = await createTestOrm(testDb, { allowGlobalContext: false }); + }); + + afterEach(async () => { + await t.close(); + }); + + it('CRONREG-012 — with MikroORM injected, fn runs inside a request context and a repository read succeeds', async () => { + const registry = new SchedulerRegistry(); + const registrar = new CronRegistrarService(registry, { isTest: () => false } as RuntimeEnvService, t.orm); + let result: unknown; + await registrar.runOnBoot('boot-sweep', async () => { + result = await t.orm.em.find(Users, {}); + }); + expect(Array.isArray(result)).toBe(true); + }); + + it('CRONREG-013 — without MikroORM injected, fn never runs and the failure is logged with a distinct message, not swallowed', async () => { + const registry = new SchedulerRegistry(); + const registrar = new CronRegistrarService(registry, { isTest: () => false } as RuntimeEnvService); // no orm + let fnRan = false; + await registrar.runOnBoot('boot-sweep', () => { + fnRan = true; + }); + expect(fnRan).toBe(false); + expect(logErrorMock).toHaveBeenCalledWith( + expect.stringMatching(/runOnBoot: no MikroORM available.*boot-sweep/), + ); + }); + }); }); diff --git a/server/tests/unit/nest/database/unit-of-work.d5.test.ts b/server/tests/unit/nest/database/unit-of-work.d5.test.ts new file mode 100644 index 000000000..658eac91d --- /dev/null +++ b/server/tests/unit/nest/database/unit-of-work.d5.test.ts @@ -0,0 +1,54 @@ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { TransactionContext } from '@mikro-orm/core'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createUser } from '../../../helpers/factories'; +import { Users } from '../../../../src/db/entities/Users.entity'; +import type { UsersRepository } from '../../../../src/db/repositories/Users.repository'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; + +/** + * D5 ratchet (task-6-fix-brief.md item 2; task-1-2-rereview.md's "half that + * IS testable"): the property `test-uow.ts`'s one-ORM-per-handle wiring + * exists to protect is not "the split (two-ORM) wiring was broken" — measured + * and disproven in that re-review, since `TransactionContext.getEntityManager` + * matches on the EntityManager's CONTEXT NAME, not the ORM instance, so even + * a repository built from a second `MikroORM.init` over the same handle + * resolves the open transaction correctly. The property that IS real, and + * WAS previously unpinned by any test, is global-context-resolving EM vs a + * manually forked one: a fork defaults to `useContext: false`, so its + * `getContext()` returns itself and ignores `TransactionContext` — a + * repository built on a fork used inside `uow.transactional(...)` would + * write outside the open transaction, on a second connection the + * transaction is holding. + */ +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let uow: UnitOfWork; + +beforeAll(async () => { + t = await createTestOrm(testDb); + uow = new UnitOfWork(t.em); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +describe('UnitOfWork transaction resolution (D5)', () => { + it('UOW-D5-001: a repository built on the global, context-resolving EM resolves the SAME EntityManager transactional() opens — a manual fork() does not', async () => { + createUser(testDb); + let globalMatches: boolean | undefined; + let forkMatches: boolean | undefined; + + await uow.transactional(async () => { + const repo = t.repo(Users) as UsersRepository; + globalMatches = repo.getEntityManager().getContext() === TransactionContext.getEntityManager(); + + const forkedRepo = t.orm.em.fork().getRepository(Users) as UsersRepository; + forkMatches = forkedRepo.getEntityManager().getContext() === TransactionContext.getEntityManager(); + }); + + expect(globalMatches).toBe(true); + expect(forkMatches).toBe(false); + }); +}); diff --git a/server/tests/unit/nest/dawarich-sync.job.test.ts b/server/tests/unit/nest/dawarich-sync.job.test.ts index d96879010..6bd034bd6 100644 --- a/server/tests/unit/nest/dawarich-sync.job.test.ts +++ b/server/tests/unit/nest/dawarich-sync.job.test.ts @@ -58,6 +58,11 @@ function makeJob(intervalSetting?: string, enabled = true) { return enabled; }), unregister: vi.fn(), + // task-6-fix-brief.md item 7: the boot-time interval read now runs + // through CronRegistrarService.runOnBoot instead of directly inline — + // this double just runs fn immediately, reproducing the pre-fix + // behaviour exactly, so every existing assertion below is unaffected. + runOnBoot: vi.fn(async (_name: string, fn: () => void | Promise) => { await fn(); }), }; // Shaped like the real `get(sql, ...params)` rather than a bare `vi.fn()` // so the stub cannot quietly drift from the signature the job calls; the @@ -153,6 +158,17 @@ describe('DawarichSyncJob bootstrap', () => { expect(registrar.register).toHaveBeenCalledTimes(1); expect(registrar.unregister).not.toHaveBeenCalled(); }); + + it('DAWARICH-JOB-013: the boot-time interval read goes through CronRegistrarService.runOnBoot (task-6-review-parity.md C1 — the boot-sweep choke point), and registers at the default cadence if it declines to run fn', async () => { + const { job, registrar, db } = makeJob('30'); + registrar.runOnBoot.mockImplementationOnce(async () => { /* simulates no ORM available — fn never runs */ }); + await job.onApplicationBootstrap(); + expect(registrar.runOnBoot).toHaveBeenCalledWith('dawarich-sync-boot', expect.any(Function)); + expect(db.get).not.toHaveBeenCalled(); + // minutes keeps its 15-default (the interval read never ran) and the job + // still registers, rather than never scheduling at all. + expect(registrar.register).toHaveBeenCalledWith('dawarich-sync', '*/15 * * * *', expect.any(Function)); + }); }); describe('DawarichSyncJob tick', () => { diff --git a/server/tests/unit/nest/doc-sync/doc-sync.job.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.job.test.ts index 347b098de..c49afcd06 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.job.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.job.test.ts @@ -82,6 +82,11 @@ function makeJob(over: Partial = {}) { return setup.registrarEnabled; }), unregister: vi.fn(), + // task-6-fix-brief.md item 7: the boot-time banner read now runs through + // CronRegistrarService.runOnBoot instead of directly inline — this + // double just runs fn immediately, reproducing the pre-fix behaviour + // exactly, so every existing assertion below is unaffected. + runOnBoot: vi.fn(async (_name: string, fn: () => void | Promise) => { await fn(); }), }; // Both stubs carry the real signatures, so a case can read back which link a @@ -150,6 +155,12 @@ describe('DocSyncJob bootstrap', () => { await job.onApplicationBootstrap(); expect(addons.isAddonEnabled).not.toHaveBeenCalled(); }); + + it('the boot-time interval banner goes through CronRegistrarService.runOnBoot (task-6-review-parity.md C1 — the boot-sweep choke point)', async () => { + const { job, registrar } = makeJob({ interval: '90' }); + await job.onApplicationBootstrap(); + expect(registrar.runOnBoot).toHaveBeenCalledWith('docsync-boot', expect.any(Function)); + }); }); describe('DocSyncJob tick', () => { diff --git a/server/tests/unit/nest/memories/journey-thumbs.job.test.ts b/server/tests/unit/nest/memories/journey-thumbs.job.test.ts new file mode 100644 index 000000000..7d8484f8c --- /dev/null +++ b/server/tests/unit/nest/memories/journey-thumbs.job.test.ts @@ -0,0 +1,83 @@ +/** + * JourneyThumbsJob boot sweep (task-6-review-parity.md C1 — this is the + * exact regression the smoke boot reproduced: `onApplicationBootstrap` used + * to call `void this.sweep()` directly, bypassing CronRegistrarService's + * request-context wrapper entirely, so once `sweepOrphanThumbs` went + * repository-backed (`AddonsService.isAddonEnabled`), the boot-time call + * threw `cannotUseGlobalContext` — silently, swallowed by the job's own + * try/catch — on every production boot). + */ +import { describe, it, expect, vi, beforeAll, afterAll, beforeEach } from 'vitest'; +import { SchedulerRegistry } from '@nestjs/schedule'; + +const logMock = vi.hoisted(() => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logError: vi.fn(), logWarn: vi.fn(), logDebug: vi.fn() })); +vi.mock('../../../../src/nest/audit/audit-log.logger', () => logMock); + +import { CronRegistrarService } from '../../../../src/nest/scheduling/cron-registrar.service'; +import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; +import { JourneyThumbsJob } from '../../../../src/nest/memories/journey-thumbs.job'; +import type { ThumbnailService } from '../../../../src/nest/memories/thumbnail.service'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { Addons } from '../../../../src/db/entities/Addons.entity'; +import type { AddonsRepository } from '../../../../src/db/repositories/Addons.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; + +beforeAll(async () => { + // Global context disallowed — the production setting — so a repository + // read with no wrapper around it genuinely throws. + t = await createTestOrm(testDb, { allowGlobalContext: false }); +}); +afterAll(async () => { await t.close(); testDb.close(); }); +beforeEach(() => vi.clearAllMocks()); + +/** + * A `sweepOrphanThumbs` double that reaches a real `AddonsRepository` read — + * the same repository-backed dependency `ThumbnailService.sweepOrphanThumbs` + * has in production — rather than mocking the property away, so this test + * actually exercises the request-context property the real chain depends on. + */ +function makeThumbnails(): Pick { + const addonsRepo = t.repo(Addons) as AddonsRepository; + return { + sweepOrphanThumbs: async () => { + await addonsRepo.isEnabled('journey'); + return 0; + }, + }; +} + +describe('JourneyThumbsJob boot sweep', () => { + it('JTHUMB-001: onApplicationBootstrap delegates the boot-time sweep through CronRegistrarService.runOnBoot, not a bare void this.sweep()', async () => { + const registered: Array<[string, () => void | Promise]> = []; + const registrar = { + isEnabled: vi.fn(() => true), + register: vi.fn(() => true), + unregister: vi.fn(), + runOnBoot: vi.fn(async (name: string, fn: () => void | Promise) => { registered.push([name, fn]); await fn(); }), + }; + const job = new JourneyThumbsJob(makeThumbnails() as ThumbnailService, registrar as unknown as CronRegistrarService); + job.onApplicationBootstrap(); + await Promise.resolve(); // flush the fire-and-forget void runOnBoot(...) call + expect(registrar.runOnBoot).toHaveBeenCalledWith('journey-thumbs-boot', expect.any(Function)); + }); + + it('JTHUMB-002: with MikroORM wired into the registrar, the boot-time sweep\'s repository read succeeds — no cannotUseGlobalContext', async () => { + const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService, t.orm); + const job = new JourneyThumbsJob(makeThumbnails() as ThumbnailService, registrar); + await registrar.runOnBoot('journey-thumbs-boot', () => job.sweep()); + expect(logMock.logError).not.toHaveBeenCalled(); + }); + + it('JTHUMB-003: without MikroORM, the boot-time sweep never runs at all — the failure is logged with runOnBoot\'s own distinct message, never swallowed into "Journey thumbnail cleanup: ..."', async () => { + const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService); // no orm + let ran = false; + const job = new JourneyThumbsJob({ sweepOrphanThumbs: async () => { ran = true; return 0; } } as ThumbnailService, registrar); + await registrar.runOnBoot('journey-thumbs-boot', () => job.sweep()); + expect(ran).toBe(false); + expect(logMock.logError).toHaveBeenCalledWith(expect.stringMatching(/runOnBoot: no MikroORM available.*journey-thumbs-boot/)); + expect(logMock.logError).not.toHaveBeenCalledWith(expect.stringContaining('Journey thumbnail cleanup')); + }); +}); diff --git a/server/tests/unit/nest/memories/trek-photo-cache.job.test.ts b/server/tests/unit/nest/memories/trek-photo-cache.job.test.ts new file mode 100644 index 000000000..d24f79da4 --- /dev/null +++ b/server/tests/unit/nest/memories/trek-photo-cache.job.test.ts @@ -0,0 +1,67 @@ +/** + * TrekPhotoCacheJob boot sweep (task-6-review-parity.md C1's "scanned for + * siblings" list: sweepExpired is raw SQL today, safe, but the boot-time + * one-off call now goes through CronRegistrarService.runOnBoot at the + * entrypoint anyway, so it stays safe once this dependency graph goes + * repository-backed too). + */ +import { describe, it, expect, vi, beforeAll, afterAll, beforeEach } from 'vitest'; +import { SchedulerRegistry } from '@nestjs/schedule'; + +const logMock = vi.hoisted(() => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logError: vi.fn(), logWarn: vi.fn(), logDebug: vi.fn() })); +vi.mock('../../../../src/nest/audit/audit-log.logger', () => logMock); + +import { CronRegistrarService } from '../../../../src/nest/scheduling/cron-registrar.service'; +import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; +import { TrekPhotoCacheJob } from '../../../../src/nest/memories/trek-photo-cache.job'; +import type { TrekPhotoCacheService } from '../../../../src/nest/memories/trek-photo-cache.service'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; + +beforeAll(async () => { t = await createTestOrm(testDb, { allowGlobalContext: false }); }); +afterAll(async () => { await t.close(); testDb.close(); }); +beforeEach(() => vi.clearAllMocks()); + +describe('TrekPhotoCacheJob boot sweep', () => { + it('TPCACHE-001: onApplicationBootstrap delegates the boot-time sweep through CronRegistrarService.runOnBoot, not a bare inline IIFE', async () => { + const cache = { sweepExpired: vi.fn(async () => {}) } as unknown as TrekPhotoCacheService; + const registrar = { + isEnabled: vi.fn(() => true), + register: vi.fn(() => true), + unregister: vi.fn(), + runOnBoot: vi.fn(async (_name: string, fn: () => void | Promise) => { await fn(); }), + }; + const job = new TrekPhotoCacheJob(cache, registrar as unknown as CronRegistrarService); + job.onApplicationBootstrap(); + await Promise.resolve(); + expect(registrar.runOnBoot).toHaveBeenCalledWith('trek-photo-cache-boot', expect.any(Function)); + expect(cache.sweepExpired).toHaveBeenCalledTimes(1); + }); + + it('TPCACHE-002: a missing cache dir at boot is still swallowed by the job\'s OWN catch, inside runOnBoot\'s wrapper', async () => { + const cache = { sweepExpired: vi.fn(async () => { throw new Error('ENOENT'); }) } as unknown as TrekPhotoCacheService; + const registrar = { + isEnabled: vi.fn(() => true), + register: vi.fn(() => true), + unregister: vi.fn(), + runOnBoot: vi.fn(async (_name: string, fn: () => void | Promise) => { await fn(); }), + }; + const job = new TrekPhotoCacheJob(cache, registrar as unknown as CronRegistrarService); + expect(() => job.onApplicationBootstrap()).not.toThrow(); + await Promise.resolve(); + expect(logMock.logError).not.toHaveBeenCalled(); // the inline try/catch around sweepExpired() still swallows it + }); + + it('TPCACHE-003: without MikroORM, the boot-time sweep never runs — logged with runOnBoot\'s own distinct message', async () => { + const cache = { sweepExpired: vi.fn(async () => {}) } as unknown as TrekPhotoCacheService; + const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService); // no orm + await registrar.runOnBoot('trek-photo-cache-boot', async () => { + try { await cache.sweepExpired(); } catch { /* cache dir may not exist yet — harmless */ } + }); + expect(cache.sweepExpired).not.toHaveBeenCalled(); + expect(logMock.logError).toHaveBeenCalledWith(expect.stringMatching(/runOnBoot: no MikroORM available.*trek-photo-cache-boot/)); + }); +}); diff --git a/server/tests/unit/nest/permissions.service.test.ts b/server/tests/unit/nest/permissions.service.test.ts index 7f1ecb631..6f0dd3d4d 100644 --- a/server/tests/unit/nest/permissions.service.test.ts +++ b/server/tests/unit/nest/permissions.service.test.ts @@ -208,7 +208,9 @@ describe('load failures', () => { // before any request/MCP/WS/cron entrypoint can reach this service, so // that race no longer exists — the message-specific swallow is dropped // and this failure now takes the same "log and serve defaults" path as - // every other repository error. + // every other repository error. (task-6-review-template.md Minor 4: this + // case does not itself assert getPermissionsCache() stays null for the + // same branch — PERM-SVC-024 below does.) const spy = vi .spyOn(appSettings, 'findByKeyPrefix') .mockRejectedValueOnce(new Error('no such table: app_settings')); diff --git a/server/tests/unit/nest/place-photos/place-photo-cache.job.test.ts b/server/tests/unit/nest/place-photos/place-photo-cache.job.test.ts new file mode 100644 index 000000000..98de703dc --- /dev/null +++ b/server/tests/unit/nest/place-photos/place-photo-cache.job.test.ts @@ -0,0 +1,61 @@ +/** + * PlacePhotoCacheJob boot sweep (task-6-review-parity.md C1's "scanned for + * siblings" list: sweepOrphans is raw SQL today, safe, but the boot-time + * one-off call now goes through CronRegistrarService.runOnBoot at the + * entrypoint anyway, so it stays safe once this dependency graph goes + * repository-backed too, without a second review having to rediscover it). + */ +import { describe, it, expect, vi, beforeAll, afterAll, beforeEach } from 'vitest'; +import { SchedulerRegistry } from '@nestjs/schedule'; + +const logMock = vi.hoisted(() => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logError: vi.fn(), logWarn: vi.fn(), logDebug: vi.fn() })); +vi.mock('../../../../src/nest/audit/audit-log.logger', () => logMock); + +import { CronRegistrarService } from '../../../../src/nest/scheduling/cron-registrar.service'; +import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; +import { PlacePhotoCacheJob } from '../../../../src/nest/place-photos/place-photo-cache.job'; +import type { PlacePhotoCacheService } from '../../../../src/nest/place-photos/place-photo-cache.service'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; + +beforeAll(async () => { t = await createTestOrm(testDb, { allowGlobalContext: false }); }); +afterAll(async () => { await t.close(); testDb.close(); }); +beforeEach(() => vi.clearAllMocks()); + +describe('PlacePhotoCacheJob boot sweep', () => { + it('PPCACHE-001: onApplicationBootstrap delegates the boot-time sweep through CronRegistrarService.runOnBoot, not a bare void this.sweep()', async () => { + const cache = { sweepOrphans: vi.fn(async () => 0) } as unknown as PlacePhotoCacheService; + const registrar = { + isEnabled: vi.fn(() => true), + register: vi.fn(() => true), + unregister: vi.fn(), + runOnBoot: vi.fn(async (_name: string, fn: () => void | Promise) => { await fn(); }), + }; + const job = new PlacePhotoCacheJob(cache, registrar as unknown as CronRegistrarService); + job.onApplicationBootstrap(); + await Promise.resolve(); + expect(registrar.runOnBoot).toHaveBeenCalledWith('place-photo-cache-boot', expect.any(Function)); + }); + + it('PPCACHE-002: with MikroORM wired into the registrar, the boot-time sweep runs and completes', async () => { + const cache = { sweepOrphans: vi.fn(async () => 3) } as unknown as PlacePhotoCacheService; + const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService, t.orm); + const job = new PlacePhotoCacheJob(cache, registrar); + await registrar.runOnBoot('place-photo-cache-boot', () => job.sweep()); + expect(cache.sweepOrphans).toHaveBeenCalledTimes(1); + expect(logMock.logError).not.toHaveBeenCalled(); + }); + + it('PPCACHE-003: without MikroORM, the boot-time sweep never runs — logged with runOnBoot\'s own distinct message, not swallowed into "Place-photo cache cleanup: ..."', async () => { + const cache = { sweepOrphans: vi.fn(async () => 0) } as unknown as PlacePhotoCacheService; + const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService); // no orm + const job = new PlacePhotoCacheJob(cache, registrar); + await registrar.runOnBoot('place-photo-cache-boot', () => job.sweep()); + expect(cache.sweepOrphans).not.toHaveBeenCalled(); + expect(logMock.logError).toHaveBeenCalledWith(expect.stringMatching(/runOnBoot: no MikroORM available.*place-photo-cache-boot/)); + expect(logMock.logError).not.toHaveBeenCalledWith(expect.stringContaining('Place-photo cache cleanup')); + }); +}); diff --git a/server/tests/unit/nest/reminder-jobs.service.test.ts b/server/tests/unit/nest/reminder-jobs.service.test.ts index 8481dd3a8..6f9616167 100644 --- a/server/tests/unit/nest/reminder-jobs.service.test.ts +++ b/server/tests/unit/nest/reminder-jobs.service.test.ts @@ -54,6 +54,11 @@ function makeJobs(overrides: { notifications?: NotificationsService } = {}) { return true; }), unregister: vi.fn(), + // task-6-fix-brief.md item 7: the boot-time banner block now runs through + // CronRegistrarService.runOnBoot instead of directly inline — this + // double just runs fn immediately, reproducing the pre-fix behaviour + // exactly, so every existing assertion below is unaffected. + runOnBoot: vi.fn(async (_name: string, fn: () => void | Promise) => { await fn(); }), }; const send = vi.fn().mockResolvedValue(undefined); const svc = new ReminderJobsService( @@ -105,6 +110,12 @@ describe('ReminderJobsService bootstrap', () => { expect(logMock.logInfo).not.toHaveBeenCalled(); }); + it('RJOB-002b — the boot banner block goes through CronRegistrarService.runOnBoot (task-6-review-parity.md C1 — the boot-sweep choke point)', async () => { + const { svc, registrar } = makeJobs(); + await svc.onApplicationBootstrap(); + expect(registrar.runOnBoot).toHaveBeenCalledWith('reminder-jobs-boot', expect.any(Function)); + }); + it('RJOB-003 — logs the enabled banners by default and the disabled ones when toggled off', async () => { const { svc } = makeJobs(); await svc.onApplicationBootstrap(); diff --git a/server/tests/unit/nest/trek-ws-adapter.test.ts b/server/tests/unit/nest/trek-ws-adapter.test.ts index f91097107..8f9297ece 100644 --- a/server/tests/unit/nest/trek-ws-adapter.test.ts +++ b/server/tests/unit/nest/trek-ws-adapter.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach, afterAll } from 'vitest'; +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; /** * TrekWsAdapter: the wire protocol and the per-socket flood guard. @@ -35,11 +35,28 @@ import { TrekWsAdapter } from '../../../src/nest/realtime/trek-ws.adapter'; import { getServer } from '../../../src/nest/realtime/ws-state'; import type { Server as HttpServer } from 'node:http'; import { createSnapshotTestDb } from '../../helpers/db-mock'; -import { createTestOrm } from '../../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { Users } from '../../../src/db/entities/Users.entity'; const testDb = createSnapshotTestDb(); -afterAll(() => testDb.close()); + +// task-6-fix-brief.md item 1: bindMessageHandlers now THROWS without an `orm` +// rather than dispatching a matched handler unwrapped (no more silent +// degrade), so every generic-behaviour test below — none of which cares +// about the D6 request-context property itself, only that a message reaches +// its handler — needs the shared adapter to carry a real (if otherwise +// unexercised) ORM. WSAD-040/041 build their OWN local adapter instances +// specifically to test the with/without-orm property and are unaffected. +let adapterOrm: TestOrm; +let adapter: TrekWsAdapter; +beforeAll(async () => { + adapterOrm = await createTestOrm(testDb); + adapter = new TrekWsAdapter({} as HttpServer, adapterOrm.orm); +}); +afterAll(async () => { + await adapterOrm.close(); + testDb.close(); +}); type MessageListener = (buffer: Buffer) => void; @@ -61,7 +78,6 @@ function fakeSocket() { }; } -const adapter = new TrekWsAdapter({} as HttpServer); const frame = (o: unknown) => Buffer.from(JSON.stringify(o)); /** The adapter hands results to Nest's transform; here it is identity. */ const transform = (v: unknown) => ({ subscribe: (o: { next: (x: unknown) => void }) => o.next(v) }) as never; @@ -126,31 +142,19 @@ describe('TrekWsAdapter D6 request context (task-2-review.md C3 ruling)', () => // shape: dispatched from bindMessageHandlers below, not from an Express // request, so nothing has forked an EntityManager for it unless the ONE // wrapper there (not per-handler) does it. - it('WSAD-040: without MikroORM passed to the adapter, a repository read inside a handler throws cannotUseGlobalContext', async () => { - const t = await createTestOrm(testDb, { allowGlobalContext: false }); - try { - let caught: unknown; - const ad = new TrekWsAdapter({} as HttpServer); // no orm - const socket = fakeSocket(); - let captured: Promise | undefined; - const capture = (v: unknown) => { - captured = v instanceof Promise ? v : Promise.resolve(v); - return { subscribe: () => {} } as never; - }; - ad.bindMessageHandlers( - socket as never, - [{ message: 'join', callback: async () => { - try { await t.orm.em.find(Users, {}); } catch (e) { caught = e; } - } }] as never, - capture, - ); - socket.emit('message', frame({ type: 'join', tripId: 1 })); - await captured; - expect(caught).toBeInstanceOf(Error); - expect(String((caught as Error).message)).toMatch(/global (EntityManager|context)/i); - } finally { - await t.close(); - } + it('WSAD-040: without MikroORM passed to the adapter, dispatch THROWS rather than running the handler unwrapped (task-6-fix-brief.md item 1: fail closed at the choke point, not just downstream)', () => { + const ad = new TrekWsAdapter({} as HttpServer); // no orm + const socket = fakeSocket(); + let handlerRan = false; + ad.bindMessageHandlers( + socket as never, + [{ message: 'join', callback: async () => { handlerRan = true; } }] as never, + () => ({ subscribe: () => {} }) as never, + ); + expect(() => socket.emit('message', frame({ type: 'join', tripId: 1 }))).toThrow(/no MikroORM available/i); + // The wrapper throws BEFORE calling the handler at all — the handler's + // own repository read never even ran unwrapped. + expect(handlerRan).toBe(false); }); it('WSAD-041: with MikroORM passed to the adapter, the SAME repository read inside a handler succeeds — the wrapper is load-bearing', async () => { diff --git a/server/tests/unit/plugins/request-context.test.ts b/server/tests/unit/plugins/request-context.test.ts index 128f15338..f07b4c815 100644 --- a/server/tests/unit/plugins/request-context.test.ts +++ b/server/tests/unit/plugins/request-context.test.ts @@ -12,10 +12,16 @@ * from its 'everybody' default to 'admin', a role-'user' actor's check must * come back `false`. Before the fix (dispatch outside any context, and * loadPermissions serving defaults on any error) it came back `true` — a - * fail-open security regression. After BOTH this round's fixes — the wrapper - * here, and `loadPermissions` rethrowing a MikroORM `ValidationError` instead - * of degrading (permissions.service.ts) — an unwrapped dispatch fails CLOSED - * (a HOST_ERROR envelope), never silently `true`. + * fail-open security regression. Task 2's round fixed it two switches away + * from the choke point — the wrapper here, and `loadPermissions` rethrowing a + * MikroORM `ValidationError` instead of degrading (permissions.service.ts) — + * so an unwrapped dispatch failed CLOSED (a HOST_ERROR envelope) but only + * because BOTH switches held. task-6-fix-brief.md item 1 (task-6-review- + * template.md's Important 1 / M-B) moves the fail-closed decision TO the + * choke point itself: an absent `resolveOrm` now makes the dispatch THROW + * before it ever reaches `rpcHost.dispatch`, so the child gets no response at + * all — not even the old HOST_ERROR envelope — and the security property no + * longer depends on `loadPermissions`' rethrow holding too. * * Global context is disallowed on purpose (`{ allowGlobalContext: false }`), * like `tests/unit/nest/database/request-context.test.ts` — the production @@ -106,16 +112,16 @@ function makeSupervisor(resolveOrm?: () => { em: EntityManager } | undefined) { } describe('PluginSupervisor request context (D6, C3)', () => { - it('CTX-PLUGIN-001: unwrapped, the RPC path fails CLOSED on a cold cache — never the old fail-open `true`', async () => { + it('CTX-PLUGIN-001: without a resolveOrm thunk, the dispatch THROWS rather than running unwrapped (task-6-fix-brief.md item 1: fail closed at the choke point, not just downstream)', async () => { permissions.invalidatePermissionsCache(); const { supervisor, sup, sent } = makeSupervisor(); // no resolveOrm — the bug's exact reproduction - await supervisor.onMessage(sup, { k: 'req', id: 'r1', method: 'trips.create', params: { _inv: 'inv-1' } }); - - expect(sent).toHaveLength(1); - const res = sent[0]; - expect(res.ok).toBe(false); - if (res.ok === false) expect(res.error.message).toMatch(/global (EntityManager|context)/i); + await expect( + supervisor.onMessage(sup, { k: 'req', id: 'r1', method: 'trips.create', params: { _inv: 'inv-1' } }), + ).rejects.toThrow(/no ORM available/i); + // The dispatch never ran, so the child never got a response at all — + // not even the old fail-closed HOST_ERROR envelope. + expect(sent).toHaveLength(0); }); it('CTX-PLUGIN-002: wrapped in withRequestContext, the same dispatch enforces the stored override — a "user" role is refused', async () => { diff --git a/server/tests/unit/plugins/supervisor-lifecycle.test.ts b/server/tests/unit/plugins/supervisor-lifecycle.test.ts index 7681aa737..234a4e530 100644 --- a/server/tests/unit/plugins/supervisor-lifecycle.test.ts +++ b/server/tests/unit/plugins/supervisor-lifecycle.test.ts @@ -4,14 +4,25 @@ * an admin depends on: re-activating a plugin that died stays possible, and a * crash-restart cycle doesn't leak the dead child's cron tasks. */ -import { describe, it, expect, vi, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeAll, afterAll, afterEach } from 'vitest'; +import Database from 'better-sqlite3'; import { PluginSupervisor } from '../../../src/nest/plugins/supervisor/plugin-supervisor'; import { RpcRateLimiter, TokenBucket } from '../../../src/nest/plugins/host/rate-limit'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; + +// task-6-fix-brief.md item 1: onMessage's 'req' branch now THROWS without a +// resolveOrm thunk (no more silent unwrapped dispatch), so the one case below +// that dispatches a real 'req' message ("refuses a throttled call...") needs a +// real ORM even though nothing it exercises reaches a repository. +const ormDb = new Database(':memory:'); +let ormHandle: TestOrm; +beforeAll(async () => { ormHandle = await createTestOrm(ormDb); }); +afterAll(async () => { await ormHandle.close(); ormDb.close(); }); function makeSupervisor() { const dispose = vi.fn(); // eslint-disable-next-line @typescript-eslint/no-explicit-any - const s = new PluginSupervisor((() => ({ dispose })) as any, {}, {}); + const s = new PluginSupervisor((() => ({ dispose })) as any, {}, {}, () => ormHandle.orm); const spawn = vi.fn(); // eslint-disable-next-line @typescript-eslint/no-explicit-any (s as any).spawn = spawn;