History

cli: `service install` takes over a running daemon instead of refusing (seamless upgrade)

`service install` is the upgrade path (the `service status` command tells users to run it on version drift), but it refused whenever any daemon already owned the data dir: "A local Executor cli-daemon is already running … Stop it first." On a real upgrade — `npm i -g executor@latest && executor service install`, or the desktop app auto-installing the service — the user has no idea which pid to kill, so the upgrade just fails. (Reproduced in a real macOS VM: install exits 1, the predecessor keeps the port, no service is installed.) Whatever holds the data dir — a foreground `executor web`, a manual `daemon run`, or an old desktop sidecar — writes the same server-control manifest, so it's the one process we're entitled to replace. Take it over: - `service install` now plans via `planServiceInstall` (pure, unit-tested): noop when the supervised service already runs this version; reinstall (bootout→bootstrap) when it runs an older/unreachable one; otherwise take over the predecessor (SIGTERM the manifest's pid, wait until it stops serving, clear its records) and install. The data dir is untouched, so connections/secrets/the bearer survive. - The supervised daemon's own startup (`runDaemonSession` under EXECUTOR_SUPERVISED) now takes over a surviving predecessor too — both the manifest owner and a pointer-recorded `daemon run` — instead of failing and crash-looping under KeepAlive. Verified end to end in a macOS VM (e2e/scripts/repro-upgrade-takeover.ts): before, install exits 1 and the predecessor keeps :4789; after, install stops it ("Replaced the running Executor cli-daemon (pid …)") and the supervised service owns the port — exactly one daemon. Plus unit tests for the planner.

Rhys Sullivan committed 5 months ago

Commit b8d36911 parent 1e1567e4 files · +307 −27Browse files

Unified · Split

4 files
  1. apps/cli/src/daemon.test.ts +69 −0
  2. apps/cli/src/daemon.ts +34 −0
  3. apps/cli/src/main.ts +102 −27
  4. e2e/scripts/repro-upgrade-takeover.ts +102 −0
apps/cli/src/daemon.test.ts+69−0
@@ -7,6 +7,7 @@ import { · 6 unmodified lines
77 canAutoStartLocalDaemonForHost,
88 isDevCliEntrypoint,
99 isExecutorServerReachable,
10+ planServiceInstall,
1011 } from "./daemon";
1112
1213 describe("isDevCliEntrypoint", () => {
@@ -93,3 +94,71 @@ describe("isExecutorServerReachable", () => { · 80 unmodified lines
9394 }),
9495 );
9596 });
97+
98+describe("planServiceInstall", () => {
99+ it("no-ops when the supervised service already runs THIS version", () => {
100+ expect(
101+ planServiceInstall({
102+ registered: true,
103+ running: true,
104+ activeVersion: "1.5.11",
105+ currentVersion: "1.5.11",
106+ }),
107+ ).toBe("noop");
108+ });
109+
110+ it("reinstalls when the supervised service runs an OLDER version (the upgrade)", () => {
111+ expect(
112+ planServiceInstall({
113+ registered: true,
114+ running: true,
115+ activeVersion: "1.5.10",
116+ currentVersion: "1.5.11",
117+ }),
118+ ).toBe("reinstall");
119+ });
120+
121+ it("reinstalls when the supervised service is up but its version is unknown/unreachable", () => {
122+ expect(
123+ planServiceInstall({
124+ registered: true,
125+ running: true,
126+ activeVersion: null,
127+ currentVersion: "1.5.11",
128+ }),
129+ ).toBe("reinstall");
130+ });
131+
132+ it("takes over when a daemon is running but NOT as the supervised service", () => {
133+ // The upgrade case that used to wall the user off: a foreground `executor
134+ // web` / manual `daemon run` / old desktop sidecar holds the data dir while
135+ // the OS service isn't registered+running yet.
136+ expect(
137+ planServiceInstall({
138+ registered: false,
139+ running: false,
140+ activeVersion: "1.5.10",
141+ currentVersion: "1.5.11",
142+ }),
143+ ).toBe("takeover-then-install");
144+ expect(
145+ planServiceInstall({
146+ registered: true, // registered but not running (stopped/flapping) → still take over
147+ running: false,
148+ activeVersion: null,
149+ currentVersion: "1.5.11",
150+ }),
151+ ).toBe("takeover-then-install");
152+ });
153+
154+ it("installs cleanly on a fresh machine (nothing running)", () => {
155+ expect(
156+ planServiceInstall({
157+ registered: false,
158+ running: false,
159+ activeVersion: null,
160+ currentVersion: "1.5.11",
161+ }),
162+ ).toBe("takeover-then-install");
163+ });
164+});
apps/cli/src/daemon.ts+34−0
@@ -282,3 +282,37 @@ export const chooseDaemonPort = (input: { · 281 unmodified lines
282282 }
283283 return fallbackPort;
284284 });
285+
286+// ---------------------------------------------------------------------------
287+// Service-install planning (pure)
288+// ---------------------------------------------------------------------------
289+
290+/**
291+ * What `service install` should do given the current service + daemon state.
292+ * `service install` IS the upgrade path (the `service status` command points
293+ * users at it on version drift), so it must take over a machine that already
294+ * has a daemon running rather than refuse and make the user hunt for a pid.
295+ */
296+export type ServiceInstallPlan = "noop" | "reinstall" | "takeover-then-install";
297+
298+/**
299+ * Decide the install action:
300+ * - `noop`: the supervised service is already running THIS version.
301+ * - `reinstall`: the supervised service is registered + running but drifted to
302+ * another version (or its manifest is unreachable) — bootout→bootstrap
303+ * repoints the unit at the current binary and restarts it.
304+ * - `takeover-then-install`: no supervised service is up, so any local daemon
305+ * that holds the data dir (a foreground `web`, a manual `daemon run`, an old
306+ * desktop sidecar) must be stopped first, then the service installed.
307+ */
308+export const planServiceInstall = (input: {
309+ readonly registered: boolean;
310+ readonly running: boolean;
311+ readonly activeVersion: string | null;
312+ readonly currentVersion: string;
313+}): ServiceInstallPlan => {
314+ if (input.registered && input.running) {
315+ return input.activeVersion === input.currentVersion ? "noop" : "reinstall";
316+ }
317+ return "takeover-then-install";
318+};
apps/cli/src/main.ts+102−27
@@ -71,6 +71,7 @@ import { · 70 unmodified lines
7171 isExecutorServerReachable,
7272 isDevCliEntrypoint,
7373 parseDaemonBaseUrl,
74+ planServiceInstall,
7475 spawnDetached,
7576 waitForReachable,
7677 waitForUnreachable,
@@ -100,7 +101,6 @@ import { · 23 unmodified lines
100101 acquireLocalServerStartLock,
101102 readLocalServerManifest,
102103 releaseLocalServerStartLock,
103− removeLocalServerManifest,
104104 removeLocalServerManifestIfOwnedBy,
105105 resolveExecutorDataDir,
106106 writeLocalServerManifest,
@@ -286,6 +286,52 @@ const assertNoOtherActiveLocalServer = (): Effect.Effect< · 179 unmodified lines
286286 );
287287 });
288288
289+/**
290+ * Take over the local server that currently owns this data directory, if any —
291+ * the seam that makes `service install` (and a supervised daemon launching while
292+ * a predecessor is still up) an UPGRADE rather than a wall. Whatever holds the
293+ * data dir — a foreground `executor web`, a manually-started `daemon run`, an
294+ * old desktop sidecar — writes the same server-control manifest, so reading it
295+ * identifies the one process we are entitled to replace (same data dir = same
296+ * logical singleton). SIGTERM it (graceful: it releases the port + removes its
297+ * own records), wait until it stops serving, then clear any stale manifest. A
298+ * dead/own-pid manifest is just cleaned. Returns the manifest we replaced (for
299+ * messaging), or null when there was nothing live to take over.
300+ */
301+const takeOverActiveLocalServer = (): Effect.Effect<
302+ ExecutorLocalServerManifest | null,
303+ Error,
304+ FileSystem.FileSystem | PlatformPath.Path
305+> =>
306+ Effect.gen(function* () {
307+ const manifest = yield* readLocalServerManifest();
308+ if (!manifest) return null;
309+ if (!isPidAlive(manifest.pid) || manifest.pid === process.pid) {
310+ yield* removeLocalServerManifestIfOwnedBy({ pid: manifest.pid }).pipe(Effect.ignore);
311+ return null;
312+ }
313+ // Graceful stop; ignore a kill error (pid may have just exited) — the
314+ // unreachable wait is the real gate.
315+ yield* terminatePid(manifest.pid).pipe(Effect.ignore);
316+ const stopped = yield* waitForUnreachable({
317+ check: isServerReachable(manifest.connection.origin),
318+ timeoutMs: DAEMON_STOP_TIMEOUT_MS,
319+ intervalMs: DAEMON_BOOT_POLL_MS,
320+ });
321+ if (!stopped) {
322+ return yield* Effect.fail(
323+ new Error(
324+ [
325+ `The existing Executor ${manifest.kind} at ${manifest.connection.origin} (pid ${manifest.pid}) did not stop within ${DAEMON_STOP_TIMEOUT_MS / 1000}s.`,
326+ "Stop it manually and re-run.",
327+ ].join("\n"),
328+ ),
329+ );
330+ }
331+ yield* removeLocalServerManifestIfOwnedBy({ pid: manifest.pid }).pipe(Effect.ignore);
332+ return manifest;
333+ });
334+
289335 const publishLocalServerManifest = (input: {
290336 readonly kind: ExecutorLocalServerKind;
291337 readonly connection: ExecutorServerConnection;
@@ -897,7 +943,13 @@ const runDaemonSession = (input: { · 605 unmodified lines
897943 // and crash-loop under KeepAlive. (Found by a real reboot test with
898944 // integration data in the DB.)
899945 if (process.env.EXECUTOR_SUPERVISED) {
900− yield* removeLocalServerManifest().pipe(Effect.ignore);
946+ // launchd/systemd is the singleton — take over whatever held this data
947+ // dir: a stale manifest from before a reboot, or a foreground/old
948+ // predecessor that survived because it wasn't the unit the OS started
949+ // (e.g. the desktop app installed the service while a CLI `web` ran).
950+ // Best-effort: if the port is somehow still held, startServer fails and
951+ // KeepAlive retries.
952+ yield* takeOverActiveLocalServer().pipe(Effect.ignore);
901953 } else {
902954 yield* assertNoOtherActiveLocalServer();
903955 }
@@ -907,15 +959,26 @@ const runDaemonSession = (input: { · 3 unmodified lines
907959 if (existing) {
908960 const existingUrl = daemonBaseUrl(existing.hostname, existing.port);
909961 if (isPidAlive(existing.pid) && (yield* isServerReachable(existingUrl))) {
910− return yield* Effect.fail(
911− new Error(
912− [
913− `A daemon is already running for scope ${scopeId} on ${daemonHost}.`,
914− `Existing daemon: ${existingUrl} (pid ${existing.pid}).`,
915− `Stop it first: ${cliPrefix} daemon stop`,
916− ].join("\n"),
917− ),
918− );
962+ if (process.env.EXECUTOR_SUPERVISED) {
963+ // A live predecessor recorded in the pointer (a manual `daemon run`):
964+ // stop it and claim the scope, rather than refusing and crash-looping.
965+ yield* terminatePid(existing.pid).pipe(Effect.ignore);
966+ yield* waitForUnreachable({
967+ check: isServerReachable(existingUrl),
968+ timeoutMs: DAEMON_STOP_TIMEOUT_MS,
969+ intervalMs: DAEMON_BOOT_POLL_MS,
970+ });
971+ } else {
972+ return yield* Effect.fail(
973+ new Error(
974+ [
975+ `A daemon is already running for scope ${scopeId} on ${daemonHost}.`,
976+ `Existing daemon: ${existingUrl} (pid ${existing.pid}).`,
977+ `Stop it first: ${cliPrefix} daemon stop`,
978+ ].join("\n"),
979+ ),
980+ );
981+ }
919982 }
920983 yield* cleanupPointer({ hostname: existing.hostname, scopeId, port: existing.port });
921984 }
@@ -2135,26 +2198,38 @@ const serviceInstallCommand = Command.make( · 1213 unmodified lines
21352198 return;
21362199 }
21372200
2138− // Don't fight an already-running local server against the same data dir
2139− // (a desktop sidecar, a foreground `executor web`, or an existing daemon).
2140− const active = yield* readActiveLocalServerManifest();
2141− if (active) {
2142− const status = yield* backend.status();
2143− if (status.registered && status.running && active.kind === "cli-daemon") {
2201+ // `service install` IS the upgrade path (see `service status` drift hint),
2202+ // so it must not wall off a machine that already has a daemon running —
2203+ // the user wouldn't know to kill it. Two cases:
2204+ // 1. The supervised service is already up → (re)install repoints the unit
2205+ // at THIS binary and restarts it (bootout→bootstrap). Same version is
2206+ // a friendly no-op; an older version is the actual upgrade.
2207+ // 2. Anything else holds the data dir (a foreground `executor web`, a
2208+ // manual `daemon run`, an old desktop sidecar) → take it over (stop it)
2209+ // before installing, so the supervised daemon can claim the port.
2210+ const status = yield* backend.status();
2211+ const active = yield* readActiveLocalServerManifest().pipe(Effect.orElseSucceed(() => null));
2212+ const plan = planServiceInstall({
2213+ registered: status.registered,
2214+ running: status.running,
2215+ activeVersion: active?.owner.version ?? null,
2216+ currentVersion: CLI_VERSION,
2217+ });
2218+ if (plan === "noop") {
2219+ const where = active ? ` at ${active.connection.origin} (pid ${active.pid})` : "";
2220+ console.log(`Executor service already running${where}.`);
2221+ return;
2222+ }
2223+ if (plan === "takeover-then-install") {
2224+ const replaced = yield* takeOverActiveLocalServer();
2225+ if (replaced) {
21442226 console.log(
2145− `Executor service already running at ${active.connection.origin} (pid ${active.pid}).`,
2227+ `Replaced the running Executor ${replaced.kind} (pid ${replaced.pid}) at ${replaced.connection.origin}.`,
21462228 );
2147− return;
21482229 }
2149− return yield* Effect.fail(
2150− new Error(
2151− [
2152− `A local Executor ${active.kind} is already running at ${active.connection.origin} (pid ${active.pid}).`,
2153− `Stop it first (quit the desktop app, or \`${cliPrefix} daemon stop\`), then re-run install.`,
2154− ].join("\n"),
2155− ),
2156− );
21572230 }
2231+ // plan === "reinstall" falls through: backend.install's bootout→bootstrap
2232+ // repoints the unit at this binary and restarts it cleanly.
21582233
21592234 // The unit carries no secret: the supervised daemon mints/loads its bearer
21602235 // from auth.json (under EXECUTOR_DATA_DIR) on first boot, and clients read
e2e/scripts/repro-upgrade-takeover.ts+102−0
@@ -0,0 +1,102 @@
1+// UPGRADE STRESS: `service install` must TAKE OVER a daemon that is already
2+// running (the upgrade path), not refuse and make the user hunt for a pid.
3+// Stage a predecessor daemon (executor daemon run, like an old install or a
4+// `executor web` a user left running), then `service install` the supervised
5+// daemon, and assert the predecessor was stopped and the supervised service now
6+// owns the canonical port — exactly one daemon. Exits non-zero if it did NOT
7+// take over (the pre-fix bug). Manual VM harness (real launchd); always
8+// discards the guest.
9+//
10+// bun e2e/scripts/repro-upgrade-takeover.ts
11+import { tartVm } from "../src/vm/tart";
12+
13+const t0 = Date.now();
14+const log = (m: string) => console.log(`[+${((Date.now() - t0) / 1000).toFixed(0)}s] ${m}`);
15+const PORT = 4789;
16+
17+const main = async () => {
18+ log("provisioning tart macos guest...");
19+ const vm = await tartVm("macos", "arm64").provision();
20+ log(`provisioned host=${vm.host}`);
21+ try {
22+ const dir = "~/ed";
23+ const exe = `${dir}/executor`;
24+ await vm.ssh(`rm -rf ${dir} && mkdir -p ${dir}`);
25+ log("pushing darwin CLI binary...");
26+ await vm.push("/tmp/wt-ops/apps/cli/dist/executor-darwin-arm64/bin/.", `${dir}/`);
27+ await vm.ssh(`chmod +x ${exe}; xattr -dr com.apple.quarantine ${dir} 2>/dev/null || true`);
28+ log(`version: ${(await vm.ssh(`${exe} --version`)).stdout.trim()}`);
29+
30+ // 1) PREDECESSOR: a daemon the user already has running (manual daemon run).
31+ log("starting PREDECESSOR daemon (executor daemon run --foreground)...");
32+ await vm.ssh(
33+ `nohup ${exe} daemon run --foreground --port ${PORT} > /tmp/pre.log 2>&1 & echo started`,
34+ );
35+ let preReachable = false;
36+ for (let i = 0; i < 20; i++) {
37+ const h = await vm.ssh(
38+ `curl -s -o /dev/null -w '%{http_code}' --max-time 3 http://127.0.0.1:${PORT}/api/health`,
39+ );
40+ if (h.stdout.trim() === "200") {
41+ preReachable = true;
42+ break;
43+ }
44+ await new Promise((r) => setTimeout(r, 1000));
45+ }
46+ log(`predecessor reachable: ${preReachable}`);
47+ const prePid = (
48+ await vm.ssh(`lsof -ti tcp:${PORT} -sTCP:LISTEN 2>/dev/null | head -1`)
49+ ).stdout.trim();
50+ log(`predecessor owns :${PORT} as pid ${prePid}`);
51+
52+ // 2) UPGRADE: install the supervised service while the predecessor runs.
53+ log("running `executor service install` (the upgrade) WITH predecessor still running...");
54+ const install = await vm.ssh(`${exe} service install --port ${PORT} 2>&1`);
55+ log(`install exit=${install.code}\n${install.stdout.trim()}\n${install.stderr.trim()}`);
56+
57+ // 3) OBSERVE for ~25s: does the supervised service take over, or flap?
58+ await new Promise((r) => setTimeout(r, 25000));
59+ const uid = (await vm.ssh("id -u")).stdout.trim();
60+ const print = await vm.ssh(
61+ `launchctl print gui/${uid}/sh.executor.daemon 2>/dev/null | grep -iE 'state|pid|last exit|runs' | head`,
62+ );
63+ log(`launchctl print:\n${print.stdout.trim() || "(service not found)"}`);
64+ const errlog = await vm.ssh(`tail -8 ~/.executor/logs/daemon.error.log 2>/dev/null`);
65+ log(`daemon.error.log tail:\n${errlog.stdout.trim() || "(empty)"}`);
66+ const ownerNow = (
67+ await vm.ssh(`lsof -ti tcp:${PORT} -sTCP:LISTEN 2>/dev/null | head -1`)
68+ ).stdout.trim();
69+ const procCount = (
70+ await vm.ssh(`pgrep -fl 'executor daemon run' 2>/dev/null | wc -l`)
71+ ).stdout.trim();
72+ const preAlive = (
73+ await vm.ssh(`kill -0 ${prePid} 2>/dev/null && echo alive || echo dead`)
74+ ).stdout.trim();
75+ log(
76+ `AFTER: :${PORT} owned by pid ${ownerNow} (predecessor was ${prePid}, now ${preAlive}); 'daemon run' procs=${procCount}`,
77+ );
78+
79+ // The bug: predecessor still owns the port (no takeover), and either the
80+ // service errored or it's flapping / a 2nd daemon exists.
81+ const tookOver = ownerNow !== "" && ownerNow !== prePid && preAlive === "dead";
82+ if (tookOver) {
83+ log("RESULT=TOOK-OVER (predecessor stopped, supervised owns the port) — upgrade is clean");
84+ } else {
85+ log(
86+ "RESULT=NO-TAKEOVER (predecessor still owns the port / service did not replace it) — BUG",
87+ );
88+ throw new Error("service install did not take over the running predecessor");
89+ }
90+ } finally {
91+ log("discarding guest VM");
92+ await vm.discard();
93+ log("discarded");
94+ }
95+};
96+
97+main()
98+ .then(() => process.exit(0))
99+ .catch((e) => {
100+ console.error("REPRO-ERROR", e);
101+ process.exit(1);
102+ });