Repository navigation
fix(orcad): refuse a data root that cannot hold the daemon's socket - #17851
klaidliadon wants to merge 2 commits into
Conversation
A data root long enough to push the daemon's endpoint past `sun_path` killed the daemon at startup — but orcad's own listener name is about 5 bytes shorter, so there is a band where orcad came up, reported ready, and served clients with terminal survival silently off. Every terminal then died on the next restart, which is the one thing forking a detached daemon exists to prevent. Found on DSM: a root of 86 characters reproduced it, 84 was fine. orcad now refuses at startup with `ORCAD_EXIT_CONFIGURATION` (78). This is not one of the unenumerable runtime reasons `startOrcadDaemon` fail-opens for: it is known before anything is attempted, permanent until an operator changes a value they supplied, and pure arithmetic. That puts it with the bind address and the instance lock, which already refuse, and `RestartPreventExitStatus=78` keeps a supervisor from restart-spinning on it. The check runs ahead of the instance lock deliberately — it reads nothing and creates nothing, so refusing a root should not first take a lock on it. **The limit is verified against the kernel rather than asserted**, because past it there is no single failure to detect: on Linux a 109-110 byte path fails EADDRINUSE against a free name, and 111+ reports a successful `listen` with no directory entry at all — which looks healthy to everything except the dev+ino reads daemon endpoint ownership is built on. The test binds at the limit and one byte past it and asserts a stat-able entry, so a wrong constant fails it. Supporting changes, all in service of that: - **Consolidate the daemon's endpoint path builders into one leaf module** (`daemon-runtime-paths.ts`), so the budget measures the real names. `daemon-spawner` re-exports `getDaemonSocketPath` / `getDaemonTokenPath` / `getDaemonPidPath` from it rather than moving them, so every existing caller imports the same name from the same place and only the definition moved. `daemon-endpoint-ownership` likewise re-exports `getDaemonSocketBindPath`. - **Measure bytes, not characters.** A non-ASCII root spends more of the budget than its length suggests. `ssh-control-socket` now shares the constant and the byte measurement instead of keeping its own copy of the 104/108 split. Boundary verified end to end against the built orcad: 85 starts with a live daemon socket, 86 exits 78. Refs stablyai#10726 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UacRCfD4XPe29bD3ZWNc57
Greptile SummaryThe PR centralizes Unix socket-path budgeting and daemon runtime path construction, then makes orcad reject data roots that cannot host the daemon endpoint.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/shared/unix-socket-path-budget.ts | Centralizes platform socket limits and UTF-8 byte-based path-budget calculation. |
| src/main/daemon/daemon-runtime-paths.ts | Consolidates daemon endpoint names and computes the longest Unix socket path required by the daemon. |
| src/main/orcad/orcad-daemon-socket-preflight.ts | Adds a side-effect-free startup refusal for data roots whose daemon endpoint exceeds the socket-path budget. |
| src/main/orcad/orcad-entry.ts | Runs the new preflight before locking the data root and classifies refusal as a configuration exit. |
| src/main/daemon/daemon-runtime-paths.test.ts | Verifies arithmetic and real kernel boundary behavior for daemon socket paths. |
| src/main/ssh/ssh-control-socket.ts | Reuses the shared platform limit and measures OpenSSH control paths in UTF-8 bytes. |
Reviews (2): Last reviewed commit: "test(orcad): bind at exactly the socket ..." | Re-trigger Greptile
The kernel-backed test bound at `limit - 1` and `limit + 1`. That pins `UNIX_SOCKET_PATH_LIMIT` from below but not from above: a constant one byte too high still binds at `limit - 1` and still fails past `limit + 1`, so the suite stays green while `checkDaemonSocketPathBudget` admits a path the kernel rejects — which is the defect this whole check exists to prevent, reintroduced by the number it is built on. Bind at exactly the limit instead: that is the length production accepts, and it implies `limit - 1`. Measured, not assumed. On Linux (node 25 / libuv 1.52.1) a 108-byte path binds and leaves a stat-able entry while 109 fails, so the boundary the constant claims is the one the kernel enforces. Unit tests run on ubuntu-latest only. Raised by Greptile on stablyai#17851. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UacRCfD4XPe29bD3ZWNc57
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
Reviewed the daemon socket-path preflight: a shared unix-socket byte budget, a consolidated daemon runtime-paths module, an orcad startup preflight that refuses over-long data roots with exit 78, and a bytes-vs-characters fix in the SSH control socket path.
- Unix socket byte budget —
unix-socket-path-budget.tscentralizes the platform limit (104 macOS / 108 elsewhere) and byte measurement; the constants are correct against both kernels. - Daemon runtime paths — socket/token/pid/bind builders move to one leaf module, re-exported from
daemon-spawneranddaemon-endpoint-ownershipso existing imports keep working. - orcad preflight —
assertDaemonSocketPathFitsruns before the instance lock and maps toORCAD_EXIT_CONFIGURATION(78), turning a silent daemon-less runtime into a loud refusal. - SSH control socket —
.lengthbecomes a byte count, so a non-ASCII$HOMEorXDG_RUNTIME_DIRis measured correctly.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
| export function describeDaemonSocketPathOverflow(budget: UnixSocketPathBudget): string { | ||
| return ( | ||
| `The terminal daemon's socket path needs ${budget.bytes} bytes and the kernel accepts ` + | ||
| `${budget.limit} (${budget.longestPath}). The daemon cannot start, so terminals would ` + |
There was a problem hiding this comment.
The PR description says the refusal message reports the measured length and limit but not the path's contents, yet this interpolates budget.longestPath — the full socket path. Either drop the path from the message or update the description to match.
There was a problem hiding this comment.
You're right, and the description was the wrong half — so I fixed it there rather than in the code.
describeDaemonSocketPathOverflow interpolates budget.longestPath deliberately. An operator cannot act on "a path is too long" without knowing which path: the daemon's socket name is the data root plus a protocol-versioned filename, and the measured string is the longer of the endpoint and its pre-publish bind name — so the overflowing path is not something they can reconstruct from the root they set. It is also a value they supplied, living in their own unit file and printed to their own stderr and journal. No credential or token appears in it.
The message keeps the path; the Review section no longer claims otherwise. Thanks for catching it — this was a claim I made about my own code that the code did not support.
There was a problem hiding this comment.
ℹ️ Delta is clean; one minor point from the prior review remains open.
Reviewed changes
Reviewed the delta since the prior pullfrog review (e2cb2e0 → e1f8ae6): one test change only.
- Kernel-boundary test now binds at exactly the limit —
daemon-runtime-paths.test.tsbuilds a path of exactlyUNIX_SOCKET_PATH_LIMITbytes and asserts it binds, plus one byte past fails. This resolves the test-gap finding: a constant one byte too high (e.g. 109 on Linux) now makes the at-limit bind fail, so the too-permissive direction is actually caught.
The remaining open thread (daemon-runtime-paths.ts:86) is unchanged: the refusal message still interpolates budget.longestPath — the full socket path — while the PR description says it reports "not the path's contents." Either drop the path from the message or update the description.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
|
Thank you! Main fixes this differently since #26483: when the data root makes the socket path too long, the daemon socket moves to a short per-user path instead ( |

ELI5
If your data root path is long enough, the terminal daemon's socket name overflows
a kernel limit and the daemon silently never starts — but orcad itself comes up
fine and says it's ready. You get a working session whose terminals all die on the
next restart. This makes orcad refuse to start instead.
What Changed
src/shared/unix-socket-path-budget.ts— the platform limit (104 onmacOS, 108 elsewhere) and a byte-length measurement, in one place.
orcad-daemon-socket-preflight.ts—assertDaemonSocketPathFits(),called from
startOrcadahead of the instance lock: it reads nothing andcreates nothing, so refusing a root should not first take a lock on it.
OrcadDaemonSocketPathErrorjoinsresolveOrcadExitCode, so this exitsORCAD_EXIT_CONFIGURATION(78) alongside the bind-address and instance-lockrefusals.
daemon-runtime-paths.ts— the daemon's endpoint path builders,consolidated into one leaf module so the budget measures the real names.
daemon-spawnerre-exportsgetDaemonSocketPath/getDaemonTokenPath/getDaemonPidPathfrom it, anddaemon-endpoint-ownershipre-exportsgetDaemonSocketBindPath, so every existing caller imports the same name fromthe same place — only the definition moved.
ssh-control-socket.tsshares the constant and the byte measurement insteadof keeping its own copy of the 104/108 split.
Why
A data root long enough to push the daemon's endpoint past
sun_pathkilled thedaemon at startup. But orcad's own listener name is about 5 bytes shorter, so
there is a band where orcad came up, reported ready, and served clients with
terminal survival silently off. Every terminal then died on the next restart —
which is the one thing forking a detached daemon exists to prevent.
Found on DSM: a root of 86 characters reproduced it, 84 was fine.
Why refuse rather than fail-open.
startOrcadDaemonfail-opens for a long tailof runtime reasons, and rightly so. This is not one of them: it is known before
anything is attempted, permanent until an operator changes a value they supplied,
and pure arithmetic. That puts it with the bind address and the instance lock,
which already refuse.
RestartPreventExitStatus=78then keeps a supervisor fromrestart-spinning on it.
Why the limit is verified against the kernel rather than asserted. Past the
limit there is no single failure to detect:
EADDRINUSEagainst a name nothingholds, and 111+ reports a successful
listenwith no directory entry at all— which looks healthy to everything except the dev+ino reads daemon endpoint
ownership is built on;
EINVALfrom 109 up.The errno is libuv-version-dependent, which is exactly why the test asserts a
stat-able directory entry rather than an errno — that assertion holds across
both. It also strengthens the case for refusing up front rather than relying on
bindto report the problem consistently.So the test binds at exactly the limit and one byte past it and asserts a
stat-able entry. Binding at the limit (rather than one under) is the assertion
that matters: a constant one byte too high still binds at
limit - 1and stillfails past
limit + 1, so only the accepted boundary catches the direction thatreadmits the defect.
Bytes, not characters. A non-ASCII root spends more of the budget than its
length suggests.
ssh-control-socketpreviously compared.length, which is thesame latent bug on a different path.
Linked Issue
Fixes #17840
Refs #10726 — that issue is the same kernel limit reached by the SSH relay's
control socket. This PR fixes the daemon's endpoint (a different path and a
different failure mode) and, as a side effect, corrects the relay's measurement to
bytes. It does not implement a shortening strategy for the relay, so
Refsratherthan
Fixes.Visual Proof
N/A— startup refusal on a headless runtime. The visible change is a clear exitwith code 78 and a message, replacing a silent misconfiguration.
Testing
unix-socket-path-budget.test.ts— the limit and byte measurement.daemon-runtime-paths.test.ts— binds a real socket at the limit and one bytepast it and asserts a stat-able directory entry, so the constant is verified
against the kernel rather than restated.
orcad-daemon-socket-preflight.test.ts— the refusal and its exit code.Boundary verified end to end against the built orcad on real hardware: a data
root of 85 characters starts with a live daemon socket; 86 exits 78.
Platforms: verified on Linux (Synology DSM). The macOS constant (104) is covered by
test, not hardware. Windows is unaffected — named pipes have no
sun_pathlimit,and the preflight is a no-op there.
AI Disclosure
Claude Opus 5 wrote the commits and this description. I reviewed them and ran the
boundary verification on the hardware that surfaced the bug.
Review
explicitly. Windows named pipes are out of scope and unaffected.
ssh-control-socketnow measures bytes, so a non-ASCII orlong remote
$HOMEis judged correctly rather than optimistically. A relaydeployed to a deep remote path gets the same treatment.
path alongside its byte count and the kernel limit. That is deliberate — an
operator cannot act on "a path is too long" without knowing which one, and the
path is a value they supplied, living in their own unit file and printed to
their own stderr. No credential or token is printed. (An earlier draft of this
description claimed the path was omitted; that was wrong about the code, and
the code is right.)
The one behaviour change is that a data root that previously produced a silently
daemon-less runtime now refuses to start — which is the point, but it is a
behaviour change an operator on a very long root would notice as a new failure.
It replaces silent data loss with a loud, actionable refusal.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
This is the base of a two-PR stack.
feat/orcad-print-service-and-doctorbuilds on it — the path consolidation here is what lets the doctor ask about the
daemon's endpoint names without pulling the daemon into the CLI's import graph.
Reviewing and landing this one first keeps that diff readable.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)