fix(sidecar): attach error listener on server-side IPC sockets#230
fix(sidecar): attach error listener on server-side IPC sockets#230Foximo24 wants to merge 1 commit intonexu-io:mainfrom
Conversation
The IPC server in createJsonIpcServer attached only a 'data' handler to incoming sockets. The server's response is sent via socket.end(payload), which races with the client's own socket.end() in requestJsonIpc (called immediately after reading the response). On Windows named pipes with Node 24 this race produces an EPIPE write error on the server side. With no 'error' listener, that error is unhandled and crashes the entire daemon process, so tools-dev reports "daemon did not expose status in time". Adding a no-op error listener on the connection socket absorbs benign close-races without affecting normal operation. The client side already has an error listener, so this matches the existing pattern. Reproduced deterministically on Windows 11 + Node v24.14.0.
|
Hi @Foximo24! π Thanks for making open-design better! |
lefarcen
left a comment
There was a problem hiding this comment.
Hi @Foximo24! π Thanks for the detailed repro β the Windows IPC race crash is a real production issue.
Code review (Lens A):
β
Root cause analysis is sound: The server calls socket.end(payload) and the client calls socket.end() immediately after reading the response β that's a classic write-after-end race on Windows named pipes with Node 24.
β
Pattern consistency: You're right that the client side (requestJsonIpc line 562) already has an error listener, so this mirrors that.
Suggestions (non-blocking):
The no-op socket.on("error", () => {}) swallows ALL socket errors, not just benign close races. If a real error occurs (e.g., unauthorized connection attempt, network issue), it's now invisible. Consider one of these:
-
Filter by error code (most robust):
socket.on("error", (err: NodeJS.ErrnoException) => { if (err.code === "EPIPE" || err.code === "ECONNRESET") return; // benign close-race console.warn("[sidecar] IPC socket error:", err); // log real issues });
-
Or at minimum, log silently so you can debug future issues:
socket.on("error", (err) => { if (process.env.DEBUG) console.warn("[sidecar] IPC socket error:", err); });
Testing gap: You mentioned "verify existing test passes" but packages/sidecar/src/index.test.ts has no tests for createJsonIpcServer/requestJsonIpc β they only test path resolution. That's a broader gap (not your fault), but means this fix can't be validated automatically.
The core fix is correct and solves the daemon crash. The suggestions above would make it more maintainable long-term, but they're not blockers.
Thanks for tackling this Windows-specific issue β it's easy to miss cross-platform IPC subtleties! π
Summary
The IPC server in
createJsonIpcServer(packages/sidecar/src/index.ts) attached only adatahandler to incoming sockets. The server's response is sent viasocket.end(payload), which races with the client's ownsocket.end()inrequestJsonIpc(called immediately after reading the response). On Windows named pipes with Node 24 this race produces anEPIPEwrite error on the server side. With noerrorlistener, that error is unhandled and crashes the entire daemon process, sotools-devreportsdaemon did not expose status in time.This adds a no-op
errorlistener on the connection socket to absorb benign close-races. The client side (requestJsonIpc) already has anerrorlistener, so this just matches the existing pattern.Repro
pnpm tools-dev start daemonconsistently crashes withEPIPEfromSocket.endatpackages/sidecar/dist/index.mjs:319(thesocket.end(\${...}\n`)` line)runningcleanlyTest plan
packages/sidecar/src/index.test.tsstill passes