fix(slack): bound username cache with ttl and lru eviction
This commit is contained in:
@@ -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-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-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-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-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-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.
|
- ✅ 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-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-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-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
|
## Executive Summary
|
||||||
|
|
||||||
Current health snapshot:
|
Current health snapshot:
|
||||||
- `pnpm typecheck`: passing
|
- `pnpm typecheck`: passing
|
||||||
- `pnpm build`: 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`)
|
- `pnpm lint`: failing (`148 errors`, `530 warnings`)
|
||||||
|
|
||||||
Top conclusions:
|
Top conclusions:
|
||||||
@@ -235,6 +236,10 @@ Remediation update (2026-02-16):
|
|||||||
- Recommended fix:
|
- Recommended fix:
|
||||||
- Use LRU/TTL cache with maximum entry count.
|
- 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`
|
### F-012 Low-Medium: Synthetic tool nudge uses invalid `tool_use_id`
|
||||||
|
|
||||||
- Severity: Low-Medium
|
- Severity: Low-Medium
|
||||||
@@ -283,6 +288,9 @@ Remediation update (2026-02-16):
|
|||||||
- Recommended fix:
|
- Recommended fix:
|
||||||
- Centralize in shared utility with size-limit support.
|
- 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
|
### F-017 Low: NativeAgent history getter returns mixed mutability semantics
|
||||||
|
|
||||||
- Severity: Low
|
- Severity: Low
|
||||||
|
|||||||
+13
-1
@@ -2601,10 +2601,22 @@
|
|||||||
"docs/plans/analysis/2026-02-16-codebase-audit-report.md"
|
"docs/plans/analysis/2026-02-16-codebase-audit-report.md"
|
||||||
],
|
],
|
||||||
"test_status": "pnpm test:run src/models/retry.test.ts + pnpm typecheck passing"
|
"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": {
|
"overall_progress": {
|
||||||
"total_test_count": 1765,
|
"total_test_count": 1770,
|
||||||
"all_tests_passing": true,
|
"all_tests_passing": true,
|
||||||
"p0_completion": "3/3 (100%)",
|
"p0_completion": "3/3 (100%)",
|
||||||
"p1_completion": "4/4 (100%)",
|
"p1_completion": "4/4 (100%)",
|
||||||
|
|||||||
@@ -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 ──────────────────────
|
// ── Mock @slack/bolt before importing adapter ──────────────────────
|
||||||
|
|
||||||
@@ -66,6 +66,10 @@ describe('SlackAdapter', () => {
|
|||||||
adapter = new SlackAdapter(baseConfig);
|
adapter = new SlackAdapter(baseConfig);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
vi.useRealTimers();
|
||||||
|
});
|
||||||
|
|
||||||
// ── Basic properties ──────────────────────────────────────────
|
// ── Basic properties ──────────────────────────────────────────
|
||||||
|
|
||||||
it('has name "slack"', () => {
|
it('has name "slack"', () => {
|
||||||
@@ -221,6 +225,52 @@ describe('SlackAdapter', () => {
|
|||||||
expect(msg.senderId).toBe('C123:2222.3333');
|
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 () => {
|
it('ignores bot messages (bot_id present)', async () => {
|
||||||
const handler = vi.fn();
|
const handler = vi.fn();
|
||||||
adapter.onMessage(handler);
|
adapter.onMessage(handler);
|
||||||
|
|||||||
@@ -31,6 +31,11 @@ export interface SlackAdapterConfig {
|
|||||||
pairingManager?: PairingManager;
|
pairingManager?: PairingManager;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
interface CachedUserName {
|
||||||
|
name: string;
|
||||||
|
expiresAt: number;
|
||||||
|
}
|
||||||
|
|
||||||
/** Minimal shape of a Slack message event from Bolt. */
|
/** Minimal shape of a Slack message event from Bolt. */
|
||||||
interface SlackMessageEvent {
|
interface SlackMessageEvent {
|
||||||
ts?: string;
|
ts?: string;
|
||||||
@@ -63,8 +68,10 @@ export class SlackAdapter implements ChannelAdapter {
|
|||||||
private app: App | null = null;
|
private app: App | null = null;
|
||||||
private messageHandler?: (msg: InboundMessage) => void;
|
private messageHandler?: (msg: InboundMessage) => void;
|
||||||
private config: SlackAdapterConfig;
|
private config: SlackAdapterConfig;
|
||||||
private userNameCache: Map<string, string> = new Map();
|
private userNameCache: Map<string, CachedUserName> = new Map();
|
||||||
private botUserId?: string;
|
private botUserId?: string;
|
||||||
|
private readonly userNameCacheTtlMs = 60 * 60 * 1_000;
|
||||||
|
private readonly userNameCacheMaxEntries = 1_000;
|
||||||
|
|
||||||
get status(): ChannelStatus {
|
get status(): ChannelStatus {
|
||||||
return this._status;
|
return this._status;
|
||||||
@@ -199,13 +206,28 @@ export class SlackAdapter implements ChannelAdapter {
|
|||||||
|
|
||||||
/** Resolve a Slack user ID to a display name, with caching. */
|
/** Resolve a Slack user ID to a display name, with caching. */
|
||||||
private async resolveUserName(userId: string): Promise<string> {
|
private async resolveUserName(userId: string): Promise<string> {
|
||||||
|
const now = Date.now();
|
||||||
const cached = this.userNameCache.get(userId);
|
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 {
|
try {
|
||||||
const result = await this.app!.client.users.info({ user: userId });
|
const result = await this.app!.client.users.info({ user: userId });
|
||||||
const name = result.user?.real_name || result.user?.name || 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;
|
return name;
|
||||||
} catch {
|
} catch {
|
||||||
return userId;
|
return userId;
|
||||||
|
|||||||
Reference in New Issue
Block a user