diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5c57484ad..5fa5853c5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -182,7 +182,7 @@ jobs: kill "$AEGIS_PID" || true fi - name: Check bundle size - run: "THRESHOLD_KB=2370\nSERVER_SIZE=$(find dist/ -name \"*.js\" ! -path \"\ + run: "THRESHOLD_KB=2375\nSERVER_SIZE=$(find dist/ -name \"*.js\" ! -path \"\ */__tests__/*\" ! -path \"*/dashboard/*\" -exec du -ck {} + | tail -1 | awk '{print $1}')\nSERVER_SIZE_KB=$((SERVER_SIZE))\n\ echo \"## Bundle Size Report\" >> \"$GITHUB_STEP_SUMMARY\"\necho \"\" >> \"\ $GITHUB_STEP_SUMMARY\"\necho \"| Scope | Size (KB) | Threshold (KB) | Status\ @@ -330,7 +330,7 @@ jobs: } - name: Check bundle size if: runner.os != 'Windows' - run: "THRESHOLD_KB=2370\nSERVER_SIZE=$(find dist/ -name \"*.js\" ! -path \"\ + run: "THRESHOLD_KB=2375\nSERVER_SIZE=$(find dist/ -name \"*.js\" ! -path \"\ */__tests__/*\" ! -path \"*/dashboard/*\" -exec du -ck {} + | tail -1 | awk '{print $1}')\nSERVER_SIZE_KB=$((SERVER_SIZE))\n\ echo \"## Bundle Size Report\" >> \"$GITHUB_STEP_SUMMARY\"\necho \"\" >> \"\ $GITHUB_STEP_SUMMARY\"\necho \"| Scope | Size (KB) | Threshold (KB) | Status\ diff --git a/src/__tests__/timer-registry.test.ts b/src/__tests__/timer-registry.test.ts new file mode 100644 index 000000000..28a87916d --- /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 274a33a94..a8cc0faa3 100644 --- a/src/server.ts +++ b/src/server.ts @@ -75,6 +75,7 @@ import { InMemoryPauseInterventionStore } from './services/acp/in-memory-pause-i import { isWindowsShutdownMessage, parseShutdownTimeoutMs } from './shutdown-utils.js'; import { ServiceContainer } from './container.js'; import type { AppContext } from './app-context.js'; +import { TimerRegistry } from './utils/timer-registry.js'; import { AcpBackend } from './services/acp/backend.js'; import { AcpSessionService } from './services/acp/session-service.js'; import { AcpTerminalBridge } from './services/acp/terminal-bridge.js'; @@ -145,6 +146,8 @@ declare module 'fastify' { // Module-level const values are still initialized here. const channels = new ChannelManager(); const eventBus = new SessionEventBus(); +// Issue #4248: Centralized timer registry for clean shutdown +const timers = new TimerRegistry(); // Issue #4116: Debounce Set for session approval callbacks (prevents duplicate notifications from rapid Telegram clicks). const recentApprovalActions = new Set(); @@ -771,8 +774,8 @@ function setupConfigWatcher(ctx: AppContext): void { ctx.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 (ctx.configReloadTimer) clearTimeout(ctx.configReloadTimer); - ctx.configReloadTimer = setTimeout(() => { + if (ctx.configReloadTimer) timers.clearTimeout(ctx.configReloadTimer); + ctx.configReloadTimer = timers.setTimeout(() => { void handleConfigReload('file-change', ctx); }, 300); }); @@ -1277,17 +1280,17 @@ registerBudgetRoutes(app, { auth: ctx.auth, budgetStore, budgetEvaluator }); registerOpenApiRoute(app); // Issue #361: Store interval refs so graceful shutdown can clear them - const reaperInterval = setInterval(() => reapStaleSessions(ctx.config.maxSessionAgeMs, ctx), ctx.config.reaperIntervalMs); - const zombieReaperInterval = setInterval(() => reapZombieSessions(ctx), ZOMBIE_REAP_INTERVAL_MS); -const metricsSaveInterval = setInterval(() => { void ctx.metrics.save(); }, 5 * 60 * 1000); + timers.setInterval(() => reapStaleSessions(ctx.config.maxSessionAgeMs, ctx), ctx.config.reaperIntervalMs); + timers.setInterval(() => reapZombieSessions(ctx), ZOMBIE_REAP_INTERVAL_MS); +timers.setInterval(() => { void ctx.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(() => ctx.auth.sweepStaleRateLimits(), 5 * 60_000); +timers.setInterval(() => ctx.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); // Issue #4004: Start orphan action sweeper @@ -1295,6 +1298,7 @@ ctx.actionSweeper?.start(); // Issue #4195: Start budget evaluation timer budgetTimer.start(); // #3227: Prune interval from StaticRateLimiter — assigned after registerDashboardStatic() + // Issue #4248: staticPruneInterval tracked via timers.track() after registration let staticPruneInterval: ReturnType | null = null; let pidFilePath = ''; @@ -1360,20 +1364,13 @@ ctx.actionSweeper?.start(); // #1753: Close config file watcher ctx.configWatcher?.close(); ctx.configWatcher = null; -if (ctx.configReloadTimer) { clearTimeout(ctx.configReloadTimer); ctx.configReloadTimer = null; } - clearInterval(reaperInterval); - clearInterval(zombieReaperInterval); - clearInterval(metricsSaveInterval); - clearInterval(meteringSaveInterval); - clearInterval(ipPruneInterval); - clearInterval(authFailPruneInterval); - clearInterval(authSweepInterval); - clearInterval(quotaSweepInterval); +if (ctx.configReloadTimer) { timers.clearTimeout(ctx.configReloadTimer); ctx.configReloadTimer = null; } + // Issue #4248: Clear all tracked timers via TimerRegistry + timers.clearAll(); // Issue #4004: Stop orphan action sweeper ctx.actionSweeper?.stop(); // Issue #4195: Stop budget evaluation timer budgetTimer.stop(); - if (staticPruneInterval) clearInterval(staticPruneInterval); rateLimiter.dispose(); // 3. Close file watchers, pipelines, and reaper @@ -1570,6 +1567,7 @@ ctx.auditLogger?.flush() ?? Promise.resolve(), // #3154: Dashboard static serving extracted to plugins/dashboard-static.ts // #3227: Capture prune interval handle for cleanup on shutdown staticPruneInterval = await registerDashboardStatic(app, { enabled: ctx.config.dashboardEnabled !== false }); + if (staticPruneInterval) timers.track(staticPruneInterval); await container.assertHealthy(); await listenWithRetry(app, ctx.config.port, ctx.config.host, ctx.config.stateDir); pidFilePath = await writePidFile(ctx.config.stateDir); diff --git a/src/utils/timer-registry.ts b/src/utils/timer-registry.ts new file mode 100644 index 000000000..20fb185b6 --- /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; + } +}