diff --git a/server/src/nest/airports/airports.module.ts b/server/src/nest/airports/airports.module.ts index 91e8175ba..a8aaeb8a4 100644 --- a/server/src/nest/airports/airports.module.ts +++ b/server/src/nest/airports/airports.module.ts @@ -2,14 +2,18 @@ import { Module } from '@nestjs/common'; import { AirportsController } from './airports.controller'; import { AirportsService } from './airports.service'; import { DatabaseModule } from '../database/database.module'; +import { SchedulingModule } from '../scheduling/scheduling.module'; import { AirportsMcp } from './airports.mcp'; /** Airports domain (L2 leaf module). Registered in AppModule. * DatabaseModule is imported explicitly: the flight-endpoint backfill needs the * connection, and @Global only reaches modules that are in the graph — which - * the airports e2e TestingModule is not. */ + * the airports e2e TestingModule is not. SchedulingModule (task-6-rereview.md + * M1) is imported for the same reason — it is deliberately NOT @Global — so + * the boot backfill can route through CronRegistrarService.runOnBoot instead + * of running outside a request context. */ @Module({ - imports: [DatabaseModule], + imports: [DatabaseModule, SchedulingModule], controllers: [AirportsController], providers: [AirportsService, AirportsMcp], }) diff --git a/server/src/nest/airports/airports.service.ts b/server/src/nest/airports/airports.service.ts index e580fc583..a5f0f4347 100644 --- a/server/src/nest/airports/airports.service.ts +++ b/server/src/nest/airports/airports.service.ts @@ -2,6 +2,7 @@ import { Injectable, OnApplicationBootstrap } from '@nestjs/common'; import type { Airport } from '@trek/shared'; import { searchAirports, findByIata, load } from './airports.data'; import { DatabaseService } from '../database/database.service'; +import { CronRegistrarService } from '../scheduling/cron-registrar.service'; /** * The in-container face of the airport dataset, plus the flight-endpoint @@ -11,17 +12,30 @@ import { DatabaseService } from '../database/database.service'; * require() back into services/ — a cycle that only held because `db` happened * to be initialised by then. It runs on application bootstrap now: it repairs * rows, nothing boots on it, and the container is fully built by that point. + * + * task-6-rereview.md M1: raw SQL only today, but this is the same boot-sweep + * shape C1 found repository-backed elsewhere, so it goes through the one + * choke point (`CronRegistrarService.runOnBoot`) every other one-off boot + * sweep uses now, rather than running outside a request context directly. + * `runOnBoot` has no `isEnabled()` gate (unlike `register()`), so — parity + * with the pre-existing behavior — the backfill still runs in every test + * harness that boots this service with a MikroORM, exactly as it did before. */ @Injectable() export class AirportsService implements OnApplicationBootstrap { - constructor(private readonly db: DatabaseService) {} + constructor( + private readonly db: DatabaseService, + private readonly registrar: CronRegistrarService, + ) {} async onApplicationBootstrap(): Promise { - try { - await this.backfillFlightEndpoints(); - } catch (err) { - console.error('[DB] Flight endpoint backfill failed:', err); - } + await this.registrar.runOnBoot('airports-flight-endpoints-boot', async () => { + try { + await this.backfillFlightEndpoints(); + } catch (err) { + console.error('[DB] Flight endpoint backfill failed:', err); + } + }); } search(query: string): Airport[] { diff --git a/server/src/nest/plugins/plugin-runtime.service.ts b/server/src/nest/plugins/plugin-runtime.service.ts index 97f1b1fb5..53e6f915f 100644 --- a/server/src/nest/plugins/plugin-runtime.service.ts +++ b/server/src/nest/plugins/plugin-runtime.service.ts @@ -3,6 +3,8 @@ import semver from 'semver'; import { MikroORM } from '@mikro-orm/core'; import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; +import { withRequestContext } from '../database/request-context'; +import { logError } from '../audit/audit-log.logger'; import { pluginsEnabled } from './kill-switch'; import { setPluginEventSink } from '../../plugin-event-sink'; import { setUserDeletedSink } from '../../plugin-user-lifecycle'; @@ -186,11 +188,14 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes private readonly userSettings: PluginUserSettingsService, private readonly registry?: PluginRegistryService, private readonly hostFactory?: PluginRpcHostFactory, - // LAST, and optional only because TypeScript forbids a required parameter after - // an optional one — Nest always injects it (OrmModule is global). A hand-built + // Optional only because TypeScript forbids a required parameter after an + // optional one — Nest always injects it (OrmModule is global). A hand-built // test instance that exercises a transaction must pass one; setOperatorEgressHosts // refuses rather than writing outside a transaction (same shape as hostFactory). - private readonly uow?: UnitOfWork, + // `@Optional()` (task-6-rereview.md M2 — harmonised with the `orm?` param two + // lines below, which got one in the previous fix wave for exactly this reason; + // a plain `?` is TS-only and does not tell Nest's DI a provider may be absent). + @Optional() private readonly uow?: UnitOfWork, // D6 (task-2-review.md C3): the supervisor wraps every plugin RPC dispatch in a // request context built from this. Nest always injects it (MikroOrmCoreModule is // global); a hand-built test instance that dispatches through a real @@ -264,15 +269,45 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // own gate still refuses to spawn, so nothing boots into a broken state. order = enabledIds; } - for (const id of order) { - this.activate(id).catch((e) => { - // A plugin whose required addon is off (or a dependency is missing) at boot - // must not stay marked enabled — reconcile the row so the UI reflects reality. - if (e instanceof PluginDependencyError || e instanceof DependencyCycleError) { - this.deactivate(id).catch(() => {}); - } - /* other failures: status is persisted as error by the supervisor hook */ - }); + // task-6-rereview.md I1: assertActivatable (inside activate) reads + // AddonsService.isAddonEnabled — repository-backed — so this boot loop needs + // the same request-context wrap every other D6 choke point got, or an + // enabled plugin with a non-empty requiredAddons throws cannotUseGlobalContext + // here, silently: the .catch below only reconciles PluginDependencyError / + // DependencyCycleError, and the outer try/catch around this whole block eats + // everything else. + // `activate()` itself stays unwrapped (its HTTP callers already run inside a + // request); only this boot-time call site needs it. Fail closed the same way + // the boot RULING (task-6-rereview.md §2) settled for CronRegistrarService. + // runOnBoot: no ORM means skip loudly and never call activate — never abort + // app.init() over a partially-wired test graph. + // task-6-rereview.md I1: assertActivatable (inside activate) reads + // AddonsService.isAddonEnabled — repository-backed — so this boot loop needs + // the same request-context wrap every other D6 choke point got, or an + // enabled plugin with a non-empty requiredAddons throws cannotUseGlobalContext + // here, silently: the .catch below only reconciles PluginDependencyError / + // DependencyCycleError, and the outer try/catch around this whole block eats + // everything else. + // `activate()` itself stays unwrapped (its HTTP callers already run inside a + // request); only this boot-time call site needs it. Fail closed the same way + // the boot RULING (task-6-rereview.md §2) settled for CronRegistrarService. + // runOnBoot: no ORM means skip loudly and never call activate — never abort + // app.init() over a partially-wired test graph. + if (!this.orm) { + if (order.length) { + logError(`PluginRuntimeService.onApplicationBootstrap: no MikroORM available — plugin(s) not activated: ${order.join(', ')}`); + } + } else { + for (const id of order) { + withRequestContext(this.orm, () => this.activate(id)).catch((e) => { + // A plugin whose required addon is off (or a dependency is missing) at boot + // must not stay marked enabled — reconcile the row so the UI reflects reality. + if (e instanceof PluginDependencyError || e instanceof DependencyCycleError) { + this.deactivate(id).catch(() => {}); + } + /* other failures: status is persisted as error by the supervisor hook */ + }); + } } } catch { /* discovery/boot must never block app init */ diff --git a/server/src/nest/plugins/supervisor/plugin-supervisor.ts b/server/src/nest/plugins/supervisor/plugin-supervisor.ts index 6f510ec04..882e039fd 100644 --- a/server/src/nest/plugins/supervisor/plugin-supervisor.ts +++ b/server/src/nest/plugins/supervisor/plugin-supervisor.ts @@ -585,6 +585,16 @@ export class PluginSupervisor { // that dispatch through this path now pass a thunk (`sharedTestOrm`). const orm = this.resolveOrm?.(); if (!orm) { + // task-6-rereview.md §5 RULING: the child's `pending` map + // (plugin-host-entry.ts) has no timeout, so leaving this 'req' + // unanswered hangs the plugin's ctx.* promise forever — and + // handleChildMessage above downgrades the throw below to one + // plugin-scoped log line, never surfacing it as a host-wiring + // failure. Answer the child first, same shape as the rate-limiter + // refusal a few lines up, THEN throw for host-side visibility. The + // dispatch still never runs either way — this only changes whether + // the caller ever hears back. + sup.child?.send({ k: 'res', id: req.id, ok: false, error: { code: 'HOST_ERROR', message: 'no ORM available to build a request context' } } satisfies RpcError); 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)); diff --git a/server/tests/integration/boot-sweeps-request-context.test.ts b/server/tests/integration/boot-sweeps-request-context.test.ts index 786b1b852..3de62faca 100644 --- a/server/tests/integration/boot-sweeps-request-context.test.ts +++ b/server/tests/integration/boot-sweeps-request-context.test.ts @@ -1,6 +1,7 @@ /** * Boot-sweep request-context regression guard (task-6-review-parity.md - * Critical C1; task-6-fix-brief.md item 7). + * Critical C1; task-6-fix-brief.md item 7; rewritten per task-6-rereview.md + * I2). * * `journey-thumbs.job.ts`'s `onApplicationBootstrap` used to call * `void this.sweep()` directly, bypassing `CronRegistrarService`'s @@ -13,17 +14,31 @@ * 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. + * I2's finding: the first version of this file never actually invoked any + * job's `onApplicationBootstrap` — it hand-called + * `registrar.runOnBoot('journey-thumbs-boot', () => job.sweep())` for each + * job, re-implementing the very routing under test. Reverting + * `journey-thumbs.job.ts`'s boot call back to the bare `void this.sweep()` — + * C1 verbatim — left that version green, because the mutation only changes + * what `onApplicationBootstrap` does, and nothing here ever called it. + * + * This version drives the seven jobs for real: `CronRegistrarService`'s + * `isEnabled()` (false under `NODE_ENV=test`, `SCHED-GATE`) is forced true so + * each job's own `onApplicationBootstrap` doesn't return before reaching its + * boot sweep, `register()` is stubbed to a no-op so no timer is armed (its + * production behaviour is CRONREG's own suite, not this file's job), and + * `runOnBoot` is wrapped with a spy that calls straight through to the real, + * production-wired implementation (`orm` is `app.get(MikroORM)` — pinned by + * orm-request-context-seams.test.ts) while collecting every promise it + * returns. Three of the seven jobs (`JourneyThumbsJob`, `PlacePhotoCacheJob`, + * `TrekPhotoCacheJob`) call `runOnBoot` fire-and-forget + * (`void this.registrar.runOnBoot(...)`) rather than awaiting it, so a + * mutation that routes one of them back to a bare `void this.sweep()` — i.e. + * bypassing `runOnBoot` (and this spy) altogether — produces a promise this + * file never captures either; the extra macrotask ticks after + * `Promise.all(collected)` give that unawaited call a chance to run (and log) + * before the assertion, so the regression still fails the test even though + * its promise was never in `collected`. */ import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import type { INestApplication } from '@nestjs/common'; @@ -66,21 +81,39 @@ describe('Every onApplicationBootstrap boot sweep runs inside a request context' 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 () => { + it('BOOT-SWEEP-001: the seven jobs\' REAL onApplicationBootstrap, driven through the real production-wired CronRegistrarService, never logs cannotUseGlobalContext / "global EntityManager"', async () => { const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const registrar = app.get(CronRegistrarService); + const isEnabledSpy = vi.spyOn(registrar, 'isEnabled').mockReturnValue(true); + // No-op: this file is about what a fired boot sweep does, not about + // arming a real cron timer on a shared registry (CRONREG's own suite + // covers register() itself). + const registerSpy = vi.spyOn(registrar, 'register').mockImplementation(() => true); + const originalRunOnBoot = registrar.runOnBoot.bind(registrar); + const collected: Promise[] = []; + const runOnBootSpy = vi.spyOn(registrar, 'runOnBoot').mockImplementation((name, fn) => { + const p = originalRunOnBoot(name, fn); + collected.push(p); + return p; + }); try { - const registrar = app.get(CronRegistrarService); + // Real onApplicationBootstrap calls, not hand-called runOnBoot — the whole + // point of I2's fix. Three are fire-and-forget (void return type); four are + // awaited because the job's own onApplicationBootstrap awaits runOnBoot. + app.get(JourneyThumbsJob).onApplicationBootstrap(); + app.get(PlacePhotoCacheJob).onApplicationBootstrap(); + app.get(TrekPhotoCacheJob).onApplicationBootstrap(); + await app.get(ReminderJobsService).onApplicationBootstrap(); + await app.get(DocSyncJob).onApplicationBootstrap(); + await app.get(AirtrailSyncJob).onApplicationBootstrap(); + await app.get(DawarichSyncJob).onApplicationBootstrap(); - 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()); + await Promise.all(collected); + // Settle any pending microtasks/macrotasks a fire-and-forget boot call left + // behind (see the file docstring: this is what still catches a job routed + // back to a bare `void this.sweep()`, whose promise never lands in `collected`). + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); const suspicious = errSpy.mock.calls .map((args) => args.map(String).join(' ')) @@ -88,6 +121,9 @@ describe('Every onApplicationBootstrap boot sweep runs inside a request context' expect(suspicious).toEqual([]); } finally { errSpy.mockRestore(); + isEnabledSpy.mockRestore(); + registerSpy.mockRestore(); + runOnBootSpy.mockRestore(); } }); }); diff --git a/server/tests/integration/bootstrap.test.ts b/server/tests/integration/bootstrap.test.ts index ff093b8b4..3849d347e 100644 --- a/server/tests/integration/bootstrap.test.ts +++ b/server/tests/integration/bootstrap.test.ts @@ -28,10 +28,18 @@ vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi. // registrar inert so a bootstrap test never schedules real jobs or runs boot // sweeps against the real uploads/data dirs. The gate itself is covered by // tests/integration/scheduler-gate.test.ts. +// +// runOnBoot (task-6-rereview.md M1): AirportsService's boot backfill has no +// isEnabled() gate of its own (unlike the seven job providers above, which +// all check isEnabled() before ever reaching runOnBoot, so they never call +// this mock's method at all) — it calls runOnBoot() unconditionally, so this +// double needs one too. A no-op, not a pass-through: "the registrar inert" +// above means no boot sweep runs in this harness, full stop. vi.mock('../../src/nest/scheduling/cron-registrar.service', () => ({ CronRegistrarService: class { isEnabled() { return false; } register() { return false; } + async runOnBoot() { /* inert — see comment above */ } unregister() {} get jobCount() { return 0; } onApplicationShutdown() {} diff --git a/server/tests/integration/orm-request-context-seams.test.ts b/server/tests/integration/orm-request-context-seams.test.ts index f54123829..2dbe5e905 100644 --- a/server/tests/integration/orm-request-context-seams.test.ts +++ b/server/tests/integration/orm-request-context-seams.test.ts @@ -9,10 +9,25 @@ * 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. + * params, `PluginSupervisor`'s `resolveOrm` thunk AND `TrekWsAdapter`'s `orm` + * constructor argument with the SAME MikroORM instance `app.get(MikroORM)` + * resolves — so a future refactor that quietly drops one of those FOUR + * constructor arguments fails here, not silently in production. + * + * task-6-rereview.md I3: the fourth seam (`TrekWsAdapter`) had no ratchet at + * all — `bootstrap.ts`'s `new TrekWsAdapter(boundHttpServer, orm)` could lose + * its second argument and only WSAD-040 (which builds its own adapter, not + * production's) would still pass, while every real WS frame would start + * throwing synchronously inside a `ws` `'message'` listener — an + * uncaughtException with no host-side net, i.e. process death, not one failed + * request. `TrekWsAdapter`'s `orm` is a plain constructor param on a class + * `bootstrap.ts` builds with `new` (never through Nest DI), so it is reached + * the same way this file already reaches `PluginSupervisor.resolveOrm()` — + * through the concrete object the app graph is actually holding, here + * `NestApplication`'s own `private readonly config: ApplicationConfig` + * (`app.useWebSocketAdapter` calls `this.config.setIoAdapter(adapter)`; see + * `@nestjs/core`'s `nest-application.js`/`.d.ts` — the field is `config`, NOT + * `applicationConfig`, despite the class's own name). */ import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import type { INestApplication } from '@nestjs/common'; @@ -50,7 +65,7 @@ describe('ORM request-context seams populated in production', () => { testDb.close(); }); - it('SEAM-001: CronRegistrarService, PluginRuntimeService and PluginSupervisor.resolveOrm() all resolve the SAME MikroORM app.get(MikroORM) does', () => { + it('SEAM-001: CronRegistrarService, PluginRuntimeService, PluginSupervisor.resolveOrm() and TrekWsAdapter all resolve the SAME MikroORM app.get(MikroORM) does', () => { const orm = app.get(MikroORM); const cronRegistrar = app.get(CronRegistrarService) as unknown as { orm?: MikroORM }; @@ -62,5 +77,9 @@ describe('ORM request-context seams populated in production', () => { }; expect(runtime.orm).toBe(orm); expect(runtime.supervisor.resolveOrm?.()).toBe(orm); + + // task-6-rereview.md I3 — the fourth seam, with no ratchet before this test. + const wsAdapter = (app as unknown as { config: { getIoAdapter(): { orm?: MikroORM } } }).config.getIoAdapter(); + expect(wsAdapter.orm).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 d42a0332a..a86dff301 100644 --- a/server/tests/integration/plugins/boot-registry-order.test.ts +++ b/server/tests/integration/plugins/boot-registry-order.test.ts @@ -53,6 +53,15 @@ import { db as dbConn } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { AuditService } from '../../../src/nest/audit/audit.service'; import { createTestAddonsService } from '../../helpers/test-addons'; +import { AddonsService } from '../../../src/nest/addons/addons.service'; +import { Addons } from '../../../src/db/entities/Addons.entity'; +import type { AddonsRepository } from '../../../src/db/repositories/Addons.repository'; +import { PhotoProviders } from '../../../src/db/entities/PhotoProviders.entity'; +import type { PhotoProvidersRepository } from '../../../src/db/repositories/PhotoProviders.repository'; +import { PhotoProviderFields } from '../../../src/db/entities/PhotoProviderFields.entity'; +import type { PhotoProviderFieldsRepository } from '../../../src/db/repositories/PhotoProviderFields.repository'; +import { AppSettings } from '../../../src/db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; import { PluginRuntimeService } from '../../../src/nest/plugins/plugin-runtime.service'; import { PluginUserSettingsService } from '../../../src/nest/plugins/plugin-user-settings.service'; import { PluginRpcHostFactory } from '../../../src/nest/plugins/host/plugin-rpc-host.factory'; @@ -155,4 +164,94 @@ describe('plugin boot vs registry scan ordering', () => { expect(row.status).toBe('active'); expect(runtime.isActive('migrator')).toBe(true); }); + + /** + * task-6-rereview.md I1: `onApplicationBootstrap`'s boot-activation loop + * (`this.activate(id)` for every enabled plugin) reaches `assertActivatable` + * → `disabledRequiredAddons` → `AddonsService.isAddonEnabled` — + * repository-backed — with no request context wrapped around it. An + * installed+enabled plugin declaring a non-empty `requiredAddons` for an + * addon that IS enabled used to throw `cannotUseGlobalContext` here, + * silently (double-swallowed: `activate(id).catch(...)` only reconciles + * `PluginDependencyError`/`DependencyCycleError`, and the outer try/catch + * around the whole discovery/boot block eats everything else) — the plugin + * never left `'inactive'` and no log line named the failure at all. + */ + it('BOOT-REG-002 an installed+enabled plugin declaring requiredAddons for an ENABLED addon activates cleanly at boot — the addon check runs inside a request context', async () => { + const dir = path.join(codeRoot, 'addongated', 'server'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'index.js'), `module.exports = { async onLoad(ctx) {} };`); + testDb.prepare("INSERT INTO addons (id, enabled) VALUES ('needsaddon_addon', 1)").run(); + testDb + .prepare( + "INSERT INTO plugins (id, status, enabled, permissions, granted_permissions, config, dependencies) VALUES ('addongated','inactive',1,'[]','[]','{}', ?)", + ) + .run(JSON.stringify({ requiredAddons: ['needsaddon_addon'] })); + + // Self-contained ORM/collaborators — independent of test 1's `t`, which is + // assigned inside ITS `it` body rather than a shared beforeAll. Global + // context is disallowed on purpose here (unlike test 1's `t`, and unlike + // createTestAddonsService's sharedTestOrm, both of which default to + // allowGlobalContext: true — the test-only convenience that would let + // AddonsService.isAddonEnabled succeed with NO request context and hide + // exactly the bug this test exists to catch): this is what makes the + // addon check below genuinely depend on the withRequestContext wrap + // rather than passing either way. + const t2 = await createTestOrm(dbConn, { allowGlobalContext: false }); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + let mod2: TestingModule | undefined; + try { + const dbs2 = new DatabaseService(dbConn); + const userSettings2 = new PluginUserSettingsService(dbs2); + const registry2 = new PluginRpcRegistry(); + // Registry already scanned — this test is about the addon check, not the + // registry-ordering bug BOOT-REG-001 pins. + registry2.register(new DbRpc(userSettings2)); + const hostFactory2 = new PluginRpcHostFactory(dbs2, registry2 as unknown as PluginRpcRegistryService); + // Built directly on t2 (not createTestAddonsService's sharedTestOrm) so + // its repositories share t2's allowGlobalContext: false ORM. + const addonsService2 = new AddonsService( + t2.repo(Addons) as AddonsRepository, + t2.repo(PhotoProviders) as PhotoProvidersRepository, + t2.repo(PhotoProviderFields) as PhotoProviderFieldsRepository, + t2.repo(AppSettings) as AppSettingsRepository, + t2.repo(Users) as UsersRepository, + dbs2, + ); + const auditLogRepo2 = t2.repo(AuditLog) as AuditLogRepository; + const usersRepo2 = t2.repo(Users) as UsersRepository; + + mod2 = await Test.createTestingModule({ + providers: [ + { + provide: PluginRuntimeService, + useFactory: () => + new PluginRuntimeService(dbs2, new AuditService(auditLogRepo2, usersRepo2), addonsService2, userSettings2, undefined, hostFactory2, undefined, t2.orm), + }, + ], + }).compile(); + await mod2.init(); + + const runtime2 = mod2.get(PluginRuntimeService); + let row = { status: 'starting', last_error: null as string | null }; + for (let i = 0; i < 100; i++) { + row = testDb.prepare("SELECT status, last_error FROM plugins WHERE id='addongated'").get() as typeof row; + if (row.status === 'active' || row.status === 'error') break; + await new Promise((r) => setTimeout(r, 50)); + } + + expect(row.last_error).toBeNull(); + expect(row.status).toBe('active'); + expect(runtime2.isActive('addongated')).toBe(true); + + 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(); + await mod2?.close(); + await t2.close(); + } + }); }); diff --git a/server/tests/unit/nest/airports/airports.service.test.ts b/server/tests/unit/nest/airports/airports.service.test.ts new file mode 100644 index 000000000..db094c197 --- /dev/null +++ b/server/tests/unit/nest/airports/airports.service.test.ts @@ -0,0 +1,81 @@ +/** + * AirportsService boot backfill (task-6-rereview.md M1 — the exact C1 shape + * task-6-review-parity.md found elsewhere: `onApplicationBootstrap` used to + * `await this.backfillFlightEndpoints()` directly, outside any request + * context. Raw SQL only today (harmless now), but the wrap belongs at the + * entrypoint — the same reasoning `place-photo-cache.job.ts`/ + * `trek-photo-cache.job.ts` already carry for their own raw-SQL sweeps — so + * it stays safe if this dependency graph goes repository-backed later, and so + * every one-off boot sweep in the tree goes through the one choke point). + */ +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 { AirportsService } from '../../../../src/nest/airports/airports.service'; +import { DatabaseService } from '../../../../src/nest/database/database.service'; +import { CronRegistrarService } from '../../../../src/nest/scheduling/cron-registrar.service'; +import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; + +beforeAll(async () => { + // Global context disallowed — the production setting — so this genuinely + // proves the boot backfill runs (or doesn't) inside a request context, + // rather than happening to work because the test default allows it. + t = await createTestOrm(testDb, { allowGlobalContext: false }); +}); +afterAll(async () => { await t.close(); testDb.close(); }); +beforeEach(() => vi.clearAllMocks()); + +describe('AirportsService boot backfill', () => { + it('AIRPORTS-SVC-001: onApplicationBootstrap delegates the boot-time backfill through CronRegistrarService.runOnBoot, not a direct await', 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 svc = new AirportsService(new DatabaseService(testDb), registrar as unknown as CronRegistrarService); + const backfillSpy = vi.spyOn(svc, 'backfillFlightEndpoints').mockResolvedValue(undefined); + await svc.onApplicationBootstrap(); + expect(registrar.runOnBoot).toHaveBeenCalledWith('airports-flight-endpoints-boot', expect.any(Function)); + expect(backfillSpy).toHaveBeenCalledTimes(1); + }); + + it('AIRPORTS-SVC-002: with MikroORM wired into the registrar, the boot-time backfill actually runs — no cannotUseGlobalContext, nothing swallowed', async () => { + const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService, t.orm); + const svc = new AirportsService(new DatabaseService(testDb), registrar); + const backfillSpy = vi.spyOn(svc, 'backfillFlightEndpoints'); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + await svc.onApplicationBootstrap(); + expect(backfillSpy).toHaveBeenCalledTimes(1); + expect(errSpy).not.toHaveBeenCalled(); + expect(logMock.logError).not.toHaveBeenCalled(); + } finally { + errSpy.mockRestore(); + } + }); + + it('AIRPORTS-SVC-003: without MikroORM, the boot-time backfill never runs at all — logged with runOnBoot\'s own distinct message, never swallowed into "[DB] Flight endpoint backfill failed"', async () => { + const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService); // no orm + const svc = new AirportsService(new DatabaseService(testDb), registrar); + const backfillSpy = vi.spyOn(svc, 'backfillFlightEndpoints'); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + await svc.onApplicationBootstrap(); + expect(backfillSpy).not.toHaveBeenCalled(); + expect(logMock.logError).toHaveBeenCalledWith(expect.stringMatching(/runOnBoot: no MikroORM available.*airports-flight-endpoints-boot/)); + expect(errSpy).not.toHaveBeenCalled(); + } finally { + errSpy.mockRestore(); + } + }); +}); diff --git a/server/tests/unit/plugins/request-context.test.ts b/server/tests/unit/plugins/request-context.test.ts index f07b4c815..e52c6c749 100644 --- a/server/tests/unit/plugins/request-context.test.ts +++ b/server/tests/unit/plugins/request-context.test.ts @@ -29,7 +29,7 @@ * shared helpers use to let ordinary unit tests call a repository with no * request wrapper at all: that default would hide this exact bug. */ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import { createSnapshotTestDb } from '../../helpers/db-mock'; import { createUser } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; @@ -112,16 +112,26 @@ function makeSupervisor(resolveOrm?: () => { em: EntityManager } | undefined) { } describe('PluginSupervisor request context (D6, C3)', () => { - 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 () => { + it('CTX-PLUGIN-001: without a resolveOrm thunk, the dispatch THROWS (host-side visibility) but the child still gets answered first (task-6-rereview.md §5 RULING: an unanswered \'req\' hangs the plugin forever — plugin-host-entry.ts\'s pending map has no timeout)', async () => { permissions.invalidatePermissionsCache(); const { supervisor, sup, sent } = makeSupervisor(); // no resolveOrm — the bug's exact reproduction + const canCreateAsSpy = vi.spyOn(guards, 'canCreateAs'); 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); + // The dispatch itself never ran — same fail-closed guarantee as before. + expect(canCreateAsSpy).not.toHaveBeenCalled(); + // But unlike the previous wave, the child DOES get a response: a HOST_ERROR + // envelope, matching the rate-limiter refusal's shape, so its ctx.* promise + // settles instead of hanging forever. + expect(sent).toHaveLength(1); + const res = sent[0]; + expect(res.ok).toBe(false); + if (res.ok === false) { + expect(res.error.code).toBe('HOST_ERROR'); + expect(res.error.message).toMatch(/no ORM available/i); + } }); it('CTX-PLUGIN-002: wrapped in withRequestContext, the same dispatch enforces the stored override — a "user" role is refused', async () => {