Repository navigation
fix(orcad): refuse a data root that cannot hold the daemon's socket #17851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
klaidliadon
wants to merge
2
commits into
stablyai:main
from
klaidliadon:fix/daemon-socket-path-budget
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { createServer } from 'node:net' | ||
| import { existsSync, mkdirSync, mkdtempSync, rmSync } from 'node:fs' | ||
| import { tmpdir } from 'node:os' | ||
| import { dirname, join } from 'node:path' | ||
| import { | ||
| checkDaemonSocketPathBudget, | ||
| getDaemonRuntimeDirPath, | ||
| getDaemonSocketBindPath, | ||
| getDaemonSocketPath | ||
| } from './daemon-runtime-paths' | ||
| import { UNIX_SOCKET_PATH_LIMIT, unixSocketPathBytes } from '../../shared/unix-socket-path-budget' | ||
|
|
||
| /** A data root whose daemon socket path lands on exactly `bytes`, or null if it cannot. */ | ||
| function rootForSocketBytes(base: string, bytes: number): string | null { | ||
| const sample = `${base}/r` | ||
| const overhead = | ||
| unixSocketPathBytes(getDaemonSocketPath(getDaemonRuntimeDirPath(sample))) - | ||
| unixSocketPathBytes(sample) | ||
| const rootBytes = bytes - overhead | ||
| const prefix = `${base}/` | ||
| if (rootBytes < unixSocketPathBytes(prefix)) { | ||
| return null | ||
| } | ||
| return prefix + 'x'.repeat(rootBytes - unixSocketPathBytes(prefix)) | ||
| } | ||
|
|
||
| describe('daemon socket path budget', () => { | ||
| it('measures the canonical endpoint, which is the longest name bound', () => { | ||
| const root = '/home/orca/.orca' | ||
| const socketPath = getDaemonSocketPath(getDaemonRuntimeDirPath(root)) | ||
| const budget = checkDaemonSocketPathBudget(root) | ||
| expect(budget.longestPath).toBe(socketPath) | ||
| // The private bind name is shorter by construction; if it ever stops being, the budget | ||
| // follows it rather than silently under-measuring. | ||
| expect(unixSocketPathBytes(getDaemonSocketBindPath(socketPath))).toBeLessThan(budget.bytes) | ||
| }) | ||
|
|
||
| it('fits an ordinary root and refuses a long one', () => { | ||
| expect(checkDaemonSocketPathBudget('/home/orca/.orca').fits).toBe(true) | ||
| expect(checkDaemonSocketPathBudget(`/home/${'x'.repeat(200)}/.orca`).fits).toBe(false) | ||
| }) | ||
|
|
||
| it.skipIf(process.platform === 'win32')('flips at exactly the limit', () => { | ||
| const atLimit = rootForSocketBytes('/tmp', UNIX_SOCKET_PATH_LIMIT) | ||
| const overLimit = rootForSocketBytes('/tmp', UNIX_SOCKET_PATH_LIMIT + 1) | ||
| expect(atLimit).not.toBeNull() | ||
| expect(checkDaemonSocketPathBudget(atLimit as string).fits).toBe(true) | ||
| expect(checkDaemonSocketPathBudget(overLimit as string).fits).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| /** | ||
| * Ties the constant to the kernel. Without this the budget is a number in a file that nothing | ||
| * disagrees with, and the defect it exists to prevent — a daemon that cannot bind — is exactly | ||
| * what a wrong number reintroduces. | ||
| * | ||
| * The assertion is "a socket exists at the path", not an errno, because past the limit there is | ||
| * no single failure: on Linux a slightly-too-long path fails EADDRINUSE against a free name, and | ||
| * a longer one reports a successful listen with no directory entry at all. A stat-able entry is | ||
| * the property `publishDaemonEndpoint` is built on — it identifies the endpoint by dev+ino — so | ||
| * it is the property worth asserting. | ||
| */ | ||
| describe('the limit is the kernel’s', () => { | ||
| function bindable(socketPath: string): Promise<boolean> { | ||
| mkdirSync(dirname(socketPath), { recursive: true }) | ||
| const server = createServer() | ||
| return new Promise<boolean>((resolve) => { | ||
| server.once('error', () => resolve(false)) | ||
| server.listen(socketPath, () => resolve(existsSync(socketPath))) | ||
| }).finally(() => new Promise((done) => server.close(() => done(null)))) | ||
| } | ||
|
|
||
| it.skipIf(process.platform === 'win32')( | ||
| 'binds at the limit and produces no endpoint past it', | ||
| async () => { | ||
| // A short base: the socket path has to be built to an exact length, and macOS tmpdir is | ||
| // already long enough to make that impossible. | ||
| const base = mkdtempSync(join(tmpdir(), 'sb-')) | ||
| try { | ||
| // Exactly the limit, because that is the length production admits: a constant one byte | ||
| // too high still binds at `limit - 1` and still fails past `limit + 1`, so only the | ||
| // accepted boundary catches it — and that is the direction that readmits the defect. | ||
| const atLimit = join(base, 'a'.repeat(UNIX_SOCKET_PATH_LIMIT - base.length - 1)) | ||
| const overLimit = `${atLimit}b` | ||
| expect(unixSocketPathBytes(atLimit)).toBe(UNIX_SOCKET_PATH_LIMIT) | ||
| expect(await bindable(atLimit)).toBe(true) | ||
| expect(unixSocketPathBytes(overLimit)).toBe(UNIX_SOCKET_PATH_LIMIT + 1) | ||
| expect(await bindable(overLimit)).toBe(false) | ||
| } finally { | ||
| rmSync(base, { recursive: true, force: true }) | ||
| } | ||
| } | ||
| ) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| /** | ||
| * Every path the daemon's endpoint is built from, and whether a data root leaves room for it. | ||
| * | ||
| * One leaf module, importing only node builtins and the protocol version, for two reasons: the | ||
| * names are a single source of truth shared by the spawner, the ownership protocol and orcad, | ||
| * and the doctor needs the arithmetic below without dragging the daemon into the CLI's graph. | ||
| */ | ||
| import { createHash, randomBytes } from 'node:crypto' | ||
| import { dirname, join } from 'node:path' | ||
| import { PROTOCOL_VERSION } from './daemon-protocol-version' | ||
| import { | ||
| measureUnixSocketPathBudget, | ||
| UNIX_SOCKET_PATH_LIMIT, | ||
| type UnixSocketPathBudget | ||
| } from '../../shared/unix-socket-path-budget' | ||
|
|
||
| /** | ||
| * The daemon's runtime directory, as a path only. Separate from `getDaemonRuntimeDir`, which | ||
| * also creates it: the socket-path budget has to measure a root before anything touches it. | ||
| */ | ||
| export function getDaemonRuntimeDirPath(userDataPath: string): string { | ||
| return join(userDataPath, 'daemon') | ||
| } | ||
|
|
||
| export function getDaemonSocketPath( | ||
| runtimeDir: string, | ||
| protocolVersion = PROTOCOL_VERSION | ||
| ): string { | ||
| // Why: Windows IPC servers use named pipes rather than filesystem socket | ||
| // files. Include the protocol version in the endpoint name so a daemon from | ||
| // an older build is never reused after a breaking protocol change. | ||
| if (process.platform === 'win32') { | ||
| const suffix = createHash('sha256').update(runtimeDir).digest('hex').slice(0, 12) | ||
| return `\\\\?\\pipe\\orca-terminal-host-v${protocolVersion}-${suffix}` | ||
| } | ||
| return join(runtimeDir, `daemon-v${protocolVersion}.sock`) | ||
| } | ||
|
|
||
| export function getDaemonTokenPath(runtimeDir: string, protocolVersion = PROTOCOL_VERSION): string { | ||
| return join(runtimeDir, `daemon-v${protocolVersion}.token`) | ||
| } | ||
|
|
||
| export function getDaemonPidPath(runtimeDir: string, protocolVersion = PROTOCOL_VERSION): string { | ||
| return join(runtimeDir, `daemon-v${protocolVersion}.pid`) | ||
| } | ||
|
|
||
| /** | ||
| * A private, same-directory name to bind before publishing the canonical endpoint. | ||
| * | ||
| * `.p`, not the `.b` this used to be: released builds sweep that pattern on age alone. Replaces | ||
| * the basename rather than extending the path, so the `sockaddr_un.sun_path` budget holds. | ||
| */ | ||
| export function getDaemonSocketBindPath(socketPath: string): string { | ||
| return join(dirname(socketPath), `.p${randomBytes(5).toString('hex')}`) | ||
| } | ||
|
|
||
| /** | ||
| * Whether a data root leaves room for the daemon's socket to exist. | ||
| * | ||
| * Why a startup question and not a runtime one: the endpoint lives under the data root, so a | ||
| * long enough root pushes it past `sockaddr_un.sun_path` and the daemon dies before it serves | ||
| * anything. orcad's own listener spends a few bytes less, so there is a band of root lengths | ||
| * where orcad comes up, reports ready, and serves clients with terminal survival silently | ||
| * switched off — every terminal then dies on the next restart, which is the one thing forking | ||
| * a detached daemon exists to prevent. | ||
| * | ||
| * Pure arithmetic: no probe, no privileges, no filesystem. That is what lets the same check | ||
| * answer for a host the doctor is only reading a unit file about. | ||
| */ | ||
| export function checkDaemonSocketPathBudget(userDataPath: string): UnixSocketPathBudget { | ||
| const socketPath = getDaemonSocketPath(getDaemonRuntimeDirPath(userDataPath)) | ||
| if (process.platform === 'win32') { | ||
| // Named pipes are not filesystem paths and carry no sun_path budget. | ||
| return { longestPath: socketPath, bytes: 0, limit: UNIX_SOCKET_PATH_LIMIT, fits: true } | ||
| } | ||
| // Both names matter: the daemon binds the private one to listen, and every client connects to | ||
| // the canonical one. The bind name is random but fixed-width, so measuring one is measuring | ||
| // all of them — and measuring both means a change to either name cannot go unnoticed here. | ||
| return measureUnixSocketPathBudget([socketPath, getDaemonSocketBindPath(socketPath)]) | ||
| } | ||
|
|
||
| /** One wording, so the startup refusal and the doctor finding cannot drift apart. */ | ||
| 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 ` + | ||
| 'not survive a restart.' | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Names only mechanisms that exist. It previously offered `--user-data <path>`, which orcad | ||
| * does not accept and — because the service commands ignored unrecognised flags — did not | ||
| * reject either: an operator following this advice got exit 0 and an `[OK] socket path fits` | ||
| * measured against the shell's own root, and would reasonably conclude it was fixed. | ||
| */ | ||
| export const DAEMON_SOCKET_PATH_REMEDY = | ||
| 'Move the data root to a shorter path: set ORCA_USER_DATA, and regenerate the service ' + | ||
| 'definition with it set if one is installed.' | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { | ||
| assertDaemonSocketPathFits, | ||
| OrcadDaemonSocketPathError | ||
| } from './orcad-daemon-socket-preflight' | ||
| import { ORCAD_EXIT_CONFIGURATION, resolveOrcadExitCode } from './orcad-entry' | ||
|
|
||
| describe('daemon socket path preflight', () => { | ||
| it('accepts an ordinary root', () => { | ||
| expect(() => assertDaemonSocketPathFits('/home/orca/.orca')).not.toThrow() | ||
| }) | ||
|
|
||
| it.skipIf(process.platform === 'win32')('refuses a root that cannot host the socket', () => { | ||
| expect(() => assertDaemonSocketPathFits(`/home/${'x'.repeat(120)}/.orca`)).toThrow( | ||
| OrcadDaemonSocketPathError | ||
| ) | ||
| }) | ||
|
|
||
| it('exits 78 so a supervisor stops instead of restart-spinning', () => { | ||
| // The whole point of refusing here rather than degrading: the generated unit sets | ||
| // RestartPreventExitStatus=78, and no restart will ever shorten the path. | ||
| expect(resolveOrcadExitCode(new OrcadDaemonSocketPathError('too long'))).toBe( | ||
| ORCAD_EXIT_CONFIGURATION | ||
| ) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /** | ||
| * Refuses a data root whose daemon socket path cannot fit in `sockaddr_un.sun_path`. | ||
| * | ||
| * Why this refuses where a failed daemon start does not: `startOrcadDaemon` is deliberately | ||
| * fail-open, because the reasons a daemon fails at runtime are unenumerable and often | ||
| * transient, and a host that cannot fork one can still serve git and worktrees. This failure | ||
| * is none of those things — it is known before anything is attempted, it is permanent until | ||
| * an operator changes a value they supplied, and it is arithmetic. That puts it with the bind | ||
| * address and the instance lock, which already refuse, rather than with the runtime failures. | ||
| * | ||
| * The alternative is what the field found: orcad's own listener is a few bytes shorter than | ||
| * the daemon's, so a root in that band comes up, reports ready, and serves normally with | ||
| * survival off. The warning it prints goes to journald, and a host with volatile journald | ||
| * loses it on the reboot that also destroys every terminal. | ||
| * | ||
| * Its own module, and importing only path arithmetic, so `orcad-entry` can call it without | ||
| * pulling the daemon (and node-pty behind it) into the static graph the native preflight | ||
| * has to run ahead of. | ||
| */ | ||
| import { | ||
| checkDaemonSocketPathBudget, | ||
| DAEMON_SOCKET_PATH_REMEDY, | ||
| describeDaemonSocketPathOverflow | ||
| } from '../daemon/daemon-runtime-paths' | ||
|
|
||
| export class OrcadDaemonSocketPathError extends Error { | ||
| readonly code = 'orcad_daemon_socket_path_too_long' | ||
| } | ||
|
|
||
| export function assertDaemonSocketPathFits(userDataPath: string): void { | ||
| const budget = checkDaemonSocketPathBudget(userDataPath) | ||
| if (!budget.fits) { | ||
| throw new OrcadDaemonSocketPathError( | ||
| `${describeDaemonSocketPathOverflow(budget)} ${DAEMON_SOCKET_PATH_REMEDY}` | ||
| ) | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and the description was the wrong half — so I fixed it there rather than in the code.
describeDaemonSocketPathOverflowinterpolatesbudget.longestPathdeliberately. 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.