refactor(utils): add TimerRegistry for centralized timer cleanup (#4248)

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);
+    });
+  });
+});

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<string>();
+// 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<void> {
         // 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<void> {
         // 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<void> {
   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<void> {
       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<void> {
       // #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<void> {
 
   // #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);

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<ReturnType<typeof setTimeout | typeof setInterval>>();
+
+  /** Wrap setTimeout — auto-untracks after firing. */
+  setTimeout<TArgs extends unknown[]>(
+    fn: (...args: TArgs) => void,
+    ms: number,
+    ...args: TArgs
+  ): ReturnType<typeof setTimeout> {
+    const id = setTimeout(() => {
+      this.handles.delete(id as ReturnType<typeof setInterval>);
+      fn(...args);
+    }, ms);
+    this.handles.add(id as ReturnType<typeof setInterval>);
+    return id;
+  }
+
+  /** Wrap setInterval — stays tracked until clearInterval or clearAll. */
+  setInterval<TArgs extends unknown[]>(fn: (...args: TArgs) => void, ms: number, ...args: TArgs): ReturnType<typeof setInterval> {
+    const id = setInterval(fn, ms, ...args) as ReturnType<typeof setInterval>;
+    this.handles.add(id);
+    return id;
+  }
+
+  /** Clear a tracked timeout. */
+  clearTimeout(id: ReturnType<typeof setTimeout>): void {
+    clearTimeout(id);
+    this.handles.delete(id as ReturnType<typeof setInterval>);
+  }
+
+  /** Clear a tracked interval. */
+  clearInterval(id: ReturnType<typeof setInterval>): 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<typeof setTimeout | typeof setInterval>): void {
+    this.handles.add(id as ReturnType<typeof setInterval>);
+  }
+
+  /** 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;
+  }
+}