From 05d8abc79d3908a2407565282c1242cb2d2cd912 Mon Sep 17 00:00:00 2001 From: William Valentin Date: Sun, 15 Feb 2026 22:21:06 -0800 Subject: [PATCH] fix(slack): bound username cache with ttl and lru eviction --- .../2026-02-16-codebase-audit-report.md | 12 ++++- docs/plans/state.json | 14 ++++- src/channels/slack/adapter.test.ts | 52 ++++++++++++++++++- src/channels/slack/adapter.ts | 28 ++++++++-- 4 files changed, 99 insertions(+), 7 deletions(-) diff --git a/docs/plans/analysis/2026-02-16-codebase-audit-report.md b/docs/plans/analysis/2026-02-16-codebase-audit-report.md index 3c48cc3..b375b0a 100644 --- a/docs/plans/analysis/2026-02-16-codebase-audit-report.md +++ b/docs/plans/analysis/2026-02-16-codebase-audit-report.md @@ -8,7 +8,7 @@ Scope: Production-risk-first audit of bugs, code improvements, and feature oppor - ✅ F-001 addressed: chat markdown rendering now sanitizes HTML before DOM insertion in `src/gateway/ui/pages/chat.js` (and legacy `src/gateway/ui/chat.html`). - ✅ F-006 addressed: inbound HTTP request bodies now enforce a configurable max-size limit (`server.max_request_body_bytes`) with `413 Payload Too Large` responses. - ✅ F-007 addressed: `ToolExecutor` timeout timer handles are now cleared in `finally`, preventing orphan timers on fast/failed tool calls. -- ✅ F-016 partially addressed: gateway + webhook body readers were consolidated into shared utility `src/utils/httpBody.ts` with size-limit enforcement. +- ✅ F-016 addressed: gateway + webhook body readers are consolidated in shared utility `src/utils/httpBody.ts` with size-limit enforcement. - ✅ F-005 addressed: ESLint JS globals now include `FileReader`, removing UI false-positive lint failures for attachment handling code. - ✅ F-010 addressed: `session.compact` audit events now emit actual message counts for `messages_before/messages_after` (tokens remain in token fields). - ✅ F-012 addressed: synthetic repeated-tool nudge no longer emits invalid `tool_result.tool_use_id`; nudge is injected as plain user text guidance. @@ -18,13 +18,14 @@ Scope: Production-risk-first audit of bugs, code improvements, and feature oppor - ✅ F-002 addressed: `config.patch` now supports durable persistence via atomic write + backup when daemon has a concrete config path, and response includes `persisted`/`persistError` so UI can distinguish runtime-only vs disk-persisted updates. - ✅ F-003 addressed: tool execution now has an `AbortSignal` contract, executor triggers abort on timeout, high-risk tools (`shell.exec`, sandbox docker exec, `process.start`, browser tools, `web.fetch`, `web.search`) respond to cancellation, and executor regression tests verify cancellable tools do not apply side effects after timeout. - ✅ F-015 addressed: retry defaults no longer classify timeout-style failures as non-retryable, improving resilience for transient timeout conditions. +- ✅ F-011 addressed: Slack user-name resolution now uses bounded TTL+LRU caching to prevent unbounded growth. ## Executive Summary Current health snapshot: - `pnpm typecheck`: passing - `pnpm build`: passing -- `pnpm test:run`: passing (`140/140` files, `1765/1765` tests) +- `pnpm test:run`: passing (`140/140` files, `1770/1770` tests) - `pnpm lint`: failing (`148 errors`, `530 warnings`) Top conclusions: @@ -235,6 +236,10 @@ Remediation update (2026-02-16): - Recommended fix: - Use LRU/TTL cache with maximum entry count. +Remediation update (2026-02-16): +- `SlackAdapter` user-name cache now has TTL expiry and max-entry LRU eviction behavior. +- Added cache regression tests for cache-hit reuse and TTL refresh in `src/channels/slack/adapter.test.ts`. + ### F-012 Low-Medium: Synthetic tool nudge uses invalid `tool_use_id` - Severity: Low-Medium @@ -283,6 +288,9 @@ Remediation update (2026-02-16): - Recommended fix: - Centralize in shared utility with size-limit support. +Remediation update (2026-02-16): +- Request body parsing is centralized in `src/utils/httpBody.ts` and consumed by both gateway server and webhook handler paths. + ### F-017 Low: NativeAgent history getter returns mixed mutability semantics - Severity: Low diff --git a/docs/plans/state.json b/docs/plans/state.json index 1174b38..931e18d 100644 --- a/docs/plans/state.json +++ b/docs/plans/state.json @@ -2601,10 +2601,22 @@ "docs/plans/analysis/2026-02-16-codebase-audit-report.md" ], "test_status": "pnpm test:run src/models/retry.test.ts + pnpm typecheck passing" + }, + "audit-followup-slack-username-cache-bounds": { + "status": "completed", + "date": "2026-02-16", + "updated": "2026-02-16", + "summary": "Added bounded Slack user-name caching with TTL expiry and LRU eviction to prevent unbounded cache growth in long-running deployments.", + "files_modified": [ + "src/channels/slack/adapter.ts", + "src/channels/slack/adapter.test.ts", + "docs/plans/analysis/2026-02-16-codebase-audit-report.md" + ], + "test_status": "pnpm test:run src/channels/slack/adapter.test.ts + pnpm typecheck passing" } }, "overall_progress": { - "total_test_count": 1765, + "total_test_count": 1770, "all_tests_passing": true, "p0_completion": "3/3 (100%)", "p1_completion": "4/4 (100%)", diff --git a/src/channels/slack/adapter.test.ts b/src/channels/slack/adapter.test.ts index 2506373..b57fe1f 100644 --- a/src/channels/slack/adapter.test.ts +++ b/src/channels/slack/adapter.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; // ── Mock @slack/bolt before importing adapter ────────────────────── @@ -66,6 +66,10 @@ describe('SlackAdapter', () => { adapter = new SlackAdapter(baseConfig); }); + afterEach(() => { + vi.useRealTimers(); + }); + // ── Basic properties ────────────────────────────────────────── it('has name "slack"', () => { @@ -221,6 +225,52 @@ describe('SlackAdapter', () => { expect(msg.senderId).toBe('C123:2222.3333'); }); + it('caches resolved usernames and avoids repeated users.info calls', async () => { + const handler = vi.fn(); + adapter.onMessage(handler); + await adapter.connect(); + + await simulateMessage({ + ts: '1111.0001', + channel: 'C123', + user: 'U456', + text: 'First message', + }); + await simulateMessage({ + ts: '1111.0002', + channel: 'C123', + user: 'U456', + text: 'Second message', + }); + + expect(mockUsersInfo).toHaveBeenCalledTimes(1); + }); + + it('refreshes cached usernames after TTL expiry', async () => { + vi.useFakeTimers(); + const handler = vi.fn(); + adapter.onMessage(handler); + await adapter.connect(); + + await simulateMessage({ + ts: '2222.0001', + channel: 'C123', + user: 'U456', + text: 'First message', + }); + expect(mockUsersInfo).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(60 * 60 * 1_000 + 1); + + await simulateMessage({ + ts: '2222.0002', + channel: 'C123', + user: 'U456', + text: 'Second message after TTL', + }); + expect(mockUsersInfo).toHaveBeenCalledTimes(2); + }); + it('ignores bot messages (bot_id present)', async () => { const handler = vi.fn(); adapter.onMessage(handler); diff --git a/src/channels/slack/adapter.ts b/src/channels/slack/adapter.ts index 6c7e5fb..00711a8 100644 --- a/src/channels/slack/adapter.ts +++ b/src/channels/slack/adapter.ts @@ -31,6 +31,11 @@ export interface SlackAdapterConfig { pairingManager?: PairingManager; } +interface CachedUserName { + name: string; + expiresAt: number; +} + /** Minimal shape of a Slack message event from Bolt. */ interface SlackMessageEvent { ts?: string; @@ -63,8 +68,10 @@ export class SlackAdapter implements ChannelAdapter { private app: App | null = null; private messageHandler?: (msg: InboundMessage) => void; private config: SlackAdapterConfig; - private userNameCache: Map = new Map(); + private userNameCache: Map = new Map(); private botUserId?: string; + private readonly userNameCacheTtlMs = 60 * 60 * 1_000; + private readonly userNameCacheMaxEntries = 1_000; get status(): ChannelStatus { return this._status; @@ -199,13 +206,28 @@ export class SlackAdapter implements ChannelAdapter { /** Resolve a Slack user ID to a display name, with caching. */ private async resolveUserName(userId: string): Promise { + const now = Date.now(); const cached = this.userNameCache.get(userId); - if (cached) {return cached;} + if (cached && cached.expiresAt > now) { + // Refresh LRU order on cache hit. + this.userNameCache.delete(userId); + this.userNameCache.set(userId, cached); + return cached.name; + } + if (cached) { + this.userNameCache.delete(userId); + } try { const result = await this.app!.client.users.info({ user: userId }); const name = result.user?.real_name || result.user?.name || userId; - this.userNameCache.set(userId, name); + this.userNameCache.set(userId, { name, expiresAt: now + this.userNameCacheTtlMs }); + if (this.userNameCache.size > this.userNameCacheMaxEntries) { + const oldestKey = this.userNameCache.keys().next().value; + if (typeof oldestKey === 'string') { + this.userNameCache.delete(oldestKey); + } + } return name; } catch { return userId;