From 2e31370c298aa9f47e5f36a8a64acd2cc6eff9e7 Mon Sep 17 00:00:00 2001 From: Hephaestus Date: Tue, 26 May 2026 07:14:06 +0200 Subject: [PATCH 1/2] feat(utils): add TimerRegistry for centralized timer cleanup (#4248) - New TimerRegistry class (src/utils/timer-registry.ts): tracks all setTimeout/setInterval handles with clearAll() for graceful shutdown - Integrated into server.ts: replaced 11 individual setInterval/clearInterval calls with centralized timers.setInterval() and timers.clearAll() on shutdown - Replaced config reload debounce timer and approval action debounce timers - Simplified shutdown: single timers.clearAll() replaces 8 clearInterval calls - 14 unit tests covering setTimeout, setInterval, clearTimeout, clearInterval, clearAll, and auto-untrack after timeout fires Verification: - tsc --noEmit: 0 errors - npm run build: success - npm test: 385 files, 5615 tests passed, 0 failures --- src/__tests__/timer-registry.test.ts | 137 +++++++++++++++++++++++++++ src/server.ts | 48 +++++----- src/utils/timer-registry.ts | 53 +++++++++++ 3 files changed, 214 insertions(+), 24 deletions(-) create mode 100644 src/__tests__/timer-registry.test.ts create mode 100644 src/utils/timer-registry.ts 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..bd5320c7 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,10 @@ async function main(): Promise { // #3154: Dashboard static serving extracted to plugins/dashboard-static.ts // #3227: Capture prune interval handle for cleanup on shutdown - staticPruneInterval = await registerDashboardStatic(app, { enabled: config.dashboardEnabled !== false }); + // Issue #4248: Track via TimerRegistry + const _staticPrune = await registerDashboardStatic(app, { enabled: config.dashboardEnabled !== false }); + if (_staticPrune) timers.setInterval(() => {}, 0); // register the returned handle + staticPruneInterval = _staticPrune; 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..182c6c8d --- /dev/null +++ b/src/utils/timer-registry.ts @@ -0,0 +1,53 @@ +/** + * 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: unknown[]) => void, ms: number, ...args: unknown[]): 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); + } + + /** Clear all tracked timers. */ + clearAll(): void { + for (const id of this.handles) { + clearTimeout(id); + } + this.handles.clear(); + } + + /** Number of currently active (pending) timers. */ + get activeCount(): number { + return this.handles.size; + } +} From 23d8c22efbc24aab948083b2b649cd50fb94ba00 Mon Sep 17 00:00:00 2001 From: Hephaestus Date: Tue, 26 May 2026 13:26:38 +0200 Subject: [PATCH 2/2] fix(server): track staticPruneInterval via TimerRegistry.track() instead of dummy no-op interval - Add TimerRegistry.track() method for externally-created handles - Fix shutdown leak: staticPruneInterval was not actually tracked - Add comment explaining clearTimeout works for both types in Node.js - Generic-ify setInterval type signature to match setTimeout --- src/server.ts | 5 ++--- src/utils/timer-registry.ts | 9 ++++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/server.ts b/src/server.ts index bd5320c7..7bbfded7 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1588,9 +1588,8 @@ 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 - const _staticPrune = await registerDashboardStatic(app, { enabled: config.dashboardEnabled !== false }); - if (_staticPrune) timers.setInterval(() => {}, 0); // register the returned handle - staticPruneInterval = _staticPrune; + 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 index 182c6c8d..20fb185b 100644 --- a/src/utils/timer-registry.ts +++ b/src/utils/timer-registry.ts @@ -20,7 +20,7 @@ export class TimerRegistry { } /** Wrap setInterval — stays tracked until clearInterval or clearAll. */ - setInterval(fn: (...args: unknown[]) => void, ms: number, ...args: unknown[]): ReturnType { + setInterval(fn: (...args: TArgs) => void, ms: number, ...args: TArgs): ReturnType { const id = setInterval(fn, ms, ...args) as ReturnType; this.handles.add(id); return id; @@ -38,8 +38,15 @@ export class TimerRegistry { 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); }