Skip to content

Commit a90c164

Browse files
author
unraid
committed
fix: close peer socket listener handoff window
CodeRabbit and Claude review found that documenting caller-owned raw socket errors still left a Promise handoff window and a stale timeout-listener risk. The peer connection API now requires a caller error handler and installs it before resolving, while cleanup removes internal error and timeout listeners on every path. Constraint: Keep the fix precise to PR #375 review feedback and avoid warning suppression or fallback behavior. Rejected: Leave the behavior documented only | still permits an unhandled socket error window between resolve and caller listener attachment. Rejected: Keep a no-op internal error listener | would silently swallow caller-owned socket errors. Confidence: high Scope-risk: narrow Directive: Do not add raw connectToPeer callers without providing a real onSocketError handler and capability handshake. Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.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: bun audit Not-tested: Manual external ACP peer runtime beyond repository tests.
1 parent a5ede23 commit a90c164

3 files changed

Lines changed: 48 additions & 17 deletions

File tree

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,9 @@ describe('startAgentSummarization', () => {
161161

162162
expect(forkCalls).toEqual([])
163163
expect(updateCalls).toEqual([])
164-
expectDebugLogContaining('no bounded context available')
164+
expectDebugLogContaining(
165+
'[AgentSummary] Skipping summary for task-1: no bounded context available',
166+
)
165167
})
166168

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

174176
expect(forkCalls).toEqual([])
175177
expect(updateCalls).toEqual([])
176-
expectDebugLogContaining('not enough messages (2)')
178+
expectDebugLogContaining(
179+
'[AgentSummary] Skipping summary for task-1: not enough messages (2)',
180+
)
177181
})
178182

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

189193
expect(forkCalls).toEqual([])
190194
expect(updateCalls).toEqual([])
191-
expectDebugLogContaining('poor mode active')
195+
expectDebugLogContaining('[AgentSummary] Skipping summary — poor mode active')
192196
expect(scheduledCount).toBe(initialScheduledCount + 1)
193197
expect(lastTimerHandle).not.toBe(initialTimerHandle)
194198
})
@@ -218,7 +222,7 @@ describe('startAgentSummarization', () => {
218222

219223
handle.stop()
220224

221-
expectDebugLogContaining('Stopping summarization for task-1')
225+
expectDebugLogContaining('[AgentSummary] Stopping summarization for task-1')
222226
expect(clearedHandles).toEqual([pendingHandle])
223227
})
224228
})

src/utils/__tests__/udsMessaging.test.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,9 @@ describe('UDS inbox retention', () => {
307307
'../udsClient.js'
308308
)
309309

310-
const error = await connectToPeer(path).then(
310+
const error = await connectToPeer(path, () => {
311+
throw new Error('Unexpected post-connect socket error')
312+
}).then(
311313
() => undefined,
312314
err => err,
313315
)
@@ -338,13 +340,25 @@ describe('UDS inbox retention', () => {
338340
})
339341

340342
let client: Socket | undefined
343+
const socketErrors: Error[] = []
341344
try {
342345
const { connectToPeer } = await import('../udsClient.js')
343-
client = await connectToPeer(path, 1000)
346+
client = await connectToPeer(
347+
path,
348+
error => {
349+
socketErrors.push(error)
350+
},
351+
1000,
352+
)
344353
await new Promise(resolve => setTimeout(resolve, 100))
345354

346355
expect(client.destroyed).toBe(false)
347-
expect(client.listenerCount('error')).toBe(0)
356+
expect(client.listenerCount('error')).toBe(1)
357+
expect(client.listenerCount('timeout')).toBe(0)
358+
359+
const socketError = new Error('post-connect failure')
360+
client.emit('error', socketError)
361+
expect(socketErrors).toEqual([socketError])
348362
} finally {
349363
client?.destroy()
350364
for (const socket of sockets) {

src/utils/udsClient.ts

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -266,36 +266,49 @@ export async function sendToUdsSocket(
266266

267267
/**
268268
* Connect to a peer and return the raw socket for bidirectional communication.
269-
* The caller is responsible for managing the connection lifecycle, including
270-
* attaching an 'error' listener immediately after the Promise resolves. This
271-
* function detaches its internal error listener on successful connect so
272-
* caller-owned socket errors are not silently swallowed.
269+
* The caller owns the post-connect lifecycle through onSocketError, which is
270+
* attached before the Promise resolves so peer socket errors cannot be
271+
* swallowed or surface through a listener handoff window.
272+
* Pre-connect failures reject with UdsPeerConnectionError.
273+
* This only opens the transport; callers still own any capability handshake.
273274
*/
274275
export function connectToPeer(
275276
socketPath: string,
277+
onSocketError: (error: Error) => void,
276278
timeoutMs = 5000,
277279
): Promise<Socket> {
278280
return new Promise<Socket>((resolve, reject) => {
279281
const conn = createConnection(socketPath)
280282
let settled = false
281-
const fail = (cause: unknown) => {
283+
const onTimeout = () => {
284+
fail(new Error('Connection timed out'))
285+
}
286+
function cleanupListeners(): void {
287+
conn.setTimeout(0)
288+
conn.off('error', fail)
289+
conn.off('timeout', onTimeout)
290+
}
291+
function fail(cause: unknown): void {
282292
if (settled) {
283293
return
284294
}
285295
settled = true
296+
cleanupListeners()
286297
conn.destroy()
287298
reject(new UdsPeerConnectionError(socketPath, cause))
288299
}
289300
conn.once('connect', () => {
301+
if (settled) {
302+
return
303+
}
290304
settled = true
291-
conn.setTimeout(0)
292-
conn.off('error', fail)
305+
cleanupListeners()
306+
conn.on('error', onSocketError)
293307
resolve(conn)
294308
})
295309
conn.on('error', fail)
296-
conn.setTimeout(timeoutMs, () => {
297-
fail(new Error('Connection timed out'))
298-
})
310+
conn.once('timeout', onTimeout)
311+
conn.setTimeout(timeoutMs)
299312
})
300313
}
301314

0 commit comments

Comments
 (0)