fix: make UDS mirror cleanup ownership-aware, not path-based - #2035
Conversation
An overlapping in-process worker restart (component reload, deploy, config change) silently loses per-worker Unix-domain-socket mirrors, downgrading a node from the UDS + proxy-terminated-TLS architecture back to TCP passthrough and its ~64k ephemeral-port ceiling, with no warning until a full restart. On Linux, the replacement worker is deliberately pre-started and bound before the outgoing worker is told to shut down (restartWorkers()'s overlapping restart, left untouched here). listenOnDomainSocket() unlinks the outgoing worker's socket file and binds a new inode at the same path. When the outgoing worker then exits and calls cleanupUdsFiles(), it unlinked by path alone — deleting the live replacement's .sock and .yaml, since the path is byte-identical (same worker index + port). Track the inode a mirror bind actually confirmed (recordUdsBindSuccess, at all four bind sites: the Node HTTP/TLS mirror, its h2 sibling, the raw-socket MQTT securePort mirror, and the Bun path) and only unlink a path when the inode currently on disk still matches what this worker recorded — a mismatched or never-recorded inode means a replacement (or nothing) owns it, so cleanupUdsFiles()/markUdsBindFailed() skip it. Also wire up the already-written cleanupSocketsDirectory() crash-path sweep on main-thread startup, before any worker can bind, as the matching defense for a hard crash that skips a worker's own exit-time cleanup. Co-Authored-By: Claude Opus <noreply@anthropic.com>
- markUdsBindFailed()'s yaml unlink required 'owned' (matching ino), but its only caller (an overlong path) always fails before listen() ever runs, so ino is never recorded — the unlink became permanently unreachable, leaking yaml metadata for a mirror that was never bound whenever TLS readiness had already written it. Split ownership into three states (owned / foreign / absent) so a failed bind can retract its own stale metadata when nothing else is on disk, while still refusing to touch a foreign (replacement-owned) file. - The HARPER_UWS_UDS bind path (registered for cleanup by http.ts like every other mirror) never called recordUdsBindSuccess, so its ino stayed unrecorded forever and cleanupUdsFiles() silently stopped cleaning it up — a regression from this branch's own earlier commit. Record ownership once createUwsServer() confirms the bind. - cleanupSocketsDirectory() ran on every startHTTPThreads() call, not just the first; a caller that starts more threads later in the same process (unitTests/apiTests/setupTestApp.mjs's addThreads(), used by multi-threaded-test.mjs) would re-sweep the sockets directory out from under already-bound mirrors. Guard it to run once per process. Co-Authored-By: Claude Opus <noreply@anthropic.com>
- cleanupUdsFiles() runs twice per worker (threadServer.js's SHUTDOWN
handler, while this worker's own socket is still open, then again
from process.on('exit') after closeServers() has released it), but
never cleared an entry's recorded inode after processing it. Inode
numbers get reused once freed — verified experimentally on this
filesystem (20/20 collisions when the outgoing worker's own socket is
closed before a new bind at the same path) — so a later, unrelated
generation could occupy the path with the exact inode number this
worker originally recorded by the time the second call runs, causing
it to wrongly conclude it still owns that generation's live files.
Clear the recorded inode after the first pass so a repeat call can
never re-derive ownership from stale state.
- cleanupSocketsDirectory() inherited the TLS_UNIXDOMAINSOCKETS gate,
so a crash that left stale files behind while UDS was enabled would
never get swept on a boot where the setting had since changed —
including an operator disabling UDS specifically because of a mirror
problem. The sockets directory is Harper-owned and holds nothing but
mirror files, so the sweep is now unconditional.
Co-Authored-By: Claude Opus <noreply@anthropic.com>
- Track (dev, ino) as bigint via statSync(path, { bigint: true })
instead of the default double. Ordinary ext4/xfs deployments never
approach 2^53, but XFS with inode64 (the modern default) and some
overlay/FUSE mounts can hand out larger inode numbers whose double
representation loses precision, letting two genuinely different
inodes compare equal — silently reopening the exact outage this
branch exists to close. Comparing dev alongside ino also fully
qualifies identity rather than relying on inode number alone.
- recordUdsBindSuccess() previously swallowed a statSync failure with
a bare catch. If a worker's own freshly-bound socket somehow can't be
stat'd, it silently treats itself as unowned for the rest of its
life and leaks both files at shutdown with nothing in the logs to
explain why — now logged as a warning.
- cleanupUdsFiles() treated 'absent' (nothing currently at the path)
the same as 'foreign' (something else owns it), skipping both. Only
'foreign' should be protective — markUdsBindFailed() already treats
'absent' as safe to retract, for the same reason: a mirror's TLS
metadata can be written (SNICallback.ready) before its socket binds,
and a bind failure other than the overlong-path precheck (the only
one markUdsBindFailed covers) left that yaml permanently stranded
once ownership tracking made cleanupUdsFiles() skip it too. Aligned
cleanupUdsFiles() with the same invariant.
Co-Authored-By: Claude Opus <noreply@anthropic.com>
Round 4's own fix (treating 'absent' as safe to retract in
cleanupUdsFiles, to match markUdsBindFailed) introduced a new
regression that the next review round caught: on the non-overlapping
restart platforms (macOS/Windows/Bun), the replacement worker starts
right after this worker's SHUTDOWN — not after it fully exits — and
can write its own fresh yaml (independent of its bind, same as any
mirror) before this worker's second cleanup pass (process.on('exit'))
runs. That second pass had already consumed its own recorded identity
in the first pass, so it would see 'absent' and delete the
replacement's yaml before the replacement ever binds its socket.
markUdsBindFailed's 'absent' really is always safe — its only caller
runs synchronously before any bind attempt, with no window for a
concurrent process to be mid-flight. cleanupUdsFiles' second call has
no such guarantee, so only 'owned' is safe there; a stale yaml with no
socket and no bind-failure to explain it is left for markUdsBindFailed
or the next full boot's cleanupSocketsDirectory() sweep.
Also drops a new unit test that stubbed harperLogger.warn via sinon —
AGENTS.md prohibits new sinon/rewire usage in tests — and removes an
env.get stub in a cleanupSocketsDirectory test that's now dead weight
since that function no longer reads config at all.
Co-Authored-By: Claude Opus <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Code Review
This pull request introduces ownership-aware lifecycle management for Unix Domain Socket (UDS) mirror files to prevent outgoing workers from deleting replacement workers' files during overlapping restarts. It captures the device and inode of bound sockets and verifies ownership before unlinking. Additionally, it ensures that the startup crash-path sweep of the sockets directory runs unconditionally and only once. The reviewer suggested a robustness improvement in ownershipOf to specifically check for ENOENT errors when stating the socket path, preventing other filesystem errors from falsely indicating that the socket is absent.
…port
The top-level `import { cleanupSocketsDirectory } from '../http.ts'`
added in an earlier commit broke every integration test: bin/run.ts
imports socketRouter.ts at its own top level (before main() parses
argv or initializes config), and a top-level import eagerly pulls in
http.ts's whole module graph at that same moment. That graph reaches
security/auth.ts, whose module-scope `table()` call now ran before
config was initialized, failing with "Unable to determine database
storage path" on every single Harper process the integration test
harness tried to start.
socketRouter.ts already avoids this for loadRootComponents.js and
threadServer.js by requiring them lazily inside startHTTPThreads()
instead of importing them at the top of the file — this just extends
that same pattern to http.ts. Uses a dynamic `await import('../http.ts')`
(matching server/status/index.ts's existing lazy import of the same
module) rather than `require()`, since `require('../http.ts')` doesn't
resolve once built to dist/ (only .js exists there) — dynamic import
specifiers get their extension rewritten by tsconfig's
rewriteRelativeImportExtensions, require() call arguments don't.
Verified against a real Harper process: reproduced the original CI
failure locally, confirmed this fixes it, and ran the broader
integrationTests/server/*.test.ts suite (129/135 passing; the 6
failures are all in ollama-backend.test.ts, which needs a live Ollama
instance and hits an unrelated local Node/JSON-import-attribute
mismatch — unrelated to this change).
Co-Authored-By: Claude Opus <noreply@anthropic.com>
Flagged independently by both gemini-code-assist and the CI claude
reviewer bot on the pushed PR: the bare `catch { return 'absent' }`
in ownershipOf() treated every statSync failure — not just ENOENT —
as "nothing is on disk." A permission or I/O error on a path a live
foreign worker actually owns would misclassify as 'absent' rather
than 'foreign', and markUdsBindFailed()'s guard only blocks on
'foreign' — so an inconclusive stat error could make it delete a
live worker's yaml metadata, reintroducing the bug class this PR
exists to fix. cleanupUdsFiles() is unaffected (it treats 'absent'
and 'foreign' identically), but the distinction matters wherever
'absent' alone is trusted.
Now checks error.code === 'ENOENT' specifically; any other error
returns 'foreign' (the conservative default) instead. Added a test
forcing a non-ENOENT stat failure deterministically via ENOTDIR
(treating a plain file as a path segment) rather than EACCES, since
permission checks aren't reliably enforced when tests run as root
(common in CI containers).
Co-Authored-By: Claude Opus <noreply@anthropic.com>
|
Reviewed Traced the ownership model against the real code paths: — |
What / why
An overlapping in-process worker restart (component reload, deploy, or a config change)
silently loses all of a node's per-worker Unix-domain-socket mirrors, downgrading it from
the UDS + proxy-terminated-TLS architecture back to TCP passthrough and its ~64k
per-tuple ephemeral-port ceiling — with no warning until a full process restart happens to
recreate the files.
Root cause: on Linux,
restartWorkers()deliberately pre-starts and binds the replacementworker before telling the outgoing worker to shut down (that overlap is intentional and is
not changed here). The replacement's
listenOnDomainSocket()unlinks the outgoingworker's socket file and binds a brand-new inode at the same path. When the outgoing worker
then exits and calls
cleanupUdsFiles(), it unlinks purely by path — deleting the livereplacement's
.sockand.yaml, since the path is byte-identical (same worker index +port). The replacement is left listening on an unlinked inode: functional, but unreachable
through its filesystem path, and no longer advertised to the fronting proxy.
Fix: make socket-file cleanup ownership-aware instead of path-based.
recordUdsBindSuccess()captures the(dev, ino)(asbigint, to avoid precision loss onfilesystems like XFS with
inode64) a mirror's bind actually confirmed, at all four bindsites: the Node HTTP/TLS mirror, its h2 sibling, the raw-socket/MQTT
securePortmirror,the uWS path, and the Bun path.
cleanupUdsFiles()unlinks a path only when the identity currently on disk still matcheswhat this worker recorded. A mismatched or never-recorded identity means a replacement (or
nothing) owns it, so the entry is skipped — both the socket and its
.yaml, since areplacement that rebound the socket owns the pair. It also consumes the recorded
identity after its first pass (this function runs twice per worker — SHUTDOWN, then
process.on('exit')— and inode numbers get reused once freed, so a stale second-passcomparison could otherwise coincidentally re-match an unrelated later generation).
markUdsBindFailed()carries an intentionally asymmetric version of the same guard: italso retracts stale metadata when nothing is currently on disk (
absent), not just when itowns the socket — its only caller runs synchronously before any bind is attempted, so it can
never itself be in the "owned" state, and there's no concurrency window for that call
specifically.
cleanupUdsFiles()deliberately does not extend the same "absent is safe"treatment to itself, because its second call can land seconds after the first, during which
a concurrently-booting replacement (on non-overlapping-restart platforms) can have written
its own fresh yaml without having bound its socket yet.
cleanupSocketsDirectory()crash-path sweep on main-threadstartup (before any worker can bind, one-shot per process), so a hard crash that skips a
worker's own exit-time cleanup can't leave stale files behind either. This is a separate
defense from the inode guard above (crash-path vs. rolling-restart) and is now unconditional
on the current config (a crash-then-reconfigure-then-reboot must still sweep).
restartWorkers()'s overlapping-restart sequencing andlistenOnDomainSocket()'sunlink-before-bind are both left exactly as they were — this is purely about who is allowed
to delete a socket file, not about restart ordering.
Deliberately not fixed here (see the PR's self-review for the full reasoning): the yaml's
publish lifecycle (
SNICallback.ready/cert-reload) is independent of the socket's bind, sosocket-inode ownership doesn't fully protect it in two narrow windows — a non-path-too-long
bind failure, and a cert reload racing an outgoing worker's shutdown drain on non-overlapping
platforms. Both are real but narrow, and the clean fix (yaml-own-identity via its embedded
pid/tid, or an atomic write) is a larger change than "who owns a socket file." Flagging fordiscussion rather than expanding scope unilaterally.
Out of scope (separate, pre-existing issue, left alone): the same overlap also permanently
kills the per-worker Node inspector port on a rolling restart, because the inspector port
isn't
SO_REUSEPORTand nothing retries the bind after the outgoing worker exits.Test plan
Extended
unitTests/server/udsMirror.test.js(46 tests in the file, all passing):path (simulating the replacement rebinding),
cleanupUdsFiles()leaves both files in placehard link, deterministic rather than allocator-dependent) between the two real-world
cleanupUdsFiles()calls a worker makes, and asserts the second call is a no-opENOENTis handled quietlymarkUdsBindFailed()'s asymmetric guard: removes when this worker owns the pair or nothingis on disk, leaves it when a live foreign socket is present
cleanupSocketsDirectory()'s unconditional sweepAlso:
npm run buildclean; fullserverunit suite 702 passing;test:unit:main4138passing with 11 pre-existing failures verified unrelated (reproduced identically on
unmodified
mainviagit stash).🤖 Generated with Claude Code