From e98f86efba29d1187b36ba64e748598cf51dd6f7 Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 22 Sep 2026 18:13:28 +0200 Subject: [PATCH] =?UTF-8?q?fix(server):=20plan=203a=20re-review=202=20foll?= =?UTF-8?q?ow-ups=20=E2=80=94=20absent-ORM=20boot=20branch=20pinned;=20air?= =?UTF-8?q?ports=20boot=20hook=20never=20rejects;=20BOOT-SWEEP-001=20asser?= =?UTF-8?q?ts=20the=20seven=20routes;=20duplicate=20comment=20removed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- server/src/nest/airports/airports.service.ts | 29 +++++-- .../nest/plugins/plugin-runtime.service.ts | 12 --- server/tests/e2e/airports.e2e.test.ts | 9 ++- .../boot-sweeps-request-context.test.ts | 30 ++++++- .../nest/airports/airports.service.test.ts | 19 +++++ .../plugin-runtime.boot-no-orm.test.ts | 81 +++++++++++++++++++ .../unit/plugins/request-context.test.ts | 35 ++++---- 7 files changed, 179 insertions(+), 36 deletions(-) create mode 100644 server/tests/unit/plugins/plugin-runtime.boot-no-orm.test.ts diff --git a/server/src/nest/airports/airports.service.ts b/server/src/nest/airports/airports.service.ts index a5f0f4347..b3e6d44ca 100644 --- a/server/src/nest/airports/airports.service.ts +++ b/server/src/nest/airports/airports.service.ts @@ -20,6 +20,17 @@ import { CronRegistrarService } from '../scheduling/cron-registrar.service'; * `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. + * + * task-6-rereview2.md M3: the pre-change code could never reject — + * `onApplicationBootstrap` awaited one `try`/`catch` around the whole thing. + * Putting the `try`/`catch` only INSIDE the `runOnBoot` callback left a gap: + * `runOnBoot` itself (its `RequestContext.create`/context machinery) has no + * catch of its own, so anything it throws before the callback runs would + * propagate out of `onApplicationBootstrap` and abort `app.init()` — a total + * boot refusal where the old code swallowed everything. The outer + * `try`/`catch` below restores that parity while the inner one stays for its + * own reason: `backfillFlightEndpoints` failing must not surface as a + * `runOnBoot`-level error. */ @Injectable() export class AirportsService implements OnApplicationBootstrap { @@ -29,13 +40,17 @@ export class AirportsService implements OnApplicationBootstrap { ) {} async onApplicationBootstrap(): Promise { - await this.registrar.runOnBoot('airports-flight-endpoints-boot', async () => { - try { - await this.backfillFlightEndpoints(); - } catch (err) { - console.error('[DB] Flight endpoint backfill failed:', err); - } - }); + try { + await this.registrar.runOnBoot('airports-flight-endpoints-boot', async () => { + try { + await this.backfillFlightEndpoints(); + } catch (err) { + console.error('[DB] Flight endpoint backfill failed:', err); + } + }); + } 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 53e6f915f..02d8d2d26 100644 --- a/server/src/nest/plugins/plugin-runtime.service.ts +++ b/server/src/nest/plugins/plugin-runtime.service.ts @@ -281,18 +281,6 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // 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(', ')}`); diff --git a/server/tests/e2e/airports.e2e.test.ts b/server/tests/e2e/airports.e2e.test.ts index bb05c382c..cc5a8c261 100644 --- a/server/tests/e2e/airports.e2e.test.ts +++ b/server/tests/e2e/airports.e2e.test.ts @@ -31,6 +31,7 @@ vi.mock('../../src/nest/airports/airports.data', async (importActual) => { import { AirportsModule } from '../../src/nest/airports/airports.module'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; import { TestUnitOfWorkModule } from '../helpers/test-uow'; +import { createTestMikroOrmModule } from '../helpers/test-orm'; const BER = { iata: 'BER', icao: 'EDDB', name: 'Berlin Brandenburg', city: 'Berlin', @@ -42,7 +43,13 @@ describe('Airports e2e (real auth guard + temp SQLite)', () => { let app: Awaited>; async function build() { - const moduleRef = await Test.createTestingModule({ imports: [await TestUnitOfWorkModule.forRoot(db), AirportsModule] }).compile(); + const moduleRef = await Test.createTestingModule({ + // task-6-rereview2.md M4: without a MikroORM in the graph, + // CronRegistrarService.runOnBoot skips the backfill and logs a genuine + // [ERROR] line on every run of this file. createTestMikroOrmModule(db) + // wires one so the boot backfill actually runs here, same as production. + imports: [await TestUnitOfWorkModule.forRoot(db), createTestMikroOrmModule(db), AirportsModule], + }).compile(); const nest = moduleRef.createNestApplication(); nest.use(cookieParser()); nest.useGlobalFilters(new TrekExceptionFilter()); diff --git a/server/tests/integration/boot-sweeps-request-context.test.ts b/server/tests/integration/boot-sweeps-request-context.test.ts index 3de62faca..4fad50145 100644 --- a/server/tests/integration/boot-sweeps-request-context.test.ts +++ b/server/tests/integration/boot-sweeps-request-context.test.ts @@ -39,6 +39,20 @@ * `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`. + * + * task-6-rereview2.md M2: the log-line assertion alone is timing-based, not + * structural, and in practice only `journey-thumbs-boot` makes it + * load-bearing — the other six sweeps are raw SQL that never reaches the EM, + * so a job silently routed back to a bare `void this.sweep()` (bypassing + * `runOnBoot` entirely) would fail here only for journey-thumbs; the per-job + * unit test is what actually catches the rest. This file drives all seven + * jobs' REAL `onApplicationBootstrap`, each of which routes through + * `runOnBoot` — but that routing itself was previously unasserted here. The + * second expectation below asserts it directly: every one of the seven boot + * names was passed to `runOnBoot`, so a job re-routed around the registrar + * fails HERE, not only in its own `*-00N` per-job suite. `journey-thumbs-boot` + * remains the one sweep whose body reaches a repository, so it is still what + * the log-line assertion below is actually exercising. */ import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import type { INestApplication } from '@nestjs/common'; @@ -81,7 +95,7 @@ describe('Every onApplicationBootstrap boot sweep runs inside a request context' testDb.close(); }); - it('BOOT-SWEEP-001: the seven jobs\' REAL onApplicationBootstrap, driven through the real production-wired CronRegistrarService, never logs cannotUseGlobalContext / "global EntityManager"', async () => { + it('BOOT-SWEEP-001: the seven jobs\' REAL onApplicationBootstrap, driven through the real production-wired CronRegistrarService, each routes through runOnBoot (structurally asserted), and journey-thumbs — the one sweep that reaches a repository today — 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); @@ -119,6 +133,20 @@ describe('Every onApplicationBootstrap boot sweep runs inside a request context' .map((args) => args.map(String).join(' ')) .filter((line) => /cannotUseGlobalContext|global EntityManager/i.test(line)); expect(suspicious).toEqual([]); + + // task-6-rereview2.md M2: structural, not timing-based — a job routed back + // to a bare `void this.sweep()` (bypassing runOnBoot entirely) fails HERE + // by name, regardless of whether its sweep body happens to touch a + // repository yet. + expect(runOnBootSpy.mock.calls.map((c) => c[0]).sort()).toEqual([ + 'airtrail-sync-boot', + 'dawarich-sync-boot', + 'docsync-boot', + 'journey-thumbs-boot', + 'place-photo-cache-boot', + 'reminder-jobs-boot', + 'trek-photo-cache-boot', + ]); } finally { errSpy.mockRestore(); isEnabledSpy.mockRestore(); diff --git a/server/tests/unit/nest/airports/airports.service.test.ts b/server/tests/unit/nest/airports/airports.service.test.ts index db094c197..10b1323ad 100644 --- a/server/tests/unit/nest/airports/airports.service.test.ts +++ b/server/tests/unit/nest/airports/airports.service.test.ts @@ -78,4 +78,23 @@ describe('AirportsService boot backfill', () => { errSpy.mockRestore(); } }); + + it('AIRPORTS-SVC-004 (task-6-rereview2.md M3): a rejecting runOnBoot (the context machinery itself, not the backfill body) resolves onApplicationBootstrap instead of aborting app.init()', async () => { + const registrar = { + isEnabled: vi.fn(() => true), + register: vi.fn(() => true), + unregister: vi.fn(), + runOnBoot: vi.fn(async () => { throw new Error('RequestContext.create blew up'); }), + }; + const svc = new AirportsService(new DatabaseService(testDb), registrar as unknown as CronRegistrarService); + const backfillSpy = vi.spyOn(svc, 'backfillFlightEndpoints'); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + await expect(svc.onApplicationBootstrap()).resolves.toBeUndefined(); + expect(backfillSpy).not.toHaveBeenCalled(); + expect(errSpy).toHaveBeenCalledWith('[DB] Flight endpoint backfill failed:', expect.any(Error)); + } finally { + errSpy.mockRestore(); + } + }); }); diff --git a/server/tests/unit/plugins/plugin-runtime.boot-no-orm.test.ts b/server/tests/unit/plugins/plugin-runtime.boot-no-orm.test.ts new file mode 100644 index 000000000..e08150a27 --- /dev/null +++ b/server/tests/unit/plugins/plugin-runtime.boot-no-orm.test.ts @@ -0,0 +1,81 @@ +/** + * task-6-rereview2.md I-A: `PluginRuntimeService.onApplicationBootstrap`'s + * absent-ORM branch (`plugin-runtime.service.ts:~296-300`) was dead + * suite-wide — every existing construction of the service that reaches + * `onApplicationBootstrap` (boot-registry-order.test.ts, plugin-host.ts) hands + * it a real MikroORM. That branch is the half of the boot RULING + * (task-6-rereview.md §2) that makes "skip loudly, never abort app.init()" + * defensible: dropping the `logError` call, or falling through to `activate` + * anyway, would ship green with no test failing. This file pins it directly. + */ +import { describe, it, expect, vi, beforeAll, afterAll, 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 { PluginRuntimeService } from '../../../src/nest/plugins/plugin-runtime.service'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { AuditService } from '../../../src/nest/audit/audit.service'; +import { PluginUserSettingsService } from '../../../src/nest/plugins/plugin-user-settings.service'; +import { createTestAddonsService } from '../../helpers/test-addons'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { AuditLog } from '../../../src/db/entities/AuditLog.entity'; +import { Users } from '../../../src/db/entities/Users.entity'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; + +beforeAll(async () => { + // Global context disallowed — the production setting — so the WITH-orm + // sibling case genuinely proves activation runs through a request context, + // the same reasoning airports.service.test.ts and BOOT-REG-002 use. + t = await createTestOrm(testDb, { allowGlobalContext: false }); +}); +afterAll(async () => { await t.close(); testDb.close(); }); +beforeEach(() => vi.clearAllMocks()); + +/** installedDepRows() reads the `plugins` table directly — no discovery needed. */ +function installEnabledPlugin(id: string): void { + testDb + .prepare( + `INSERT INTO plugins (id, name, status, enabled, version, api_version, permissions, granted_permissions, capabilities, config, dependencies) + VALUES (?, ?, 'inactive', 1, '1.0.0', 1, '[]', '[]', '{}', '{}', '{}')`, + ) + .run(id, id); +} + +async function buildRuntime(withOrm: boolean): Promise { + const dbs = new DatabaseService(testDb); + const audit = new AuditService(t.repo(AuditLog), t.repo(Users)); + const addons = await createTestAddonsService(testDb, dbs); + const userSettings = new PluginUserSettingsService(dbs); + return new PluginRuntimeService(dbs, audit, addons, userSettings, undefined, undefined, undefined, withOrm ? t.orm : undefined); +} + +describe('PluginRuntimeService.onApplicationBootstrap — absent-ORM boot branch (task-6-rereview2.md I-A)', () => { + it('RT-BOOT-NOORM-001: without an ORM, boot logs the skip line naming the plugin id and never calls activate', async () => { + installEnabledPlugin('rtboot-noorm'); + const rt = await buildRuntime(false); + const activateSpy = vi.spyOn(rt, 'activate'); + + await rt.onApplicationBootstrap(); + + expect(logMock.logError).toHaveBeenCalledTimes(1); + const [message] = logMock.logError.mock.calls[0] as [string]; + expect(message).toMatch(/PluginRuntimeService\.onApplicationBootstrap: no MikroORM available/); + expect(message).toContain('rtboot-noorm'); + expect(activateSpy).not.toHaveBeenCalled(); + }); + + it('RT-BOOT-NOORM-002: WITH an ORM, the same arrangement activates the plugin — proves case 001 is not vacuous', async () => { + installEnabledPlugin('rtboot-withorm'); + const rt = await buildRuntime(true); + const activateSpy = vi.spyOn(rt, 'activate').mockResolvedValue(undefined); + + await rt.onApplicationBootstrap(); + + expect(activateSpy).toHaveBeenCalledWith('rtboot-withorm'); + expect(logMock.logError).not.toHaveBeenCalled(); + }); +}); diff --git a/server/tests/unit/plugins/request-context.test.ts b/server/tests/unit/plugins/request-context.test.ts index b291b334a..17c6662c3 100644 --- a/server/tests/unit/plugins/request-context.test.ts +++ b/server/tests/unit/plugins/request-context.test.ts @@ -115,21 +115,26 @@ describe('PluginSupervisor request context (D6, C3)', () => { 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 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); + try { + await expect( + supervisor.onMessage(sup, { k: 'req', id: 'r1', method: 'trips.create', params: { _inv: 'inv-1' } }), + ).rejects.toThrow(/no ORM available/i); + // 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); + } + } finally { + // task-6-rereview2.md M5: never restored before — a leak waiting for the + // first mockImplementation another test in this file adds to `guards`. + canCreateAsSpy.mockRestore(); } });