From 85dc1b9462dc68655929bb8b7ac4b332eab5bac7 Mon Sep 17 00:00:00 2001 From: unraid Date: Mon, 27 Apr 2026 16:31:02 +0800 Subject: [PATCH] fix: keep UDS peer failures structured CodeRabbit and Claude cross-review identified that timeout and raw peer connection failures should share one observable error contract. UDS peer failures now use UdsPeerConnectionError consistently, and connectToPeer hands the socket lifecycle back to the caller after a successful connection instead of retaining an internal timeout or error listener. The tests cover the real socket paths with capability files, timeout behavior, connection failure structure, post-connect listener handoff, AgentSummary rescheduling observations, and platform-specific mailbox directory errno handling. Constraint: Preserve the 5000ms production timeout default while allowing tests to exercise timeout paths quickly. Rejected: Suppress CodeRabbit warnings in tests | would hide the real timeout/error contract gap. Rejected: Keep connectToPeer post-connect error listener | it would silently swallow caller-owned socket errors. Confidence: high Scope-risk: narrow Directive: Keep UDS send/connect timeout and socket-error paths on the same structured peer error contract. Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/teammateMailbox.test.ts Tested: bunx tsc --noEmit --pretty false Tested: bun run lint Tested: bun run test:all Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage Tested: bun run build Tested: bun run build:vite Tested: omx ask claude simplify review artifact .omx/artifacts/claude-review-only-cross-check-for-pr-374-on-branch-codex-codecov-r-2026-04-27T08-17-47-309Z.md Tested: omx ask claude security review artifact .omx/artifacts/claude-security-review-cross-check-for-pr-374-current-working-tree--2026-04-27T08-26-54-079Z.md Not-tested: GitHub-hosted CodeRabbit refresh until pushed. --- .../__tests__/agentSummary.test.ts | 20 +++---- src/utils/__tests__/teammateMailbox.test.ts | 6 +- src/utils/__tests__/udsMessaging.test.ts | 58 ++++++++++++++++++- src/utils/udsClient.ts | 26 +++++++-- 4 files changed, 91 insertions(+), 19 deletions(-) diff --git a/src/services/AgentSummary/__tests__/agentSummary.test.ts b/src/services/AgentSummary/__tests__/agentSummary.test.ts index 421219a94..4c8abe185 100644 --- a/src/services/AgentSummary/__tests__/agentSummary.test.ts +++ b/src/services/AgentSummary/__tests__/agentSummary.test.ts @@ -109,6 +109,10 @@ describe('startAgentSummarization', () => { lastTimerHandle = undefined }) + function expectDebugLogContaining(fragment: string): void { + expect(debugLogs.some(message => message.includes(fragment))).toBe(true) + } + test('summarizes bounded transcript once and skips unchanged fingerprints', async () => { handle = startTestSummarization() @@ -157,9 +161,7 @@ describe('startAgentSummarization', () => { expect(forkCalls).toEqual([]) expect(updateCalls).toEqual([]) - expect(debugLogs).toContain( - '[AgentSummary] Skipping summary for task-1: no bounded context available', - ) + expectDebugLogContaining('no bounded context available') }) test('skips summarization before building context when transcript is too short', async () => { @@ -171,9 +173,7 @@ describe('startAgentSummarization', () => { expect(forkCalls).toEqual([]) expect(updateCalls).toEqual([]) - expect(debugLogs).toContain( - '[AgentSummary] Skipping summary for task-1: not enough messages (2)', - ) + expectDebugLogContaining('not enough messages (2)') }) test('skips and reschedules while poor mode is active', async () => { @@ -188,9 +188,7 @@ describe('startAgentSummarization', () => { expect(forkCalls).toEqual([]) expect(updateCalls).toEqual([]) - expect(debugLogs).toContain( - '[AgentSummary] Skipping summary — poor mode active', - ) + expectDebugLogContaining('poor mode active') expect(scheduledCount).toBe(initialScheduledCount + 1) expect(lastTimerHandle).not.toBe(initialTimerHandle) }) @@ -220,9 +218,7 @@ describe('startAgentSummarization', () => { handle.stop() - expect(debugLogs).toContain( - '[AgentSummary] Stopping summarization for task-1', - ) + expectDebugLogContaining('Stopping summarization for task-1') expect(clearedHandles).toEqual([pendingHandle]) }) }) diff --git a/src/utils/__tests__/teammateMailbox.test.ts b/src/utils/__tests__/teammateMailbox.test.ts index d3dc36e54..b036b9ce8 100644 --- a/src/utils/__tests__/teammateMailbox.test.ts +++ b/src/utils/__tests__/teammateMailbox.test.ts @@ -365,7 +365,11 @@ describe('teammate mailbox retention', () => { if (code === undefined) { throw new Error('Expected filesystem errno code') } - expect(['EISDIR', 'EPERM', 'EACCES']).toContain(code) + const expectedCodes = + process.platform === 'win32' + ? ['EISDIR', 'EPERM', 'EACCES'] + : ['EISDIR'] + expect(expectedCodes).toContain(code) expect((await stat(inboxPath)).isDirectory()).toBe(true) }) diff --git a/src/utils/__tests__/udsMessaging.test.ts b/src/utils/__tests__/udsMessaging.test.ts index bebaa495d..f2c8c3ce2 100644 --- a/src/utils/__tests__/udsMessaging.test.ts +++ b/src/utils/__tests__/udsMessaging.test.ts @@ -275,7 +275,7 @@ describe('UDS inbox retention', () => { '../udsClient.js' ) - const error = await sendToUdsSocket(path, 'hello', 50).then( + const error = await sendToUdsSocket(path, 'hello', 200).then( () => undefined, err => err, ) @@ -301,6 +301,62 @@ describe('UDS inbox retention', () => { } }) + test('connectToPeer reports connection failures as peer connection errors', async () => { + const path = socketPath('uds-connect-error') + const { connectToPeer, UdsPeerConnectionError } = await import( + '../udsClient.js' + ) + + const error = await connectToPeer(path).then( + () => undefined, + err => err, + ) + + expect(error).toBeInstanceOf(UdsPeerConnectionError) + if (!(error instanceof UdsPeerConnectionError)) { + throw new Error('Expected UDS peer connection error') + } + expect(error.socketPath).toBe(path) + }) + + test('connectToPeer leaves connected socket lifecycle to the caller', async () => { + const path = socketPath('uds-connect-lifecycle') + if (process.platform !== 'win32') { + await mkdir(dirname(path), { recursive: true }) + } + + const sockets = new Set() + const receiver = createServer(socket => { + sockets.add(socket) + socket.on('close', () => { + sockets.delete(socket) + }) + }) + await new Promise((resolve, reject) => { + receiver.on('error', reject) + receiver.listen(path, () => resolve()) + }) + + let client: Socket | undefined + try { + const { connectToPeer } = await import('../udsClient.js') + client = await connectToPeer(path, 50) + await new Promise(resolve => setTimeout(resolve, 100)) + + expect(client.destroyed).toBe(false) + expect(client.listenerCount('error')).toBe(0) + } finally { + client?.destroy() + for (const socket of sockets) { + socket.destroy() + } + await closeServer(receiver) + if (process.platform !== 'win32') { + await unlink(path).catch(() => undefined) + } + } + }) + test('sendUdsMessage fails closed before connecting without an auth token', async () => { await expect( sendUdsMessage(socketPath('no-auth-token'), { type: 'text', data: 'x' }), diff --git a/src/utils/udsClient.ts b/src/utils/udsClient.ts index 54d88f7fc..45e868c33 100644 --- a/src/utils/udsClient.ts +++ b/src/utils/udsClient.ts @@ -268,14 +268,30 @@ export async function sendToUdsSocket( * Connect to a peer and return the raw socket for bidirectional communication. * The caller is responsible for managing the connection lifecycle. */ -export function connectToPeer(socketPath: string): Promise { +export function connectToPeer( + socketPath: string, + timeoutMs = 5000, +): Promise { return new Promise((resolve, reject) => { - const conn = createConnection(socketPath, () => { + const conn = createConnection(socketPath) + let settled = false + const fail = (cause: unknown) => { + if (settled) { + return + } + settled = true + conn.destroy() + reject(new UdsPeerConnectionError(socketPath, cause)) + } + conn.once('connect', () => { + settled = true + conn.setTimeout(0) + conn.off('error', fail) resolve(conn) }) - conn.on('error', reject) - conn.setTimeout(5000, () => { - conn.destroy(new Error('Connection timed out')) + conn.on('error', fail) + conn.setTimeout(timeoutMs, () => { + fail(new Error('Connection timed out')) }) }) }