From 49c0550bbba61a183e0b510ab75a3f2ad711084a Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 19:37:00 +0000 Subject: [PATCH 01/43] feat(daemon): reinstall and restart a stale hackd service automatically `hack daemon start` and `restart` now bring an installed login service up to the running CLI (HACK-2216). A systemd unit that runs this CLI's executable (same path or a link to the same file) but differs from what it renders is rewritten with the install checks and restarted; a running hackd of another version under that unit is restarted through systemctl. A unit for another existing hack, an unreadable ExecStart, another Hack home, an isolated HACK_HOME and HACK_DAEMON_SERVICE_READ_ONLY keep their refusals. A launchd plist whose program is current but whose content differs is rewritten too. The pure decision lives in planDaemonReconcile (src/daemon/reconcile.ts). `hack update` runs the new executable's `daemon start --json` after a successful install when a service is installed and hackd is running; a failure is reported with `hack daemon install` and never fails the update. `hack daemon start|stop|restart|install --json` print one envelope with data schema hack.daemon-/v1 (manager, action, stale reasons, daemon version/pid); failures use the new E_DAEMON_SERVICE code. Co-Authored-By: Claude Opus 5.5 --- docs/architecture.md | 111 +++- docs/cli.md | 2 +- docs/reference/cli.md | 8 +- docs/versions/v5/guides/upgrade-from-v4.md | 6 +- .../v5/reference/command-contracts.md | 9 +- docs/versions/v5/upgrade.md | 10 +- .../hack-diagnose/references/error-codes.md | 3 +- src/agents/error-code-reference.ts | 5 + src/commands/daemon-result.ts | 83 +++ src/commands/daemon-systemd.ts | 495 ++++++++++++++++-- src/commands/daemon-upgrade.ts | 183 +++++++ src/commands/daemon.ts | 274 ++++++++-- src/commands/update.ts | 29 +- src/daemon/client.ts | 2 + src/daemon/launchd.ts | 61 ++- src/daemon/reconcile.ts | 299 +++++++++++ src/daemon/systemd.ts | 6 +- src/lib/cli-result.ts | 1 + src/ui/logger.ts | 30 ++ tests/daemon-reconcile.test.ts | 345 ++++++++++++ tests/daemon-systemd-cli.test.ts | 98 ++++ tests/daemon-systemd.test.ts | 189 +++++++ tests/launchd-isolation.test.ts | 2 +- 23 files changed, 2127 insertions(+), 124 deletions(-) create mode 100644 src/commands/daemon-result.ts create mode 100644 src/commands/daemon-upgrade.ts create mode 100644 src/daemon/reconcile.ts create mode 100644 tests/daemon-reconcile.test.ts diff --git a/docs/architecture.md b/docs/architecture.md index b8ea15c85..20a1cba58 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -431,7 +431,8 @@ the proof is reported with its pid and left running. When a proven hackd cannot be confirmed stopped, `stop` exits 1 and says it is still running, and `restart`, `start` and the `install` handover stop there. A pid file that survived an unclean shutdown and now names another program does not count as a running hackd. If -systemd runs an incompatible hackd from a unit that points at another `hack`, +systemd runs an incompatible hackd from a unit that points at another `hack` +(another file, not a link to this one), `hack daemon status`, `hack doctor` and `hack daemon restart` all point to `hack daemon install`, because a restart would start the same version again. `install` reports success only when systemd runs the hackd that answers. The unit: @@ -522,19 +523,23 @@ several `ExecStart=` lines make the program unknown: `start` leaves that unit as it is and says so, and `hack daemon status` and `hack doctor` report it. An unknown program is never treated as a missing one. -`hack daemon start` and autostart never rewrite a unit whose program still exists, -even when the `hack` on the current `PATH` is a different one: they say so, and +`hack daemon start` and autostart never rewrite a unit that runs another existing +`hack` (not this CLI's executable or a link to it): they say so, and `hack daemon install` switches the service. If systemd runs an incompatible hackd from such a unit, `start` reports it instead of replacing it, because -systemd would only start the same version again. Only when the unit's program no -longer exists may `start` repair it, and only to a durable location: the path +systemd would only start the same version again. A unit that runs this CLI's +executable but is stale is rewritten and restarted; see +[After an upgrade](#after-an-upgrade-a-stale-service-is-repaired-by-start). When the unit's program no +longer exists, `start` retargets it, but only to a durable location: the path and the file it resolves to must both be outside temporary directories, package caches and source checkouts. The unit keeps the path as written. It then replaces just the program in `ExecStart` and keeps every other line, including your edits; otherwise it reports the problem and asks you to run `hack daemon install`. If an earlier `daemon-reload` failed (`NeedDaemonReload=yes`), `start` and `install` reload before they start. Other edits to the file show up as drift in -`hack daemon status` and `hack doctor`, and `hack daemon install` replaces them. +`hack daemon status` and `hack doctor`; `hack daemon start` (for a unit that +runs this CLI's executable) and `hack daemon install` replace them. Put local +changes in a drop-in (`systemctl --user edit hackd`). A systemd user manager runs only while you have a login session, unless linger is on. `hack daemon install` and `hack daemon status` report linger. When it is off, @@ -563,6 +568,100 @@ or reloads the unit (`hack daemon` runs with `HACK_DAEMON_SERVICE_READ_ONLY=1`). default `~/.hack` root: an isolated `HACK_HOME` or `HACK_SYSTEMD_DISABLE=1` turns systemd management off (`HACK_SYSTEMD_ALLOW_ISOLATED_HOME=1` opts in). +### After an upgrade: a stale service is repaired by `start` + +`hack daemon start` and `hack daemon restart` bring an installed login service up +to the running CLI. The service is stale when: + +- `content`: the unit (or launchd plist) differs from what this CLI writes for + the same program, for example after an upgrade changed the unit's `PATH`; +- `program-missing`: the program it runs no longer exists; +- `version-mismatch`: the hackd it runs reports another version than the CLI. + +On Linux, `start` rewrites a `content`-stale unit only when it runs this CLI's +executable: the same path, or a path that resolves to the same file. The program +path is kept as written, so a stable symlink stays a symlink, and an enabled +unit stays enabled while a disabled one stays disabled. The rewrite makes the same checks as +`install`: the program and every directory on the unit's `PATH` must be safe +from other accounts, and the unit directories are checked and created the same +way. A program at a location that may not last is reported, as `install` does. +When hackd is running under the unit, `start` then restarts it with +`systemctl --user restart`, after the loaded-file, program and stop-hook checks +that `stop` makes. A hackd of another version under this CLI's unit is restarted +the same way, so the new executable runs. + +`start` does not repair the service, and goes on as before, when it cannot +prove the service is this CLI's: + +- the unit runs another existing `hack` (one you may have chosen): it is not + rewritten; a running hackd of another version is refused with + `hack daemon install`, which switches the service; +- systemd cannot say what the unit runs (an `ExecStart` Hack does not read): a + running hackd of another version is refused; +- the unit serves another Hack home, or Hack home isolation + (`HACK_SYSTEMD_ALLOW_ISOLATED_HOME`, `HACK_SYSTEMD_DISABLE`) keeps this CLI + off the user service: hackd is started directly, as before; +- `HACK_DAEMON_SERVICE_READ_ONLY=1` (`hack doctor --fix` without a TTY): the + unit is never written or reloaded. A hackd of another version under this + CLI's unit is still restarted, which does not change the unit. + +On macOS, a loaded launchd plist whose program is current but whose other +content (for example its environment) differs is rewritten too, and launchd +restarts hackd from it. + +```mermaid +stateDiagram-v2 + [*] --> Observe + Observe --> Direct: no usable unit, another home, or isolated + Observe --> Plan: unit serves this home + Plan --> Refuse: program gone and read-only + Plan --> Rewrite: content stale and program is this CLI's, or program gone + Plan --> Refuse: version mismatch and unit runs another or unreadable program + Plan --> Start: not running + Plan --> Restart: running and version mismatch + Plan --> Keep: running, current (or stale but not ours, or read-only) + Rewrite --> Start: not running + Rewrite --> Restart: running + Direct --> [*] + Start --> [*] + Restart --> [*] + Keep --> [*] + Refuse --> [*] + classDef ok stroke-width:1.6px + classDef live stroke-width:1.6px + classDef fail stroke-width:1.6px + class Keep ok + class Rewrite live + class Restart live + class Refuse fail +``` + +After `hack update` installs a release, it runs the new executable's +`hack daemon start --json` when a login service is installed and hackd is +running, so the service reaches the new version. It never starts a daemon +that was not running and never fails the update: a failure is reported in the +update result (`daemon.status: "failed"`) with `hack daemon install` as the +next step. A stopped service picks up the new version at its next +`hack daemon start`. + +`hack daemon start|stop|restart|install --json` print one envelope and nothing +else on stdout (human lines go to stderr). Success is +`{"ok": true, "data": {...}}` with: + +| Field | Meaning | +| --- | --- | +| `schema` | `hack.daemon-/v1` | +| `manager` | `systemd`, `launchd` or `none` (hackd runs directly) | +| `action` | `none`, `started`, `restarted`, `reinstalled`, `stopped` or `installed` | +| `stale`, `stale_reasons` | whether the service or daemon was stale, and why: `content`, `program-missing`, `version-mismatch` | +| `cli_version` | this CLI's version | +| `daemon` | `running`, `pid`, `version` and `compatible` after the command | + +A failure is `{"ok": false, "error": {"code": "E_DAEMON_SERVICE", "message", +"detail"}}`, where `message` is the last warning or error the command printed and +`detail` is the same data. Access refusals keep their own code (`E_PERMISSION`), +and `start --foreground --json` is `E_USAGE`. Exit codes are unchanged. + ## Native backend internals The native backend is built around explicit ownership: every run has one owner, diff --git a/docs/cli.md b/docs/cli.md index dc4b4c9ff..5cbccc621 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -24,7 +24,7 @@ terminal). - `hack session` — persistent project workspaces (tmux-first) - `hack doctor` / `hack doctor --fix` — validate and repair local setup - `hack upgrade check` — read-only upgrade check of this machine and every registered project: a one-screen summary, the projects with findings (worktrees folded; `--verbose` for every checkout), and a plan with one step per kind of change, then the next command; `hack upgrade apply [--only ] [--project ] [--dry-run]` runs the plan's apply steps (`.hack/.gitignore` refresh, stale agent files) for every checkout after keeping a snapshot and a journal; `hack upgrade snapshot` and `hack upgrade rollback --rollback-manifest ` keep and restore the files the upgrade changes -- `hack daemon` — optional local daemon for faster JSON project status; `hack daemon install` starts it at login (launchd on macOS, a systemd user unit on Linux). On Linux without linger it still stops at your last logout and does not start at boot; `loginctl enable-linger` changes that +- `hack daemon` — optional local daemon for faster JSON project status; `hack daemon install` starts it at login (launchd on macOS, a systemd user unit on Linux). On Linux without linger it still stops at your last logout and does not start at boot; `loginctl enable-linger` changes that. After an upgrade, `hack daemon start` (and `hack update`) rewrites a stale login service that runs the same executable and restarts hackd on the new version; `start`, `stop`, `restart` and `install` accept `--json` Candidate `hack-next doctor` and `hack-v5 doctor` report a missing hackd as optional for Compose workflows. They do not recommend starting the stable daemon or diff --git a/docs/reference/cli.md b/docs/reference/cli.md index af5b832cf..5ae82bf65 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -1678,7 +1678,7 @@ hack daemon [options] | Command | Summary | | --- | --- | -| `hack daemon start` | Start hackd (local daemon) | +| `hack daemon start` | Start hackd (local daemon); reinstalls and restarts a stale login service | | `hack daemon stop` | Stop hackd | | `hack daemon restart` | Restart hackd | | `hack daemon status` | Show hackd status | @@ -1698,7 +1698,7 @@ hack daemon [options] ## `hack daemon start` -Start hackd (local daemon) +Start hackd (local daemon); reinstalls and restarts a stale login service ### Usage @@ -1711,6 +1711,7 @@ hack daemon start [options] | Option | Description | | --- | --- | | `--foreground` | Run hackd in the foreground (debug) | +| `--json` | Output JSON (machine-readable) | | `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | | `--help, -h` | Show help | | `--version, -v` | Show version | @@ -1729,6 +1730,7 @@ hack daemon stop [options] | Option | Description | | --- | --- | +| `--json` | Output JSON (machine-readable) | | `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | | `--help, -h` | Show help | | `--version, -v` | Show version | @@ -1747,6 +1749,7 @@ hack daemon restart [options] | Option | Description | | --- | --- | +| `--json` | Output JSON (machine-readable) | | `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | | `--help, -h` | Show help | | `--version, -v` | Show version | @@ -1843,6 +1846,7 @@ hack daemon install [options] | `--no-run-at-load` | Do not start hackd automatically on login (macOS launchd) | | `--gui-only` | Only run in GUI sessions (macOS launchd; default) | | `--no-gui-only` | Run in all session types, including SSH (macOS launchd) | +| `--json` | Output JSON (machine-readable) | | `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | | `--help, -h` | Show help | | `--version, -v` | Show version | diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index b6c033e7e..9b790ecb0 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -692,8 +692,10 @@ Source: [current env migration and cleanup guidance](https://github.com/hack-dan Installing another CLI does not replace an already running daemon. For an unmanaged daemon, `daemon start` replaces a reachable incompatible daemon; `daemon restart` is the explicit replacement path. With the newer Linux systemd -service, a unit pointing at a different existing executable needs explicit -`daemon install`: restarting that unit would run its old executable again. +service, `daemon start` rewrites and restarts a stale unit that runs the same +executable (or a link to it). A unit pointing at a different existing +executable needs explicit `daemon install`: restarting that unit would run its +old executable again. Keep this separate from a side-by-side candidate trial: the launcher disables daemon autostart, and stable `hackd` stays stopped while the candidate shares its home. The macOS trials confirmed that the stable diff --git a/docs/versions/v5/reference/command-contracts.md b/docs/versions/v5/reference/command-contracts.md index 157389ee3..4ceba86bb 100644 --- a/docs/versions/v5/reference/command-contracts.md +++ b/docs/versions/v5/reference/command-contracts.md @@ -157,14 +157,14 @@ Source: `src/commands/env.ts:437 (backendUseSpec)`. No command alias is declared | `hack log-pipe` | `--format `; `--stream ` | Present | | `hack doctor` | `--path, -p `; `--fix`; `--migrate-env-config`; `--domain-migration preview\|apply\|rollback`; `--json`; `--browser-url https://app.hack`; `--browser-result unknown\|works\|fails\|permission-denied`; `--branch `; `--native-cleanup inspect`; `--native-run-mapping inspect\|repair`; `--expect-selection <64-hex>`; `--accept-legacy-device-rebind` | Present | | `hack crash-capture` | `--path, -p `; `--since ` | Present | -| `hack daemon start` | `--foreground` | Present | -| `hack daemon stop` | none | Present | -| `hack daemon restart` | none | Present | +| `hack daemon start` | `--foreground`; `--json` | Present | +| `hack daemon stop` | `--json` | Present | +| `hack daemon restart` | `--json` | Present | | `hack daemon status` | `--json` | Present | | `hack daemon metrics` | none | Present | | `hack daemon logs` | `--tail ` | Present | | `hack daemon clear` | none | Present | -| `hack daemon install` | `--run-at-load`; `--no-run-at-load`; `--gui-only`; `--no-gui-only` | Present | +| `hack daemon install` | `--run-at-load`; `--no-run-at-load`; `--gui-only`; `--no-gui-only`; `--json` | Present | | `hack daemon uninstall` | none | Present | | `hack secrets set [name]` | `--service ` | Present | | `hack secrets get [name]` | `--service ` | Present | @@ -332,6 +332,7 @@ Thrown usage errors with an explicit `--json` token become E_USAGE; other thrown | usage | bare projects[], total nullable, host[], host_total nullable, runtime_ok:boolean, runtime_error nullable; details adds containers[] | failures exit 1; optional error string. No samples exits 0 with warning:no_usage_samples. Watch uses the interactive redraw path even with JSON selected; do not treat watch as a JSON report stream. | | global status | bare generated_at/caddy/logging/networks/gateway/summary | exits 0 even if summary health false; gateway is retained unsupported data, excluded from public contract | | daemon status | bare report below | running returns 0; starting/stale/unavailable may return 1 | +| daemon start/stop/restart/install | shared envelope; data schema `hack.daemon-/v1`, manager (systemd/launchd/none), action (none/started/restarted/reinstalled/stopped/installed), stale:boolean, stale_reasons (content/program-missing/version-mismatch), cli_version, daemon.{running,pid,version,compatible} | failure is `E_DAEMON_SERVICE` with the same data as detail; access refusals `E_PERMISSION`; `start --foreground --json` is `E_USAGE`; exit codes unchanged | | host ps | bare {generatedAt,snapshotAvailable,commands,lifecycle} | timestamp string, availability boolean; nested telemetry remains specialized | | session panes/capture/tail | JSON-lines events below | no shared envelope | | update | command-owned object with ok:boolean, string error on failure | error is not shared error.{code,message}; success varies by check/install outcome; exit follows ok | diff --git a/docs/versions/v5/upgrade.md b/docs/versions/v5/upgrade.md index 72cac1178..85f5a22ea 100644 --- a/docs/versions/v5/upgrade.md +++ b/docs/versions/v5/upgrade.md @@ -113,8 +113,12 @@ backup scripts, source evidence and checks still awaiting real-machine trials. ## The daemon after an upgrade Installing a CLI does not replace a running daemon. After choosing a durable v5 -install, use `hack daemon restart` for an unmanaged incompatible daemon. If a -login service still points at the old executable, use the chosen install's -`hack daemon install`. Keep this outside the shared-home candidate trial; +install, run its `hack daemon start` (or `hack daemon restart`). When the login +service runs that same executable, start rewrites a stale unit or plist and +restarts hackd on the new version; `hack update` does this by itself when a +service is installed and hackd is running. If the login service points at +another existing executable, start reports it instead: use the chosen install's +`hack daemon install`. Add `--json` for one machine-readable result. Keep this +outside the shared-home candidate trial; [daemon and service details](guides/upgrade-from-v4.md#daemon-old-lock-and-missing-checkouts) explain the difference. diff --git a/plugins/hack/skills/hack-diagnose/references/error-codes.md b/plugins/hack/skills/hack-diagnose/references/error-codes.md index 36e09998b..2383af0d5 100644 --- a/plugins/hack/skills/hack-diagnose/references/error-codes.md +++ b/plugins/hack/skills/hack-diagnose/references/error-codes.md @@ -38,9 +38,10 @@ stay within the request. | `E_ADMISSION_LIMIT` | Too many background commands are active for this session or Hack home. | `hack commands show --json` for the running commands | | `E_REQUEST_CONFLICT` | A background request key was reused with different inputs; the earlier command is unchanged. | `hack commands show --json` | | `E_JOURNAL_VERSION` | A background command journal was written by a newer Hack journal format. | `hack --version` | +| `E_DAEMON_SERVICE` | hackd or its login service could not be started, stopped, restarted, installed or repaired, or Hack refused because it could not prove the daemon or service belongs to this Hack home and executable. | `hack daemon status --json` | | `E_USAGE` | Arguments or flags were invalid for the command. | `hack --help` | | `E_UNEXPECTED` | An unclassified failure; the message is the only detail. | `hack doctor --json` | --- -Content revision: `e104e92c08a5`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `ec326e7f2eb8`. Generated by Hack; edit the canonical source and regenerate the plugin. diff --git a/src/agents/error-code-reference.ts b/src/agents/error-code-reference.ts index 72fa4f670..0ef6ddeee 100644 --- a/src/agents/error-code-reference.ts +++ b/src/agents/error-code-reference.ts @@ -163,6 +163,11 @@ const ERROR_CODE_GUIDES: Readonly> = { "A background command journal was written by a newer Hack journal format.", readFirst: "`hack --version`", }, + E_DAEMON_SERVICE: { + meaning: + "hackd or its login service could not be started, stopped, restarted, installed or repaired, or Hack refused because it could not prove the daemon or service belongs to this Hack home and executable.", + readFirst: "`hack daemon status --json`", + }, E_USAGE: { meaning: "Arguments or flags were invalid for the command.", readFirst: "`hack --help`", diff --git a/src/commands/daemon-result.ts b/src/commands/daemon-result.ts new file mode 100644 index 000000000..fc6689432 --- /dev/null +++ b/src/commands/daemon-result.ts @@ -0,0 +1,83 @@ +import { DAEMON_CLI_VERSION, probeDaemonApi } from "../daemon/client.ts"; +import type { DaemonPaths } from "../daemon/paths.ts"; +import { + buildDaemonCommandData, + createDaemonCommandTrace, + type DaemonCommandName, + type DaemonCommandTrace, + type DaemonObservation, +} from "../daemon/reconcile.ts"; +import { readDaemonStatus } from "../daemon/status.ts"; +import { emitCliResult, errorResult, okResult } from "../lib/cli-result.ts"; +import { + captureLoggedProblems, + setLoggerBackendOverride, +} from "../ui/logger.ts"; + +/** + * Runs one `hack daemon start|stop|restart|install` step and, with + * `--json`, prints its single envelope (HACK-2216, HACK-2238): + * `{ok: true, data: {schema: "hack.daemon-/v1", ...}}`, or on a + * nonzero exit `{ok: false, error: {code: "E_DAEMON_SERVICE", message, + * detail}}` whose message is the last warning or error the step logged and + * whose detail is the same data. Human lines go to stderr meanwhile, so + * stdout holds only the envelope. Exit codes are unchanged. A thrown error + * (for example an access refusal) propagates to the CLI's own `--json` + * error handling. + */ +export async function runDaemonCommand(opts: { + readonly command: DaemonCommandName; + readonly json: boolean; + readonly paths: DaemonPaths; + readonly run: (trace: DaemonCommandTrace) => Promise; +}): Promise { + const trace = createDaemonCommandTrace(); + if (!opts.json) { + return await opts.run(trace); + } + setLoggerBackendOverride({ backend: "console" }); + const problems = captureLoggedProblems(); + let code: number; + try { + code = await opts.run(trace); + } finally { + problems.stop(); + setLoggerBackendOverride({ backend: null }); + } + const data = buildDaemonCommandData({ + command: opts.command, + trace, + daemon: await observeDaemon({ paths: opts.paths }), + cliVersion: DAEMON_CLI_VERSION, + }); + emitCliResult({ + result: + code === 0 + ? okResult({ data }) + : errorResult({ + code: "E_DAEMON_SERVICE", + message: problems.last() ?? `hack daemon ${opts.command} failed`, + detail: data, + }), + }); + return code; +} + +/** The daemon after the command: its pid file and what its API reports. */ +async function observeDaemon(opts: { + readonly paths: DaemonPaths; +}): Promise { + const status = await readDaemonStatus({ paths: opts.paths }); + if (!(status.running && status.socketExists)) { + return { running: status.running, pid: status.pid, version: null }; + } + const probe = await probeDaemonApi({ + socketPath: opts.paths.socketPath, + timeoutMs: 1000, + }); + return { + running: true, + pid: status.pid, + version: probe.reachable ? probe.version : null, + }; +} diff --git a/src/commands/daemon-systemd.ts b/src/commands/daemon-systemd.ts index b0d501baf..6de9f61f8 100644 --- a/src/commands/daemon-systemd.ts +++ b/src/commands/daemon-systemd.ts @@ -1,7 +1,8 @@ -import { mkdir, rm, writeFile } from "node:fs/promises"; +import { mkdir, realpath, rm, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, resolve } from "node:path"; import { GLOBAL_DAEMON_DIR_NAME } from "../constants.ts"; +import { DAEMON_CLI_VERSION } from "../daemon/client.ts"; import type { DaemonPaths } from "../daemon/paths.ts"; import { type DaemonProcessProbe, @@ -12,6 +13,13 @@ import { proveOwnedDaemonProcess, } from "../daemon/process.ts"; import { readLinuxParentPid } from "../daemon/process-image.ts"; +import { + type DaemonCommandTrace, + type DaemonReconcilePlan, + planDaemonReconcile, + recordReconcile, + type ServiceUnitState, +} from "../daemon/reconcile.ts"; import { type DaemonServiceSummary, summarizeSystemdService, @@ -38,6 +46,7 @@ import { readSystemdServiceStatus, readSystemdUnitHackHome, renderedServicePathDirectories, + renderSystemdUnit, resolveExpectedSystemdUnit, resolveSystemdAccess, resolveSystemdUnitPath, @@ -45,6 +54,7 @@ import { SYSTEMD_UNIT_NAME, type SystemdDeps, type SystemdServiceStatus, + type SystemdUnitDrift, systemdDaemonReload, systemdServiceAction, uninstallSystemdUnit, @@ -93,6 +103,10 @@ export interface SystemdCommandHooks { * directories so they do not depend on the host's. */ readonly systemPathDirectories?: readonly string[]; + /** Records what the command decided, for `--json` (HACK-2216). */ + readonly trace?: DaemonCommandTrace; + /** Defaults to this CLI's version; tests pin it. */ + readonly cliVersion?: string; } /** Status for `daemon status` and doctor; null off Linux or when access is denied. */ @@ -228,6 +242,177 @@ export async function startViaSystemdIfInstalled( return 1; } +/** + * `hack daemon start` found this home's hackd running (HACK-2216). When + * hackd.service runs it, brings the service up to this CLI with + * {@link planDaemonReconcile}: rewrite a stale unit (as + * {@link prepareSystemdUnitForStart} does) and `systemctl --user restart` it + * when the unit changed or the daemon reports another version. A version + * mismatch that a restart cannot fix (the unit runs another existing + * `hack`, or an ExecStart hack does not read) is refused with the + * `hack daemon install` step. + * + * Returns null when this does not apply (no usable unit, systemd does not + * run `pid`, the unit serves another Hack home) or nothing needs to change; + * the caller then goes on as before. Otherwise the exit code. + */ +export async function reconcileRunningSystemdDaemon( + hooks: Omit & { + readonly pid: number; + /** What the daemon's API reported; null when it did not answer. */ + readonly version: string | null; + } +): Promise { + const deps = hooks.deps ?? defaultSystemdDeps(); + if (!resolveSystemdAccess({ env: deps.env }).allowed) { + return null; + } + const unitPath = resolveSystemdUnitPath({ env: deps.env }); + if (!(await pathExists(unitPath))) { + return null; + } + const status = await readSystemdServiceStatus({ unitPath, deps }); + if ( + !status?.availability.available || + status.stateError !== null || + !(await systemdRunsPid({ + status, + pid: hooks.pid, + isPidInUnit: hooks.isPidInUnit, + })) + ) { + return null; + } + const unitHome = await checkUnitServesHome({ + hackHome: dirname(hooks.paths.root), + deps, + action: "starting", + }); + if (unitHome.kind !== "ours") { + return null; + } + const expected = await ( + hooks.resolveExpected ?? resolveExpectedSystemdUnit + )(); + const marker = resolve(hooks.paths.root, RESTART_PENDING_FILE); + const plan = planDaemonReconcile({ + unit: serviceUnitState(status.drift), + programIsOurs: await unitRunsExpected({ status, expected }), + readOnly: isSystemdReadOnly(deps.env), + running: { version: hooks.version }, + restartPending: await pathExists(marker), + cliVersion: hooks.cliVersion ?? DAEMON_CLI_VERSION, + }); + if (plan.service === "keep") { + if (hooks.trace) { + hooks.trace.manager = "systemd"; + } + return null; + } + recordReconcile({ trace: hooks.trace, manager: "systemd", plan }); + if (plan.service === "refuse") { + logger.warn({ + message: describeRunningRefusal({ + plan, + status, + expected, + pid: hooks.pid, + }), + }); + return 1; + } + return await restartReconciledUnit({ + hooks, + plan, + status, + unitPath, + deps, + marker, + }); +} + +/** The refusal for a running hackd whose version a restart cannot fix. */ +function describeRunningRefusal(opts: { + readonly plan: DaemonReconcilePlan; + readonly status: SystemdServiceStatus; + readonly expected: { readonly hackBinPath: string } | null; + readonly pid: number; +}): string { + const { plan, status, expected } = opts; + if (plan.blocked === "read-only") { + return `${describeMissingProgram(status)}; not changing the service here | Run: hack daemon install`; + } + if (plan.blocked === "unknown-program" || status.program === null) { + return `hackd (pid ${opts.pid}) is incompatible with this CLI, and ${SYSTEMD_UNIT_NAME} has an ExecStart hack does not read, so a restart is not known to run this version | Check: systemctl --user cat ${SYSTEMD_UNIT_NAME}; to switch it to this hack, run: hack daemon install`; + } + const target = expected + ? ` (switches the service to ${expected.hackBinPath})` + : ""; + return `hackd (pid ${opts.pid}) is incompatible with this CLI, and ${SYSTEMD_UNIT_NAME} runs ${status.program}, so restarting it would start the same version again | Run: hack daemon install${target}`; +} + +/** + * Rewrites the unit when the plan says so, then restarts it through + * systemd. A restart runs the unit's stop hooks and stops what hackd + * started, so the same checks as `stop` and `start` run first. The + * restart-pending marker (shared with install) is written before the unit + * changes and removed only after the restart, so a failure in between is + * retried as a restart. + */ +async function restartReconciledUnit(opts: { + readonly hooks: Omit; + readonly plan: DaemonReconcilePlan; + readonly status: SystemdServiceStatus; + readonly unitPath: string; + readonly deps: SystemdDeps; + readonly marker: string; +}): Promise { + const { hooks, plan, status, unitPath, deps } = opts; + if (plan.rewrite !== "none") { + await writeRestartMarker({ paths: hooks.paths }); + const rewritten = await rewriteUnitForPlan({ + plan, + status, + unitPath, + deps, + hooks, + expected: await (hooks.resolveExpected ?? resolveExpectedSystemdUnit)(), + }); + if (rewritten !== null) { + return rewritten; + } + } + const blocked = + (await checkSystemdUnitTrusted({ unitPath })) ?? + (await checkSystemdLoadedUnitPaths({ unitPath, deps })) ?? + (await checkUnitProgramTrusted({ unitPath })) ?? + (await checkSystemdStopHooks({ unitPath, deps })); + if (blocked !== null) { + logger.error({ message: blocked }); + return 1; + } + const why = plan.reasons.includes("version-mismatch") + ? "it runs another hack version" + : "its unit changed"; + logger.info({ message: `Restarting ${SYSTEMD_UNIT_NAME}: ${why}` }); + const restarted = await systemdServiceAction({ action: "restart", deps }); + if (!restarted.ok) { + logger.error({ + message: `systemctl --user restart ${SYSTEMD_UNIT_NAME} failed: ${restarted.error} | Check: ${JOURNAL_HINT}`, + }); + return 1; + } + await rm(opts.marker, { force: true }); + if (await hooks.waitForReady(READY_TIMEOUT_MS)) { + logger.success({ message: "hackd restarted (systemd)" }); + return 0; + } + logger.warn({ + message: `systemd restarted hackd but it did not report ready yet | Check: hack daemon logs --tail 200 and ${JOURNAL_HINT}`, + }); + return 1; +} + /** * The unit is a per-user singleton: starting (or repairing) it from a * shell with another Hack home would start someone else's daemon, and this @@ -302,16 +487,21 @@ async function checkUnitServesHome(opts: { * installed unit before starting it. Returns an exit code to stop with, or * null to go on and start. * - * Owner decision (HACK-2052): start never rewrites a unit whose program - * still exists, even when this CLI resolves a different `hack`; it reports - * the difference and `hack daemon install` switches it. That keeps a user's - * whole-file edits and never points a login service at a temporary binary. - * Only when the program is gone may start retarget it, and only to a - * durable location ({@link assessExecutableDurability}), replacing just the - * ExecStart program so every other line is kept. Under - * {@link isSystemdReadOnly} (`hack doctor --fix` without a TTY) it neither - * writes nor reloads. A pending reload (`NeedDaemonReload=yes`, left by a - * failed reload after a write) is completed first, so a retry recovers. + * The decision is {@link planDaemonReconcile} (HACK-2216): + * - a unit that runs this CLI's executable (the same path, or one that + * resolves to the same file) but differs from what this CLI renders for + * that program is rewritten whole, as `hack daemon install` would, keeping + * its program path (a stable symlink stays a symlink) and whether it is + * enabled; + * - a unit whose program is gone is retargeted to this CLI, only to a + * durable location ({@link assessExecutableDurability}), replacing just + * the ExecStart program so every other line is kept; + * - a unit that runs another existing `hack` is never rewritten (HACK-2052: + * the user may have chosen it); start reports the difference and + * `hack daemon install` switches it. + * Under {@link isSystemdReadOnly} (`hack doctor --fix` without a TTY) it + * neither writes nor reloads. A pending reload (`NeedDaemonReload=yes`, left + * by a failed reload after a write) is completed first, so a retry recovers. */ async function prepareSystemdUnitForStart(opts: { readonly status: SystemdServiceStatus; @@ -319,7 +509,11 @@ async function prepareSystemdUnitForStart(opts: { readonly deps: SystemdDeps; readonly hooks: Pick< SystemdCommandHooks, - "resolveExpected" | "assessDurability" + | "resolveExpected" + | "assessDurability" + | "trace" + | "cliVersion" + | "systemPathDirectories" >; }): Promise { const { status, unitPath, deps, hooks } = opts; @@ -334,33 +528,149 @@ async function prepareSystemdUnitForStart(opts: { message: `${SYSTEMD_UNIT_NAME} has an ExecStart hack does not read (a prefix, specifier or escape); starting it unchanged`, }); } - if (status.drift !== "program-missing") { - return await completePendingReload({ status, deps, expected, readOnly }); - } - - const missing = `${SYSTEMD_UNIT_NAME} runs a program that no longer exists (${status.program ?? "unreadable ExecStart"})`; - if (readOnly) { + const plan = planDaemonReconcile({ + unit: serviceUnitState(status.drift), + programIsOurs: await unitRunsExpected({ status, expected }), + readOnly, + running: null, + cliVersion: hooks.cliVersion ?? DAEMON_CLI_VERSION, + }); + recordReconcile({ trace: hooks.trace, manager: "systemd", plan }); + if (plan.service === "refuse") { logger.error({ - message: `${missing}; not changing the service here | Run: hack daemon install`, + message: `${describeMissingProgram(status)}; not changing the service here | Run: hack daemon install`, }); return 1; } - if (!expected) { + if (plan.rewrite !== "none") { + return await rewriteUnitForPlan({ + plan, + status, + unitPath, + deps, + hooks, + expected, + }); + } + return await completePendingReload({ + status, + deps, + expected, + readOnly, + plan, + }); +} + +/** {@link ServiceUnitState} for a unit's {@link SystemdUnitDrift}. */ +function serviceUnitState(drift: SystemdUnitDrift): ServiceUnitState { + return drift === "none" ? "current" : drift; +} + +/** + * Whether the unit runs this CLI's executable: the same path, or a path + * that resolves to the same file (for example a stable symlink to it). + */ +async function unitRunsExpected(opts: { + readonly status: SystemdServiceStatus; + readonly expected: { readonly hackBinPath: string } | null; +}): Promise { + const { status, expected } = opts; + if (status.program === null || expected === null) { + return false; + } + if (status.program === expected.hackBinPath) { + return true; + } + const real = async (path: string): Promise => { + try { + return await realpath(path); + } catch { + return null; + } + }; + const [unitReal, ourReal] = await Promise.all([ + real(status.program), + real(expected.hackBinPath), + ]); + return unitReal !== null && unitReal === ourReal; +} + +function describeMissingProgram(status: SystemdServiceStatus): string { + return `${SYSTEMD_UNIT_NAME} runs a program that no longer exists (${status.program ?? "unreadable ExecStart"})`; +} + +type UnitRewrite = + | { readonly unitText: string; readonly done: string } + | { readonly error: string }; + +/** + * Carries out a plan's unit rewrite (`content` or `program`) with the + * checks `hack daemon install` uses: the program and the PATH it gives + * hackd must be safe from other accounts, and {@link installSystemdUnit} + * checks the unit directories. An enabled unit stays enabled and a + * disabled one stays disabled. Returns 1 (reported) on failure, else null. + */ +async function rewriteUnitForPlan(opts: { + readonly plan: DaemonReconcilePlan; + readonly status: SystemdServiceStatus; + readonly unitPath: string; + readonly deps: SystemdDeps; + readonly hooks: Pick< + SystemdCommandHooks, + "assessDurability" | "systemPathDirectories" + >; + readonly expected: { readonly hackBinPath: string } | null; +}): Promise { + const { plan, status, unitPath, deps, hooks, expected } = opts; + const rewritten = + plan.rewrite === "program" + ? await retargetMissingProgram({ status, unitPath, hooks, expected }) + : await renderCurrentUnit({ status, deps, hooks }); + if ("error" in rewritten) { + logger.error({ message: rewritten.error }); + return 1; + } + const repaired = await installSystemdUnit({ + unitPath, + unitText: rewritten.unitText, + deps, + // Keep the user's choice: a disabled unit is repaired, not re-enabled. + enable: status.enabled, + forceReload: status.needsReload, + }); + if (!repaired.ok) { logger.error({ - message: `${missing} and no installed hack was found to replace it | Put hack on PATH, then run: hack daemon install`, + message: `Failed to repair ${SYSTEMD_UNIT_NAME}: ${repaired.error}`, }); return 1; } + logger.warn({ message: rewritten.done }); + return null; +} + +/** The unit text for a `program` rewrite: only ExecStart is replaced. */ +async function retargetMissingProgram(opts: { + readonly status: SystemdServiceStatus; + readonly unitPath: string; + readonly hooks: Pick; + readonly expected: { readonly hackBinPath: string } | null; +}): Promise { + const { expected } = opts; + const missing = describeMissingProgram(opts.status); + if (!expected) { + return { + error: `${missing} and no installed hack was found to replace it | Put hack on PATH, then run: hack daemon install`, + }; + } const durability = await ( - hooks.assessDurability ?? assessExecutableDurability + opts.hooks.assessDurability ?? assessExecutableDurability )({ path: expected.hackBinPath }); if (!durability.durable) { - logger.error({ - message: `${missing}. This CLI (${expected.hackBinPath}) is not a stable location for a login service: ${durability.reason} | Install hack to a stable location, then run: hack daemon install`, - }); - return 1; + return { + error: `${missing}. This CLI (${expected.hackBinPath}) is not a stable location for a login service: ${durability.reason} | Install hack to a stable location, then run: hack daemon install`, + }; } - const unitText = await readTextFile(unitPath); + const unitText = await readTextFile(opts.unitPath); const retargeted = unitText === null ? null @@ -369,47 +679,87 @@ async function prepareSystemdUnitForStart(opts: { hackBinPath: expected.hackBinPath, }); if (retargeted === null) { - logger.error({ - message: `${missing} and its ExecStart could not be updated | Run: hack daemon install`, - }); - return 1; + return { + error: `${missing} and its ExecStart could not be updated | Run: hack daemon install`, + }; } - const repaired = await installSystemdUnit({ - unitPath, + return { unitText: retargeted, - deps, - // Keep the user's choice: a disabled unit is repaired, not re-enabled. - enable: status.enabled, - forceReload: status.needsReload, + done: `Repaired ${SYSTEMD_UNIT_NAME}: its program no longer existed; it now runs ${expected.hackBinPath} (other lines kept)`, + }; +} + +/** + * The unit text for a `content` rewrite: what this CLI renders for the + * unit's own program, after the program and PATH checks install runs. + * Durability is only reported, as install does: the program is unchanged. + */ +async function renderCurrentUnit(opts: { + readonly status: SystemdServiceStatus; + readonly deps: SystemdDeps; + readonly hooks: Pick< + SystemdCommandHooks, + "assessDurability" | "systemPathDirectories" + >; +}): Promise { + const program = opts.status.program; + const unitText = + program === null ? null : renderSystemdUnit({ hackBinPath: program }); + if (program === null || unitText === null) { + return { + error: `${SYSTEMD_UNIT_NAME} differs from what this hack writes, but its program cannot be written into a new unit | Run: hack daemon install`, + }; + } + const unsafe = await checkInstallPrograms({ + hackBinPath: program, + deps: opts.deps, + systemPathDirectories: opts.hooks.systemPathDirectories, }); - if (!repaired.ok) { - logger.error({ - message: `Failed to repair ${SYSTEMD_UNIT_NAME}: ${repaired.error}`, + if (unsafe !== null) { + return { error: unsafe }; + } + const durability = await ( + opts.hooks.assessDurability ?? assessExecutableDurability + )({ path: program }); + if (!durability.durable) { + logger.warn({ + message: `The service runs ${program}, which may not last: ${durability.reason}. Install hack to a stable location and run \`hack daemon install\` from there.`, }); - return 1; } - logger.warn({ - message: `Repaired ${SYSTEMD_UNIT_NAME}: its program no longer existed; it now runs ${expected.hackBinPath} (other lines kept)`, - }); - return null; + return { + unitText, + done: `Updated ${SYSTEMD_UNIT_NAME}: it differed from what this hack writes for ${program} (local changes belong in a drop-in: systemctl --user edit hackd)`, + }; } /** - * Start with an existing program: never rewrite the unit, only note a - * different `hack` and finish a reload that an earlier write left pending. + * Start without a rewrite: note a unit left stale (it runs another `hack`, + * or writes are not allowed here) and finish a reload that an earlier + * write left pending. */ async function completePendingReload(opts: { readonly status: SystemdServiceStatus; readonly deps: SystemdDeps; readonly expected: { readonly hackBinPath: string } | null; readonly readOnly: boolean; + readonly plan: DaemonReconcilePlan; }): Promise { - const { status, expected } = opts; - if (expected && status.program && expected.hackBinPath !== status.program) { + const { status, expected, plan } = opts; + if ( + expected && + status.program && + expected.hackBinPath !== status.program && + !(await unitRunsExpected({ status, expected })) + ) { logger.info({ message: `${SYSTEMD_UNIT_NAME} runs ${status.program}; this CLI is ${expected.hackBinPath}. To switch the service to it, run: hack daemon install`, }); } + if (plan.blocked === "read-only") { + logger.warn({ + message: `${SYSTEMD_UNIT_NAME} differs from what this hack writes; not changing it here | Run: hack daemon install`, + }); + } if (!status.needsReload) { return null; } @@ -873,7 +1223,9 @@ export async function describeSystemdIncompatibleOwner(opts: { return null; } const expected = await (opts.resolveExpected ?? resolveExpectedSystemdUnit)(); - if (!expected || expected.hackBinPath === status.program) { + // The same executable under another path (a stable symlink to it) is + // not "another hack": a restart runs this version (HACK-2216). + if (!expected || (await unitRunsExpected({ status, expected }))) { return null; } return `hackd (pid ${opts.pid}) is incompatible with this CLI, and ${SYSTEMD_UNIT_NAME} runs ${status.program}, so restarting it would start the same version again | Run: hack daemon install (switches the service to ${expected.hackBinPath})`; @@ -1023,6 +1375,7 @@ export async function installDaemonSystemdService( }); return 1; } + recordInstall({ trace: hooks.trace, before, changed: installed.changed }); if (installed.changed) { logger.success({ message: before?.installed @@ -1079,6 +1432,30 @@ export async function installDaemonSystemdService( return ready ? 0 : 1; } +/** Records what install changed, for `--json`. */ +function recordInstall(opts: { + readonly trace: DaemonCommandTrace | undefined; + readonly before: SystemdServiceStatus | null; + readonly changed: boolean; +}): void { + const { trace, before } = opts; + if (!trace) { + return; + } + trace.manager = "systemd"; + if (!opts.changed) { + trace.action = "none"; + return; + } + trace.action = before?.installed ? "reinstalled" : "installed"; + if (before?.installed && before.drift === "content") { + trace.stale.add("content"); + } + if (before?.installed && before.drift === "program-missing") { + trace.stale.add("program-missing"); + } +} + /** * What install checks before it writes anything: the programs and PATH the * new unit uses ({@link checkInstallPrograms}), then the unit as systemd @@ -1268,16 +1645,24 @@ async function noteRestartIfActiveUnitChanges(opts: { readonly unitPath: string; readonly unitText: string; }): Promise { - const marker = resolve(opts.hooks.paths.root, RESTART_PENDING_FILE); if ( opts.before?.active && (await readTextFile(opts.unitPath)) !== opts.unitText ) { - // Owner-only whatever the umask: a shared ~/.hack would fail the - // service PATH check (and hackd's own state-directory check). - await mkdir(opts.hooks.paths.root, { recursive: true, mode: 0o700 }); - await writeFile(marker, "restart hackd.service to load its new unit\n"); + return await writeRestartMarker({ paths: opts.hooks.paths }); } + return resolve(opts.hooks.paths.root, RESTART_PENDING_FILE); +} + +/** Writes the restart-pending marker; returns its path. */ +async function writeRestartMarker(opts: { + readonly paths: DaemonPaths; +}): Promise { + const marker = resolve(opts.paths.root, RESTART_PENDING_FILE); + // Owner-only whatever the umask: a shared ~/.hack would fail the + // service PATH check (and hackd's own state-directory check). + await mkdir(opts.paths.root, { recursive: true, mode: 0o700 }); + await writeFile(marker, "restart hackd.service to load its new unit\n"); return marker; } diff --git a/src/commands/daemon-upgrade.ts b/src/commands/daemon-upgrade.ts new file mode 100644 index 000000000..151402b89 --- /dev/null +++ b/src/commands/daemon-upgrade.ts @@ -0,0 +1,183 @@ +import { getLaunchdServiceStatus } from "../daemon/launchd.ts"; +import { resolveDaemonPaths } from "../daemon/paths.ts"; +import type { + DaemonCommandAction, + DaemonServiceManager, + DaemonStaleReason, +} from "../daemon/reconcile.ts"; +import { readDaemonStatus } from "../daemon/status.ts"; +import { childEnvironment } from "../lib/child-environment.ts"; +import { isRecord } from "../lib/guards.ts"; +import { resolveSystemdStatus } from "./daemon-systemd.ts"; + +/** + * What `hack update` did about hackd after installing a new release + * (HACK-2216). Best effort: it never fails the update. + * - `skipped`: no login service is installed for this Hack home + * (`no-service`), or it is installed but hackd is not running + * (`not-running`; nothing is started, and the next `hack daemon start` + * brings the service up to the new version); + * - `reconciled`: the new executable's `hack daemon start --json` ran and + * succeeded; + * - `failed`: it ran and failed, or could not be run; `next_step` says how + * to finish by hand. + */ +export type PostUpdateDaemonResult = + | { + readonly status: "skipped"; + readonly reason: "no-service" | "not-running"; + } + | { + readonly status: "reconciled"; + readonly manager: DaemonServiceManager; + readonly action: DaemonCommandAction; + readonly stale_reasons: readonly DaemonStaleReason[]; + } + | { + readonly status: "failed"; + readonly message: string; + readonly next_step: "hack daemon install"; + }; + +/** What the post-update step needs to know before it runs anything. */ +export interface PostUpdateDaemonState { + readonly serviceInstalled: boolean; + readonly daemonRunning: boolean; +} + +const RECONCILE_TIMEOUT_MS = 60_000; +const MAX_OUTPUT_CHARS = 64 * 1024; + +/** + * Runs the same reconcile as `hack daemon start` with the newly installed + * executable, so the decision uses the new version's unit renderer and + * version (this process is still the old CLI). Only when a login service + * is installed and hackd is running: a daemon that was not running is not + * started. + */ +export async function reconcileDaemonAfterUpdate(opts: { + /** The executable the update just replaced, now the new version. */ + readonly binaryPath: string; + /** Defaults to reading the systemd unit or launchd plist and pid file. */ + readonly observe?: () => Promise; + /** Defaults to running the executable with a bounded wait. */ + readonly runCli?: ( + argv: readonly string[] + ) => Promise<{ readonly exitCode: number; readonly stdout: string }>; +}): Promise { + const state = await (opts.observe ?? observePostUpdateDaemon)(); + if (!state.serviceInstalled) { + return { status: "skipped", reason: "no-service" }; + } + if (!state.daemonRunning) { + return { status: "skipped", reason: "not-running" }; + } + let run: { readonly exitCode: number; readonly stdout: string }; + try { + run = await (opts.runCli ?? runBounded)([ + opts.binaryPath, + "daemon", + "start", + "--json", + ]); + } catch (error: unknown) { + return failed(error instanceof Error ? error.message : String(error)); + } + return parseStartEnvelope({ exitCode: run.exitCode, stdout: run.stdout }); +} + +/** + * Reads `hack daemon start --json` output. Anything off the + * `hack.daemon-start/v1` contract is a failure, never a success. + */ +export function parseStartEnvelope(opts: { + readonly exitCode: number; + readonly stdout: string; +}): PostUpdateDaemonResult { + let parsed: unknown; + try { + parsed = JSON.parse(opts.stdout); + } catch { + return failed( + `hack daemon start exited ${opts.exitCode} without a JSON result` + ); + } + if (!isRecord(parsed)) { + return failed("hack daemon start printed an unexpected result"); + } + if (parsed.ok === true && isRecord(parsed.data)) { + const data = parsed.data; + if (data.schema !== "hack.daemon-start/v1" || opts.exitCode !== 0) { + return failed("hack daemon start printed an unexpected result"); + } + return { + status: "reconciled", + manager: oneOf(data.manager, ["systemd", "launchd", "none"], "none"), + action: oneOf( + data.action, + ["none", "started", "restarted", "reinstalled", "stopped", "installed"], + "none" + ), + stale_reasons: Array.isArray(data.stale_reasons) + ? data.stale_reasons.filter( + (reason): reason is DaemonStaleReason => + reason === "content" || + reason === "program-missing" || + reason === "version-mismatch" + ) + : [], + }; + } + const error = isRecord(parsed.error) ? parsed.error : null; + return failed( + typeof error?.message === "string" + ? error.message + : `hack daemon start exited ${opts.exitCode}` + ); +} + +function oneOf( + value: unknown, + allowed: readonly T[], + fallback: T +): T { + return allowed.find((candidate) => candidate === value) ?? fallback; +} + +function failed(message: string): PostUpdateDaemonResult { + return { status: "failed", message, next_step: "hack daemon install" }; +} + +async function observePostUpdateDaemon(): Promise { + const paths = resolveDaemonPaths({}); + let serviceInstalled = false; + if (process.platform === "linux") { + serviceInstalled = + (await resolveSystemdStatus())?.status.installed === true; + } else if (process.platform === "darwin") { + serviceInstalled = (await getLaunchdServiceStatus({ paths })).installed; + } + const daemon = await readDaemonStatus({ paths }); + return { serviceInstalled, daemonRunning: daemon.running }; +} + +async function runBounded( + argv: readonly string[] +): Promise<{ readonly exitCode: number; readonly stdout: string }> { + const proc = Bun.spawn([...argv], { + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + env: childEnvironment({ ...process.env, HACK_NO_INTERACTIVE: "1" }), + }); + const timer = setTimeout(() => proc.kill("SIGKILL"), RECONCILE_TIMEOUT_MS); + try { + const [exitCode, stdout] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + ]); + return { exitCode, stdout: stdout.slice(0, MAX_OUTPUT_CHARS) }; + } finally { + clearTimeout(timer); + } +} diff --git a/src/commands/daemon.ts b/src/commands/daemon.ts index 482bb2616..e2be43b16 100644 --- a/src/commands/daemon.ts +++ b/src/commands/daemon.ts @@ -34,6 +34,10 @@ import { terminateProvenDaemon, waitForProcessExit, } from "../daemon/process.ts"; +import { + createDaemonCommandTrace, + type DaemonCommandTrace, +} from "../daemon/reconcile.ts"; import { runDaemon } from "../daemon/server.ts"; import { type DaemonServiceSummary, @@ -56,9 +60,11 @@ import { detectDockerBackend, } from "../lib/runtime-guidance.ts"; import { logger } from "../ui/logger.ts"; +import { runDaemonCommand } from "./daemon-result.ts"; import { describeSystemdIncompatibleOwner, installDaemonSystemdService, + reconcileRunningSystemdDaemon, resolveSystemdSummary, startViaSystemdIfInstalled, stopViaSystemdIfInstalled, @@ -75,9 +81,10 @@ const optForeground = defineOption({ const startSpec = defineCommand({ name: "start", - summary: "Start hackd (local daemon)", + summary: + "Start hackd (local daemon); reinstalls and restarts a stale login service", group: "Diagnostics", - options: [optForeground] as const, + options: [optForeground, optJson] as const, positionals: [], subcommands: [], } as const); @@ -86,7 +93,7 @@ const stopSpec = defineCommand({ name: "stop", summary: "Stop hackd", group: "Diagnostics", - options: [], + options: [optJson] as const, positionals: [], subcommands: [], } as const); @@ -131,7 +138,7 @@ const restartSpec = defineCommand({ name: "restart", summary: "Restart hackd", group: "Diagnostics", - options: [], + options: [optJson] as const, positionals: [], subcommands: [], } as const); @@ -169,7 +176,13 @@ const installSpec = defineCommand({ summary: "Install hackd as a login service (launchd on macOS, systemd user unit on Linux)", group: "Diagnostics", - options: [optRunAtLoad, optNoRunAtLoad, optGuiOnly, optNoGuiOnly] as const, + options: [ + optRunAtLoad, + optNoRunAtLoad, + optGuiOnly, + optNoGuiOnly, + optJson, + ] as const, positionals: [], subcommands: [], } as const); @@ -250,7 +263,36 @@ async function handleDaemonStart({ readonly ctx: CliContext; readonly args: DaemonStartArgs; }): Promise { + const json = args.options.json === true; + if (json && args.options.foreground) { + throw new CliUsageError("--json cannot be used with --foreground"); + } const paths = resolveDaemonPaths({}); + return await runDaemonCommand({ + command: "start", + json, + paths, + run: (trace) => + startDaemon({ + paths, + foreground: args.options.foreground === true, + trace, + }), + }); +} + +/** + * `hack daemon start`. With an installed login service it also brings a + * stale service up to this CLI (HACK-2216): see + * {@link reconcileRunningSystemdDaemon}, `prepareSystemdUnitForStart` and + * {@link startViaLaunchdIfLoaded}. `trace` records what it did for `--json`. + */ +async function startDaemon(opts: { + readonly paths: DaemonPaths; + readonly foreground: boolean; + readonly trace: DaemonCommandTrace; +}): Promise { + const { paths, trace } = opts; const status = await readDaemonStatus({ paths }); // A pid that survived an unclean shutdown may now belong to another @@ -258,18 +300,47 @@ async function handleDaemonStart({ const livePid = status.running ? status.pid : null; const pidIsDaemon = livePid !== null && (await isHackDaemonProcess({ pid: livePid })); + let replaced = false; if (livePid !== null && pidIsDaemon) { const running = await handleRunningDaemonAtStart({ pid: livePid, socketExists: status.socketExists, paths, + trace, + foreground: opts.foreground, }); if (running !== null) { return running; } + replaced = true; + } + const code = await startStoppedDaemon({ + paths, + foreground: opts.foreground, + trace, + trackedPid: status.pid, + stalePid: pidIsDaemon ? null : livePid, + }); + if (code === 0 && replaced && trace.action === "started") { + trace.action = "restarted"; } + return code; +} - const sweep = await sweepOrphanDaemons({ trackedPid: status.pid, paths }); +/** `start` once no hackd of this home runs (or it was just replaced). */ +async function startStoppedDaemon(opts: { + readonly paths: DaemonPaths; + readonly foreground: boolean; + readonly trace: DaemonCommandTrace; + readonly trackedPid: number | null; + /** A live pid already shown not to be hackd. */ + readonly stalePid: number | null; +}): Promise { + const { paths, trace } = opts; + const sweep = await sweepOrphanDaemons({ + trackedPid: opts.trackedPid, + paths, + }); if (!sweep.allStopped) { return 1; } @@ -278,46 +349,57 @@ async function handleDaemonStart({ // before touching the pid/socket files: systemd (a pending restart) or // another starter may be writing them right now, and hackd replaces its // own socket when it starts. - if (process.platform === "linux" && !args.options.foreground) { + if (process.platform === "linux" && !opts.foreground) { const viaSystemd = await startViaSystemdIfInstalled({ paths, waitForReady: (timeoutMs) => waitForDaemonStart({ paths, timeoutMs }), + trace, }); if (viaSystemd !== null) { return viaSystemd; } + // The unit serves another home, or systemd is unusable: hackd starts + // directly, whatever was noted about the unit. + trace.manager = "none"; } const raced = await clearStaleDaemonFilesBeforeStart({ paths, - stalePid: pidIsDaemon ? null : livePid, + stalePid: opts.stalePid, }); if (raced !== null) { return raced; } - if (args.options.foreground) { + if (opts.foreground) { await runDaemon({ paths, foreground: true }); return 0; } - const viaLaunchd = await startViaLaunchdIfLoaded({ paths }); + const viaLaunchd = await startViaLaunchdIfLoaded({ paths, trace }); if (viaLaunchd !== null) { return viaLaunchd; } - return await startDaemonDetached({ paths }); + const started = await startDaemonDetached({ paths }); + trace.manager = "none"; + trace.action = started === 0 ? "started" : trace.action; + return started; } /** * When launchd manages the daemon, start through launchd. Returns null when - * no launchd service is loaded (unchanged macOS behavior, moved out of - * `handleDaemonStart`). + * no launchd service is loaded, or with `onlyIfStale` when its plist is + * current. A plist whose program is gone ("repaired") or whose content + * differs from what this CLI renders ("refreshed", HACK-2216) is rewritten + * first; `kickstart -k` then (re)starts hackd from it. */ async function startViaLaunchdIfLoaded(opts: { readonly paths: DaemonPaths; + readonly trace: DaemonCommandTrace; + readonly onlyIfStale?: boolean; }): Promise { - const { paths } = opts; + const { paths, trace } = opts; // When launchd manages the daemon, start through launchd — spawning a // bare process next to a loaded agent means two process managers fight // over one socket and pid file. @@ -325,13 +407,25 @@ async function startViaLaunchdIfLoaded(opts: { if (!launchdStatus?.loaded) { return null; } + trace.manager = "launchd"; const repair = await repairLaunchdProgramIfInvalid({ paths }); if (repair === "repaired") { + trace.stale.add("program-missing"); logger.warn({ message: "Repaired launchd service: its program path was invalid (stale or compiled-binary virtual path)", }); + } else if (repair === "refreshed") { + trace.stale.add("content"); + logger.warn({ + message: + "Updated launchd service: its plist differed from what this hack writes", + }); + } else if (opts.onlyIfStale) { + return null; } + trace.action = + repair === "repaired" || repair === "refreshed" ? "reinstalled" : "started"; // launchd owns the daemon: never spawn a bare process next to it — // two process managers for one socket is how machines end up with // start-then-exit flapping and stale-pid contradictions. @@ -465,23 +559,49 @@ export async function stopProvenDaemon(opts: { /** * `hack daemon start` found a live pid. Returns an exit code when start is - * done (already running, or an incompatible systemd-run hackd that only - * `hack daemon install` can switch), or null after replacing an - * incompatible directly started hackd. + * done (already running, a stale login service brought up to this CLI, or + * an incompatible systemd-run hackd that only `hack daemon install` can + * switch), or null after replacing an incompatible directly started hackd. */ async function handleRunningDaemonAtStart(opts: { readonly pid: number; readonly socketExists: boolean; readonly paths: DaemonPaths; + readonly trace: DaemonCommandTrace; + readonly foreground: boolean; }): Promise { + const { paths, trace } = opts; const api = await checkDaemonApi({ socketExists: opts.socketExists, - paths: opts.paths, + paths, }); + if (process.platform === "linux" && !opts.foreground) { + const reconciled = await reconcileRunningSystemdDaemon({ + paths, + pid: opts.pid, + version: api.reachable ? api.version : null, + waitForReady: (timeoutMs) => waitForDaemonStart({ paths, timeoutMs }), + trace, + }); + if (reconciled !== null) { + return reconciled; + } + } if (!(api.reachable && !api.compatible)) { + if (process.platform === "darwin" && !opts.foreground) { + const refreshed = await startViaLaunchdIfLoaded({ + paths, + trace, + onlyIfStale: true, + }); + if (refreshed !== null) { + return refreshed; + } + } logger.success({ message: `hackd already running (pid ${opts.pid})` }); return 0; } + trace.stale.add("version-mismatch"); // systemd runs this hackd from a unit that points at another `hack`: // replacing it would only make systemd start the same old binary again. // Say so instead (start never rewrites such a unit). @@ -582,12 +702,39 @@ async function startDaemonDetached(opts: { } async function handleDaemonStop({ - args: _args, + args, }: { readonly ctx: CliContext; readonly args: DaemonStopArgs; }): Promise { const paths = resolveDaemonPaths({}); + return await runDaemonCommand({ + command: "stop", + json: args.options.json === true, + paths, + run: (trace) => stopDaemon({ paths, trace }), + }); +} + +/** `hack daemon stop`; `trace` records who stopped what for `--json`. */ +async function stopDaemon(opts: { + readonly paths: DaemonPaths; + readonly trace: DaemonCommandTrace; +}): Promise { + const { paths, trace } = opts; + const code = await stopDaemonSteps({ paths, trace }); + if (trace.manager === "none" && process.platform === "darwin") { + const launchd = await resolveLaunchdStatus({ paths }); + trace.manager = launchd?.loaded ? "launchd" : "none"; + } + return code; +} + +async function stopDaemonSteps(opts: { + readonly paths: DaemonPaths; + readonly trace: DaemonCommandTrace; +}): Promise { + const { paths, trace } = opts; const initial = await readDaemonStatus({ paths }); // An installed hackd.service is stopped through systemd even when the pid @@ -619,6 +766,10 @@ async function handleDaemonStop({ pollMs: 200, }); } + if (viaSystemd.handled) { + trace.manager = "systemd"; + trace.action = "stopped"; + } const status = viaSystemd.handled ? await readDaemonStatus({ paths }) @@ -663,6 +814,7 @@ async function handleDaemonStop({ if (!(await stopProvenDaemon({ proof, paths }))) { return 1; } + trace.action = "stopped"; logger.success({ message: "hackd stopped" }); return 0; } @@ -722,15 +874,23 @@ async function handleDaemonStatus({ async function checkDaemonApi(opts: { readonly socketExists: boolean; readonly paths: DaemonPaths; -}): Promise<{ readonly reachable: boolean; readonly compatible: boolean }> { +}): Promise<{ + readonly reachable: boolean; + readonly compatible: boolean; + readonly version: string | null; +}> { if (!opts.socketExists) { - return { reachable: false, compatible: false }; + return { reachable: false, compatible: false, version: null }; } const probe = await probeDaemonApi({ socketPath: opts.paths.socketPath, timeoutMs: 500, }); - return { reachable: probe.reachable, compatible: probe.compatible }; + return { + reachable: probe.reachable, + compatible: probe.compatible, + version: probe.version, + }; } async function resolveLaunchdStatus(opts: { @@ -954,48 +1114,64 @@ async function handleDaemonClear({ } async function handleDaemonRestart({ - args: _args, - ctx, + args, }: { readonly ctx: CliContext; readonly args: DaemonRestartArgs; }): Promise { - const stopArgs: DaemonStopArgs = { - options: {}, - positionals: {}, - raw: { argv: [], positionals: [] }, - }; - const startArgs: DaemonStartArgs = { - options: { foreground: false }, - positionals: {}, - raw: { argv: [], positionals: [] }, - }; + const paths = resolveDaemonPaths({}); + return await runDaemonCommand({ + command: "restart", + json: args.options.json === true, + paths, + run: (trace) => restartDaemon({ paths, trace }), + }); +} +/** + * `hack daemon restart`: stop, then start, which also brings a stale login + * service up to this CLI. `trace` reports the start half. + */ +async function restartDaemon(opts: { + readonly paths: DaemonPaths; + readonly trace: DaemonCommandTrace; +}): Promise { + const { paths, trace } = opts; // Restarting an incompatible hackd that systemd runs from another, // still existing `hack` only starts the same version again. - const paths = resolveDaemonPaths({}); const status = await readDaemonStatus({ paths }); - if (process.platform === "linux" && status.running && status.pid !== null) { + if (status.running && status.pid !== null) { const api = await checkDaemonApi({ socketExists: status.socketExists, paths, }); + const incompatible = api.reachable && !api.compatible; + if (incompatible) { + trace.stale.add("version-mismatch"); + } const owner = - api.reachable && !api.compatible + incompatible && process.platform === "linux" ? await describeSystemdIncompatibleOwner({ pid: status.pid }) : null; if (owner !== null) { + trace.manager = "systemd"; logger.warn({ message: owner }); return 1; } } // A refused stop (the pid could not be proven to be this HACK_HOME's // hackd) must not turn into "already running" success. - const stopped = await handleDaemonStop({ ctx, args: stopArgs }); + const stopTrace = createDaemonCommandTrace(); + const stopped = await stopDaemon({ paths, trace: stopTrace }); if (stopped !== 0) { + trace.manager = stopTrace.manager; return stopped; } - return await handleDaemonStart({ ctx, args: startArgs }); + const started = await startDaemon({ paths, foreground: false, trace }); + if (started === 0 && trace.action === "started") { + trace.action = "restarted"; + } + return started; } async function waitForDaemonStart({ @@ -1022,8 +1198,22 @@ async function handleDaemonInstall({ readonly ctx: CliContext; readonly args: DaemonInstallArgs; }): Promise { + const paths = resolveDaemonPaths({}); + return await runDaemonCommand({ + command: "install", + json: args.options.json === true, + paths, + run: (trace) => installDaemon({ args, paths, trace }), + }); +} + +async function installDaemon(opts: { + readonly args: DaemonInstallArgs; + readonly paths: DaemonPaths; + readonly trace: DaemonCommandTrace; +}): Promise { + const { args, paths, trace } = opts; if (process.platform === "linux") { - const paths = resolveDaemonPaths({}); return await installDaemonSystemdService({ paths, ignoredOptions: ( @@ -1034,6 +1224,7 @@ async function handleDaemonInstall({ startDetached: () => startDaemonDetached({ paths }), stopDirect: (proof) => stopProvenDaemon({ proof, paths }), waitForReady: (timeoutMs) => waitForDaemonStart({ paths, timeoutMs }), + trace, }); } if (process.platform !== "darwin") { @@ -1044,8 +1235,8 @@ async function handleDaemonInstall({ return 1; } assertLaunchdAccess(); + trace.manager = "launchd"; - const paths = resolveDaemonPaths({}); const controlPlane = await readControlPlaneConfig({}); const runAtLoad = resolveRunAtLoadOption({ @@ -1092,6 +1283,7 @@ async function handleDaemonInstall({ message: "hackd launchd service already installed (config unchanged)", }); } else { + trace.action = "installed"; logger.success({ message: "hackd launchd service installed" }); } diff --git a/src/commands/update.ts b/src/commands/update.ts index 00358f1eb..8696e77ac 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -20,6 +20,10 @@ import { resolveUpdateTarget, selectCliTarballAsset, } from "../lib/self-update.ts"; +import { + type PostUpdateDaemonResult, + reconcileDaemonAfterUpdate, +} from "./daemon-upgrade.ts"; const optCheck = defineOption({ name: "check", @@ -248,6 +252,9 @@ const handleUpdate: CommandHandlerFor = async ({ } const mutagenProvision = await ensureMutagenAfterUpdate(); + // An installed login service still runs the old hackd: bring it up to + // the new executable (best effort; never fails the update). + const daemon = await reconcileDaemonAfterUpdate({ binaryPath: bin.path }); return writeResult({ json: args.options.json === true, @@ -262,11 +269,30 @@ const handleUpdate: CommandHandlerFor = async ({ assetsDir, mutagen: mutagenProvision, backupManifest, + daemon, }, - human: `Updated to v${latestVersion}. Backup manifest: ${backupManifest}`, + human: [ + `Updated to v${latestVersion}. Backup manifest: ${backupManifest}`, + ...describePostUpdateDaemon(daemon), + ].join("\n"), }); }; +/** The human line for {@link reconcileDaemonAfterUpdate}, if any. */ +function describePostUpdateDaemon(result: PostUpdateDaemonResult): string[] { + if (result.status === "skipped") { + return []; + } + if (result.status === "failed") { + return [ + `hackd was not updated: ${result.message} | Run: ${result.next_step}`, + ]; + } + return result.action === "none" + ? [] + : [`hackd ${result.action} (${result.manager}) on the new version.`]; +} + async function confirmInstall(opts: { readonly yes: boolean; readonly isTty: boolean; @@ -439,6 +465,7 @@ type UpdateOutput = readonly assetsDir: string; readonly mutagen?: MutagenProvisionResult; readonly backupManifest?: string; + readonly daemon?: PostUpdateDaemonResult; } | { readonly ok: false; diff --git a/src/daemon/client.ts b/src/daemon/client.ts index f0c394d95..05e7698d9 100644 --- a/src/daemon/client.ts +++ b/src/daemon/client.ts @@ -20,6 +20,8 @@ type PackageJsonType = { } & Record; const packageJson = pkg as unknown as PackageJsonType; +/** This CLI's version; a hackd reporting another one is incompatible. */ +export const DAEMON_CLI_VERSION: string = packageJson.version; const DAEMON_AUTOSTART_COOLDOWN_MS = 15_000; const DAEMON_AUTOSTART_WAIT_MS = 3500; const DAEMON_AUTOSTART_POLL_MS = 125; diff --git a/src/daemon/launchd.ts b/src/daemon/launchd.ts index 336e8114b..b1f71709e 100644 --- a/src/daemon/launchd.ts +++ b/src/daemon/launchd.ts @@ -383,17 +383,56 @@ export function hasBoundedLaunchdOutput(opts: { ); } +/** + * `repaired`: the program path was invalid or stale and was replaced; + * `refreshed`: the program was current but the rest of the plist (its + * environment, output paths or other keys) differed from what this CLI + * renders, and the plist was rewritten (HACK-2216). + */ export type LaunchdProgramRepairResult = | "ok" | "repaired" + | "refreshed" | "not-installed" | "failed"; +/** The install options an existing plist was rendered with. */ +export function readLaunchdPlistOptions(opts: { + readonly plistText: string; +}): Pick { + return { + runAtLoad: opts.plistText.includes("RunAtLoad\n "), + guiSessionOnly: opts.plistText.includes("LimitLoadToSessionType"), + }; +} + +/** + * Whether an installed plist is exactly what this CLI renders for the same + * program, home and install options. Any other difference (an environment + * entry added or removed by an upgrade, unbounded output paths) is stale. + */ +export function isLaunchdPlistCurrent(opts: { + readonly plistText: string; + readonly hackBinPath: string; + readonly home: string; +}): boolean { + return ( + opts.plistText === + renderLaunchdPlist({ + hackBinPath: opts.hackBinPath, + home: opts.home, + ...readLaunchdPlistOptions({ plistText: opts.plistText }), + }) + ); +} + /** * Repairs an installed launchd plist whose program path is invalid — a * compiled-binary virtual path (/$bunfs) or a binary that no longer exists * (e.g. an upgraded homebrew Cellar path). Broken plists leave launchd - * flapping with exit status 78 and the daemon unable to stay up. + * flapping with exit status 78 and the daemon unable to stay up. A plist + * whose program is current but whose other content differs from what this + * CLI renders is rewritten too ("refreshed"). * * Reports "not-installed" without reading the plist when * {@link resolveLaunchdAccess} denies access: this process does not own the @@ -421,7 +460,17 @@ export async function repairLaunchdProgramIfInvalid(opts: { // the OLD binary forever (incompatible-daemon loop). Normalize to the // current stable resolution whenever it differs. const valid = exists && (resolved === null || program === resolved); - if (valid && hasBoundedLaunchdOutput({ plistText })) { + // Without a resolvable hack this CLI cannot render (or install) a new + // plist, so only the output paths are checked then. + const current = + resolved === null + ? hasBoundedLaunchdOutput({ plistText }) + : isLaunchdPlistCurrent({ + plistText, + hackBinPath: resolved, + home: resolveUserHomeDir(), + }); + if (valid && current) { return "ok"; } if (!exists && resolved === null) { @@ -432,11 +481,13 @@ export async function repairLaunchdProgramIfInvalid(opts: { paths: opts.paths, config: { installed: true, - runAtLoad: plistText.includes("RunAtLoad\n "), - guiSessionOnly: plistText.includes("LimitLoadToSessionType"), + ...readLaunchdPlistOptions({ plistText }), }, }); - return result.ok ? "repaired" : "failed"; + if (!result.ok) { + return "failed"; + } + return valid ? "refreshed" : "repaired"; } export interface LaunchdUninstallResult { diff --git a/src/daemon/reconcile.ts b/src/daemon/reconcile.ts new file mode 100644 index 000000000..0075e7c07 --- /dev/null +++ b/src/daemon/reconcile.ts @@ -0,0 +1,299 @@ +/** + * What `hack daemon start` (and restart, and the post-update hook) does + * about an installed login service that no longer matches this CLI + * (HACK-2216). The decision is pure; `src/commands/daemon-systemd.ts` and + * `src/commands/daemon.ts` observe the unit and the running daemon, call + * {@link planDaemonReconcile} and carry the plan out. + */ + +/** Who runs hackd: a systemd user unit, a launchd agent, or nobody. */ +export type DaemonServiceManager = "systemd" | "launchd" | "none"; + +/** + * Why the service or daemon is stale, as a closed set for `--json`: + * - `content`: the unit or plist differs from what this CLI renders for the + * same program; + * - `program-missing`: the program it runs no longer exists; + * - `version-mismatch`: the running hackd reports another version than this + * CLI. + */ +export type DaemonStaleReason = + | "content" + | "program-missing" + | "version-mismatch"; + +export const DAEMON_STALE_REASON_ORDER: readonly DaemonStaleReason[] = [ + "content", + "program-missing", + "version-mismatch", +]; + +/** What a `hack daemon` command did, as reported by `--json`. */ +export type DaemonCommandAction = + | "none" + | "started" + | "restarted" + | "reinstalled" + | "stopped" + | "installed"; + +/** + * How the installed unit compares with what this CLI renders for the + * unit's own program (see `computeDrift` in `src/daemon/systemd.ts`). + * `program-unknown`: its ExecStart is not understood, so it is never + * rewritten. + */ +export type ServiceUnitState = + | "current" + | "content" + | "program-missing" + | "program-unknown"; + +/** + * Why a stale service was not repaired: + * - `read-only`: `HACK_DAEMON_SERVICE_READ_ONLY=1` (`hack doctor --fix` + * without a TTY) forbids writing the unit; + * - `other-program`: the unit runs another existing `hack`, which the user + * may have chosen; `hack daemon install` switches it; + * - `unknown-program`: what the unit runs cannot be read, so a restart is + * not known to run this version. + */ +export type DaemonReconcileBlock = + | "read-only" + | "other-program" + | "unknown-program"; + +export interface DaemonReconcileInput { + readonly unit: ServiceUnitState; + /** + * The unit runs this CLI's executable: the same path, or a path that + * resolves to the same file. False when the program is unknown or missing. + */ + readonly programIsOurs: boolean; + readonly readOnly: boolean; + /** + * The hackd the service manager runs now, or null when it runs none. + * `version` is null when the daemon's API did not report one; that is not + * evidence of a mismatch. + */ + readonly running: { readonly version: string | null } | null; + /** + * An earlier repair rewrote the unit but its restart did not complete, + * so the running service still has the old unit loaded. + */ + readonly restartPending?: boolean; + readonly cliVersion: string; +} + +export interface DaemonReconcilePlan { + /** + * `program`: retarget ExecStart to this CLI (the old program is gone); + * `content`: rewrite the whole unit for its own program. + */ + readonly rewrite: "none" | "content" | "program"; + /** + * `keep`: leave the running daemon as it is; `refuse`: stop with an error + * (see `blocked`). + */ + readonly service: "keep" | "start" | "restart" | "refuse"; + readonly reasons: readonly DaemonStaleReason[]; + /** Why a stale part was left as it is; null when nothing was. */ + readonly blocked: DaemonReconcileBlock | null; +} + +/** + * Decides how to bring an installed service and its daemon up to this CLI. + * + * Invariants: + * - a unit is rewritten only when it runs this CLI's executable (`content`) + * or its program is gone (`program`), and never under `readOnly`; + * - a running daemon is restarted only when the restart is known to run + * this CLI's executable, or the unit changed under it; + * - a version mismatch that a restart cannot fix is refused, never + * reported as success. + */ +export function planDaemonReconcile( + input: DaemonReconcileInput +): DaemonReconcilePlan { + const mismatch = + input.running !== null && + input.running.version !== null && + input.running.version !== input.cliVersion; + const reasons = staleReasons({ input, mismatch }); + + if (input.unit === "program-missing") { + if (input.readOnly) { + return { + rewrite: "none", + service: "refuse", + reasons, + blocked: "read-only", + }; + } + return { + rewrite: "program", + service: input.running === null ? "start" : "restart", + reasons, + blocked: null, + }; + } + + const { rewrite, blocked } = planContentRewrite(input); + if (input.running === null) { + return { rewrite, service: "start", reasons, blocked }; + } + if (mismatch && !input.programIsOurs) { + return { + rewrite, + service: "refuse", + reasons, + blocked: + input.unit === "program-unknown" ? "unknown-program" : "other-program", + }; + } + const restart = + mismatch || rewrite !== "none" || input.restartPending === true; + return { rewrite, service: restart ? "restart" : "keep", reasons, blocked }; +} + +function staleReasons(opts: { + readonly input: DaemonReconcileInput; + readonly mismatch: boolean; +}): DaemonStaleReason[] { + const { input } = opts; + const reasons: DaemonStaleReason[] = []; + if (input.unit === "content" || input.restartPending === true) { + reasons.push("content"); + } + if (input.unit === "program-missing") { + reasons.push("program-missing"); + } + if (opts.mismatch) { + reasons.push("version-mismatch"); + } + return reasons; +} + +/** Whether stale content (not a missing program) may be rewritten. */ +function planContentRewrite(input: DaemonReconcileInput): { + readonly rewrite: "none" | "content"; + readonly blocked: DaemonReconcileBlock | null; +} { + if (input.unit !== "content") { + return { rewrite: "none", blocked: null }; + } + if (input.readOnly) { + return { rewrite: "none", blocked: "read-only" }; + } + return input.programIsOurs + ? { rewrite: "content", blocked: null } + : { rewrite: "none", blocked: "other-program" }; +} + +/** The `--json` action for a plan that was carried out. */ +export function reconcileAction( + plan: DaemonReconcilePlan +): DaemonCommandAction { + if (plan.rewrite !== "none") { + return "reinstalled"; + } + if (plan.service === "restart") { + return "restarted"; + } + if (plan.service === "start") { + return "started"; + } + return "none"; +} + +/** + * What one `hack daemon` command observed and did, filled in as it runs + * and rendered by `--json`. The decisions themselves are made by + * {@link planDaemonReconcile} and the existing ownership checks; this only + * records their result. + */ +export interface DaemonCommandTrace { + manager: DaemonServiceManager; + action: DaemonCommandAction; + readonly stale: Set; +} + +export function createDaemonCommandTrace(): DaemonCommandTrace { + return { manager: "none", action: "none", stale: new Set() }; +} + +/** Records a carried-out plan on `trace`. */ +export function recordReconcile(opts: { + readonly trace: DaemonCommandTrace | undefined; + readonly manager: DaemonServiceManager; + readonly plan: DaemonReconcilePlan; +}): void { + const { trace } = opts; + if (!trace) { + return; + } + trace.manager = opts.manager; + trace.action = reconcileAction(opts.plan); + for (const reason of opts.plan.reasons) { + trace.stale.add(reason); + } +} + +/** The stale reasons in a stable order for output. */ +export function sortedStaleReasons( + stale: ReadonlySet +): DaemonStaleReason[] { + return DAEMON_STALE_REASON_ORDER.filter((reason) => stale.has(reason)); +} + +/** The `hack daemon` commands that report a `--json` result. */ +export type DaemonCommandName = "start" | "stop" | "restart" | "install"; + +/** The daemon as observed after the command. */ +export interface DaemonObservation { + readonly running: boolean; + readonly pid: number | null; + /** What its API reported; null when it did not answer. */ + readonly version: string | null; +} + +/** + * `data` of the `--json` envelope for `hack daemon `: + * `hack.daemon-/v1`. Closed enums only; no paths and no values + * from the environment. + */ +export function buildDaemonCommandData(opts: { + readonly command: DaemonCommandName; + readonly trace: DaemonCommandTrace; + readonly daemon: DaemonObservation; + readonly cliVersion: string; +}): { + readonly schema: string; + readonly manager: DaemonServiceManager; + readonly action: DaemonCommandAction; + readonly stale: boolean; + readonly stale_reasons: readonly DaemonStaleReason[]; + readonly cli_version: string; + readonly daemon: { + readonly running: boolean; + readonly pid: number | null; + readonly version: string | null; + readonly compatible: boolean; + }; +} { + const reasons = sortedStaleReasons(opts.trace.stale); + return { + schema: `hack.daemon-${opts.command}/v1`, + manager: opts.trace.manager, + action: opts.trace.action, + stale: reasons.length > 0, + stale_reasons: reasons, + cli_version: opts.cliVersion, + daemon: { + running: opts.daemon.running, + pid: opts.daemon.running ? opts.daemon.pid : null, + version: opts.daemon.version, + compatible: opts.daemon.version === opts.cliVersion, + }, + }; +} diff --git a/src/daemon/systemd.ts b/src/daemon/systemd.ts index 33da37e10..a772e427c 100644 --- a/src/daemon/systemd.ts +++ b/src/daemon/systemd.ts @@ -47,8 +47,10 @@ import { * first `hack` on PATH), not its symlink target, so a stable symlink such as * `~/.local/bin/hack` keeps working when an upgrade moves the real binary. * When the recorded program no longer exists, `hack daemon start` rewrites - * the unit; any other difference is reported as drift and fixed by - * `hack daemon install`. + * the unit. When the unit runs this CLI's executable but differs from what + * it renders, or its hackd reports another version, start rewrites and + * restarts it (HACK-2216, `src/daemon/reconcile.ts`). A unit that runs + * another existing `hack` is reported and switched by `hack daemon install`. * - `Restart=on-failure`: a crash restarts hackd; `hack daemon stop` (a clean * stop through systemctl) does not. Start limits stop a crash loop. * - The default `KillMode=control-group` is kept on purpose: stopping or diff --git a/src/lib/cli-result.ts b/src/lib/cli-result.ts index 099e20fb7..826e52794 100644 --- a/src/lib/cli-result.ts +++ b/src/lib/cli-result.ts @@ -47,6 +47,7 @@ export const HACK_ERROR_CODES = [ "E_ADMISSION_LIMIT", "E_REQUEST_CONFLICT", "E_JOURNAL_VERSION", + "E_DAEMON_SERVICE", "E_USAGE", "E_UNEXPECTED", ] as const; diff --git a/src/ui/logger.ts b/src/ui/logger.ts index 0f5318734..e32c324fe 100644 --- a/src/ui/logger.ts +++ b/src/ui/logger.ts @@ -158,8 +158,38 @@ function logWithGum(level: LogLevel, { message, fields }: LogInput): void { } } +let problemSink: ((message: string) => void) | null = null; + +/** + * Records the warning and error messages logged until `stop` is called, so + * a `--json` handler whose steps report failures through the logger can put + * the last one in its error envelope. The lines are still logged as usual + * (to stderr under the `console` backend). One capture at a time. + */ +export function captureLoggedProblems(): { + readonly last: () => string | null; + readonly stop: () => void; +} { + let last: string | null = null; + const sink = (message: string) => { + last = message; + }; + problemSink = sink; + return { + last: () => last, + stop: () => { + if (problemSink === sink) { + problemSink = null; + } + }, + }; +} + export const logger: Logger = (() => { const emit = (level: LogLevel, input: LogInput) => { + if (problemSink && (level === "warn" || level === "error")) { + problemSink(input.message); + } const backend = resolveBackend(); if (backend === "gum") { return logWithGum(level, input); diff --git a/tests/daemon-reconcile.test.ts b/tests/daemon-reconcile.test.ts new file mode 100644 index 000000000..c8be7a40d --- /dev/null +++ b/tests/daemon-reconcile.test.ts @@ -0,0 +1,345 @@ +import { expect, test } from "bun:test"; + +import { + parseStartEnvelope, + reconcileDaemonAfterUpdate, +} from "../src/commands/daemon-upgrade.ts"; +import { + isLaunchdPlistCurrent, + renderLaunchdPlist, +} from "../src/daemon/launchd.ts"; +import { + buildDaemonCommandData, + createDaemonCommandTrace, + type DaemonReconcileInput, + planDaemonReconcile, + reconcileAction, +} from "../src/daemon/reconcile.ts"; + +/** HACK-2216: the reconcile decision and its `--json` contract, no I/O. */ + +const base: DaemonReconcileInput = { + unit: "current", + programIsOurs: true, + readOnly: false, + running: null, + cliVersion: "2.0.0", +}; + +test("a current unit with nothing running is only started", () => { + const plan = planDaemonReconcile(base); + expect(plan).toEqual({ + rewrite: "none", + service: "start", + reasons: [], + blocked: null, + }); + expect(reconcileAction(plan)).toBe("started"); +}); + +test("a current unit running this version is kept", () => { + const plan = planDaemonReconcile({ + ...base, + running: { version: "2.0.0" }, + }); + expect(plan.service).toBe("keep"); + expect(reconcileAction(plan)).toBe("none"); +}); + +test("a version mismatch under this hack's unit restarts it", () => { + const plan = planDaemonReconcile({ + ...base, + running: { version: "1.0.0" }, + }); + expect(plan).toEqual({ + rewrite: "none", + service: "restart", + reasons: ["version-mismatch"], + blocked: null, + }); + expect(reconcileAction(plan)).toBe("restarted"); +}); + +test("an unknown daemon version is not a mismatch", () => { + const plan = planDaemonReconcile({ ...base, running: { version: null } }); + expect(plan.service).toBe("keep"); + expect(plan.reasons).toEqual([]); +}); + +test("stale content of this hack's unit is rewritten, then started or restarted", () => { + const stopped = planDaemonReconcile({ ...base, unit: "content" }); + expect(stopped).toMatchObject({ rewrite: "content", service: "start" }); + expect(reconcileAction(stopped)).toBe("reinstalled"); + const running = planDaemonReconcile({ + ...base, + unit: "content", + running: { version: "1.0.0" }, + }); + expect(running).toEqual({ + rewrite: "content", + service: "restart", + reasons: ["content", "version-mismatch"], + blocked: null, + }); +}); + +test("a unit for another existing hack is never rewritten", () => { + const stopped = planDaemonReconcile({ + ...base, + unit: "content", + programIsOurs: false, + }); + expect(stopped).toMatchObject({ + rewrite: "none", + service: "start", + blocked: "other-program", + }); + const compatible = planDaemonReconcile({ + ...base, + programIsOurs: false, + running: { version: "2.0.0" }, + }); + expect(compatible.service).toBe("keep"); + const mismatch = planDaemonReconcile({ + ...base, + programIsOurs: false, + running: { version: "1.0.0" }, + }); + expect(mismatch).toMatchObject({ + rewrite: "none", + service: "refuse", + blocked: "other-program", + }); +}); + +test("an unreadable ExecStart with a mismatched daemon is refused", () => { + const plan = planDaemonReconcile({ + ...base, + unit: "program-unknown", + programIsOurs: false, + running: { version: "1.0.0" }, + }); + expect(plan).toMatchObject({ + service: "refuse", + blocked: "unknown-program", + rewrite: "none", + }); +}); + +test("a missing program is retargeted, except read-only", () => { + const plan = planDaemonReconcile({ + ...base, + unit: "program-missing", + programIsOurs: false, + }); + expect(plan).toEqual({ + rewrite: "program", + service: "start", + reasons: ["program-missing"], + blocked: null, + }); + expect( + planDaemonReconcile({ + ...base, + unit: "program-missing", + programIsOurs: false, + running: { version: "1.0.0" }, + }).service + ).toBe("restart"); + expect( + planDaemonReconcile({ + ...base, + unit: "program-missing", + programIsOurs: false, + readOnly: true, + }) + ).toMatchObject({ rewrite: "none", service: "refuse", blocked: "read-only" }); +}); + +test("read-only never rewrites stale content but still restarts a mismatch", () => { + const plan = planDaemonReconcile({ + ...base, + unit: "content", + readOnly: true, + running: { version: "1.0.0" }, + }); + expect(plan).toMatchObject({ + rewrite: "none", + service: "restart", + blocked: "read-only", + }); + expect( + planDaemonReconcile({ + ...base, + unit: "content", + readOnly: true, + running: { version: "2.0.0" }, + }).service + ).toBe("keep"); +}); + +test("a restart left pending by an earlier rewrite is completed", () => { + const plan = planDaemonReconcile({ + ...base, + running: { version: "2.0.0" }, + restartPending: true, + }); + expect(plan).toMatchObject({ service: "restart", reasons: ["content"] }); +}); + +test("the --json data is closed and schema-tagged", () => { + const trace = createDaemonCommandTrace(); + trace.manager = "systemd"; + trace.action = "reinstalled"; + trace.stale.add("version-mismatch"); + trace.stale.add("content"); + expect( + buildDaemonCommandData({ + command: "start", + trace, + daemon: { running: true, pid: 12, version: "2.0.0" }, + cliVersion: "2.0.0", + }) + ).toEqual({ + schema: "hack.daemon-start/v1", + manager: "systemd", + action: "reinstalled", + stale: true, + stale_reasons: ["content", "version-mismatch"], + cli_version: "2.0.0", + daemon: { running: true, pid: 12, version: "2.0.0", compatible: true }, + }); + expect( + buildDaemonCommandData({ + command: "stop", + trace: createDaemonCommandTrace(), + daemon: { running: false, pid: 12, version: null }, + cliVersion: "2.0.0", + }) + ).toMatchObject({ + schema: "hack.daemon-stop/v1", + stale: false, + stale_reasons: [], + daemon: { running: false, pid: null, compatible: false }, + }); +}); + +test("an environment-only plist change is stale", () => { + const opts = { + hackBinPath: "/opt/hack/bin/hack", + home: "/Users/example", + runAtLoad: true, + guiSessionOnly: true, + }; + const current = renderLaunchdPlist(opts); + expect( + isLaunchdPlistCurrent({ + plistText: current, + hackBinPath: opts.hackBinPath, + home: opts.home, + }) + ).toBe(true); + const oldPath = current.replace(":/opt/homebrew/bin", ""); + expect(oldPath).not.toBe(current); + expect( + isLaunchdPlistCurrent({ + plistText: oldPath, + hackBinPath: opts.hackBinPath, + home: opts.home, + }) + ).toBe(false); +}); + +test("after an update, nothing runs without an installed service or a running daemon", async () => { + const ran: string[][] = []; + const runCli = (argv: readonly string[]) => { + ran.push([...argv]); + return Promise.resolve({ exitCode: 0, stdout: "" }); + }; + expect( + await reconcileDaemonAfterUpdate({ + binaryPath: "/opt/hack/bin/hack", + observe: () => + Promise.resolve({ serviceInstalled: false, daemonRunning: true }), + runCli, + }) + ).toEqual({ status: "skipped", reason: "no-service" }); + expect( + await reconcileDaemonAfterUpdate({ + binaryPath: "/opt/hack/bin/hack", + observe: () => + Promise.resolve({ serviceInstalled: true, daemonRunning: false }), + runCli, + }) + ).toEqual({ status: "skipped", reason: "not-running" }); + expect(ran).toEqual([]); +}); + +test("after an update, the new executable's start --json reconciles the service", async () => { + const ran: string[][] = []; + const result = await reconcileDaemonAfterUpdate({ + binaryPath: "/opt/hack/bin/hack", + observe: () => + Promise.resolve({ serviceInstalled: true, daemonRunning: true }), + runCli: (argv) => { + ran.push([...argv]); + return Promise.resolve({ + exitCode: 0, + stdout: JSON.stringify({ + ok: true, + data: { + schema: "hack.daemon-start/v1", + manager: "systemd", + action: "restarted", + stale_reasons: ["version-mismatch"], + }, + }), + }); + }, + }); + expect(ran).toEqual([["/opt/hack/bin/hack", "daemon", "start", "--json"]]); + expect(result).toEqual({ + status: "reconciled", + manager: "systemd", + action: "restarted", + stale_reasons: ["version-mismatch"], + }); +}); + +test("a failed or unreadable post-update reconcile is reported, never fatal", async () => { + expect( + parseStartEnvelope({ + exitCode: 1, + stdout: JSON.stringify({ + ok: false, + error: { code: "E_DAEMON_SERVICE", message: "refused" }, + }), + }) + ).toEqual({ + status: "failed", + message: "refused", + next_step: "hack daemon install", + }); + expect(parseStartEnvelope({ exitCode: 1, stdout: "" })).toMatchObject({ + status: "failed", + next_step: "hack daemon install", + }); + expect( + parseStartEnvelope({ + exitCode: 0, + stdout: JSON.stringify({ ok: true, data: { schema: "other/v1" } }), + }) + ).toMatchObject({ status: "failed" }); + expect( + await reconcileDaemonAfterUpdate({ + binaryPath: "/opt/hack/bin/hack", + observe: () => + Promise.resolve({ serviceInstalled: true, daemonRunning: true }), + runCli: () => Promise.reject(new Error("spawn failed")), + }) + ).toEqual({ + status: "failed", + message: "spawn failed", + next_step: "hack daemon install", + }); +}); diff --git a/tests/daemon-systemd-cli.test.ts b/tests/daemon-systemd-cli.test.ts index f01837beb..5fb133d42 100644 --- a/tests/daemon-systemd-cli.test.ts +++ b/tests/daemon-systemd-cli.test.ts @@ -467,3 +467,101 @@ installFlowTest( }, 90_000 ); + +installFlowTest( + "start --json rewrites a stale unit, restarts the service and prints one envelope", + async () => { + fixture = await createFixture(); + const f = fixture; + expect((await hack(f, ["daemon", "install"])).code).toBe(0); + const before = await readOwnedPid(f); + expect(before).not.toBeNull(); + const current = await readFile(f.unitPath, "utf8"); + // What an older hack rendered: same program, other content (HACK-2216). + await writeFile( + f.unitPath, + current.replace("RestartSec=2", "RestartSec=9") + ); + + const start = await hack(f, ["daemon", "start", "--json"]); + expect(start.code).toBe(0); + const envelope = JSON.parse(start.stdout) as { + ok: boolean; + data: Record; + }; + expect(envelope.ok).toBe(true); + expect(envelope.data).toMatchObject({ + schema: "hack.daemon-start/v1", + manager: "systemd", + action: "reinstalled", + stale: true, + stale_reasons: ["content"], + daemon: { running: true, compatible: true }, + }); + expect(await readFile(f.unitPath, "utf8")).toBe(current); + expect(await calls(f)).toContain("--user restart hackd.service"); + const after = await readOwnedPid(f); + expect(after).not.toBeNull(); + expect(after).not.toBe(before); + + // Nothing stale now: a second start changes nothing. + const again = await hack(f, ["daemon", "start", "--json"]); + expect(again.code).toBe(0); + expect(JSON.parse(again.stdout)).toMatchObject({ + ok: true, + data: { action: "none", stale: false, manager: "systemd" }, + }); + + const stop = await hack(f, ["daemon", "stop", "--json"]); + expect(stop.code).toBe(0); + expect(JSON.parse(stop.stdout)).toMatchObject({ + ok: true, + data: { + schema: "hack.daemon-stop/v1", + manager: "systemd", + action: "stopped", + }, + }); + expect((await hack(f, ["daemon", "uninstall"])).code).toBe(0); + }, + 90_000 +); + +test.skipIf(process.platform !== "linux")( + "--json with an isolated HACK_HOME: stop reports a no-op, --foreground is a usage error", + async () => { + fixture = await createFixture(); + const f = fixture; + const isolated = { + ...f, + env: { ...f.env, HACK_HOME: join(f.root, "isolated") }, + }; + const stop = await hack(isolated, ["daemon", "stop", "--json"]); + expect(stop.code).toBe(0); + expect(JSON.parse(stop.stdout)).toEqual({ + ok: true, + data: { + schema: "hack.daemon-stop/v1", + manager: "none", + action: "none", + stale: false, + stale_reasons: [], + cli_version: expect.any(String), + daemon: { running: false, pid: null, version: null, compatible: false }, + }, + }); + const foreground = await hack(isolated, [ + "daemon", + "start", + "--foreground", + "--json", + ]); + expect(foreground.code).toBe(1); + expect(JSON.parse(foreground.stdout)).toMatchObject({ + ok: false, + error: { code: "E_USAGE" }, + }); + expect(await calls(f)).toEqual([]); + }, + 90_000 +); diff --git a/tests/daemon-systemd.test.ts b/tests/daemon-systemd.test.ts index 3f79163bc..c685146bc 100644 --- a/tests/daemon-systemd.test.ts +++ b/tests/daemon-systemd.test.ts @@ -15,6 +15,7 @@ import { dirname, join } from "node:path"; import { describeSystemdIncompatibleOwner, installDaemonSystemdService, + reconcileRunningSystemdDaemon, startViaSystemdIfInstalled, stopViaSystemdIfInstalled, uninstallDaemonSystemdService, @@ -26,6 +27,7 @@ import type { DaemonProcessProof, } from "../src/daemon/process.ts"; import type { ProcessImage } from "../src/daemon/process-image.ts"; +import { createDaemonCommandTrace } from "../src/daemon/reconcile.ts"; import { summarizeSystemdService } from "../src/daemon/service-summary.ts"; import { buildDaemonRepairMessage, @@ -4381,3 +4383,190 @@ linuxTest( expect((await stop(null)).refused).toContain("not proven"); } ); + +// HACK-2216: start brings a stale service up to this CLI. + +/** A unit for `program` with one extra line: same program, stale content. */ +async function writeStaleUnit(fake: Fake): Promise { + const stale = unitText().replace("RestartSec=2", "RestartSec=9"); + await installSystemdUnit({ unitPath, unitText: stale, deps: fake.deps }); + fake.calls.length = 0; + return stale; +} + +function runningHooks(fake: Fake, version: string | null) { + return { + ...startHooks(fake), + pid: 4242, + version, + cliVersion: "2.0.0", + trace: createDaemonCommandTrace(), + }; +} + +linuxTest( + "start rewrites a stale unit that runs this hack, keeping it disabled", + async () => { + const fake = fakeSystemd(); + await writeStaleUnit(fake); + fake.enabled = false; + const trace = createDaemonCommandTrace(); + expect( + await startViaSystemdIfInstalled({ ...startHooks(fake), trace }) + ).toBe(0); + expect(await readFile(unitPath, "utf8")).toBe(unitText()); + expect(fake.calls).not.toContain("systemctl --user enable hackd.service"); + const reload = fake.calls.indexOf("systemctl --user daemon-reload"); + expect(reload).toBeGreaterThanOrEqual(0); + expect( + fake.calls.indexOf("systemctl --user start hackd.service") + ).toBeGreaterThan(reload); + expect(trace.manager).toBe("systemd"); + expect(trace.action).toBe("reinstalled"); + expect([...trace.stale]).toEqual(["content"]); + } +); + +linuxTest( + "start rewrites a stale unit whose program is a link to this hack, keeping the link", + async () => { + const fake = fakeSystemd(); + const link = join(root, "bin", "hack-link"); + await symlink(program, link); + const stale = (renderSystemdUnit({ hackBinPath: link }) ?? "").replace( + "RestartSec=2", + "RestartSec=9" + ); + await installSystemdUnit({ unitPath, unitText: stale, deps: fake.deps }); + expect(await startViaSystemdIfInstalled(startHooks(fake))).toBe(0); + expect(await readFile(unitPath, "utf8")).toBe( + renderSystemdUnit({ hackBinPath: link }) ?? "" + ); + } +); + +linuxTest("read-only start leaves a stale unit as it is", async () => { + const fake = fakeSystemd(); + fake.deps = { + ...fake.deps, + env: { ...fake.deps.env, [SYSTEMD_READ_ONLY_ENV]: "1" }, + }; + const stale = await writeStaleUnit(fake); + expect(await startViaSystemdIfInstalled(startHooks(fake))).toBe(0); + expect(await readFile(unitPath, "utf8")).toBe(stale); + expect(fake.calls).not.toContain("systemctl --user daemon-reload"); + expect(fake.calls).toContain("systemctl --user start hackd.service"); +}); + +linuxTest( + "a running hackd of another version under this hack's unit is restarted through systemd", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await installSystemdUnit({ + unitPath, + unitText: unitText(), + deps: fake.deps, + }); + fake.calls.length = 0; + const hooks = runningHooks(fake, "1.0.0"); + expect(await reconcileRunningSystemdDaemon(hooks)).toBe(0); + expect(fake.calls).toContain("systemctl --user restart hackd.service"); + expect(fake.calls).not.toContain("systemctl --user daemon-reload"); + expect(hooks.trace.action).toBe("restarted"); + expect([...hooks.trace.stale]).toEqual(["version-mismatch"]); + } +); + +linuxTest( + "a running hackd with a stale unit is rewritten and restarted", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await writeStaleUnit(fake); + const hooks = runningHooks(fake, "2.0.0"); + expect(await reconcileRunningSystemdDaemon(hooks)).toBe(0); + expect(await readFile(unitPath, "utf8")).toBe(unitText()); + const reload = fake.calls.indexOf("systemctl --user daemon-reload"); + expect(reload).toBeGreaterThanOrEqual(0); + expect( + fake.calls.indexOf("systemctl --user restart hackd.service") + ).toBeGreaterThan(reload); + expect(hooks.trace.action).toBe("reinstalled"); + // The restart completed, so no restart stays pending. + expect( + await Bun.file( + join(root, ".hack", "daemon", "hackd.service.restart-pending") + ).exists() + ).toBe(false); + } +); + +linuxTest("a current unit running this version is left alone", async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await installSystemdUnit({ + unitPath, + unitText: unitText(), + deps: fake.deps, + }); + fake.calls.length = 0; + const hooks = runningHooks(fake, "2.0.0"); + expect(await reconcileRunningSystemdDaemon(hooks)).toBeNull(); + expect(fake.calls).not.toContain("systemctl --user restart hackd.service"); + expect(hooks.trace.manager).toBe("systemd"); + expect(hooks.trace.action).toBe("none"); +}); + +linuxTest( + "a version mismatch under a unit for another existing hack is refused, unchanged", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await writeOldUnit(fake); + const before = await readFile(unitPath, "utf8"); + const hooks = runningHooks(fake, "1.0.0"); + expect(await reconcileRunningSystemdDaemon(hooks)).toBe(1); + expect(await readFile(unitPath, "utf8")).toBe(before); + expect(fake.calls).not.toContain("systemctl --user restart hackd.service"); + expect(fake.calls).not.toContain("systemctl --user daemon-reload"); + } +); + +linuxTest( + "a hackd systemd does not run is left to the direct-start path", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await installSystemdUnit({ + unitPath, + unitText: unitText(), + deps: fake.deps, + }); + fake.calls.length = 0; + expect( + await reconcileRunningSystemdDaemon({ + ...runningHooks(fake, "1.0.0"), + pid: 777, + isPidInUnit: () => Promise.resolve(false), + }) + ).toBeNull(); + expect(fake.calls).not.toContain("systemctl --user restart hackd.service"); + } +); + +linuxTest( + "an incompatible hackd under a unit that links to this hack is not refused", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + const link = join(root, "bin", "hack-link"); + await symlink(program, link); + await installSystemdUnit({ + unitPath, + unitText: renderSystemdUnit({ hackBinPath: link }) ?? "", + deps: fake.deps, + }); + expect( + await describeSystemdIncompatibleOwner({ + pid: 4242, + deps: fake.deps, + resolveExpected: startHooks(fake).resolveExpected, + }) + ).toBeNull(); + } +); diff --git a/tests/launchd-isolation.test.ts b/tests/launchd-isolation.test.ts index 8d86f5144..8f4f52b67 100644 --- a/tests/launchd-isolation.test.ts +++ b/tests/launchd-isolation.test.ts @@ -339,7 +339,7 @@ test("repairs legacy output paths even when the installed program is still curre }); expect(run.code).toBe(0); expect(JSON.parse(run.stdout)).toEqual({ - repair: "repaired", + repair: "refreshed", secondRepair: "ok", }); expect(await readFile(fixture.plistPath, "utf8")).toBe(plist); From 91f8e18aff9f4cf81013dd892da40147abf3bdd7 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 19:56:49 +0000 Subject: [PATCH 02/43] fix(daemon): guard automatic service reconcile (review round 1) Review round 1 on HACK-2216: - Never rewrite or restart the service from inside it. Linux compares /proc/self/cgroup with the unit's ControlGroup (through the systemctl runner); macOS checks whether the launchd job pid is an ancestor. A stale but compatible daemon is kept (exit 0, blocked inside-service), a mismatched one refused (exit 1); an unreadable check counts as inside. - Keep the service's Hack home: a managed unit whose main file sets HACK_HOME, HOME or an EnvironmentFile that the renderer would drop is not rewritten, and after a rewrite the unit's home is read again before the restart. - launchd: a plist that runs another existing hack (compared by realpath and dev/ino) is never retargeted; only a missing or virtual program is. A mismatched daemon under it is refused with hack daemon install. - hack update: require hackd to answer on this home's socket before reconciling, run the child in reconcile-only mode (HACK_DAEMON_RECONCILE_ONLY=1, which never starts a stopped daemon), and keep observation inside the best-effort boundary. - Owner policy: rewrite on content drift only when the unit starts with Hack's managed header; launchd plists get an equivalent managed marker. Hand-written units keep the HACK-2052 behaviour (report, no rewrite). --json data gains a closed `blocked` field. Co-Authored-By: Claude Opus 5.5 --- docs/architecture.md | 100 ++++-- docs/cli.md | 2 +- docs/versions/v5/guides/upgrade-from-v4.md | 5 +- .../v5/reference/command-contracts.md | 2 +- docs/versions/v5/upgrade.md | 11 +- src/commands/daemon-launchd.ts | 191 +++++++++++ src/commands/daemon-systemd.ts | 103 ++++-- src/commands/daemon-upgrade.ts | 88 +++-- src/commands/daemon.ts | 113 +++---- src/daemon/launchd.ts | 184 ++++++++--- src/daemon/process-ancestry.ts | 66 ++++ src/daemon/reconcile.ts | 71 +++- src/daemon/systemd.ts | 97 +++++- tests/daemon-reconcile.test.ts | 305 ++++++++++++++++++ tests/daemon-systemd-cli.test.ts | 17 + tests/daemon-systemd.test.ts | 143 ++++++++ tests/launchd-isolation.test.ts | 83 +++++ 17 files changed, 1373 insertions(+), 208 deletions(-) create mode 100644 src/commands/daemon-launchd.ts create mode 100644 src/daemon/process-ancestry.ts diff --git a/docs/architecture.md b/docs/architecture.md index 20a1cba58..933035cd3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -527,8 +527,9 @@ program is never treated as a missing one. `hack` (not this CLI's executable or a link to it): they say so, and `hack daemon install` switches the service. If systemd runs an incompatible hackd from such a unit, `start` reports it instead of replacing it, because -systemd would only start the same version again. A unit that runs this CLI's -executable but is stale is rewritten and restarted; see +systemd would only start the same version again. A stale unit that runs this +CLI's executable and starts with Hack's managed header is rewritten and +restarted; see [After an upgrade](#after-an-upgrade-a-stale-service-is-repaired-by-start). When the unit's program no longer exists, `start` retargets it, but only to a durable location: the path and the file it resolves to must both be outside temporary directories, package @@ -537,9 +538,10 @@ program in `ExecStart` and keeps every other line, including your edits; otherwise it reports the problem and asks you to run `hack daemon install`. If an earlier `daemon-reload` failed (`NeedDaemonReload=yes`), `start` and `install` reload before they start. Other edits to the file show up as drift in -`hack daemon status` and `hack doctor`; `hack daemon start` (for a unit that -runs this CLI's executable) and `hack daemon install` replace them. Put local -changes in a drop-in (`systemctl --user edit hackd`). +`hack daemon status` and `hack doctor`. `hack daemon install` replaces them, and +so does `hack daemon start` when the unit runs this CLI's executable and still +starts with Hack's managed header. Put local changes in a drop-in +(`systemctl --user edit hackd`). A systemd user manager runs only while you have a login session, unless linger is on. `hack daemon install` and `hack daemon status` report linger. When it is off, @@ -578,17 +580,43 @@ to the running CLI. The service is stale when: - `program-missing`: the program it runs no longer exists; - `version-mismatch`: the hackd it runs reports another version than the CLI. -On Linux, `start` rewrites a `content`-stale unit only when it runs this CLI's -executable: the same path, or a path that resolves to the same file. The program -path is kept as written, so a stable symlink stays a symlink, and an enabled -unit stays enabled while a disabled one stays disabled. The rewrite makes the same checks as -`install`: the program and every directory on the unit's `PATH` must be safe -from other accounts, and the unit directories are checked and created the same -way. A program at a location that may not last is reported, as `install` does. -When hackd is running under the unit, `start` then restarts it with -`systemctl --user restart`, after the loaded-file, program and stop-hook checks -that `stop` makes. A hackd of another version under this CLI's unit is restarted -the same way, so the new executable runs. +On Linux, `start` rewrites a `content`-stale unit automatically only when both +of these hold: + +- the unit runs this CLI's executable: the same path, or a path that resolves to + the same file (same resolved path, or same device and inode); +- its main file starts with Hack's managed header + (`# Managed by hack: written by \`hack daemon install\`...`), whose next line + says local changes are replaced on the next install. + +A unit without that header was written or edited by hand. It keeps the earlier +behaviour: `start` reports the drift with `hack daemon install` as the next step +and does not rewrite it. A managed unit whose main file sets the service's Hack +home (`HACK_HOME` or `HOME` in `Environment=`, or any `EnvironmentFile=`) is not +rewritten either, because the unit Hack renders would drop that binding and move +the service to the default home. Move the setting into a drop-in +(`systemctl --user edit hackd`), then run `hack daemon install`. + +The rewrite keeps the program path as written, so a stable symlink stays a +symlink, and an enabled unit stays enabled while a disabled one stays disabled. +It makes the same checks as `install`: the program and every directory on the +unit's `PATH` must be safe from other accounts, and the unit directories are +checked and created the same way. A program at a location that may not last is +reported, as `install` does. When hackd is running under the unit, `start` reads +the unit's Hack home again after the `daemon-reload`, refuses if it no longer +serves this home, and otherwise restarts it with `systemctl --user restart`, +after the loaded-file, program and stop-hook checks that `stop` makes. A hackd of +another version under this CLI's unit is restarted the same way, even when the +unit was not rewritten, so the new executable runs. + +`start` never rewrites or restarts the service from inside it. On Linux it +compares its own cgroup (`/proc/self/cgroup`) with the unit's `ControlGroup`. On +macOS it checks whether the launchd job's process is one of its ancestors. A job +that hackd started (for example a supervisor job running `hack daemon start`) +would otherwise stop itself. From inside, a compatible daemon is kept: `start` +exits 0 with action `none` and `blocked: "inside-service"`. A daemon of another +version is refused with exit 1 and nothing is stopped. If the check cannot be +made, `start` treats the caller as inside. `start` does not repair the service, and goes on as before, when it cannot prove the service is this CLI's: @@ -605,9 +633,15 @@ prove the service is this CLI's: unit is never written or reloaded. A hackd of another version under this CLI's unit is still restarted, which does not change the unit. -On macOS, a loaded launchd plist whose program is current but whose other -content (for example its environment) differs is rewritten too, and launchd -restarts hackd from it. +On macOS the same rules apply to the launchd plist. A plist whose program is +gone (or is a compiled-binary virtual path) is retargeted to this CLI. A plist +that runs this CLI's executable is rewritten when it differs from what this CLI +writes, but only if it carries Hack's managed marker (a +`` comment that `hack daemon install` now writes) or +its output paths are not `/dev/null` (the earlier output repair). launchd then +restarts hackd from it. A plist that runs another existing `hack` is left as it +is, even an older versioned install path. A running hackd of another version +under it is refused with `hack daemon install`. ```mermaid stateDiagram-v2 @@ -615,13 +649,17 @@ stateDiagram-v2 Observe --> Direct: no usable unit, another home, or isolated Observe --> Plan: unit serves this home Plan --> Refuse: program gone and read-only - Plan --> Rewrite: content stale and program is this CLI's, or program gone + Plan --> Rewrite: program gone + Plan --> Rewrite: content stale, this CLI's program, managed header, no home binding Plan --> Refuse: version mismatch and unit runs another or unreadable program + Plan --> Refuse: running mismatch and caller inside the service Plan --> Start: not running Plan --> Restart: running and version mismatch - Plan --> Keep: running, current (or stale but not ours, or read-only) + Plan --> Keep: running, current, or stale but not rewritable, or caller inside the service Rewrite --> Start: not running - Rewrite --> Restart: running + Rewrite --> Recheck: running + Recheck --> Restart: unit still serves this home + Recheck --> Refuse: unit now serves another home Direct --> [*] Start --> [*] Restart --> [*] @@ -636,13 +674,16 @@ stateDiagram-v2 class Refuse fail ``` -After `hack update` installs a release, it runs the new executable's -`hack daemon start --json` when a login service is installed and hackd is -running, so the service reaches the new version. It never starts a daemon -that was not running and never fails the update: a failure is reported in the -update result (`daemon.status: "failed"`) with `hack daemon install` as the -next step. A stopped service picks up the new version at its next -`hack daemon start`. +After `hack update` installs a release, it checks for an installed login service +and for this home's hackd. A live pid is not enough: hackd must answer on this +home's socket. When both are found, it runs the new executable's +`hack daemon start --json` in reconcile-only mode (`HACK_DAEMON_RECONCILE_ONLY=1`). +In that mode `start` does nothing (action `none`) when no hackd of this home is +running, so a daemon that stopped in the meantime is not started. The step never +fails the update, including when it cannot read the service state. A failure is +reported in the update result (`daemon.status: "failed"`) with +`hack daemon install` as the next step. A stopped service picks up the new +version at its next `hack daemon start`. `hack daemon start|stop|restart|install --json` print one envelope and nothing else on stdout (human lines go to stderr). Success is @@ -654,6 +695,7 @@ else on stdout (human lines go to stderr). Success is | `manager` | `systemd`, `launchd` or `none` (hackd runs directly) | | `action` | `none`, `started`, `restarted`, `reinstalled`, `stopped` or `installed` | | `stale`, `stale_reasons` | whether the service or daemon was stale, and why: `content`, `program-missing`, `version-mismatch` | +| `blocked` | why a stale service was left as it is, or `null`: `read-only`, `other-program`, `unknown-program`, `unmanaged`, `home-binding`, `inside-service` | | `cli_version` | this CLI's version | | `daemon` | `running`, `pid`, `version` and `compatible` after the command | diff --git a/docs/cli.md b/docs/cli.md index 5cbccc621..bdb2e118e 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -24,7 +24,7 @@ terminal). - `hack session` — persistent project workspaces (tmux-first) - `hack doctor` / `hack doctor --fix` — validate and repair local setup - `hack upgrade check` — read-only upgrade check of this machine and every registered project: a one-screen summary, the projects with findings (worktrees folded; `--verbose` for every checkout), and a plan with one step per kind of change, then the next command; `hack upgrade apply [--only ] [--project ] [--dry-run]` runs the plan's apply steps (`.hack/.gitignore` refresh, stale agent files) for every checkout after keeping a snapshot and a journal; `hack upgrade snapshot` and `hack upgrade rollback --rollback-manifest ` keep and restore the files the upgrade changes -- `hack daemon` — optional local daemon for faster JSON project status; `hack daemon install` starts it at login (launchd on macOS, a systemd user unit on Linux). On Linux without linger it still stops at your last logout and does not start at boot; `loginctl enable-linger` changes that. After an upgrade, `hack daemon start` (and `hack update`) rewrites a stale login service that runs the same executable and restarts hackd on the new version; `start`, `stop`, `restart` and `install` accept `--json` +- `hack daemon` — optional local daemon for faster JSON project status; `hack daemon install` starts it at login (launchd on macOS, a systemd user unit on Linux). On Linux without linger it still stops at your last logout and does not start at boot; `loginctl enable-linger` changes that. After an upgrade, `hack daemon start` (and `hack update`) restarts hackd on the new version when its login service runs the same executable, and rewrites a stale unit or plist that Hack wrote (managed header); `start`, `stop`, `restart` and `install` accept `--json` Candidate `hack-next doctor` and `hack-v5 doctor` report a missing hackd as optional for Compose workflows. They do not recommend starting the stable daemon or diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index 9b790ecb0..fe8d1d0a3 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -692,8 +692,9 @@ Source: [current env migration and cleanup guidance](https://github.com/hack-dan Installing another CLI does not replace an already running daemon. For an unmanaged daemon, `daemon start` replaces a reachable incompatible daemon; `daemon restart` is the explicit replacement path. With the newer Linux systemd -service, `daemon start` rewrites and restarts a stale unit that runs the same -executable (or a link to it). A unit pointing at a different existing +service, `daemon start` restarts a stale service that runs the same executable +(or a link to it), and rewrites its unit when Hack wrote it (managed header). A +hand-written unit is reported, not rewritten. A unit pointing at a different existing executable needs explicit `daemon install`: restarting that unit would run its old executable again. Keep this separate from a side-by-side candidate diff --git a/docs/versions/v5/reference/command-contracts.md b/docs/versions/v5/reference/command-contracts.md index 4ceba86bb..0d7810877 100644 --- a/docs/versions/v5/reference/command-contracts.md +++ b/docs/versions/v5/reference/command-contracts.md @@ -332,7 +332,7 @@ Thrown usage errors with an explicit `--json` token become E_USAGE; other thrown | usage | bare projects[], total nullable, host[], host_total nullable, runtime_ok:boolean, runtime_error nullable; details adds containers[] | failures exit 1; optional error string. No samples exits 0 with warning:no_usage_samples. Watch uses the interactive redraw path even with JSON selected; do not treat watch as a JSON report stream. | | global status | bare generated_at/caddy/logging/networks/gateway/summary | exits 0 even if summary health false; gateway is retained unsupported data, excluded from public contract | | daemon status | bare report below | running returns 0; starting/stale/unavailable may return 1 | -| daemon start/stop/restart/install | shared envelope; data schema `hack.daemon-/v1`, manager (systemd/launchd/none), action (none/started/restarted/reinstalled/stopped/installed), stale:boolean, stale_reasons (content/program-missing/version-mismatch), cli_version, daemon.{running,pid,version,compatible} | failure is `E_DAEMON_SERVICE` with the same data as detail; access refusals `E_PERMISSION`; `start --foreground --json` is `E_USAGE`; exit codes unchanged | +| daemon start/stop/restart/install | shared envelope; data schema `hack.daemon-/v1`, manager (systemd/launchd/none), action (none/started/restarted/reinstalled/stopped/installed), stale:boolean, stale_reasons (content/program-missing/version-mismatch), blocked (null/read-only/other-program/unknown-program/unmanaged/home-binding/inside-service), cli_version, daemon.{running,pid,version,compatible} | failure is `E_DAEMON_SERVICE` with the same data as detail; access refusals `E_PERMISSION`; `start --foreground --json` is `E_USAGE`; exit codes unchanged | | host ps | bare {generatedAt,snapshotAvailable,commands,lifecycle} | timestamp string, availability boolean; nested telemetry remains specialized | | session panes/capture/tail | JSON-lines events below | no shared envelope | | update | command-owned object with ok:boolean, string error on failure | error is not shared error.{code,message}; success varies by check/install outcome; exit follows ok | diff --git a/docs/versions/v5/upgrade.md b/docs/versions/v5/upgrade.md index 85f5a22ea..907d952f4 100644 --- a/docs/versions/v5/upgrade.md +++ b/docs/versions/v5/upgrade.md @@ -114,11 +114,12 @@ backup scripts, source evidence and checks still awaiting real-machine trials. Installing a CLI does not replace a running daemon. After choosing a durable v5 install, run its `hack daemon start` (or `hack daemon restart`). When the login -service runs that same executable, start rewrites a stale unit or plist and -restarts hackd on the new version; `hack update` does this by itself when a -service is installed and hackd is running. If the login service points at -another existing executable, start reports it instead: use the chosen install's -`hack daemon install`. Add `--json` for one machine-readable result. Keep this +service runs that same executable, start restarts hackd on the new version and +rewrites a stale unit or plist that Hack wrote (it carries Hack's managed +header). `hack update` does this by itself when a service is installed and +hackd is running. A hand-written unit, a unit that sets its own Hack home, or a +service that points at another existing executable is reported instead: use the +chosen install's `hack daemon install`. Add `--json` for one machine-readable result. Keep this outside the shared-home candidate trial; [daemon and service details](guides/upgrade-from-v4.md#daemon-old-lock-and-missing-checkouts) explain the difference. diff --git a/src/commands/daemon-launchd.ts b/src/commands/daemon-launchd.ts new file mode 100644 index 000000000..ba2997a0e --- /dev/null +++ b/src/commands/daemon-launchd.ts @@ -0,0 +1,191 @@ +import { + getLaunchdServiceStatus, + kickstartLaunchdService, + type LaunchdServiceStatus, + readLaunchdProgramState, + repairLaunchdProgramIfInvalid, +} from "../daemon/launchd.ts"; +import type { DaemonPaths } from "../daemon/paths.ts"; +import { hasAncestorProcess } from "../daemon/process-ancestry.ts"; +import type { DaemonCommandTrace } from "../daemon/reconcile.ts"; +import { logger } from "../ui/logger.ts"; + +/** + * macOS `hack daemon start` through a loaded launchd agent (HACK-2216). + * `waitForReady` and the ancestry reader are injected so the decisions can + * be driven without launchd. + */ + +export interface LaunchdStartHooks { + readonly paths: DaemonPaths; + readonly trace: DaemonCommandTrace; + readonly waitForReady: (timeoutMs: number) => Promise; + /** Defaults to the real launchd status. */ + readonly readStatus?: () => Promise; + /** Defaults to {@link hasAncestorProcess} for this process. */ + readonly isAncestor?: (pid: number) => Promise; +} + +async function readStatus( + hooks: LaunchdStartHooks +): Promise { + if (hooks.readStatus) { + return await hooks.readStatus(); + } + return process.platform === "darwin" + ? await getLaunchdServiceStatus({ paths: hooks.paths }) + : null; +} + +/** + * Whether this process runs under the launchd job: its pid is an ancestor + * of this process (a job hackd started runs `hack daemon start`). Restarting + * the job would stop the caller. An unreadable ancestry counts as inside. + */ +async function runsInsideJob(opts: { + readonly hooks: LaunchdStartHooks; + readonly status: LaunchdServiceStatus; +}): Promise { + const jobPid = opts.status.pid; + if (jobPid === null || jobPid <= 1) { + return false; + } + const isAncestor = + opts.hooks.isAncestor ?? + ((pid: number) => hasAncestorProcess({ ancestor: pid })); + return (await isAncestor(jobPid)) !== false; +} + +/** + * `start` found this home's hackd running and launchd has the agent + * loaded. Returns an exit code when start is done, or null to go on as + * before (already running, or replace an incompatible daemon): + * - from inside the job nothing is rewritten or restarted: a compatible + * daemon is kept, an incompatible one refused; + * - an incompatible daemon under a plist that runs another existing `hack` + * is refused with `hack daemon install` (kickstart would run it again); + * - a compatible daemon under a stale plist is refreshed and restarted. + */ +export async function checkRunningLaunchdDaemon( + hooks: LaunchdStartHooks & { + readonly pid: number; + readonly compatible: boolean; + } +): Promise { + const status = await readStatus(hooks); + if (!status?.loaded) { + return null; + } + const { trace } = hooks; + trace.manager = "launchd"; + if (await runsInsideJob({ hooks, status })) { + trace.blocked = "inside-service"; + if (hooks.compatible) { + return null; + } + trace.stale.add("version-mismatch"); + logger.warn({ + message: `hackd (pid ${hooks.pid}) is incompatible with this CLI, but this command runs inside the launchd job (a job hackd started), so restarting it would stop this command too; nothing was changed | Run hack daemon start from a shell outside the service`, + }); + return 1; + } + if (hooks.compatible) { + return await startViaLaunchdIfLoaded({ ...hooks, onlyIfStale: true }); + } + const program = await readLaunchdProgramState({ paths: hooks.paths }); + if (program.kind === "other") { + trace.stale.add("version-mismatch"); + trace.blocked = "other-program"; + logger.warn({ + message: `hackd (pid ${hooks.pid}) is incompatible with this CLI, and the launchd service runs ${program.program ?? "another hack"}, so restarting it would start the same version again | Run: hack daemon install`, + }); + return 1; + } + return null; +} + +/** + * When launchd manages the daemon, start through launchd. Returns null when + * no launchd service is loaded, or with `onlyIfStale` when nothing was + * rewritten. See {@link repairLaunchdProgramIfInvalid} for what is + * rewritten; `kickstart -k` then (re)starts hackd. + */ +export async function startViaLaunchdIfLoaded( + hooks: LaunchdStartHooks & { readonly onlyIfStale?: boolean } +): Promise { + const { paths, trace } = hooks; + // When launchd manages the daemon, start through launchd — spawning a + // bare process next to a loaded agent means two process managers fight + // over one socket and pid file. + const status = await readStatus(hooks); + if (!status?.loaded) { + return null; + } + trace.manager = "launchd"; + const repair = await repairLaunchdProgramIfInvalid({ paths }); + const rewritten = noteRepair({ repair, trace }); + if (!rewritten && hooks.onlyIfStale) { + return null; + } + trace.action = rewritten ? "reinstalled" : "started"; + // launchd owns the daemon: never spawn a bare process next to it — + // two process managers for one socket is how machines end up with + // start-then-exit flapping and stale-pid contradictions. + const kick = await kickstartLaunchdService(); + if (!kick.ok) { + logger.error({ + message: `launchd kickstart failed: ${kick.error ?? "unknown error"} | Check: hack daemon logs --tail 200`, + }); + return 1; + } + if (await hooks.waitForReady(8000)) { + logger.success({ message: "hackd started (launchd)" }); + return 0; + } + logger.warn({ + message: + "launchd kickstarted hackd but it did not report ready yet | Check: hack daemon logs --tail 200", + }); + return 1; +} + +/** Reports a repair result; true when the plist was rewritten. */ +function noteRepair(opts: { + readonly repair: Awaited>; + readonly trace: DaemonCommandTrace; +}): boolean { + const { repair, trace } = opts; + switch (repair) { + case "repaired": + trace.stale.add("program-missing"); + logger.warn({ + message: + "Repaired launchd service: its program no longer existed (or was a compiled-binary virtual path)", + }); + return true; + case "refreshed": + trace.stale.add("content"); + logger.warn({ + message: + "Updated launchd service: its plist differed from what this hack writes", + }); + return true; + case "other-program": + trace.blocked = "other-program"; + logger.info({ + message: + "The launchd service runs another hack; it is left as it is | To switch it to this hack, run: hack daemon install", + }); + return false; + case "unmanaged": + trace.stale.add("content"); + trace.blocked = "unmanaged"; + logger.info({ + message: + "The launchd plist differs from what this hack writes, but it was not written by this hack (no managed marker), so it is left as it is | To replace it, run: hack daemon install", + }); + return false; + default: + return false; + } +} diff --git a/src/commands/daemon-systemd.ts b/src/commands/daemon-systemd.ts index 6de9f61f8..8d7c22802 100644 --- a/src/commands/daemon-systemd.ts +++ b/src/commands/daemon-systemd.ts @@ -1,8 +1,9 @@ -import { mkdir, realpath, rm, writeFile } from "node:fs/promises"; +import { mkdir, rm, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, resolve } from "node:path"; import { GLOBAL_DAEMON_DIR_NAME } from "../constants.ts"; import { DAEMON_CLI_VERSION } from "../daemon/client.ts"; +import { isSameExecutableFile } from "../daemon/launchd.ts"; import type { DaemonPaths } from "../daemon/paths.ts"; import { type DaemonProcessProbe, @@ -35,13 +36,16 @@ import { defaultSystemdDeps, describeSystemdUnavailable, extractSystemdExecProgram, + findDroppedHomeBinding, installSystemdUnit, + isHackManagedUnit, isPidInSystemdUnit, isSystemdReadOnly, LINGER_ENABLE_COMMAND, MANAGER_STOPPABLE_STATES, probeSystemdUserManager, RENDERED_SYSTEM_PATH_DIRECTORIES, + readInvokerInSystemdUnit, readLingerStatus, readSystemdServiceStatus, readSystemdUnitHackHome, @@ -103,6 +107,8 @@ export interface SystemdCommandHooks { * directories so they do not depend on the host's. */ readonly systemPathDirectories?: readonly string[]; + /** Defaults to reading `/proc/self/cgroup`; tests pin it. */ + readonly readSelfCgroup?: () => Promise; /** Records what the command decided, for `--json` (HACK-2216). */ readonly trace?: DaemonCommandTrace; /** Defaults to this CLI's version; tests pin it. */ @@ -295,21 +301,28 @@ export async function reconcileRunningSystemdDaemon( hooks.resolveExpected ?? resolveExpectedSystemdUnit )(); const marker = resolve(hooks.paths.root, RESTART_PENDING_FILE); + // A job hackd started (a supervisor job running `hack daemon start`) + // runs in the unit's cgroup: restarting the unit would stop the caller. + // Unknown counts as inside. + const invoker = await readInvokerInSystemdUnit({ + deps, + readSelfCgroup: hooks.readSelfCgroup, + }); const plan = planDaemonReconcile({ unit: serviceUnitState(status.drift), programIsOurs: await unitRunsExpected({ status, expected }), readOnly: isSystemdReadOnly(deps.env), running: { version: hooks.version }, restartPending: await pathExists(marker), + contentBlock: await readContentBlock({ status, unitPath }), + invokerInsideService: invoker !== "outside", cliVersion: hooks.cliVersion ?? DAEMON_CLI_VERSION, }); + recordReconcile({ trace: hooks.trace, manager: "systemd", plan }); if (plan.service === "keep") { - if (hooks.trace) { - hooks.trace.manager = "systemd"; - } + noteKeptStaleUnit({ plan }); return null; } - recordReconcile({ trace: hooks.trace, manager: "systemd", plan }); if (plan.service === "refuse") { logger.warn({ message: describeRunningRefusal({ @@ -339,6 +352,9 @@ function describeRunningRefusal(opts: { readonly pid: number; }): string { const { plan, status, expected } = opts; + if (plan.blocked === "inside-service") { + return `hackd (pid ${opts.pid}) is incompatible with this CLI, but this command runs inside ${SYSTEMD_UNIT_NAME} (a job hackd started), so restarting it would stop this command too; nothing was changed | Run hack daemon start from a shell outside the service`; + } if (plan.blocked === "read-only") { return `${describeMissingProgram(status)}; not changing the service here | Run: hack daemon install`; } @@ -381,6 +397,19 @@ async function restartReconciledUnit(opts: { if (rewritten !== null) { return rewritten; } + // The rewrite reloaded systemd: check again which home the unit now + // serves before restarting it, as start does after its own repair. + const unitHome = await checkUnitServesHome({ + hackHome: dirname(hooks.paths.root), + deps, + action: "starting", + }); + if (unitHome.kind !== "ours") { + logger.error({ + message: `${unitHome.message}; not restarting it after the rewrite | Run: hack daemon install`, + }); + return 1; + } } const blocked = (await checkSystemdUnitTrusted({ unitPath })) ?? @@ -489,8 +518,9 @@ async function checkUnitServesHome(opts: { * * The decision is {@link planDaemonReconcile} (HACK-2216): * - a unit that runs this CLI's executable (the same path, or one that - * resolves to the same file) but differs from what this CLI renders for - * that program is rewritten whole, as `hack daemon install` would, keeping + * resolves to the same file), starts with {@link SYSTEMD_MANAGED_HEADER} + * and sets no Hack home a rewrite would drop, but differs from what this + * CLI renders for that program is rewritten whole, as install would, keeping * its program path (a stable symlink stays a symlink) and whether it is * enabled; * - a unit whose program is gone is retargeted to this CLI, only to a @@ -533,6 +563,7 @@ async function prepareSystemdUnitForStart(opts: { programIsOurs: await unitRunsExpected({ status, expected }), readOnly, running: null, + contentBlock: await readContentBlock({ status, unitPath }), cliVersion: hooks.cliVersion ?? DAEMON_CLI_VERSION, }); recordReconcile({ trace: hooks.trace, manager: "systemd", plan }); @@ -578,21 +609,50 @@ async function unitRunsExpected(opts: { if (status.program === null || expected === null) { return false; } - if (status.program === expected.hackBinPath) { - return true; + return await isSameExecutableFile(status.program, expected.hackBinPath); +} + +/** + * Why a content-stale unit must not be rewritten automatically (HACK-2216): + * `unmanaged` when its main file does not start with + * {@link SYSTEMD_MANAGED_HEADER} (owner decision: hand-written units keep the + * HACK-2052 behaviour), `home-binding` when it binds the service to a Hack + * home that the rendered unit would drop ({@link findDroppedHomeBinding}). + */ +async function readContentBlock(opts: { + readonly status: SystemdServiceStatus; + readonly unitPath: string; +}): Promise<"unmanaged" | "home-binding" | null> { + const { status } = opts; + if (status.drift !== "content" || status.program === null) { + return null; + } + const unitText = await readTextFile(opts.unitPath); + if (unitText === null || !isHackManagedUnit({ unitText })) { + return "unmanaged"; + } + const rendered = renderSystemdUnit({ hackBinPath: status.program }); + return rendered === null || findDroppedHomeBinding({ unitText, rendered }) + ? "home-binding" + : null; +} + +/** Says why a stale unit was left as it is, when it was. */ +function noteKeptStaleUnit(opts: { readonly plan: DaemonReconcilePlan }): void { + const { blocked } = opts.plan; + if (blocked === "unmanaged") { + logger.info({ + message: `${SYSTEMD_UNIT_NAME} differs from what this hack writes, but it was not written by hack (no managed header), so it is left as it is | To replace it, run: hack daemon install`, + }); + } else if (blocked === "home-binding") { + logger.warn({ + message: `${SYSTEMD_UNIT_NAME} differs from what this hack writes, but it sets the service's Hack home (HACK_HOME, HOME or an EnvironmentFile) in its main file, which a rewrite would drop; it is left as it is | Move that setting into a drop-in (systemctl --user edit hackd), then run: hack daemon install`, + }); + } else if (blocked === "inside-service") { + logger.warn({ + message: `${SYSTEMD_UNIT_NAME} is stale, but this command runs inside it (a job hackd started), so it was not rewritten or restarted | Run hack daemon start from a shell outside the service`, + }); } - const real = async (path: string): Promise => { - try { - return await realpath(path); - } catch { - return null; - } - }; - const [unitReal, ourReal] = await Promise.all([ - real(status.program), - real(expected.hackBinPath), - ]); - return unitReal !== null && unitReal === ourReal; } function describeMissingProgram(status: SystemdServiceStatus): string { @@ -760,6 +820,7 @@ async function completePendingReload(opts: { message: `${SYSTEMD_UNIT_NAME} differs from what this hack writes; not changing it here | Run: hack daemon install`, }); } + noteKeptStaleUnit({ plan }); if (!status.needsReload) { return null; } diff --git a/src/commands/daemon-upgrade.ts b/src/commands/daemon-upgrade.ts index 151402b89..a6f8a4bde 100644 --- a/src/commands/daemon-upgrade.ts +++ b/src/commands/daemon-upgrade.ts @@ -1,9 +1,11 @@ +import { probeDaemonApi } from "../daemon/client.ts"; import { getLaunchdServiceStatus } from "../daemon/launchd.ts"; -import { resolveDaemonPaths } from "../daemon/paths.ts"; -import type { - DaemonCommandAction, - DaemonServiceManager, - DaemonStaleReason, +import { type DaemonPaths, resolveDaemonPaths } from "../daemon/paths.ts"; +import { + DAEMON_RECONCILE_ONLY_ENV, + type DaemonCommandAction, + type DaemonServiceManager, + type DaemonStaleReason, } from "../daemon/reconcile.ts"; import { readDaemonStatus } from "../daemon/status.ts"; import { childEnvironment } from "../lib/child-environment.ts"; @@ -65,25 +67,28 @@ export async function reconcileDaemonAfterUpdate(opts: { argv: readonly string[] ) => Promise<{ readonly exitCode: number; readonly stdout: string }>; }): Promise { - const state = await (opts.observe ?? observePostUpdateDaemon)(); - if (!state.serviceInstalled) { - return { status: "skipped", reason: "no-service" }; - } - if (!state.daemonRunning) { - return { status: "skipped", reason: "not-running" }; - } - let run: { readonly exitCode: number; readonly stdout: string }; + // Everything here is best effort: the binary is already replaced, so no + // failure (observing included) may fail the update. try { - run = await (opts.runCli ?? runBounded)([ + const state = await (opts.observe ?? (() => observePostUpdateDaemon()))(); + if (!state.serviceInstalled) { + return { status: "skipped", reason: "no-service" }; + } + if (!state.daemonRunning) { + return { status: "skipped", reason: "not-running" }; + } + // Reconcile-only: if hackd stopped since it was observed, the child + // does nothing instead of starting it. + const run = await (opts.runCli ?? runBounded)([ opts.binaryPath, "daemon", "start", "--json", ]); + return parseStartEnvelope({ exitCode: run.exitCode, stdout: run.stdout }); } catch (error: unknown) { return failed(error instanceof Error ? error.message : String(error)); } - return parseStartEnvelope({ exitCode: run.exitCode, stdout: run.stdout }); } /** @@ -148,17 +153,46 @@ function failed(message: string): PostUpdateDaemonResult { return { status: "failed", message, next_step: "hack daemon install" }; } -async function observePostUpdateDaemon(): Promise { - const paths = resolveDaemonPaths({}); - let serviceInstalled = false; - if (process.platform === "linux") { - serviceInstalled = - (await resolveSystemdStatus())?.status.installed === true; - } else if (process.platform === "darwin") { - serviceInstalled = (await getLaunchdServiceStatus({ paths })).installed; +/** + * Whether a login service is installed and this home's hackd is running. + * A live pid alone is not enough (a stale pid file can name a reused pid): + * hackd must answer on this home's socket. + */ +export async function observePostUpdateDaemon( + opts: { + readonly paths?: DaemonPaths; + /** Defaults to reading the systemd unit or launchd plist. */ + readonly serviceInstalled?: () => Promise; + } = {} +): Promise { + const paths = opts.paths ?? resolveDaemonPaths({}); + const serviceInstalled = await ( + opts.serviceInstalled ?? (() => readServiceInstalled({ paths })) + )(); + if (!serviceInstalled) { + return { serviceInstalled, daemonRunning: false }; } const daemon = await readDaemonStatus({ paths }); - return { serviceInstalled, daemonRunning: daemon.running }; + if (!(daemon.running && daemon.socketExists)) { + return { serviceInstalled, daemonRunning: false }; + } + const probe = await probeDaemonApi({ + socketPath: paths.socketPath, + timeoutMs: 1000, + }); + return { serviceInstalled, daemonRunning: probe.reachable }; +} + +async function readServiceInstalled(opts: { + readonly paths: DaemonPaths; +}): Promise { + if (process.platform === "linux") { + return (await resolveSystemdStatus())?.status.installed === true; + } + if (process.platform === "darwin") { + return (await getLaunchdServiceStatus({ paths: opts.paths })).installed; + } + return false; } async function runBounded( @@ -168,7 +202,11 @@ async function runBounded( stdin: "ignore", stdout: "pipe", stderr: "ignore", - env: childEnvironment({ ...process.env, HACK_NO_INTERACTIVE: "1" }), + env: childEnvironment({ + ...process.env, + HACK_NO_INTERACTIVE: "1", + [DAEMON_RECONCILE_ONLY_ENV]: "1", + }), }); const timer = setTimeout(() => proc.kill("SIGKILL"), RECONCILE_TIMEOUT_MS); try { diff --git a/src/commands/daemon.ts b/src/commands/daemon.ts index e2be43b16..8acbb89c4 100644 --- a/src/commands/daemon.ts +++ b/src/commands/daemon.ts @@ -15,7 +15,6 @@ import { installLaunchdService, kickstartLaunchdService, type LaunchdServiceStatus, - repairLaunchdProgramIfInvalid, resolveLaunchdAccess, uninstallLaunchdService, } from "../daemon/launchd.ts"; @@ -36,6 +35,7 @@ import { } from "../daemon/process.ts"; import { createDaemonCommandTrace, + DAEMON_RECONCILE_ONLY_ENV, type DaemonCommandTrace, } from "../daemon/reconcile.ts"; import { runDaemon } from "../daemon/server.ts"; @@ -60,6 +60,10 @@ import { detectDockerBackend, } from "../lib/runtime-guidance.ts"; import { logger } from "../ui/logger.ts"; +import { + checkRunningLaunchdDaemon, + startViaLaunchdIfLoaded, +} from "./daemon-launchd.ts"; import { runDaemonCommand } from "./daemon-result.ts"; import { describeSystemdIncompatibleOwner, @@ -277,6 +281,8 @@ async function handleDaemonStart({ paths, foreground: args.options.foreground === true, trace, + reconcileOnly: + (process.env[DAEMON_RECONCILE_ONLY_ENV] ?? "").trim() === "1", }), }); } @@ -291,6 +297,12 @@ async function startDaemon(opts: { readonly paths: DaemonPaths; readonly foreground: boolean; readonly trace: DaemonCommandTrace; + /** + * {@link DAEMON_RECONCILE_ONLY_ENV} (the post-update step): only bring a + * running hackd and its service up to date; never start one that is not + * running, even if it stopped after the caller looked. + */ + readonly reconcileOnly?: boolean; }): Promise { const { paths, trace } = opts; const status = await readDaemonStatus({ paths }); @@ -300,6 +312,12 @@ async function startDaemon(opts: { const livePid = status.running ? status.pid : null; const pidIsDaemon = livePid !== null && (await isHackDaemonProcess({ pid: livePid })); + if (opts.reconcileOnly && !(pidIsDaemon && status.socketExists)) { + logger.info({ + message: "hackd is not running; nothing to bring up to date", + }); + return 0; + } let replaced = false; if (livePid !== null && pidIsDaemon) { const running = await handleRunningDaemonAtStart({ @@ -376,7 +394,11 @@ async function startStoppedDaemon(opts: { return 0; } - const viaLaunchd = await startViaLaunchdIfLoaded({ paths, trace }); + const viaLaunchd = await startViaLaunchdIfLoaded({ + paths, + trace, + waitForReady: (timeoutMs) => waitForDaemonStart({ paths, timeoutMs }), + }); if (viaLaunchd !== null) { return viaLaunchd; } @@ -387,70 +409,6 @@ async function startStoppedDaemon(opts: { return started; } -/** - * When launchd manages the daemon, start through launchd. Returns null when - * no launchd service is loaded, or with `onlyIfStale` when its plist is - * current. A plist whose program is gone ("repaired") or whose content - * differs from what this CLI renders ("refreshed", HACK-2216) is rewritten - * first; `kickstart -k` then (re)starts hackd from it. - */ -async function startViaLaunchdIfLoaded(opts: { - readonly paths: DaemonPaths; - readonly trace: DaemonCommandTrace; - readonly onlyIfStale?: boolean; -}): Promise { - const { paths, trace } = opts; - // When launchd manages the daemon, start through launchd — spawning a - // bare process next to a loaded agent means two process managers fight - // over one socket and pid file. - const launchdStatus = await resolveLaunchdStatus({ paths }); - if (!launchdStatus?.loaded) { - return null; - } - trace.manager = "launchd"; - const repair = await repairLaunchdProgramIfInvalid({ paths }); - if (repair === "repaired") { - trace.stale.add("program-missing"); - logger.warn({ - message: - "Repaired launchd service: its program path was invalid (stale or compiled-binary virtual path)", - }); - } else if (repair === "refreshed") { - trace.stale.add("content"); - logger.warn({ - message: - "Updated launchd service: its plist differed from what this hack writes", - }); - } else if (opts.onlyIfStale) { - return null; - } - trace.action = - repair === "repaired" || repair === "refreshed" ? "reinstalled" : "started"; - // launchd owns the daemon: never spawn a bare process next to it — - // two process managers for one socket is how machines end up with - // start-then-exit flapping and stale-pid contradictions. - const kick = await kickstartLaunchdService(); - if (!kick.ok) { - logger.error({ - message: `launchd kickstart failed: ${kick.error ?? "unknown error"} | Check: hack daemon logs --tail 200`, - }); - return 1; - } - const startedViaLaunchd = await waitForDaemonStart({ - paths, - timeoutMs: 8000, - }); - if (startedViaLaunchd) { - logger.success({ message: "hackd started (launchd)" }); - return 0; - } - logger.warn({ - message: - "launchd kickstarted hackd but it did not report ready yet | Check: hack daemon logs --tail 200", - }); - return 1; -} - /** * `hack daemon stop` could not prove the pid-file pid is this HACK_HOME's * hackd. A pid that is not hackd at all (reused after an unclean shutdown) @@ -587,17 +545,20 @@ async function handleRunningDaemonAtStart(opts: { return reconciled; } } - if (!(api.reachable && !api.compatible)) { - if (process.platform === "darwin" && !opts.foreground) { - const refreshed = await startViaLaunchdIfLoaded({ - paths, - trace, - onlyIfStale: true, - }); - if (refreshed !== null) { - return refreshed; - } + const compatible = !(api.reachable && !api.compatible); + if (process.platform === "darwin" && !opts.foreground) { + const viaLaunchd = await checkRunningLaunchdDaemon({ + paths, + trace, + pid: opts.pid, + compatible, + waitForReady: (timeoutMs) => waitForDaemonStart({ paths, timeoutMs }), + }); + if (viaLaunchd !== null) { + return viaLaunchd; } + } + if (compatible) { logger.success({ message: `hackd already running (pid ${opts.pid})` }); return 0; } diff --git a/src/daemon/launchd.ts b/src/daemon/launchd.ts index b1f71709e..6aebd8f69 100644 --- a/src/daemon/launchd.ts +++ b/src/daemon/launchd.ts @@ -1,3 +1,4 @@ +import { realpath, stat } from "node:fs/promises"; import { homedir } from "node:os"; import { basename, dirname, resolve } from "node:path"; import { DAEMON_LAUNCHD_LABEL, GLOBAL_HACK_DIR_NAME } from "../constants.ts"; @@ -153,6 +154,21 @@ export interface LaunchdPlistOptions { readonly guiSessionOnly: boolean; } +/** + * Marks a plist Hack wrote and may rewrite whole (HACK-2216). A plist + * without it, such as one written by an older Hack or edited by hand, is + * only repaired when its program is gone. + */ +export const LAUNCHD_MANAGED_MARKER = + ""; + +/** Whether the plist carries {@link LAUNCHD_MANAGED_MARKER}. */ +export function isHackManagedPlist(opts: { + readonly plistText: string; +}): boolean { + return opts.plistText.split("\n").includes(LAUNCHD_MANAGED_MARKER); +} + /** * Generates the launchd plist XML for hackd. */ @@ -160,6 +176,7 @@ export function renderLaunchdPlist(opts: LaunchdPlistOptions): string { const lines = [ '', '', + LAUNCHD_MANAGED_MARKER, '', "", " Label", @@ -230,9 +247,15 @@ export interface LaunchdInstallResult { export async function installLaunchdService({ paths, config, + hackBinPath: keepProgram, }: { readonly paths: DaemonPaths; readonly config: DaemonLaunchdConfig; + /** + * The program to write, when a refresh keeps the plist's own path (a + * link to this CLI stays a link). Defaults to this CLI's resolution. + */ + readonly hackBinPath?: string; }): Promise { const access = resolveLaunchdAccess(); if (!access.allowed) { @@ -242,8 +265,11 @@ export async function installLaunchdService({ return { ok: false, error: "launchd is only available on macOS" }; } - const invocation = await resolveHackInvocation(); - const hackBinPath = await resolveLaunchdHackBinPath({ invocation }); + const hackBinPath = + keepProgram ?? + (await resolveLaunchdHackBinPath({ + invocation: await resolveHackInvocation(), + })); if (!hackBinPath) { return { ok: false, @@ -347,6 +373,32 @@ function normalizeHackExecutablePath(raw: string | null): string | null { return trimmed; } +/** + * Whether two paths name the same executable file: the same path, the same + * resolved path, or the same device and inode (a hard link or a bind + * mount). False when either cannot be read. A service whose program is + * the same file as this CLI's runs this version; any other existing + * program may be one the user chose (HACK-2216). + */ +export async function isSameExecutableFile( + a: string, + b: string +): Promise { + if (a === b) { + return true; + } + try { + const [realA, realB] = await Promise.all([realpath(a), realpath(b)]); + if (realA === realB) { + return true; + } + const [statA, statB] = await Promise.all([stat(realA), stat(realB)]); + return statA.dev === statB.dev && statA.ino === statB.ino; + } catch { + return false; + } +} + /** * Paths inside a compiled Bun binary's virtual filesystem. They pass * pathExists from INSIDE the same process but no other process (launchd @@ -387,12 +439,17 @@ export function hasBoundedLaunchdOutput(opts: { * `repaired`: the program path was invalid or stale and was replaced; * `refreshed`: the program was current but the rest of the plist (its * environment, output paths or other keys) differed from what this CLI - * renders, and the plist was rewritten (HACK-2216). + * renders, and the plist was rewritten (HACK-2216); + * `other-program`: it runs another existing program, left as it is; + * `unmanaged`: it runs this CLI's executable and differs only in content, + * but has no {@link LAUNCHD_MANAGED_MARKER}, so it was left as it is. */ export type LaunchdProgramRepairResult = | "ok" | "repaired" | "refreshed" + | "other-program" + | "unmanaged" | "not-installed" | "failed"; @@ -427,12 +484,61 @@ export function isLaunchdPlistCurrent(opts: { } /** - * Repairs an installed launchd plist whose program path is invalid — a - * compiled-binary virtual path (/$bunfs) or a binary that no longer exists - * (e.g. an upgraded homebrew Cellar path). Broken plists leave launchd - * flapping with exit status 78 and the daemon unable to stay up. A plist - * whose program is current but whose other content differs from what this - * CLI renders is rewritten too ("refreshed"). + * What the installed plist's program is, relative to this CLI: + * `missing` (absent, unreadable or a compiled-binary virtual path: never + * runnable), `ours` (the same file as this CLI's resolution, see + * {@link isSameExecutableFile}), or `other` (another existing program, which + * the user may have chosen; only `hack daemon install` switches it). + * `resolved` is this CLI's program, null when none resolves. + */ +export type LaunchdProgramState = + | { + readonly kind: "missing" | "ours" | "other"; + readonly program: string | null; + readonly resolved: string | null; + } + | { readonly kind: "not-installed" }; + +export async function readLaunchdProgramState(opts: { + readonly paths: DaemonPaths; + readonly plistText?: string | null; +}): Promise { + if (!resolveLaunchdAccess().allowed) { + return { kind: "not-installed" }; + } + const plistText = + opts.plistText ?? (await readTextFile(opts.paths.launchdPlistPath)); + if (plistText === null) { + return { kind: "not-installed" }; + } + const program = extractLaunchdProgramPath({ plistText }); + const resolved = await resolveLaunchdHackBinPath({ + invocation: await resolveHackInvocation(), + }); + const exists = + program !== null && + !isVirtualExecutablePath(program) && + (await pathExists(program)); + if (!exists) { + return { kind: "missing", program, resolved }; + } + const ours = + resolved !== null && (await isSameExecutableFile(program, resolved)); + return { kind: ours ? "ours" : "other", program, resolved }; +} + +/** + * Repairs an installed launchd plist (HACK-2216): + * - a program that is gone or a compiled-binary virtual path (/$bunfs) is + * retargeted to this CLI ("repaired"): such a plist leaves launchd + * flapping with exit status 78; + * - a plist that runs this CLI's executable but differs from what it + * renders is rewritten for its own program path ("refreshed"), only when + * it carries {@link LAUNCHD_MANAGED_MARKER}, or when its output paths are + * unbounded (the older repair, kept); + * - another existing program is left alone ("other-program"), even a stale + * versioned path; `hack daemon install` switches it. A plist without the + * marker that only differs in content is left alone too ("unmanaged"). * * Reports "not-installed" without reading the plist when * {@link resolveLaunchdAccess} denies access: this process does not own the @@ -448,46 +554,50 @@ export async function repairLaunchdProgramIfInvalid(opts: { if (plistText === null) { return "not-installed"; } - const program = extractLaunchdProgramPath({ plistText }); - const invocation = await resolveHackInvocation(); - const resolved = await resolveLaunchdHackBinPath({ invocation }); - const exists = - program !== null && - !isVirtualExecutablePath(program) && - (await pathExists(program)); - // A program that still exists can still be stale: a versioned homebrew - // Cellar path survives until brew cleanup, so kickstart would relaunch - // the OLD binary forever (incompatible-daemon loop). Normalize to the - // current stable resolution whenever it differs. - const valid = exists && (resolved === null || program === resolved); - // Without a resolvable hack this CLI cannot render (or install) a new - // plist, so only the output paths are checked then. - const current = - resolved === null - ? hasBoundedLaunchdOutput({ plistText }) - : isLaunchdPlistCurrent({ - plistText, - hackBinPath: resolved, - home: resolveUserHomeDir(), - }); - if (valid && current) { - return "ok"; - } - if (!exists && resolved === null) { + const state = await readLaunchdProgramState({ + paths: opts.paths, + plistText, + }); + if (state.kind === "not-installed") { + return "not-installed"; + } + if (state.kind === "other") { + return "other-program"; + } + if (state.kind === "missing" && state.resolved === null) { return "failed"; } - + const program = + state.kind === "ours" && state.program !== null ? state.program : null; + if (program !== null) { + const current = isLaunchdPlistCurrent({ + plistText, + hackBinPath: program, + home: resolveUserHomeDir(), + }); + if (current) { + return "ok"; + } + if ( + hasBoundedLaunchdOutput({ plistText }) && + !isHackManagedPlist({ plistText }) + ) { + return "unmanaged"; + } + } const result = await installLaunchdService({ paths: opts.paths, config: { installed: true, ...readLaunchdPlistOptions({ plistText }), }, + // A refresh keeps the plist's own program path; a repair uses ours. + ...(program === null ? {} : { hackBinPath: program }), }); if (!result.ok) { return "failed"; } - return valid ? "refreshed" : "repaired"; + return program === null ? "repaired" : "refreshed"; } export interface LaunchdUninstallResult { diff --git a/src/daemon/process-ancestry.ts b/src/daemon/process-ancestry.ts new file mode 100644 index 000000000..eb986ae04 --- /dev/null +++ b/src/daemon/process-ancestry.ts @@ -0,0 +1,66 @@ +import { readLinuxParentPid } from "./process-image.ts"; + +const MAX_DEPTH = 64; +const DIGITS = /^\d+$/; + +/** + * Whether `ancestor` is this process (or `pid`) or one of its ancestors, + * walking parent pids up to pid 1. Used before restarting a launchd job: + * a `hack daemon start` run by a job hackd started must not restart the + * service it runs under (HACK-2216). Null when the chain cannot be read; + * callers then refuse. + */ +export async function hasAncestorProcess(opts: { + readonly ancestor: number; + readonly pid?: number; + /** Defaults to `/proc` on Linux and `ps` on macOS. */ + readonly readParent?: (pid: number) => Promise; +}): Promise { + const readParent = opts.readParent ?? defaultReadParent; + let current = opts.pid ?? process.pid; + for (let depth = 0; depth < MAX_DEPTH; depth++) { + if (current === opts.ancestor) { + return true; + } + if (current <= 1) { + return false; + } + const parent = await readParent(current); + if (parent === null) { + return null; + } + current = parent; + } + return null; +} + +function defaultReadParent(pid: number): Promise { + return process.platform === "linux" + ? readLinuxParentPid(pid) + : readParentWithPs(pid); +} + +async function readParentWithPs(pid: number): Promise { + try { + const proc = Bun.spawn(["/bin/ps", "-o", "ppid=", "-p", String(pid)], { + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + }); + const timer = setTimeout(() => proc.kill("SIGKILL"), 5000); + try { + const [code, text] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + ]); + const value = text.trim(); + return code === 0 && DIGITS.test(value) + ? Number.parseInt(value, 10) + : null; + } finally { + clearTimeout(timer); + } + } catch { + return null; + } +} diff --git a/src/daemon/reconcile.ts b/src/daemon/reconcile.ts index 0075e7c07..07f296c47 100644 --- a/src/daemon/reconcile.ts +++ b/src/daemon/reconcile.ts @@ -6,6 +6,14 @@ * {@link planDaemonReconcile} and carry the plan out. */ +/** + * Set to `1` by `hack update` for the new executable's + * `hack daemon start --json`: reconcile a running hackd only. When no hackd + * of this home is running, start does nothing (action `none`), so a daemon + * that stopped after the update looked is never started. + */ +export const DAEMON_RECONCILE_ONLY_ENV = "HACK_DAEMON_RECONCILE_ONLY"; + /** Who runs hackd: a systemd user unit, a launchd agent, or nobody. */ export type DaemonServiceManager = "systemd" | "launchd" | "none"; @@ -56,12 +64,22 @@ export type ServiceUnitState = * - `other-program`: the unit runs another existing `hack`, which the user * may have chosen; `hack daemon install` switches it; * - `unknown-program`: what the unit runs cannot be read, so a restart is - * not known to run this version. + * not known to run this version; + * - `unmanaged`: the unit's main file does not start with Hack's managed + * header (it was written or edited by hand), so it is never rewritten + * automatically; `hack daemon install` replaces it; + * - `home-binding`: the unit's main file binds the service to a Hack home + * (`HACK_HOME`, `HOME` or an `EnvironmentFile=`) that a rewrite would drop; + * - `inside-service`: this command runs inside the service (for example a + * job hackd started), so restarting or rewriting it would stop the caller. */ export type DaemonReconcileBlock = | "read-only" | "other-program" - | "unknown-program"; + | "unknown-program" + | "unmanaged" + | "home-binding" + | "inside-service"; export interface DaemonReconcileInput { readonly unit: ServiceUnitState; @@ -82,6 +100,16 @@ export interface DaemonReconcileInput { * so the running service still has the old unit loaded. */ readonly restartPending?: boolean; + /** + * Why the unit's content must not be rewritten automatically even though + * it runs this CLI's executable; null or absent when it may be. + */ + readonly contentBlock?: "unmanaged" | "home-binding" | null; + /** + * This process runs inside the service, or that could not be ruled out. + * Only meaningful while the service runs hackd. + */ + readonly invokerInsideService?: boolean; readonly cliVersion: string; } @@ -110,10 +138,31 @@ export interface DaemonReconcilePlan { * - a running daemon is restarted only when the restart is known to run * this CLI's executable, or the unit changed under it; * - a version mismatch that a restart cannot fix is refused, never - * reported as success. + * reported as success; + * - nothing is rewritten or restarted from inside the running service: a + * stale but compatible daemon is kept, a mismatched one refused. */ export function planDaemonReconcile( input: DaemonReconcileInput +): DaemonReconcilePlan { + const plan = planWithoutCallerGuard(input); + if ( + input.running === null || + input.invokerInsideService !== true || + (plan.service !== "restart" && plan.rewrite === "none") + ) { + return plan; + } + return { + rewrite: "none", + service: plan.reasons.includes("version-mismatch") ? "refuse" : "keep", + reasons: plan.reasons, + blocked: "inside-service", + }; +} + +function planWithoutCallerGuard( + input: DaemonReconcileInput ): DaemonReconcilePlan { const mismatch = input.running !== null && @@ -185,9 +234,12 @@ function planContentRewrite(input: DaemonReconcileInput): { if (input.readOnly) { return { rewrite: "none", blocked: "read-only" }; } - return input.programIsOurs - ? { rewrite: "content", blocked: null } - : { rewrite: "none", blocked: "other-program" }; + if (!input.programIsOurs) { + return { rewrite: "none", blocked: "other-program" }; + } + return input.contentBlock + ? { rewrite: "none", blocked: input.contentBlock } + : { rewrite: "content", blocked: null }; } /** The `--json` action for a plan that was carried out. */ @@ -216,10 +268,12 @@ export interface DaemonCommandTrace { manager: DaemonServiceManager; action: DaemonCommandAction; readonly stale: Set; + /** Why a stale service was left as it is, if it was. */ + blocked: DaemonReconcileBlock | null; } export function createDaemonCommandTrace(): DaemonCommandTrace { - return { manager: "none", action: "none", stale: new Set() }; + return { manager: "none", action: "none", stale: new Set(), blocked: null }; } /** Records a carried-out plan on `trace`. */ @@ -234,6 +288,7 @@ export function recordReconcile(opts: { } trace.manager = opts.manager; trace.action = reconcileAction(opts.plan); + trace.blocked = opts.plan.blocked; for (const reason of opts.plan.reasons) { trace.stale.add(reason); } @@ -273,6 +328,7 @@ export function buildDaemonCommandData(opts: { readonly action: DaemonCommandAction; readonly stale: boolean; readonly stale_reasons: readonly DaemonStaleReason[]; + readonly blocked: DaemonReconcileBlock | null; readonly cli_version: string; readonly daemon: { readonly running: boolean; @@ -288,6 +344,7 @@ export function buildDaemonCommandData(opts: { action: opts.trace.action, stale: reasons.length > 0, stale_reasons: reasons, + blocked: opts.trace.blocked, cli_version: opts.cliVersion, daemon: { running: opts.daemon.running, diff --git a/src/daemon/systemd.ts b/src/daemon/systemd.ts index a772e427c..6656bcb03 100644 --- a/src/daemon/systemd.ts +++ b/src/daemon/systemd.ts @@ -47,9 +47,10 @@ import { * first `hack` on PATH), not its symlink target, so a stable symlink such as * `~/.local/bin/hack` keeps working when an upgrade moves the real binary. * When the recorded program no longer exists, `hack daemon start` rewrites - * the unit. When the unit runs this CLI's executable but differs from what - * it renders, or its hackd reports another version, start rewrites and - * restarts it (HACK-2216, `src/daemon/reconcile.ts`). A unit that runs + * the unit. When the unit runs this CLI's executable, start restarts a + * hackd of another version, and rewrites the unit when it differs from what + * this CLI renders and still carries {@link SYSTEMD_MANAGED_HEADER} + * (HACK-2216, `src/daemon/reconcile.ts`). A unit that runs * another existing `hack` is reported and switched by `hack daemon install`. * - `Restart=on-failure`: a crash restarts hackd; `hack daemon stop` (a clean * stop through systemctl) does not. Start limits stop a crash loop. @@ -487,6 +488,94 @@ export const RENDERED_SYSTEM_PATH_DIRECTORIES = [ "/bin", ] as const; +/** + * The first line of every unit Hack writes. Its second line tells the user + * that local changes are replaced on the next install, so only a unit that + * starts with it is rewritten automatically (HACK-2216). + */ +export const SYSTEMD_MANAGED_HEADER = + "# Managed by hack: written by `hack daemon install`, removed by `hack daemon uninstall`."; + +/** Whether the unit file's main text starts with {@link SYSTEMD_MANAGED_HEADER}. */ +export function isHackManagedUnit(opts: { + readonly unitText: string; +}): boolean { + return opts.unitText.split("\n", 1)[0] === SYSTEMD_MANAGED_HEADER; +} + +const ENVIRONMENT_SETTING = + /^\s*(Environment|EnvironmentFile|PassEnvironment|UnsetEnvironment)\s*=(.*)$/; +const HOME_BINDING_KEY = /(^|[\s"'])(HACK_HOME|HOME)(=|\s|"|'|$)/; + +/** + * An environment line of the unit's main file that binds the service to a + * Hack home (`HACK_HOME` or `HOME`, or any `EnvironmentFile=`, whose + * contents cannot be read here) and that `rendered` would drop. Rewriting + * such a unit would silently move the service to another home, so the + * automatic rewrite is refused; null when there is none (HACK-2216). + */ +export function findDroppedHomeBinding(opts: { + readonly unitText: string; + readonly rendered: string; +}): string | null { + const kept = new Set(opts.rendered.split("\n").map((line) => line.trim())); + for (const line of opts.unitText.split("\n")) { + const match = ENVIRONMENT_SETTING.exec(line); + if (match === null || kept.has(line.trim())) { + continue; + } + if ( + match[1] === "EnvironmentFile" || + HOME_BINDING_KEY.test(match[2] ?? "") + ) { + return line.trim(); + } + } + return null; +} + +/** + * Whether this process runs inside hackd.service, compared by cgroup: its + * own `/proc/self/cgroup` path is the unit's `ControlGroup` or below it. + * Restarting the unit from inside it (a supervisor job running + * `hack daemon start`) would stop the caller itself (HACK-2216). `unknown` + * when either side cannot be read; callers then refuse. + */ +export async function readInvokerInSystemdUnit(opts: { + readonly deps: SystemdDeps; + /** Defaults to reading `/proc/self/cgroup`. */ + readonly readSelfCgroup?: () => Promise; +}): Promise<"inside" | "outside" | "unknown"> { + const show = await opts.deps.run("systemctl", [ + "--user", + "--no-pager", + "show", + SYSTEMD_UNIT_NAME, + "--property=ControlGroup", + ]); + if (show.exitCode !== 0) { + return "unknown"; + } + const group = (parseSystemdProperties(show.stdout).ControlGroup ?? "").trim(); + if (group.length === 0) { + // No cgroup: the unit runs nothing, so nothing can run inside it. + return "outside"; + } + const self = await ( + opts.readSelfCgroup ?? (() => readTextFile("/proc/self/cgroup")) + )(); + if (self === null) { + return "unknown"; + } + const inside = self.split("\n").some((line) => { + // `::`; the path is everything after the 2nd `:`. + const parts = line.trimEnd().split(":"); + const path = parts.slice(2).join(":"); + return path === group || path.startsWith(`${group}/`); + }); + return inside ? "inside" : "outside"; +} + /** * Renders the unit file. Returns null for a program path the unit cannot * represent safely (relative, a compiled Bun virtual path, or containing @@ -511,7 +600,7 @@ export function renderSystemdUnit(opts: { "%h/.hack/bin", ].join(":"); return [ - "# Managed by hack: written by `hack daemon install`, removed by `hack daemon uninstall`.", + SYSTEMD_MANAGED_HEADER, "# Local changes are replaced on the next install; use `systemctl --user edit hackd` for overrides.", "[Unit]", "Description=hack local daemon (hackd)", diff --git a/tests/daemon-reconcile.test.ts b/tests/daemon-reconcile.test.ts index c8be7a40d..e7b8867aa 100644 --- a/tests/daemon-reconcile.test.ts +++ b/tests/daemon-reconcile.test.ts @@ -1,13 +1,30 @@ import { expect, test } from "bun:test"; +import { + chmod, + link, + mkdir, + mkdtemp, + rm, + symlink, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { checkRunningLaunchdDaemon } from "../src/commands/daemon-launchd.ts"; import { + observePostUpdateDaemon, parseStartEnvelope, reconcileDaemonAfterUpdate, } from "../src/commands/daemon-upgrade.ts"; import { + isHackManagedPlist, isLaunchdPlistCurrent, + isSameExecutableFile, renderLaunchdPlist, } from "../src/daemon/launchd.ts"; +import { resolveDaemonPaths } from "../src/daemon/paths.ts"; +import { hasAncestorProcess } from "../src/daemon/process-ancestry.ts"; import { buildDaemonCommandData, createDaemonCommandTrace, @@ -15,6 +32,13 @@ import { planDaemonReconcile, reconcileAction, } from "../src/daemon/reconcile.ts"; +import { + findDroppedHomeBinding, + isHackManagedUnit, + readInvokerInSystemdUnit, + renderSystemdUnit, + type SystemdDeps, +} from "../src/daemon/systemd.ts"; /** HACK-2216: the reconcile decision and its `--json` contract, no I/O. */ @@ -206,6 +230,7 @@ test("the --json data is closed and schema-tagged", () => { action: "reinstalled", stale: true, stale_reasons: ["content", "version-mismatch"], + blocked: null, cli_version: "2.0.0", daemon: { running: true, pid: 12, version: "2.0.0", compatible: true }, }); @@ -343,3 +368,283 @@ test("a failed or unreadable post-update reconcile is reported, never fatal", as next_step: "hack daemon install", }); }); + +// Review round 1 (HACK-2216). + +test("nothing is rewritten or restarted from inside the running service", () => { + const stale = planDaemonReconcile({ + ...base, + unit: "content", + running: { version: "2.0.0" }, + invokerInsideService: true, + }); + expect(stale).toEqual({ + rewrite: "none", + service: "keep", + reasons: ["content"], + blocked: "inside-service", + }); + expect( + planDaemonReconcile({ + ...base, + running: { version: "1.0.0" }, + invokerInsideService: true, + }) + ).toMatchObject({ service: "refuse", blocked: "inside-service" }); + // A current, compatible service is unaffected. + expect( + planDaemonReconcile({ + ...base, + running: { version: "2.0.0" }, + invokerInsideService: true, + }) + ).toMatchObject({ service: "keep", blocked: null }); +}); + +test("an unmanaged or home-bound unit is not rewritten, but a mismatch still restarts", () => { + for (const contentBlock of ["unmanaged", "home-binding"] as const) { + expect( + planDaemonReconcile({ ...base, unit: "content", contentBlock }) + ).toMatchObject({ + rewrite: "none", + service: "start", + blocked: contentBlock, + }); + expect( + planDaemonReconcile({ + ...base, + unit: "content", + contentBlock, + running: { version: "1.0.0" }, + }) + ).toMatchObject({ + rewrite: "none", + service: "restart", + blocked: contentBlock, + }); + } +}); + +test("the managed header and dropped home bindings are recognized", () => { + const rendered = + renderSystemdUnit({ hackBinPath: "/opt/hack/bin/hack" }) ?? ""; + expect(isHackManagedUnit({ unitText: rendered })).toBe(true); + expect(isHackManagedUnit({ unitText: `# mine\n${rendered}` })).toBe(false); + expect(findDroppedHomeBinding({ unitText: rendered, rendered })).toBeNull(); + const bound = rendered.replace( + "[Service]\n", + '[Service]\nEnvironment="HACK_HOME=/srv/other/.hack"\n' + ); + expect(findDroppedHomeBinding({ unitText: bound, rendered })).toBe( + 'Environment="HACK_HOME=/srv/other/.hack"' + ); + const homed = rendered.replace( + "[Service]\n", + "[Service]\nEnvironment=HOME=/srv/u\n" + ); + expect(findDroppedHomeBinding({ unitText: homed, rendered })).toBe( + "Environment=HOME=/srv/u" + ); + const file = rendered.replace( + "[Service]\n", + "[Service]\nEnvironmentFile=/srv/env\n" + ); + expect(findDroppedHomeBinding({ unitText: file, rendered })).toBe( + "EnvironmentFile=/srv/env" + ); + // Other variables are not a home binding. + const other = rendered.replace( + "[Service]\n", + "[Service]\nEnvironment=FOO=1\n" + ); + expect(findDroppedHomeBinding({ unitText: other, rendered })).toBeNull(); +}); + +function cgroupDeps(group: string | null): SystemdDeps { + return { + env: {}, + uid: 1000, + run: () => + Promise.resolve( + group === null + ? { exitCode: 1, stdout: "", stderr: "Failed to connect to bus" } + : { exitCode: 0, stdout: `ControlGroup=${group}\n`, stderr: "" } + ), + }; +} + +test("the caller is inside hackd.service when its cgroup is the unit's or below", async () => { + const unit = + "/user.slice/user-1000.slice/user@1000.service/app.slice/hackd.service"; + const self = (path: string) => () => Promise.resolve(`0::${path}\n`); + const read = (group: string | null, selfPath: string | null) => + readInvokerInSystemdUnit({ + deps: cgroupDeps(group), + readSelfCgroup: + selfPath === null ? () => Promise.resolve(null) : self(selfPath), + }); + expect(await read(unit, unit)).toBe("inside"); + expect(await read(unit, `${unit}/job-1`)).toBe("inside"); + expect(await read(unit, `${unit}-other`)).toBe("outside"); + expect(await read("", unit)).toBe("outside"); + expect(await read(null, unit)).toBe("unknown"); + expect(await read(unit, null)).toBe("unknown"); +}); + +test("process ancestry walks parent pids and reports an unreadable chain", async () => { + const parents = new Map([ + [50, 40], + [40, 30], + [30, 1], + ]); + const readParent = (pid: number) => Promise.resolve(parents.get(pid) ?? null); + expect(await hasAncestorProcess({ ancestor: 30, pid: 50, readParent })).toBe( + true + ); + expect(await hasAncestorProcess({ ancestor: 99, pid: 50, readParent })).toBe( + false + ); + expect( + await hasAncestorProcess({ ancestor: 99, pid: 60, readParent }) + ).toBeNull(); +}); + +test("launchd: inside the job, a running hackd is kept or refused, never restarted", async () => { + const trace = createDaemonCommandTrace(); + const hooks = { + paths: resolveDaemonPaths({ home: "/nonexistent-home" }), + trace, + pid: 4242, + waitForReady: () => Promise.reject(new Error("must not start")), + readStatus: () => + Promise.resolve({ + installed: true, + loaded: true, + running: true, + pid: 4242, + exitStatus: null, + }), + isAncestor: (pid: number) => Promise.resolve(pid === 4242), + }; + expect( + await checkRunningLaunchdDaemon({ ...hooks, compatible: true }) + ).toBeNull(); + expect(trace.blocked).toBe("inside-service"); + expect(await checkRunningLaunchdDaemon({ ...hooks, compatible: false })).toBe( + 1 + ); + // Unreadable ancestry counts as inside. + expect( + await checkRunningLaunchdDaemon({ + ...hooks, + compatible: false, + isAncestor: () => Promise.resolve(null), + }) + ).toBe(1); +}); + +test("the launchd plist carries the managed marker", () => { + const plist = renderLaunchdPlist({ + hackBinPath: "/opt/hack/bin/hack", + home: "/Users/example", + runAtLoad: false, + guiSessionOnly: false, + }); + expect(isHackManagedPlist({ plistText: plist })).toBe(true); + expect( + isHackManagedPlist({ plistText: "\n" }) + ).toBe(false); +}); + +test("executable identity follows links, not path strings", async () => { + const dir = await mkdtemp(join(tmpdir(), "hack-same-exe-")); + try { + const real = join(dir, "hack"); + const other = join(dir, "hack-other"); + await writeFile(real, "#!/bin/sh\n"); + await writeFile(other, "#!/bin/sh\n"); + await chmod(real, 0o755); + await symlink(real, join(dir, "hack-link")); + await link(real, join(dir, "hack-hard")); + expect(await isSameExecutableFile(join(dir, "hack-link"), real)).toBe(true); + expect(await isSameExecutableFile(join(dir, "hack-hard"), real)).toBe(true); + expect(await isSameExecutableFile(other, real)).toBe(false); + expect(await isSameExecutableFile(join(dir, "missing"), real)).toBe(false); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("after an update, a stale pid file is not a running daemon", async () => { + const home = await mkdtemp(join(tmpdir(), "hack-stale-pid-")); + try { + const paths = resolveDaemonPaths({ home }); + await mkdir(paths.root, { recursive: true }); + // A live pid that is not hackd (this test process), and no socket. + await writeFile(paths.pidPath, `${process.pid}\n`); + const installed = () => Promise.resolve(true); + expect( + await observePostUpdateDaemon({ paths, serviceInstalled: installed }) + ).toEqual({ serviceInstalled: true, daemonRunning: false }); + // A socket path nobody answers on is not proof either. + await writeFile(paths.socketPath, ""); + expect( + await observePostUpdateDaemon({ paths, serviceInstalled: installed }) + ).toEqual({ serviceInstalled: true, daemonRunning: false }); + expect( + await observePostUpdateDaemon({ + paths, + serviceInstalled: () => Promise.resolve(false), + }) + ).toEqual({ serviceInstalled: false, daemonRunning: false }); + } finally { + await rm(home, { recursive: true, force: true }); + } +}); + +test("after an update, a rejected observer is reported, never thrown", async () => { + expect( + await reconcileDaemonAfterUpdate({ + binaryPath: "/opt/hack/bin/hack", + observe: () => Promise.reject(new Error("status unreadable")), + runCli: () => Promise.reject(new Error("must not run")), + }) + ).toEqual({ + status: "failed", + message: "status unreadable", + next_step: "hack daemon install", + }); +}); + +test("after an update, a daemon that stopped before the child ran is not started", async () => { + // The child runs reconcile-only and reports `none`: a no-op, not a start. + // (tests/daemon-systemd-cli.test.ts runs that mode through the CLI.) + const ran: Record[] = []; + const result = await reconcileDaemonAfterUpdate({ + binaryPath: "/opt/hack/bin/hack", + observe: () => + Promise.resolve({ serviceInstalled: true, daemonRunning: true }), + runCli: () => { + ran.push({}); + return Promise.resolve({ + exitCode: 0, + stdout: JSON.stringify({ + ok: true, + data: { + schema: "hack.daemon-start/v1", + manager: "none", + action: "none", + stale_reasons: [], + }, + }), + }); + }, + }); + expect(ran).toHaveLength(1); + expect(result).toEqual({ + status: "reconciled", + manager: "none", + action: "none", + stale_reasons: [], + }); +}); diff --git a/tests/daemon-systemd-cli.test.ts b/tests/daemon-systemd-cli.test.ts index 5fb133d42..db59f7a5c 100644 --- a/tests/daemon-systemd-cli.test.ts +++ b/tests/daemon-systemd-cli.test.ts @@ -546,10 +546,27 @@ test.skipIf(process.platform !== "linux")( action: "none", stale: false, stale_reasons: [], + blocked: null, cli_version: expect.any(String), daemon: { running: false, pid: null, version: null, compatible: false }, }, }); + // The post-update child's reconcile-only mode never starts hackd. + const reconcileOnly = await hack( + { + ...isolated, + env: { ...isolated.env, HACK_DAEMON_RECONCILE_ONLY: "1" }, + }, + ["daemon", "start", "--json"] + ); + expect(reconcileOnly.code).toBe(0); + expect(JSON.parse(reconcileOnly.stdout)).toMatchObject({ + ok: true, + data: { action: "none", daemon: { running: false } }, + }); + expect( + await Bun.file(join(f.root, "isolated", "daemon", "hackd.pid")).exists() + ).toBe(false); const foreground = await hack(isolated, [ "daemon", "start", diff --git a/tests/daemon-systemd.test.ts b/tests/daemon-systemd.test.ts index c685146bc..e4801490c 100644 --- a/tests/daemon-systemd.test.ts +++ b/tests/daemon-systemd.test.ts @@ -4570,3 +4570,146 @@ linuxTest( ).toBeNull(); } ); + +// Review round 1 (HACK-2216). + +const UNIT_CGROUP = + "/user.slice/user-1000.slice/user@1000.service/app.slice/hackd.service"; + +/** Answers `--property=ControlGroup` for the fake; everything else as before. */ +function withControlGroup(fake: Fake): void { + const run = fake.deps.run; + fake.deps = { + ...fake.deps, + run: (command, args) => + args.includes("--property=ControlGroup") + ? Promise.resolve({ + exitCode: 0, + stdout: `ControlGroup=${UNIT_CGROUP}\n`, + stderr: "", + }) + : run(command, args), + }; +} + +linuxTest( + "from inside hackd.service, a mismatched daemon is refused and nothing is restarted", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await writeStaleUnit(fake); + const stale = await readFile(unitPath, "utf8"); + withControlGroup(fake); + const inside = () => Promise.resolve(`0::${UNIT_CGROUP}/job-7\n`); + const mismatch = { + ...runningHooks(fake, "1.0.0"), + readSelfCgroup: inside, + }; + expect(await reconcileRunningSystemdDaemon(mismatch)).toBe(1); + expect(mismatch.trace.blocked).toBe("inside-service"); + // Compatible but stale: kept, nothing rewritten, start goes on. + const compatible = { + ...runningHooks(fake, "2.0.0"), + readSelfCgroup: inside, + }; + expect(await reconcileRunningSystemdDaemon(compatible)).toBeNull(); + expect(compatible.trace.action).toBe("none"); + expect(compatible.trace.blocked).toBe("inside-service"); + expect(await readFile(unitPath, "utf8")).toBe(stale); + expect(fake.calls).not.toContain("systemctl --user restart hackd.service"); + expect(fake.calls).not.toContain("systemctl --user daemon-reload"); + } +); + +linuxTest( + "outside hackd.service (another cgroup), the restart goes ahead", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await installSystemdUnit({ + unitPath, + unitText: unitText(), + deps: fake.deps, + }); + fake.calls.length = 0; + withControlGroup(fake); + expect( + await reconcileRunningSystemdDaemon({ + ...runningHooks(fake, "1.0.0"), + readSelfCgroup: () => + Promise.resolve("0::/user.slice/user-1000.slice/session-2.scope\n"), + }) + ).toBe(0); + expect(fake.calls).toContain("systemctl --user restart hackd.service"); + } +); + +linuxTest( + "a stale unit without hack's managed header is reported, not rewritten", + async () => { + const fake = fakeSystemd(); + const handWritten = unitText() + .replace(/^# Managed by hack.*\n/, "# my unit\n") + .replace("RestartSec=2", "RestartSec=9"); + await installSystemdUnit({ + unitPath, + unitText: handWritten, + deps: fake.deps, + }); + fake.calls.length = 0; + const trace = createDaemonCommandTrace(); + expect( + await startViaSystemdIfInstalled({ ...startHooks(fake), trace }) + ).toBe(0); + expect(await readFile(unitPath, "utf8")).toBe(handWritten); + expect(fake.calls).not.toContain("systemctl --user daemon-reload"); + expect(fake.calls).toContain("systemctl --user start hackd.service"); + expect(trace.action).toBe("started"); + expect(trace.blocked).toBe("unmanaged"); + expect([...trace.stale]).toEqual(["content"]); + } +); + +linuxTest( + "a custom-home unit is not rewritten into the default home, and a mismatch restarts it unchanged", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + const home = join(root, ".hack"); + const custom = unitText() + .replace("[Service]\n", `[Service]\nEnvironment="HACK_HOME=${home}"\n`) + .replace("RestartSec=2", "RestartSec=9"); + await installSystemdUnit({ unitPath, unitText: custom, deps: fake.deps }); + fake.unitEnvironment = `PATH=${sysBin} HACK_HOME=${home}`; + fake.calls.length = 0; + const hooks = runningHooks(fake, "1.0.0"); + expect(await reconcileRunningSystemdDaemon(hooks)).toBe(0); + expect(await readFile(unitPath, "utf8")).toBe(custom); + expect(fake.calls).toContain("systemctl --user restart hackd.service"); + expect(fake.calls).not.toContain("systemctl --user daemon-reload"); + expect(hooks.trace.blocked).toBe("home-binding"); + expect(hooks.trace.action).toBe("restarted"); + } +); + +linuxTest( + "after a rewrite, the unit's home is checked again before the restart", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await writeStaleUnit(fake); + // A drop-in that only takes effect at the reload points the service + // at another home. + const run = fake.deps.run; + fake.deps = { + ...fake.deps, + run: (command, args) => { + if (args.includes("daemon-reload")) { + fake.unitEnvironment = `PATH=${sysBin} HACK_HOME=${join(root, "other")}`; + } + return run(command, args); + }, + }; + expect( + await reconcileRunningSystemdDaemon(runningHooks(fake, "1.0.0")) + ).toBe(1); + expect(fake.calls).toContain("systemctl --user daemon-reload"); + expect(fake.calls).not.toContain("systemctl --user restart hackd.service"); + } +); diff --git a/tests/launchd-isolation.test.ts b/tests/launchd-isolation.test.ts index 8f4f52b67..96124a0d4 100644 --- a/tests/launchd-isolation.test.ts +++ b/tests/launchd-isolation.test.ts @@ -1,5 +1,6 @@ import { afterEach, expect, test } from "bun:test"; import { createHash } from "node:crypto"; +import { chmodSync, copyFileSync, mkdirSync } from "node:fs"; import { chmod, mkdir, @@ -349,6 +350,88 @@ test("repairs legacy output paths even when the installed program is still curre expect(verbs).toEqual(["bootout", "bootstrap"]); }); +/** Runs the driver's two repairs against `plist`; returns results and calls. */ +async function repairPlist(plist: (fixture: Fixture) => string): Promise<{ + readonly results: unknown; + readonly after: string; + readonly verbs: string[]; +}> { + const fixture = await makeFixture(); + await writeFile(fixture.plistPath, plist(fixture)); + const run = await runAsDarwin({ + argv: [DRIVER, "repair-output"], + cwd: fixture.root, + env: fixture.env({ HACK_HOME: join(fixture.home, ".hack") }), + }); + expect(run.code).toBe(0); + return { + results: JSON.parse(run.stdout), + after: await readFile(fixture.plistPath, "utf8"), + verbs: (await launchctlCalls(fixture)).map( + (line) => line.split(" ")[0] ?? "" + ), + }; +} + +test("a plist that runs another existing hack is never retargeted (HACK-2216)", async () => { + let written = ""; + const { results, after, verbs } = await repairPlist((fixture) => { + const other = join(fixture.root, "other-bin", "hack"); + // Created before the driver runs: another installed hack the user chose. + mkdirSync(join(fixture.root, "other-bin"), { recursive: true }); + copyFileSync(join(fixture.root, "tools", "hack"), other); + chmodSync(other, 0o755); + written = renderLaunchdPlist({ + hackBinPath: other, + home: fixture.home, + runAtLoad: false, + guiSessionOnly: false, + }); + return written; + }); + expect(results).toEqual({ + repair: "other-program", + secondRepair: "other-program", + }); + expect(after).toBe(written); + expect(verbs).toEqual([]); +}); + +test("a plist for this hack without the managed marker is not refreshed (HACK-2216)", async () => { + let written = ""; + const { results, after, verbs } = await repairPlist((fixture) => { + written = renderLaunchdPlist({ + hackBinPath: join(fixture.root, "tools", "hack"), + home: fixture.home, + runAtLoad: false, + guiSessionOnly: false, + }) + .split("\n") + .filter((line) => !line.startsWith("` comment that `hack daemon install` now writes) or -its output paths are not `/dev/null` (the earlier output repair). launchd then -restarts hackd from it. A plist that runs another existing `hack` is left as it -is, even an older versioned install path. A running hackd of another version -under it is refused with `hack daemon install`. +On macOS, Hack reads the plist only at its own LaunchAgents path for its label +(`dance.hack.hackd`). launchd has no drop-ins, so the systemd header rule does +not apply there. A plist counts as Hack's when it carries Hack's managed marker +(a `` comment that `hack daemon install` now writes) +or its `Label` is Hack's. That covers plists written by earlier Hack versions, +which have no marker. + +- A plist whose program is gone (or is a compiled-binary virtual path) is + retargeted to this CLI. +- A plist that runs this CLI's executable (same file) is rewritten with the + marker when it differs from what this CLI writes, if it is Hack's or its + output paths are not `/dev/null` (the earlier output repair). launchd then + restarts hackd from it. One that is not Hack's (no marker and another + `Label`) is only reported. +- A plist that runs another existing `hack` is left as it is, even an older + versioned install path. A running hackd of another version under it is + refused with `hack daemon install`. ```mermaid stateDiagram-v2 diff --git a/docs/versions/v5/upgrade.md b/docs/versions/v5/upgrade.md index 907d952f4..790a35973 100644 --- a/docs/versions/v5/upgrade.md +++ b/docs/versions/v5/upgrade.md @@ -115,8 +115,9 @@ backup scripts, source evidence and checks still awaiting real-machine trials. Installing a CLI does not replace a running daemon. After choosing a durable v5 install, run its `hack daemon start` (or `hack daemon restart`). When the login service runs that same executable, start restarts hackd on the new version and -rewrites a stale unit or plist that Hack wrote (it carries Hack's managed -header). `hack update` does this by itself when a service is installed and +rewrites a stale unit that Hack wrote (it carries Hack's managed header) or a +stale launchd plist with Hack's label, including one an earlier Hack wrote. +`hack update` does this by itself when a service is installed and hackd is running. A hand-written unit, a unit that sets its own Hack home, or a service that points at another existing executable is reported instead: use the chosen install's `hack daemon install`. Add `--json` for one machine-readable result. Keep this diff --git a/src/commands/daemon-launchd.ts b/src/commands/daemon-launchd.ts index ba2997a0e..a840dff59 100644 --- a/src/commands/daemon-launchd.ts +++ b/src/commands/daemon-launchd.ts @@ -182,7 +182,7 @@ function noteRepair(opts: { trace.blocked = "unmanaged"; logger.info({ message: - "The launchd plist differs from what this hack writes, but it was not written by this hack (no managed marker), so it is left as it is | To replace it, run: hack daemon install", + "The launchd plist differs from what this hack writes, but it is not hack's (no managed marker and another Label), so it is left as it is | To replace it, run: hack daemon install", }); return false; default: diff --git a/src/daemon/launchd.ts b/src/daemon/launchd.ts index 6aebd8f69..68d90678b 100644 --- a/src/daemon/launchd.ts +++ b/src/daemon/launchd.ts @@ -155,18 +155,38 @@ export interface LaunchdPlistOptions { } /** - * Marks a plist Hack wrote and may rewrite whole (HACK-2216). A plist - * without it, such as one written by an older Hack or edited by hand, is - * only repaired when its program is gone. + * Marks a plist Hack wrote and may rewrite whole (HACK-2216). Plists from + * earlier Hack versions lack it; see {@link isHackManagedPlist}. */ export const LAUNCHD_MANAGED_MARKER = ""; -/** Whether the plist carries {@link LAUNCHD_MANAGED_MARKER}. */ +const PLIST_LABEL_PATTERN = /Label<\/key>\s*([^<]*)<\/string>/; + +/** The plist's `Label` string, or null when it has none. */ +export function readLaunchdPlistLabel(opts: { + readonly plistText: string; +}): string | null { + return PLIST_LABEL_PATTERN.exec(opts.plistText)?.[1] ?? null; +} + +/** + * Whether Hack may rewrite the plist whole (owner decision, HACK-2216): it + * carries {@link LAUNCHD_MANAGED_MARKER}, or its `Label` is Hack's own + * ({@link DAEMON_LAUNCHD_LABEL}). Callers read it only from Hack's own + * LaunchAgents path and rewrite it only when its program is this CLI's + * executable or is gone, so a plist from an earlier Hack (no marker) is + * still refreshed on upgrade. launchd has no drop-ins, so the systemd + * header rule (HACK-2052) does not carry over; a plist that runs another + * existing program is still left alone. + */ export function isHackManagedPlist(opts: { readonly plistText: string; }): boolean { - return opts.plistText.split("\n").includes(LAUNCHD_MANAGED_MARKER); + return ( + opts.plistText.split("\n").includes(LAUNCHD_MANAGED_MARKER) || + readLaunchdPlistLabel(opts) === DAEMON_LAUNCHD_LABEL + ); } /** @@ -442,7 +462,8 @@ export function hasBoundedLaunchdOutput(opts: { * renders, and the plist was rewritten (HACK-2216); * `other-program`: it runs another existing program, left as it is; * `unmanaged`: it runs this CLI's executable and differs only in content, - * but has no {@link LAUNCHD_MANAGED_MARKER}, so it was left as it is. + * but is not Hack's ({@link isHackManagedPlist}: no marker and another + * `Label`), so it was left as it is. */ export type LaunchdProgramRepairResult = | "ok" @@ -533,12 +554,13 @@ export async function readLaunchdProgramState(opts: { * retargeted to this CLI ("repaired"): such a plist leaves launchd * flapping with exit status 78; * - a plist that runs this CLI's executable but differs from what it - * renders is rewritten for its own program path ("refreshed"), only when - * it carries {@link LAUNCHD_MANAGED_MARKER}, or when its output paths are - * unbounded (the older repair, kept); + * renders is rewritten for its own program path, with the marker + * ("refreshed"), when it is Hack's ({@link isHackManagedPlist}: the + * marker, or Hack's `Label`, which covers plists from earlier Hack + * versions) or its output paths are unbounded (the older repair, kept); * - another existing program is left alone ("other-program"), even a stale - * versioned path; `hack daemon install` switches it. A plist without the - * marker that only differs in content is left alone too ("unmanaged"). + * versioned path; `hack daemon install` switches it. A plist that is not + * Hack's and only differs in content is left alone too ("unmanaged"). * * Reports "not-installed" without reading the plist when * {@link resolveLaunchdAccess} denies access: this process does not own the diff --git a/tests/daemon-reconcile.test.ts b/tests/daemon-reconcile.test.ts index e7b8867aa..8ad7cf57f 100644 --- a/tests/daemon-reconcile.test.ts +++ b/tests/daemon-reconcile.test.ts @@ -21,6 +21,7 @@ import { isHackManagedPlist, isLaunchdPlistCurrent, isSameExecutableFile, + LAUNCHD_MANAGED_MARKER, renderLaunchdPlist, } from "../src/daemon/launchd.ts"; import { resolveDaemonPaths } from "../src/daemon/paths.ts"; @@ -551,9 +552,19 @@ test("the launchd plist carries the managed marker", () => { guiSessionOnly: false, }); expect(isHackManagedPlist({ plistText: plist })).toBe(true); + expect(plist).toContain(LAUNCHD_MANAGED_MARKER); expect( isHackManagedPlist({ plistText: "\n" }) ).toBe(false); + // An earlier Hack's plist (no marker) still counts by its Label. + const legacy = plist.replace(`${LAUNCHD_MANAGED_MARKER}\n`, ""); + expect(legacy).not.toContain(LAUNCHD_MANAGED_MARKER); + expect(isHackManagedPlist({ plistText: legacy })).toBe(true); + expect( + isHackManagedPlist({ + plistText: legacy.replace("dance.hack.hackd", "example.custom.hackd"), + }) + ).toBe(false); }); test("executable identity follows links, not path strings", async () => { diff --git a/tests/launchd-isolation.test.ts b/tests/launchd-isolation.test.ts index 96124a0d4..3bdb10274 100644 --- a/tests/launchd-isolation.test.ts +++ b/tests/launchd-isolation.test.ts @@ -397,20 +397,46 @@ test("a plist that runs another existing hack is never retargeted (HACK-2216)", expect(verbs).toEqual([]); }); -test("a plist for this hack without the managed marker is not refreshed (HACK-2216)", async () => { +/** What an earlier Hack wrote: no marker, and an older environment. */ +function legacyPlist(fixture: Fixture): { legacy: string; current: string } { + const current = renderLaunchdPlist({ + hackBinPath: join(fixture.root, "tools", "hack"), + home: fixture.home, + runAtLoad: false, + guiSessionOnly: false, + }); + const legacy = current + .split("\n") + .filter((line) => !line.startsWith("/g; +const TAG = /^<(\/?)([A-Za-z][\w.-]*)(\s+[^<>]*?)?(\/?)>/; +/** Values that hold plain text (a `` is read by the dict reader). */ +const SCALAR_TAGS = new Set(["string", "integer", "real", "date", "data"]); +const EMPTY_VALUE_TAGS = new Set(["true", "false", "string", "array", "dict"]); + +class PlistShapeError extends Error {} + +/** + * Unknown when: the text is not a property list; it has an unterminated + * comment, CDATA, a processing instruction or a doctype past the prolog; + * its root is not a ``; a tag is malformed or unbalanced; a root key + * repeats; `Program` is not a string or `ProgramArguments` not an array + * starting with a string; or the path uses an XML escape or is relative. + * Comments are dropped as launchd drops them, and keys of nested dicts + * (for example inside `EnvironmentVariables`) never count. + */ +export function parseLaunchdPlistProgram(opts: { + readonly plistText: string; +}): LaunchdPlistProgram { + let root: Map | null; + try { + root = readRootDict(opts.plistText); + } catch (error: unknown) { + return { + kind: "unknown", + reason: error instanceof PlistShapeError ? error.message : "unreadable", + }; + } + if (root === null) { + return { kind: "none" }; + } + const program = root.get("Program"); + if (program !== undefined) { + return program.kind === "string" + ? programPath(program.text) + : { kind: "unknown", reason: "Program is not a plain string value" }; + } + const args = root.get("ProgramArguments"); + if (args === undefined) { + return { kind: "none" }; + } + return args.kind === "array" && args.first?.kind === "string" + ? programPath(args.first.text) + : { + kind: "unknown", + reason: "ProgramArguments is not an array that starts with a string", + }; +} + +function programPath(raw: string): LaunchdPlistProgram { + const path = raw.trim(); + if (path.includes("&")) { + return { kind: "unknown", reason: "the program path uses an XML escape" }; + } + if (!path.startsWith("/")) { + return { kind: "unknown", reason: "the program path is not absolute" }; + } + return { kind: "path", path }; +} + +/** The root dict's keys, or null for an empty ``. Throws on shape. */ +function readRootDict(text: string): Map | null { + const tokens = tokenize(text); + let i = skipSpace(tokens, 0); + const plist = tokens[i]; + if (plist?.kind === "empty" && plist.name === "plist") { + expectEnd(tokens, i + 1); + return null; + } + if (!(plist?.kind === "open" && plist.name === "plist")) { + throw new PlistShapeError("not a property list"); + } + i = skipSpace(tokens, i + 1); + const first = tokens[i]; + if (first?.kind === "close" && first.name === "plist") { + expectEnd(tokens, i + 1); + return null; + } + if (first?.kind === "empty" && first.name === "dict") { + i = expectClose(tokens, skipSpace(tokens, i + 1), "plist"); + expectEnd(tokens, i); + return new Map(); + } + if (!(first?.kind === "open" && first.name === "dict")) { + throw new PlistShapeError("the plist root is not a dict"); + } + const parsed = readDictBody(tokens, i + 1, true); + i = expectClose(tokens, skipSpace(tokens, parsed.next), "plist"); + expectEnd(tokens, i); + return parsed.keys; +} + +function tokenize(input: string): Token[] { + const text = input.replace(PROLOG, "").replace(COMMENT, ""); + if (text.includes(" 0) { + const lt = rest.indexOf("<"); + if (lt < 0) { + tokens.push({ kind: "text", text: rest }); + break; + } + if (lt > 0) { + tokens.push({ kind: "text", text: rest.slice(0, lt) }); + rest = rest.slice(lt); + continue; + } + const tag = readTag(rest); + tokens.push(tag.token); + rest = rest.slice(tag.length); + } + return tokens; +} + +function readTag(rest: string): { + readonly token: Token; + readonly length: number; +} { + const match = TAG.exec(rest); + if (match === null) { + throw new PlistShapeError("it has a malformed tag"); + } + const [whole, closing, name = "", , selfClosing] = match; + if (closing && selfClosing) { + throw new PlistShapeError("it has a malformed tag"); + } + let kind: "open" | "close" | "empty" = "open"; + if (closing) { + kind = "close"; + } else if (selfClosing) { + kind = "empty"; + } + return { token: { kind, name }, length: whole.length }; +} + +function skipSpace(tokens: readonly Token[], start: number): number { + let i = start; + while (true) { + const token = tokens[i]; + if (!(token?.kind === "text" && token.text.trim() === "")) { + return i; + } + i += 1; + } +} + +function expectClose( + tokens: readonly Token[], + i: number, + name: string +): number { + const token = tokens[i]; + if (!(token?.kind === "close" && token.name === name)) { + throw new PlistShapeError(`it has an unbalanced <${name}>`); + } + return i + 1; +} + +function expectEnd(tokens: readonly Token[], i: number): void { + if (skipSpace(tokens, i) !== tokens.length) { + throw new PlistShapeError("it has content after the root element"); + } +} + +/** + * Reads ``/value pairs up to ``. Only the root dict keeps its + * keys; a nested dict is read for shape and discarded. + */ +function readDictBody( + tokens: readonly Token[], + start: number, + keep: boolean +): { readonly keys: Map; readonly next: number } { + const keys = new Map(); + let i = start; + while (true) { + i = skipSpace(tokens, i); + const token = tokens[i]; + if (token?.kind === "close" && token.name === "dict") { + return { keys, next: i + 1 }; + } + if (!(token?.kind === "open" && token.name === "key")) { + throw new PlistShapeError("a dict entry is not a "); + } + const key = readScalar(tokens, i, "key"); + const value = readValue(tokens, skipSpace(tokens, key.next)); + const name = key.text.trim(); + if (keep && keys.has(name)) { + throw new PlistShapeError(`the key ${name} appears twice`); + } + if (keep) { + keys.set(name, value.value); + } + i = value.next; + } +} + +function readScalar( + tokens: readonly Token[], + start: number, + name: string +): { readonly text: string; readonly next: number } { + const body = tokens[start + 1]; + if (body?.kind === "close" && body.name === name) { + return { text: "", next: start + 2 }; + } + if (body?.kind !== "text") { + throw new PlistShapeError(`a <${name}> does not hold plain text`); + } + return { text: body.text, next: expectClose(tokens, start + 2, name) }; +} + +function emptyValue(name: string): PlistValue { + if (name === "string") { + return { kind: "string", text: "" }; + } + if (name === "array") { + return { kind: "array", first: null }; + } + return { kind: "other" }; +} + +type ValueRead = { readonly value: PlistValue; readonly next: number }; + +function readValue(tokens: readonly Token[], start: number): ValueRead { + const token = tokens[start]; + if (token?.kind === "empty" && EMPTY_VALUE_TAGS.has(token.name)) { + return { value: emptyValue(token.name), next: start + 1 }; + } + if (token?.kind !== "open") { + throw new PlistShapeError("a dict key has no value"); + } + if (token.name === "dict") { + return { + value: { kind: "other" }, + next: readDictBody(tokens, start + 1, false).next, + }; + } + if (token.name === "array") { + return readArray(tokens, start + 1); + } + if (!SCALAR_TAGS.has(token.name)) { + throw new PlistShapeError(`it has an unexpected <${token.name}> value`); + } + const scalar = readScalar(tokens, start, token.name); + const value: PlistValue = + token.name === "string" + ? { kind: "string", text: scalar.text } + : { kind: "other" }; + return { value, next: scalar.next }; +} + +function readArray(tokens: readonly Token[], start: number): ValueRead { + let i = start; + let first: PlistValue | null = null; + while (true) { + i = skipSpace(tokens, i); + const next = tokens[i]; + if (next?.kind === "close" && next.name === "array") { + return { value: { kind: "array", first }, next: i + 1 }; + } + const item = readValue(tokens, i); + first ??= item.value; + i = item.next; + } +} diff --git a/src/daemon/launchd.ts b/src/daemon/launchd.ts index d2ae6eee0..8cedaae30 100644 --- a/src/daemon/launchd.ts +++ b/src/daemon/launchd.ts @@ -15,8 +15,15 @@ import { resolveHackInvocation } from "../lib/hack-cli.ts"; import { normalizeHomeDir, resolveUserHomeDir } from "../lib/home-dir.ts"; import { boundedTimeoutMs } from "../lib/process-deadline.ts"; import { findExecutableInPath } from "../lib/shell.ts"; +import { parseLaunchdPlistProgram } from "./launchd-plist.ts"; import type { DaemonPaths } from "./paths.ts"; +// biome-ignore lint/performance/noBarrelFile: keeps launchd's public surface in one module +export { + type LaunchdPlistProgram, + parseLaunchdPlistProgram, +} from "./launchd-plist.ts"; + const PID_PATTERN = /pid\s*=\s*(\d+)/i; const STATE_PATTERN = /state\s*=\s*(\w+)/i; const LAST_EXIT_CODE_PATTERN = /last exit code\s*=\s*(-?\d+)/i; @@ -429,85 +436,27 @@ export function isVirtualExecutablePath(path: string): boolean { } /** - * The executable launchd runs for a plist, with launchd's precedence: - * `Program` when present, else `ProgramArguments[0]` (HACK-2216). - * `none`: neither key is present, so nothing is run (absence established). - * `unknown`: the text is not a property list, a key appears twice, its - * value is not a plain string (or array of strings), it uses an XML escape - * this reader does not decode, or the path is not absolute. Unknown is - * never evidence that the program is missing: callers leave such a plist - * as it is. + * Whether a plist's program file exists, for deciding whether a retarget + * is allowed (HACK-2216). `missing` only for ENOENT or ENOTDIR; any other + * error (EACCES, EIO, ELOOP...) is `unknown`, which never authorizes an + * automatic replacement. {@link pathExists} is not used here: it reports + * every error as absence. */ -export type LaunchdPlistProgram = - | { readonly kind: "path"; readonly path: string } - | { readonly kind: "none" } - | { readonly kind: "unknown"; readonly reason: string }; - -const PLIST_ROOT = //]/; -const PROGRAM_KEY = /\s*Program\s*<\/key>/g; -const PROGRAM_ARGUMENTS_KEY = /\s*ProgramArguments\s*<\/key>/g; -const STRING_VALUE = /^\s*([^<]*)<\/string>/; -const ARRAY_FIRST_STRING = /^\s*\s*([^<]*)<\/string>/; - -export function parseLaunchdPlistProgram(opts: { - readonly plistText: string; -}): LaunchdPlistProgram { - const text = opts.plistText; - if (!PLIST_ROOT.test(text)) { - return { kind: "unknown", reason: "not a property list" }; - } - const program = readKeyValue({ text, key: PROGRAM_KEY, value: STRING_VALUE }); - if (program.kind !== "absent") { - return program.kind === "value" - ? programPath(program.value) - : { kind: "unknown", reason: `Program ${program.reason}` }; - } - const args = readKeyValue({ - text, - key: PROGRAM_ARGUMENTS_KEY, - value: ARRAY_FIRST_STRING, - }); - if (args.kind === "absent") { - return { kind: "none" }; - } - return args.kind === "value" - ? programPath(args.value) - : { kind: "unknown", reason: `ProgramArguments ${args.reason}` }; -} - -function readKeyValue(opts: { - readonly text: string; - readonly key: RegExp; - readonly value: RegExp; -}): - | { readonly kind: "absent" } - | { readonly kind: "value"; readonly value: string } - | { readonly kind: "invalid"; readonly reason: string } { - const keys = [...opts.text.matchAll(opts.key)]; - if (keys.length === 0) { - return { kind: "absent" }; - } - if (keys.length > 1) { - return { kind: "invalid", reason: "appears more than once" }; - } - const key = keys[0]; - const rest = opts.text.slice((key?.index ?? 0) + (key?.[0].length ?? 0)); - const match = opts.value.exec(rest); - if (match?.[1] === undefined) { - return { kind: "invalid", reason: "is not a plain string value" }; - } - return { kind: "value", value: match[1] }; -} - -function programPath(raw: string): LaunchdPlistProgram { - const path = raw.trim(); - if (path.includes("&")) { - return { kind: "unknown", reason: "the program path uses an XML escape" }; - } - if (!path.startsWith("/")) { - return { kind: "unknown", reason: "the program path is not absolute" }; +export async function classifyLaunchdProgramPath(opts: { + readonly path: string; + /** Defaults to `fs.stat`; tests inject errors. */ + readonly stat?: (path: string) => Promise; +}): Promise<"exists" | "missing" | "unknown"> { + try { + await (opts.stat ?? stat)(opts.path); + return "exists"; + } catch (error: unknown) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { readonly code?: unknown }).code + : undefined; + return code === "ENOENT" || code === "ENOTDIR" ? "missing" : "unknown"; } - return { kind: "path", path }; } /** @@ -607,6 +556,8 @@ export type LaunchdProgramState = export async function readLaunchdProgramState(opts: { readonly paths: DaemonPaths; readonly plistText?: string | null; + /** Defaults to `fs.stat`; tests inject errors. */ + readonly stat?: (path: string) => Promise; }): Promise { if (!resolveLaunchdAccess().allowed) { return { kind: "not-installed" }; @@ -625,14 +576,17 @@ export async function readLaunchdProgramState(opts: { } const program = parsed.kind === "path" ? parsed.path : null; // Absence is established only by no program at all, a virtual path, or - // a path that does not exist. - const exists = - program !== null && - !isVirtualExecutablePath(program) && - (await pathExists(program)); - if (!exists) { + // a path stat reports as ENOENT/ENOTDIR; any other stat error is unknown. + if (program === null || isVirtualExecutablePath(program)) { return { kind: "missing", program, resolved }; } + const presence = await classifyLaunchdProgramPath({ + path: program, + stat: opts.stat, + }); + if (presence !== "exists") { + return { kind: presence, program, resolved }; + } const ours = resolved !== null && (await isSameExecutableFile(program, resolved)); return { kind: ours ? "ours" : "other", program, resolved }; diff --git a/tests/daemon-orphan.test.ts b/tests/daemon-orphan.test.ts index 105de8294..e2cdc5c2b 100644 --- a/tests/daemon-orphan.test.ts +++ b/tests/daemon-orphan.test.ts @@ -159,6 +159,7 @@ test("extracts the launchd program path from plist text", () => { // A property list root is required: other text is unreadable (HACK-2216). const plist = [ '', + "", "Label", "dance.hack.hackd", "ProgramArguments", @@ -166,6 +167,7 @@ test("extracts the launchd program path from plist text", () => { " /$bunfs/root/hack", " daemon", "", + "", "", ].join("\n"); expect(extractLaunchdProgramPath({ plistText: plist })).toBe( diff --git a/tests/daemon-reconcile.test.ts b/tests/daemon-reconcile.test.ts index c0314db0f..7abaafb79 100644 --- a/tests/daemon-reconcile.test.ts +++ b/tests/daemon-reconcile.test.ts @@ -21,6 +21,7 @@ import { reconcileDaemonAfterUpdate, } from "../src/commands/daemon-upgrade.ts"; import { + classifyLaunchdProgramPath, isHackManagedPlist, isLaunchdPlistCurrent, isSameExecutableFile, @@ -829,3 +830,76 @@ test("after an update, a daemon that stopped before the child ran is not started stale_reasons: [], }); }); + +// Review round 3 (HACK-2216): only root-dict keys count, and only +// ENOENT/ENOTDIR establish a missing program. + +const ROUND3_PLIST = (body: string) => + `\n\n\n\n Label\n dance.hack.hackd\n${body}\n\n\n`; +const ROUND3_ARGS = (first: string) => + ` ProgramArguments\n \n ${first}\n daemon\n `; + +test("a Program key nested in another dict does not count", () => { + const plist = ROUND3_PLIST( + `${ROUND3_ARGS("/opt/other/hack")}\n EnvironmentVariables\n \n Program\n /missing/path\n ProgramArguments\n /missing/too\n ` + ); + expect(parseLaunchdPlistProgram({ plistText: plist })).toEqual({ + kind: "path", + path: "/opt/other/hack", + }); + // Nested only: the root names no program. + expect( + parseLaunchdPlistProgram({ + plistText: ROUND3_PLIST( + " EnvironmentVariables\n \n Program\n /x/hack\n " + ), + }) + ).toEqual({ kind: "none" }); +}); + +test("a commented-out Program key does not count", () => { + const plist = ROUND3_PLIST( + ` \n${ROUND3_ARGS("/opt/other/hack")}` + ); + expect(parseLaunchdPlistProgram({ plistText: plist })).toEqual({ + kind: "path", + path: "/opt/other/hack", + }); +}); + +test("structures the reader does not handle are unknown", () => { + for (const body of [ + " Program\n ", + `${ROUND3_ARGS("/opt/other/hack")}\n ", + }); + return written; + }); + expect(results).toEqual({ + repair: "other-program", + secondRepair: "other-program", + }); + expect(after).toBe(written); + expect(verbs).toEqual([]); +}); + +test.skipIf(process.getuid?.() === 0)( + "a program path that cannot be checked (EACCES) is never replaced", + async () => { + let locked = ""; + let written = ""; + try { + const { results, after, verbs } = await repairPlist((fixture) => { + locked = join(fixture.root, "locked"); + mkdirSync(locked, { recursive: true }); + copyFileSync(join(fixture.root, "tools", "hack"), join(locked, "hack")); + // No search permission: stat of locked/hack fails with EACCES. + chmodSync(locked, 0o000); + written = rootPlist({ + firstArgument: join(locked, "hack"), + extra: "", + }); + return written; + }); + expect(results).toEqual({ + repair: "unknown-program", + secondRepair: "unknown-program", + }); + expect(after).toBe(written); + expect(verbs).toEqual([]); + } finally { + if (locked) { + chmodSync(locked, 0o755); + } + } + } +); + test("macOS restart --json refuses a launchd service running another hack, before stopping anything", async () => { const fixture = await makeFixture(); const plist = programPlist({ program: otherHack(fixture) }); From 720f51edc5bd44a39ad2fbe79bf112187603f849 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 20:36:06 +0000 Subject: [PATCH 07/43] fix(daemon): fail closed on the final restart read, read-only pending restarts and escaped plist keys Review round 4 on HACK-2216: - The final ownership proof before an automatic restart needs a fresh, available state read with no state error; a failed read refuses (blocked unproven-owner) and the earlier snapshot is never reused. - Under HACK_DAEMON_SERVICE_READ_ONLY a pending restart alone no longer restarts a daemon that already runs this version; it stays pending and is reported (blocked read-only). A version mismatch is still restarted. - launchd plist root keys are read exactly as written (no trimming), and a root key with an XML entity or character reference is unknown; program values are not trimmed either. Co-Authored-By: Claude Opus 5.5 --- docs/architecture.md | 16 ++++-- src/commands/daemon-systemd.ts | 72 +++++++++++++++++++++++++-- src/daemon/launchd-plist.ts | 11 ++++- src/daemon/reconcile.ts | 18 +++++-- tests/daemon-reconcile.test.ts | 54 +++++++++++++++++++++ tests/daemon-systemd.test.ts | 86 +++++++++++++++++++++++++++++++++ tests/launchd-isolation.test.ts | 16 ++++++ 7 files changed, 259 insertions(+), 14 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 48428f2fc..cb921960e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -617,6 +617,9 @@ pid runs in the unit's cgroup is not enough. This home's hackd may have been started by a job of another home's daemon, or a reload may have pointed the configuration at this home while the running daemon serves another. Without the proof, `start` exits 1 with `blocked: "unproven-owner"` and changes nothing. +The final proof uses a fresh read of the unit's state. If that read fails or +systemd is unavailable, the restart is refused the same way; an earlier read +is never reused, because a failed read reports no main process. When an earlier repair wrote the unit but its `daemon-reload` failed, the next `start` finishes that reload (and checks the unit's home again) before it restarts. If the reload fails again, the restart is not attempted and the @@ -648,7 +651,10 @@ prove the service is this CLI's: off the user service: hackd is started directly, as before; - `HACK_DAEMON_SERVICE_READ_ONLY=1` (`hack doctor --fix` without a TTY): the unit is never written or reloaded. A hackd of another version under this - CLI's unit is still restarted, which does not change the unit. + CLI's unit is still restarted, which does not change the unit. A pending + reload or restart stays pending and is reported (`blocked: "read-only"`), but + on its own it never restarts a hackd that already runs this version, so + repeated read-only starts do not keep restarting it. On macOS, Hack reads the plist only at its own LaunchAgents path for its label (`dance.hack.hackd`). launchd has no drop-ins, so the systemd header rule does @@ -665,9 +671,11 @@ count. Some plists cannot be read: - the text is not a property list, or its root is not a dict; - it has CDATA, an unterminated comment, a malformed or unbalanced tag, or a value type the reader does not know; -- a root key repeats; -- the program value is not a plain string, uses an XML escape, or is a relative - path. +- a root key repeats, or uses an XML entity or character reference (launchd + reads `Program` as `Program`). Keys are compared exactly as written, so + ` Program ` with spaces is another key; +- the program value is not a plain string, uses an XML escape, or is not an + absolute path exactly as written (it is not trimmed). Such a plist is never replaced automatically; `start` reports it and `hack daemon install` replaces it. diff --git a/src/commands/daemon-systemd.ts b/src/commands/daemon-systemd.ts index 34cc0c967..e2a2a241d 100644 --- a/src/commands/daemon-systemd.ts +++ b/src/commands/daemon-systemd.ts @@ -336,6 +336,11 @@ export async function reconcileRunningSystemdDaemon( recordReconcile({ trace: hooks.trace, manager: "systemd", plan }); if (plan.service === "keep") { noteKeptStaleUnit({ plan }); + if (plan.blocked === "read-only") { + logger.warn({ + message: `${SYSTEMD_UNIT_NAME} has changes that are not loaded yet (a pending reload or restart); not reloading or restarting it here, and hackd already runs this version | Run: hack daemon install (or hack daemon start outside hack doctor --fix)`, + }); + } return null; } if (plan.service === "refuse") { @@ -456,6 +461,64 @@ async function refuseUnprovenRestart(opts: { return 1; } +/** + * The reload may have changed what the unit is configured for: prove + * ownership again just before the restart lands, from a fresh state read + * only. A failed read reports no main process, which is not evidence that + * none runs, so it is refused ({@link refuseUnobservedRestart}), never + * replaced by an earlier snapshot. Returns 1 (reported) or null. + */ +async function proveOwnershipBeforeRestart(opts: { + readonly hooks: Omit; + readonly unitPath: string; + readonly deps: SystemdDeps; + readonly homePid: number; +}): Promise { + const { hooks, deps } = opts; + const fresh = await readSystemdServiceStatus({ + unitPath: opts.unitPath, + deps, + }); + if (fresh === null) { + return refuseUnobservedRestart({ + hooks, + detail: "systemd could not be asked", + }); + } + if (!fresh.availability.available) { + return refuseUnobservedRestart({ + hooks, + detail: describeSystemdUnavailable(fresh.availability), + }); + } + if (fresh.stateError !== null) { + return refuseUnobservedRestart({ hooks, detail: fresh.stateError }); + } + return await refuseUnprovenRestart({ + hooks: { ...hooks, pid: opts.homePid }, + status: fresh, + deps, + }); +} + +/** + * The final ownership proof could not observe the unit's state: refused + * like an unproven owner (`blocked: "unproven-owner"`, no restart). + */ +function refuseUnobservedRestart(opts: { + readonly hooks: Pick; + readonly detail: string; +}): number { + if (opts.hooks.trace) { + opts.hooks.trace.action = "none"; + opts.hooks.trace.blocked = "unproven-owner"; + } + logger.warn({ + message: `Not restarting ${SYSTEMD_UNIT_NAME} to update it: its state could not be read again just before the restart (${opts.detail}), so whose daemon it runs cannot be proven | Run hack daemon start again; check: systemctl --user status ${SYSTEMD_UNIT_NAME}`, + }); + return 1; +} + /** The refusal for a running hackd whose version a restart cannot fix. */ function describeRunningRefusal(opts: { readonly plan: DaemonReconcilePlan; @@ -546,12 +609,11 @@ async function restartReconciledUnit(opts: { logger.error({ message: blocked }); return 1; } - // The reload may have changed what the unit is configured for: prove - // ownership again just before the restart lands. - const unproven = await refuseUnprovenRestart({ - hooks: { ...hooks, pid: opts.homePid }, - status: (await readSystemdServiceStatus({ unitPath, deps })) ?? status, + const unproven = await proveOwnershipBeforeRestart({ + hooks, + unitPath, deps, + homePid: opts.homePid, }); if (unproven !== null) { return unproven; diff --git a/src/daemon/launchd-plist.ts b/src/daemon/launchd-plist.ts index 6f5b2daa9..4a49ec29e 100644 --- a/src/daemon/launchd-plist.ts +++ b/src/daemon/launchd-plist.ts @@ -77,7 +77,8 @@ export function parseLaunchdPlistProgram(opts: { } function programPath(raw: string): LaunchdPlistProgram { - const path = raw.trim(); + // Exactly as written: launchd does not trim string values. + const path = raw; if (path.includes("&")) { return { kind: "unknown", reason: "the program path uses an XML escape" }; } @@ -218,7 +219,13 @@ function readDictBody( } const key = readScalar(tokens, i, "key"); const value = readValue(tokens, skipSpace(tokens, key.next)); - const name = key.text.trim(); + // Key identity is the exact text: launchd neither trims nor knows + // another spelling. An entity or character reference (`Program` + // is `Program` to launchd) is not decoded here, so it is unknown. + const name = key.text; + if (keep && name.includes("&")) { + throw new PlistShapeError("a root key uses an XML escape"); + } if (keep && keys.has(name)) { throw new PlistShapeError(`the key ${name} appears twice`); } diff --git a/src/daemon/reconcile.ts b/src/daemon/reconcile.ts index 7cb999e00..e3fe85718 100644 --- a/src/daemon/reconcile.ts +++ b/src/daemon/reconcile.ts @@ -204,9 +204,21 @@ function planWithoutCallerGuard( input.unit === "program-unknown" ? "unknown-program" : "other-program", }; } - const restart = - mismatch || rewrite !== "none" || input.restartPending === true; - return { rewrite, service: restart ? "restart" : "keep", reasons, blocked }; + // Under read-only a pending restart cannot load anything new (the reload + // that would is not allowed), so it alone never restarts a compatible + // daemon; that would repeat on every start. It stays reported. + const pendingRestart = input.restartPending === true && !input.readOnly; + const restart = mismatch || rewrite !== "none" || pendingRestart; + const pendingBlocked = + input.restartPending === true && input.readOnly && !restart + ? "read-only" + : null; + return { + rewrite, + service: restart ? "restart" : "keep", + reasons, + blocked: blocked ?? pendingBlocked, + }; } function staleReasons(opts: { diff --git a/tests/daemon-reconcile.test.ts b/tests/daemon-reconcile.test.ts index 7abaafb79..bdd6480c4 100644 --- a/tests/daemon-reconcile.test.ts +++ b/tests/daemon-reconcile.test.ts @@ -903,3 +903,57 @@ test("only ENOENT and ENOTDIR make a plist program missing", async () => { ); expect(await classify(() => Promise.resolve({}))).toBe("exists"); }); + +// Review round 4 (HACK-2216). + +test("read-only: a pending restart alone keeps a compatible daemon", () => { + expect( + planDaemonReconcile({ + ...base, + readOnly: true, + running: { version: "2.0.0" }, + restartPending: true, + }) + ).toEqual({ + rewrite: "none", + service: "keep", + reasons: ["content"], + blocked: "read-only", + }); + // A version mismatch is still restarted. + expect( + planDaemonReconcile({ + ...base, + readOnly: true, + running: { version: "1.0.0" }, + restartPending: true, + }).service + ).toBe("restart"); +}); + +test("root keys are read exactly: escapes are unknown, whitespace is another key", () => { + const escaped = ROUND3_PLIST( + " Program\n /opt/other/hack" + ); + expect(parseLaunchdPlistProgram({ plistText: escaped }).kind).toBe("unknown"); + const named = ROUND3_PLIST( + " ProgramArguments\n /opt/other/hack" + ); + expect(parseLaunchdPlistProgram({ plistText: named }).kind).toBe("unknown"); + // ` Program ` is not `Program` to launchd: ProgramArguments decides. + const spaced = ROUND3_PLIST( + ` Program \n /missing/path\n${ROUND3_ARGS("/opt/other/hack")}` + ); + expect(parseLaunchdPlistProgram({ plistText: spaced })).toEqual({ + kind: "path", + path: "/opt/other/hack", + }); + // Values are not trimmed either. + expect( + parseLaunchdPlistProgram({ + plistText: ROUND3_PLIST( + " Program\n /opt/other/hack" + ), + }).kind + ).toBe("unknown"); +}); diff --git a/tests/daemon-systemd.test.ts b/tests/daemon-systemd.test.ts index 89f4bd9c1..240c52f09 100644 --- a/tests/daemon-systemd.test.ts +++ b/tests/daemon-systemd.test.ts @@ -4840,3 +4840,89 @@ linuxTest( expect(await Bun.file(marker).exists()).toBe(false); } ); + +// Review round 4 (HACK-2216). + +linuxTest( + "a failed fresh state read before the restart refuses it, without falling back", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + await installSystemdUnit({ + unitPath, + unitText: unitText(), + deps: fake.deps, + }); + fake.calls.length = 0; + let stateReads = 0; + const run = fake.deps.run; + fake.deps = { + ...fake.deps, + run: (command, args) => { + if (args.some((arg) => arg.includes("MainPID"))) { + stateReads += 1; + // The first read (the plan) works; the final re-read fails. + if (stateReads >= 2) { + return Promise.resolve({ + exitCode: 1, + stdout: "", + stderr: "Failed to get properties: Connection timed out", + }); + } + } + return run(command, args); + }, + }; + const hooks = runningHooks(fake, "1.0.0"); + expect(await reconcileRunningSystemdDaemon(hooks)).toBe(1); + expect(stateReads).toBe(2); + expect(hooks.trace.blocked).toBe("unproven-owner"); + expect(hooks.trace.action).toBe("none"); + // The home queries still answered; the restart was refused anyway. + expect(fake.calls.some((call) => call.includes("Environment"))).toBe(true); + expect(fake.calls).not.toContain("systemctl --user restart hackd.service"); + } +); + +linuxTest( + "read-only: a pending reload does not restart a daemon that already runs this version", + async () => { + const fake = fakeSystemd({ active: true, mainPid: 4242 }); + fake.deps = { + ...fake.deps, + env: { ...fake.deps.env, [SYSTEMD_READ_ONLY_ENV]: "1" }, + }; + await installSystemdUnit({ + unitPath, + unitText: unitText(), + deps: fake.deps, + }); + fake.needsReload = true; + const marker = join( + root, + ".hack", + "daemon", + "hackd.service.restart-pending" + ); + + // First start: another version runs, so it is restarted (without a + // reload, which read-only forbids); the pending state is kept. + fake.calls.length = 0; + expect( + await reconcileRunningSystemdDaemon(runningHooks(fake, "1.0.0")) + ).toBe(0); + expect(fake.calls).toContain("systemctl --user restart hackd.service"); + expect(fake.calls).not.toContain("systemctl --user daemon-reload"); + expect(await Bun.file(marker).exists()).toBe(true); + + // Second start: the daemon now runs this version. The marker alone + // does not restart it again. + fake.calls.length = 0; + const second = runningHooks(fake, "2.0.0"); + expect(await reconcileRunningSystemdDaemon(second)).toBeNull(); + expect(fake.calls).not.toContain("systemctl --user restart hackd.service"); + expect(fake.calls).not.toContain("systemctl --user daemon-reload"); + expect(second.trace.action).toBe("none"); + expect(second.trace.blocked).toBe("read-only"); + expect(await Bun.file(marker).exists()).toBe(true); + } +); diff --git a/tests/launchd-isolation.test.ts b/tests/launchd-isolation.test.ts index 277f61ea6..a9026ea32 100644 --- a/tests/launchd-isolation.test.ts +++ b/tests/launchd-isolation.test.ts @@ -598,6 +598,22 @@ test("a commented-out Program key does not make a foreign top-level program miss expect(verbs).toEqual([]); }); +test("an XML-escaped Program key pointing at another hack is never replaced", async () => { + let written = ""; + const { results, after, verbs } = await repairPlist((fixture) => { + // `Program` is `Program` to launchd; no ProgramArguments, so a + // reader that missed it would see no program and retarget. + written = `\n\n\n Label\n dance.hack.hackd\n Program\n ${otherHack(fixture)}\n\n\n`; + return written; + }); + expect(results).toEqual({ + repair: "unknown-program", + secondRepair: "unknown-program", + }); + expect(after).toBe(written); + expect(verbs).toEqual([]); +}); + test.skipIf(process.getuid?.() === 0)( "a program path that cannot be checked (EACCES) is never replaced", async () => { From 3f5ebed22e20d9e588c83f7d5b88ec91dabf95d1 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 20:47:26 +0000 Subject: [PATCH 08/43] fix(daemon): keep reconcile-only out of replacement daemons and prove launchd job ownership Codex findings on HACK-2216: - spawnDetachedDaemon now drops HACK_DAEMON_RECONCILE_ONLY (next to HACK_DAEMON_SERVICE_READ_ONLY) via detachedDaemonEnvironment, and a --foreground start ignores the flag, so a daemon a post-update reconcile replaces actually runs. - Before any automatic launchd refresh or retarget, the plist must not bind another home (HACK_HOME, or a HOME other than the one Hack writes): blocked home-binding. A running job's pid must be proven this home's hackd (proveOwnedDaemonProcess); a running job without one is unproven: blocked unproven-owner. Start refuses to kickstart such a job. Co-Authored-By: Claude Opus 5.5 --- docs/architecture.md | 13 ++- src/commands/daemon-launchd.ts | 23 +++++ src/commands/daemon.ts | 4 + src/daemon/launchd-plist.ts | 62 +++++++++++- src/daemon/launchd.ts | 164 +++++++++++++++++++++++++------ src/daemon/spawn.ts | 31 +++++- tests/daemon-reconcile.test.ts | 80 +++++++++++++++ tests/daemon-systemd-cli.test.ts | 56 +++++++++++ tests/launchd-isolation.test.ts | 87 ++++++++++++++-- 9 files changed, 470 insertions(+), 50 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index cb921960e..0d12d0100 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -697,6 +697,14 @@ Such a plist is never replaced automatically; `start` reports it and - When a needed rewrite does not complete (unloading, writing or loading the plist fails), `start` exits 1 with `E_DAEMON_SERVICE` instead of reporting hackd as running: the unload may already have stopped it. +- Before any automatic rewrite (a refresh or a retarget), the loaded job must be + this home's. If the plist's `EnvironmentVariables` sets `HACK_HOME`, or a + `HOME` other than the one Hack writes for this home, it is left alone + (`blocked: "home-binding"`). If the job is running, its pid must be proven to + be this home's hackd, holding a socket in this home's `daemon/` directory + (the proof `stop` uses). Otherwise it is left alone + (`blocked: "unproven-owner"`). A running job without a pid counts as + unproven. ```mermaid stateDiagram-v2 @@ -734,7 +742,10 @@ and for this home's hackd. A live pid is not enough: hackd must answer on this home's socket. When both are found, it runs the new executable's `hack daemon start --json` in reconcile-only mode (`HACK_DAEMON_RECONCILE_ONLY=1`). In that mode `start` does nothing (action `none`) when no hackd of this home is -running, so a daemon that stopped in the meantime is not started. The step never +running, so a daemon that stopped in the meantime is not started. The flag +applies to that one invocation only. A daemon it starts to replace an +incompatible one never inherits it, and a `--foreground` start ignores it, so +the replacement runs normally. The step never fails the update, including when it cannot read the service state. A failure is reported in the update result (`daemon.status: "failed"`) with `hack daemon install` as the next step. A stopped service picks up the new diff --git a/src/commands/daemon-launchd.ts b/src/commands/daemon-launchd.ts index 0c0278793..eecaccf79 100644 --- a/src/commands/daemon-launchd.ts +++ b/src/commands/daemon-launchd.ts @@ -198,6 +198,15 @@ export async function startViaLaunchdIfLoaded( // not own. return await refuseForeignLaunchdProgram({ ...hooks, verb: "start" }); } + if (repair === "home-binding" || repair === "unproven-owner") { + // The loaded job is not provably this home's: kickstarting it would + // (re)start another home's daemon. + logger.error({ + message: + "Not starting hackd through launchd: the loaded launchd job is not proven to serve this Hack home; nothing was changed | Check: hack daemon status; to take the service over for this home, run: hack daemon install", + }); + return 1; + } trace.action = rewritten ? "reinstalled" : "started"; // launchd owns the daemon: never spawn a bare process next to it — // two process managers for one socket is how machines end up with @@ -255,6 +264,20 @@ function noteRepair(opts: { "The launchd service's program cannot be read (Program/ProgramArguments); it is left as it is | To replace it, run: hack daemon install", }); return false; + case "home-binding": + trace.blocked = "home-binding"; + logger.warn({ + message: + "The launchd plist binds another Hack home (HACK_HOME or HOME in EnvironmentVariables), so it is not rewritten from here | To take the service over for this home, run: hack daemon install", + }); + return false; + case "unproven-owner": + trace.blocked = "unproven-owner"; + logger.warn({ + message: + "The loaded launchd job is not proven to be this Hack home's hackd, so it is not rewritten or restarted from here | Check: hack daemon status", + }); + return false; case "unmanaged": trace.stale.add("content"); trace.blocked = "unmanaged"; diff --git a/src/commands/daemon.ts b/src/commands/daemon.ts index 1dbcd4467..dfc3fda7c 100644 --- a/src/commands/daemon.ts +++ b/src/commands/daemon.ts @@ -282,7 +282,11 @@ async function handleDaemonStart({ paths, foreground: args.options.foreground === true, trace, + // A foreground start is a daemon being run (by a manager, or as the + // replacement a reconcile spawned), never a reconcile: the flag + // must not stop it from running. reconcileOnly: + args.options.foreground !== true && (process.env[DAEMON_RECONCILE_ONLY_ENV] ?? "").trim() === "1", }), }); diff --git a/src/daemon/launchd-plist.ts b/src/daemon/launchd-plist.ts index 4a49ec29e..ef5b9f1b6 100644 --- a/src/daemon/launchd-plist.ts +++ b/src/daemon/launchd-plist.ts @@ -23,6 +23,10 @@ type Token = type PlistValue = | { readonly kind: "string"; readonly text: string } | { readonly kind: "array"; readonly first: PlistValue | null } + | { + readonly kind: "dict"; + readonly keys: ReadonlyMap; + } | { readonly kind: "other" }; const PROLOG = /^\s*(<\?xml[^?]*\?>)?\s*([]*>)?/; @@ -76,6 +80,47 @@ export function parseLaunchdPlistProgram(opts: { }; } +/** + * The plist's root `EnvironmentVariables` (HACK-2216), for the home + * binding check before an automatic rewrite: `env` maps each name to its + * string value (null for a non-string value); an absent dict is empty. + * `unknown` when the plist cannot be read ({@link parseLaunchdPlistProgram}) + * or `EnvironmentVariables` is not a dict. + */ +export function parseLaunchdPlistEnvironment(opts: { + readonly plistText: string; +}): + | { + readonly kind: "env"; + readonly env: ReadonlyMap; + } + | { readonly kind: "unknown"; readonly reason: string } { + let root: Map | null; + try { + root = readRootDict(opts.plistText); + } catch (error: unknown) { + return { + kind: "unknown", + reason: error instanceof PlistShapeError ? error.message : "unreadable", + }; + } + const value = root?.get("EnvironmentVariables"); + if (value === undefined) { + return { kind: "env", env: new Map() }; + } + if (value.kind !== "dict") { + return { + kind: "unknown", + reason: "EnvironmentVariables is not a dict", + }; + } + const env = new Map(); + for (const [name, entry] of value.keys) { + env.set(name, entry.kind === "string" ? entry.text : null); + } + return { kind: "env", env }; +} + function programPath(raw: string): LaunchdPlistProgram { // Exactly as written: launchd does not trim string values. const path = raw; @@ -198,8 +243,9 @@ function expectEnd(tokens: readonly Token[], i: number): void { } /** - * Reads ``/value pairs up to ``. Only the root dict keeps its - * keys; a nested dict is read for shape and discarded. + * Reads ``/value pairs up to ``. Keys are only ever looked up + * in the dict a caller asks for (Program in the root, HACK_HOME in the + * root's EnvironmentVariables), never searched across nesting. */ function readDictBody( tokens: readonly Token[], @@ -224,7 +270,7 @@ function readDictBody( // is `Program` to launchd) is not decoded here, so it is unknown. const name = key.text; if (keep && name.includes("&")) { - throw new PlistShapeError("a root key uses an XML escape"); + throw new PlistShapeError("a key uses an XML escape"); } if (keep && keys.has(name)) { throw new PlistShapeError(`the key ${name} appears twice`); @@ -258,6 +304,9 @@ function emptyValue(name: string): PlistValue { if (name === "array") { return { kind: "array", first: null }; } + if (name === "dict") { + return { kind: "dict", keys: new Map() }; + } return { kind: "other" }; } @@ -272,9 +321,12 @@ function readValue(tokens: readonly Token[], start: number): ValueRead { throw new PlistShapeError("a dict key has no value"); } if (token.name === "dict") { + // Nested dicts are kept (EnvironmentVariables binds the service's home) + // and held to the same rules: an escaped or repeated key is unknown. + const nested = readDictBody(tokens, start + 1, true); return { - value: { kind: "other" }, - next: readDictBody(tokens, start + 1, false).next, + value: { kind: "dict", keys: nested.keys }, + next: nested.next, }; } if (token.name === "array") { diff --git a/src/daemon/launchd.ts b/src/daemon/launchd.ts index 8cedaae30..93605f567 100644 --- a/src/daemon/launchd.ts +++ b/src/daemon/launchd.ts @@ -15,8 +15,12 @@ import { resolveHackInvocation } from "../lib/hack-cli.ts"; import { normalizeHomeDir, resolveUserHomeDir } from "../lib/home-dir.ts"; import { boundedTimeoutMs } from "../lib/process-deadline.ts"; import { findExecutableInPath } from "../lib/shell.ts"; -import { parseLaunchdPlistProgram } from "./launchd-plist.ts"; +import { + parseLaunchdPlistEnvironment, + parseLaunchdPlistProgram, +} from "./launchd-plist.ts"; import type { DaemonPaths } from "./paths.ts"; +import { type DaemonProcessProof, proveOwnedDaemonProcess } from "./process.ts"; // biome-ignore lint/performance/noBarrelFile: keeps launchd's public surface in one module export { @@ -491,6 +495,11 @@ export function hasBoundedLaunchdOutput(opts: { * but is not Hack's ({@link isHackManagedPlist}: no marker and another * `Label`), so it was left as it is; * `unknown-program`: its program cannot be read, left as it is; + * `home-binding`: a rewrite was needed, but the plist binds another Hack + * home (`HACK_HOME`, or a `HOME` the renderer would not write), left as it + * is ({@link plistKeepsThisHome}); + * `unproven-owner`: a rewrite was needed, but the loaded job is not proven + * to be this home's ({@link proveLaunchdJobOwnership}), left as it is; * `failed`: a rewrite was needed but did not complete (unload, write or * load failed, or no installable hack resolves); the service may be * stopped. @@ -501,6 +510,8 @@ export type LaunchdProgramRepairResult = | "refreshed" | "other-program" | "unknown-program" + | "home-binding" + | "unproven-owner" | "unmanaged" | "not-installed" | "failed"; @@ -592,6 +603,55 @@ export async function readLaunchdProgramState(opts: { return { kind: ours ? "ours" : "other", program, resolved }; } +/** + * Whether a rewrite would keep the plist's home binding: its + * `EnvironmentVariables` sets no `HACK_HOME` (the renderer writes none) and + * its `HOME`, if set, is the home the renderer writes. An unreadable + * environment is not proof (false). + */ +export function plistKeepsThisHome(opts: { + readonly plistText: string; + readonly home?: string; +}): boolean { + const parsed = parseLaunchdPlistEnvironment({ plistText: opts.plistText }); + if (parsed.kind !== "env") { + return false; + } + if (parsed.env.has("HACK_HOME")) { + return false; + } + const home = parsed.env.get("HOME"); + return home === undefined || home === (opts.home ?? resolveUserHomeDir()); +} + +/** + * Whether the loaded job may be unloaded as this home's: it runs nothing + * (not running and no pid), or its pid is proven to be this home's hackd + * ({@link proveOwnedDaemonProcess}: a hackd holding a socket in this home's + * daemon directory). A running job without a pid, or one that fails the + * proof, is not this home's (false). + */ +export async function proveLaunchdJobOwnership(opts: { + readonly paths: DaemonPaths; + readonly status?: LaunchdServiceStatus; + readonly prove?: (pid: number) => Promise; +}): Promise { + const status = + opts.status ?? (await getLaunchdServiceStatus({ paths: opts.paths })); + if (!status.loaded) { + return true; + } + if (status.pid === null || status.pid <= 1) { + return !status.running; + } + const proof = await ( + opts.prove ?? + ((pid: number) => + proveOwnedDaemonProcess({ pid, daemonRoot: opts.paths.root })) + )(status.pid); + return proof.kind === "owned"; +} + /** * Repairs an installed launchd plist (HACK-2216): * - a program that is gone or a compiled-binary virtual path (/$bunfs) is @@ -612,6 +672,8 @@ export async function readLaunchdProgramState(opts: { */ export async function repairLaunchdProgramIfInvalid(opts: { readonly paths: DaemonPaths; + /** Defaults to {@link proveLaunchdJobOwnership}; tests pin it. */ + readonly proveJob?: () => Promise; }): Promise { if (!resolveLaunchdAccess().allowed) { return "not-installed"; @@ -620,39 +682,22 @@ export async function repairLaunchdProgramIfInvalid(opts: { if (plistText === null) { return "not-installed"; } - const state = await readLaunchdProgramState({ - paths: opts.paths, - plistText, - }); - if (state.kind === "not-installed") { - return "not-installed"; - } - if (state.kind === "other") { - return "other-program"; + const decided = await decideLaunchdRewrite({ paths: opts.paths, plistText }); + if (decided.kind === "result") { + return decided.result; } - if (state.kind === "unknown") { - return "unknown-program"; + const { program } = decided; + // A rewrite unloads the loaded job and replaces its plist: it must be + // this home's (HACK-2216). The plist must not bind another home, and a + // running job must be this home's proven hackd. Unknown refuses. + if (!plistKeepsThisHome({ plistText })) { + return "home-binding"; } - if (state.kind === "missing" && state.resolved === null) { - return "failed"; - } - const program = - state.kind === "ours" && state.program !== null ? state.program : null; - if (program !== null) { - const current = isLaunchdPlistCurrent({ - plistText, - hackBinPath: program, - home: resolveUserHomeDir(), - }); - if (current) { - return "ok"; - } - if ( - hasBoundedLaunchdOutput({ plistText }) && - !isHackManagedPlist({ plistText }) - ) { - return "unmanaged"; - } + const proven = await ( + opts.proveJob ?? (() => proveLaunchdJobOwnership({ paths: opts.paths })) + )(); + if (!proven) { + return "unproven-owner"; } const result = await installLaunchdService({ paths: opts.paths, @@ -669,6 +714,61 @@ export async function repairLaunchdProgramIfInvalid(opts: { return program === null ? "repaired" : "refreshed"; } +/** + * Whether the plist needs a rewrite, before any ownership check: a final + * `result` (nothing to rewrite, or a reason not to), or `rewrite` with the + * program to keep (null: retarget to this CLI). + */ +async function decideLaunchdRewrite(opts: { + readonly paths: DaemonPaths; + readonly plistText: string; +}): Promise< + | { readonly kind: "result"; readonly result: LaunchdProgramRepairResult } + | { readonly kind: "rewrite"; readonly program: string | null } +> { + const { plistText } = opts; + const state = await readLaunchdProgramState({ + paths: opts.paths, + plistText, + }); + const final = (result: LaunchdProgramRepairResult) => + ({ kind: "result", result }) as const; + switch (state.kind) { + case "not-installed": + return final("not-installed"); + case "other": + return final("other-program"); + case "unknown": + return final("unknown-program"); + case "missing": + return state.resolved === null + ? final("failed") + : { kind: "rewrite", program: null }; + default: + break; + } + const program = state.program; + if (program === null) { + return { kind: "rewrite", program: null }; + } + if ( + isLaunchdPlistCurrent({ + plistText, + hackBinPath: program, + home: resolveUserHomeDir(), + }) + ) { + return final("ok"); + } + if ( + hasBoundedLaunchdOutput({ plistText }) && + !isHackManagedPlist({ plistText }) + ) { + return final("unmanaged"); + } + return { kind: "rewrite", program }; +} + export interface LaunchdUninstallResult { readonly ok: boolean; readonly error?: string; diff --git a/src/daemon/spawn.ts b/src/daemon/spawn.ts index 14e99bc83..30d8de74f 100644 --- a/src/daemon/spawn.ts +++ b/src/daemon/spawn.ts @@ -1,4 +1,29 @@ import { hackReexecEnvironment } from "../lib/child-environment.ts"; + +/** + * Flags that scope one `hack daemon` invocation and must not reach the + * daemon it starts (or anything that daemon starts): + * - `HACK_DAEMON_SERVICE_READ_ONLY`: `hack doctor --fix` sets it for that + * one repair; + * - `HACK_DAEMON_RECONCILE_ONLY`: `hack update` sets it for the new + * executable's `daemon start --json`, which never starts a stopped + * daemon. A daemon it replaces must start normally (HACK-2216). + */ +const INVOCATION_ONLY_ENV_KEYS = [ + "HACK_DAEMON_SERVICE_READ_ONLY", + "HACK_DAEMON_RECONCILE_ONLY", +] as const; + +/** The environment for a detached hackd: the caller's, minus those flags. */ +export function detachedDaemonEnvironment( + base: Readonly> +): Record { + const env: Record = { ...base }; + for (const key of INVOCATION_ONLY_ENV_KEYS) { + Reflect.deleteProperty(env, key); + } + return env; +} /** * Spawns a hackd launcher that must outlive its caller. * @@ -18,12 +43,8 @@ import { hackReexecEnvironment } from "../lib/child-environment.ts"; export function spawnDetachedDaemon(opts: { readonly argv: readonly string[]; }): { readonly pid: number } { - // `hack doctor --fix` may run this with the read-only service flag; it is - // for that one repair, not for the daemon and everything it starts. - const env: Record = { ...process.env }; - Reflect.deleteProperty(env, "HACK_DAEMON_SERVICE_READ_ONLY"); const proc = Bun.spawn([...opts.argv], { - env: hackReexecEnvironment(env), + env: hackReexecEnvironment(detachedDaemonEnvironment(process.env)), stdin: "ignore", stdout: "ignore", stderr: "ignore", diff --git a/tests/daemon-reconcile.test.ts b/tests/daemon-reconcile.test.ts index bdd6480c4..9f88cf3e7 100644 --- a/tests/daemon-reconcile.test.ts +++ b/tests/daemon-reconcile.test.ts @@ -27,9 +27,12 @@ import { isSameExecutableFile, LAUNCHD_MANAGED_MARKER, parseLaunchdPlistProgram, + plistKeepsThisHome, + proveLaunchdJobOwnership, renderLaunchdPlist, } from "../src/daemon/launchd.ts"; import { resolveDaemonPaths } from "../src/daemon/paths.ts"; +import type { DaemonProcessProof } from "../src/daemon/process.ts"; import { hasAncestorProcess } from "../src/daemon/process-ancestry.ts"; import { buildDaemonCommandData, @@ -38,6 +41,7 @@ import { planDaemonReconcile, reconcileAction, } from "../src/daemon/reconcile.ts"; +import { detachedDaemonEnvironment } from "../src/daemon/spawn.ts"; import { findDroppedHomeBinding, isHackManagedUnit, @@ -957,3 +961,79 @@ test("root keys are read exactly: escapes are unknown, whitespace is another key }).kind ).toBe("unknown"); }); + +// Codex findings on PR #854 (HACK-2216). + +test("a replacement daemon's environment drops the invocation-only flags", () => { + const env = detachedDaemonEnvironment({ + PATH: "/usr/bin", + HACK_DAEMON_RECONCILE_ONLY: "1", + HACK_DAEMON_SERVICE_READ_ONLY: "1", + HACK_HOME: "/srv/u/.hack", + }); + expect(env).toEqual({ PATH: "/usr/bin", HACK_HOME: "/srv/u/.hack" }); + expect("HACK_DAEMON_RECONCILE_ONLY" in env).toBe(false); +}); + +test("a plist keeps this home only without HACK_HOME and with this HOME", () => { + const plist = (env: string) => + `\n\n\n Label\n dance.hack.hackd\n EnvironmentVariables\n \n${env}\n \n\n\n`; + const home = "/Users/example"; + const keeps = (text: string) => plistKeepsThisHome({ plistText: text, home }); + expect( + keeps(plist(` HOME\n ${home}`)) + ).toBe(true); + expect(keeps(plist(""))).toBe(true); + expect( + keeps( + plist( + ` HOME\n ${home}\n HACK_HOME\n ${home}/.hack` + ) + ) + ).toBe(false); + expect( + keeps(plist(" HOME\n /srv/other-home")) + ).toBe(false); + // An escaped key inside the environment is unreadable: not proof. + expect( + keeps(plist(" HACK_HOME\n /x")) + ).toBe(false); +}); + +test("a loaded launchd job is this home's only when it runs nothing or its pid is proven", async () => { + const paths = resolveDaemonPaths({ home: "/nonexistent-home" }); + const status = (running: boolean, pid: number | null) => ({ + installed: true, + loaded: true, + running, + pid, + exitStatus: null, + }); + const owned = () => + Promise.resolve({ kind: "owned" } as unknown as DaemonProcessProof); + const refused = () => + Promise.resolve({ + kind: "refused", + reason: "foreign-socket", + } as DaemonProcessProof); + expect( + await proveLaunchdJobOwnership({ paths, status: status(false, null) }) + ).toBe(true); + expect( + await proveLaunchdJobOwnership({ paths, status: status(true, null) }) + ).toBe(false); + expect( + await proveLaunchdJobOwnership({ + paths, + status: status(true, 4242), + prove: owned, + }) + ).toBe(true); + expect( + await proveLaunchdJobOwnership({ + paths, + status: status(true, 4242), + prove: refused, + }) + ).toBe(false); +}); diff --git a/tests/daemon-systemd-cli.test.ts b/tests/daemon-systemd-cli.test.ts index 036fde78c..559cd1288 100644 --- a/tests/daemon-systemd-cli.test.ts +++ b/tests/daemon-systemd-cli.test.ts @@ -530,6 +530,62 @@ installFlowTest( 90_000 ); +test.skipIf(process.platform !== "linux")( + "a replacement daemon started under reconcile-only mode still runs (HACK-2216)", + async () => { + fixture = await createFixture(); + const f = fixture; + const home = join(f.root, "isolated"); + const env = { + ...f.env, + HACK_HOME: home, + HACK_DAEMON_RECONCILE_ONLY: "1", + }; + // The foreground run is what a replacement spawn executes: even if the + // flag leaked into it, it must run the daemon, not exit as a no-op. + const daemon = Bun.spawn([f.hackBin, "daemon", "start", "--foreground"], { + env, + cwd: f.root, + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + }); + try { + let running = false; + for (let i = 0; i < 100 && !running; i++) { + const status = await hack({ ...f, env }, [ + "daemon", + "status", + "--json", + ]); + running = + (JSON.parse(status.stdout) as { status?: string }).status === + "running"; + if (!running) { + await Bun.sleep(100); + } + } + expect(running).toBe(true); + // The reconcile-only start itself sees it running and changes nothing. + const reconcile = await hack({ ...f, env }, [ + "daemon", + "start", + "--json", + ]); + expect(JSON.parse(reconcile.stdout)).toMatchObject({ + ok: true, + data: { action: "none", daemon: { running: true, compatible: true } }, + }); + } finally { + const { HACK_DAEMON_RECONCILE_ONLY: _drop, ...plain } = env; + await hack({ ...f, env: plain }, ["daemon", "stop"]); + daemon.kill(); + await daemon.exited; + } + }, + 90_000 +); + test.skipIf(process.platform !== "linux")( "--json with an isolated HACK_HOME: stop reports a no-op, --foreground is a usage error", async () => { diff --git a/tests/launchd-isolation.test.ts b/tests/launchd-isolation.test.ts index a9026ea32..45a785269 100644 --- a/tests/launchd-isolation.test.ts +++ b/tests/launchd-isolation.test.ts @@ -107,6 +107,8 @@ async function makeFixture(): Promise { `printf '%s\\n' "$*" >> "${launchctlLog}"`, // HACK_TEST_LAUNCHCTL_FAIL= makes that verb fail (HACK-2216). 'if [ -n "$HACK_TEST_LAUNCHCTL_FAIL" ] && [ "$1" = "$HACK_TEST_LAUNCHCTL_FAIL" ]; then echo "simulated $1 failure" >&2; exit 5; fi', + // HACK_TEST_LAUNCHCTL_JOB=stopped: the job is loaded but runs nothing. + `if [ "$1" = "print" ] && [ "$HACK_TEST_LAUNCHCTL_JOB" = "stopped" ]; then printf 'state = not running\\n'; exit 0; fi`, `if [ "$1" = "print" ]; then printf 'state = running\\npid = 4242\\n'; fi`, `if [ "$1" = "kickstart" ]; then ${touchSocket}; fi`, "exit 0", @@ -338,7 +340,10 @@ test("repairs legacy output paths even when the installed program is still curre const run = await runAsDarwin({ argv: [DRIVER, "repair-output"], cwd: fixture.root, - env: fixture.env({ HACK_HOME: join(fixture.home, ".hack") }), + env: fixture.env({ + HACK_HOME: join(fixture.home, ".hack"), + HACK_TEST_LAUNCHCTL_JOB: "stopped", + }), }); expect(run.code).toBe(0); expect(JSON.parse(run.stdout)).toEqual({ @@ -349,7 +354,8 @@ test("repairs legacy output paths even when the installed program is still curre const verbs = (await launchctlCalls(fixture)).map( (line) => line.split(" ")[0] ); - expect(verbs).toEqual(["bootout", "bootstrap"]); + // `print`: the job's state is read before a rewrite (HACK-2216). + expect(verbs).toEqual(["print", "bootout", "bootstrap"]); }); /** Runs the driver's two repairs against `plist`; returns results and calls. */ @@ -366,7 +372,13 @@ async function repairPlist( const run = await runAsDarwin({ argv: [DRIVER, "repair-output"], cwd: fixture.root, - env: fixture.env({ HACK_HOME: join(fixture.home, ".hack"), ...extraEnv }), + // A loaded job that runs nothing unless a test says otherwise: a + // running job must first be proven this home's (HACK-2216). + env: fixture.env({ + HACK_HOME: join(fixture.home, ".hack"), + HACK_TEST_LAUNCHCTL_JOB: "stopped", + ...extraEnv, + }), }); expect(run.code).toBe(0); return { @@ -429,7 +441,7 @@ test("an unmarked legacy plist with Hack's label and this hack's program is refr expect(results).toEqual({ repair: "refreshed", secondRepair: "ok" }); expect(after).toBe(current); expect(after).toContain(" Observe @@ -795,7 +811,7 @@ else on stdout (human lines go to stderr). Success is | `manager` | `systemd`, `launchd` or `none` (hackd runs directly) | | `action` | `none`, `started`, `restarted`, `reinstalled`, `stopped` or `installed` | | `stale`, `stale_reasons` | whether the service or daemon was stale, and why: `content`, `program-missing`, `version-mismatch` | -| `blocked` | why a stale service was left as it is, or `null`: `read-only`, `other-program`, `unknown-program`, `unmanaged`, `home-binding`, `inside-service`, `unproven-owner`, `rewrite-refused` | +| `blocked` | why a stale service was left as it is, or `null`: `read-only`, `other-program`, `unknown-program`, `unmanaged`, `home-binding`, `inside-service`, `unproven-owner`, `rewrite-refused`, `service-pending` | | `fallback` | `"direct"` when a replaced daemon was started directly because the service could not start it, else `null` | | `cli_version` | this CLI's version | | `daemon` | `running`, `pid`, `version` and `compatible` after the command | diff --git a/docs/versions/v5/reference/command-contracts.md b/docs/versions/v5/reference/command-contracts.md index d3d25947a..0105b9499 100644 --- a/docs/versions/v5/reference/command-contracts.md +++ b/docs/versions/v5/reference/command-contracts.md @@ -332,7 +332,7 @@ Thrown usage errors with an explicit `--json` token become E_USAGE; other thrown | usage | bare projects[], total nullable, host[], host_total nullable, runtime_ok:boolean, runtime_error nullable; details adds containers[] | failures exit 1; optional error string. No samples exits 0 with warning:no_usage_samples. Watch uses the interactive redraw path even with JSON selected; do not treat watch as a JSON report stream. | | global status | bare generated_at/caddy/logging/networks/gateway/summary | exits 0 even if summary health false; gateway is retained unsupported data, excluded from public contract | | daemon status | bare report below | running returns 0; starting/stale/unavailable may return 1 | -| daemon start/stop/restart/install | shared envelope; data schema `hack.daemon-/v1`, manager (systemd/launchd/none), action (none/started/restarted/reinstalled/stopped/installed), stale:boolean, stale_reasons (content/program-missing/version-mismatch), blocked (null/read-only/other-program/unknown-program/unmanaged/home-binding/inside-service/unproven-owner/rewrite-refused), fallback (null/direct), cli_version, daemon.{running,pid,version,compatible} | failure is `E_DAEMON_SERVICE` with the same data as detail; access refusals `E_PERMISSION`; `start --foreground --json` is `E_USAGE`; exit codes unchanged | +| daemon start/stop/restart/install | shared envelope; data schema `hack.daemon-/v1`, manager (systemd/launchd/none), action (none/started/restarted/reinstalled/stopped/installed), stale:boolean, stale_reasons (content/program-missing/version-mismatch), blocked (null/read-only/other-program/unknown-program/unmanaged/home-binding/inside-service/unproven-owner/rewrite-refused/service-pending), fallback (null/direct), cli_version, daemon.{running,pid,version,compatible} | failure is `E_DAEMON_SERVICE` with the same data as detail; access refusals `E_PERMISSION`; `start --foreground --json` is `E_USAGE`; exit codes unchanged | | host ps | bare {generatedAt,snapshotAvailable,commands,lifecycle} | timestamp string, availability boolean; nested telemetry remains specialized | | session panes/capture/tail | JSON-lines events below | no shared envelope | | update | command-owned object with ok:boolean, string error on failure | error is not shared error.{code,message}; success varies by check/install outcome; exit follows ok | diff --git a/src/commands/daemon-launchd.ts b/src/commands/daemon-launchd.ts index 1d4f73f17..ee9b2a2a6 100644 --- a/src/commands/daemon-launchd.ts +++ b/src/commands/daemon-launchd.ts @@ -37,6 +37,11 @@ export interface LaunchdStartHooks { readonly readPlist?: () => Promise; /** Defaults to {@link proveLaunchdJobOwnership}; tests pin it. */ readonly proveJob?: () => Promise; + /** + * Called before launchd may be asked to start hackd (a reload or a + * kickstart), so the caller can tell a refusal from a failed start. + */ + readonly onStartRequested?: () => void; /** Defaults to {@link kickstartLaunchdService}. */ readonly kickstart?: () => Promise<{ readonly ok: boolean; @@ -223,6 +228,9 @@ export async function startViaLaunchdIfLoaded( return null; } trace.manager = "launchd"; + // A repair may reload (bootstrap) the job, which can start it: from here + // on launchd may have been asked to start hackd. + hooks.onStartRequested?.(); const repair = await ( hooks.repair ?? (() => repairLaunchdProgramIfInvalid({ paths })) )(); diff --git a/src/commands/daemon-replace.ts b/src/commands/daemon-replace.ts index c901ab72a..23cace1c9 100644 --- a/src/commands/daemon-replace.ts +++ b/src/commands/daemon-replace.ts @@ -1,50 +1,105 @@ import type { DaemonCommandTrace } from "../daemon/reconcile.ts"; import { logger } from "../ui/logger.ts"; +/** + * How `start` tried to start hackd once none of this home's ran, reported + * by the start itself (never inferred from the `--json` trace): + * - `service-refused`: the service path refused before asking the manager + * to start anything (an untrusted unit, an unreadable home, a refused + * rewrite...); + * - `service-attempted`: the manager was asked to start (or load) hackd; + * - `direct-attempted`: hackd was started directly, or another starter + * owns the start (no service involved, nothing to fall back to). + * `manager` names the service involved, null when none was. + */ +export interface ReplacementStartOutcome { + readonly kind: "service-refused" | "service-attempted" | "direct-attempted"; + readonly exitCode: number; + readonly manager: "systemd" | "launchd" | null; +} + +/** + * Whether the service manager will not start hackd on its own any more: + * `idle` only on positive evidence (nothing queued, no restart or KeepAlive + * retry pending); `pending` when it may still start it; `unknown` when it + * cannot be told. Only `idle` allows a direct start next to it. + */ +export type ServiceManagerState = "idle" | "pending" | "unknown"; + /** * `start` has stopped this home's incompatible, running hackd to replace * it (HACK-2216). Starts the replacement through the service manager, and - * when that is refused or fails for any reason after the stop, starts this - * home's hackd directly instead (the detached start used when the unit - * serves another home), so a replacement never leaves hackd stopped. + * when that is refused, fails, or throws after the stop, starts this home's + * hackd directly instead (the detached start used when the unit serves + * another home), so a replacement does not leave hackd stopped. * - * The pre-stop checks refuse what they can foresee; this covers what they - * cannot (runtime failures, changes between the check and the start). The - * direct start runs only when the service path was used (`trace.manager` - * is not `none`; otherwise it was already the direct start) and no hackd - * of this home answers now (the service may have started one that is - * still getting ready). The daemon was running before the stop, so this - * is allowed in the post-update reconcile-only mode too. + * The direct start runs only when all of these hold: + * - the service path was involved (`outcome.kind` is not + * `direct-attempted`: a direct start, or another starter, already ran); + * - no hackd of this home answers now; + * - the manager is positively idle ({@link ServiceManagerState}): otherwise + * a pending start, restart timer or KeepAlive retry could start a second + * daemon next to the direct one. Then the service error stands with + * `blocked: "service-pending"`. + * The daemon was running before the stop, so this is allowed in the + * post-update reconcile-only mode too. * * Success reports action `restarted` with `fallback: "direct"` and keeps - * the service path's `blocked` reason; if the direct start also fails, - * the service path's exit code is returned. + * the service path's `blocked` reason. If the direct start also fails, the + * service's result stands: its exit code, or its error rethrown. */ export async function startReplacementDaemon(opts: { readonly trace: DaemonCommandTrace; - readonly startViaService: () => Promise; + readonly startViaService: () => Promise; readonly startDirect: () => Promise; /** Whether this home's hackd runs (pid alive and socket present) now. */ readonly isRunning: () => Promise; + /** The manager's state; `manager` null: the platform's own manager. */ + readonly managerState: ( + manager: "systemd" | "launchd" | null + ) => Promise; }): Promise { const { trace } = opts; - const code = await opts.startViaService(); - if (code === 0) { + let outcome: ReplacementStartOutcome; + let thrown: { readonly error: unknown } | null = null; + try { + outcome = await opts.startViaService(); + } catch (error: unknown) { + // A rejection after the stop is a failed service start, not a reason + // to leave hackd stopped; which manager got how far is unknown. + thrown = { error }; + outcome = { kind: "service-attempted", exitCode: 1, manager: null }; + } + if (outcome.exitCode === 0) { if (trace.action === "started") { trace.action = "restarted"; } return 0; } - if (trace.manager === "none" || (await opts.isRunning())) { - return code; + const fail = (): number => { + if (thrown) { + throw thrown.error; + } + return outcome.exitCode; + }; + if (outcome.kind === "direct-attempted" || (await opts.isRunning())) { + return fail(); + } + const state = await opts.managerState(outcome.manager); + const manager = outcome.manager ?? "service"; + if (state !== "idle") { + trace.blocked = "service-pending"; + logger.warn({ + message: `The ${manager} service could not start the replacement hackd, and it ${state === "pending" ? "may still start it (a queued start, restart or retry is pending)" : "could not be checked for a pending start"}, so hackd was not started directly next to it | Check: hack daemon status in a moment; if hackd is still stopped, run: hack daemon install`, + }); + return fail(); } - const manager = trace.manager; logger.warn({ message: `The ${manager} service could not start the replacement hackd; starting it directly so it keeps running (it stops at logout or reboot) | Run: hack daemon install`, }); const direct = await opts.startDirect(); if (direct !== 0) { - return code; + return fail(); } trace.action = "restarted"; trace.fallback = "direct"; diff --git a/src/commands/daemon-systemd.ts b/src/commands/daemon-systemd.ts index 186c5214b..eb823c4fe 100644 --- a/src/commands/daemon-systemd.ts +++ b/src/commands/daemon-systemd.ts @@ -124,6 +124,11 @@ export interface SystemdCommandHooks { readonly probe?: DaemonProcessProbe; /** Test seam: a process's parent pid (defaults to /proc). */ readonly parentPid?: (pid: number) => Promise; + /** + * Called just before systemd is asked to start the unit, so the caller + * can tell a refusal (nothing asked) from a failed start. + */ + readonly onStartRequested?: () => void; /** Defaults to reading `/proc/self/cgroup`; tests pin it. */ readonly readSelfCgroup?: () => Promise; /** Records what the command decided, for `--json` (HACK-2216). */ @@ -351,6 +356,7 @@ export async function startViaSystemdIfInstalled( logger.error({ message: loaded }); return 1; } + hooks.onStartRequested?.(); const started = await systemdServiceAction({ action: "start", deps }); if (!started.ok) { logger.error({ diff --git a/src/commands/daemon.ts b/src/commands/daemon.ts index 1ff388152..f807edca9 100644 --- a/src/commands/daemon.ts +++ b/src/commands/daemon.ts @@ -15,6 +15,7 @@ import { installLaunchdService, kickstartLaunchdService, type LaunchdServiceStatus, + readLaunchdManagerState, resolveLaunchdAccess, uninstallLaunchdService, } from "../daemon/launchd.ts"; @@ -51,6 +52,10 @@ import { type DaemonStatusReport, readDaemonStatus, } from "../daemon/status.ts"; +import { + defaultSystemdDeps, + readSystemdManagerState, +} from "../daemon/systemd.ts"; import { HackCliError } from "../lib/cli-result.ts"; import { updateGlobalConfig } from "../lib/config.ts"; import { readTextFile } from "../lib/fs.ts"; @@ -65,7 +70,11 @@ import { refuseForeignLaunchdProgram, startViaLaunchdIfLoaded, } from "./daemon-launchd.ts"; -import { startReplacementDaemon } from "./daemon-replace.ts"; +import { + type ReplacementStartOutcome, + type ServiceManagerState, + startReplacementDaemon, +} from "./daemon-replace.ts"; import { runDaemonCommand } from "./daemon-result.ts"; import { describeSystemdIncompatibleOwner, @@ -348,9 +357,10 @@ async function startDaemon(opts: { stalePid: pidIsDaemon ? null : livePid, }); if (!replaced || opts.foreground) { - return await startStopped(); + return (await startStopped()).exitCode; } - // A running daemon was just stopped: never leave hackd stopped. + // A running daemon was just stopped: never leave hackd stopped, unless + // the manager may still start it (see startReplacementDaemon). return await startReplacementDaemon({ trace, startViaService: startStopped, @@ -359,10 +369,32 @@ async function startDaemon(opts: { const now = await readDaemonStatus({ paths }); return now.running && now.socketExists; }, + managerState: (manager) => readServiceManagerState({ manager, paths }), }); } -/** `start` once no hackd of this home runs (or it was just replaced). */ +/** + * The service manager's state for {@link startReplacementDaemon}; `null`: + * the platform's own manager (systemd on Linux, launchd on macOS). + */ +function readServiceManagerState(opts: { + readonly manager: "systemd" | "launchd" | null; + readonly paths: DaemonPaths; +}): Promise { + const manager = + opts.manager ?? (process.platform === "darwin" ? "launchd" : "systemd"); + if (manager === "launchd") { + return readLaunchdManagerState({ paths: opts.paths }); + } + return process.platform === "linux" + ? readSystemdManagerState({ deps: defaultSystemdDeps() }) + : Promise.resolve("idle"); +} + +/** + * `start` once no hackd of this home runs (or it was just replaced). The + * outcome says how far it got, for the replacement fallback. + */ async function startStoppedDaemon(opts: { readonly paths: DaemonPaths; readonly foreground: boolean; @@ -370,14 +402,15 @@ async function startStoppedDaemon(opts: { readonly trackedPid: number | null; /** A live pid already shown not to be hackd. */ readonly stalePid: number | null; -}): Promise { +}): Promise { const { paths, trace } = opts; const sweep = await sweepOrphanDaemons({ trackedPid: opts.trackedPid, paths, }); if (!sweep.allStopped) { - return 1; + // A proven orphan still holds the socket: any start would fail. + return { kind: "direct-attempted", exitCode: 1, manager: null }; } // An installed hackd.service owns the daemon on Linux. Start through it @@ -385,13 +418,21 @@ async function startStoppedDaemon(opts: { // another starter may be writing them right now, and hackd replaces its // own socket when it starts. if (process.platform === "linux" && !opts.foreground) { + let requested = false; const viaSystemd = await startViaSystemdIfInstalled({ paths, waitForReady: (timeoutMs) => waitForDaemonStart({ paths, timeoutMs }), trace, + onStartRequested: () => { + requested = true; + }, }); if (viaSystemd !== null) { - return viaSystemd; + return { + kind: requested ? "service-attempted" : "service-refused", + exitCode: viaSystemd, + manager: "systemd", + }; } // The unit serves another home, or systemd is unusable: hackd starts // directly, whatever was noted about the unit. @@ -403,27 +444,36 @@ async function startStoppedDaemon(opts: { stalePid: opts.stalePid, }); if (raced !== null) { - return raced; + // Another starter owns this start. + return { kind: "direct-attempted", exitCode: raced, manager: null }; } if (opts.foreground) { await runDaemon({ paths, foreground: true }); - return 0; + return { kind: "direct-attempted", exitCode: 0, manager: null }; } + let requested = false; const viaLaunchd = await startViaLaunchdIfLoaded({ paths, trace, waitForReady: (timeoutMs) => waitForDaemonStart({ paths, timeoutMs }), + onStartRequested: () => { + requested = true; + }, }); if (viaLaunchd !== null) { - return viaLaunchd; + return { + kind: requested ? "service-attempted" : "service-refused", + exitCode: viaLaunchd, + manager: "launchd", + }; } const started = await startDaemonDetached({ paths }); trace.manager = "none"; trace.action = started === 0 ? "started" : trace.action; - return started; + return { kind: "direct-attempted", exitCode: started, manager: null }; } /** diff --git a/src/daemon/launchd-plist.ts b/src/daemon/launchd-plist.ts index ef5b9f1b6..4d9798398 100644 --- a/src/daemon/launchd-plist.ts +++ b/src/daemon/launchd-plist.ts @@ -27,6 +27,7 @@ type PlistValue = readonly kind: "dict"; readonly keys: ReadonlyMap; } + | { readonly kind: "bool"; readonly value: boolean } | { readonly kind: "other" }; const PROLOG = /^\s*(<\?xml[^?]*\?>)?\s*([]*>)?/; @@ -121,6 +122,42 @@ export function parseLaunchdPlistEnvironment(opts: { return { kind: "env", env }; } +/** + * Whether launchd may start the job again on its own (HACK-2216): `none` + * when the root has no `KeepAlive` (or `KeepAlive` false) and no + * `RunAtLoad` true; `retry` when `KeepAlive` is true or `RunAtLoad` is + * true; `unknown` for a conditional `KeepAlive` dict (whether it fires + * depends on state this reader does not see), any other value, or an + * unreadable plist. + */ +export function parseLaunchdPlistRetry(opts: { + readonly plistText: string; +}): "none" | "retry" | "unknown" { + let root: Map | null; + try { + root = readRootDict(opts.plistText); + } catch { + return "unknown"; + } + const keepAlive = root?.get("KeepAlive"); + const runAtLoad = root?.get("RunAtLoad"); + const flag = (value: PlistValue | undefined) => { + if (value === undefined) { + return "false" as const; + } + if (value.kind === "bool") { + return value.value ? ("true" as const) : ("false" as const); + } + return "unknown" as const; + }; + const keep = flag(keepAlive); + const load = flag(runAtLoad); + if (keep === "unknown" || load === "unknown") { + return "unknown"; + } + return keep === "true" || load === "true" ? "retry" : "none"; +} + function programPath(raw: string): LaunchdPlistProgram { // Exactly as written: launchd does not trim string values. const path = raw; @@ -307,6 +344,9 @@ function emptyValue(name: string): PlistValue { if (name === "dict") { return { kind: "dict", keys: new Map() }; } + if (name === "true" || name === "false") { + return { kind: "bool", value: name === "true" }; + } return { kind: "other" }; } diff --git a/src/daemon/launchd.ts b/src/daemon/launchd.ts index 60c079a17..02ef64fba 100644 --- a/src/daemon/launchd.ts +++ b/src/daemon/launchd.ts @@ -18,6 +18,7 @@ import { findExecutableInPath } from "../lib/shell.ts"; import { parseLaunchdPlistEnvironment, parseLaunchdPlistProgram, + parseLaunchdPlistRetry, } from "./launchd-plist.ts"; import type { DaemonPaths } from "./paths.ts"; import { type DaemonProcessProof, proveOwnedDaemonProcess } from "./process.ts"; @@ -624,6 +625,48 @@ export function plistKeepsThisHome(opts: { return home === undefined || home === (opts.home ?? resolveUserHomeDir()); } +/** + * Whether launchd will not start hackd on its own any more (HACK-2216): + * `idle` when no plist is installed (or this process may not use launchd), + * when launchd positively reports the job absent (`not-found`), or when + * the loaded job is not running and its plist asks for no retry + * ({@link parseLaunchdPlistRetry}). `pending` when it runs or will retry; + * `unknown` when the query failed, the plist is unreadable, or its + * `KeepAlive` is conditional. + */ +export async function readLaunchdManagerState(opts: { + readonly paths: DaemonPaths; + readonly status?: LaunchdServiceStatus; + readonly plistText?: string | null; +}): Promise<"idle" | "pending" | "unknown"> { + if (!resolveLaunchdAccess().allowed) { + return "idle"; + } + const status = + opts.status ?? (await getLaunchdServiceStatus({ paths: opts.paths })); + if (!status.installed) { + return "idle"; + } + if (!status.loaded) { + return status.observation === "not-found" ? "idle" : "unknown"; + } + if (status.running || status.pid !== null) { + return "pending"; + } + const plistText = + opts.plistText === undefined + ? await readTextFile(opts.paths.launchdPlistPath) + : opts.plistText; + if (plistText === null) { + return "unknown"; + } + const retry = parseLaunchdPlistRetry({ plistText }); + if (retry === "none") { + return "idle"; + } + return retry === "retry" ? "pending" : "unknown"; +} + /** * Whether the loaded job may be unloaded as this home's: launchd positively * reports it absent (`observation: "not-found"`; a failed query proves diff --git a/src/daemon/reconcile.ts b/src/daemon/reconcile.ts index 20e85d373..96f25bb96 100644 --- a/src/daemon/reconcile.ts +++ b/src/daemon/reconcile.ts @@ -78,7 +78,11 @@ export type ServiceUnitState = * - `rewrite-refused`: the unit needs a rewrite before it can start, but * the rewrite would be refused (no durable `hack` to retarget to, or a * program or service PATH that fails install's checks), so a running - * daemon was not stopped to make room for it. + * daemon was not stopped to make room for it; + * - `service-pending`: a replaced daemon's service start failed, but the + * manager may still start it (a queued job, restart timer or KeepAlive + * retry) or could not be checked, so hackd was not started directly + * next to it. */ export type DaemonReconcileBlock = | "read-only" @@ -88,7 +92,8 @@ export type DaemonReconcileBlock = | "home-binding" | "inside-service" | "unproven-owner" - | "rewrite-refused"; + | "rewrite-refused" + | "service-pending"; export interface DaemonReconcileInput { readonly unit: ServiceUnitState; diff --git a/src/daemon/systemd.ts b/src/daemon/systemd.ts index 73b440590..0edccf29c 100644 --- a/src/daemon/systemd.ts +++ b/src/daemon/systemd.ts @@ -2611,6 +2611,62 @@ function errorMessage(error: unknown): string { * drop-in can set HACK_HOME), its PATH directories and the programs it * runs are safe (a drop-in can set PATH or add Exec* entries). */ +/** + * Whether systemd will not start hackd.service on its own any more + * (HACK-2216): `idle` only when `ActiveState` is `inactive` or `failed`, + * `SubState` is not `auto-restart` (no restart timer) and no `Job` is + * queued, or when systemd positively reports the unit not loaded + * (`LoadState=not-found`). `pending` when it is activating, active, + * deactivating, restarting or has a job; `unknown` when the query fails or + * leaves any of these out. + */ +export function classifySystemdManagerState(opts: { + readonly exitCode: number; + readonly stdout: string; +}): "idle" | "pending" | "unknown" { + if (opts.exitCode !== 0) { + return "unknown"; + } + const props = parseSystemdProperties(opts.stdout); + if (props.LoadState === "not-found") { + return "idle"; + } + const active = props.ActiveState; + const sub = props.SubState; + const job = props.Job; + if (active === undefined || sub === undefined || job === undefined) { + return "unknown"; + } + if (job.length > 0) { + return "pending"; + } + if (sub === "auto-restart") { + return "pending"; + } + return active === "inactive" || active === "failed" ? "idle" : "pending"; +} + +export async function readSystemdManagerState(opts: { + readonly deps: SystemdDeps; +}): Promise<"idle" | "pending" | "unknown"> { + if (!resolveSystemdAccess({ env: opts.deps.env }).allowed) { + // This process may not use the user service: it cannot have started + // one, and does not manage one. + return "idle"; + } + const show = await opts.deps.run("systemctl", [ + "--user", + "--no-pager", + "show", + SYSTEMD_UNIT_NAME, + // --all: print the requested properties even when empty, so "no job" + // is reported as `Job=` rather than left out (left out is unknown). + "--all", + "--property=LoadState,ActiveState,SubState,Job", + ]); + return classifySystemdManagerState(show); +} + /** * The service as systemd has it loaded now, drop-ins included: its * effective PATH directories and every program it runs must pass the diff --git a/tests/daemon-reconcile.test.ts b/tests/daemon-reconcile.test.ts index b6ec04728..184e1a5c4 100644 --- a/tests/daemon-reconcile.test.ts +++ b/tests/daemon-reconcile.test.ts @@ -15,7 +15,11 @@ import { checkRunningLaunchdDaemon, startViaLaunchdIfLoaded, } from "../src/commands/daemon-launchd.ts"; -import { startReplacementDaemon } from "../src/commands/daemon-replace.ts"; +import { + type ReplacementStartOutcome, + type ServiceManagerState, + startReplacementDaemon, +} from "../src/commands/daemon-replace.ts"; import { observePostUpdateDaemon, parseStartEnvelope, @@ -31,6 +35,7 @@ import { parseLaunchdPlistProgram, plistKeepsThisHome, proveLaunchdJobOwnership, + readLaunchdManagerState, renderLaunchdPlist, } from "../src/daemon/launchd.ts"; import { resolveDaemonPaths } from "../src/daemon/paths.ts"; @@ -45,6 +50,7 @@ import { } from "../src/daemon/reconcile.ts"; import { detachedDaemonEnvironment } from "../src/daemon/spawn.ts"; import { + classifySystemdManagerState, findDroppedHomeBinding, isHackManagedUnit, readInvokerInSystemdUnit, @@ -1162,93 +1168,216 @@ test("launchd: an incompatible direct daemon is not replaced when the loaded job expect(effects).toEqual([]); }); -// Review round 10 (HACK-2216): a replacement never leaves hackd stopped. +// Review rounds 10 and 11 (HACK-2216): a replacement does not leave hackd +// stopped, but never starts a second daemon next to a pending manager. + +type ReplacementCase = { + readonly outcome?: ReplacementStartOutcome; + readonly rejects?: Error; + readonly running?: boolean; + readonly managerState?: ServiceManagerState; + readonly directCode?: number; +}; -test("a refused service start after the stop falls back to a direct start", async () => { +async function runReplacement(c: ReplacementCase) { const trace = createDaemonCommandTrace(); const steps: string[] = []; - const code = await startReplacementDaemon({ - trace, - startViaService: () => { - steps.push("service"); - // As startViaSystemdIfInstalled does after a post-stop refusal. - trace.manager = "systemd"; - trace.blocked = "rewrite-refused"; - return Promise.resolve(1); - }, - startDirect: () => { - steps.push("direct"); - return Promise.resolve(0); - }, - isRunning: () => Promise.resolve(false), - }); - expect(code).toBe(0); - expect(steps).toEqual(["service", "direct"]); - expect(trace).toMatchObject({ - manager: "systemd", - action: "restarted", - blocked: "rewrite-refused", - fallback: "direct", + const asked: Array<"systemd" | "launchd" | null> = []; + let thrown: unknown = null; + let code: number | null = null; + try { + code = await startReplacementDaemon({ + trace, + startViaService: () => { + steps.push("service"); + if (c.rejects) { + return Promise.reject(c.rejects); + } + return Promise.resolve( + c.outcome ?? { + kind: "service-attempted", + exitCode: 1, + manager: "systemd", + } + ); + }, + startDirect: () => { + steps.push("direct"); + return Promise.resolve(c.directCode ?? 0); + }, + isRunning: () => Promise.resolve(c.running ?? false), + managerState: (manager) => { + asked.push(manager); + return Promise.resolve(c.managerState ?? "idle"); + }, + }); + } catch (error: unknown) { + thrown = error; + } + return { code, thrown, steps, asked, trace }; +} + +test("a failed service start falls back to a direct start once the manager is idle", async () => { + const r = await runReplacement({ + outcome: { kind: "service-attempted", exitCode: 1, manager: "systemd" }, }); + expect(r.code).toBe(0); + expect(r.steps).toEqual(["service", "direct"]); + expect(r.asked).toEqual(["systemd"]); + expect(r.trace).toMatchObject({ action: "restarted", fallback: "direct" }); expect( buildDaemonCommandData({ command: "start", - trace, + trace: r.trace, daemon: { running: true, pid: 9, version: "2.0.0" }, cliVersion: "2.0.0", }) - ).toMatchObject({ - action: "restarted", - blocked: "rewrite-refused", - fallback: "direct", - }); + ).toMatchObject({ action: "restarted", fallback: "direct" }); }); -test("the fallback is not used when the service start worked, is still starting, or was already direct", async () => { - const run = async (opts: { - readonly serviceCode: number; - readonly manager: "systemd" | "launchd" | "none"; - readonly running: boolean; - readonly directCode?: number; - }) => { - const trace = createDaemonCommandTrace(); - const steps: string[] = []; - const code = await startReplacementDaemon({ - trace, - startViaService: () => { - steps.push("service"); - trace.manager = opts.manager; - trace.action = opts.serviceCode === 0 ? "started" : "none"; - return Promise.resolve(opts.serviceCode); - }, - startDirect: () => { - steps.push("direct"); - return Promise.resolve(opts.directCode ?? 0); - }, - isRunning: () => Promise.resolve(opts.running), +test("a service still activating (no pid or socket yet) gets no direct start", async () => { + for (const managerState of ["pending", "unknown"] as const) { + const r = await runReplacement({ + outcome: { kind: "service-attempted", exitCode: 1, manager: "systemd" }, + running: false, + managerState, }); - return { code, steps, trace }; - }; - const ok = await run({ serviceCode: 0, manager: "launchd", running: true }); - expect(ok).toMatchObject({ code: 0, steps: ["service"] }); - expect(ok.trace.action).toBe("restarted"); + expect(r.code).toBe(1); + expect(r.steps).toEqual(["service"]); + expect(r.trace.blocked).toBe("service-pending"); + expect(r.trace.fallback).toBeNull(); + } +}); + +test("a rejected service start is treated as a failed one, behind the same gate", async () => { + const boom = new Error("systemctl crashed"); + const idle = await runReplacement({ rejects: boom }); + expect(idle.thrown).toBeNull(); + expect(idle.code).toBe(0); + expect(idle.steps).toEqual(["service", "direct"]); + // Which manager got how far is unknown: the platform's own is asked. + expect(idle.asked).toEqual([null]); + expect(idle.trace.fallback).toBe("direct"); + // Pending, or the direct start fails too: the original error stands. + const pending = await runReplacement({ + rejects: boom, + managerState: "pending", + }); + expect(pending.thrown).toBe(boom); + expect(pending.steps).toEqual(["service"]); + const failing = await runReplacement({ rejects: boom, directCode: 1 }); + expect(failing.thrown).toBe(boom); + expect(failing.steps).toEqual(["service", "direct"]); +}); + +test("an early service refusal with an empty trace still falls back", async () => { + // As an untrusted unit refuses before systemd is asked: nothing was + // recorded on the trace, but the outcome says the service refused. + const r = await runReplacement({ + outcome: { kind: "service-refused", exitCode: 1, manager: "systemd" }, + }); + expect(r.code).toBe(0); + expect(r.steps).toEqual(["service", "direct"]); + expect(r.trace.manager).toBe("none"); + expect(r.trace.fallback).toBe("direct"); +}); + +test("no direct start when the service worked, hackd already answers, or the start was direct", async () => { + const ok = await runReplacement({ + outcome: { kind: "service-attempted", exitCode: 0, manager: "launchd" }, + }); + expect(ok.code).toBe(0); + expect(ok.steps).toEqual(["service"]); expect(ok.trace.fallback).toBeNull(); - // The service started hackd but it was not ready in time: no second one. + const answering = await runReplacement({ running: true }); + expect(answering.steps).toEqual(["service"]); + expect(answering.code).toBe(1); + const direct = await runReplacement({ + outcome: { kind: "direct-attempted", exitCode: 1, manager: null }, + }); + expect(direct.steps).toEqual(["service"]); + expect(direct.asked).toEqual([]); + const failing = await runReplacement({ directCode: 1 }); + expect(failing.code).toBe(1); + expect(failing.trace.fallback).toBeNull(); +}); + +test("systemd counts as idle only when inactive or failed, not restarting, with no job", () => { + const show = (text: string, exitCode = 0) => + classifySystemdManagerState({ exitCode, stdout: text }); + expect( + show("LoadState=loaded\nActiveState=inactive\nSubState=dead\nJob=\n") + ).toBe("idle"); + expect( + show("LoadState=loaded\nActiveState=failed\nSubState=failed\nJob=\n") + ).toBe("idle"); + expect(show("LoadState=not-found\nActiveState=inactive\n")).toBe("idle"); expect( - (await run({ serviceCode: 1, manager: "systemd", running: true })).steps - ).toEqual(["service"]); - // The service path was not used: the direct start already ran. + show("LoadState=loaded\nActiveState=activating\nSubState=start\nJob=12\n") + ).toBe("pending"); expect( - (await run({ serviceCode: 1, manager: "none", running: false })).steps - ).toEqual(["service"]); - // The direct start fails too: the service path's error stands. - const both = await run({ - serviceCode: 1, - manager: "launchd", + show( + "LoadState=loaded\nActiveState=activating\nSubState=auto-restart\nJob=\n" + ) + ).toBe("pending"); + expect( + show("LoadState=loaded\nActiveState=inactive\nSubState=dead\nJob=34\n") + ).toBe("pending"); + // A property left out, or a failed query, is not evidence. + expect(show("LoadState=loaded\nActiveState=inactive\nSubState=dead\n")).toBe( + "unknown" + ); + expect(show("", 1)).toBe("unknown"); +}); + +test("launchd counts as idle only when absent, or loaded, stopped and not retrying", async () => { + const paths = resolveDaemonPaths({ home: "/nonexistent-home" }); + const status = ( + over: Partial<{ + installed: boolean; + loaded: boolean; + running: boolean; + pid: number | null; + observation: "reported" | "not-found" | "failed"; + }> + ) => ({ + installed: true, + loaded: true, running: false, - directCode: 1, + pid: null, + exitStatus: null, + observation: "reported" as const, + ...over, }); - expect(both.code).toBe(1); - expect(both.steps).toEqual(["service", "direct"]); - expect(both.trace.fallback).toBeNull(); + const plist = (keys: string) => + `\n\n\n Label\n dance.hack.hackd\n${keys}\n\n\n`; + const state = ( + s: ReturnType, + text: string | null = plist("") + ) => readLaunchdManagerState({ paths, status: s, plistText: text }); + expect(await state(status({ installed: false, loaded: false }))).toBe("idle"); + expect(await state(status({ loaded: false, observation: "not-found" }))).toBe( + "idle" + ); + expect(await state(status({ loaded: false, observation: "failed" }))).toBe( + "unknown" + ); + expect(await state(status({ running: true, pid: 4242 }))).toBe("pending"); + expect(await state(status({}))).toBe("idle"); + expect( + await state(status({}), plist(" KeepAlive\n ")) + ).toBe("pending"); + expect( + await state(status({}), plist(" RunAtLoad\n ")) + ).toBe("pending"); + // Hack's own plist: a conditional KeepAlive dict cannot be told. + expect( + await state( + status({}), + plist( + " KeepAlive\n \n SuccessfulExit\n \n " + ) + ) + ).toBe("unknown"); + expect(await state(status({}), null)).toBe("unknown"); }); diff --git a/tests/daemon-systemd.test.ts b/tests/daemon-systemd.test.ts index 4a0205ec6..93748802e 100644 --- a/tests/daemon-systemd.test.ts +++ b/tests/daemon-systemd.test.ts @@ -53,6 +53,7 @@ import { parseSystemdPathList, probeSystemdUserManager, programFileProblem, + readSystemdManagerState, readSystemdServicePathDirectories, readSystemdServiceStatus, renderSystemdUnit, @@ -5152,3 +5153,64 @@ linuxTest( expect(await readFile(unitPath, "utf8")).toBe(stale); } ); + +// Review round 11 (HACK-2216): explicit start outcomes and manager state. + +linuxTest( + "an early systemd refusal is reported as not having asked systemd to start", + async () => { + const fake = fakeSystemd(); + await installSystemdUnit({ + unitPath, + unitText: unitText(), + deps: fake.deps, + }); + // A unit file others could write: refused before any start request. + await chmod(unitPath, 0o666); + let requested = 0; + const trace = createDaemonCommandTrace(); + fake.calls.length = 0; + expect( + await startViaSystemdIfInstalled({ + ...startHooks(fake), + trace, + onStartRequested: () => { + requested += 1; + }, + }) + ).toBe(1); + expect(requested).toBe(0); + // Nothing reached the trace: the outcome, not the trace, decides. + expect(trace.manager).toBe("none"); + expect(fake.calls).not.toContain("systemctl --user start hackd.service"); + + // A usable unit: systemd is asked, and the hook says so first. + await chmod(unitPath, 0o644); + expect( + await startViaSystemdIfInstalled({ + ...startHooks(fake), + onStartRequested: () => { + requested += 1; + }, + }) + ).toBe(0); + expect(requested).toBe(1); + } +); + +linuxTest("systemd's manager state is read through the runner", async () => { + const fake = fakeSystemd(); + let answer = + "LoadState=loaded\nActiveState=activating\nSubState=start\nJob=7\n"; + const run = fake.deps.run; + const deps: SystemdDeps = { + ...fake.deps, + run: (command, args) => + args.includes("--property=LoadState,ActiveState,SubState,Job") + ? Promise.resolve({ exitCode: 0, stdout: answer, stderr: "" }) + : run(command, args), + }; + expect(await readSystemdManagerState({ deps })).toBe("pending"); + answer = "LoadState=loaded\nActiveState=inactive\nSubState=dead\nJob=\n"; + expect(await readSystemdManagerState({ deps })).toBe("idle"); +}); From cb3aafad15f2a139e63eb5451f40ed4610533b4c Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 21:41:55 +0000 Subject: [PATCH 15/43] fix(daemon): treat launchd as idle only on its confirmed not-found answer Review round 12 on HACK-2216 (owner decision, simplify): the replacement fallback's launchd gate now queries launchctl print for Hack's label whether or not the plist file exists, and is idle only on the confirmed not-found result (exit 113 with 'Could not find service'). Every loaded job, whatever its plist file says, and every other result is unknown. This drops the missing-plist shortcut and the on-disk KeepAlive/RunAtLoad reading (parseLaunchdPlistRetry removed). Co-Authored-By: Claude Opus 5.5 --- docs/architecture.md | 11 +-- src/commands/daemon.ts | 2 +- src/daemon/launchd-plist.ts | 40 ----------- src/daemon/launchd.ts | 68 +++++++++--------- tests/daemon-reconcile.test.ts | 82 ++++++++++------------ tests/fixtures/launchd-isolation-driver.ts | 8 +++ tests/launchd-isolation.test.ts | 65 +++++++++++++++++ 7 files changed, 150 insertions(+), 126 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 2264676c8..1863ff70f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -746,10 +746,13 @@ Such a plist is never replaced automatically; `start` reports it and evidence that the manager is idle: - systemd: `ActiveState` is `inactive` or `failed`, `SubState` is not `auto-restart`, and no `Job` is queued (or the unit is not loaded); - - launchd: the job is not loaded (launchd says so), or it is loaded, not - running, and its plist asks for no retry (`KeepAlive` and `RunAtLoad` not - true). Hack's own plist has a conditional `KeepAlive`, which cannot be - told, so a loaded Hack plist counts as unknown. + - launchd: only when `launchctl print` for Hack's label answers not-found + (exit 113, "Could not find service"). Hack asks launchd even when the plist + file is gone, because moving or deleting the file does not unload the job. + A loaded job is never idle, whatever its plist file says: the loaded + configuration can differ from the file, and `KeepAlive`, `RunAtLoad`, + `StartInterval`, `StartCalendarInterval` or `WatchPaths` can start it + again. On macOS the fallback therefore runs only once the job is unloaded. When the manager may still start hackd, or cannot be checked, `start` returns the service's error with `blocked: "service-pending"` and says the diff --git a/src/commands/daemon.ts b/src/commands/daemon.ts index f807edca9..e86f65367 100644 --- a/src/commands/daemon.ts +++ b/src/commands/daemon.ts @@ -384,7 +384,7 @@ function readServiceManagerState(opts: { const manager = opts.manager ?? (process.platform === "darwin" ? "launchd" : "systemd"); if (manager === "launchd") { - return readLaunchdManagerState({ paths: opts.paths }); + return readLaunchdManagerState(); } return process.platform === "linux" ? readSystemdManagerState({ deps: defaultSystemdDeps() }) diff --git a/src/daemon/launchd-plist.ts b/src/daemon/launchd-plist.ts index 4d9798398..ef5b9f1b6 100644 --- a/src/daemon/launchd-plist.ts +++ b/src/daemon/launchd-plist.ts @@ -27,7 +27,6 @@ type PlistValue = readonly kind: "dict"; readonly keys: ReadonlyMap; } - | { readonly kind: "bool"; readonly value: boolean } | { readonly kind: "other" }; const PROLOG = /^\s*(<\?xml[^?]*\?>)?\s*([]*>)?/; @@ -122,42 +121,6 @@ export function parseLaunchdPlistEnvironment(opts: { return { kind: "env", env }; } -/** - * Whether launchd may start the job again on its own (HACK-2216): `none` - * when the root has no `KeepAlive` (or `KeepAlive` false) and no - * `RunAtLoad` true; `retry` when `KeepAlive` is true or `RunAtLoad` is - * true; `unknown` for a conditional `KeepAlive` dict (whether it fires - * depends on state this reader does not see), any other value, or an - * unreadable plist. - */ -export function parseLaunchdPlistRetry(opts: { - readonly plistText: string; -}): "none" | "retry" | "unknown" { - let root: Map | null; - try { - root = readRootDict(opts.plistText); - } catch { - return "unknown"; - } - const keepAlive = root?.get("KeepAlive"); - const runAtLoad = root?.get("RunAtLoad"); - const flag = (value: PlistValue | undefined) => { - if (value === undefined) { - return "false" as const; - } - if (value.kind === "bool") { - return value.value ? ("true" as const) : ("false" as const); - } - return "unknown" as const; - }; - const keep = flag(keepAlive); - const load = flag(runAtLoad); - if (keep === "unknown" || load === "unknown") { - return "unknown"; - } - return keep === "true" || load === "true" ? "retry" : "none"; -} - function programPath(raw: string): LaunchdPlistProgram { // Exactly as written: launchd does not trim string values. const path = raw; @@ -344,9 +307,6 @@ function emptyValue(name: string): PlistValue { if (name === "dict") { return { kind: "dict", keys: new Map() }; } - if (name === "true" || name === "false") { - return { kind: "bool", value: name === "true" }; - } return { kind: "other" }; } diff --git a/src/daemon/launchd.ts b/src/daemon/launchd.ts index 02ef64fba..499408816 100644 --- a/src/daemon/launchd.ts +++ b/src/daemon/launchd.ts @@ -18,7 +18,6 @@ import { findExecutableInPath } from "../lib/shell.ts"; import { parseLaunchdPlistEnvironment, parseLaunchdPlistProgram, - parseLaunchdPlistRetry, } from "./launchd-plist.ts"; import type { DaemonPaths } from "./paths.ts"; import { type DaemonProcessProof, proveOwnedDaemonProcess } from "./process.ts"; @@ -626,45 +625,42 @@ export function plistKeepsThisHome(opts: { } /** - * Whether launchd will not start hackd on its own any more (HACK-2216): - * `idle` when no plist is installed (or this process may not use launchd), - * when launchd positively reports the job absent (`not-found`), or when - * the loaded job is not running and its plist asks for no retry - * ({@link parseLaunchdPlistRetry}). `pending` when it runs or will retry; - * `unknown` when the query failed, the plist is unreadable, or its - * `KeepAlive` is conditional. + * Whether launchd will not start hackd on its own any more (HACK-2216, + * owner decision): `idle` only when `launchctl print` for Hack's label, + * queried whether or not the plist file exists (moving or deleting it does + * not unload the job), positively answers not-found + * ({@link isLaunchdServiceNotFound}). Any loaded job is `unknown`, whatever + * its plist on disk says: the loaded configuration can differ from the + * file, and KeepAlive, RunAtLoad, StartInterval, StartCalendarInterval or + * WatchPaths can start it again. Any other result is `unknown` too. + * + * A process that may not use launchd (an isolated HACK_HOME, or + * HACK_LAUNCHD_DISABLE) cannot query it; it never started a launchd job + * either, so the job it would ask about is not this replacement's: `idle`. */ -export async function readLaunchdManagerState(opts: { - readonly paths: DaemonPaths; - readonly status?: LaunchdServiceStatus; - readonly plistText?: string | null; -}): Promise<"idle" | "pending" | "unknown"> { +export async function readLaunchdManagerState( + opts: { + /** Defaults to `launchctl print gui//