diff --git a/src/__tests__/timer-registry.test.ts b/src/__tests__/timer-registry.test.ts new file mode 100644 index 00000000..28a87916 --- /dev/null +++ b/src/__tests__/timer-registry.test.ts @@ -0,0 +1,137 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { TimerRegistry } from '../utils/timer-registry.js'; + +describe('TimerRegistry', () => { + let registry: TimerRegistry; + + beforeEach(() => { + registry = new TimerRegistry(); + vi.useFakeTimers({ shouldAdvanceTime: true }); + }); + + afterEach(() => { + registry.clearAll(); + vi.useRealTimers(); + }); + + describe('setTimeout', () => { + it('wraps setTimeout and fires callback after delay', () => { + const fn = vi.fn(); + registry.setTimeout(fn, 100); + vi.advanceTimersByTime(100); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('tracks active timer and removes after firing', () => { + registry.setTimeout(() => {}, 100); + expect(registry.activeCount).toBe(1); + vi.advanceTimersByTime(100); + expect(registry.activeCount).toBe(0); + }); + + it('passes through extra arguments', () => { + const fn = vi.fn(); + registry.setTimeout(fn, 50, 'a', 'b'); + vi.advanceTimersByTime(50); + expect(fn).toHaveBeenCalledWith('a', 'b'); + }); + }); + + describe('setInterval', () => { + it('wraps setInterval and fires repeatedly', () => { + const fn = vi.fn(); + registry.setInterval(fn, 100); + vi.advanceTimersByTime(350); + expect(fn).toHaveBeenCalledTimes(3); + }); + + it('tracks interval and clears when clearInterval is called', () => { + const fn = vi.fn(); + const id = registry.setInterval(fn, 100); + expect(registry.activeCount).toBe(1); + registry.clearInterval(id); + expect(registry.activeCount).toBe(0); + vi.advanceTimersByTime(200); + expect(fn).toHaveBeenCalledTimes(0); + }); + }); + + describe('clearTimeout', () => { + it('prevents callback from firing', () => { + const fn = vi.fn(); + const id = registry.setTimeout(fn, 100); + registry.clearTimeout(id); + vi.advanceTimersByTime(200); + expect(fn).toHaveBeenCalledTimes(0); + }); + + it('untracks the timer', () => { + const id = registry.setTimeout(() => {}, 100); + expect(registry.activeCount).toBe(1); + registry.clearTimeout(id); + expect(registry.activeCount).toBe(0); + }); + }); + + describe('clearAll', () => { + it('clears all tracked timeouts and intervals', () => { + registry.setTimeout(() => {}, 1000); + registry.setInterval(() => {}, 1000); + registry.setTimeout(() => {}, 2000); + expect(registry.activeCount).toBe(3); + registry.clearAll(); + expect(registry.activeCount).toBe(0); + }); + + it('prevents all callbacks from firing', () => { + const fn1 = vi.fn(); + const fn2 = vi.fn(); + registry.setTimeout(fn1, 100); + registry.setInterval(fn2, 50); + registry.clearAll(); + vi.advanceTimersByTime(500); + expect(fn1).toHaveBeenCalledTimes(0); + expect(fn2).toHaveBeenCalledTimes(0); + }); + }); + + describe('activeCount', () => { + it('returns 0 for new registry', () => { + expect(registry.activeCount).toBe(0); + }); + + it('increases when timers are added, decreases when cleared or fired', () => { + expect(registry.activeCount).toBe(0); + registry.setTimeout(() => {}, 100); + expect(registry.activeCount).toBe(1); + registry.setInterval(() => {}, 100); + expect(registry.activeCount).toBe(2); + vi.advanceTimersByTime(100); + expect(registry.activeCount).toBe(1); + }); + }); + + describe('edge cases', () => { + it('handles idempotent clearAll calls', () => { + registry.clearAll(); + registry.clearAll(); + expect(registry.activeCount).toBe(0); + }); + + it('handles setTimeout with delay 0', () => { + const fn = vi.fn(); + registry.setTimeout(fn, 0); + vi.advanceTimersByTime(0); + expect(fn).toHaveBeenCalledTimes(1); + expect(registry.activeCount).toBe(0); + }); + + it('still tracks timer after clearInterval even if clearAll is called later', () => { + const id = registry.setInterval(() => {}, 100); + registry.clearInterval(id); + expect(registry.activeCount).toBe(0); + registry.clearAll(); + expect(registry.activeCount).toBe(0); + }); + }); +}); diff --git a/src/server.ts b/src/server.ts index 56c7d448..7bbfded7 100644 --- a/src/server.ts +++ b/src/server.ts @@ -16,6 +16,7 @@ import fastifyRateLimit from '@fastify/rate-limit'; import fs from 'node:fs/promises'; import { existsSync, readFileSync, watch, type FSWatcher } from 'node:fs'; import { getAuthTokenFilePath, persistAuthTokenFile } from './utils/auth-token-path.js'; +import { TimerRegistry } from './utils/timer-registry.js'; import fastifyWebsocket from '@fastify/websocket'; import fastifyCors from '@fastify/cors'; import crypto from 'node:crypto'; @@ -162,6 +163,8 @@ let dashboardTokenSessions = new DashboardSessionStore(); let configWatcher: FSWatcher | null = null; // Issue #4116: Debounce Set for session approval callbacks (prevents duplicate notifications from rapid Telegram clicks). const recentApprovalActions = new Set(); +// Issue #4248: Centralized timer registry for clean shutdown +const timers = new TimerRegistry(); let acpLocalProfile: AcpLocalStorageProfile | null = null; let acpSessionService: AcpSessionService | null = null; let acpBackend: AcpBackend | null = null; @@ -194,7 +197,7 @@ async function handleInbound(cmd: InboundCommand): Promise { // Issue #4116: Debounce — skip if this session was already processed recently. if (recentApprovalActions.has(cmd.sessionId)) break; recentApprovalActions.add(cmd.sessionId); - setTimeout(() => recentApprovalActions.delete(cmd.sessionId), 2000); + timers.setTimeout(() => recentApprovalActions.delete(cmd.sessionId), 2000); // Issue #4117: Include actor info (Telegram user) in approvedBy. const approveActor = cmd.actor?.type === 'telegram' ? `telegram:${cmd.actor.userId} (${cmd.actor.firstName})` @@ -218,7 +221,7 @@ async function handleInbound(cmd: InboundCommand): Promise { // Issue #4116: Debounce — skip if this session was already processed recently. if (recentApprovalActions.has(cmd.sessionId)) break; recentApprovalActions.add(cmd.sessionId); - setTimeout(() => recentApprovalActions.delete(cmd.sessionId), 2000); + timers.setTimeout(() => recentApprovalActions.delete(cmd.sessionId), 2000); // Issue #4117: Include actor info in rejection log. const rejectActor = cmd.actor?.type === 'telegram' ? `telegram:${cmd.actor.userId} (${cmd.actor.firstName})` @@ -793,8 +796,8 @@ function setupConfigWatcher(): void { configWatcher = watch(configPath, (_eventType) => { // Accept all event types — editors emit rename (atomic save), change, or undefined. // Debounce: FS events can fire multiple times for one save - if (configReloadTimer) clearTimeout(configReloadTimer); - configReloadTimer = setTimeout(() => { + if (configReloadTimer) timers.clearTimeout(configReloadTimer); + configReloadTimer = timers.setTimeout(() => { void handleConfigReload('file-change'); }, 300); }); @@ -1298,19 +1301,20 @@ async function main(): Promise { registerOpenApiRoute(app); // Issue #361: Store interval refs so graceful shutdown can clear them - const reaperInterval = setInterval(() => reapStaleSessions(config.maxSessionAgeMs), config.reaperIntervalMs); - const zombieReaperInterval = setInterval(() => reapZombieSessions(), ZOMBIE_REAP_INTERVAL_MS); - const metricsSaveInterval = setInterval(() => { void metrics.save(); }, 5 * 60 * 1000); + // Issue #4248: All intervals tracked via TimerRegistry for clean shutdown + timers.setInterval(() => reapStaleSessions(config.maxSessionAgeMs), config.reaperIntervalMs); + timers.setInterval(() => reapZombieSessions(), ZOMBIE_REAP_INTERVAL_MS); + timers.setInterval(() => { void metrics.save(); }, 5 * 60 * 1000); // Issue #3310: Periodically persist metering data. - const meteringSaveInterval = setInterval(() => { void metering.save(); }, 5 * 60 * 1000); + timers.setInterval(() => { void metering.save(); }, 5 * 60 * 1000); // #357: Prune stale IP rate-limit entries every minute - const ipPruneInterval = setInterval(pruneIpRateLimits, 60_000); + timers.setInterval(pruneIpRateLimits, 60_000); // #632: Prune stale auth failure rate-limit buckets every minute - const authFailPruneInterval = setInterval(pruneAuthFailLimits, 60_000); + timers.setInterval(pruneAuthFailLimits, 60_000); // #398: Sweep stale API key rate limit buckets every 5 minutes - const authSweepInterval = setInterval(() => auth.sweepStaleRateLimits(), 5 * 60_000); + timers.setInterval(() => auth.sweepStaleRateLimits(), 5 * 60_000); // #2452: Sweep expired quota usage entries every 5 minutes to prevent unbounded growth - const quotaSweepInterval = setInterval(() => routeCtx.quotas.sweep(), 5 * 60_000); + timers.setInterval(() => routeCtx.quotas.sweep(), 5 * 60_000); // Issue #4004: Start orphan action sweeper actionSweeper?.start(); // Issue #4195: Start budget evaluation timer @@ -1333,7 +1337,7 @@ async function main(): Promise { attributes: { signal }, }); - const forceExitTimer = setTimeout(() => { + const forceExitTimer = timers.setTimeout(() => { logger.error({ component: 'server', operation: 'graceful_shutdown_timeout', @@ -1381,20 +1385,13 @@ async function main(): Promise { // #1753: Close config file watcher configWatcher?.close(); configWatcher = null; - if (configReloadTimer) { clearTimeout(configReloadTimer); configReloadTimer = null; } - clearInterval(reaperInterval); - clearInterval(zombieReaperInterval); - clearInterval(metricsSaveInterval); - clearInterval(meteringSaveInterval); - clearInterval(ipPruneInterval); - clearInterval(authFailPruneInterval); - clearInterval(authSweepInterval); - clearInterval(quotaSweepInterval); + // Issue #4248: Clear all tracked timers via TimerRegistry + timers.clearAll(); + configReloadTimer = null; // Issue #4004: Stop orphan action sweeper actionSweeper?.stop(); // Issue #4195: Stop budget evaluation timer budgetTimer.stop(); - if (staticPruneInterval) clearInterval(staticPruneInterval); rateLimiter.dispose(); // 3. Close file watchers, pipelines, and reaper @@ -1590,7 +1587,9 @@ async function main(): Promise { // #3154: Dashboard static serving extracted to plugins/dashboard-static.ts // #3227: Capture prune interval handle for cleanup on shutdown + // Issue #4248: Track via TimerRegistry staticPruneInterval = await registerDashboardStatic(app, { enabled: config.dashboardEnabled !== false }); + if (staticPruneInterval) timers.track(staticPruneInterval); // track for clean shutdown await container.assertHealthy(); await listenWithRetry(app, config.port, config.host, config.stateDir); pidFilePath = await writePidFile(config.stateDir); diff --git a/src/utils/timer-registry.ts b/src/utils/timer-registry.ts new file mode 100644 index 00000000..20fb185b --- /dev/null +++ b/src/utils/timer-registry.ts @@ -0,0 +1,60 @@ +/** + * Centralized registry for setTimeout/setInterval handles. + * Enables clean bulk-clear on shutdown and prevents orphaned timers. + */ +export class TimerRegistry { + private readonly handles = new Set>(); + + /** Wrap setTimeout — auto-untracks after firing. */ + setTimeout( + fn: (...args: TArgs) => void, + ms: number, + ...args: TArgs + ): ReturnType { + const id = setTimeout(() => { + this.handles.delete(id as ReturnType); + fn(...args); + }, ms); + this.handles.add(id as ReturnType); + return id; + } + + /** Wrap setInterval — stays tracked until clearInterval or clearAll. */ + setInterval(fn: (...args: TArgs) => void, ms: number, ...args: TArgs): ReturnType { + const id = setInterval(fn, ms, ...args) as ReturnType; + this.handles.add(id); + return id; + } + + /** Clear a tracked timeout. */ + clearTimeout(id: ReturnType): void { + clearTimeout(id); + this.handles.delete(id as ReturnType); + } + + /** Clear a tracked interval. */ + clearInterval(id: ReturnType): void { + clearInterval(id); + this.handles.delete(id); + } + + + /** Track an externally-created handle (e.g. from a plugin that returns its own interval). */ + track(id: ReturnType): void { + this.handles.add(id as ReturnType); + } + + /** Clear all tracked timers. */ + clearAll(): void { + // clearTimeout also clears intervals in Node.js — using it for both is safe + for (const id of this.handles) { + clearTimeout(id); + } + this.handles.clear(); + } + + /** Number of currently active (pending) timers. */ + get activeCount(): number { + return this.handles.size; + } +}