diff --git a/lib/__tests__/scheduler-timer-module-copies.test.ts b/lib/__tests__/scheduler-timer-module-copies.test.ts new file mode 100644 index 000000000..8beb553ee --- /dev/null +++ b/lib/__tests__/scheduler-timer-module-copies.test.ts @@ -0,0 +1,62 @@ +// @vitest-environment node +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// Next loads lib/telemetry and lib/version-check once per bundle. The timer +// sat in a module-level variable, so the admin route's reschedule() acted on +// its own copy: the timer instrumentation started kept running next to the +// new one (two heartbeats), and turning consent off didn't stop it. + +vi.mock('@/lib/admin/third-party-mode', () => ({ isNoThirdPartyTrafficOn: async () => false })); + +let dir: string; + +beforeEach(() => { + dir = mkdtempSync(path.join(tmpdir(), 'bw-scheduler-copies-')); + vi.stubEnv('TELEMETRY_DATA_DIR', path.join(dir, 'telemetry')); + vi.stubEnv('VERSION_CHECK_DATA_DIR', path.join(dir, 'version-check')); + vi.stubEnv('BULWARK_TELEMETRY', 'on'); + vi.stubEnv('BULWARK_UPDATE_CHECK', ''); + vi.stubEnv('BULWARK_UPDATE_CHECK_URL', 'https://version.example/'); + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); +}); + +afterEach(() => { + vi.useRealTimers(); + vi.unstubAllEnvs(); + vi.resetModules(); + rmSync(dir, { recursive: true, force: true }); +}); + +async function copy(load: () => Promise): Promise { + vi.resetModules(); + return load(); +} + +describe('scheduler timers across module copies', () => { + it('telemetry: a stop from another copy clears the running timer, a restart leaves one', async () => { + const instrumentation = await copy(() => import('@/lib/telemetry/sender')); + const route = await copy(() => import('@/lib/telemetry/sender')); + + await instrumentation.startScheduler(); + expect(vi.getTimerCount()).toBe(1); + await route.startScheduler(); + expect(vi.getTimerCount()).toBe(1); + await route.stopScheduler(); + expect(vi.getTimerCount()).toBe(0); + }); + + it('version check: the same', async () => { + const instrumentation = await copy(() => import('@/lib/version-check/sender')); + const route = await copy(() => import('@/lib/version-check/sender')); + + await instrumentation.startScheduler(); + expect(vi.getTimerCount()).toBe(1); + await route.startScheduler(); + expect(vi.getTimerCount()).toBe(1); + await route.stopScheduler(); + expect(vi.getTimerCount()).toBe(0); + }); +}); diff --git a/lib/telemetry/sender.ts b/lib/telemetry/sender.ts index f47b5a374..a06386565 100644 --- a/lib/telemetry/sender.ts +++ b/lib/telemetry/sender.ts @@ -8,7 +8,14 @@ const DAY_MS = 24 * 60 * 60 * 1000; const JITTER_MS = 2 * 60 * 60 * 1000; // ± 2 hours const FIRST_DELAY_MS = 60 * 60 * 1000; // 1 hour after consent -let currentTimer: NodeJS.Timeout | null = null; +// On globalThis: Next loads this module once per bundle (instrumentation, +// each route), and a reschedule from a route must clear the timer the +// instrumentation copy started, not a copy of its own. +const TIMER_KEY = Symbol.for('bulwark.telemetry.timer'); +function timer(): { current: NodeJS.Timeout | null } { + const g = globalThis as typeof globalThis & { [TIMER_KEY]?: { current: NodeJS.Timeout | null } }; + return (g[TIMER_KEY] ??= { current: null }); +} function jitteredDelay(base: number): number { const j = (Math.random() * 2 - 1) * JITTER_MS; @@ -64,12 +71,14 @@ export async function sendOnce(opts?: { reason?: string }): Promise<{ } async function scheduleNext(delayMs: number): Promise { - if (currentTimer) clearTimeout(currentTimer); + const t = timer(); + if (t.current) clearTimeout(t.current); const at = new Date(Date.now() + delayMs).toISOString(); await updateState((state) => { state.nextScheduledAt = at; }); - currentTimer = setTimeout(() => { void tick(); }, delayMs); + if (t.current) clearTimeout(t.current); + t.current = setTimeout(() => { void tick(); }, delayMs); // Don't keep the process alive just for this. - currentTimer.unref?.(); + t.current.unref?.(); } async function tick(): Promise { @@ -104,8 +113,9 @@ export async function startScheduler(): Promise { } export async function stopScheduler(): Promise { - if (currentTimer) clearTimeout(currentTimer); - currentTimer = null; + const t = timer(); + if (t.current) clearTimeout(t.current); + t.current = null; } // Called when consent flips on/off or the collector URL changes via the UI. diff --git a/lib/version-check/sender.ts b/lib/version-check/sender.ts index 34009c652..7904dad93 100644 --- a/lib/version-check/sender.ts +++ b/lib/version-check/sender.ts @@ -13,7 +13,14 @@ const JITTER_MS = 5 * 60 * 1000; // ± 5 min, keeps containers from syncin const FIRST_DELAY_MS = 30 * 1000; // first check ~30s after boot const FAILURE_BACKOFF_MS = 15 * 60 * 1000; // after a failed fetch, retry in 15 min -let currentTimer: NodeJS.Timeout | null = null; +// On globalThis: Next loads this module once per bundle (instrumentation, +// each route), and a reschedule from a route must clear the timer the +// instrumentation copy started, not a copy of its own. +const TIMER_KEY = Symbol.for('bulwark.versionCheck.timer'); +function timer(): { current: NodeJS.Timeout | null } { + const g = globalThis as typeof globalThis & { [TIMER_KEY]?: { current: NodeJS.Timeout | null } }; + return (g[TIMER_KEY] ??= { current: null }); +} /** What checkOnce reports while NO_THIRD_PARTY_TRAFFIC is on. */ export const NO_THIRD_PARTY_ERROR = 'off: no third-party traffic'; @@ -62,12 +69,14 @@ export async function checkOnce(opts?: { reason?: string }): Promise<{ } async function scheduleNext(delayMs: number): Promise { - if (currentTimer) clearTimeout(currentTimer); + const t = timer(); + if (t.current) clearTimeout(t.current); const at = new Date(Date.now() + delayMs).toISOString(); await updateState((state) => { state.nextScheduledAt = at; }); - currentTimer = setTimeout(() => { void tick(); }, delayMs); + if (t.current) clearTimeout(t.current); + t.current = setTimeout(() => { void tick(); }, delayMs); // Don't keep the process alive just for this. - currentTimer.unref?.(); + t.current.unref?.(); } async function tick(): Promise { @@ -122,6 +131,7 @@ export async function startScheduler(): Promise { } export async function stopScheduler(): Promise { - if (currentTimer) clearTimeout(currentTimer); - currentTimer = null; + const t = timer(); + if (t.current) clearTimeout(t.current); + t.current = null; }