From a1a380559961ba7ce1659a6fbac5abcdcb4aad2c Mon Sep 17 00:00:00 2001 From: Rick Gao Date: Tue, 14 Jul 2026 22:10:36 -0700 Subject: [PATCH] fix(daemon): only unlink the socket this daemon owns, on drain/exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On `restart`, the old daemon's drain() closed its server early but unlinked the socket file LATE — after the (possibly slow) provider.shutdown() flush. A daemon spawned during that window bound the path, and the old daemon's late unlink then deleted the NEW daemon's live socket ~a second later, orphaning it (listening on a dangling inode) so the next hook cold-started another daemon: duplicate, flapping daemons. Record the inode of the bound socket and only release a socket file we still own (releaseOwnedSocket); drain() releases it up-front instead of after the flush; the process-exit handler is ownership-checked too. Cold herds were already safe; this closes the restart / slow-drain path. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/daemon.ts | 37 +++++++++++--- tests/daemon-restart-orphan.test.ts | 78 +++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 8 deletions(-) create mode 100644 tests/daemon-restart-orphan.test.ts diff --git a/src/daemon.ts b/src/daemon.ts index dfc4c8b..c6f896d 100644 --- a/src/daemon.ts +++ b/src/daemon.ts @@ -474,6 +474,11 @@ function newSessionState(options: NewSessionStateOptions): SessionState { export class GlobalDaemon { private server?: net.Server; + /** Inode of the socket file this daemon bound, recorded at listen() time. On + * shutdown we unlink the socket only if it still has this inode, so a + * slow-draining daemon never deletes a successor that already reclaimed the + * path (which would orphan the successor and spawn a duplicate daemon). */ + private ownedSocketInode: number | null = null; private running = false; private lastActivity = Date.now(); /** Inactivity shutdown threshold. Overridable via WEAVE_INACTIVITY_MS (ms) for @@ -531,11 +536,10 @@ export class GlobalDaemon { // mistake for a live daemon. Routing SIGHUP through shutdown() unlinks it. process.on('SIGHUP', () => void this.shutdown('SIGHUP')); // Belt-and-suspenders: catch any non-signal exit (uncaught exception, - // process.exit from elsewhere) and remove the inode. Does NOT cover SIGKILL - // or OOM — the hook handler's probe handles those at next event. - process.on('exit', () => { - try { if (fs.existsSync(this.socketPath)) fs.unlinkSync(this.socketPath); } catch { /* nothing more we can do */ } - }); + // process.exit from elsewhere) and release our socket inode. Ownership-checked + // so we never delete a successor's socket. Does NOT cover SIGKILL or OOM — + // the hook handler's probe handles those at next event. + process.on('exit', () => this.releaseOwnedSocket()); // Check at most every 60s, but more frequently when the timeout is short // (env-overridden for tests) so a low WEAVE_INACTIVITY_MS is honored promptly. @@ -555,6 +559,19 @@ export class GlobalDaemon { }); } + /** Unlink the socket file only if it is still the one THIS daemon bound (same + * inode), then relinquish the claim. A slow drain must never delete a + * successor daemon that already reclaimed the path — that orphans the + * successor and makes the next hook cold-start a duplicate. */ + private releaseOwnedSocket(): void { + const owned = this.ownedSocketInode; + this.ownedSocketInode = null; + if (owned === null) return; + try { + if (fs.statSync(this.socketPath).ino === owned) fs.unlinkSync(this.socketPath); + } catch { /* already gone or unreadable — nothing to release */ } + } + /** Create a fresh server and listen once, resolving on success and rejecting * on the first listen error. A new server per attempt — one that errored on * listen cannot be reused. Socket is owner-only (umask 0o077). */ @@ -571,6 +588,8 @@ export class GlobalDaemon { process.umask(prevUmask); server.removeListener('error', onError); this.server = server; + // Remember which inode we bound so shutdown only unlinks our own socket. + try { this.ownedSocketInode = fs.statSync(this.socketPath).ino; } catch { this.ownedSocketInode = null; } resolve(); }); }); @@ -2019,6 +2038,11 @@ export class GlobalDaemon { private async drain(reason: string): Promise { this.log('INFO', `Shutdown: ${reason}`); this.server?.close(); + // Release the socket now, right after we stop accepting — NOT after the + // (possibly slow) provider.shutdown() flush below. Otherwise a daemon spawned + // during that window binds this path and our late unlink would delete its + // live socket. Ownership-checked so we only remove the file we bound. + this.releaseOwnedSocket(); // Backstop: close any queued team-member invoke_agent spans whose teammate // never emitted a TeammateIdle (e.g. teammate crashed, or daemon exits // mid-triage) so they flush as ended spans instead of leaking. @@ -2043,9 +2067,6 @@ export class GlobalDaemon { for (const session of this.sessions.values()) { session.transcript.close(); } - if (fs.existsSync(this.socketPath)) { - fs.unlinkSync(this.socketPath); - } } // ── helpers ─────────────────────────────────────────────────────────────── diff --git a/tests/daemon-restart-orphan.test.ts b/tests/daemon-restart-orphan.test.ts new file mode 100644 index 0000000..81d37d9 --- /dev/null +++ b/tests/daemon-restart-orphan.test.ts @@ -0,0 +1,78 @@ +// SPDX-FileCopyrightText: 2026 CoreWeave, Inc. +// SPDX-License-Identifier: MIT +// SPDX-PackageName: weave-claude-code + +// Regression: a daemon must only unlink the socket file it actually bound. +// +// The bug: on `restart`, the old daemon's drain() closed its server early but +// unlinked the socket LATE — after the (potentially slow) provider.shutdown() +// flush. A daemon spawned during that window bound the path, and the old +// daemon's late unlink then deleted the NEW daemon's live socket ~a second +// later, orphaning it (listening on a dangling inode) so the next hook cold- +// started yet another daemon: duplicate, flapping daemons. +// +// The fix records the inode of the socket this daemon bound and only releases a +// socket file it still owns (`releaseOwnedSocket`), and drain() releases it +// up-front rather than after the flush. These tests pin the ownership check. +// +// Sockets live under /tmp for the macOS 104-char UNIX socket path cap. + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import * as fs from 'node:fs'; +import * as net from 'node:net'; +import * as path from 'node:path'; +import { GlobalDaemon } from '../src/daemon.ts'; + +// Private methods/fields we drive directly. No project/key → tracing disabled, +// isolating the socket lifecycle from OTel. +type DaemonInternals = { + bindSocketWithHerdProtection(): Promise; + releaseOwnedSocket(): void; + server?: net.Server; +}; + +function makeDaemon(sock: string, dir: string): DaemonInternals { + return new GlobalDaemon(sock, path.join(dir, 'd.log'), null, null, 'https://x', false, 'claude-code') as unknown as DaemonInternals; +} + +test('releaseOwnedSocket unlinks the socket this daemon bound', async () => { + const dir = fs.mkdtempSync('/tmp/wcp-own-'); + const sock = path.join(dir, 'daemon.sock'); + const d = makeDaemon(sock, dir); + try { + await d.bindSocketWithHerdProtection(); + assert.ok(fs.existsSync(sock), 'daemon bound its socket'); + d.releaseOwnedSocket(); + assert.ok(!fs.existsSync(sock), 'releaseOwnedSocket removes the socket it owns'); + } finally { + try { d.server?.close(); } catch { /* best effort */ } + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('releaseOwnedSocket leaves a socket a successor rebound (different inode)', async () => { + const dir = fs.mkdtempSync('/tmp/wcp-own-'); + const sock = path.join(dir, 'daemon.sock'); + const d = makeDaemon(sock, dir); + const successor = net.createServer(); + try { + await d.bindSocketWithHerdProtection(); // records the owned inode + + // A successor reclaims the path with a fresh socket (new inode), exactly as a + // restart-spawned daemon does while the old daemon is still draining. + fs.unlinkSync(sock); + await new Promise((r) => successor.listen(sock, () => r())); + const successorInode = fs.statSync(sock).ino; + + // The old daemon releasing its socket must NOT delete the successor's. + d.releaseOwnedSocket(); + + assert.ok(fs.existsSync(sock), 'successor socket must survive the old daemon releasing'); + assert.equal(fs.statSync(sock).ino, successorInode, "the socket is still the successor's"); + } finally { + try { successor.close(); } catch { /* best effort */ } + try { d.server?.close(); } catch { /* best effort */ } + fs.rmSync(dir, { recursive: true, force: true }); + } +});