From e383e63ab900bb37dff3ec81eb3e12d5c89b086f Mon Sep 17 00:00:00 2001 From: Theo Gravity Date: Fri, 23 Oct 2026 18:30:04 -0700 Subject: [PATCH 1/5] fix(update): fan-out downloads get full bytes; stuck updates explain themselves The lazy-release fetch shared one in-flight promise whose single stream was already spent for every joiner but the first, so "Update all" handed all but one node an empty download. fetchArtifact now resolves only once the bytes are complete and verified on disk, hands out a handle that opens its own reader per call, and a digest mismatch fails the fetch before any byte is served. The not-supervised refusal composes from the runtime report (live or held), naming whether a definition exists and what a launchd GUI-domain refusal means. On the node, kickstart exit 125 carries the signed-in-at-the-display hint, and a refused restart after the swap reports the installed binary, the manager's reason, and the two ways forward. --- ...node-update-fanout-and-restart-guidance.md | 8 + apps/node/agent/docs/service.md | 6 + apps/node/agent/docs/update.md | 13 ++ apps/node/agent/src/__tests__/service.test.ts | 51 +++++ apps/node/agent/src/__tests__/update.test.ts | 40 +++- apps/node/agent/src/service.ts | 15 +- apps/node/agent/src/update.ts | 11 +- apps/server/api/AGENTS.md | 6 +- apps/server/api/docs/node-artifacts.md | 32 ++- apps/server/api/docs/self-management.md | 10 + .../src/api/__tests__/downloads-route.test.ts | 111 +++++++--- apps/server/api/src/api/downloads.route.ts | 14 +- .../nodes/__tests__/update-node.route.test.ts | 121 ++++++++++- .../api/src/api/nodes/update-node.route.ts | 47 ++++- .../src/services/__tests__/releases.test.ts | 85 ++++++-- apps/server/api/src/services/releases.ts | 190 +++++++++++------- 16 files changed, 610 insertions(+), 150 deletions(-) create mode 100644 .changeset/node-update-fanout-and-restart-guidance.md diff --git a/.changeset/node-update-fanout-and-restart-guidance.md b/.changeset/node-update-fanout-and-restart-guidance.md new file mode 100644 index 000000000..d537b3ecd --- /dev/null +++ b/.changeset/node-update-fanout-and-restart-guidance.md @@ -0,0 +1,8 @@ +--- +"@internal/server": patch +"@internal/node": patch +--- + +Fix "Update all" handing every node but the first an empty download. When several machines asked at once, the shared fetch handed out one spent stream; each caller now opens its own read of the finished download, and a digest mismatch fails the fetch before any byte is served. + +Say more when an update refuses or stalls. The "not supervised" refusal now names whether a service definition exists on that machine and, for a launchd domain that needs someone signed in at the display, says so. When the swap lands but the restart is refused, the node reports the new binary as installed, shows the manager's own reason, and names the two ways to get it running. diff --git a/apps/node/agent/docs/service.md b/apps/node/agent/docs/service.md index 865f71426..4b515c3f7 100644 --- a/apps/node/agent/docs/service.md +++ b/apps/node/agent/docs/service.md @@ -152,6 +152,12 @@ stopped. Start is `bootstrap` falling back to `kickstart -k`; restart is `kickstart -k` (a bare `kickstart` on a running job changes nothing); stop is `bootout` (KeepAlive undoes a mere kill), and on Linux `stop`, never `disable --now`: un-enabling is what uninstall is for. +A `kickstart` that answers `Domain does not support specified action` (exit +125) is the GUI-session half of the domain rule: `gui/` exists only while +someone is signed in at the display, so an SSH-only operator gets that refusal +and a hint to run `subshell run` by hand (measured on a Mac Studio, +2026-10-23); the hint ships in the error line because launchd's own words +name no remedy. **The pane guard is the reason those live here rather than in the caller.** This node's subshells run their tmux servers as CHILDREN of the daemon, so a diff --git a/apps/node/agent/docs/update.md b/apps/node/agent/docs/update.md index 583052ad5..79ab8f9bb 100644 --- a/apps/node/agent/docs/update.md +++ b/apps/node/agent/docs/update.md @@ -73,6 +73,19 @@ budget, and it is a belt: the plane pushes `set_allowed_dirs` on every accepted of the timer never firing is a stale ~70 MB `.previous`; the cost of it firing early is the rollback. +**A restart that fails AFTER the swap is a report, not a refusal.** The +keyboard path's `restart` is not an all-or-nothing gate: the swap is already +done, and the manager's refusal to bounce is its own outcome. `applyUpdate` +then returns `restarted: false` with a `note` that says all of it in order: +the new binary IS installed, the running daemon is still the old one, the +manager's own first line is the reason, and the two ways forward (`subshell +service restart`, or stop and restart the running `subshell run`) are named. +Bare-manager-line notes read as "the update failed", which on 2026-10-23 left +a Mac Studio's operator standing next to an installed 2.3.1 trying to +re-download it. The launchd domain cause (kickstart exit 125: a user's `gui/ +` agents need someone signed in at the display) gets its own sentence +from `controlService`, which is where launchd's words are first-hand. + **Refusals are the WIRE CONSTANTS, never sentences.** The plane matches `NodeRpcError.detail` by equality, so `applyUpdate` throws `UpdateRefused` carrying a `NODE_RESULT_*` string alongside the human message, and the executor diff --git a/apps/node/agent/src/__tests__/service.test.ts b/apps/node/agent/src/__tests__/service.test.ts index 296f28c78..25536069a 100644 --- a/apps/node/agent/src/__tests__/service.test.ts +++ b/apps/node/agent/src/__tests__/service.test.ts @@ -1194,6 +1194,57 @@ describe("controlService", () => { expect(s.calls.at(-1)).toEqual(["launchctl", "kickstart", "-k", TARGET]); }); + // Live incident 2026-10-23 (thedarknesss-Mac-Studio): a manual update swapped + // the binary, then `kickstart -k` answered 125 "Domain does not support + // specified action" — the gui/ agent domain exists only while someone is + // signed in at the machine's own display, so a daemon started from an SSH + // session sits outside launchd's reach. The bare exit code told nobody that, + // or what to do. + test("darwin: a kickstart DOMAIN failure names the GUI-session cause and the way through", async () => { + const s = stub({ + platform: "darwin", + respond: (cmd) => + cmd[0] === "plutil" + ? { code: 0, out: "true\n", err: "" } + : cmd[1] === "print" + ? { code: 0, out: "\tstate = running\n\tpid = 5150\n", err: "" } + : cmd[1] === "kickstart" + ? { + code: 125, + out: "", + err: 'Could not kickstart service "dev.subshell.client": 125: Domain does not support specified action\n', + } + : { code: 0, out: "", err: "" }, + }); + s.files.set(PLIST, "AbandonProcessGroupRunAtLoad"); + const r = await controlService(s.deps, "restart"); + expect(r.code).toBe(1); + expect(msgLine(r.err)).toContain("launchctl kickstart -k failed (exit 125)"); + expect(msgLine(r.err)).toContain("Domain does not support specified action"); + expect(msgLine(r.err)).toContain("signed in at this machine's own display"); + expect(msgLine(r.err)).toContain("subshell run"); + }); + + test("darwin: other kickstart failures point at service status, not the domain story", async () => { + const s = stub({ + platform: "darwin", + respond: (cmd) => + cmd[0] === "plutil" + ? { code: 0, out: "true\n", err: "" } + : cmd[1] === "print" + ? { code: 0, out: "\tstate = running\n\tpid = 5150\n", err: "" } + : cmd[1] === "kickstart" + ? { code: 1, out: "", err: "BootStrap failed for some other reason\n" } + : { code: 0, out: "", err: "" }, + }); + s.files.set(PLIST, "AbandonProcessGroupRunAtLoad"); + const r = await controlService(s.deps, "restart"); + expect(r.code).toBe(1); + expect(msgLine(r.err)).toContain("BootStrap failed for some other reason"); + expect(msgLine(r.err)).toContain("subshell service status"); + expect(msgLine(r.err)).not.toContain("signed in at"); + }); + test("darwin: restart of a STOPPED job bootstraps it — kickstart needs a loaded job", async () => { const s = darwinServiceStub({ loaded: false }); const r = await controlService(s.deps, "restart"); diff --git a/apps/node/agent/src/__tests__/update.test.ts b/apps/node/agent/src/__tests__/update.test.ts index 2ea95ddef..cefcc60be 100644 --- a/apps/node/agent/src/__tests__/update.test.ts +++ b/apps/node/agent/src/__tests__/update.test.ts @@ -670,7 +670,45 @@ describe("applyUpdate", () => { restartService: async () => ({ code: 1, out: "", err: "subshell: this would close 3 subshells\nuse --force" }), }); expect(applied.restarted).toBe(false); - expect(applied.note).toBe("subshell: this would close 3 subshells"); + // The note says ALL of it, in the order the person needs it: the swap + // landed, the running daemon is the OLD one, the manager's own words are + // the reason, and two ways forward are named. A bare manager line read + // like nothing had happened. + expect(applied.note).toContain("the new binary is installed"); + expect(applied.note).toContain("this would close 3 subshells"); + expect(applied.note).toContain("subshell service restart"); + expect(await readFile(binary, "utf8")).toBe("NEXT"); + }); + + it("the refused-restart note names the swap first — the Mac-Studio kickstart shape", async () => { + // Live incident 2026-10-23: `launchctl kickstart -k` answered 125 ("Domain + // does not support specified action") after a manual update had ALREADY + // replaced the binary. The operator read that as a failed update; the + // daemon was one restart away from the new version. The note must make + // the installed-and-standing-still state unmistakable. + const { binary, root, dataDir } = await installedNode(); + pretendInstalledAt(binary); + const local = join(root, "next"); + await writeFile(local, "NEXT"); + const applied = await applyUpdate({ + binaryDeps: NO_SERVICE_DEFINITION, + source: { kind: "file", path: local }, + version: "0.9.3", + force: true, + restart: true, + origin: "cli", + dataDir, + probeVersion: async () => "0.9.3", + restartService: async () => ({ + code: 1, + out: "", + err: 'subshell: launchctl kickstart -k failed (exit 125): Could not kickstart service "dev.subshell.client": 125: Domain does not support specified action', + }), + }); + expect(applied.restarted).toBe(false); + expect(applied.note).toContain("the new binary is installed"); + expect(applied.note).toContain("still runs"); + expect(applied.note).toContain("launchctl kickstart -k failed (exit 125)"); expect(await readFile(binary, "utf8")).toBe("NEXT"); }); }); diff --git a/apps/node/agent/src/service.ts b/apps/node/agent/src/service.ts index afc73eb3b..1ffc4682a 100644 --- a/apps/node/agent/src/service.ts +++ b/apps/node/agent/src/service.ts @@ -1314,7 +1314,20 @@ export async function controlService( if (state.state === "stopped") return bootstrapDarwin(deps, target, DONE.restart, warning, state.definitionPath ?? plistPath(deps.home)); const res = await deps.runCmd(["launchctl", "kickstart", "-k", target]); - if (res.code !== 0) return errLine(`launchctl kickstart -k failed (exit ${res.code}): ${cmdDetail(res)}`); + if (res.code !== 0) { + // A bare launchctl exit code is not an action. "Domain does not support + // specified action" (125) is the common one on a headless Mac: the + // gui/ agent domain exists only while someone is signed in at the + // machine's own display, so a daemon started over SSH sits outside + // launchd's reach and no `-k` will ever land. Say the cause and the + // way forward; everything else at least gets pointed at the status + // view, which carries launchd's own words. + const detail = cmdDetail(res); + const hint = /domain/i.test(detail) + ? "; the user's launchd domain needs someone signed in at this machine's own display (its agents run in a GUI session, not an SSH one). Start the daemon by hand with `subshell run`, or sign in at the console and try again" + : "; run `subshell service status` to see what launchd says about this job"; + return errLine(`launchctl kickstart -k failed (exit ${res.code}): ${detail}${hint}`); + } return done(DONE.restart); } // start diff --git a/apps/node/agent/src/update.ts b/apps/node/agent/src/update.ts index 397197c76..a7fc475a3 100644 --- a/apps/node/agent/src/update.ts +++ b/apps/node/agent/src/update.ts @@ -820,12 +820,21 @@ export async function applyUpdate(input: ApplyUpdateInput): Promise { const TARGET = "darwin-arm64"; const binaryPath = join(NODE_ARTIFACTS_DIR, `subshell-node-cli-${TARGET}`); const BODY = "a convincing darwin binary"; + /** The bytes the same tag answers once the source has moved on — the + * release-advance tests' release B. Bytes and announced digest stay + * COHERENT, because since the 2026-10-23 fan-out fix the plane hashes the + * whole file before serving, so an announcement that its own bytes cannot + * satisfy reads as a failed download, not as an advanced release. */ + const BODY_B = "release B's darwin binary, published after the update was ordered"; let cookie = ""; let _userId = ""; - let release: { url: string; stop: () => void; serveDigest: string }; - /** What /manifest's `assets` map names; "" falls back to the served bytes' digest. */ - let servedManifestDigest = ""; + let release: { url: string; stop: () => void; serveDigest: string; digestB: string }; + /** Flip the source to release B: /bin serves its bytes and /manifest names them. */ + let servingReleaseB = false; beforeAll(async () => { await setupAuthTables(); @@ -1451,6 +1458,9 @@ describe("/api/downloads/node/* — the lazy fetch", () => { const hasher = new Bun.CryptoHasher("sha256"); hasher.update(new TextEncoder().encode(BODY)); const digest = hasher.digest("hex"); + const hasherB = new Bun.CryptoHasher("sha256"); + hasherB.update(new TextEncoder().encode(BODY_B)); + const digestB = hasherB.digest("hex"); const server = Bun.serve({ port: 0, fetch(request) { @@ -1477,7 +1487,7 @@ describe("/api/downloads/node/* — the lazy fetch", () => { }, ]); } - if (url.pathname === "/bin") return new Response(BODY); + if (url.pathname === "/bin") return new Response(servingReleaseB ? BODY_B : BODY); if (url.pathname === "/sha") return new Response(`${release.serveDigest}\n`); if (url.pathname === "/sig") return new Response(TEST_ARMOR); if (url.pathname === "/manifest") { @@ -1487,17 +1497,17 @@ describe("/api/downloads/node/* — the lazy fetch", () => { nodeProtocol: NODE_PROTOCOL_VERSION, minNodeVersion: MIN_NODE_VERSION, commit: "0123456789abcdef0123456789abcdef01234567", - // `servedManifestDigest` defaults to the digest of the bytes /bin - // serves; the release-advance test below moves it to a DIFFERENT - // value, which is what "the source now answers another release" - // means without a second server. - assets: { [`subshell-node-cli-${TARGET}`]: servedManifestDigest || digest }, + // The release-advance tests flip `servingReleaseB`, which moves + // /bin and this map together to release B's coherent pair — what + // "the source now answers another release" means without a + // second server. + assets: { [`subshell-node-cli-${TARGET}`]: servingReleaseB ? digestB : digest }, }); } return new Response("no", { status: 404 }); }, }); - release = { url: `http://127.0.0.1:${server.port}`, stop: () => server.stop(true), serveDigest: digest }; + release = { url: `http://127.0.0.1:${server.port}`, stop: () => server.stop(true), serveDigest: digest, digestB }; // The minisign crypto stands in, as in the releases/update-node suites: // it is pinned once against real `tauri signer` fixtures in the protocol // package, and this suite's subject is the download route's caching and @@ -1636,6 +1646,43 @@ describe("/api/downloads/node/* — the lazy fetch", () => { } }); + it("serves TWO simultaneous update downloads with full bytes each — the fan-out incident", async () => { + // The 2026-10-23 "Update all" incident, at the route the fan-out actually + // hits: every node the operator pressed was answered by the SAME + // in-flight fetch, and the shared stream let only the first response + // carry bytes. The coalescer now shares the settled file and each + // response opens its own read — so every concurrent node downloads + // completely, not just the one whose request happened to be first. + setReleaseUrlForTests(`${release.url}/releases`); + resetReleaseCacheForTests(); + resetUpdateTokensForTests(); + rmSync(binaryPath, { force: true }); + rmSync(`${binaryPath}.sha256`, { force: true }); + rmSync(join(NODE_ARTIFACTS_DIR, ".fetched.json"), { force: true }); + try { + const tokens = [1, 2, 3].map((n) => mintUpdateToken(`fan${n}`, TARGET, release.serveDigest)); + const resps = await Promise.all( + tokens.map((token) => + app.handle( + new Request(`http://localhost/api/downloads/node/${TARGET}?update_token=${encodeURIComponent(token)}`), + ), + ), + ); + const bodies = await Promise.all(resps.map((res) => res.text())); + expect(bodies.map((body) => body.length)).toEqual([BODY.length, BODY.length, BODY.length]); + bodies.forEach((body) => { + expect(body).toBe(BODY); + }); + // The cached shelf is still the one clean copy: bytes arrived once and + // landed intact. + expect(readFileSync(binaryPath, "utf8")).toBe(BODY); + } finally { + rmSync(binaryPath, { force: true }); + rmSync(`${binaryPath}.sha256`, { force: true }); + rmSync(join(NODE_ARTIFACTS_DIR, ".fetched.json"), { force: true }); + } + }); + it("an update token refuses a release the source has moved past, before any byte is served", async () => { // Release-coherent serving, the second half: the token names release A's // digest, but the index's 15-minute TTL means the source may answer B by @@ -1652,9 +1699,10 @@ describe("/api/downloads/node/* — the lazy fetch", () => { const STALE = "a stale darwin binary, ignored either way"; writeFileSync(binaryPath, STALE); try { - // Release B: the manifest now names bytes nobody published here. The - // manifest is memoized on the index entry, so the memo must drop too. - servedManifestDigest = "b".repeat(64); + // Release B: the source now answers coherent bytes nobody published + // here, under the same tag. The manifest is memoized on the index + // entry, so the memo must drop too. + servingReleaseB = true; resetReleaseCacheForTests(); const token = mintUpdateToken("n1", TARGET, release.serveDigest); @@ -1666,11 +1714,13 @@ describe("/api/downloads/node/* — the lazy fetch", () => { expect(body.code).toBe("NODE_UPDATE_UNAVAILABLE"); expect(body.message).toContain("node-artifacts"); expect(body.message).toContain("release:cli-node"); - // Nothing streamed: the stale file is untouched and nothing was - // recorded as ours. (The upstream request head is made before the - // comparison; it is the stream that never starts.) + // Nothing served: the fetched B bytes went through a temp copy that the + // refusal deleted, the stale file stands untouched, and nothing was + // recorded as ours — the comparison now lands BEFORE the first byte, + // because the fetch verifies the whole file before handing anything out. expect(readFileSync(binaryPath, "utf8")).toBe(STALE); expect(existsSync(join(NODE_ARTIFACTS_DIR, ".fetched.json"))).toBe(false); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.includes(".download-"))).toBe(false); // The token was spent by the refused request. expect( ( @@ -1680,7 +1730,7 @@ describe("/api/downloads/node/* — the lazy fetch", () => { ).status, ).toBe(401); } finally { - servedManifestDigest = ""; + servingReleaseB = false; resetReleaseCacheForTests(); rmSync(binaryPath, { force: true }); rmSync(`${binaryPath}.sha256`, { force: true }); @@ -1701,7 +1751,7 @@ describe("/api/downloads/node/* — the lazy fetch", () => { rmSync(`${binaryPath}.sha256`, { force: true }); rmSync(join(NODE_ARTIFACTS_DIR, ".fetched.json"), { force: true }); try { - servedManifestDigest = "b".repeat(64); + servingReleaseB = true; resetReleaseCacheForTests(); const token = mintUpdateToken("n1", TARGET, release.serveDigest); @@ -1714,13 +1764,24 @@ describe("/api/downloads/node/* — the lazy fetch", () => { expect(body.message).toContain("no longer the newest"); expect(body.message).toContain("Press Update again"); // The stale-artifact remedy is NOT the answer here: nobody published - // anything. + // anything to this disk. expect(body.message).not.toContain("node-artifacts"); - // Nothing streamed, nothing cached: the fetch was cancelled and the - // directory holds no record of release B. - expect(existsSync(binaryPath)).toBe(false); - expect(existsSync(join(NODE_ARTIFACTS_DIR, ".fetched.json"))).toBe(false); - // The token was spent by the refused request. + // NO BYTE of release B was served to the refused node — the fetch + // verifies the whole file before the route hands anything out, so the + // digest comparison lands first. What it DID land is the shelf: with + // nothing there before, B is now cached, because B is exactly what the + // next installer would be served, and the retry fast-paths onto it. + expect(readFileSync(binaryPath, "utf8")).toBe(BODY_B); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.includes(".download-"))).toBe(false); + // Press Update again: the retry is ordered against what the source now + // serves, and the served answer is the release the token names. + const retry = mintUpdateToken("n1", TARGET, release.digestB); + const again = await app.handle( + new Request(`http://localhost/api/downloads/node/${TARGET}?update_token=${encodeURIComponent(retry)}`), + ); + expect(again.status).toBe(200); + expect(await again.text()).toBe(BODY_B); + // The refused request's own token was spent by it, not by the retry. expect( ( await app.handle( @@ -1729,7 +1790,7 @@ describe("/api/downloads/node/* — the lazy fetch", () => { ).status, ).toBe(401); } finally { - servedManifestDigest = ""; + servingReleaseB = false; resetReleaseCacheForTests(); rmSync(binaryPath, { force: true }); rmSync(`${binaryPath}.sha256`, { force: true }); diff --git a/apps/server/api/src/api/downloads.route.ts b/apps/server/api/src/api/downloads.route.ts index 5539d015a..c735b7044 100644 --- a/apps/server/api/src/api/downloads.route.ts +++ b/apps/server/api/src/api/downloads.route.ts @@ -173,7 +173,7 @@ export const downloadsRoutes = new Elysia({ prefix: "/api/downloads" }).use(apiM } try { const fetched = await fetchArtifact(params.target, { cache: decision.cache }); - return new Response(fetched.stream, { headers }); + return new Response(fetched.body(), { headers }); } catch (error) { // A release that cannot be read is the same OUTCOME as an unpublished // build — the machine cannot install — so it is the same 404 rather than @@ -249,10 +249,14 @@ export const downloadsRoutes = new Elysia({ prefix: "/api/downloads" }).use(apiM // outstanding token, and streaming those bytes against the command's // digest would land the node in exactly the refusal this path exists to // prevent, one download later. So the comparison happens before the - // first byte is served, and a mismatch cancels the stream: nothing of - // the newer release reaches the node or the cache. + // first byte is served — and since the fetch verifies the whole file + // before this line runs (the "Update all" fan-out fix of 2026-10-23), a + // mismatch now stops with NO byte served: `dispose` removes the temp + // copy the stream-through fetch wrote, and where the fetch cached, the + // newer release stays on the shelf because it IS the release an + // installer would serve next — the retry fast-paths on it. if (fetched.digest !== expected) { - await fetched.stream.cancel().catch(() => {}); + await fetched.dispose(); // Two causes, two sentences: a STALE DISK file is the operator's // artifact problem (publish or hand update), but an EMPTY disk with // the index moved past the token is nothing anyone published; the @@ -269,7 +273,7 @@ export const downloadsRoutes = new Elysia({ prefix: "/api/downloads" }).use(apiM }), ); } - return new Response(fetched.stream, { headers }); + return new Response(fetched.body(), { headers }); } catch (error) { getLogger().warn(`node artifacts: could not fetch ${params.target} from the release: ${errorText(error)}`); // The release could not be served. With nothing (or nothing readable) diff --git a/apps/server/api/src/api/nodes/__tests__/update-node.route.test.ts b/apps/server/api/src/api/nodes/__tests__/update-node.route.test.ts index 2180ca3b7..25cfaaba6 100644 --- a/apps/server/api/src/api/nodes/__tests__/update-node.route.test.ts +++ b/apps/server/api/src/api/nodes/__tests__/update-node.route.test.ts @@ -331,7 +331,13 @@ describe("POST /api/nodes/:id/update", () => { function goHeld( nodeId: string, protocolVersion: number = NODE_PROTOCOL_VERSION, - entry: Partial<{ agentVersion: string; os: string; arch: string; reason: HeldReason }> = {}, + entry: Partial<{ + agentVersion: string; + os: string; + arch: string; + reason: HeldReason; + runtime?: NodeRuntimeReport; + }> = {}, ): ReturnType { const sock = fakeSocket(); const conn = attachConnection(nodeId, sock); @@ -343,6 +349,18 @@ describe("POST /api/nodes/:id/update", () => { arch: entry.arch ?? "x64", onIdle: () => undefined, }); + // A held socket that reached `ready` keeps what it reported: identity and + // runtime are persisted BEFORE the gate that holds the connection. A test + // passes `runtime` to stand in for exactly that frozen answer. + if (entry.runtime) { + conn.agent = { + dataDir: "/d", + capabilities: [], + hostname: "held-machine", + agentVersion: entry.agentVersion ?? "0.8.0", + runtime: entry.runtime, + }; + } return sock; } @@ -362,7 +380,10 @@ describe("POST /api/nodes/:id/update", () => { conn: "live" | "held" = "live", facts: NodeRuntimeReport | null = runtime, ): Promise<{ res: Response; cmd: Record }> { - const sock = conn === "held" ? goHeld(nodeId) : goOnline(nodeId, facts); + const sock = + conn === "held" + ? goHeld(nodeId, NODE_PROTOCOL_VERSION, facts ? { runtime: facts } : {}) + : goOnline(nodeId, facts); const record = conn === "held" ? getHeld(nodeId)?.conn : getLive(nodeId); if (!record) throw new Error("no connection record"); const resP = req("POST", `/api/nodes/${nodeId}/update`, { cookie, body }); @@ -853,6 +874,102 @@ describe("POST /api/nodes/:id/update", () => { expect(state?.message).toContain("not running under a service manager"); }); + it('names WHICH half of "not supervised" is true, from the runtime facts already in hand', async () => { + // Surfaced 2026-10-23: the bare constant gave the same sentence to three + // different machines — one with a definition installed whose daemon was + // started by hand, one with nothing installed at all, and a headless Mac + // whose launchd user-agent domain does not answer (nobody signed in at + // the display). The node's own `ready` already froze the answers into the + // report the plane holds; the refusal composes from those, never from + // anything fetched after the refusal. (The fixtures' `supervised: true` + // beside a spoken refusal is deliberately contradictory: wording is + // composed from the SERVICE block, and only the agent's answer decides + // the branch.) + useFakeRelease(); + + // A definition IS installed there, so the daemon answering did not start + // from it: the remedy is to bring it back under the manager. + const idA = await mkNode(); + const a = await updateWithAnswer(idA, aliceCookie, {}, { ok: false, error: NODE_RESULT_NOT_SUPERVISED }); + const msgA = ((await a.res.json()) as { message: string }).message; + expect(msgA).toContain("A service definition is installed there"); + expect(msgA).toContain("subshell service install"); + expect(msgA).toContain("subshell update"); + expect(msgA).not.toContain("Nothing is installed"); + expect(msgA).not.toContain("launchd"); + resetNodeRegistryForTests(); + + // Nothing is installed at all, so nothing COULD restart anything. + const noneInstalled: NodeRuntimeReport = { ...runtime, service: { ...runtime.service, installed: false } }; + const idB = await mkNode(); + const b = await updateWithAnswer( + idB, + aliceCookie, + {}, + { ok: false, error: NODE_RESULT_NOT_SUPERVISED }, + "live", + noneInstalled, + ); + const msgB = ((await b.res.json()) as { message: string }).message; + expect(msgB).toContain("Nothing is installed there"); + expect(msgB).toContain("subshell service install"); + resetNodeRegistryForTests(); + + // The Mac-Studio shape: a launchd job the manager cannot answer for. + const launchdUnknown: NodeRuntimeReport = { + ...runtime, + service: { ...runtime.service, manager: "launchd", state: "unknown", pid: null }, + }; + const idC = await mkNode(); + const c = await updateWithAnswer( + idC, + aliceCookie, + {}, + { ok: false, error: NODE_RESULT_NOT_SUPERVISED }, + "live", + launchdUnknown, + ); + const msgC = ((await c.res.json()) as { message: string }).message; + expect(msgC).toContain("A service definition is installed there"); + expect(msgC).toContain("launchd is not answering"); + expect(msgC).toContain("signed in at the machine's own display"); + resetNodeRegistryForTests(); + + // No runtime report at all: say nothing beyond the bare fact rather than + // invent a shape for that machine. + const idD = await mkNode(); + const d = await updateWithAnswer( + idD, + aliceCookie, + {}, + { ok: false, error: NODE_RESULT_NOT_SUPERVISED }, + "live", + null, + ); + const msgD = ((await d.res.json()) as { message: string }).message; + expect(msgD).toContain("update it where it was started"); + expect(msgD).not.toContain("subshell service install"); + resetNodeRegistryForTests(); + + // The held arm: a refused old agent is exactly the machine an update gets + // pressed on, and its pre-gate `ready` carries the same facts — the + // enrichment reads the held record too, and the held case is the common + // one for the headless Mac. + const idE = await mkNode(); + const e = await updateWithAnswer( + idE, + aliceCookie, + {}, + { ok: false, error: NODE_RESULT_NOT_SUPERVISED }, + "held", + launchdUnknown, + ); + const msgE = ((await e.res.json()) as { message: string }).message; + expect(msgE).toContain("launchd is not answering"); + expect(msgE).toContain("signed in at the machine's own display"); + resetNodeRegistryForTests(); + }); + // ── the update tracker (design 2026-09-25) ──────────────────────────────── // // The route owns three of the tracker's moments — the order opened, the 202 diff --git a/apps/server/api/src/api/nodes/update-node.route.ts b/apps/server/api/src/api/nodes/update-node.route.ts index 2dabaeeb5..beec21069 100644 --- a/apps/server/api/src/api/nodes/update-node.route.ts +++ b/apps/server/api/src/api/nodes/update-node.route.ts @@ -9,6 +9,7 @@ import { NODE_RESULT_NOT_SUPERVISED, NODE_RESULT_VERSION_MISMATCH, NODE_SIGNED_UPDATES_PROTOCOL_VERSION, + type NodeRuntimeReport, type NodeTarget, semverLt, } from "@internal/subshell-protocol"; @@ -67,7 +68,11 @@ interface Refusal { * lands here. The message names the verb to type at the keyboard, which is the * only remedy left for it. */ -function refusalFor(err: NodeRpcError, paneSafety: "keeps" | "kills" | "unknown" | undefined): Refusal { +function refusalFor( + err: NodeRpcError, + paneSafety: "keeps" | "kills" | "unknown" | undefined, + service: NodeRuntimeReport["service"] | undefined, +): Refusal { if (err.code === "offline") { return { code: BackendErrorCodes.NODE_OFFLINE, message: err.message }; } @@ -79,11 +84,28 @@ function refusalFor(err: NodeRpcError, paneSafety: "keeps" | "kills" | "unknown" }; } if (err.detail === NODE_RESULT_NOT_SUPERVISED) { - return { - code: BackendErrorCodes.NODE_NOT_SUPERVISED, - message: - "That node is not running under a service manager, so nothing would restart it into the new binary; update it where it was started", - }; + // The bare constant says only "not supervised". What a person can act on + // is WHICH half of that is true on that machine, and the node's `ready` + // already froze the answer in the runtime report the plane holds: whether + // a service definition is installed there at all, and whether the + // manager even answers about it. Composed from facts already in hand — + // nothing new crosses the wire, and the equality mapping above stands. + const base = "That node is not running under a service manager, so nothing would restart it into the new binary"; + let message = + service === undefined + ? `${base}; update it where it was started` + : service.installed + ? `${base}. A service definition is installed there, so the daemon answering here did not start from it; run \`subshell service install\` on that machine to bring it back under the manager, or update there with \`subshell update\`` + : `${base}. Nothing is installed there, so nothing could restart anything; run \`subshell service install\` on that machine and updates will work from here, or update there with \`subshell update\``; + if (service?.manager === "launchd" && service.state === "unknown") { + // launchd did not answer for the job. The macOS user-agent domain is + // the common cause on a headless box: it exists only while someone is + // signed in at the machine's own display, so a daemon started over SSH + // is running OUTSIDE launchd's reach by definition. + message += + ". launchd is not answering for this machine's user agents; on macOS that domain needs someone signed in at the machine's own display"; + } + return { code: BackendErrorCodes.NODE_NOT_SUPERVISED, message }; } if (err.detail === NODE_RESULT_KILLS_PANES) { return { @@ -399,8 +421,15 @@ export const updateNodeRoute = new Elysia() const from = reported ?? "unknown"; // Read BEFORE sending: the command is about to take this socket down, - // and this is what decides the WORDING of a pane-safety refusal. - const paneSafety = live?.agent?.runtime?.service.paneSafety; + // and this is what decides the WORDING of a pane-safety refusal. The + // whole service block is read for the same reason — a "not supervised" + // refusal names which half is true on that machine from these same + // frozen facts (2026-10-23 surfacing), not from anything fetched after. + // The held record counts too: its `ready` is that machine's last honest + // answer, persisted before the gate that held it, and a refused old + // agent is exactly the machine an update gets pressed on. + const runtimeService = live?.agent?.runtime?.service ?? held?.conn.agent?.runtime?.service; + const paneSafety = runtimeService?.paneSafety; // The tracker opens its entry the instant the order is real (design // 2026-09-25): AFTER every 409 gate above, because a refused offer // ordered nothing and must not render as an update in flight, and BEFORE @@ -459,7 +488,7 @@ export const updateNodeRoute = new Elysia() metadataJson: JSON.stringify({ from, to: release.version, forced: body.force === true }), }); } - const refusal = refusalFor(err, paneSafety); + const refusal = refusalFor(err, paneSafety, runtimeService); // Two codes are NOT the node saying it did nothing. `timeout`: the // plane stopped waiting on a five-minute window that contains a // ~70 MB download. `offline` (review 2026-09-26): failConnPendings diff --git a/apps/server/api/src/services/__tests__/releases.test.ts b/apps/server/api/src/services/__tests__/releases.test.ts index 5fd285c80..5f1fb4fe0 100644 --- a/apps/server/api/src/services/__tests__/releases.test.ts +++ b/apps/server/api/src/services/__tests__/releases.test.ts @@ -1,5 +1,5 @@ import { afterAll, afterEach, beforeEach, describe, expect, it } from "bun:test"; -import { existsSync, mkdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { MIN_NODE_VERSION, @@ -310,24 +310,25 @@ describe("fetchArtifact", () => { // has to survive on its own mkdir. rmSync(NODE_ARTIFACTS_DIR, { recursive: true, force: true }); const fetched = await fetchArtifact(TARGET); - await drain(fetched.stream); + await drain(fetched.body()); expect(existsSync(artifactPath(TARGET))).toBe(true); expect(readFileSync(artifactPath(TARGET), "utf8")).toBe("a convincing binary"); }); - it("streams the bytes and caches them once verified", async () => { + it("downloads the bytes in full and caches them once verified", async () => { const fetched = await fetchArtifact(TARGET); expect(fetched.tag).toBe("cli-node-v9.9.9"); - const bytes = await drain(fetched.stream); - expect(bytes).toBe(fake.assets.get(BINARY)?.byteLength ?? -1); - // Cached only after the digest matched, with its sidecar beside it so the - // `.sha256` route is answered locally next time. + expect(await drain(fetched.body())).toBe(fake.assets.get(BINARY)?.byteLength ?? -1); + // The cache landed the moment the fetch resolved — the digest gate runs on + // the whole file BEFORE any caller holds a body, the shape the 2026-10-23 + // fan-out fix moved to — with its sidecar beside it so the `.sha256` + // route is answered locally next time. expect(readFileSync(artifactPath(TARGET), "utf8")).toBe("a convincing binary"); expect(readFileSync(`${artifactPath(TARGET)}.sha256`, "utf8").trim()).toBe(fetched.digest); }); it("records what it fetched — tag and the signed manifest's commit — so the file is known to be ours", async () => { - await drain((await fetchArtifact(TARGET)).stream); + await fetchArtifact(TARGET); const manifest = JSON.parse(readFileSync(join(NODE_ARTIFACTS_DIR, ".fetched.json"), "utf8")); expect(manifest[TARGET]?.tag).toBe("cli-node-v9.9.9"); expect(manifest[TARGET]?.digest).toBe((await fetchArtifact(TARGET)).digest); @@ -336,16 +337,18 @@ describe("fetchArtifact", () => { expect(manifest[TARGET]?.commit).toBe("0123456789abcdef0123456789abcdef01234567"); }); - it("errors the stream and caches NOTHING when the digest is wrong", async () => { + it("refuses and caches NOTHING when the digest is wrong", async () => { // The SIGNED manifest announces a digest the bytes do not have — a - // corrupted transfer, or a release whose assets disagree. + // corrupted transfer, or a release whose assets disagree. The whole file + // is hashed before the fetch resolves, so the mismatch throws from the + // fetch itself and no caller ever holds a body of bad bytes. fake.assets.set(RELEASE_MANIFEST_NAME, manifestBody({ assets: { [BINARY]: "0".repeat(64) } })); - const fetched = await fetchArtifact(TARGET); - expect(drain(fetched.stream)).rejects.toThrow(/did not match the digest/); - // The node sees a truncated download and fails its own check; the next - // machine must not be served the bad bytes from a cache. + await expect(fetchArtifact(TARGET)).rejects.toThrow(/did not match the published digest/); + // The next machine must not be served the bad bytes from a cache, and the + // partial temp must not survive to look like a shelf file. expect(existsSync(artifactPath(TARGET))).toBe(false); expect(existsSync(join(NODE_ARTIFACTS_DIR, ".fetched.json"))).toBe(false); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.includes(".download-"))).toBe(false); }); it("caches NOTHING when the signature fails — the release source alone cannot push bytes here", async () => { @@ -370,11 +373,49 @@ describe("fetchArtifact", () => { expect(fetchArtifact(TARGET)).rejects.toThrow(/publishes no/); }); - it("downloads once when two machines ask at the same moment", async () => { + it("downloads once when two machines ask at the same moment — and both get a live body", async () => { + // The 2026-10-23 "Update all" incident: the coalesced value carried ONE + // ReadableStream, the first download consumed it, and every joiner's + // response wrapped a dead body — N minus one node reported a download + // failure while one update ran. The shared value is the settled file now, + // and each body() opens it again. const [a, b] = await Promise.all([fetchArtifact(TARGET), fetchArtifact(TARGET)]); - // Same in-flight fetch, so the 80 MB is spent once rather than twice. - expect(a).toBe(b); - await drain(a.stream); + expect(a).toBe(b); // same in-flight fetch: the 80 MB is spent once... + expect(fake.assetFetches.get(BINARY)).toBe(1); + // ...and BOTH callers drain the whole thing, in any order. + expect(await drain(a.body())).toBe(BINARY_PAYLOAD.bytes.byteLength); + expect(await drain(b.body())).toBe(BINARY_PAYLOAD.bytes.byteLength); + }); + + it("keeps the operator's file when asked not to cache, and removes its own temp copy after the serve", async () => { + // A file already sitting at the target path may be the operator's + // hand-published binary: `cache: false` serves the release bytes from a + // temp copy and leaves the path alone (the download route passes false + // exactly when the disk holds a different digest). + writeFileSync(artifactPath(TARGET), "the operator's own"); + const fetched = await fetchArtifact(TARGET, { cache: false }); + expect(await drain(fetched.body())).toBe(BINARY_PAYLOAD.bytes.byteLength); + expect(readFileSync(artifactPath(TARGET), "utf8")).toBe("the operator's own"); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.includes(".download-"))).toBe(false); + }); + + it("dispose removes the temp copy when the caller never serves it", async () => { + writeFileSync(artifactPath(TARGET), "the operator's own"); + const fetched = await fetchArtifact(TARGET, { cache: false }); + await fetched.dispose(); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.includes(".download-"))).toBe(false); + expect(readFileSync(artifactPath(TARGET), "utf8")).toBe("the operator's own"); + }); + + it("a cancelled temp serve still removes the file", async () => { + // The node hung up mid-download — the response body is cancelled, and the + // temp must go anyway. + writeFileSync(artifactPath(TARGET), "the operator's own"); + const fetched = await fetchArtifact(TARGET, { cache: false }); + const reader = fetched.body().getReader(); + await reader.read(); + await reader.cancel(); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.includes(".download-"))).toBe(false); }); }); @@ -522,7 +563,7 @@ describe("egress pin (round-3 sweep C12)", () => { fake.assets.set("signed-blob-storage", BINARY_PAYLOAD.bytes); fake.redirects.set(BINARY, "/asset/signed-blob-storage"); const fetched = await fetchArtifact(TARGET); - expect(await drain(fetched.stream)).toBe(BINARY_PAYLOAD.bytes.byteLength); + expect(await drain(fetched.body())).toBe(BINARY_PAYLOAD.bytes.byteLength); expect(readFileSync(artifactPath(TARGET), "utf8")).toBe("a convincing binary"); expect(fake.assetFetches.get("signed-blob-storage")).toBe(1); }); @@ -549,7 +590,7 @@ describe("superseding", () => { JSON.stringify({ [TARGET]: { tag: "cli-node-v1.0.0", digest: "x", fetchedAt: "2026-01-01T00:00:00.000Z" } }), ); - await drain((await fetchArtifact(TARGET)).stream); + await fetchArtifact(TARGET); // Ours was superseded and replaced by the new release's bytes... expect(readFileSync(artifactPath(TARGET), "utf8")).toBe("a convincing binary"); @@ -570,7 +611,7 @@ describe("superseding", () => { }), ); - await drain((await fetchArtifact(TARGET)).stream); + await fetchArtifact(TARGET); expect(existsSync(darwin)).toBe(false); const manifest = JSON.parse(readFileSync(join(NODE_ARTIFACTS_DIR, ".fetched.json"), "utf8")); @@ -585,7 +626,7 @@ describe("superseding", () => { "darwin-arm64": { tag: "cli-node-v9.9.9", digest: "x", fetchedAt: "2026-01-01T00:00:00.000Z" }, }), ); - await drain((await fetchArtifact(TARGET)).stream); + await fetchArtifact(TARGET); expect(readFileSync(join(NODE_ARTIFACTS_DIR, "subshell-node-cli-darwin-arm64"), "utf8")).toBe( "current darwin build", ); diff --git a/apps/server/api/src/services/releases.ts b/apps/server/api/src/services/releases.ts index 3a3d8bb79..6f012ad9d 100644 --- a/apps/server/api/src/services/releases.ts +++ b/apps/server/api/src/services/releases.ts @@ -786,15 +786,35 @@ async function record(target: NodeTarget, tag: string, digest: string, commit: s // Fetching // --------------------------------------------------------------------------- -/** In-flight fetches, so two enrollments of one platform download once. */ +/** + * In-flight fetches, so two enrollments of one platform download once. The + * promise resolves only once the bytes are COMPLETE on disk, which is what + * makes the shared value shareable: see {@link FetchedArtifact} for why an + * earlier shape that resolved a live stream early broke exactly one use of + * this map. + */ const inFlight = new Map>(); -/** A verified-on-the-fly artifact: the bytes to serve, and what they are. */ +/** A verified artifact resting on disk: the bytes to serve, and what they are. */ export interface FetchedArtifact { - /** The body to hand the client. Errors mid-flight if the digest does not match. */ - stream: ReadableStream; digest: string; tag: string; + /** + * A response body for ONE caller. Every call opens its own read of the + * verified bytes, so coalesced callers each get a live body. There is + * deliberately no `stream` property to hold: a ReadableStream can be + * consumed exactly once, and the "Update all" incident of 2026-10-23 was + * the coalesced fetch handing one to N concurrent downloads — the first + * node's update ran, every joiner's wrapped a dead body and its node + * reported a download failure. + */ + body(): ReadableStream; + /** + * Give up without serving (the digest gate refusing, say). Removes the temp + * copy when the fetch kept one; removes nothing when the bytes are cached, + * because a cached release is the shelf's own. + */ + dispose(): Promise; } /** Options for {@link fetchArtifact}. */ @@ -812,21 +832,35 @@ export interface FetchArtifactOptions { } /** - * Fetch one platform's agent binary, streaming it to the caller as it arrives. + * Fetch one platform's agent binary, verify it in full, and hand the caller a + * fresh read of the verified bytes. * * The expected digest comes from the release's SIGNED manifest (spec * 2026-09-17 §5 path 3) — verified before the first binary byte is fetched, * because the manifest and its signature are two tiny reads and having the - * publisher's word before the 80 MB is what lets this stream be cached at all. - * A release whose signature does not verify gets NO stream and NO cache entry; + * publisher's word before the 80 MB is what lets these bytes be cached at all. + * A release whose signature does not verify gets NO bytes and NO cache entry; * the downloads route's 404 is unchanged from the outside, and `install.sh`'s * own digest check now sits behind a signature check rather than beside a * same-source sidecar. + * + * Coalescing: concurrent callers of one target SHARE the download. The + * in-flight promise resolves only when the bytes are complete and verified on + * disk, so every joiner receives the same {@link FetchedArtifact} and opens + * its own body from it. The download-before-serve order costs the first + * caller's first byte (it now waits for the plane's full fetch from the + * release source) and buys the whole fan-out case: see the incident note on + * {@link FetchedArtifact}. A `cache: false` caller does NOT join: its bytes + * live in a temp file whose cleanup belongs to that one caller, and sharing + * it would let one joiner's cleanup delete another's serve mid-flight. The + * mixed case (a `cache: true` fetch already running) still rides it: the + * first caller's cache flag governs the shared download, same as before. */ export async function fetchArtifact(target: NodeTarget, options: FetchArtifactOptions = {}): Promise { + if (options.cache === false) return fetchArtifactUncoordinated(target, false); const existing = inFlight.get(target); if (existing) return existing; - const work = fetchArtifactUncoordinated(target, options.cache ?? true).finally(() => inFlight.delete(target)); + const work = fetchArtifactUncoordinated(target, true).finally(() => inFlight.delete(target)); inFlight.set(target, work); return work; } @@ -852,85 +886,97 @@ async function fetchArtifactUncoordinated(target: NodeTarget, cache: boolean): P throw new Error(`${release.tag}'s signed manifest names no ${names.binary}`); } - // C12. The lazy-fetch egress: `browser_download_url` is the release source's - // choice, and a mirror the operator configured can name any host — or name - // an allowed host that redirects to any other. The digest proves the bytes - // are what the release says they are, it does not prove where the plane was - // told to knock. An off-list URL, or an off-list hop, is refused here, - // before the connection, with the host named and logged. - const upstream = await fetchWithPinnedEgress(binaryUrl); - if (!upstream.ok || upstream.body === null) { - throw new Error(`${binaryUrl} answered ${upstream.status}`); - } - // The directory is this module's ONE production creator. Nothing else ever // makes it — not boot, not the publish scripts on this host — so an instance // that has never had `release:cli-node` run into its data dir died right here // on `writer()` with ENOENT, and the node's download answered 404 with the // plane's own filesystem named as nothing. Measured on mac-builder // 2026-09-20; every test green the whole time because every test fixture - // created the directory itself. The directory is created only when this - // fetch will cache: a stream-through fetch writes nothing, and the sink - // below must not exist before its directory does (that ENOENT is the - // measured failure above). - const tmp = `${artifactPath(target)}.fetch-${process.pid}`; - if (cache) await mkdir(NODE_ARTIFACTS_DIR, { recursive: true }); - // Caching is optional (see {@link FetchArtifactOptions}); verifying is not. - // The sink is null exactly when the caller asked for stream-through only. - const sink = cache ? Bun.file(tmp).writer() : null; - const hasher = new Bun.CryptoHasher("sha256"); - let seen = 0; + // created the directory itself. It is created for BOTH fetch shapes now: + // the cached rename needs it, and so does the temp file a stream-through + // fetch is written to before it is served and removed. + await mkdir(NODE_ARTIFACTS_DIR, { recursive: true }); + + // Download in full, hash as it goes, refuse a mismatch. All of that, plus + // the C12 pinned egress (the release source's `browser_download_url` may + // name any host, and an off-list URL or hop is refused before the + // connection) and the size cap, lives inside `downloadVerified` — the same + // discipline the server's own update download runs, beside the caller that + // EXECUTES the bytes. The fetch used to stream through while hashing and + // error the response at end of body; see the FetchedArtifact note for what + // that shared-stream shape cost. + let tmp: string; + try { + tmp = await downloadVerified({ + url: binaryUrl, + expectedDigest: expected, + destDir: NODE_ARTIFACTS_DIR, + destName: `subshell-node-cli-${target}.fetch`, + }); + } catch (error) { + // Keep the digest failure in the artifact log line the streaming design + // emitted: the release identity lives here, and `downloadVerified` is + // release-agnostic by contract. + if (error instanceof Error && error.message.includes("did not match the published digest")) { + getLogger().warn(`node artifacts: ${names.binary} from ${release.tag} failed its digest check: ${error.message}`); + } + throw error; + } - const reader = upstream.body.getReader(); - const stream = new ReadableStream({ + if (cache) { + await rename(tmp, artifactPath(target)); + // The sidecar is written from the MANIFEST's digest, not the other way + // round: `install.sh` keeps reading the file, it just no longer decides + // anything (spec 2026-09-17 §4). + await writeFile(`${artifactPath(target)}.sha256`, `${expected}\n`); + await record(target, release.tag, expected, manifest.manifest.commit); + getLogger().info(`node artifacts: cached ${names.binary} from ${release.tag}`); + const file = Bun.file(artifactPath(target)); + return { digest: expected, tag: release.tag, body: () => file.stream(), dispose: async () => {} }; + } + + // Stream-through: the file at the artifact path was not ours to overwrite + // (it may be the operator's hand-published binary), so these bytes live in + // the temp copy only, and that copy is deleted when the response ends. + // Not coalesced (see {@link fetchArtifact}) precisely because that cleanup + // is one caller's own. + return { + digest: expected, + tag: release.tag, + body: () => drainedFile(tmp), + dispose: () => rm(tmp, { force: true }).catch(() => {}), + }; +} + +/** + * A response body that reads a temp copy of verified bytes and removes the + * file once the read is over, however it ends: drained, cancelled, or broken + * mid-way. A leftover partial must never look like a shelf file, and a + * complete one left behind is 80 MB nobody asked the disk to keep. + */ +function drainedFile(tmp: string): ReadableStream { + const reader = Bun.file(tmp).stream().getReader(); + const sweep = (): Promise => rm(tmp, { force: true }).catch(() => {}); + return new ReadableStream({ async pull(controller) { - const { done, value } = await reader.read(); - if (!done) { - seen += value.byteLength; - if (seen > MAX_ARTIFACT_BYTES) { - await discard(sink, tmp); - controller.error(new Error(`${names.binary} exceeded ${MAX_ARTIFACT_BYTES} bytes`)); + try { + const { done, value } = await reader.read(); + if (done) { + await sweep(); + controller.close(); return; } - hasher.update(value); - sink?.write(value); controller.enqueue(value); - return; + } catch (error) { + await sweep(); + controller.error(error instanceof Error ? error : new Error(String(error))); } - // The end of the body is the only place the answer exists. A mismatch - // ERRORS the response rather than completing it, so the node sees a - // truncated transfer and its own sha check fails — and nothing - // unverified reaches the cache. - const actual = hasher.digest("hex"); - if (actual !== expected) { - await discard(sink, tmp); - getLogger().warn( - `node artifacts: ${names.binary} from ${release.tag} failed its digest check (expected ${expected}, got ${actual})`, - ); - controller.error(new Error(`${names.binary} did not match the digest the ${release.tag} manifest signed`)); - return; - } - if (sink !== null) { - await sink.end(); - await rename(tmp, artifactPath(target)); - // The sidecar is written from the MANIFEST's digest, not the other way - // round: `install.sh` keeps reading the file, it just no longer decides - // anything (spec 2026-09-17 §4). - await writeFile(`${artifactPath(target)}.sha256`, `${expected}\n`); - await record(target, release.tag, expected, manifest.manifest.commit); - getLogger().info(`node artifacts: cached ${names.binary} from ${release.tag} (${seen} bytes)`); - } - controller.close(); }, - async cancel() { - // The node hung up. Keep nothing: a partial file that later looked - // complete would be served to the next machine. - await reader.cancel().catch(() => {}); - await discard(sink, tmp); + async cancel(reason) { + await reader.cancel(reason).catch(() => {}); + await sweep(); }, }); - - return { stream, digest: expected, tag: release.tag }; } /** Drop a partial download, best effort — it must never survive to be served. */ From 73e52c6f485644f0a7822da8a6c4b6cb0c34a775 Mon Sep 17 00:00:00 2001 From: Theo Gravity Date: Fri, 23 Oct 2026 18:52:22 -0700 Subject: [PATCH 2/5] =?UTF-8?q?fix(update):=20review=20pass=20on=20the=20f?= =?UTF-8?q?an-out=20fix=20=E2=80=94=20retrying=20joiners,=20per-download?= =?UTF-8?q?=20temps,=20honest=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed fetch no longer drags its joiners down with it: a waiter that inherits a rejection retries once onto whatever newer attempt is running (or starts one), so one poisoned digest poisons one caller, not the fan-out. downloadVerified names its temp per download (pid plus a uuid), which is what two parallel cache:false fetches of one stale-shelf target need to stop interleaving writers and cross-deleting each other, and drainedFile opens its reader on first pull so a materialization that throws still sweeps the temp instead of leaking it. security.md still told the pre-fix story ("streamed through and hashed on the way past") and cited line anchors that had moved; both now describe the shape that shipped. Two regression tests cover the new behavior: a joiner's retry refetches, and side-by-side cache:false fetches both drain full bytes and leave the operator's file and no .download- residue. --- apps/server/api/AGENTS.md | 5 +- .../src/services/__tests__/releases.test.ts | 35 ++++++++ apps/server/api/src/services/releases.ts | 84 ++++++++++++++----- docs/security.md | 31 +++---- 4 files changed, 116 insertions(+), 39 deletions(-) diff --git a/apps/server/api/AGENTS.md b/apps/server/api/AGENTS.md index 305ac2e62..5e06e002b 100644 --- a/apps/server/api/AGENTS.md +++ b/apps/server/api/AGENTS.md @@ -123,8 +123,9 @@ unconsumed setup key, or a still-valid one-time `?update_token=`; never anonymous), and fetches the binary LAZILY from the project's own `cli-node-v*` GitHub release when that is the answer, downloading it in full and hashing the whole file against the SIGNED manifest's `assets` map before -any byte is served (the 2026-10-23 fan-out fix: an in-flight fetch is shared, -and a shared one-shot stream handed every joiner but the first an empty body). Which release a +any byte is served (the 2026-10-23 fan-out fix: an in-flight fetch is +shared, and a shared one-shot stream handed every joiner but the first an +empty body). Which release a node is OFFERED (`NODE_PROTOCOL_VERSION` matching), the three-way refusal grammar, the shelf-reconciled versus release-coherent serving (spec 2026-10-09: the copy is asked what it is before it wins), and the surfaces diff --git a/apps/server/api/src/services/__tests__/releases.test.ts b/apps/server/api/src/services/__tests__/releases.test.ts index 5f1fb4fe0..7ce261032 100644 --- a/apps/server/api/src/services/__tests__/releases.test.ts +++ b/apps/server/api/src/services/__tests__/releases.test.ts @@ -387,6 +387,41 @@ describe("fetchArtifact", () => { expect(await drain(b.body())).toBe(BINARY_PAYLOAD.bytes.byteLength); }); + it("does not make a joiner inherit a failed attempt — the next fetch is a fresh one", async () => { + // A rejection must not be sticky. The first fetch fails mid-download (the + // bytes disagree with the signed manifest); whoever joined it while it ran + // retries onto a new attempt rather than inheriting the old promise's + // rejection, so one failed read cannot blackhole the target for the rest + // of the index TTL. Both attempts fail here because the release is broken + // for everyone — what the test pins is the SECOND upstream fetch. + fake.assets.set(BINARY, new TextEncoder().encode("corrupted in transit")); + const p1 = fetchArtifact(TARGET); + const p2 = fetchArtifact(TARGET); // same tick: joins p1's attempt + await expect(p1).rejects.toThrow(/did not match the published digest/); + await expect(p2).rejects.toThrow(/did not match the published digest/); + expect(fake.assetFetches.get(BINARY)).toBe(2); + expect(existsSync(artifactPath(TARGET))).toBe(false); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.includes(".download-"))).toBe(false); + }); + + it("runs two cache:false fetches of one target side by side without touching each other", async () => { + // The stale-shelf arm of an "Update all": every tokened download asks for + // cache:false bytes, and those fetches deliberately do not coalesce (each + // one's temp cleanup is its own). They used to collide on a pid-keyed + // temp name — two writers in one file, and one caller's sweep unlinking + // another's in-flight copy. Each download now owns a unique temp. + writeFileSync(artifactPath(TARGET), "the operator's own"); + const [a, b] = await Promise.all([ + fetchArtifact(TARGET, { cache: false }), + fetchArtifact(TARGET, { cache: false }), + ]); + expect(fake.assetFetches.get(BINARY)).toBe(2); + expect(await drain(a.body())).toBe(BINARY_PAYLOAD.bytes.byteLength); + expect(await drain(b.body())).toBe(BINARY_PAYLOAD.bytes.byteLength); + expect(readFileSync(artifactPath(TARGET), "utf8")).toBe("the operator's own"); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.includes(".download-"))).toBe(false); + }); + it("keeps the operator's file when asked not to cache, and removes its own temp copy after the serve", async () => { // A file already sitting at the target path may be the operator's // hand-published binary: `cache: false` serves the release bytes from a diff --git a/apps/server/api/src/services/releases.ts b/apps/server/api/src/services/releases.ts index 6f012ad9d..ef52d0482 100644 --- a/apps/server/api/src/services/releases.ts +++ b/apps/server/api/src/services/releases.ts @@ -18,17 +18,18 @@ * pays the tiny reads at most once per TTL) and the update verb — never a * routine of their own. Binary BYTES are still lazy: nothing over ~1 KB * moves until a machine is actually being served. - * - **The bytes are streamed THROUGH, not staged and then served.** The - * alternative — download 80 MB, verify, then serve it — doubles the wait and - * holds an HTTP connection open with no bytes on it for as long as a minute, - * which is where reverse-proxy read timeouts live. Streaming hashes each - * chunk on its way past; a digest mismatch ERRORS the response mid-flight, - * so the node sees a truncated download and fails its own check. Nothing - * unverified is ever cached, and `install.sh` verifies the digest itself - * before the first `chmod +x` — that check is what makes streaming sound, - * and it predates this module. {@link downloadVerified} is the same - * hash-as-you-go discipline for the case with no client to stream to: the - * server's own update, which writes a file and then execs it. + * - **The bytes are downloaded IN FULL and verified before anything is + * served.** The old shape streamed the release through while hashing on the + * way past, and paid for it on 2026-10-23: the coalescer's shared live + * stream was already consumed for every joiner but the first, and the + * mid-flight error it produced was a truncated download nobody could tell + * from a broken network. Now {@link downloadVerified} writes the whole file + * to a temp path (hashing each chunk, bounded by `MAX_ARTIFACT_BYTES` as it + * goes), a mismatch fails the FETCH itself with the temp deleted, and the + * caller serves a fresh read of bytes that already passed. Nothing + * unverified is cached, and `install.sh` still verifies the digest itself + * before the first `chmod +x` — that check predates this module and stands + * as the node's own defense. * - **A file we fetched is ours to replace; a file the operator put there is * not.** Cached artifacts are recorded in a manifest with the release tag * they came from, and only manifest-recorded files are ever superseded or @@ -50,6 +51,7 @@ * Updates page already renders, because "why is there no update" is the * question the page exists to answer. */ +import { randomUUID } from "node:crypto"; import { mkdir, rename, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { @@ -850,16 +852,33 @@ export interface FetchArtifactOptions { * its own body from it. The download-before-serve order costs the first * caller's first byte (it now waits for the plane's full fetch from the * release source) and buys the whole fan-out case: see the incident note on - * {@link FetchedArtifact}. A `cache: false` caller does NOT join: its bytes - * live in a temp file whose cleanup belongs to that one caller, and sharing - * it would let one joiner's cleanup delete another's serve mid-flight. The - * mixed case (a `cache: true` fetch already running) still rides it: the - * first caller's cache flag governs the shared download, same as before. + * {@link FetchedArtifact}. A `cache: false` caller never shares, in either + * direction: its bytes live in a temp file whose cleanup is that one + * caller's, and coalescing it would let one joiner's cleanup delete + * another's serve mid-flight — so it neither joins the in-flight cached + * fetch nor becomes joinable (its own temp name is unique per download), and + * a `cache: true` caller arriving meanwhile starts a fresh shared download. + * Two parallel fetches of one target over one stale-shelf fan-out is the + * accepted cost of never touching the operator's file. */ export async function fetchArtifact(target: NodeTarget, options: FetchArtifactOptions = {}): Promise { if (options.cache === false) return fetchArtifactUncoordinated(target, false); const existing = inFlight.get(target); - if (existing) return existing; + if (existing) { + // A failed fetch is NOT sticky for its joiners. The settled attempt is + // deleted from the map the moment it rejects, but callers who joined it + // mid-flight were handed that same promise, and one failed read inside + // the 15-minute index TTL would otherwise reject every download of that + // target until the cache expires. A joiner whose attempt failed retries + // once onto whatever newer attempt is running (or starts one), which is + // what the route's "press Update again" copy promises the operator. + try { + return await existing; + } catch { + const retry = inFlight.get(target); + if (retry !== undefined && retry !== existing) return retry; + } + } const work = fetchArtifactUncoordinated(target, true).finally(() => inFlight.delete(target)); inFlight.set(target, work); return work; @@ -954,13 +973,24 @@ async function fetchArtifactUncoordinated(target: NodeTarget, cache: boolean): P * mid-way. A leftover partial must never look like a shelf file, and a * complete one left behind is 80 MB nobody asked the disk to keep. */ +/** Bun's own reader type for a file stream — annotated anywhere else and the + * lib types disagree with what `stream().getReader()` actually returns. */ +function fileReader(tmp: string) { + return Bun.file(tmp).stream().getReader(); +} + function drainedFile(tmp: string): ReadableStream { - const reader = Bun.file(tmp).stream().getReader(); const sweep = (): Promise => rm(tmp, { force: true }).catch(() => {}); + // The reader opens on the FIRST pull, not beside construction: a + // materialization that throws before anything consumed the stream would + // skip both the sweep and `controller.error`, leaving the temp file on disk + // and handing the caller a generic stream error. + let opened: ReturnType | null = null; return new ReadableStream({ async pull(controller) { try { - const { done, value } = await reader.read(); + if (opened === null) opened = fileReader(tmp); + const { done, value } = await opened.read(); if (done) { await sweep(); controller.close(); @@ -973,7 +1003,7 @@ function drainedFile(tmp: string): ReadableStream { } }, async cancel(reason) { - await reader.cancel(reason).catch(() => {}); + await opened?.cancel(reason).catch(() => {}); await sweep(); }, }); @@ -994,7 +1024,7 @@ async function discard(sink: { end: () => unknown } | null, tmp: string): Promis /** * The digest for a target this instance does not have, without downloading the * binary. `install.sh` asks for the sha AFTER the binary, so this is normally - * served from the file the streaming fetch just wrote; it exists for the order + * served from the cache entry the binary's fetch just wrote; it exists for the order * being the other way round — and for `POST /api/nodes/:id/update`, which * embeds it in the command. * @@ -1018,7 +1048,7 @@ export interface DownloadVerifiedInput { expectedDigest: string; /** Directory the temp file is written in — the binary's OWN directory, so the later rename is on one filesystem. */ destDir: string; - /** Base name of the temp file; `.download-` is appended. */ + /** Base name of the temp file; a unique `.download--` suffix is appended. */ destName: string; /** Progress, called per chunk. `total` is null when the server sent no length. */ onProgress?: (received: number, total: number | null) => void; @@ -1036,12 +1066,20 @@ export interface DownloadVerifiedInput { * promise is "these bytes are the ones that release published", and a file * this returns is not yet something anyone has decided to run. * + * Each download gets its OWN temp file: two concurrent uncoalesced fetches of + * one target (the stale-shelf arm of an "Update all", where every tokened + * download asks for `cache: false` bytes) used to collide on a name keyed + * only by pid, interleave two writers into one file, and let one caller's + * cleanup unlink another's in-flight copy — the same shape the server's own + * update hit before its job-slot claim made it single-flight (see + * `services/server-update.ts`). + * * @returns the temp file's path, on a digest match * @throws when the download fails, exceeds {@link MAX_ARTIFACT_BYTES}, or does * not match — in every case having deleted the partial file */ export async function downloadVerified(input: DownloadVerifiedInput): Promise { - const tmp = join(input.destDir, `${input.destName}.download-${process.pid}`); + const tmp = join(input.destDir, `${input.destName}.download-${process.pid}-${randomUUID()}`); // The server's own update passes a URL read from the release list — the // same untrusted field the lazy fetch pins (C12), hops included, so the // pin belongs here too: this download writes a file and the CALLER diff --git a/docs/security.md b/docs/security.md index b5bb7f207..e17eb2a74 100644 --- a/docs/security.md +++ b/docs/security.md @@ -1783,18 +1783,20 @@ posture: authenticated (cookie, unconsumed setup key, or `?update_token=`, per the credential list above), so this is not a way for an anonymous caller to make the plane fetch anything. -- **The bytes are streamed through and hashed on the way past**, against the - digest taken from the signed manifest's `assets` map: the binary's - `.sha256` sidecar is never fetched; since 2026-09-17 it is WRITTEN from - that digest (`releases.ts:648-660,717-720`), and the manifest itself is - verified before a single byte of the binary is requested. A mismatch ERRORS - the response mid-flight, so the node sees a truncated download; nothing - unverified is ever cached. The stream is bounded mid-flight by - `MAX_ARTIFACT_BYTES` (300 MB); an endless body cannot fill the disk - (`releases.ts:81,690-694,810-813`). What makes streaming sound rather than - merely fast is the check that was already there: `install.sh` verifies the - digest itself before the first `chmod +x`, so the node never trusts the - plane's word for what it received. +- **The bytes are downloaded IN FULL, to disk, and hashed while written**, + against the digest taken from the signed manifest's `assets` map: the + binary's `.sha256` sidecar is never fetched; since 2026-09-17 it is WRITTEN + from that digest, and the manifest itself is verified before a single byte + of the binary is requested (`downloadVerified` and the cache write in + `releases.ts`). A mismatch fails the FETCH itself: the uniquely-named temp is + deleted and nothing is cached, so a node's download is either complete + verified bytes or a clean refusal, never a truncated body; the mid-flight + error this replaced was the 2026-10-23 "Update all" incident's other face, + where one shared stream left every joiner but the first with an empty one. + The ingest is bounded by `MAX_ARTIFACT_BYTES` (300 MB) chunk by chunk; an + endless body cannot fill the disk. And the check that predates all of it + still stands: `install.sh` verifies the digest itself before the first + `chmod +x`, so the node never trusts the plane's word for what it received. - **Authenticity is the publisher's key, not the channel's** (2026-09-17). The fetch verifies `release-manifest.json.sig` against the compiled-in `RELEASE_PUBKEY` and takes the digest from the signed payload; a @@ -1840,8 +1842,9 @@ posture: one-time `?update_token=` that binds the digest the update command named (from the signed manifest), and is served release-coherently instead: the disk file only when it hashes to that digest, the verified release fetched - around it otherwise (streamed through, never overwriting the hand-published - file), and a 409 naming the remedy when the disk copy is stale and the + around it otherwise (served from a self-cleaning temp copy, never + overwriting the hand-published file), and a 409 naming the remedy when the + disk copy is stale and the release cannot be fetched (2026-09-21). - **The shelf answers for itself at serve time (2026-10-09).** "Disk wins, always" meant a plane that had crossed a protocol bump kept handing out an From 190dcd92b3ea3dfb52b9e3ada062ab5d7d1aec54 Mon Sep 17 00:00:00 2001 From: Theo Gravity Date: Fri, 23 Oct 2026 19:04:22 -0700 Subject: [PATCH 3/5] =?UTF-8?q?fix(update):=20round-2=20review=20=E2=80=94?= =?UTF-8?q?=20the=20old=20streaming=20story=20loses=20its=20last=20comment?= =?UTF-8?q?=20anchors,=20the=20GUI=20hint=20narrows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of the fan-out review found no MAJOR: the comments were the only survivors. downloads.route.ts's cache:false note, node-artifact-serve's rule 3 and broken-pair note, the shelf-serve test, and one fetchArtifact note still described bytes streaming through without landing on disk; they now say what happens, which is a uniquely-named temp copy the response removes. The response-body docblock moves off fileReader onto drainedFile, the function it describes, and the joiner comment stops overstating the pre-fix harm: without the retry, it was the downloads already coalesced onto the failed attempt that died, not every download for the TTL. The launchctl kickstart hint keys on the exact sentence its story is true of (Domain does not support specified action) instead of the bare word domain, so a find-service-in-domain failure keeps its status pointer; a new near-miss test pins the split. --- apps/node/agent/src/__tests__/service.test.ts | 26 +++++++++++++++++ apps/node/agent/src/service.ts | 11 ++++--- .../src/api/__tests__/downloads-route.test.ts | 4 +-- .../__tests__/downloads-shelf-serve.test.ts | 7 +++-- apps/server/api/src/api/downloads.route.ts | 9 +++--- .../server/api/src/lib/node-artifact-serve.ts | 10 +++---- apps/server/api/src/services/releases.ts | 29 ++++++++++--------- 7 files changed, 64 insertions(+), 32 deletions(-) diff --git a/apps/node/agent/src/__tests__/service.test.ts b/apps/node/agent/src/__tests__/service.test.ts index 25536069a..9a20ec7a1 100644 --- a/apps/node/agent/src/__tests__/service.test.ts +++ b/apps/node/agent/src/__tests__/service.test.ts @@ -1245,6 +1245,32 @@ describe("controlService", () => { expect(msgLine(r.err)).not.toContain("signed in at"); }); + // The GUI story claims someone needs to sign in at the display, which is + // true only of the "Domain does not support specified action" refusal. A + // near-miss that merely mentions a domain must not borrow the story. + test("darwin: a domain-word failure that is not the GUI-domain refusal gets the status pointer", async () => { + const s = stub({ + platform: "darwin", + respond: (cmd) => + cmd[0] === "plutil" + ? { code: 0, out: "true\n", err: "" } + : cmd[1] === "print" + ? { code: 0, out: "\tstate = running\n\tpid = 5150\n", err: "" } + : cmd[1] === "kickstart" + ? { + code: 3, + out: "", + err: 'Could not find service "dev.subshell.client" in domain for uid: 1000\n', + } + : { code: 0, out: "", err: "" }, + }); + s.files.set(PLIST, "AbandonProcessGroupRunAtLoad"); + const r = await controlService(s.deps, "restart"); + expect(r.code).toBe(1); + expect(msgLine(r.err)).toContain("subshell service status"); + expect(msgLine(r.err)).not.toContain("signed in at"); + }); + test("darwin: restart of a STOPPED job bootstraps it — kickstart needs a loaded job", async () => { const s = darwinServiceStub({ loaded: false }); const r = await controlService(s.deps, "restart"); diff --git a/apps/node/agent/src/service.ts b/apps/node/agent/src/service.ts index 1ffc4682a..dd5040559 100644 --- a/apps/node/agent/src/service.ts +++ b/apps/node/agent/src/service.ts @@ -1319,11 +1319,14 @@ export async function controlService( // specified action" (125) is the common one on a headless Mac: the // gui/ agent domain exists only while someone is signed in at the // machine's own display, so a daemon started over SSH sits outside - // launchd's reach and no `-k` will ever land. Say the cause and the - // way forward; everything else at least gets pointed at the status - // view, which carries launchd's own words. + // launchd's reach and no `-k` will ever land. Only that exact sentence + // earns the GUI-session story; other launchctl failures that merely + // mention a domain (the "find service ... in domain" family) get the + // status pointer instead, because the sign-in story would be false for + // them. Everything else at least gets pointed at the status view, + // which carries launchd's own words. const detail = cmdDetail(res); - const hint = /domain/i.test(detail) + const hint = /domain does not support specified action/i.test(detail) ? "; the user's launchd domain needs someone signed in at this machine's own display (its agents run in a GUI session, not an SSH one). Start the daemon by hand with `subshell run`, or sign in at the console and try again" : "; run `subshell service status` to see what launchd says about this job"; return errLine(`launchctl kickstart -k failed (exit ${res.code}): ${detail}${hint}`); diff --git a/apps/server/api/src/api/__tests__/downloads-route.test.ts b/apps/server/api/src/api/__tests__/downloads-route.test.ts index beef44c36..272e20be1 100644 --- a/apps/server/api/src/api/__tests__/downloads-route.test.ts +++ b/apps/server/api/src/api/__tests__/downloads-route.test.ts @@ -1577,8 +1577,8 @@ describe("/api/downloads/node/* — the lazy fetch", () => { it("an update token serves the release bytes past a stale disk artifact, and never overwrites it", async () => { // Release-coherent serving (2026-09-21): the token's download is measured // against the digest the update command named, so a stale hand-published - // file on disk is fetched around, streamed through, and left byte for byte - // alone. + // file on disk is fetched around into its own temp copy and left byte + // for byte alone. setReleaseUrlForTests(`${release.url}/releases`); resetReleaseCacheForTests(); resetUpdateTokensForTests(); diff --git a/apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts b/apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts index 9244f0cdd..c5328b657 100644 --- a/apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts +++ b/apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts @@ -404,11 +404,12 @@ describe("/api/downloads/node — the shelf answers for itself", () => { expect(sha.status).toBe(404); }); - it("an inconsistent shelf with a compatible release: the release streams PAST the broken pair", async () => { + it("an inconsistent shelf with a compatible release: the release serves PAST the broken pair", async () => { // The shelf's bytes and its sidecar deny each other, so the shelf loses // precedence entirely (rule 1's broken cell) — and because these bytes - // are the OPERATOR's (no ledger entry), the release is streamed through - // without touching them: the operator republishes, the server does not. + // are the OPERATOR's (no ledger entry), the release is served from its + // own temp copy without touching them: the operator republishes, the + // server does not. const shelf = plantShelf(shelfScript(MIN_NODE_VERSION, NODE_PROTOCOL_VERSION)); writeFileSync(`${binaryPath}.sha256`, `${"f".repeat(64)}\n`); const res = await get(`/api/downloads/node/${TARGET}`); diff --git a/apps/server/api/src/api/downloads.route.ts b/apps/server/api/src/api/downloads.route.ts index c735b7044..a16a33059 100644 --- a/apps/server/api/src/api/downloads.route.ts +++ b/apps/server/api/src/api/downloads.route.ts @@ -241,18 +241,19 @@ export const downloadsRoutes = new Elysia({ prefix: "/api/downloads" }).use(apiM try { // Absent disk: cache the verified bytes, as the installer lazy fetch // always has (same `.fetched.json` semantics). Present but different: - // stream the release bytes THROUGH only, because the file there may be - // the operator's hand-published binary and is never overwritten here. + // serve the release bytes from their own temp copy, because the file + // there may be the operator's hand-published binary and is never + // overwritten here. const fetched = await fetchArtifact(params.target, { cache: onDisk === null }); // The release the source serves NOW must be the release the COMMAND // named. The index's 15-minute TTL can carry a newer release past an - // outstanding token, and streaming those bytes against the command's + // outstanding token, and serving those bytes against the command's // digest would land the node in exactly the refusal this path exists to // prevent, one download later. So the comparison happens before the // first byte is served — and since the fetch verifies the whole file // before this line runs (the "Update all" fan-out fix of 2026-10-23), a // mismatch now stops with NO byte served: `dispose` removes the temp - // copy the stream-through fetch wrote, and where the fetch cached, the + // copy the fetch wrote, and where the fetch cached, the // newer release stays on the shelf because it IS the release an // installer would serve next — the retry fast-paths on it. if (fetched.digest !== expected) { diff --git a/apps/server/api/src/lib/node-artifact-serve.ts b/apps/server/api/src/lib/node-artifact-serve.ts index dc2a98794..70f46b627 100644 --- a/apps/server/api/src/lib/node-artifact-serve.ts +++ b/apps/server/api/src/lib/node-artifact-serve.ts @@ -41,8 +41,8 @@ import { artifactPath, artifactSha, artifactStat, diskArtifactSha256Cached } fro * newest build this server can talk to, which is the whole promise the * one-liner makes. Bytes this instance fetched (`.fetched.json` says so, * digest to the digest) are REPLACED by the cache; bytes it did not are - * streamed past, never overwritten, same rule the update route learned on - * 2026-09-21; + * served past from their own temp copy, never overwritten, same rule the + * update route learned on 2026-09-21; * 4. no compatible release at all → the shelf serves only while it can * actually connect (its reported protocol matches, its version clears the * floor); an unmeasurable shelf serves (unknown is not a verdict); a @@ -313,8 +313,8 @@ export async function resolveNodeServe(target: NodeTarget): Promise { const sweep = (): Promise => rm(tmp, { force: true }).catch(() => {}); // The reader opens on the FIRST pull, not beside construction: a @@ -1057,9 +1057,10 @@ export interface DownloadVerifiedInput { /** * Download a release asset to a temp file, hashing as it goes. * - * The same discipline {@link fetchArtifact} applies, for the case with no - * client to stream to: the server's own update writes a file and then EXECS - * it, so a mismatch has to leave nothing behind rather than error a response. + * The same discipline {@link fetchArtifact} applies, for the caller that + * writes a file and then EXECS it (the server's own update): a mismatch has + * to leave nothing behind, because a file someone might run is a worse + * leftover than a failed serve. * * It never chmods. Making a downloaded file executable is the caller's act and * belongs beside the caller's own version probe — this function's whole From 7b77739dda5fbd7327b0c65d62d66c3015072687 Mon Sep 17 00:00:00 2001 From: Theo Gravity Date: Fri, 23 Oct 2026 19:15:38 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix(update):=20round-3=20review=20=E2=80=94?= =?UTF-8?q?=20retire=20the=20last=20two=20uses=20of=20the=20old=20stream?= =?UTF-8?q?=20word?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One test comment still said the operator's bytes are streamed PAST, and the air-gapped arm said a refusal beats streaming to a node. The first names a mechanism the fan-out fix removed (those bytes now serve from a temp copy); the second never had a stream to name (that arm fetches nothing). Comments only. --- apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts | 2 +- apps/server/api/src/api/downloads.route.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts b/apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts index c5328b657..37a3bca18 100644 --- a/apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts +++ b/apps/server/api/src/api/__tests__/downloads-shelf-serve.test.ts @@ -198,7 +198,7 @@ describe("/api/downloads/node — the shelf answers for itself", () => { const shelf = plantShelf(shelfScript(MIN_NODE_VERSION, NODE_PROTOCOL_VERSION)); const res = await get(`/api/downloads/node/${TARGET}`); expect(await res.text()).toBe(NEWBODY); - // Operator bytes, no ledger entry: streamed PAST, never overwritten. + // Operator bytes, no ledger entry: served PAST from a temp copy, never overwritten. expect(readFileSync(binaryPath, "utf8")).toBe(shelf); const sha = await get(`/api/downloads/node/${TARGET}.sha256`); diff --git a/apps/server/api/src/api/downloads.route.ts b/apps/server/api/src/api/downloads.route.ts index a16a33059..0f83242c7 100644 --- a/apps/server/api/src/api/downloads.route.ts +++ b/apps/server/api/src/api/downloads.route.ts @@ -227,7 +227,7 @@ export const downloadsRoutes = new Elysia({ prefix: "/api/downloads" }).use(apiM // The air-gapped plane serves only what it holds. Nothing held: the // same 404 the installer path answers, which the node reports as a // download failure. Something held but not the release: the incoherent - // offer, refused with the remedy instead of being streamed to a node + // offer, refused with the remedy instead of being served to a node // that would refuse it after the download. return status( onDisk === null ? 404 : 409, From 3d35c062f4d2f4c7531b574165d9bf529ff6544c Mon Sep 17 00:00:00 2001 From: Theo Gravity Date: Fri, 23 Oct 2026 19:28:30 -0700 Subject: [PATCH 5/5] =?UTF-8?q?fix(update):=20round-4=20review=20=E2=80=94?= =?UTF-8?q?=20crash=20strays=20get=20swept,=20stale=20comment=20anchors=20?= =?UTF-8?q?go,=20vacuous=20temp=20assertions=20made=20honest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit downloadVerified's temp name is unique per attempt now, which is what the fan-out needs and what a SIGKILL turns into a new orphan every crash: no in-process path can clean what the kill interrupted, so the disk was one crash-loop away from accumulating artifact-sized strays with no reclaim path. Each download now sweeps matching strays older than an hour before it writes (the mtime of a live download stays fresh, so the age bound keeps a concurrent fetch's copy out of reach), and a new test plants an old stray beside a young one and expects only the corpse to go. Two comment anchors still described the retired serve-through shape in the present tense (the ledger docblock, the uncoalesced arm's label); they now match what the code does. Four assertions in the downloadVerified suite hunted the old pid-only temp name, which nothing writes anymore, and so could no longer fail; they scan for prefix strays instead, which is the honest form. The digest-failure phrase is a named constant so the artifact-log branch cannot silently drift from what downloadVerified throws, and the fan-out test's title counts its own tokens. --- apps/server/api/docs/node-artifacts.md | 4 ++ .../src/api/__tests__/downloads-route.test.ts | 2 +- .../src/services/__tests__/releases.test.ts | 43 ++++++++++---- apps/server/api/src/services/releases.ts | 58 ++++++++++++++++--- 4 files changed, 87 insertions(+), 20 deletions(-) diff --git a/apps/server/api/docs/node-artifacts.md b/apps/server/api/docs/node-artifacts.md index d33acbbf5..7c24724db 100644 --- a/apps/server/api/docs/node-artifacts.md +++ b/apps/server/api/docs/node-artifacts.md @@ -66,6 +66,10 @@ only when it hashes to it, and anything else is fetched around: an absent disk file is cached exactly as the installer lazy fetch always did, while a present-but-different file is served from a temp copy and never overwritten (the hand-published-binary rule holds; the temp goes when the response ends). +A download killed outright strands its uniquely-named temp, because no +in-process path cleans what a SIGKILL interrupted, so each download sweeps +strays older than an hour first; the age bound is what makes that safe beside +a concurrent fetch's live copy, whose mtime stays fresh while it is written. Since the 2026-10-23 fan-out fix the fetch lands COMPLETE and verified on disk before the route hands anything out: the release-vs-token digest comparison therefore runs with no byte served, a mismatch answers 409, and an diff --git a/apps/server/api/src/api/__tests__/downloads-route.test.ts b/apps/server/api/src/api/__tests__/downloads-route.test.ts index 272e20be1..563a1607b 100644 --- a/apps/server/api/src/api/__tests__/downloads-route.test.ts +++ b/apps/server/api/src/api/__tests__/downloads-route.test.ts @@ -1646,7 +1646,7 @@ describe("/api/downloads/node/* — the lazy fetch", () => { } }); - it("serves TWO simultaneous update downloads with full bytes each — the fan-out incident", async () => { + it("serves THREE simultaneous update downloads with full bytes each — the fan-out incident", async () => { // The 2026-10-23 "Update all" incident, at the route the fan-out actually // hits: every node the operator pressed was answered by the SAME // in-flight fetch, and the shared stream let only the first response diff --git a/apps/server/api/src/services/__tests__/releases.test.ts b/apps/server/api/src/services/__tests__/releases.test.ts index 7ce261032..fdeecc86c 100644 --- a/apps/server/api/src/services/__tests__/releases.test.ts +++ b/apps/server/api/src/services/__tests__/releases.test.ts @@ -1,5 +1,5 @@ import { afterAll, afterEach, beforeEach, describe, expect, it } from "bun:test"; -import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, utimesSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { MIN_NODE_VERSION, @@ -23,6 +23,7 @@ import { releaseSeams, resetReleaseCacheForTests, resolveReleases, + STALE_DOWNLOAD_MAX_AGE_MS, setReleaseUrlForTests, signedAssetDigest, } from "@/services/releases.js"; @@ -547,7 +548,7 @@ describe("egress pin (round-3 sweep C12)", () => { }), ).rejects.toThrow(/is neither a GitHub release host nor the configured release source's host/); expect(hits).toBe(0); - expect(existsSync(join(NODE_ARTIFACTS_DIR, `subshell-server.download-${process.pid}`))).toBe(false); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.startsWith("subshell-server.download-"))).toBe(false); } finally { attacker.stop(true); } @@ -698,20 +699,18 @@ describe("downloadVerified", () => { it("deletes the partial file and throws on a mismatch", async () => { // The server's own update EXECS what this writes, so a mismatch must leave - // nothing behind rather than merely report. - let path = ""; + // nothing behind rather than merely report. The temp name carries a uuid + // now, so the honest check is "no stray of this prefix survives", not one + // hand-guessed path. await expect( downloadVerified({ url: assetUrl(BINARY), expectedDigest: "0".repeat(64), destDir: NODE_ARTIFACTS_DIR, destName: "subshell-server", - onProgress: () => { - path = join(NODE_ARTIFACTS_DIR, `subshell-server.download-${process.pid}`); - }, }), ).rejects.toThrow(/did not match the published digest/); - expect(existsSync(path)).toBe(false); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.startsWith("subshell-server.download-"))).toBe(false); }); it("reports progress as the bytes arrive", async () => { @@ -737,7 +736,7 @@ describe("downloadVerified", () => { destName: "subshell-server", }), ).rejects.toThrow(/is neither a GitHub release host nor the configured release source's host/); - expect(existsSync(join(NODE_ARTIFACTS_DIR, `subshell-server.download-${process.pid}`))).toBe(false); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.startsWith("subshell-server.download-"))).toBe(false); }); it("throws and keeps nothing when the asset is not there", async () => { @@ -749,6 +748,30 @@ describe("downloadVerified", () => { destName: "subshell-server", }), ).rejects.toThrow(/answered 404/); - expect(existsSync(join(NODE_ARTIFACTS_DIR, `subshell-server.download-${process.pid}`))).toBe(false); + expect(readdirSync(NODE_ARTIFACTS_DIR).some((name) => name.startsWith("subshell-server.download-"))).toBe(false); + }); + + it("sweeps a crashed attempt's stray before writing, and leaves a young one alone", async () => { + // A SIGKILL between the write and the serve strands a uniquely-named + // temp: no in-process path cleans what the kill interrupted, and because + // the name is per-attempt, every crash would pile up another artifact's + // worth of bytes forever. Each download reclaims strays past the age + // bound. A YOUNG stray survives, because it could be a concurrent + // fetch's live copy — that is what the age bound is for. + const stray = join(NODE_ARTIFACTS_DIR, "subshell-server.download-99999-deadbeef"); + const young = join(NODE_ARTIFACTS_DIR, "subshell-server.download-99998-live"); + writeFileSync(stray, "orphaned from a killed process"); + writeFileSync(young, "someone could be mid-write"); + const old = new Date(Date.now() - STALE_DOWNLOAD_MAX_AGE_MS - 60_000); + utimesSync(stray, old, old); + const { digest } = payload("a convincing binary"); + await downloadVerified({ + url: assetUrl(BINARY), + expectedDigest: digest, + destDir: NODE_ARTIFACTS_DIR, + destName: "subshell-server", + }); + expect(existsSync(stray)).toBe(false); + expect(existsSync(young)).toBe(true); }); }); diff --git a/apps/server/api/src/services/releases.ts b/apps/server/api/src/services/releases.ts index 527545a94..75ed7c5bc 100644 --- a/apps/server/api/src/services/releases.ts +++ b/apps/server/api/src/services/releases.ts @@ -52,7 +52,7 @@ * question the page exists to answer. */ import { randomUUID } from "node:crypto"; -import { mkdir, rename, rm, writeFile } from "node:fs/promises"; +import { mkdir, readdir, rename, rm, stat, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { MIN_NODE_VERSION, @@ -85,6 +85,16 @@ const METADATA_TIMEOUT_MS = 15_000; */ export const MAX_ARTIFACT_BYTES = 300 * 1024 * 1024; +/** How old a `.download-*` stray must be before a fresh download sweeps it. + * One hour clears any honest download at 300 MB/s-and-change, so a stray + * older than this is a crashed attempt's corpse, not a live write. @internal */ +export const STALE_DOWNLOAD_MAX_AGE_MS = 60 * 60 * 1000; + +/** The digest-failure phrase, shared because {@link fetchArtifactUncoordinated} + * pattern-matches what {@link downloadVerified} throws to keep the artifact + * log line truthful. Tests pin the shipped string with a literal regex. */ +const DIGEST_MISMATCH_PHRASE = "did not match the published digest"; + /** * What this instance fetched, and from which release. * @@ -728,8 +738,9 @@ async function writeManifest(manifest: FetchedManifest): Promise { * The recorded digest is the whole point: a caller compares it against the * bytes ACTUALLY on disk, and only a match makes the file "ours" — something * this instance may replace. An operator who edited the file since our fetch - * (or who put it there) reclaimed it, and the stream-past rule of the update - * route applies: serve the release through, leave the bytes alone. + * (or who put it there) reclaimed it, and the serve-past rule of the update + * route applies: serve the release from its own temp copy, leave the bytes + * alone. */ export async function fetchedLedgerEntry(target: NodeTarget): Promise<{ tag: string; digest: string } | null> { const entry = (await readManifest())[target]; @@ -936,7 +947,7 @@ async function fetchArtifactUncoordinated(target: NodeTarget, cache: boolean): P // Keep the digest failure in the artifact log line the streaming design // emitted: the release identity lives here, and `downloadVerified` is // release-agnostic by contract. - if (error instanceof Error && error.message.includes("did not match the published digest")) { + if (error instanceof Error && error.message.includes(DIGEST_MISMATCH_PHRASE)) { getLogger().warn(`node artifacts: ${names.binary} from ${release.tag} failed its digest check: ${error.message}`); } throw error; @@ -954,7 +965,7 @@ async function fetchArtifactUncoordinated(target: NodeTarget, cache: boolean): P return { digest: expected, tag: release.tag, body: () => file.stream(), dispose: async () => {} }; } - // Stream-through: the file at the artifact path was not ours to overwrite + // Served-from-temp: the file at the artifact path was not ours to overwrite // (it may be the operator's hand-published binary), so these bytes live in // the temp copy only, and that copy is deleted when the response ends. // Not coalesced (see {@link fetchArtifact}) precisely because that cleanup @@ -1079,8 +1090,39 @@ export interface DownloadVerifiedInput { * @throws when the download fails, exceeds {@link MAX_ARTIFACT_BYTES}, or does * not match — in every case having deleted the partial file */ +/** + * Remove strays a crashed attempt left behind: no in-process path can clean + * what a SIGKILL interrupted, and since the temp name is unique per attempt + * (so concurrent fetches cannot collide), each crash leaves a NEW name rather + * than one bounded slot to reuse. Every download therefore sweeps matching + * strays older than {@link STALE_DOWNLOAD_MAX_AGE_MS} before it writes. Age + * is the whole safety story: a live download of anything under the size cap + * is minutes old, so it is never a stray. Best effort by design: a directory + * we cannot read, or a file another writer just touched, costs a retry later, + * never a failed download. + */ +async function sweepStaleDownloads(destDir: string, prefix: string, keep: string): Promise { + const staleBefore = Date.now() - STALE_DOWNLOAD_MAX_AGE_MS; + let names: string[]; + try { + names = await readdir(destDir); + } catch { + return; + } + for (const name of names) { + if (!name.startsWith(prefix) || name === keep) continue; + const path = join(destDir, name); + const info = await stat(path).catch(() => null); + if (info !== null && info.mtimeMs < staleBefore) { + await rm(path, { force: true }).catch(() => {}); + } + } +} + export async function downloadVerified(input: DownloadVerifiedInput): Promise { - const tmp = join(input.destDir, `${input.destName}.download-${process.pid}-${randomUUID()}`); + const tmpName = `${input.destName}.download-${process.pid}-${randomUUID()}`; + const tmp = join(input.destDir, tmpName); + await sweepStaleDownloads(input.destDir, `${input.destName}.download-`, tmpName); // The server's own update passes a URL read from the release list — the // same untrusted field the lazy fetch pins (C12), hops included, so the // pin belongs here too: this download writes a file and the CALLER @@ -1112,9 +1154,7 @@ export async function downloadVerified(input: DownloadVerifiedInput): Promise {}); - throw new Error( - `${input.url} did not match the published digest (expected ${input.expectedDigest}, got ${actual})`, - ); + throw new Error(`${input.url} ${DIGEST_MISMATCH_PHRASE} (expected ${input.expectedDigest}, got ${actual})`); } return tmp; }