Skip to content

Commit 85dc1b9

Browse files
author
unraid
committed
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.
1 parent b47731a commit 85dc1b9

4 files changed

Lines changed: 91 additions & 19 deletions

File tree

src/services/AgentSummary/__tests__/agentSummary.test.ts

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,10 @@ describe('startAgentSummarization', () => {
109109
lastTimerHandle = undefined
110110
})
111111

112+
function expectDebugLogContaining(fragment: string): void {
113+
expect(debugLogs.some(message => message.includes(fragment))).toBe(true)
114+
}
115+
112116
test('summarizes bounded transcript once and skips unchanged fingerprints', async () => {
113117
handle = startTestSummarization()
114118

@@ -157,9 +161,7 @@ describe('startAgentSummarization', () => {
157161

158162
expect(forkCalls).toEqual([])
159163
expect(updateCalls).toEqual([])
160-
expect(debugLogs).toContain(
161-
'[AgentSummary] Skipping summary for task-1: no bounded context available',
162-
)
164+
expectDebugLogContaining('no bounded context available')
163165
})
164166

165167
test('skips summarization before building context when transcript is too short', async () => {
@@ -171,9 +173,7 @@ describe('startAgentSummarization', () => {
171173

172174
expect(forkCalls).toEqual([])
173175
expect(updateCalls).toEqual([])
174-
expect(debugLogs).toContain(
175-
'[AgentSummary] Skipping summary for task-1: not enough messages (2)',
176-
)
176+
expectDebugLogContaining('not enough messages (2)')
177177
})
178178

179179
test('skips and reschedules while poor mode is active', async () => {
@@ -188,9 +188,7 @@ describe('startAgentSummarization', () => {
188188

189189
expect(forkCalls).toEqual([])
190190
expect(updateCalls).toEqual([])
191-
expect(debugLogs).toContain(
192-
'[AgentSummary] Skipping summary — poor mode active',
193-
)
191+
expectDebugLogContaining('poor mode active')
194192
expect(scheduledCount).toBe(initialScheduledCount + 1)
195193
expect(lastTimerHandle).not.toBe(initialTimerHandle)
196194
})
@@ -220,9 +218,7 @@ describe('startAgentSummarization', () => {
220218

221219
handle.stop()
222220

223-
expect(debugLogs).toContain(
224-
'[AgentSummary] Stopping summarization for task-1',
225-
)
221+
expectDebugLogContaining('Stopping summarization for task-1')
226222
expect(clearedHandles).toEqual([pendingHandle])
227223
})
228224
})

src/utils/__tests__/teammateMailbox.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,11 @@ describe('teammate mailbox retention', () => {
365365
if (code === undefined) {
366366
throw new Error('Expected filesystem errno code')
367367
}
368-
expect(['EISDIR', 'EPERM', 'EACCES']).toContain(code)
368+
const expectedCodes =
369+
process.platform === 'win32'
370+
? ['EISDIR', 'EPERM', 'EACCES']
371+
: ['EISDIR']
372+
expect(expectedCodes).toContain(code)
369373
expect((await stat(inboxPath)).isDirectory()).toBe(true)
370374
})
371375

src/utils/__tests__/udsMessaging.test.ts

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ describe('UDS inbox retention', () => {
275275
'../udsClient.js'
276276
)
277277

278-
const error = await sendToUdsSocket(path, 'hello', 50).then(
278+
const error = await sendToUdsSocket(path, 'hello', 200).then(
279279
() => undefined,
280280
err => err,
281281
)
@@ -301,6 +301,62 @@ describe('UDS inbox retention', () => {
301301
}
302302
})
303303

304+
test('connectToPeer reports connection failures as peer connection errors', async () => {
305+
const path = socketPath('uds-connect-error')
306+
const { connectToPeer, UdsPeerConnectionError } = await import(
307+
'../udsClient.js'
308+
)
309+
310+
const error = await connectToPeer(path).then(
311+
() => undefined,
312+
err => err,
313+
)
314+
315+
expect(error).toBeInstanceOf(UdsPeerConnectionError)
316+
if (!(error instanceof UdsPeerConnectionError)) {
317+
throw new Error('Expected UDS peer connection error')
318+
}
319+
expect(error.socketPath).toBe(path)
320+
})
321+
322+
test('connectToPeer leaves connected socket lifecycle to the caller', async () => {
323+
const path = socketPath('uds-connect-lifecycle')
324+
if (process.platform !== 'win32') {
325+
await mkdir(dirname(path), { recursive: true })
326+
}
327+
328+
const sockets = new Set<Socket>()
329+
const receiver = createServer(socket => {
330+
sockets.add(socket)
331+
socket.on('close', () => {
332+
sockets.delete(socket)
333+
})
334+
})
335+
await new Promise<void>((resolve, reject) => {
336+
receiver.on('error', reject)
337+
receiver.listen(path, () => resolve())
338+
})
339+
340+
let client: Socket | undefined
341+
try {
342+
const { connectToPeer } = await import('../udsClient.js')
343+
client = await connectToPeer(path, 50)
344+
await new Promise(resolve => setTimeout(resolve, 100))
345+
346+
expect(client.destroyed).toBe(false)
347+
expect(client.listenerCount('error')).toBe(0)
348+
} finally {
349+
client?.destroy()
350+
for (const socket of sockets) {
351+
socket.destroy()
352+
}
353+
await closeServer(receiver)
354+
if (process.platform !== 'win32') {
355+
await unlink(path).catch(() => undefined)
356+
}
357+
}
358+
})
359+
304360
test('sendUdsMessage fails closed before connecting without an auth token', async () => {
305361
await expect(
306362
sendUdsMessage(socketPath('no-auth-token'), { type: 'text', data: 'x' }),

src/utils/udsClient.ts

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -268,14 +268,30 @@ export async function sendToUdsSocket(
268268
* Connect to a peer and return the raw socket for bidirectional communication.
269269
* The caller is responsible for managing the connection lifecycle.
270270
*/
271-
export function connectToPeer(socketPath: string): Promise<Socket> {
271+
export function connectToPeer(
272+
socketPath: string,
273+
timeoutMs = 5000,
274+
): Promise<Socket> {
272275
return new Promise<Socket>((resolve, reject) => {
273-
const conn = createConnection(socketPath, () => {
276+
const conn = createConnection(socketPath)
277+
let settled = false
278+
const fail = (cause: unknown) => {
279+
if (settled) {
280+
return
281+
}
282+
settled = true
283+
conn.destroy()
284+
reject(new UdsPeerConnectionError(socketPath, cause))
285+
}
286+
conn.once('connect', () => {
287+
settled = true
288+
conn.setTimeout(0)
289+
conn.off('error', fail)
274290
resolve(conn)
275291
})
276-
conn.on('error', reject)
277-
conn.setTimeout(5000, () => {
278-
conn.destroy(new Error('Connection timed out'))
292+
conn.on('error', fail)
293+
conn.setTimeout(timeoutMs, () => {
294+
fail(new Error('Connection timed out'))
279295
})
280296
})
281297
}

0 commit comments

Comments
 (0)