mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-19 05:26:47 +00:00
fix(server): plan 3a re-review follow-ups — plugin boot activation and the airports backfill inside a request context; the supervisor answers the child before failing closed; BOOT-SWEEP-001 drives the real boot hooks; ws adapter seam pinned
This commit is contained in:
@@ -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],
|
||||
})
|
||||
|
||||
@@ -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<void> {
|
||||
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[] {
|
||||
|
||||
@@ -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 */
|
||||
|
||||
@@ -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));
|
||||
|
||||
@@ -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<void>[] = [];
|
||||
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();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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() {}
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<void>]> = [];
|
||||
const registrar = {
|
||||
isEnabled: vi.fn(() => true),
|
||||
register: vi.fn(() => true),
|
||||
unregister: vi.fn(),
|
||||
runOnBoot: vi.fn(async (name: string, fn: () => void | Promise<void>) => { 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();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user