From 78b63972d095e1d56f17d0977c882d2dd55fdb92 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 10 Aug 2026 21:56:13 +0200 Subject: [PATCH] refactor(server): move the version check and demo reset into the admin domain --- server/src/index.ts | 2 - server/src/nest/admin/admin.bridge.ts | 58 ------------- server/src/nest/admin/admin.helpers.ts | 12 +-- server/src/nest/admin/admin.module.ts | 7 +- server/src/nest/admin/admin.service.ts | 2 +- server/src/nest/admin/demo-reset.job.ts | 35 ++++++++ server/src/nest/admin/version-check.job.ts | 32 +++++++ server/src/scheduler.ts | 39 +-------- server/tests/unit/nest/admin-jobs.test.ts | 87 ++++++++++++++++++++ server/tests/unit/nest/admin.service.test.ts | 20 +++-- 10 files changed, 180 insertions(+), 114 deletions(-) delete mode 100644 server/src/nest/admin/admin.bridge.ts create mode 100644 server/src/nest/admin/demo-reset.job.ts create mode 100644 server/src/nest/admin/version-check.job.ts create mode 100644 server/tests/unit/nest/admin-jobs.test.ts diff --git a/server/src/index.ts b/server/src/index.ts index aa0e5bdba..1893de5f4 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -101,8 +101,6 @@ const onListen = () => { // belongs at boot. airtrail: nestApp.get(AirtrailSyncService), }); - scheduler.startVersionCheck(); - scheduler.startDemoReset(); scheduler.startIdempotencyCleanup(); scheduler.startTrekPhotoCacheCleanup(); scheduler.startPlacePhotoCacheCleanup(); diff --git a/server/src/nest/admin/admin.bridge.ts b/server/src/nest/admin/admin.bridge.ts deleted file mode 100644 index 8cad2020e..000000000 --- a/server/src/nest/admin/admin.bridge.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { db } from '../../db/database'; -import { DatabaseService } from '../database/database.service'; -import { RealtimeService } from '../realtime/realtime.service'; -import { AddonsService } from '../addons/addons.service'; -import { SettingsService } from '../settings/settings.service'; -import { AuthService } from '../auth/auth.service'; -import { PasskeyService } from '../auth/passkey.service'; -import { UserCleanupService } from '../auth/user-cleanup.service'; -import { WebauthnConfigService } from '../auth/webauthn-config.service'; -import { TripMembershipService } from '../trip-membership/trip-membership.service'; -import { PackingService } from '../packing/packing.service'; -import { PermissionsService } from '../permissions/permissions.service'; -import { MailerService } from '../notifications/mailer/mailer.service'; -import { NotificationPreferencesService } from '../notifications/notification-preferences.service'; -import { NtfyService } from '../notifications/transports/ntfy.service'; -import { WebhookService } from '../notifications/transports/webhook.service'; -import { NotificationsService } from '../notifications/notifications.service'; -import { AdminService } from './admin.service'; -import { EphemeralTokenService } from '../auth/ephemeral-token.service'; - -/** - * Non-Nest entry point for the admin domain — for code running OUTSIDE the Nest - * container. Exactly one consumer: the daily version-check cron in - * scheduler.ts, which lazy-requires this instead of the retired - * services/adminService. Exports the legacy name 1:1 so the repoint was an - * import-path-only diff. Inside the container, inject AdminService instead. - * Delete this file if the scheduler ever moves into Nest's @nestjs/schedule. - * - * The 5-minute version cache is module-scoped in admin.helpers.ts, so this - * instance and the container singleton share it — the cron and - * GET /api/admin/version-check hit GitHub once between them, as they did before - * the fold. - * - * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto - * the shared better-sqlite3 singleton. - */ -const dbs = new DatabaseService(db); -const realtime = new RealtimeService(); -const permissions = new PermissionsService(dbs); -const webauthn = new WebauthnConfigService(dbs); -const userCleanup = new UserCleanupService(dbs); -const mailer = new MailerService(dbs); -const notifPrefs = new NotificationPreferencesService(dbs, mailer); -const auth = new AuthService(dbs, permissions, new TripMembershipService(dbs), webauthn, userCleanup, mailer, new EphemeralTokenService()); -const admin = new AdminService( - dbs, - new AddonsService(dbs), - new PasskeyService(dbs, auth, webauthn), - auth, - permissions, - new NotificationsService(dbs, realtime, mailer, new WebhookService(dbs), new NtfyService(dbs), notifPrefs), - userCleanup, - new RealtimeService(), -); - -export function checkAndNotifyVersion(): Promise { - return admin.checkAndNotifyVersion(); -} diff --git a/server/src/nest/admin/admin.helpers.ts b/server/src/nest/admin/admin.helpers.ts index c06ed7aff..2c984c210 100644 --- a/server/src/nest/admin/admin.helpers.ts +++ b/server/src/nest/admin/admin.helpers.ts @@ -6,11 +6,13 @@ import fs from 'fs'; * atlas-geo precedent). * * The version cache is module-scoped **on purpose** (the permissions-cache - * precedent, permissions.bridge.ts): scheduler.ts reaches checkAndNotifyVersion - * through admin.bridge's instance while GET /api/admin/version-check runs on the - * container singleton, and the legacy module shared one 5-minute cache between - * them. Instance state would double the GitHub traffic and let the scheduler - * notify on a version the UI has not seen yet. + * precedent, permissions.bridge.ts): the daily version-check cron + * (VersionCheckJob) and GET /api/admin/version-check both run on the container + * singleton now, but the module scope keeps the one-5-minute-cache guarantee + * independent of how many AdminService instances exist (the legacy module and + * the retired admin bridge relied on exactly that). Instance state would double + * the GitHub traffic and let the cron notify on a version the UI has not seen + * yet. */ /** bcrypt cost factor for user passwords — kept in sync with authService. */ diff --git a/server/src/nest/admin/admin.module.ts b/server/src/nest/admin/admin.module.ts index adba9dc6d..2efa9e50f 100644 --- a/server/src/nest/admin/admin.module.ts +++ b/server/src/nest/admin/admin.module.ts @@ -3,6 +3,9 @@ import { TokensModule } from '../tokens/tokens.module'; import { OauthModule } from '../oauth/oauth.module'; import { AdminController } from './admin.controller'; import { AdminService } from './admin.service'; +import { VersionCheckJob } from './version-check.job'; +import { DemoResetJob } from './demo-reset.job'; +import { SchedulingModule } from '../scheduling/scheduling.module'; import { PluginsRuntimeModule } from '../plugins/plugins-runtime.module'; import { SettingsModule } from '../settings/settings.module'; import { AuditModule } from '../audit/audit.module'; @@ -20,8 +23,8 @@ import { PackingModule } from '../packing/packing.module'; import { PermissionsModule } from '../permissions/permissions.module'; @Module({ - imports: [PluginsRuntimeModule, SettingsModule, AuditModule, AddonsModule, AuthModule, NotificationsModule, PackingModule, PermissionsModule, TokensModule, OauthModule], + imports: [PluginsRuntimeModule, SettingsModule, AuditModule, AddonsModule, AuthModule, NotificationsModule, PackingModule, PermissionsModule, TokensModule, OauthModule, SchedulingModule], controllers: [AdminController], - providers: [AdminService], + providers: [AdminService, VersionCheckJob, DemoResetJob], }) export class AdminModule {} diff --git a/server/src/nest/admin/admin.service.ts b/server/src/nest/admin/admin.service.ts index 74c28ce99..6cd0e71ee 100644 --- a/server/src/nest/admin/admin.service.ts +++ b/server/src/nest/admin/admin.service.ts @@ -10,7 +10,7 @@ import { updateJwtSecret } from '../../config'; // the whole tools fan-out (and via the domain bridges, the Nest services) into // every consumer of this module — a nest→mcp→nest module cycle. The // invalidateMcpSessions barrel import below predates the fold and is deliberately -// separate: it is only reached from the controller, never from admin.bridge. +// separate: it is only reached from the controller, never from the cron path. import { revokeUserSessions, revokeUserSessionsForClient } from '../../mcp/sessionManager'; import { invalidateMcpSessions } from '../../mcp'; import { emitUserDeleted } from '../../plugin-user-lifecycle'; diff --git a/server/src/nest/admin/demo-reset.job.ts b/server/src/nest/admin/demo-reset.job.ts new file mode 100644 index 000000000..f2c004e7b --- /dev/null +++ b/server/src/nest/admin/demo-reset.job.ts @@ -0,0 +1,35 @@ +import { Injectable, type OnApplicationBootstrap } from '@nestjs/common'; +import { logInfo, logError } from '../audit/audit-log.logger'; +import { RuntimeEnvService } from '../app-config/runtime-env.service'; +import { CronRegistrarService } from '../scheduling/cron-registrar.service'; +import { resetDemoUser } from '../../demo/demo-reset'; + +/** + * Demo mode: hourly reset of demo user data (moved from src/scheduler.ts). + * Gated on DEMO_MODE at bootstrap — parity: toggling it always required a + * restart — and scheduled in the server-local zone (the old cron.schedule call + * passed no timezone). demo-reset is a static import: its module top is + * side-effect-free, and the dangerous closeDb/reinitialize sequence stays + * inside resetDemoUser, called at tick time. + */ +@Injectable() +export class DemoResetJob implements OnApplicationBootstrap { + constructor( + private readonly runtimeEnv: RuntimeEnvService, + private readonly registrar: CronRegistrarService, + ) {} + + onApplicationBootstrap(): void { + if (!this.registrar.isEnabled() || !this.runtimeEnv.isDemoMode()) return; + this.registrar.register('demo-reset', '0 * * * *', () => this.tick(), { timezone: 'none' }); + logInfo('Demo hourly reset scheduled'); + } + + tick(): void { + try { + resetDemoUser(); + } catch (err: unknown) { + logError(`Demo reset: ${err instanceof Error ? err.message : err}`); + } + } +} diff --git a/server/src/nest/admin/version-check.job.ts b/server/src/nest/admin/version-check.job.ts new file mode 100644 index 000000000..158d27eb0 --- /dev/null +++ b/server/src/nest/admin/version-check.job.ts @@ -0,0 +1,32 @@ +import { Injectable, type OnApplicationBootstrap } from '@nestjs/common'; +import { logError } from '../audit/audit-log.logger'; +import { AdminService } from './admin.service'; +import { CronRegistrarService } from '../scheduling/cron-registrar.service'; + +/** + * Daily version check (moved from src/scheduler.ts — it was the only consumer + * of the old admin bridge, which died with the move). checkAndNotifyVersion + * shares the module-scoped 5-minute cache in admin.helpers.ts with + * GET /api/admin/version-check, so the cron and the route hit GitHub once + * between them. No boot log — parity with the old starter. + */ +@Injectable() +export class VersionCheckJob implements OnApplicationBootstrap { + constructor( + private readonly admin: AdminService, + private readonly registrar: CronRegistrarService, + ) {} + + onApplicationBootstrap(): void { + if (!this.registrar.isEnabled()) return; + this.registrar.register('version-check', '0 9 * * *', () => this.tick()); + } + + async tick(): Promise { + try { + await this.admin.checkAndNotifyVersion(); + } catch (err: unknown) { + logError(`Version check: ${err instanceof Error ? err.message : err}`); + } + } +} diff --git a/server/src/scheduler.ts b/server/src/scheduler.ts index b5b8a09df..e0293b6e9 100644 --- a/server/src/scheduler.ts +++ b/server/src/scheduler.ts @@ -26,41 +26,6 @@ export function setSchedulerDeps(next: SchedulerDeps): void { deps = next; } -// Demo mode: hourly reset of demo user data -let demoTask: ScheduledTask | null = null; - -function startDemoReset(): void { - if (demoTask) { demoTask.stop(); demoTask = null; } - if (!readEnv().demo.enabled) return; - - demoTask = cron.schedule('0 * * * *', () => { - try { - const { resetDemoUser } = require('./demo/demo-reset'); - resetDemoUser(); - } catch (err: unknown) { - logError(`Demo reset: ${err instanceof Error ? err.message : err}`); - } - }); - logInfo('Demo hourly reset scheduled'); -} - -// Version check: daily at 9 AM — notify admins if a new TREK release is available -let versionCheckTask: ScheduledTask | null = null; - -function startVersionCheck(): void { - if (versionCheckTask) { versionCheckTask.stop(); versionCheckTask = null; } - - const tz = readEnv().app.tz || 'UTC'; - versionCheckTask = cron.schedule('0 9 * * *', async () => { - try { - const { checkAndNotifyVersion } = require('./nest/admin/admin.bridge'); - await checkAndNotifyVersion(); - } catch (err: unknown) { - logError(`Version check: ${err instanceof Error ? err.message : err}`); - } - }, { timezone: tz }); -} - // Idempotency key cleanup: nightly at 3 AM — delete keys past their TTL. // The TTL must exceed any realistic offline window: the TREK client replays // queued mutations with their X-Idempotency-Key when it reconnects, so a key @@ -200,12 +165,10 @@ function startAirTrailSync(): void { } function stop(): void { - if (demoTask) { demoTask.stop(); demoTask = null; } - if (versionCheckTask) { versionCheckTask.stop(); versionCheckTask = null; } if (idempotencyCleanupTask) { idempotencyCleanupTask.stop(); idempotencyCleanupTask = null; } if (trekPhotoCacheTask) { trekPhotoCacheTask.stop(); trekPhotoCacheTask = null; } if (placePhotoCacheTask) { placePhotoCacheTask.stop(); placePhotoCacheTask = null; } if (airtrailSyncTask) { airtrailSyncTask.stop(); airtrailSyncTask = null; } } -export { stop, startDemoReset, startVersionCheck, startIdempotencyCleanup, purgeExpiredIdempotencyKeys, startTrekPhotoCacheCleanup, startPlacePhotoCacheCleanup, startAirTrailSync }; +export { stop, startIdempotencyCleanup, purgeExpiredIdempotencyKeys, startTrekPhotoCacheCleanup, startPlacePhotoCacheCleanup, startAirTrailSync }; diff --git a/server/tests/unit/nest/admin-jobs.test.ts b/server/tests/unit/nest/admin-jobs.test.ts new file mode 100644 index 000000000..0df3fae62 --- /dev/null +++ b/server/tests/unit/nest/admin-jobs.test.ts @@ -0,0 +1,87 @@ +/** + * The admin domain's cron providers — VersionCheckJob and DemoResetJob + * (moved from src/scheduler.ts). Registration, gating, and tick error + * containment; the version-check end-to-end path is VCJOB-001 in + * admin.service.test.ts. + */ +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() })); +const { resetDemoUserMock } = vi.hoisted(() => ({ resetDemoUserMock: vi.fn() })); + +vi.mock('../../../src/nest/audit/audit-log.logger', () => logMock); +vi.mock('../../../src/demo/demo-reset', () => ({ resetDemoUser: resetDemoUserMock, saveBaseline: vi.fn(), hasBaseline: vi.fn() })); + +import { VersionCheckJob } from '../../../src/nest/admin/version-check.job'; +import { DemoResetJob } from '../../../src/nest/admin/demo-reset.job'; +import type { AdminService } from '../../../src/nest/admin/admin.service'; +import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; +import type { CronRegistrarService } from '../../../src/nest/scheduling/cron-registrar.service'; + +function registrarStub(enabled = true) { + return { + isEnabled: vi.fn(() => enabled), + register: vi.fn(() => enabled), + unregister: vi.fn(), + }; +} + +beforeEach(() => vi.clearAllMocks()); + +describe('VersionCheckJob', () => { + it('AJOB-001 — registers the daily 9 AM cron, with no boot log (parity)', () => { + const registrar = registrarStub(); + new VersionCheckJob({} as AdminService, registrar as unknown as CronRegistrarService).onApplicationBootstrap(); + expect(registrar.register).toHaveBeenCalledWith('version-check', '0 9 * * *', expect.any(Function)); + expect(logMock.logInfo).not.toHaveBeenCalled(); + }); + + it('AJOB-002 — does not register under the test gate', () => { + const registrar = registrarStub(false); + new VersionCheckJob({} as AdminService, registrar as unknown as CronRegistrarService).onApplicationBootstrap(); + expect(registrar.register).not.toHaveBeenCalled(); + }); + + it('AJOB-003 — a throwing check is contained to the Version check log line', async () => { + const admin = { checkAndNotifyVersion: vi.fn().mockRejectedValue(new Error('github down')) } as unknown as AdminService; + const job = new VersionCheckJob(admin, registrarStub() as unknown as CronRegistrarService); + await expect(job.tick()).resolves.toBeUndefined(); + expect(logMock.logError).toHaveBeenCalledWith('Version check: github down'); + }); +}); + +describe('DemoResetJob', () => { + function make(demo: boolean, enabled = true) { + const registrar = registrarStub(enabled); + const runtimeEnv = { isDemoMode: () => demo } as RuntimeEnvService; + return { job: new DemoResetJob(runtimeEnv, registrar as unknown as CronRegistrarService), registrar }; + } + + it('AJOB-004 — registers the hourly server-local cron and logs the banner when demo mode is on', () => { + const { job, registrar } = make(true); + job.onApplicationBootstrap(); + expect(registrar.register).toHaveBeenCalledWith('demo-reset', '0 * * * *', expect.any(Function), { timezone: 'none' }); + expect(logMock.logInfo).toHaveBeenCalledWith('Demo hourly reset scheduled'); + }); + + it('AJOB-005 — stays silent when demo mode is off or the gate is closed', () => { + const off = make(false); + off.job.onApplicationBootstrap(); + expect(off.registrar.register).not.toHaveBeenCalled(); + + const gated = make(true, false); + gated.job.onApplicationBootstrap(); + expect(gated.registrar.register).not.toHaveBeenCalled(); + expect(logMock.logInfo).not.toHaveBeenCalled(); + }); + + it('AJOB-006 — the tick runs resetDemoUser and contains a throw to the Demo reset log line', () => { + const { job } = make(true); + job.tick(); + expect(resetDemoUserMock).toHaveBeenCalledTimes(1); + + resetDemoUserMock.mockImplementation(() => { throw new Error('baseline gone'); }); + expect(() => job.tick()).not.toThrow(); + expect(logMock.logError).toHaveBeenCalledWith('Demo reset: baseline gone'); + }); +}); diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index 4044cf773..5c803b8a2 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -6,7 +6,8 @@ * their functions to tests/unit/nest/packing.service.test.ts. * Constructs the service directly over a real in-memory SQLite DB (repo * convention — no TestingModule). Focuses on validation/error branches that the - * integration tests don't exercise. ADMIN-BR-001 pins the admin.bridge export. + * integration tests don't exercise. VCJOB-001 pins the version-check cron path + * (it replaced ADMIN-BR-001 when the old admin bridge died with the cron move). */ import { describe, it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from 'vitest'; @@ -71,7 +72,8 @@ import { PackingService } from '../../../src/nest/packing/packing.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import { AdminService } from '../../../src/nest/admin/admin.service'; -import { checkAndNotifyVersion as bridgeCheckAndNotifyVersion } from '../../../src/nest/admin/admin.bridge'; +import { VersionCheckJob } from '../../../src/nest/admin/version-check.job'; +import type { CronRegistrarService } from '../../../src/nest/scheduling/cron-registrar.service'; import { __clearVersionCacheForTests } from '../../../src/nest/admin/admin.helpers'; import { makeNotificationsService, makeNotificationPreferencesService } from '../../helpers/notifications'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; @@ -477,10 +479,12 @@ describe('updateAddon', () => { }); }); -// ── admin.bridge ────────────────────────────────────────────────────────────── +// ── version-check cron ──────────────────────────────────────────────────────── -describe('admin.bridge', () => { - it('ADMIN-BR-001 — checkAndNotifyVersion delegates to AdminService over the shared db Proxy', async () => { +describe('version-check job', () => { + const registrarStub = { isEnabled: () => true, register: vi.fn(() => true), unregister: vi.fn() } as unknown as CronRegistrarService; + + it('VCJOB-001 — the cron tick notifies and shares the module-scoped version cache with the route', async () => { createAdmin(testDb); __clearVersionCacheForTests(); const fetchMock = vi.fn().mockResolvedValue({ @@ -490,15 +494,15 @@ describe('admin.bridge', () => { }); vi.stubGlobal('fetch', fetchMock); - await bridgeCheckAndNotifyVersion(); + await new VersionCheckJob(svc, registrarStub).tick(); const notified = testDb .prepare('SELECT value FROM app_settings WHERE key = ?') .get('last_notified_version') as { value: string } | undefined; expect(notified?.value).toBe('99.9.9'); - // The version cache is module-scoped in admin.helpers, so the bridge instance - // and the container singleton share it — the second read must not re-fetch. + // The version cache is module-scoped in admin.helpers, so the cron and + // GET /api/admin/version-check hit GitHub once between them. expect(await svc.checkVersion()).toMatchObject({ latest: '99.9.9' }); expect(fetchMock).toHaveBeenCalledTimes(1); vi.unstubAllGlobals();