From 8c450d3489189f6f4810aeb63ce9b7facad0cf7d Mon Sep 17 00:00:00 2001 From: devswha Date: Mon, 9 Nov 2026 03:08:43 +0900 Subject: [PATCH 1/5] fix(machines): cancel stops a follow-up update there, only the release shown, one job per PC as ssh resolves it The rest of #3367 (items 2-5); see the PR for the full description. --- changelog.d/pc-install-followups.md | 7 ++ scripts/conventions.test.ts | 1 + server/api.contract.test.ts | 15 +++- server/dispatch.ts | 4 +- server/machine-api.ts | 5 ++ server/machine-own-update.test.ts | 58 +++++++++++++++- server/machines.ts | 83 ++++++++++++++++++---- server/pc-install-routes.test.ts | 104 +++++++++++++++++++++++++--- server/pc-install.ts | 7 +- server/pc-update-all.test.ts | 5 +- server/pc-update-cancel.test.ts | 35 ++++++++++ server/pc-update.ts | 54 +++++++++++++-- server/ssh-address.ts | 99 +++++++++++++++++++------- server/supervisor.ts | 3 +- server/update-api.test.ts | 20 ++++++ server/update-api.ts | 14 +++- server/update-ipc.test.ts | 2 + server/updater.test.ts | 48 +++++++++++++ server/updater.ts | 39 ++++++++++- shared/protocol.ts | 3 + shared/update.ts | 12 +++- 21 files changed, 546 insertions(+), 72 deletions(-) create mode 100644 changelog.d/pc-install-followups.md create mode 100644 server/pc-update-cancel.test.ts diff --git a/changelog.d/pc-install-followups.md b/changelog.d/pc-install-followups.md new file mode 100644 index 0000000000..3244468522 --- /dev/null +++ b/changelog.d/pc-install-followups.md @@ -0,0 +1,7 @@ +### Fixed +- Cancelling an update of another PC asks that PC's own Ovis to stop it, while it still checks or builds, + and says it stopped only once that PC confirms it; otherwise (an older Ovis, or one already restarting) + it says the update may still finish there, never that it ended. +- An install over SSH that finds an older Ovis there updates it only to the release shown (this app's). +- One install or setup per PC holds even when ssh cannot resolve an address, and resolving addresses no + longer holds the server up. diff --git a/scripts/conventions.test.ts b/scripts/conventions.test.ts index 4a44ef697f..f7f987e0b0 100644 --- a/scripts/conventions.test.ts +++ b/scripts/conventions.test.ts @@ -130,6 +130,7 @@ describe("demo transport", () => { */ const DEMO_404: Record = { "/api/account/enroll": "account linking: no screen of the app asks for it yet (the account screens come with their own demo answers)", + "/api/updates/cancel": "a connection server asks a PC's own Ovis to stop its update; the demo's browser never does", "/api/account/introduce": "account linking: no screen of the app asks for it yet (the account screens come with their own demo answers)", "/api/account/introduction": "account linking: no screen of the app asks for it yet (the account screens come with their own demo answers)", "/api/account/link": "account linking: no screen of the app asks for it yet (the account screens come with their own demo answers)", diff --git a/server/api.contract.test.ts b/server/api.contract.test.ts index 73e327c7dd..d17cd28470 100644 --- a/server/api.contract.test.ts +++ b/server/api.contract.test.ts @@ -850,9 +850,9 @@ describe("update API", () => { const protectedState = mkdtempSync(join(tmpdir(), "herdr-update-auth-")); const protectedServer = createServer({ port: 0, stateDir: protectedState, token: "test-update-token" }); try { - for (const path of ["/api/updates", "/api/updates/notes", "/api/updates/installed", "/api/updates/check", "/api/updates/install"]) { + for (const path of ["/api/updates", "/api/updates/notes", "/api/updates/installed", "/api/updates/check", "/api/updates/install", "/api/updates/cancel"]) { const response = await fetch(`http://localhost:${protectedServer.port}${path}`, { - method: path === "/api/updates/check" || path === "/api/updates/install" ? "POST" : "GET", headers: { "x-herdr-update": "1" }, + method: path === "/api/updates/check" || path === "/api/updates/install" || path === "/api/updates/cancel" ? "POST" : "GET", headers: { "x-herdr-update": "1" }, }); expect(response.status).toBe(401); } @@ -891,6 +891,14 @@ describe("update API", () => { expect(requests).toEqual([]); expect((await call("/api/updates/install", `Bearer ${bridge}`, JSON.stringify({ expected_revision: offered }))).status).toBe(202); expect(requests).toEqual([{ command: "install", expected_revision: offered }]); + // and stop that install while it builds (POST /api/updates/cancel), once its supervisor can + status.phase = "building"; + expect((await call("/api/updates/cancel", `Bearer ${bridge}`, "")).status).toBe(409); + status.install_cancellable = true; + expect((await call("/api/updates/cancel", `Bearer ${bridge}`, "")).status).toBe(202); + expect(requests.at(-1)).toEqual({ command: "cancel" }); + status.phase = "idle"; + requests.splice(1); // a wrong bridge token is refused like any other, and the token opens no other update route expect((await call("/api/updates", `Bearer ${"c".repeat(64)}`)).status).toBe(401); for (const path of ["/api/updates/notes", "/api/updates/installed", "/api/updates/check", "/api/herdr/update"]) expect((await call(path, `Bearer ${bridge}`)).status).toBe(401); @@ -900,7 +908,8 @@ describe("update API", () => { }); const install = JSON.stringify({ expected_revision: offered }); for (const [method, path, body] of [["GET", "/api/updates?anything=1"], ["HEAD", "/api/updates"], ["POST", "/api/updates", install], ["DELETE", "/api/updates"], - ["GET", "/api/updates/install"], ["PUT", "/api/updates/install", install], ["POST", "/api/updates/install?anything=1", install]] as const) { + ["GET", "/api/updates/install"], ["PUT", "/api/updates/install", install], ["POST", "/api/updates/install?anything=1", install], + ["GET", "/api/updates/cancel"], ["POST", "/api/updates/cancel?anything=1", ""]] as const) { expect({ method, path, status: (await send(method, path, body)).status }).toEqual({ method, path, status: 401 }); } expect(requests).toHaveLength(1); diff --git a/server/dispatch.ts b/server/dispatch.ts index 0d1e55ea4f..c3224c912b 100644 --- a/server/dispatch.ts +++ b/server/dispatch.ts @@ -163,7 +163,7 @@ export async function handleRequest(request: Request, ctx: ServerContext): Promi // replaced (server/machines.ts ownOffer). An install still names the offered revision and the // custom header (update-api.ts), and whoever holds the token already drives every pane here. Only those two // requests, each by its own method and with no query: the token opens nothing else of these routes (#2054) - const bridgeUpdatePath = url.search === "" && (pathname === "/api/updates" ? request.method === "GET" : pathname === "/api/updates/install" && request.method === "POST"); + const bridgeUpdatePath = url.search === "" && (pathname === "/api/updates" ? request.method === "GET" : (pathname === "/api/updates/install" || pathname === "/api/updates/cancel") && request.method === "POST"); // the same server runs the move to herdr-core here for the person on its side (docs/runtime-move.md, // "On another PC"); each move route still asks for the app's own header and no query (updateRequestAllowed) const bridgeMovePath = RUNTIME_MOVE_ROUTE.test(pathname); @@ -491,7 +491,7 @@ export async function handleRequest(request: Request, ctx: ServerContext): Promi // installing an update on this PC (Ovis or herdr; a check installs too where OVIS_AUTO_UPDATE=1) is the owner's, as changing its PCs is (machine-api.ts): from any device // he signed in or paired himself. The connection server's bridge token asks its own install by revision (bridgeUpdatePath) - if (request.method === "POST" && (pathname === "/api/updates/install" || pathname === "/api/updates/check" || pathname === "/api/herdr/update") && !ownerAccess(access) && !(bridgeAuthorized && bridgeUpdatePath)) { + if (request.method === "POST" && (pathname === "/api/updates/install" || pathname === "/api/updates/cancel" || pathname === "/api/updates/check" || pathname === "/api/herdr/update") && !ownerAccess(access) && !(bridgeAuthorized && bridgeUpdatePath)) { setRequestRoute(pathname); return jsonResponse({ error: { code: "forbidden", message: "Only the owner's own signed-in or paired devices can update this PC, from this app's own page." } }, 403); } diff --git a/server/machine-api.ts b/server/machine-api.ts index c1f35976da..07c94d7d3d 100644 --- a/server/machine-api.ts +++ b/server/machine-api.ts @@ -330,6 +330,8 @@ export async function handleMachineRequest(request: Request, manager: MachineMan setRequestRoute("/api/machines/setup"); const body = await request.json() as SetupRequest; // a setup that updates what runs there is an update of that PC: the owner's only, as above + // the addresses its one-job-per-PC check compares, resolved first (ssh -G, off the event loop, 3 s at most) + await manager.resolveFor(body); return jsonResponse(manager.setup(body), 202); } if (parts.length === 2) { @@ -362,6 +364,7 @@ export async function handleMachineRequest(request: Request, manager: MachineMan if (request.method !== "POST") return fail("method_not_allowed", "Use POST", 405); const body: unknown = await request.json().catch(() => null); if (!isJsonObject(body)) return fail("invalid_body", "Expected an SSH address, or the machine_id of a PC added here", 400); + await manager.resolveFor(body); try { return jsonResponse(manager.install(body as unknown as PcInstallRequest) satisfies PcInstallJob, 202); } catch (error) { if (error instanceof PcUpdateRefused) return fail("install_unavailable", error.message, 409); @@ -413,6 +416,8 @@ export async function handleMachineRequest(request: Request, manager: MachineMan const body: unknown = text.trim() === "" ? {} : (() => { try { return JSON.parse(text); } catch { return null; } })(); const shown = isJsonObject(body) ? body.revision : undefined; if (!isJsonObject(body) || shown !== undefined && typeof shown !== "string") return fail("invalid_body", "Expected nothing, or { revision } of the offer shown", 400); + // its address and those of the running jobs, resolved first, as for a setup + await manager.resolveFor({ machine_id: id }); return jsonResponse(manager.updateBridge(id, false, shown as string | undefined), 202); } if (parts.length === 1 && request.method === "PATCH") { diff --git a/server/machine-own-update.test.ts b/server/machine-own-update.test.ts index 084949f73a..80ad3c6974 100644 --- a/server/machine-own-update.test.ts +++ b/server/machine-own-update.test.ts @@ -42,6 +42,8 @@ interface Remote { bundle?: string; /** what its status says once asked to install, one answer per read (the last repeats); it restarts on the new version after `restartAfter` reads, never without */ afterInstall?: { statuses: Partial[]; restartAfter?: number }; + /** the install request's answer waits for gate (reached once it arrived) */ + installHeld?: { reached(): void; gate: Promise }; /** what the PC holds of this app's bridge runtime, as its inspection prints it; an older one when unset */ runtime?: "bundle-ready" | "bundle-older" | null; /** a bridge this app started serves the socket, not the PC's own Ovis */ @@ -112,7 +114,7 @@ function pc(remote: Remote, options: { registered?: boolean; autoUpdate?: boolea expect(port).toBe(REGISTERED_PORT); // what answers there tells which address it is: the PC's own, or another its dialog was given const at = this.target.destination; - relays.push(Bun.serve({ hostname: "127.0.0.1", port: local, async fetch(request, server) { + const relay = Bun.serve({ hostname: "127.0.0.1", port: local, async fetch(request, server) { const path = new URL(request.url).pathname; requests.push(`${request.method} ${path}`); if (request.headers.get("authorization") !== `Bearer ${descriptor.token}`) return new Response("unauthorized", { status: 401 }); @@ -129,8 +131,12 @@ function pc(remote: Remote, options: { registered?: boolean; autoUpdate?: boolea return before === null ? new Response("unavailable", { status: 503 }) : Response.json({ ...remote.status, ...before }); } if (path === "/api/updates" && request.method === "GET") return remote.status ? Response.json(remote.status) : Response.json({ error: { code: "unauthorized", message: "token required" } }, { status: 401 }); + // asked to stop the install it builds: it stops, and says so + if (path === "/api/updates/cancel" && request.method === "POST") { if (remote.afterInstall) remote.afterInstall = { statuses: [{ phase: "error", error_code: "update_cancelled", error: "The update was cancelled before it was installed." }] }; return new Response(null, { status: 202 }); } if (path === "/api/updates/install" && request.method === "POST") { expect(request.headers.get("x-herdr-update")).toBe("1"); + // its answer held back, as when it is still on its way + if (remote.installHeld) { remote.installHeld.reached(); await remote.installHeld.gate; } installs.push(await request.json()); const answer = remote.install?.[Math.min(installAttempts++, remote.install.length - 1)] ?? { status: 202 }; // busy: an install of its own runs there already @@ -156,7 +162,8 @@ function pc(remote: Remote, options: { registered?: boolean; autoUpdate?: boolea if (path === "/api/session") return Response.json({ snapshot: rosters.get(at) ?? { panes: [], workspaces: [], fixture_at: at, finish_epoch: `run@${at}` } }); if (path === "/ws" && server.upgrade(request, { data: undefined })) return; return new Response("not found", { status: 404 }); - }, websocket: { open(ws) { if (away) ws.close(); else sockets.add(ws); }, close(ws) { sockets.delete(ws); }, message() {} } })); + }, websocket: { open(ws) { if (away) ws.close(); else sockets.add(ws); }, close(ws) { sockets.delete(ws); }, message() {} } }); + relays.push(relay); }; /** its own Ovis stops, as a restart, its update, a crash or a reboot stops it, and later starts again as another process */ const ovis = { @@ -611,6 +618,53 @@ describe("a PC whose own Ovis is older than this app", () => { expect(connected.ovis_update).toEqual({ how: "own", current_version: "0.5.0", latest_version: "0.5.1", updatable: true, reason: null, revision: OFFERED }); }); + it("never asks that PC to stop its update when the PC is removed here: only a cancel asked for does", async () => { + const { manager, requests, until } = pc({ status: managedOffer({ install_cancellable: true }), afterInstall: { statuses: [{ phase: "building", step: "build" }] } }, { ownUpdate: { pollMs: 5 } }); + const building = await until((machine) => /updating itself/.test(machine.updating?.step ?? "")); + const jobs = (manager as unknown as { jobs: Map }> }).jobs; + manager.remove(building.id); + expect([...jobs.values()].some((job) => job.cancelling !== undefined)).toBe(false); + expect(requests).not.toContain("POST /api/updates/cancel"); + }); + + it("never asks that PC to stop its update when this server stops (its own restart, say): only a cancel asked for here does", async () => { + const { manager, requests, until } = pc({ status: managedOffer({ install_cancellable: true }), afterInstall: { statuses: [{ phase: "building", step: "build" }] } }, { ownUpdate: { pollMs: 5 } }); + await until((machine) => /updating itself/.test(machine.updating?.step ?? "")); + const jobs = (manager as unknown as { jobs: Map }> }).jobs; + manager.stop(); + // no request to stop it was started for any job (it would be under way by now: cancelJob starts it at once) + expect([...jobs.values()].some((job) => job.cancelling !== undefined)).toBe(false); + expect(requests).not.toContain("POST /api/updates/cancel"); + }); + + it("asks that PC to stop an update cancelled while the answer to its install request is still on its way", async () => { + const arrived = Promise.withResolvers(), gate = Promise.withResolvers(); + const { manager, requests, until } = pc({ status: managedOffer({ install_cancellable: true }), afterInstall: { statuses: [{ phase: "building", step: "build" }] }, installHeld: { reached: () => arrived.resolve(), gate: gate.promise } }, { ownUpdate: { pollMs: 5 } }); + await arrived.promise; + // the PC took the install; its answer has not come back yet when the update is cancelled here + const machine = manager.list().find((pc) => pc.updating)!; + expect(manager.cancelUpdate(machine.id)).toBe(true); + gate.resolve(); + await until(() => requests.includes("POST /api/updates/cancel")); + }); + + it("asks that PC to stop its update when the update is cancelled here while it builds, over the job's own connection", async () => { + const { manager, requests, until } = pc({ status: managedOffer({ install_cancellable: true }), afterInstall: { statuses: [{ phase: "building", step: "build" }] } }, { ownUpdate: { pollMs: 5 } }); + const building = await until((machine) => /updating itself/.test(machine.updating?.step ?? "")); + // each SSH close, in order with what the PC was asked: its connection must outlast the request to stop + const close = SshConnection.prototype.close; + SshConnection.prototype.close = function (this: SshConnection) { requests.push("SSH CLOSE"); return close.call(this); }; + cleanups.push(() => { SshConnection.prototype.close = close; }); + const from = requests.length; + expect(manager.cancelUpdate(building.id)).toBe(true); + await until(() => requests.includes("POST /api/updates/cancel")); + const after = requests.slice(from); + expect(after.indexOf("POST /api/updates/cancel")).toBeLessThan(after.includes("SSH CLOSE") ? after.indexOf("SSH CLOSE") : Infinity); + // it stopped there: nothing says it may still finish + const ended = await until((machine) => !machine.updating); + expect(ended.update_error ?? "").not.toContain("may still finish"); + }); + it("follows an install that its own Settings already started instead of failing", async () => { const { installs, until } = pc({ status: managedOffer(), install: [{ status: 409, code: "update_busy" }], afterInstall: { statuses: [{ phase: "building", step: "build" }], restartAfter: 2 } }, { ownUpdate: { pollMs: 5 } }); diff --git a/server/machines.ts b/server/machines.ts index 1a973c4df7..5cfaf54f0e 100644 --- a/server/machines.ts +++ b/server/machines.ts @@ -28,8 +28,9 @@ import { decodeOffer, SERVER_ID_PATTERN, isWssOrigin, type RelayOffer } from ".. import { offerFromCode, TypedCodeError } from "../shared/relay-code-offer.ts"; import { TailnetPcError, TailnetPcs, type TailnetPcsOptions } from "./tailnet-pcs.ts"; import { sshCandidates, type SshCandidatesOptions } from "./ssh-candidates.ts"; -import { followOwnUpdate, offerFromStatus, readOwnStatus } from "./pc-update.ts"; +import { followOwnUpdate, offerFromStatus, readOwnStatus , cancelledWords, cancelOwnUpdate, OwnUpdateCancelled } from "./pc-update.ts"; import { appRelease, INSTALLER, installPlanFor, PcInstaller, samePc, type InstallConnect, type InstallPlan } from "./pc-install.ts"; +import { warmSshAddresses } from "./ssh-address.ts"; import { readMoveOffer } from "./remote-move.ts"; import type { RelaySocket } from "../shared/relay-channel.ts"; @@ -99,7 +100,7 @@ interface Runtime { /** the connection generation whose read of that offer is under way */ offerReading?: number; /** its own Ovis updating itself, asked from here through its connection (updateOwn) */ - ownJob?: { id: string; abort: AbortController; finished?: Promise }; + ownJob?: { id: string; abort: AbortController; finished?: Promise; /** cancelled by the person (cancelUpdate), not by a disable or removal */ asked?: boolean }; /** * What an install over SSH can put right where its own Ovis refused to update (PcInstaller): replace a server * started by hand, or restart one whose supervisor is too old; the process and the address it was seen at. @@ -111,6 +112,12 @@ interface Runtime { /** An update asked for that the PC cannot take now: answered 409 `update_unavailable` with these words. */ export class PcUpdateRefused extends Error {} interface JobState { + /** an own update asked for in this job (runOwnUpdate): asks that PC to stop it, and says what became of it */ + ownCancel?: () => Promise; + /** cancelled, its connection still closing (after ownCancel): its PC stays busy */ + closing?: boolean; + /** that request to stop, until it is done: the setup's own ending waits for it before closing the connection */ + cancelling?: Promise; update: boolean; /** Explicit consent for the next Windows cold-start attempt only. Never persisted. */ headlessStart?: boolean; @@ -158,6 +165,11 @@ const releaseRank = (version: string): number[] | null => { const parts = /^(\d{1,6})\.(\d{1,6})\.(\d{1,6})(?:-beta\.(\d{1,6}))?$/.exec(version); return parts ? [Number(parts[1]), Number(parts[2]), Number(parts[3]), parts[4] === undefined ? Infinity : Number(parts[4])] : null; }; +/** the same release however it is written (a v prefix, build metadata): compared by its rank */ +function sameRelease(a: string | null | undefined, b: string): boolean { + const [x, y] = [a ? releaseRank(a.replace(/^v/, "").replace(/\+.*$/, "")) : null, releaseRank(b.replace(/^v/, "").replace(/\+.*$/, ""))]; + return !!x && !!y && x.every((part, i) => part === y[i]); +} /** a release version newer than this app's; one that cannot be compared counts as newer */ function newerThanThisApp(version: string | null | undefined): boolean { if (!version) return false; @@ -477,6 +489,8 @@ export class MachineManager { } } } + // the saved PCs' addresses, resolved in the background now they are loaded, for the checks that keep one job per PC + void warmSshAddresses([...this.machines.values()].flatMap((runtime) => runtime.machine.kind === "ssh" && runtime.machine.target ? [runtime.machine.target] : [])); void this.refreshLocal(); this.localTimer = setInterval(() => void this.refreshLocal(), 5000); this.localTimer.unref(); @@ -924,7 +938,7 @@ export class MachineManager { if (this.updateRunning(runtime)) throw new PcUpdateRefused("This PC is already updating."); // another PC saved at the same address may be installing or connecting there right now const at = runtime.machine.target; - if (this.installer.runningAt(at) || [...this.jobs.values()].some((job) => !TERMINAL.has(job.public.phase) && samePc(job.public.target, at))) throw new PcUpdateRefused("That PC is being installed or connected already; wait for it to finish."); + if (this.installer.runningAt(at) || [...this.jobs.values()].some((job) => (!TERMINAL.has(job.public.phase) || job.closing) && samePc(job.public.target, at))) throw new PcUpdateRefused("That PC is being installed or connected already; wait for it to finish."); // only as its card offers it: an enabled PC this app may connect to and manage, never past this app's release const offer = this.updateOffer(runtime); if (offer?.how !== "install" || !offer.updatable) throw new PcUpdateRefused(offer?.reason ?? "Nothing on that PC needs installing from here; update it with Update."); @@ -936,12 +950,29 @@ export class MachineManager { } const target = validateTarget(request); // the same PC as ssh resolves it (an alias, or its default port given), added for herdr's default session - if ([...this.machines.values()].some((runtime) => runtime.machine.target && !runtime.machine.target.session && samePc(runtime.machine.target, target))) throw new PcUpdateRefused("That PC is already added here; update it from its card."); - if (this.installer.runningAt(target) || [...this.jobs.values()].some((job) => !TERMINAL.has(job.public.phase) && samePc(job.public.target, target))) throw new PcUpdateRefused("That PC is being installed or connected already; wait for it to finish."); + if ([...this.machines.values()].some((runtime) => runtime.machine.target && !runtime.machine.target.session && samePc(runtime.machine.target, target, false))) throw new PcUpdateRefused("That PC is already added here; update it from its card."); + if (this.installer.runningAt(target) || [...this.jobs.values()].some((job) => (!TERMINAL.has(job.public.phase) || job.closing) && samePc(job.public.target, target))) throw new PcUpdateRefused("That PC is being installed or connected already; wait for it to finish."); return this.installer.start({ target, ...(typeof name === "string" && name.trim() ? { name: name.trim() } : {}) }); } installJob(id: string): PcInstallJob | undefined { return this.installer.job(id); } cancelInstall(id: string): void { this.installer.cancel(id); } + /** + * Resolves (ssh -G, in the background, 3 s at most each) the address a setup or install names and those of the jobs and + * PCs it is checked against, so its one-job-per-PC and already-added checks compare what ssh resolves, a dotted alias + * or an IP for a host name included. One that fails stays compared as written (a bare alias as possibly any PC). + */ + async resolveFor(body: unknown): Promise { + const request = (typeof body === "object" && body !== null ? body : {}) as { destination?: unknown; port?: unknown; machine_id?: unknown }; + const targets: SshTarget[] = []; + // only an address the setup would take: never anything ssh could read as an option + if (typeof request.destination === "string") { try { targets.push(validateTarget({ destination: request.destination, ...(typeof request.port === "number" ? { port: request.port } : {}) })); } catch { /* refused by the setup itself */ } } + const saved = typeof request.machine_id === "string" ? this.machines.get(request.machine_id)?.machine.target : undefined; + if (saved) targets.push(saved); + for (const job of this.jobs.values()) if (!TERMINAL.has(job.public.phase) || job.closing) targets.push(job.public.target); + for (const job of this.installer.running()) targets.push(job.target); + for (const runtime of this.machines.values()) if (runtime.machine.kind === "ssh" && runtime.machine.target) targets.push(runtime.machine.target); + await warmSshAddresses(targets); + } /** the connect an install ends with: the usual setup, with the update asked for when the install restarted it */ private installConnect: InstallConnect = (request) => { // keys only, as the install itself: a PC that asks for a password or a new host key by now fails with why @@ -966,6 +997,9 @@ export class MachineManager { // one on offer by then is refused, never installed (runOwnUpdate) const past = pastThisApp(result.remote_ovis ? { updatable: true, latest_version: result.remote_ovis.latest_version } : null); if (past) return past; + // without a card's revision (a replacement, or a fresh install that found an older Ovis there), the release shown was + // this app's own: its update installs that release only, never another one the PC offers by now + if (!request.revision && !sameRelease(result.remote_ovis?.latest_version, APP_VERSION)) return `That PC offers another release now (${result.remote_ovis?.latest_version ? `v${result.remote_ovis.latest_version}` : "none it names"}) than the one shown (v${APP_VERSION}). Look at it again, then update.`; const revision = request.revision ?? this.jobs.get(current.id)?.offeredRevision; if (!revision) return UNREAD_RELEASE; current = start(true, revision); request.onJob?.(current.id); result = await ended(current); @@ -1027,13 +1061,13 @@ export class MachineManager { cancelUpdate(id: string): boolean { const runtime = this.machines.get(id); if (!runtime) throw new Error("PC not found"); - if (runtime.ownJob) { runtime.ownJob.abort.abort(); return true; } + if (runtime.ownJob) { runtime.ownJob.asked = true; runtime.ownJob.abort.abort(); return true; } const install = this.installer.runningFor(id); if (install) { this.installer.cancel(install.id); return true; } if (this.updateQueued.delete(id)) { runtime.machine.updating = null; this.emit(); return true; } // a bridge update running now, an automatic one too (it stays in autoQueued until it ends) const job = [...this.jobs.values()].find((j) => j.runtime === runtime && j.update && !TERMINAL.has(j.public.phase)); - if (job) { this.cancelJob(job.public.id); return true; } + if (job) { this.cancelJob(job.public.id, false, true); return true; } // an automatic bridge update waiting its turn: it is not started, as after a Cancel of a running one if (this.autoQueued.has(id)) { runtime.updateCancelled = true; return true; } return false; @@ -1137,6 +1171,7 @@ export class MachineManager { }; show("Asking this PC's own Ovis to update itself…"); const finished = followOwnUpdate({ + cancelled: () => !this.stopped && runtime.ownJob?.asked === true, endpoint: () => same() && runtime.machine.enabled && runtime.machine.state === "connected" ? runtime.endpoint : undefined, revision, // it came back on a version this app refuses, or was turned away: it no longer reconnects by itself @@ -1147,7 +1182,9 @@ export class MachineManager { // the release it now runs is no longer on offer; a newer one a read since has found stays if (runtime.offerRevision === revision) { runtime.ownOffer = null; runtime.offerRevision = undefined; } }, (error: unknown) => { - if (!abort.signal.aborted && !this.stopped) runtime.machine.update_error = error instanceof Error ? error.message : String(error); + // a cancel's own words are kept when the update may go on there + // a cancel's outcome there (stopped, or may still finish) is told as well + if (!this.stopped && (!abort.signal.aborted || (error instanceof OwnUpdateCancelled && error.there !== "none"))) runtime.machine.update_error = error instanceof Error ? error.message : String(error); }).finally(() => { if (runtime.ownJob?.id !== id) return; runtime.ownJob = undefined; @@ -1176,7 +1213,7 @@ export class MachineManager { if (existing?.machine.kind === "hc") throw new Error(HC_MACHINE_MANAGED); if (existing && [...this.jobs.values()].some((j) => j.public.machine_id === existing.machine.id && !["connected", "failed", "cancelled"].includes(j.public.phase))) throw new Error("A connection job for this PC is already running"); // and one per PC as ssh resolves it, for another saved PC or a new one too, whatever herdr session each names - if ([...this.jobs.values()].some((job) => !TERMINAL.has(job.public.phase) && samePc(job.public.target, target))) throw new Error("This app is already connecting to this PC; wait for that to finish, or cancel it."); + if ([...this.jobs.values()].some((job) => (!TERMINAL.has(job.public.phase) || job.closing) && samePc(job.public.target, target))) throw new Error("This app is already connecting to this PC; wait for that to finish, or cancel it."); const id = randomUUID(); const machineId = existing?.machine.id ?? randomUUID(); if (existing?.machine.herdr_profile_id && (!existing.machine.enabled || !existing.machine.target || !sameSshSession(target, existing.machine.target))) throw new Error("This PC is managed by herdr; refresh its saved machine before setup"); @@ -1274,6 +1311,8 @@ export class MachineManager { // A PC whose own Ovis is away, or still checking for releases, waits the same way: both end by themselves. const unreachable = ownsRuntime && job.update && !!existing && !(e instanceof MachineActionRequired) && (error === OWN_OVIS_AWAY || error === OWN_CHECK_RUNNING || (reached ? !ssh.connected() : !SSH_REFUSED.test(error))); + // a cancel asking that PC to stop its update goes over this connection: it is closed only once that is done + if (job.cancelling) await job.cancelling; if (ownsRuntime) this.disconnect(runtime); // An interrupted setup says nothing about the bridge: restore a saved PC's retries. // Only explicit cancellation suppresses automatic updates; a deadline is not a refusal. @@ -1344,7 +1383,7 @@ export class MachineManager { action(id: string, action: SetupAction): void { const job = this.jobs.get(id); if (!job) throw new Error("Setup job not found"); - if (action.action === "cancel") { this.cancelJob(id); return; } + if (action.action === "cancel") { this.cancelJob(id, false, true); return; } if (action.action === "start_headless") { if (job.abort.signal.aborted || job.public.phase !== "starting" || (job.public.action_required !== "windows_login" && job.public.action_required !== "windows_start_failed")) throw new Error("This PC is not waiting for Windows sign-in"); job.public.action_required = null; @@ -1368,7 +1407,11 @@ export class MachineManager { job.public.step = actionStep(action.action); pending.resolve(action.action === "answer" ? action.answer : "approved"); } - private cancelJob(id: string, timedOut = false): void { + /** `asked`: the person cancelled it here (its Cancel, or the PC's cancel of its update): only then is that PC asked to stop its own update */ + private cancelJob(id: string, timedOut = false, asked = false): void { + // an own update asked for in this job: that PC is asked to stop it first, over this job's connection, then it closes + // only a cancel asked for here does: this server stopping (its own restart, say) only ends the waiting + const ownCancel = this.stopped || !asked ? undefined : this.jobs.get(id)?.ownCancel; const job = this.jobs.get(id); if (!job || ["connected", "failed", "cancelled"].includes(job.public.phase)) return; job.timedOut = timedOut; @@ -1376,7 +1419,10 @@ export class MachineManager { if (timedOut) job.public.error = "Connection timed out"; job.public.action_required = null; job.headlessStart = false; job.abort.abort(); job.pending?.reject(new Error("Setup cancelled")); job.pending = undefined; - job.ssh?.close(); clearTimeout(job.timer); + clearTimeout(job.timer); + // its PC stays busy until that connection has closed + if (ownCancel) { job.closing = true; job.cancelling = ownCancel().then((words) => { job.public.error = words; this.emit(); }).finally(() => { job.ssh?.close(); job.closing = false; }); } + else job.ssh?.close(); } private async prepare(runtime: Runtime, job?: JobState): Promise { const generation = runtime.generation; @@ -1738,10 +1784,13 @@ export class MachineManager { redirect: "manual", signal: AbortSignal.any([job.abort.signal, AbortSignal.timeout(BRIDGE_ANSWER_MS)]), }); + // the last status read there, which a cancel's request to stop it goes by + let seen: Partial | null = null; const read = async (): Promise> => { const response = await updates(); if (!response.ok) { await discard(response); throw new Error(`That PC's Ovis did not tell its update status (${response.status})`); } - return await readCappedJson | null>(response, REMOTE_LIMITS.ownUpdates) ?? {}; + seen = await readCappedJson | null>(response, REMOTE_LIMITS.ownUpdates) ?? {}; + return seen; }; // a check for releases running there is no install, and offers nothing until it ends const settled = async (): Promise> => { @@ -1763,8 +1812,14 @@ export class MachineManager { const revision = before.latest_revision; if (before.available !== true || typeof revision !== "string" || !REVISION.test(revision)) throw new Error("That PC's Ovis has no update on offer any more. Check its Settings → Updates there, then reconnect."); if (job.expectedRevision && revision !== job.expectedRevision) throw new Error(CHANGED_OFFER); + // from its 202 (or an answer the cancel cut off) a cancel asks that PC to stop it, over this job's connection, before + // that closes (cancelJob); never an install its own Settings started, which a refusal says + // armed before it is asked (a cancel while its answer is on its way may follow an install already accepted); + // disarmed by a refusal: an install the PC's own Settings started (update_busy) is never this one to stop + job.ownCancel = async () => cancelledWords(await cancelOwnUpdate(endpoint, seen, true)); const response = await updates(revision); if (response.status === 202) { await discard(response); break; } + job.ownCancel = undefined; const refusal = (await readCappedJson<{ error?: { code?: unknown; message?: unknown } } | null>(response, REMOTE_LIMITS.bridgeError).catch(() => null))?.error; // busy: an install of it (its own Settings, another app) runs, and is followed; or a check that // started since the status was read, and the install is asked for again once it ends @@ -1783,6 +1838,8 @@ export class MachineManager { const next = (await host.descriptors(ssh)).find((d) => d.socket_path === expectedSocket && Number.isSafeInteger(d.pid) && d.pid > 0 && d.pid !== descriptor.pid); if (next && await host.alive(ssh, next.pid)) { if (next.bundle_version !== REMOTE_BUNDLE_VERSION && !newerBundle(next.bundle_version)) throw new MachineActionRequired(`This PC's Ovis updated itself, but its version still differs from this app's (v${remoteWords(next.bundle_version)}; this app requires v${REMOTE_BUNDLE_VERSION}). Update it on that PC (Settings → Updates), then reconnect; it was left running.`, "setup"); + // its update is done: a cancel from here on has nothing to stop there + job.ownCancel = undefined; this.stage(job, "starting", "Connecting to this PC's updated Ovis…"); return next; } diff --git a/server/pc-install-routes.test.ts b/server/pc-install-routes.test.ts index d8303bf15d..397991c17a 100644 --- a/server/pc-install-routes.test.ts +++ b/server/pc-install-routes.test.ts @@ -1,5 +1,5 @@ -import { afterEach, expect, it, spyOn } from "bun:test"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { afterEach, expect, it, spyOn, setSystemTime } from "bun:test"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { Machine, PcInstallJob, RemoteOvis, SshTarget } from "../shared/machines.ts"; @@ -8,7 +8,9 @@ import { handleMachineRequest } from "./machine-api.ts"; import { MachineManager } from "./machines.ts"; import type { PushService } from "./push.ts"; import { SshConnection } from "./ssh.ts"; -import { forgetSshAddresses, sshAddresses } from "./ssh-address.ts"; +/** this app's own release, as the install's steps show it */ +const APP_VERSION = (JSON.parse(readFileSync(join(import.meta.dir, "..", "package.json"), "utf8")) as { version: string }).version; +import { forgetSshAddresses, literalAddress, samePc, sshAddresses, warmSshAddresses } from "./ssh-address.ts"; /** ssh -G as it runs, for the tests that read a config of their own; every other test reads none */ const realResolve = sshAddresses.resolve; @@ -40,7 +42,8 @@ function fixture() { }; SshConnection.prototype.close = function (this: SshConnection) { (this as unknown as { refuse?: () => void }).refuse?.(); }; const manager = new MachineManager(dir, {} as PushService, {} as CompletionTracker, async () => { throw new Error("isolated local offline"); }); - const resolve = spyOn(sshAddresses, "resolve").mockReturnValue(null); + // addresses as written: no developer's or CI's SSH config is read + const resolve = spyOn(sshAddresses, "resolve").mockImplementation(async (target) => literalAddress(target)); forgetSshAddresses(); cleanups.push(() => { resolve.mockRestore(); forgetSshAddresses(); }); cleanups.push(() => { manager.stop(); Object.assign(SshConnection.prototype, real); reconnect.mockRestore(); rmSync(dir, { recursive: true, force: true }); }); @@ -161,14 +164,16 @@ it("updates an older Ovis a fresh install finds already installed there, only to const { asked, outcomes, connect } = connects(manager); // no machine_id: a PC not added here yet, whose own Ovis the launcher started at its old release; its update installs // the release this connect read, and was checked against this app's - outcomes.push({ ...OLDER, remote_ovis: { latest_version: "0.5.0" }, offeredRevision: "b".repeat(40) }, { phase: "connected" }); + outcomes.push({ ...OLDER, remote_ovis: { latest_version: APP_VERSION }, offeredRevision: "b".repeat(40) }, { phase: "connected" }); expect(await connect({ target: { destination: "me@fresh.invalid" }, update: true }).finished).toBeNull(); expect(asked).toEqual([{ update: false, revision: undefined }, { update: true, revision: "b".repeat(40) }]); // a beta of a release newer than this app, one whose version it does not tell, or no release read: no update is started for (const [outcome, words] of [ [{ ...OLDER, remote_ovis: { latest_version: "999.0.0-beta.1" }, offeredRevision: "b".repeat(40) }, "newer than this app"], [{ ...OLDER, remote_ovis: { latest_version: null }, offeredRevision: "b".repeat(40) }, "does not tell its version"], - [{ ...OLDER, remote_ovis: { latest_version: "0.5.0" } }, "did not tell which release"], + [{ ...OLDER, remote_ovis: { latest_version: APP_VERSION } }, "did not tell which release"], + // the release shown was this app's own (v0.5.1): another one the PC offers by now is not installed + [{ ...OLDER, remote_ovis: { latest_version: "0.5.0" }, offeredRevision: "b".repeat(40) }, `another release now (v0.5.0) than the one shown (v${APP_VERSION})`], ] as const) { asked.length = 0; outcomes.push(outcome); @@ -220,7 +225,8 @@ it("cancels an automatic bridge update that is already running, not only one wai (manager as unknown as { autoQueued: Set }).autoQueued.add(SAVED); const cancel = spyOn(manager as unknown as { cancelJob(id: string): void }, "cancelJob").mockImplementation(() => {}); expect(manager.cancelUpdate(SAVED)).toBe(true); - expect(cancel).toHaveBeenCalledWith("auto-1"); + // the person's cancel: that PC may be asked to stop its own update + expect(cancel).toHaveBeenCalledWith("auto-1", false, true); }); it("starts no bridge update while the PC's own Ovis updates itself", () => { @@ -283,18 +289,21 @@ it("ends a restart's install with the PC's own update it started: its error, or }); /** the SSH config these tests resolve addresses with (ssh -G -F): an alias of one host, and that host itself */ -function sshConfig(): void { +function sshConfig(): (targets: SshTarget[]) => Promise { const dir = mkdtempSync(join(tmpdir(), "ovis-ssh-config-")); const config = join(dir, "config"); - writeFileSync(config, "Host new-alias\n HostName new.invalid\n User me\n\nHost lab-alias\n HostName lab.invalid\n User me\n\nHost *\n User me\n"); + writeFileSync(config, "Host new-alias\n HostName new.invalid\n User me\n\nHost lab-alias\n HostName lab.invalid\n User me\n\nHost new.home\n HostName new.invalid\n User me\n\nHost *\n User me\n"); spyOn(sshAddresses, "resolve").mockImplementation((target) => realResolve.call(sshAddresses, target, config)); forgetSshAddresses(); cleanups.push(() => rmSync(dir, { recursive: true, force: true })); + // resolved ahead, as the manager does for its saved PCs + return (targets: SshTarget[]) => warmSshAddresses(targets); } it.skipIf(!Bun.which("ssh"))("keeps one install or setup per PC as ssh resolves it: an alias, or the default port given, is the same PC", async () => { const { manager, reached } = fixture(); - sshConfig(); + const warm = sshConfig(); + await warm([{ destination: "me@new.invalid" }, { destination: "new-alias" }, { destination: "me@new.invalid", port: 22 }, { destination: "lab-alias" }, { destination: "me@lab.invalid" }, { destination: "me@lab.invalid", port: 22 }]); const job = manager.install({ destination: "me@new.invalid" }); await reached("me@new.invalid"); // the same PC by its alias, and by its address with the default port given @@ -308,7 +317,8 @@ it.skipIf(!Bun.which("ssh"))("keeps one install or setup per PC as ssh resolves it.skipIf(!Bun.which("ssh"))("keeps one setup at a time per PC as ssh resolves it, whatever herdr session each names", async () => { const { manager, reached } = fixture(); - sshConfig(); + const warm = sshConfig(); + await warm([{ destination: "new-alias" }, { destination: "me@new.invalid" }, { destination: "me@new.invalid", port: 22 }, { destination: "me@other.invalid" }]); const first = manager.setup({ destination: "new-alias" }); await reached("new-alias"); for (const second of [{ destination: "me@new.invalid" }, { destination: "me@new.invalid", port: 22, session: "work" }, { destination: "new-alias", session: "work" }]) { @@ -318,3 +328,75 @@ it.skipIf(!Bun.which("ssh"))("keeps one setup at a time per PC as ssh resolves i expect(() => manager.setup({ destination: "me@other.invalid" })).not.toThrow(); manager.action(first.id, { action: "cancel" }); }); + +it("counts a PC that ssh cannot resolve as possibly busy, so an alias never starts a second job beside one", async () => { + const { manager, reached } = fixture(); + const job = manager.install({ destination: "me@new.invalid" }); + await reached("me@new.invalid"); + // an alias ssh -G could not resolve (it failed, or ran out of time): it may be that same PC + (sshAddresses.resolve as unknown as { mockImplementation(fn: (target: SshTarget) => Promise): void }).mockImplementation(async (target) => target.destination === "unresolved-alias" ? null : literalAddress(target)); + forgetSshAddresses(); + await warmSshAddresses([{ destination: "unresolved-alias" }]); + expect(() => manager.setup({ destination: "unresolved-alias" })).toThrow("Ovis is being installed on this PC"); + // and is never refused as already added for that alone + manager.cancelInstall(job.id); +}); + +it("starts a first job for another PC at once, while its address is still being looked up", async () => { + const { manager, reached } = fixture(); + const job = manager.install({ destination: "me@new.invalid" }); + await reached("me@new.invalid"); + // nothing looked up yet for the other PC: compared as written meanwhile, never refused as possibly the same + forgetSshAddresses(); + expect(() => manager.setup({ destination: "me@other.invalid" })).not.toThrow(); + manager.cancelInstall(job.id); +}); + +it("refuses an alias typed for the first time while its host has a job, before ssh has resolved it", async () => { + const { manager, reached } = fixture(); + const job = manager.install({ destination: "me@new.invalid" }); + await reached("me@new.invalid"); + // nothing looked up for the alias yet: it may name that PC + forgetSshAddresses(); + expect(() => manager.setup({ destination: "new-alias" })).toThrow(); + manager.cancelInstall(job.id); +}); + +it.skipIf(!Bun.which("ssh"))("resolves a dotted alias typed for the first time before checking it, through the HTTP route", async () => { + const { manager, reached, call } = fixture(); + sshConfig(); + const job = manager.install({ destination: "me@new.invalid" }); + await reached("me@new.invalid"); + forgetSshAddresses(); + // never looked up before: the route resolves it first, and it names that same PC + const answer = await call("install", "POST", { destination: "new.home" }); + expect(answer.status).toBe(409); + manager.cancelInstall(job.id); +}); + +it("keeps a saved PC's resolved address past its minute while it is looked up again, so a bare alias is never taken for another PC", async () => { + fixture(); + const desktop = { destination: "desktop" }, other = { destination: "me@other.invalid" }; + await warmSshAddresses([desktop, other]); + expect(samePc(desktop, other)).toBe(false); + // a minute later, and another address looked up meanwhile: the saved one still answers as it was resolved + setSystemTime(new Date(Date.now() + 61_000)); + try { + await warmSshAddresses([{ destination: "me@third.invalid" }]); + expect(samePc(desktop, other)).toBe(false); + } finally { setSystemTime(); } +}); + +it("ends a lookup at its limit even when something ssh started holds its output open", async () => { + const dir = mkdtempSync(join(tmpdir(), "ovis-ssh-")); + cleanups.push(() => rmSync(dir, { recursive: true, force: true })); + // an "ssh" whose child outlives it, holding stdout, as a hung Match exec can + const ssh = join(dir, "ssh"); + writeFileSync(ssh, "#!/bin/sh\nsleep 30 &\nsleep 30\n", { mode: 0o755 }); + const before = process.env["OVIS_SSH"]; + process.env["OVIS_SSH"] = ssh; + cleanups.push(() => { if (before === undefined) delete process.env["OVIS_SSH"]; else process.env["OVIS_SSH"] = before; }); + const started = Date.now(); + expect(await realResolve.call(sshAddresses, { destination: "held.invalid" })).toBeNull(); + expect(Date.now() - started).toBeLessThan(6000); +}, 10_000); diff --git a/server/pc-install.ts b/server/pc-install.ts index 38a7721508..72ef56ac54 100644 --- a/server/pc-install.ts +++ b/server/pc-install.ts @@ -8,7 +8,7 @@ import { shellQuote } from "./machine-security.ts"; import type { ProcessBirth } from "./process-owner.ts"; import { posixHost, REMOTE_PATH } from "./remote-host.ts"; import { SshConnection } from "./ssh.ts"; -import { sshAddress } from "./ssh-address.ts"; +import { samePc } from "./ssh-address.ts"; /** * Ovis installed or put right on another PC over SSH, from the PC in use (docs/remote-pcs.md): the official @@ -112,8 +112,7 @@ export const restoreServer = (dir: string): string => `d=${shellQuote(dir)}; [ -L "$d/exe" ] || { rm -rf "$d"; echo 'the copy of that server is gone' >&2; exit 1; }; "$d/exe" -e ${shellQuote(RESTORE_LAUNCHER)} "$d"`; /** removes what captureServer saved, once the server it copied is not to be started again */ export const forgetServer = (dir: string): string => `rm -rf ${shellQuote(dir)}`; -/** the same PC over SSH as ssh resolves it (an alias, or the default port given, is the same one), whatever herdr session each names */ -export const samePc = (a: SshTarget, b: SshTarget): boolean => sshAddress(a) === sshAddress(b); +export { samePc } from "./ssh-address.ts"; /** * The official installer's run on that PC, for one SSH command. Downloaded first, so a failed download is told as one * (piped, sh would run an empty script and exit 0), then run with nothing on stdin, on herdr (a herdr plugin that updates @@ -234,6 +233,8 @@ export class PcInstaller { return job; } job(id: string): PcInstallJob | undefined { return this.jobs.get(id)?.public; } + /** the installs still running, of any PC */ + running(): PcInstallJob[] { return [...this.jobs.values()].filter((state) => !state.settled).map((state) => state.public); } async finished(id: string): Promise { await this.jobs.get(id)?.finished; } /** the install running for a PC added here, if any */ runningFor(machineId: string): PcInstallJob | undefined { diff --git a/server/pc-update-all.test.ts b/server/pc-update-all.test.ts index 0413b0c03f..70ba675206 100644 --- a/server/pc-update-all.test.ts +++ b/server/pc-update-all.test.ts @@ -98,7 +98,7 @@ it("takes a PC out of the queue on cancel, or when it is disabled or removed, an expect(asked).toEqual([A, A]); }); -it("ends the wait of a running own update on cancel, with no failure told", async () => { +it("ends the wait of a running own update on cancel, and says it may still finish there when that PC cannot stop it", async () => { const { internals, pc, until, request } = fixture(); const offered = "a".repeat(40); let installing = false; @@ -118,7 +118,8 @@ it("ends the wait of a running own update on cancel, with no failure told", asyn await until(() => installing && pc(A).updating?.step.includes("building") === true); expect(await (await request(`${A}/update`, "DELETE")).json()).toEqual({ cancelled: true }); await until(() => !pc(A).updating); - expect(pc(A).update_error ?? null).toBeNull(); + // an older Ovis that cannot stop it (no install_cancellable): never told as ended + expect(pc(A).update_error).toContain("may still finish it there"); }); it("leaves a disabled PC out of Update all and its badge, so it is never connected again by it", async () => { diff --git a/server/pc-update-cancel.test.ts b/server/pc-update-cancel.test.ts new file mode 100644 index 0000000000..558f776b2e --- /dev/null +++ b/server/pc-update-cancel.test.ts @@ -0,0 +1,35 @@ +import { afterEach, expect, it } from "bun:test"; +import { unmanagedUpdateStatus, type UpdateStatus } from "../shared/update.ts"; +import { followOwnUpdate } from "./pc-update.ts"; + +// A connected PC's own Ovis, stood in for over HTTP: it offers one release, and starts building it when asked +const OFFERED = "a".repeat(40); +const servers: { stop(): void }[] = []; +afterEach(() => { for (const server of servers.splice(0)) server.stop(); }); + +function ownOvis(cancellable: boolean) { + let building = false, cancelled = false; + const asked: string[] = []; + const server = Bun.serve({ port: 0, hostname: "127.0.0.1", fetch: async (request) => { + const { pathname } = new URL(request.url); + asked.push(`${request.method} ${pathname}`); + if (pathname === "/api/updates/install") { building = true; return new Response(null, { status: 202 }); } + if (pathname === "/api/updates/cancel") { building = false; cancelled = true; return new Response(null, { status: 202 }); } + const status: UpdateStatus = { ...unmanagedUpdateStatus(), managed: true, blocked_reason: null, available: true, current_version: "0.5.0", latest_version: "0.5.1", + latest_revision: OFFERED, revision_bound_install: true, ...(cancellable ? { install_cancellable: true } : {}), ...(building ? { phase: "building", step: "build" } : {}), ...(cancelled ? { phase: "error", error_code: "update_cancelled", error: "The update was cancelled before it was installed." } : {}) }; + return Response.json(status); + } }); + servers.push(server); + return { endpoint: { url: `http://127.0.0.1:${server.port}`, token: "fictional" }, asked }; +} + +for (const cancellable of [true, false]) { + it(`asks that PC to stop an update cancelled here while it builds${cancellable ? "" : ", and says it finishes there when its Ovis cannot"}`, async () => { + const pc = ownOvis(cancellable); + const abort = new AbortController(); + // cancelled once that PC reports it building + const follow = followOwnUpdate({ endpoint: () => pc.endpoint, revision: OFFERED, signal: abort.signal, cancelled: () => true, pollMs: 5, onStep: (step) => { if (step.includes("updating itself")) abort.abort(); } }); + await expect(follow).rejects.toThrow(cancellable ? "stopped it before it was installed" : "may still finish it there"); + expect(pc.asked.includes("POST /api/updates/cancel")).toBe(cancellable); + }); +} diff --git a/server/pc-update.ts b/server/pc-update.ts index 7db7bd2e64..18884e6490 100644 --- a/server/pc-update.ts +++ b/server/pc-update.ts @@ -54,6 +54,8 @@ export interface OwnUpdateOptions { /** the release the person was shown (its latest_revision): another one on offer now is refused, not installed */ revision?: string; signal: AbortSignal; + /** whether the abort was a cancel asked for here (never this server stopping): only then is that PC asked to stop */ + cancelled?(): boolean; onStep(step: string): void; /** tests shorten these */ pollMs?: number; @@ -61,12 +63,52 @@ export interface OwnUpdateOptions { checkMs?: number; } +/** + * After a cancel here: asks that PC's own Ovis to stop the update it was asked for, while it still builds. "stopped" when + * it did, "continues" when it could not (an older Ovis that cannot, or one restarting onto the new release already), + * "none" when no update had started there. Never followed out of the tunnel, and bounded in time. + */ +export async function cancelOwnUpdate(endpoint: Endpoint | undefined, last: Partial | null, started: boolean, confirmMs = 10_000): Promise<"stopped" | "continues" | "none"> { + if (!started) return "none"; + if (!endpoint || last?.install_cancellable !== true) return "continues"; + try { + const response = await fetch(`${endpoint.url}/api/updates/cancel`, { method: "POST", redirect: "manual", headers: { authorization: `Bearer ${endpoint.token}`, "x-herdr-update": "1" }, signal: AbortSignal.timeout(BRIDGE_ANSWER_MS) }); + await discard(response); + if (response.status !== 202) return "continues"; + // a 202 is only asked: stopped once its own status says so, within a bounded wait + for (const until = Date.now() + confirmMs; Date.now() < until;) { + const status = await readOwnStatus(endpoint, AbortSignal.timeout(BRIDGE_ANSWER_MS)).catch(() => null); + if (status?.error_code === "update_cancelled") return "stopped"; + if (status && !installing(status)) return "continues"; + await Bun.sleep(250); + } + return "continues"; + } catch { return "continues"; } +} +/** an update cancelled here, with what became of it there: "continues" when it may still finish there */ +export class OwnUpdateCancelled extends Error { + constructor(readonly there: "stopped" | "continues" | "none") { super(cancelledWords(there)); } +} +/** what a cancel of an update that ran there says: never that the update ended, when it goes on */ +export const cancelledWords = (there: "stopped" | "continues" | "none"): string => there === "stopped" + ? "Update cancelled: that PC's own Ovis stopped it before it was installed." + : there === "continues" ? "Update cancelled here, but that PC's own Ovis may still finish it there (it had started, and did not confirm it stopped): look at its Settings → Updates." : "Update cancelled"; + /** * Asks a connected PC's own Ovis to install the release it offers, then follows it until the PC answers again on * that release. Its connection drops while it restarts: the PC reconnects as any PC does, and this waits for it. * A cancel ends only the waiting: an install that started there runs to its end. Throws in plain words. */ export async function followOwnUpdate(options: OwnUpdateOptions): Promise { + const state = { last: null as Partial | null, asked: false }; + try { await followOwn(options, state); } + catch (error) { + // cancelled once the install was asked for (its answer may have been cut off too): that PC is asked to stop it + if (options.signal.aborted && state.asked && options.cancelled?.() === true) throw new OwnUpdateCancelled(await cancelOwnUpdate(options.endpoint(), state.last, true)); + throw error; + } +} +async function followOwn(options: OwnUpdateOptions, state: { last: Partial | null; asked: boolean }): Promise { const { signal } = options; const pollMs = options.pollMs ?? 2000, startMs = options.startMs ?? 60_000, checkMs = options.checkMs ?? 30_000; const pause = () => new Promise((resolve) => { @@ -74,11 +116,12 @@ export async function followOwnUpdate(options: OwnUpdateOptions): Promise const timer = setTimeout(done, pollMs); signal.addEventListener("abort", done, { once: true }); }); + // the last status read, and whether its update started there: what a cancel asks it to stop const cancelled = () => { if (signal.aborted) throw new Error("Update cancelled"); }; const read = async (): Promise | null> => { const endpoint = options.endpoint(); if (!endpoint) return null; - try { return await readOwnStatus(endpoint, signal); } catch { cancelled(); return null; } + try { const status = await readOwnStatus(endpoint, signal); if (status) state.last = status; return status; } catch { cancelled(); return null; } }; // a check for releases running there offers nothing until it ends const settled = async (): Promise> => { @@ -104,10 +147,13 @@ export async function followOwnUpdate(options: OwnUpdateOptions): Promise if (!now.updatable) throw new Error(now.reason ?? "That PC's Ovis cannot take this update."); const endpoint = options.endpoint(); if (!endpoint) throw new Error("This PC is not connected; update it once it is back."); - const response = await fetch(`${endpoint.url}/api/updates/install`, { method: "POST", redirect: "manual", body: JSON.stringify({ expected_revision: revision }), + // cancelled before the request could leave: nothing was asked of that PC + cancelled(); + let response: Response; + try { response = await fetch(`${endpoint.url}/api/updates/install`, { method: "POST", redirect: "manual", body: JSON.stringify({ expected_revision: revision }), headers: { authorization: `Bearer ${endpoint.token}`, "x-herdr-update": "1", "content-type": "application/json" }, - signal: AbortSignal.any([signal, AbortSignal.timeout(BRIDGE_ANSWER_MS)]) }); - if (response.status === 202) { await discard(response); break; } + signal: AbortSignal.any([signal, AbortSignal.timeout(BRIDGE_ANSWER_MS)]) }); } catch (error) { if (signal.aborted) state.asked = true; throw error; } + if (response.status === 202) { state.asked = true; await discard(response); break; } const refusal = (await readCappedJson<{ error?: { code?: unknown; message?: unknown } } | null>(response, REMOTE_LIMITS.bridgeError).catch(() => null))?.error; if (refusal?.code === "update_offer_changed") throw new Error(CHANGED); // busy: an install of it runs there already (its own Settings, another app), which is followed; or a check diff --git a/server/ssh-address.ts b/server/ssh-address.ts index 6c8f2556a5..1d0dba52b6 100644 --- a/server/ssh-address.ts +++ b/server/ssh-address.ts @@ -4,47 +4,98 @@ import { sshClient } from "./ssh.ts"; /** how long a resolved address is reused: the SSH config may change, but not between one job's checks */ const KEPT_MS = 60_000; -const kept = new Map(); +/** how many resolved addresses are kept at most */ +const KEPT_MOST = 512; +/** how long ssh -G's failure for a target stands before it is asked again */ +const FAILED_MS = 10_000; +/** each target's resolved address, or null where ssh -G failed or ran out of time */ +const kept = new Map(); /** * The PC an SSH target reaches, as OpenSSH resolves it (`ssh -G`, which reads the config and connects to nothing): * `user@hostname:port`. Two aliases of one host, or a port given and the default 22, are the same PC. When ssh cannot * tell, the target's own words stand: its user (or this account's) at its host, on its port or 22. */ -export function sshAddress(target: SshTarget): string { - const key = `${target.destination}\0${target.port ?? ""}`; - const now = Date.now(); - const hit = kept.get(key); - if (hit && now - hit.at < KEPT_MS) return hit.address; - const address = sshAddresses.resolve(target) ?? literalAddress(target); - kept.set(key, { address, at: now }); - return address; +export function sshAddress(target: SshTarget): string | null | undefined { + const hit = kept.get(keyOf(target)); + // never on the request path: resolved (again) in the background, and what is kept meanwhile stands + if (!fresh(hit)) void warmSshAddresses([target]); + // undefined: never resolved yet; null: ssh -G failed + return hit?.address; } +const fresh = (hit: { address: string | null; at: number } | undefined) => !!hit && Date.now() - hit.at < (hit.address === null ? FAILED_MS : KEPT_MS); +const keyOf = (target: SshTarget) => `${target.destination}\0${target.port ?? ""}`; +const sshArgs = (target: SshTarget, config?: string) => [sshClient().ssh, ...(config ? ["-F", config] : []), "-G", ...(target.port ? ["-p", String(target.port)] : []), "--", target.destination]; +function parseResolved(output: string): string | null { + const said = new Map(); + for (const line of output.split("\n")) { + const space = line.indexOf(" "); + if (space > 0) said.set(line.slice(0, space).toLowerCase(), line.slice(space + 1).trim()); + } + const user = said.get("user"), hostname = said.get("hostname"), port = said.get("port"); + return user && hostname && port ? `${user}@${hostname.toLowerCase()}:${port}` : null; +} + +/** + * The same PC as ssh resolves both. When either is not resolved (its lookup still runs, or ssh -G failed), the two are + * compared as written, except that a bare alias (a host with no dot, as an SSH config's Host names it) may be any PC: for + * a check that keeps one job per PC (`unknown` true, the default) it counts as the same, so an alias never slips past, + * while two full host names that differ never hold each other up. A check that would refuse something outright on a + * match (already added here) passes false, and compares as written only. + */ +export function samePc(a: SshTarget, b: SshTarget, unknown = true): boolean { + const x = sshAddress(a), y = sshAddress(b); + if (typeof x === "string" && typeof y === "string") return x === y; + const written = literalAddress(a) === literalAddress(b); + return written || (unknown && (bareAlias(a) || bareAlias(b))); +} +/** a host named as an SSH config's alias may name it: no dot, and no IPv6 address */ +const bareAlias = (target: SshTarget) => { const host = target.destination.slice(target.destination.lastIndexOf("@") + 1); return !host.includes(".") && !host.includes(":"); }; + +const pending = new Map>(); +/** bumped by forgetSshAddresses: a lookup that ends after it keeps nothing */ +let generation = 0; +/** Resolves these targets in the background (once each at a time), so the checks that need them find them kept. */ +export async function warmSshAddresses(targets: SshTarget[]): Promise { + await Promise.all(targets.map((target) => { + const key = keyOf(target); + if (fresh(kept.get(key))) return; + const running = pending.get(key); + if (running) return running; + const asked = generation; + const work = sshAddresses.resolve(target).then((address) => { + if (asked !== generation) return; + kept.delete(key); + kept.set(key, { address, at: Date.now() }); + // an address past its time is still used while it is looked up again, so none goes for its age: only the + // least recently resolved, once more are kept than any PC list holds + for (const old of kept.keys()) { if (kept.size <= KEPT_MOST) break; kept.delete(old); } + }).finally(() => { if (asked === generation) pending.delete(key); }); + pending.set(key, work); + return work; + })); +} +const RESOLVE_MS = 3000; -/** The resolver, replaceable in tests with one that reads a config file of their own. */ +/** The resolver, replaceable in tests with one that reads a config file of their own. Killed outright (SIGKILL) at the limit. */ export const sshAddresses = { - resolve(target: SshTarget, config?: string): string | null { + async resolve(target: SshTarget, config?: string): Promise { try { - const run = Bun.spawnSync([sshClient().ssh, ...(config ? ["-F", config] : []), "-G", ...(target.port ? ["-p", String(target.port)] : []), target.destination], { - stdin: "ignore", stdout: "pipe", stderr: "ignore", timeout: 3000, windowsHide: true, - }); - if (run.exitCode !== 0) return null; - const said = new Map(); - for (const line of run.stdout.toString().split("\n")) { - const space = line.indexOf(" "); - if (space > 0) said.set(line.slice(0, space).toLowerCase(), line.slice(space + 1).trim()); - } - const user = said.get("user"), hostname = said.get("hostname"), port = said.get("port"); - return user && hostname && port ? `${user}@${hostname.toLowerCase()}:${port}` : null; + const run = Bun.spawn(sshArgs(target, config), { stdin: "ignore", stdout: "pipe", stderr: "ignore", windowsHide: true }); + // the read ends at the limit too: a child ssh left running (a Match exec, say) may hold its output open after it is killed + let timer: ReturnType | undefined; + const limit = new Promise((resolve) => { timer = setTimeout(() => { run.kill("SIGKILL"); resolve(null); }, RESOLVE_MS); }); + const done = Promise.all([new Response(run.stdout).text(), run.exited]).then(([output, code]) => code === 0 ? parseResolved(output) : null); + try { return await Promise.race([done, limit]); } finally { clearTimeout(timer); } } catch { return null; } }, }; -function literalAddress(target: SshTarget): string { +export function literalAddress(target: SshTarget): string { const at = target.destination.lastIndexOf("@"); const user = at >= 0 ? target.destination.slice(0, at) : userInfo().username; return `${user}@${target.destination.slice(at + 1).toLowerCase()}:${target.port ?? 22}`; } /** forgets what was resolved: for tests that change the resolver between cases */ -export function forgetSshAddresses(): void { kept.clear(); } +export function forgetSshAddresses(): void { kept.clear(); pending.clear(); generation++; } diff --git a/server/supervisor.ts b/server/supervisor.ts index ab531841af..1d18a7136a 100644 --- a/server/supervisor.ts +++ b/server/supervisor.ts @@ -149,7 +149,8 @@ export async function runSupervisor(root = resolve(import.meta.dir, "..")) { if (request) { // a browser's Check or Update is a user's own request: it does not wait the unshallow // backoff out, the 5-minute timer does - void started.promise.then(() => updater.request(request.command, true, request.command === "install" ? request.expected_revision : undefined)); + if (request.command === "cancel") updater.cancel(); + else void started.promise.then(() => updater.request(request.command, true, request.command === "install" ? request.expected_revision : undefined)); } }, onExit(proc, code) { diff --git a/server/update-api.test.ts b/server/update-api.test.ts index 30875c926d..115cdbaf9f 100644 --- a/server/update-api.test.ts +++ b/server/update-api.test.ts @@ -60,6 +60,26 @@ const answer = async (response: Response | Promise) => { return { status: resolved.status, code: ((await resolved.json()) as { error?: { code?: string } }).error?.code ?? null }; }; +describe("a cancel", () => { + it("stops only an install that builds, and only where its supervisor can", async () => { + let status = offer({ phase: "building", install_cancellable: true }); + const updates = service(() => status); + const cancel = () => answer(handleUpdateRequest(install(""), "/api/updates/cancel", updates)); + expect(await cancel()).toEqual({ status: 202, code: null }); + expect(updates.requests).toEqual([{ command: "cancel" }]); + // nothing building (restarting onto the new release, or idle): past or nothing to stop + // still checking what it installs: stopped before it builds + status = offer({ phase: "checking", install_cancellable: true }); + expect(await cancel()).toEqual({ status: 202, code: null }); + updates.requests.splice(1); + for (const phase of ["restarting", "idle"] as const) { status = offer({ phase, install_cancellable: true }); expect(await cancel()).toEqual({ status: 409, code: "update_not_cancellable" }); } + // an older supervisor cannot + status = offer({ phase: "building" }); + expect(await cancel()).toEqual({ status: 409, code: "update_restart_required" }); + expect(updates.requests).toHaveLength(1); + }); +}); + describe("a manual install", () => { it("rejects unbound manual installs", async () => { const updates = service(() => offer()); diff --git a/server/update-api.ts b/server/update-api.ts index 253feb8c82..56d0c770fd 100644 --- a/server/update-api.ts +++ b/server/update-api.ts @@ -70,7 +70,7 @@ export function connectUpdater(channel?: SupervisorChannel): UpdateService { // an install goes as a message an older supervisor does not know, never as its unbound `install` request: (request) => supervisor?.send(request.command === "install" ? { type: "update-install-revision", expected_revision: request.expected_revision } - : { type: "update-command", command: "check" }), + : request.command === "cancel" ? { type: "update-cancel" } : { type: "update-command", command: "check" }), // the supervisor starts the bridge in the directory it serves: the release's, or the source checkout ...(managed ? { servedRevision: servedRevision(process.cwd()) } : {}), }; @@ -145,13 +145,21 @@ export function handleUpdateRequest(request: Request, pathname: string, service? } if (pathname === "/api/updates/notes" && request.method === "GET") { setRequestRoute(pathname); return reply(service?.notes() ?? noUpdateNotes()); } if (pathname === "/api/updates/installed" && request.method === "GET") { setRequestRoute(pathname); return reply(service?.installed() ?? noInstalledNotes()); } - if (request.method !== "POST" || (pathname !== "/api/updates/check" && pathname !== "/api/updates/install")) { - return fail("method_not_allowed", "Use GET /api/updates, /api/updates/notes or /api/updates/installed, or POST /api/updates/check or /install", 405); + if (request.method !== "POST" || (pathname !== "/api/updates/check" && pathname !== "/api/updates/install" && pathname !== "/api/updates/cancel")) { + return fail("method_not_allowed", "Use GET /api/updates, /api/updates/notes or /api/updates/installed, or POST /api/updates/check, /install or /cancel", 405); } setRequestRoute(pathname); if (!updateRequestAllowed(request)) return fail("invalid_update_request", "Use the update controls from this app.", 403); if (!service || !status.managed) return fail("updates_unmanaged", status.blocked_reason ?? "Updates unavailable", 409); const busy = () => fail("update_busy", "An update operation is already running.", 409); + // stops an install while it builds; one restarting onto its new release is past stopping + if (pathname.endsWith("/cancel")) { + if (status.install_cancellable !== true) return fail("update_restart_required", "Restart the app before cancelling an update from here.", 409); + // building, or still checking what it installs: stopped before anything is built + if (status.phase !== "building" && status.phase !== "checking") return fail("update_not_cancellable", "No update is building that could be cancelled.", 409); + service.request({ command: "cancel" }); + return reply({ accepted: true }, 202); + } if (pathname.endsWith("/check")) { if (running(status)) return busy(); service.request({ command: "check" }); diff --git a/server/update-ipc.test.ts b/server/update-ipc.test.ts index 6536114006..b22d76eee4 100644 --- a/server/update-ipc.test.ts +++ b/server/update-ipc.test.ts @@ -68,6 +68,8 @@ describe("the update IPC between the bridge and its supervisor", () => { // what an older bridge sends for its Update: no revision, so no install expect(readUpdateIpcRequest({ type: "update-command", command: "install" })).toBeNull(); expect(readUpdateIpcRequest({ type: "update-command", command: "check" })).toEqual({ command: "check" }); + // a cancel goes as a type of its own, which an older supervisor ignores + expect(readUpdateIpcRequest({ type: "update-cancel" })).toEqual({ command: "cancel" }); }); it("reads only a full lowercase commit id, and nothing from a message out of shape", () => { diff --git a/server/updater.test.ts b/server/updater.test.ts index 4701d2662f..2da8a5cb23 100644 --- a/server/updater.test.ts +++ b/server/updater.test.ts @@ -1540,3 +1540,51 @@ describe("update channels", () => { } finally { rmSync(appState, { recursive: true, force: true }); } }); }); + +it("cancels an install while it builds: nothing is installed, its offer stays, and it is not taken for a failed release", async () => { + const running = updater.status.current_revision; + // a release whose build never ends on its own + writeFileSync(join(upstream, "build-fixture.ts"), "await new Promise(() => {});"); + await git(upstream, "add", "."); + await git(upstream, "commit", "-qm", "slow build"); + await git(upstream, "tag", "v9.0.0"); + const building = Promise.withResolvers(); + const watched = new Updater({ ...updater.options, publish(status) { if (status.step === "build") building.resolve(); } }); + await watched.initialize(); + try { + // nothing to cancel before an install runs + expect(watched.cancel()).toBe(false); + const install = watched.request("install"); + await building.promise; + expect(watched.cancel()).toBe(true); + await install; + expect(watched.status).toMatchObject({ phase: "error", error_code: "update_cancelled", current_revision: running, available: true, install_cancellable: true }); + expect(existsSync(join(stateDir, "failed.json"))).toBe(false); + // the 5-minute timer's check (auto-update on) leaves the cancelled release alone: it would build it again otherwise + (watched.options as { autoUpdate: boolean }).autoUpdate = true; + await watched.request("check"); + expect(watched.status.phase).toBe("idle"); + expect(watched.status.current_revision).toBe(running); + } finally { watched.stop(); } +}); + +it("stops an install cancelled while it still checks what is on offer, before anything is built", async () => { + writeFileSync(join(upstream, "build-fixture.ts"), "await new Promise(() => {});"); + await git(upstream, "add", "."); + await git(upstream, "commit", "-qm", "slow build"); + await git(upstream, "tag", "v9.0.1"); + const seen: Array = []; + const watched: Updater = new Updater({ ...updater.options, publish(status) { + seen.push(status.step ?? null); + // asked as soon as the run starts checking + if (status.phase === "checking" && !asked) asked = watched.cancel(); + } }); + let asked = false; + await watched.initialize(); + try { + await watched.request("install"); + expect(asked).toBe(true); + expect(watched.status).toMatchObject({ phase: "error", error_code: "update_cancelled", available: true }); + expect(seen).not.toContain("build"); + } finally { watched.stop(); } +}); diff --git a/server/updater.ts b/server/updater.ts index 2bfb6892c6..ebb178b22a 100644 --- a/server/updater.ts +++ b/server/updater.ts @@ -213,6 +213,12 @@ export class Updater { /** What the last update brought; set with the release that runs. */ installed: InstalledNotes = noInstalledNotes(); private busy = false; + /** the install running now, which cancel() stops while it builds */ + private installing: AbortController | null = null; + /** the revision a cancel stopped: the timer's automatic install leaves it until it is asked for again, or a newer one comes */ + private cancelledRevision: string | null = null; + /** a cancel asked for while the run still checked: honoured before anything is built */ + private cancelPending = false; private timer?: ReturnType; private initialTimer?: ReturnType; private sourceRevision = ""; @@ -236,6 +242,7 @@ export class Updater { channel?: () => UpdateChannel; }) { this.status.auto_update = options.autoUpdate; this.status.error = options.notice ?? null; + this.status.install_cancellable = true; this.status.channel = this.channel(); } @@ -460,6 +467,14 @@ export class Updater { return run(this.options.root, ["git", "fetch", "--no-tags", "--unshallow", "origin"], this.controller.signal, timeout); } + /** Stops the install that is building now: true when there was one to stop. Once it restarts, false. */ + cancel(): boolean { + if (this.installing && this.status.phase === "building") { this.installing.abort(new Error("Update cancelled")); return true; } + // asked while it still checks what is on offer: it stops before it builds anything + if (this.busy && this.status.phase === "checking") { this.cancelPending = true; return true; } + return false; + } + /** `byUser`: the request came from the settings' Check or Update (through the supervisor), not * from the 5-minute timer; only the timer waits a failed unshallow fetch's backoff out. * `expectedRevision`: the release a manual install was offered; the install goes ahead only @@ -483,15 +498,25 @@ export class Updater { } if ((command === "install" || this.options.autoUpdate) && this.status.available) { const revision = this.status.latest_revision!; + // a revision cancelled here is installed again only when asked for, never by the timer + if (command === "check" && revision === this.cancelledRevision) { this.patch({ phase: "idle", step: null }); return; } + if (command === "install") this.cancelledRevision = null; if (command === "check" && revision === this.failedRevision) { this.patch({ phase: "error", error: "Automatic installation of this revision previously failed. Use Update and restart to retry, or wait for a newer revision." }); return; } attempted = revision; + if (this.cancelPending) { + this.cancelledRevision = revision; + this.patch({ phase: "error", step: null, error_code: "update_cancelled", error: "The update was cancelled before it was installed." }); + return; + } + this.installing = new AbortController(); + const signal = AbortSignal.any([this.controller.signal, this.installing.signal]); this.patch({ phase: "building", step: "download" }); stage = mkdtempSync(join(this.options.stateDir, "release-")); // Network filesystems and low-memory builds can take minutes even without a network fetch. - const run = (args: string[], timeout = 10 * 60_000) => runCommand(stage!, args, this.controller.signal, timeout); + const run = (args: string[], timeout = 10 * 60_000) => runCommand(stage!, args, signal, timeout); await run(["git", "clone", "--quiet", "--no-checkout", "--no-hardlinks", this.options.root, "."]); await run(["git", "fetch", "--quiet", this.options.root, revision]); await run(["git", "checkout", "--quiet", "--detach", revision]); @@ -516,7 +541,9 @@ export class Updater { this.patch({ blocked_reason: reason, available: false }); throw new Error(reason); } - this.controller.signal.throwIfAborted(); + signal.throwIfAborted(); + // from here it restarts onto the new release: too late to cancel + this.installing = null; const previous = this.release; // what it replaces, on record for the supervisor the release starts: that one was not here for the install const next: Release = { directory: stage, revision, source_revision: this.sourceRevision, @@ -550,6 +577,12 @@ export class Updater { } this.patch({ phase: "idle", step: null }); } catch (error) { + // cancelled while it built: nothing was installed, its offer stays, and it is no failed release + if (this.installing?.signal.aborted && !this.controller.signal.aborted) { + this.cancelledRevision = attempted; + this.patch({ phase: "error", step: null, error_code: "update_cancelled", error: "The update was cancelled before it was installed." }); + return; + } if (attempted && !this.controller.signal.aborted) { this.failedRevision = attempted; try { @@ -560,6 +593,8 @@ export class Updater { this.patch({ phase: "error", step: null, error: error instanceof Error ? error.message : String(error), ...(checked ? {} : { available: false }) }); } finally { if (stage) rmSync(stage, { recursive: true, force: true }); + this.installing = null; + this.cancelPending = false; this.busy = false; } } diff --git a/shared/protocol.ts b/shared/protocol.ts index 600fca2666..ef5e93f768 100644 --- a/shared/protocol.ts +++ b/shared/protocol.ts @@ -185,6 +185,9 @@ export type { DevicePairedEvent, DeviceRemovedEvent, PairingIntent, RelayPairing * token (which cannot read /api/access) also `port` and `serving_url`, where it answers * POST /api/updates/check -> 202 { accepted: true } * POST /api/updates/install -> 202 { accepted: true }; UpdateInstallRequest JSON body + * POST /api/updates/cancel -> 202 { accepted: true }: stops the install that builds (install_cancellable); 409 update_not_cancellable or update_restart_required + * (the bridge token may cancel any install that builds there, as it may start one: it is this + * PC owner's own connection server, which asks only for an install it requested) * (at most 1 KiB): 400 invalid_update_install for a missing or malformed revision, * 409 update_restart_required where the supervisor does not advertise * revision_bound_install, 409 update_offer_changed when the offer is no longer that diff --git a/shared/update.ts b/shared/update.ts index b264a01199..18ec8d5650 100644 --- a/shared/update.ts +++ b/shared/update.ts @@ -38,6 +38,12 @@ export interface UpdateStatus { * restart instead of offering an install. */ revision_bound_install?: boolean; + /** + * The supervisor stops an install while it builds when asked (POST /api/updates/cancel): it then + * ends `error` with `error_code: "update_cancelled"`, nothing installed and its offer kept. Once it + * restarts onto the new release, it is too late. Absent from an older supervisor, which cannot. + */ + install_cancellable?: boolean; /** * Where this server answers, told only to the connection server that reaches it as its PC (its * bridge token), which cannot read GET /api/access (#2191): the port it listens on, and the HTTPS @@ -68,7 +74,7 @@ export function readUpdateChannel(value: string | null | undefined): UpdateChann return name === "beta" || name === "stable" ? name : null; } -export type UpdateErrorCode = "update_offer_changed"; +export type UpdateErrorCode = "update_offer_changed" | "update_cancelled"; /** A full commit id, as `latest_revision` names it. */ export const REVISION = /^[0-9a-f]{40,64}$/; @@ -76,7 +82,7 @@ export const REVISION = /^[0-9a-f]{40,64}$/; * What the bridge asks of the update service: a check, or the install of the exact revision the * person saw offered. A manual install never goes without its revision. */ -export type UpdateRequest = { command: "check" } | { command: "install"; expected_revision: string }; +export type UpdateRequest = { command: "check" } | { command: "install"; expected_revision: string } | { command: "cancel" }; /** * The supervisor's reading of a bridge's IPC message. `update-command` carries only a check: an @@ -88,6 +94,8 @@ export function readUpdateIpcRequest(message: unknown): UpdateRequest | null { const value = message as { type?: unknown; command?: unknown; expected_revision?: unknown } | null | undefined; if (typeof value !== "object" || value === null) return null; if (value.type === "update-command" && value.command === "check") return { command: "check" }; + // a cancel travels as a type an older supervisor does not know, and so ignores + if (value.type === "update-cancel") return { command: "cancel" }; if (value.type === "update-install-revision" && typeof value.expected_revision === "string" && REVISION.test(value.expected_revision)) { return { command: "install", expected_revision: value.expected_revision }; } From c9d10a5bb9969fd97567f1a8bd1482a9c040cc41 Mon Sep 17 00:00:00 2001 From: devswha Date: Mon, 9 Nov 2026 03:25:47 +0900 Subject: [PATCH 2/5] fix(updates): a cancel names the install it stops; an install's update is bound to this app's build; tests run no ssh -G Merge pass's review (Codex, route: axwith), reproduced HIGH: - A remote cancel carried no identity, so it could stop another device's update. An install request now gets its own id in its 202 (POST /api/updates/install -> { install_id }), carried to the supervisor; POST /api/updates/cancel requires { install_id } (400 otherwise), and the updater stops only the install that id names. The connection server cancels with the id its 202 gave; an answer cut off (no id) is told as may still finish, never cancelled blind. MEDIUMs: - An install's own Cancel is the person's cancel: its follow-up update there is asked to stop. - The update after a fresh install or a replacement is bound to this app's own build (OVIS_REVISION): another build of the same version the PC offers is refused, and a source build says it cannot tell which build it showed. - No test runs the account's ssh -G or reads its SSH config: under bun test the preload replaces the resolver (server/ssh-address.ts) with addresses as written; the alias tests use an ssh-config stand-in, and the output-limit test a fake ssh. --- changelog.d/pc-install-followups.md | 6 ++-- scripts/test-herdr.ts | 7 +++++ server/api.contract.test.ts | 20 +++++++----- server/machine-own-update.test.ts | 15 ++++++--- server/machines.ts | 19 +++++++---- server/pc-install-routes.test.ts | 49 ++++++++++++++++++++++------- server/pc-update-cancel.test.ts | 8 +++-- server/pc-update.ts | 24 ++++++++------ server/ssh-address.ts | 14 ++++++--- server/supervisor.ts | 4 +-- server/update-api.test.ts | 11 ++++--- server/update-api.ts | 42 ++++++++++++++++++------- server/update-ipc.test.ts | 10 +++--- server/updater.test.ts | 12 ++++--- server/updater.ts | 15 +++++++-- shared/protocol.ts | 6 ++-- shared/update.ts | 13 +++++--- 17 files changed, 191 insertions(+), 84 deletions(-) diff --git a/changelog.d/pc-install-followups.md b/changelog.d/pc-install-followups.md index 3244468522..a7a3d4575c 100644 --- a/changelog.d/pc-install-followups.md +++ b/changelog.d/pc-install-followups.md @@ -1,7 +1,9 @@ ### Fixed -- Cancelling an update of another PC asks that PC's own Ovis to stop it, while it still checks or builds, +- Cancelling an update of another PC asks that PC's own Ovis to stop that update only (by the id its + install request was given), while it still checks or builds, and says it stopped only once that PC confirms it; otherwise (an older Ovis, or one already restarting) it says the update may still finish there, never that it ended. -- An install over SSH that finds an older Ovis there updates it only to the release shown (this app's). +- An install over SSH that finds an older Ovis there updates it only to the release and build shown + (this app's own), and cancelling that install stops its update there too. - One install or setup per PC holds even when ssh cannot resolve an address, and resolving addresses no longer holds the server up. diff --git a/scripts/test-herdr.ts b/scripts/test-herdr.ts index 8043686233..2642eceeba 100644 --- a/scripts/test-herdr.ts +++ b/scripts/test-herdr.ts @@ -24,6 +24,13 @@ process.env["TMPDIR"] = realpathSync(tmpdir()); // herdr socket, in this process and the children tests spawn, whatever XDG_CONFIG_HOME a test sets later process.env["OVIS_TEST_USER_XDG_CONFIG_HOME"] ??= process.env["XDG_CONFIG_HOME"] ?? ""; +// under bun test, no test runs the account's ssh -G or reads its SSH config: addresses resolve as written, and a test that +// needs another answer replaces the resolver itself (server/ssh-address.ts) +if (process.env["NODE_ENV"] === "test") { + const { literalAddress, sshAddresses } = await import("../server/ssh-address.ts"); + sshAddresses.resolve = async (target) => literalAddress(target); +} + export const TEST_SESSION = process.env["HERDR_TEST_SESSION"] || "ovis-test"; /** diff --git a/server/api.contract.test.ts b/server/api.contract.test.ts index d17cd28470..8ebc83c445 100644 --- a/server/api.contract.test.ts +++ b/server/api.contract.test.ts @@ -831,7 +831,9 @@ describe("update API", () => { status = { ...status, latest_revision: offered }; const accepted = await install(JSON.stringify({ expected_revision: offered })); expect(accepted.status).toBe(202); - expect(requests).toEqual([{ command: "install", expected_revision: offered }]); + // with its own id, which only its 202 tells: a cancel must name it + const { install_id } = await accepted.json() as { install_id: string }; + expect(requests).toEqual([{ command: "install", expected_revision: offered, install_id }]); // a release that moves after the 202 is told by the status, not by the request that finished status = { ...status, phase: "error", latest_revision: moved, error_code: "update_offer_changed", error: "A different release is on offer than the one asked for, so nothing was installed. Review it, then update again." }; expect(await (await fetch(`${origin}/api/updates`)).json()).toMatchObject({ phase: "error", error_code: "update_offer_changed", latest_revision: moved }); @@ -842,7 +844,7 @@ describe("update API", () => { expect(await code(older)).toBe("update_restart_required"); // a check stays bodyless expect((await fetch(`${origin}/api/updates/check`, { method: "POST", headers: { "x-herdr-update": "1" } })).status).toBe(202); - expect(requests).toEqual([{ command: "install", expected_revision: offered }, { command: "check" }]); + expect(requests).toEqual([{ command: "install", expected_revision: offered, install_id }, { command: "check" }]); } finally { managed.stop(); rmSync(managedState, { recursive: true, force: true }); } }); @@ -889,14 +891,18 @@ describe("update API", () => { // the install still names the offer, as from the app's own controls expect((await call("/api/updates/install", `Bearer ${bridge}`, "{}")).status).toBe(400); expect(requests).toEqual([]); - expect((await call("/api/updates/install", `Bearer ${bridge}`, JSON.stringify({ expected_revision: offered }))).status).toBe(202); - expect(requests).toEqual([{ command: "install", expected_revision: offered }]); + const granted = await call("/api/updates/install", `Bearer ${bridge}`, JSON.stringify({ expected_revision: offered })); + expect(granted.status).toBe(202); + const { install_id } = await granted.json() as { install_id: string }; + expect(requests).toEqual([{ command: "install", expected_revision: offered, install_id }]); // and stop that install while it builds (POST /api/updates/cancel), once its supervisor can status.phase = "building"; - expect((await call("/api/updates/cancel", `Bearer ${bridge}`, "")).status).toBe(409); + expect((await call("/api/updates/cancel", `Bearer ${bridge}`, JSON.stringify({ install_id }))).status).toBe(409); status.install_cancellable = true; - expect((await call("/api/updates/cancel", `Bearer ${bridge}`, "")).status).toBe(202); - expect(requests.at(-1)).toEqual({ command: "cancel" }); + // a cancel that names no install is refused; the one it names is passed on, for the supervisor to stop that one only + expect((await call("/api/updates/cancel", `Bearer ${bridge}`, "{}")).status).toBe(400); + expect((await call("/api/updates/cancel", `Bearer ${bridge}`, JSON.stringify({ install_id }))).status).toBe(202); + expect(requests.at(-1)).toEqual({ command: "cancel", install_id }); status.phase = "idle"; requests.splice(1); // a wrong bridge token is refused like any other, and the token opens no other update route diff --git a/server/machine-own-update.test.ts b/server/machine-own-update.test.ts index 80ad3c6974..891178185a 100644 --- a/server/machine-own-update.test.ts +++ b/server/machine-own-update.test.ts @@ -132,7 +132,7 @@ function pc(remote: Remote, options: { registered?: boolean; autoUpdate?: boolea } if (path === "/api/updates" && request.method === "GET") return remote.status ? Response.json(remote.status) : Response.json({ error: { code: "unauthorized", message: "token required" } }, { status: 401 }); // asked to stop the install it builds: it stops, and says so - if (path === "/api/updates/cancel" && request.method === "POST") { if (remote.afterInstall) remote.afterInstall = { statuses: [{ phase: "error", error_code: "update_cancelled", error: "The update was cancelled before it was installed." }] }; return new Response(null, { status: 202 }); } + if (path === "/api/updates/cancel" && request.method === "POST") { if ((await request.json() as { install_id?: string }).install_id !== "11111111-2222-4333-8444-555555555555") return Response.json({ error: { code: "invalid_update_cancel" } }, { status: 400 }); if (remote.afterInstall) remote.afterInstall = { statuses: [{ phase: "error", error_code: "update_cancelled", error: "The update was cancelled before it was installed." }] }; return new Response(null, { status: 202 }); } if (path === "/api/updates/install" && request.method === "POST") { expect(request.headers.get("x-herdr-update")).toBe("1"); // its answer held back, as when it is still on its way @@ -145,7 +145,7 @@ function pc(remote: Remote, options: { registered?: boolean; autoUpdate?: boolea installed = true; // its supervisor installs and restarts it: the new version registers a bridge of its own if (!remote.afterInstall) restart(); - return Response.json({ accepted: true }, { status: 202 }); + return Response.json({ accepted: true, install_id: "11111111-2222-4333-8444-555555555555" }, { status: 202 }); } if (path === "/api/bridge" && heldBridge) { const held = heldBridge; heldBridge = null; held.reached.resolve(); await held.gate.promise; } if (path === "/api/bridge") return Response.json({ ...descriptor, ...(restarted && remote.replacementLiveBundle ? { bundle_version: remote.replacementLiveBundle } : {}), socket_id: "fixture:1", herdr: { version: at === "fixture@pc.invalid" ? "0.9.3" : "0.9.4", protocol: 22 } }); @@ -637,17 +637,22 @@ describe("a PC whose own Ovis is older than this app", () => { expect(requests).not.toContain("POST /api/updates/cancel"); }); - it("asks that PC to stop an update cancelled while the answer to its install request is still on its way", async () => { + it("says an update cancelled while the answer to its install request is on its way may still finish there, and stops nothing it cannot name", async () => { const arrived = Promise.withResolvers(), gate = Promise.withResolvers(); const { manager, requests, until } = pc({ status: managedOffer({ install_cancellable: true }), afterInstall: { statuses: [{ phase: "building", step: "build" }] }, installHeld: { reached: () => arrived.resolve(), gate: gate.promise } }, { ownUpdate: { pollMs: 5 } }); await arrived.promise; - // the PC took the install; its answer has not come back yet when the update is cancelled here + // the PC took the install; its answer, which names it, has not come back when the update is cancelled here const machine = manager.list().find((pc) => pc.updating)!; expect(manager.cancelUpdate(machine.id)).toBe(true); gate.resolve(); - await until(() => requests.includes("POST /api/updates/cancel")); + // the job says so, once its request to stop (which cannot name the install) is settled + const jobs = (manager as unknown as { jobs: Map }).jobs; + await until(() => [...jobs.values()].some((job) => /may still finish it there/.test(job.public.error ?? ""))); + // never a cancel without the install's id: it could stop another device's install + expect(requests).not.toContain("POST /api/updates/cancel"); }); + it("asks that PC to stop its update when the update is cancelled here while it builds, over the job's own connection", async () => { const { manager, requests, until } = pc({ status: managedOffer({ install_cancellable: true }), afterInstall: { statuses: [{ phase: "building", step: "build" }] } }, { ownUpdate: { pollMs: 5 } }); const building = await until((machine) => /updating itself/.test(machine.updating?.step ?? "")); diff --git a/server/machines.ts b/server/machines.ts index 5cfaf54f0e..48e8a3d017 100644 --- a/server/machines.ts +++ b/server/machines.ts @@ -28,7 +28,7 @@ import { decodeOffer, SERVER_ID_PATTERN, isWssOrigin, type RelayOffer } from ".. import { offerFromCode, TypedCodeError } from "../shared/relay-code-offer.ts"; import { TailnetPcError, TailnetPcs, type TailnetPcsOptions } from "./tailnet-pcs.ts"; import { sshCandidates, type SshCandidatesOptions } from "./ssh-candidates.ts"; -import { followOwnUpdate, offerFromStatus, readOwnStatus , cancelledWords, cancelOwnUpdate, OwnUpdateCancelled } from "./pc-update.ts"; +import { followOwnUpdate, offerFromStatus, readOwnStatus, cancelledWords, cancelOwnUpdate, installIdOf, OwnUpdateCancelled } from "./pc-update.ts"; import { appRelease, INSTALLER, installPlanFor, PcInstaller, samePc, type InstallConnect, type InstallPlan } from "./pc-install.ts"; import { warmSshAddresses } from "./ssh-address.ts"; import { readMoveOffer } from "./remote-move.ts"; @@ -1000,8 +1000,12 @@ export class MachineManager { // without a card's revision (a replacement, or a fresh install that found an older Ovis there), the release shown was // this app's own: its update installs that release only, never another one the PC offers by now if (!request.revision && !sameRelease(result.remote_ovis?.latest_version, APP_VERSION)) return `That PC offers another release now (${result.remote_ovis?.latest_version ? `v${result.remote_ovis.latest_version}` : "none it names"}) than the one shown (v${APP_VERSION}). Look at it again, then update.`; - const revision = request.revision ?? this.jobs.get(current.id)?.offeredRevision; - if (!revision) return UNREAD_RELEASE; + // the build shown is this app's own (its revision): another build of that version the PC offers is never adopted + const offered = this.jobs.get(current.id)?.offeredRevision; + if (!offered) return UNREAD_RELEASE; + const revision = request.revision ?? (process.env["OVIS_REVISION"] || undefined); + if (!revision) return "This app is not a release build, so it cannot tell which build of its release it showed; update that PC from its own Settings → Updates."; + if (!request.revision && offered !== revision) return `That PC offers another build of v${APP_VERSION} than this app's, the one shown. Look at it again, then update.`; current = start(true, revision); request.onJob?.(current.id); result = await ended(current); followed = true; } @@ -1019,7 +1023,8 @@ export class MachineManager { } return result.phase === "connected" ? null : result.error ?? `The connection to that PC ended: ${result.step}`; })(); - return { job_id: current.id, cancel: () => { cancelled = true; this.cancelJob(current.id); own?.abort.abort(); }, finished }; + // the person's cancel of the install: its follow-up update there is asked to stop as well + return { job_id: current.id, cancel: () => { cancelled = true; this.cancelJob(current.id, false, true); if (own) own.asked = true; own?.abort.abort(); }, finished }; }; /** an install for a PC added here shows on its card as its update does */ private showInstall(job: PcInstallJob): void { @@ -1816,9 +1821,11 @@ export class MachineManager { // that closes (cancelJob); never an install its own Settings started, which a refusal says // armed before it is asked (a cancel while its answer is on its way may follow an install already accepted); // disarmed by a refusal: an install the PC's own Settings started (update_busy) is never this one to stop - job.ownCancel = async () => cancelledWords(await cancelOwnUpdate(endpoint, seen, true)); + // the id its 202 gives: a cancel names it, so only this install is stopped there (none when the answer was cut off) + let installId: string | null = null; + job.ownCancel = async () => cancelledWords(await cancelOwnUpdate(endpoint, seen, installId)); const response = await updates(revision); - if (response.status === 202) { await discard(response); break; } + if (response.status === 202) { installId = await installIdOf(response); break; } job.ownCancel = undefined; const refusal = (await readCappedJson<{ error?: { code?: unknown; message?: unknown } } | null>(response, REMOTE_LIMITS.bridgeError).catch(() => null))?.error; // busy: an install of it (its own Settings, another app) runs, and is followed; or a check that diff --git a/server/pc-install-routes.test.ts b/server/pc-install-routes.test.ts index 397991c17a..de260fcfba 100644 --- a/server/pc-install-routes.test.ts +++ b/server/pc-install-routes.test.ts @@ -10,10 +10,8 @@ import type { PushService } from "./push.ts"; import { SshConnection } from "./ssh.ts"; /** this app's own release, as the install's steps show it */ const APP_VERSION = (JSON.parse(readFileSync(join(import.meta.dir, "..", "package.json"), "utf8")) as { version: string }).version; -import { forgetSshAddresses, literalAddress, samePc, sshAddresses, warmSshAddresses } from "./ssh-address.ts"; +import {forgetSshAddresses, literalAddress, resolveWithSsh, samePc, sshAddresses, warmSshAddresses } from "./ssh-address.ts"; -/** ssh -G as it runs, for the tests that read a config of their own; every other test reads none */ -const realResolve = sshAddresses.resolve; // The install routes and the one-job-per-PC rule on a manager with one saved SSH PC. SSH never connects: each // connection waits until the test lets it in, refuses it, or it is closed. No PC, herdr or real state is touched. @@ -164,6 +162,10 @@ it("updates an older Ovis a fresh install finds already installed there, only to const { asked, outcomes, connect } = connects(manager); // no machine_id: a PC not added here yet, whose own Ovis the launcher started at its old release; its update installs // the release this connect read, and was checked against this app's + // this app runs the release it showed, build "b…" + const before = process.env["OVIS_REVISION"]; + process.env["OVIS_REVISION"] = "b".repeat(40); + cleanups.push(() => { if (before === undefined) delete process.env["OVIS_REVISION"]; else process.env["OVIS_REVISION"] = before; }); outcomes.push({ ...OLDER, remote_ovis: { latest_version: APP_VERSION }, offeredRevision: "b".repeat(40) }, { phase: "connected" }); expect(await connect({ target: { destination: "me@fresh.invalid" }, update: true }).finished).toBeNull(); expect(asked).toEqual([{ update: false, revision: undefined }, { update: true, revision: "b".repeat(40) }]); @@ -174,6 +176,8 @@ it("updates an older Ovis a fresh install finds already installed there, only to [{ ...OLDER, remote_ovis: { latest_version: APP_VERSION } }, "did not tell which release"], // the release shown was this app's own (v0.5.1): another one the PC offers by now is not installed [{ ...OLDER, remote_ovis: { latest_version: "0.5.0" }, offeredRevision: "b".repeat(40) }, `another release now (v0.5.0) than the one shown (v${APP_VERSION})`], + // the same version, another build than this app's (the one shown): never adopted + [{ ...OLDER, remote_ovis: { latest_version: APP_VERSION }, offeredRevision: "c".repeat(40) }, `another build of v${APP_VERSION}`], ] as const) { asked.length = 0; outcomes.push(outcome); @@ -289,18 +293,23 @@ it("ends a restart's install with the PC's own update it started: its error, or }); /** the SSH config these tests resolve addresses with (ssh -G -F): an alias of one host, and that host itself */ +/** + * What ssh -G would answer for this test SSH config, without running ssh or reading any account's config: + * Host new-alias and new.home are new.invalid, Host lab-alias is lab.invalid, and every host's user is me. + */ function sshConfig(): (targets: SshTarget[]) => Promise { - const dir = mkdtempSync(join(tmpdir(), "ovis-ssh-config-")); - const config = join(dir, "config"); - writeFileSync(config, "Host new-alias\n HostName new.invalid\n User me\n\nHost lab-alias\n HostName lab.invalid\n User me\n\nHost new.home\n HostName new.invalid\n User me\n\nHost *\n User me\n"); - spyOn(sshAddresses, "resolve").mockImplementation((target) => realResolve.call(sshAddresses, target, config)); + const aliases: Record = { "new-alias": "new.invalid", "lab-alias": "lab.invalid", "new.home": "new.invalid" }; + spyOn(sshAddresses, "resolve").mockImplementation(async (target) => { + const at = target.destination.lastIndexOf("@"); + const host = target.destination.slice(at + 1); + return `${at >= 0 ? target.destination.slice(0, at) : "me"}@${aliases[host] ?? host}:${target.port ?? 22}`; + }); forgetSshAddresses(); - cleanups.push(() => rmSync(dir, { recursive: true, force: true })); // resolved ahead, as the manager does for its saved PCs return (targets: SshTarget[]) => warmSshAddresses(targets); } -it.skipIf(!Bun.which("ssh"))("keeps one install or setup per PC as ssh resolves it: an alias, or the default port given, is the same PC", async () => { +it("keeps one install or setup per PC as ssh resolves it: an alias, or the default port given, is the same PC", async () => { const { manager, reached } = fixture(); const warm = sshConfig(); await warm([{ destination: "me@new.invalid" }, { destination: "new-alias" }, { destination: "me@new.invalid", port: 22 }, { destination: "lab-alias" }, { destination: "me@lab.invalid" }, { destination: "me@lab.invalid", port: 22 }]); @@ -315,7 +324,7 @@ it.skipIf(!Bun.which("ssh"))("keeps one install or setup per PC as ssh resolves for (const same of [{ destination: "lab-alias" }, { destination: "me@lab.invalid", port: 22 }]) expect(() => manager.install(same)).toThrow("already added here"); }); -it.skipIf(!Bun.which("ssh"))("keeps one setup at a time per PC as ssh resolves it, whatever herdr session each names", async () => { +it("keeps one setup at a time per PC as ssh resolves it, whatever herdr session each names", async () => { const { manager, reached } = fixture(); const warm = sshConfig(); await warm([{ destination: "new-alias" }, { destination: "me@new.invalid" }, { destination: "me@new.invalid", port: 22 }, { destination: "me@other.invalid" }]); @@ -362,7 +371,7 @@ it("refuses an alias typed for the first time while its host has a job, before s manager.cancelInstall(job.id); }); -it.skipIf(!Bun.which("ssh"))("resolves a dotted alias typed for the first time before checking it, through the HTTP route", async () => { +it("resolves a dotted alias typed for the first time before checking it, through the HTTP route", async () => { const { manager, reached, call } = fixture(); sshConfig(); const job = manager.install({ destination: "me@new.invalid" }); @@ -397,6 +406,22 @@ it("ends a lookup at its limit even when something ssh started holds its output process.env["OVIS_SSH"] = ssh; cleanups.push(() => { if (before === undefined) delete process.env["OVIS_SSH"]; else process.env["OVIS_SSH"] = before; }); const started = Date.now(); - expect(await realResolve.call(sshAddresses, { destination: "held.invalid" })).toBeNull(); + expect(await resolveWithSsh({ destination: "held.invalid" })).toBeNull(); expect(Date.now() - started).toBeLessThan(6000); }, 10_000); + +it("runs no ssh -G under bun test: the resolver is the test stand-in, never ssh itself", () => { + expect(sshAddresses.resolve).not.toBe(resolveWithSsh); +}); + +it("cancels an install's follow-up connect as the person's cancel, so its update there is asked to stop too", async () => { + const { manager } = fixture(); + const { outcomes, connect } = connects(manager); + outcomes.push({ phase: "connected" }); + const cancel = spyOn(manager as unknown as { cancelJob(id: string, timedOut?: boolean, asked?: boolean): void }, "cancelJob").mockImplementation(() => {}); + const handle = connect({ target: { destination: "me@fresh.invalid" }, update: true }) as unknown as { job_id: string; cancel(): void; finished: Promise }; + handle.cancel(); + expect(cancel).toHaveBeenCalledWith(handle.job_id, false, true); + cancel.mockRestore(); + await handle.finished; +}); diff --git a/server/pc-update-cancel.test.ts b/server/pc-update-cancel.test.ts index 558f776b2e..31b9a52f71 100644 --- a/server/pc-update-cancel.test.ts +++ b/server/pc-update-cancel.test.ts @@ -13,8 +13,12 @@ function ownOvis(cancellable: boolean) { const server = Bun.serve({ port: 0, hostname: "127.0.0.1", fetch: async (request) => { const { pathname } = new URL(request.url); asked.push(`${request.method} ${pathname}`); - if (pathname === "/api/updates/install") { building = true; return new Response(null, { status: 202 }); } - if (pathname === "/api/updates/cancel") { building = false; cancelled = true; return new Response(null, { status: 202 }); } + if (pathname === "/api/updates/install") { building = true; return Response.json({ accepted: true, install_id: "11111111-2222-4333-8444-555555555555" }, { status: 202 }); } + if (pathname === "/api/updates/cancel") { + // only the install it names + if ((await request.json() as { install_id?: string }).install_id !== "11111111-2222-4333-8444-555555555555") return Response.json({ error: { code: "invalid_update_cancel" } }, { status: 400 }); + building = false; cancelled = true; return new Response(null, { status: 202 }); + } const status: UpdateStatus = { ...unmanagedUpdateStatus(), managed: true, blocked_reason: null, available: true, current_version: "0.5.0", latest_version: "0.5.1", latest_revision: OFFERED, revision_bound_install: true, ...(cancellable ? { install_cancellable: true } : {}), ...(building ? { phase: "building", step: "build" } : {}), ...(cancelled ? { phase: "error", error_code: "update_cancelled", error: "The update was cancelled before it was installed." } : {}) }; return Response.json(status); diff --git a/server/pc-update.ts b/server/pc-update.ts index 18884e6490..758f654e1f 100644 --- a/server/pc-update.ts +++ b/server/pc-update.ts @@ -1,5 +1,5 @@ import type { PcUpdateOffer } from "../shared/machines.ts"; -import { REVISION, type UpdateStatus } from "../shared/update.ts"; +import { REVISION, type UpdateStatus, INSTALL_ID } from "../shared/update.ts"; import { discard, readCappedJson, REMOTE_LIMITS } from "./remote-json.ts"; /** @@ -68,11 +68,12 @@ export interface OwnUpdateOptions { * it did, "continues" when it could not (an older Ovis that cannot, or one restarting onto the new release already), * "none" when no update had started there. Never followed out of the tunnel, and bounded in time. */ -export async function cancelOwnUpdate(endpoint: Endpoint | undefined, last: Partial | null, started: boolean, confirmMs = 10_000): Promise<"stopped" | "continues" | "none"> { - if (!started) return "none"; - if (!endpoint || last?.install_cancellable !== true) return "continues"; +export async function cancelOwnUpdate(endpoint: Endpoint | undefined, last: Partial | null, installId: string | null, confirmMs = 10_000): Promise<"stopped" | "continues" | "none"> { + // only the install this app asked for, by the id its 202 gave: without it (its answer cut off, an older Ovis) nothing is asked + if (!endpoint || last?.install_cancellable !== true || !installId) return "continues"; try { - const response = await fetch(`${endpoint.url}/api/updates/cancel`, { method: "POST", redirect: "manual", headers: { authorization: `Bearer ${endpoint.token}`, "x-herdr-update": "1" }, signal: AbortSignal.timeout(BRIDGE_ANSWER_MS) }); + const response = await fetch(`${endpoint.url}/api/updates/cancel`, { method: "POST", redirect: "manual", body: JSON.stringify({ install_id: installId }), + headers: { authorization: `Bearer ${endpoint.token}`, "x-herdr-update": "1", "content-type": "application/json" }, signal: AbortSignal.timeout(BRIDGE_ANSWER_MS) }); await discard(response); if (response.status !== 202) return "continues"; // a 202 is only asked: stopped once its own status says so, within a bounded wait @@ -85,6 +86,11 @@ export async function cancelOwnUpdate(endpoint: Endpoint | undefined, last: Part return "continues"; } catch { return "continues"; } } +/** the id a PC's 202 gave the install this app asked for (POST /api/updates/install), or null */ +export async function installIdOf(response: Response): Promise { + const body = await readCappedJson<{ install_id?: unknown } | null>(response, REMOTE_LIMITS.bridgeError).catch(() => null); + return typeof body?.install_id === "string" && INSTALL_ID.test(body.install_id) ? body.install_id : null; +} /** an update cancelled here, with what became of it there: "continues" when it may still finish there */ export class OwnUpdateCancelled extends Error { constructor(readonly there: "stopped" | "continues" | "none") { super(cancelledWords(there)); } @@ -100,15 +106,15 @@ export const cancelledWords = (there: "stopped" | "continues" | "none"): string * A cancel ends only the waiting: an install that started there runs to its end. Throws in plain words. */ export async function followOwnUpdate(options: OwnUpdateOptions): Promise { - const state = { last: null as Partial | null, asked: false }; + const state = { last: null as Partial | null, asked: false, installId: null as string | null }; try { await followOwn(options, state); } catch (error) { // cancelled once the install was asked for (its answer may have been cut off too): that PC is asked to stop it - if (options.signal.aborted && state.asked && options.cancelled?.() === true) throw new OwnUpdateCancelled(await cancelOwnUpdate(options.endpoint(), state.last, true)); + if (options.signal.aborted && state.asked && options.cancelled?.() === true) throw new OwnUpdateCancelled(await cancelOwnUpdate(options.endpoint(), state.last, state.installId)); throw error; } } -async function followOwn(options: OwnUpdateOptions, state: { last: Partial | null; asked: boolean }): Promise { +async function followOwn(options: OwnUpdateOptions, state: { last: Partial | null; asked: boolean; installId: string | null }): Promise { const { signal } = options; const pollMs = options.pollMs ?? 2000, startMs = options.startMs ?? 60_000, checkMs = options.checkMs ?? 30_000; const pause = () => new Promise((resolve) => { @@ -153,7 +159,7 @@ async function followOwn(options: OwnUpdateOptions, state: { last: Partial(response, REMOTE_LIMITS.bridgeError).catch(() => null))?.error; if (refusal?.code === "update_offer_changed") throw new Error(CHANGED); // busy: an install of it runs there already (its own Settings, another app), which is followed; or a check diff --git a/server/ssh-address.ts b/server/ssh-address.ts index 1d0dba52b6..9a425342e0 100644 --- a/server/ssh-address.ts +++ b/server/ssh-address.ts @@ -77,9 +77,13 @@ export async function warmSshAddresses(targets: SshTarget[]): Promise { } const RESOLVE_MS = 3000; -/** The resolver, replaceable in tests with one that reads a config file of their own. Killed outright (SIGKILL) at the limit. */ -export const sshAddresses = { - async resolve(target: SshTarget, config?: string): Promise { +/** The resolver, replaced in tests (scripts/test-herdr.ts): never the account's ssh or its config there. */ +export const sshAddresses: { resolve(target: SshTarget, config?: string): Promise } = { + resolve: (target, config) => resolveWithSsh(target, config), +}; +/** ssh -G itself, killed outright (SIGKILL) at the limit, its output read no longer than that */ +export async function resolveWithSsh(target: SshTarget, config?: string): Promise { + { try { const run = Bun.spawn(sshArgs(target, config), { stdin: "ignore", stdout: "pipe", stderr: "ignore", windowsHide: true }); // the read ends at the limit too: a child ssh left running (a Match exec, say) may hold its output open after it is killed @@ -88,8 +92,8 @@ export const sshAddresses = { const done = Promise.all([new Response(run.stdout).text(), run.exited]).then(([output, code]) => code === 0 ? parseResolved(output) : null); try { return await Promise.race([done, limit]); } finally { clearTimeout(timer); } } catch { return null; } - }, -}; + } +} export function literalAddress(target: SshTarget): string { const at = target.destination.lastIndexOf("@"); diff --git a/server/supervisor.ts b/server/supervisor.ts index 1d18a7136a..1f5186a7a6 100644 --- a/server/supervisor.ts +++ b/server/supervisor.ts @@ -149,8 +149,8 @@ export async function runSupervisor(root = resolve(import.meta.dir, "..")) { if (request) { // a browser's Check or Update is a user's own request: it does not wait the unshallow // backoff out, the 5-minute timer does - if (request.command === "cancel") updater.cancel(); - else void started.promise.then(() => updater.request(request.command, true, request.command === "install" ? request.expected_revision : undefined)); + if (request.command === "cancel") updater.cancel(request.install_id); + else void started.promise.then(() => updater.request(request.command, true, request.command === "install" ? request.expected_revision : undefined, request.command === "install" ? request.install_id : undefined)); } }, onExit(proc, code) { diff --git a/server/update-api.test.ts b/server/update-api.test.ts index 115cdbaf9f..98bec17634 100644 --- a/server/update-api.test.ts +++ b/server/update-api.test.ts @@ -2,7 +2,7 @@ import { afterEach, describe, expect, it } from "bun:test"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { DESKTOP_APP_UPDATES, noInstalledNotes, noUpdateNotes, unmanagedUpdateStatus, type UpdateRequest, type UpdateStatus } from "../shared/update.ts"; +import { DESKTOP_APP_UPDATES, noInstalledNotes, noUpdateNotes, unmanagedUpdateStatus, type UpdateRequest, type UpdateStatus, INSTALL_ID } from "../shared/update.ts"; import { connectUpdater, handleUpdateRequest, servedRevision, type UpdateService } from "./update-api.ts"; const directories: string[] = []; @@ -64,9 +64,12 @@ describe("a cancel", () => { it("stops only an install that builds, and only where its supervisor can", async () => { let status = offer({ phase: "building", install_cancellable: true }); const updates = service(() => status); - const cancel = () => answer(handleUpdateRequest(install(""), "/api/updates/cancel", updates)); + const cancel = (id: string | null = "11111111-2222-4333-8444-555555555555") => answer(handleUpdateRequest(install(id === null ? "{}" : JSON.stringify({ install_id: id })), "/api/updates/cancel", updates)); + // only by naming the install it stops (its 202's install_id): never another's + for (const id of [null, "not-an-id", "ABCDEF11-2222-4333-8444-555555555555"]) expect(await cancel(id)).toEqual({ status: 400, code: "invalid_update_cancel" }); + expect(updates.requests).toEqual([]); expect(await cancel()).toEqual({ status: 202, code: null }); - expect(updates.requests).toEqual([{ command: "cancel" }]); + expect(updates.requests).toEqual([{ command: "cancel", install_id: "11111111-2222-4333-8444-555555555555" }]); // nothing building (restarting onto the new release, or idle): past or nothing to stop // still checking what it installs: stopped before it builds status = offer({ phase: "checking", install_cancellable: true }); @@ -97,7 +100,7 @@ describe("a manual install", () => { let status = offer(); const updates = service(() => status); expect(await answer(handleUpdateRequest(install(JSON.stringify({ expected_revision: offered })), "/api/updates/install", updates))).toEqual({ status: 202, code: null }); - expect(updates.requests).toEqual([{ command: "install", expected_revision: offered }]); + expect(updates.requests).toEqual([{ command: "install", expected_revision: offered, install_id: expect.stringMatching(INSTALL_ID) }]); // another release came on offer since the person was shown this one: refused before anything is asked status = offer({ latest_revision: moved }); expect(await answer(handleUpdateRequest(install(JSON.stringify({ expected_revision: offered })), "/api/updates/install", updates))).toEqual({ status: 409, code: "update_offer_changed" }); diff --git a/server/update-api.ts b/server/update-api.ts index 56d0c770fd..120eec4051 100644 --- a/server/update-api.ts +++ b/server/update-api.ts @@ -1,4 +1,5 @@ -import { noInstalledNotes, noUpdateNotes, readInstalledNotes, readUpdateNotes, REVISION, unmanagedUpdateStatus, type InstalledNotes, type UpdateNotes, type UpdateRequest, type UpdateStatus } from "../shared/update.ts"; +import { INSTALL_ID, noInstalledNotes, noUpdateNotes, readInstalledNotes, readUpdateNotes, REVISION, unmanagedUpdateStatus, type InstalledNotes, type UpdateNotes, type UpdateRequest, type UpdateStatus } from "../shared/update.ts"; +import { randomUUID } from "node:crypto"; import { join } from "node:path"; import { isJsonObject, jsonResponse, setRequestRoute } from "./http.ts"; @@ -69,8 +70,8 @@ export function connectUpdater(channel?: SupervisorChannel): UpdateService { installed: () => installed, // an install goes as a message an older supervisor does not know, never as its unbound `install` request: (request) => supervisor?.send(request.command === "install" - ? { type: "update-install-revision", expected_revision: request.expected_revision } - : request.command === "cancel" ? { type: "update-cancel" } : { type: "update-command", command: "check" }), + ? { type: "update-install-revision", expected_revision: request.expected_revision, ...(request.install_id ? { install_id: request.install_id } : {}) } + : request.command === "cancel" ? { type: "update-cancel", install_id: request.install_id } : { type: "update-command", command: "check" }), // the supervisor starts the bridge in the directory it serves: the release's, or the source checkout ...(managed ? { servedRevision: servedRevision(process.cwd()) } : {}), }; @@ -105,6 +106,18 @@ const INSTALL_BODY_LIMIT = 1024; /** The revision an install asks for, from a JSON body of at most INSTALL_BODY_LIMIT bytes; null when it names none. */ async function expectedRevision(request: Request): Promise { + const revision = (await smallBody(request))?.["expected_revision"]; + return typeof revision === "string" && REVISION.test(revision) ? revision : null; +} + +/** The install a cancel names (its install_id, as its 202 gave it); null when it names none. */ +async function cancelledInstall(request: Request): Promise { + const id = (await smallBody(request))?.["install_id"]; + return typeof id === "string" && INSTALL_ID.test(id) ? id : null; +} + +/** A JSON object body of at most INSTALL_BODY_LIMIT bytes, or null. */ +async function smallBody(request: Request): Promise | null> { if (!request.body) return null; const reader = request.body.getReader(); const chunks: Uint8Array[] = []; @@ -118,8 +131,7 @@ async function expectedRevision(request: Request): Promise { } try { const body: unknown = JSON.parse(new TextDecoder().decode(Buffer.concat(chunks))); - const revision = isJsonObject(body) ? body["expected_revision"] : null; - return typeof revision === "string" && REVISION.test(revision) ? revision : null; + return isJsonObject(body) ? body : null; } catch { return null; } } @@ -153,12 +165,16 @@ export function handleUpdateRequest(request: Request, pathname: string, service? if (!service || !status.managed) return fail("updates_unmanaged", status.blocked_reason ?? "Updates unavailable", 409); const busy = () => fail("update_busy", "An update operation is already running.", 409); // stops an install while it builds; one restarting onto its new release is past stopping + // only by the one that asked for that install: it names the install_id its 202 gave if (pathname.endsWith("/cancel")) { - if (status.install_cancellable !== true) return fail("update_restart_required", "Restart the app before cancelling an update from here.", 409); - // building, or still checking what it installs: stopped before anything is built - if (status.phase !== "building" && status.phase !== "checking") return fail("update_not_cancellable", "No update is building that could be cancelled.", 409); - service.request({ command: "cancel" }); - return reply({ accepted: true }, 202); + return cancelledInstall(request).then((id) => { + if (id === null) return fail("invalid_update_cancel", "Name the install_id of the install to cancel.", 400); + if (status.install_cancellable !== true) return fail("update_restart_required", "Restart the app before cancelling an update from here.", 409); + // building, or still checking what it installs: stopped before anything is built (the supervisor stops only that install) + if (status.phase !== "building" && status.phase !== "checking") return fail("update_not_cancellable", "No update is building that could be cancelled.", 409); + service.request({ command: "cancel", install_id: id }); + return reply({ accepted: true }, 202); + }); } if (pathname.endsWith("/check")) { if (running(status)) return busy(); @@ -173,7 +189,9 @@ export function handleUpdateRequest(request: Request, pathname: string, service? if (now.revision_bound_install !== true) return fail("update_restart_required", "Restart the app before installing an update from here.", 409); if (now.blocked_reason || !now.available) return fail("update_unavailable", now.blocked_reason ?? "No update is available. Check again first.", 409); if (now.latest_revision !== expected) return fail("update_offer_changed", "A different update is on offer now. Review it, then update again.", 409); - service.request({ command: "install", expected_revision: expected }); - return reply({ accepted: true }, 202); + // its own id, for a cancel by the one that asked for it + const id = randomUUID(); + service.request({ command: "install", expected_revision: expected, install_id: id }); + return reply({ accepted: true, install_id: id }, 202); }); } diff --git a/server/update-ipc.test.ts b/server/update-ipc.test.ts index b22d76eee4..7378e522d3 100644 --- a/server/update-ipc.test.ts +++ b/server/update-ipc.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { noInstalledNotes, noUpdateNotes, readUpdateIpcRequest, unmanagedUpdateStatus, type UpdateCommand, type UpdateRequest, type UpdateStatus } from "../shared/update.ts"; +import { noInstalledNotes, noUpdateNotes, readUpdateIpcRequest, unmanagedUpdateStatus, type UpdateCommand, type UpdateRequest, type UpdateStatus, INSTALL_ID } from "../shared/update.ts"; import { connectUpdater, handleUpdateRequest } from "./update-api.ts"; // The bridge and the supervisor that runs it, over a stand-in for their IPC channel. Messages go @@ -64,12 +64,14 @@ describe("the update IPC between the bridge and its supervisor", () => { current.tell(offer(A)); expect(current.bridge.status().revision_bound_install).toBe(true); expect(await post(current.bridge, A)).toEqual({ status: 202, code: null }); - expect(current.requests).toEqual([{ command: "install", expected_revision: A }]); + expect(current.requests).toEqual([{ command: "install", expected_revision: A, install_id: expect.stringMatching(INSTALL_ID) }]); // what an older bridge sends for its Update: no revision, so no install expect(readUpdateIpcRequest({ type: "update-command", command: "install" })).toBeNull(); expect(readUpdateIpcRequest({ type: "update-command", command: "check" })).toEqual({ command: "check" }); // a cancel goes as a type of its own, which an older supervisor ignores - expect(readUpdateIpcRequest({ type: "update-cancel" })).toEqual({ command: "cancel" }); + // naming the install it stops; one that names none, or not an id, is no cancel + expect(readUpdateIpcRequest({ type: "update-cancel", install_id: "11111111-2222-4333-8444-555555555555" })).toEqual({ command: "cancel", install_id: "11111111-2222-4333-8444-555555555555" }); + for (const install_id of [undefined, "", "x", 7]) expect(readUpdateIpcRequest({ type: "update-cancel", install_id })).toBeNull(); }); it("reads only a full lowercase commit id, and nothing from a message out of shape", () => { @@ -92,7 +94,7 @@ describe("the update IPC between the bridge and its supervisor", () => { expect(current.requests).toEqual([]); // the second tab rendered B and installs B, by name expect(await post(current.bridge, B)).toEqual({ status: 202, code: null }); - expect(current.requests).toEqual([{ command: "install", expected_revision: B }]); + expect(current.requests).toEqual([{ command: "install", expected_revision: B, install_id: expect.stringMatching(INSTALL_ID) }]); // under an older supervisor neither tab installs anything const older = olderSupervisor(); older.tell(offer(A)); diff --git a/server/updater.test.ts b/server/updater.test.ts index 2da8a5cb23..eecc711e4e 100644 --- a/server/updater.test.ts +++ b/server/updater.test.ts @@ -1553,10 +1553,12 @@ it("cancels an install while it builds: nothing is installed, its offer stays, a await watched.initialize(); try { // nothing to cancel before an install runs - expect(watched.cancel()).toBe(false); - const install = watched.request("install"); + expect(watched.cancel("11111111-2222-4333-8444-555555555555")).toBe(false); + const install = watched.request("install", true, undefined, "11111111-2222-4333-8444-555555555555"); await building.promise; - expect(watched.cancel()).toBe(true); + // another's id (another device's install, or this PC's own Settings') never stops it + expect(watched.cancel("99999999-2222-4333-8444-555555555555")).toBe(false); + expect(watched.cancel("11111111-2222-4333-8444-555555555555")).toBe(true); await install; expect(watched.status).toMatchObject({ phase: "error", error_code: "update_cancelled", current_revision: running, available: true, install_cancellable: true }); expect(existsSync(join(stateDir, "failed.json"))).toBe(false); @@ -1577,12 +1579,12 @@ it("stops an install cancelled while it still checks what is on offer, before an const watched: Updater = new Updater({ ...updater.options, publish(status) { seen.push(status.step ?? null); // asked as soon as the run starts checking - if (status.phase === "checking" && !asked) asked = watched.cancel(); + if (status.phase === "checking" && !asked) asked = watched.cancel("11111111-2222-4333-8444-555555555555"); } }); let asked = false; await watched.initialize(); try { - await watched.request("install"); + await watched.request("install", true, undefined, "11111111-2222-4333-8444-555555555555"); expect(asked).toBe(true); expect(watched.status).toMatchObject({ phase: "error", error_code: "update_cancelled", available: true }); expect(seen).not.toContain("build"); diff --git a/server/updater.ts b/server/updater.ts index ebb178b22a..4727fa367d 100644 --- a/server/updater.ts +++ b/server/updater.ts @@ -219,6 +219,8 @@ export class Updater { private cancelledRevision: string | null = null; /** a cancel asked for while the run still checked: honoured before anything is built */ private cancelPending = false; + /** the id the install running now was asked for with, or null (the timer's, or no id given) */ + private runId: string | null = null; private timer?: ReturnType; private initialTimer?: ReturnType; private sourceRevision = ""; @@ -467,8 +469,12 @@ export class Updater { return run(this.options.root, ["git", "fetch", "--no-tags", "--unshallow", "origin"], this.controller.signal, timeout); } - /** Stops the install that is building now: true when there was one to stop. Once it restarts, false. */ - cancel(): boolean { + /** + * Stops the install running now, only when `installId` names it (the id its install request carried): true when it did. + * Another's install (another device's, or this PC's own Settings') is never stopped. Once it restarts, false. + */ + cancel(installId: string): boolean { + if (!this.runId || installId !== this.runId) return false; if (this.installing && this.status.phase === "building") { this.installing.abort(new Error("Update cancelled")); return true; } // asked while it still checks what is on offer: it stops before it builds anything if (this.busy && this.status.phase === "checking") { this.cancelPending = true; return true; } @@ -479,9 +485,11 @@ export class Updater { * from the 5-minute timer; only the timer waits a failed unshallow fetch's backoff out. * `expectedRevision`: the release a manual install was offered; the install goes ahead only * while the check it starts with still offers exactly that one. */ - async request(command: UpdateCommand, byUser = false, expectedRevision?: string): Promise { + async request(command: UpdateCommand, byUser = false, expectedRevision?: string, installId?: string): Promise { if (this.busy || !this.status.managed || this.controller.signal.aborted) return; this.busy = true; + // the id of the install this run is (its request's): what a cancel must name + this.runId = command === "install" ? installId ?? null : null; let stage: string | null = null; let attempted: string | null = null; let checked = false; @@ -595,6 +603,7 @@ export class Updater { if (stage) rmSync(stage, { recursive: true, force: true }); this.installing = null; this.cancelPending = false; + this.runId = null; this.busy = false; } } diff --git a/shared/protocol.ts b/shared/protocol.ts index ef5e93f768..c1f1ae80b7 100644 --- a/shared/protocol.ts +++ b/shared/protocol.ts @@ -184,8 +184,10 @@ export type { DevicePairedEvent, DeviceRemovedEvent, PairingIntent, RelayPairing * GET /api/updates -> UpdateStatus (shared/update.ts), no-store; to the bridge * token (which cannot read /api/access) also `port` and `serving_url`, where it answers * POST /api/updates/check -> 202 { accepted: true } - * POST /api/updates/install -> 202 { accepted: true }; UpdateInstallRequest JSON body - * POST /api/updates/cancel -> 202 { accepted: true }: stops the install that builds (install_cancellable); 409 update_not_cancellable or update_restart_required + * POST /api/updates/install -> 202 { accepted: true, install_id }; UpdateInstallRequest JSON body + * POST /api/updates/cancel -> 202 { accepted: true }; { install_id } JSON body, as the install's 202 gave it: stops that + * install only, while it checks or builds (install_cancellable); 400 invalid_update_cancel, + * 409 update_not_cancellable or update_restart_required * (the bridge token may cancel any install that builds there, as it may start one: it is this * PC owner's own connection server, which asks only for an install it requested) * (at most 1 KiB): 400 invalid_update_install for a missing or malformed revision, diff --git a/shared/update.ts b/shared/update.ts index 18ec8d5650..923bf58825 100644 --- a/shared/update.ts +++ b/shared/update.ts @@ -82,7 +82,12 @@ export const REVISION = /^[0-9a-f]{40,64}$/; * What the bridge asks of the update service: a check, or the install of the exact revision the * person saw offered. A manual install never goes without its revision. */ -export type UpdateRequest = { command: "check" } | { command: "install"; expected_revision: string } | { command: "cancel" }; +export type UpdateRequest = { command: "check" } | { command: "install"; expected_revision: string; install_id?: string } | { command: "cancel"; install_id: string }; +/** + * An install's own id, given in its 202 (POST /api/updates/install) to whoever asked for it: a cancel names it, so only the one + * that asked for an install can stop it, never another device's or the PC's own Settings'. + */ +export const INSTALL_ID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/; /** * The supervisor's reading of a bridge's IPC message. `update-command` carries only a check: an @@ -91,13 +96,13 @@ export type UpdateRequest = { command: "check" } | { command: "install"; expecte * running it as an install of whatever its own check finds. */ export function readUpdateIpcRequest(message: unknown): UpdateRequest | null { - const value = message as { type?: unknown; command?: unknown; expected_revision?: unknown } | null | undefined; + const value = message as { type?: unknown; command?: unknown; expected_revision?: unknown; install_id?: unknown } | null | undefined; if (typeof value !== "object" || value === null) return null; if (value.type === "update-command" && value.command === "check") return { command: "check" }; // a cancel travels as a type an older supervisor does not know, and so ignores - if (value.type === "update-cancel") return { command: "cancel" }; + if (value.type === "update-cancel") return typeof value.install_id === "string" && INSTALL_ID.test(value.install_id) ? { command: "cancel", install_id: value.install_id } : null; if (value.type === "update-install-revision" && typeof value.expected_revision === "string" && REVISION.test(value.expected_revision)) { - return { command: "install", expected_revision: value.expected_revision }; + return { command: "install", expected_revision: value.expected_revision, ...(typeof value.install_id === "string" && INSTALL_ID.test(value.install_id) ? { install_id: value.install_id } : {}) }; } return null; } From dbd6472b7e040f6ee6664a6ef7358663aed4ad26 Mon Sep 17 00:00:00 2001 From: devswha Date: Mon, 9 Nov 2026 03:42:09 +0900 Subject: [PATCH 3/5] fix(updates): the cancel IPC gets a name no older supervisor knows; a wrong or expired install_id is refused like no update The lead's last round on #3384 (remote bundles can update the server apart from its supervisor): - The supervisor IPC message is update-cancel-v2: a supervisor that cancelled without an id knew update-cancel, and ignores the new name. The old name is never read as a cancel. - POST /api/updates/cancel passes on only the install_id this server was last asked for, while that install checks or builds; a wrong or expired id gets the same 409 update_not_cancellable, word for word, as when no update runs, so the answer tells nothing of another's install. --- server/update-api.test.ts | 34 +++++++++++++++++++++++++++++++--- server/update-api.ts | 11 ++++++++--- server/update-ipc.test.ts | 6 ++++-- shared/protocol.ts | 3 ++- shared/update.ts | 4 ++-- 5 files changed, 47 insertions(+), 11 deletions(-) diff --git a/server/update-api.test.ts b/server/update-api.test.ts index 98bec17634..5f8acfec8f 100644 --- a/server/update-api.test.ts +++ b/server/update-api.test.ts @@ -62,14 +62,19 @@ const answer = async (response: Response | Promise) => { describe("a cancel", () => { it("stops only an install that builds, and only where its supervisor can", async () => { - let status = offer({ phase: "building", install_cancellable: true }); + let status = offer(); const updates = service(() => status); - const cancel = (id: string | null = "11111111-2222-4333-8444-555555555555") => answer(handleUpdateRequest(install(id === null ? "{}" : JSON.stringify({ install_id: id })), "/api/updates/cancel", updates)); + // the install it may cancel: the one this server was asked for, by the id its 202 gave + const granted = await handleUpdateRequest(install(JSON.stringify({ expected_revision: offered })), "/api/updates/install", updates); + const { install_id: issued } = await granted.json() as { install_id: string }; + updates.requests.splice(0); + status = offer({ phase: "building", install_cancellable: true }); + const cancel = (id: string | null = issued) => answer(handleUpdateRequest(install(id === null ? "{}" : JSON.stringify({ install_id: id })), "/api/updates/cancel", updates)); // only by naming the install it stops (its 202's install_id): never another's for (const id of [null, "not-an-id", "ABCDEF11-2222-4333-8444-555555555555"]) expect(await cancel(id)).toEqual({ status: 400, code: "invalid_update_cancel" }); expect(updates.requests).toEqual([]); expect(await cancel()).toEqual({ status: 202, code: null }); - expect(updates.requests).toEqual([{ command: "cancel", install_id: "11111111-2222-4333-8444-555555555555" }]); + expect(updates.requests).toEqual([{ command: "cancel", install_id: issued }]); // nothing building (restarting onto the new release, or idle): past or nothing to stop // still checking what it installs: stopped before it builds status = offer({ phase: "checking", install_cancellable: true }); @@ -83,6 +88,29 @@ describe("a cancel", () => { }); }); +describe("a cancel by its install's id", () => { + it("refuses a wrong or expired id exactly as it refuses one when no update runs", async () => { + let status = offer(); + const updates = service(() => status); + const granted = await handleUpdateRequest(install(JSON.stringify({ expected_revision: offered })), "/api/updates/install", updates); + const { install_id } = await granted.json() as { install_id: string }; + const cancel = async (id: string) => { const response = await handleUpdateRequest(install(JSON.stringify({ install_id: id })), "/api/updates/cancel", updates); return { status: response.status, body: await response.json() }; }; + // its install builds: another id is refused, as if no update ran + status = offer({ phase: "building", install_cancellable: true }); + const wrong = await cancel("99999999-2222-4333-8444-555555555555"); + status = offer({ phase: "idle", install_cancellable: true }); + const none = await cancel("99999999-2222-4333-8444-555555555555"); + const expired = await cancel(install_id); + expect(wrong).toEqual(none); + expect(expired).toEqual(none); + expect(none.status).toBe(409); + // only its own id, while it runs, is passed on + status = offer({ phase: "building", install_cancellable: true }); + expect((await cancel(install_id)).status).toBe(202); + expect(updates.requests.filter((request) => request.command === "cancel")).toEqual([{ command: "cancel", install_id }]); + }); +}); + describe("a manual install", () => { it("rejects unbound manual installs", async () => { const updates = service(() => offer()); diff --git a/server/update-api.ts b/server/update-api.ts index 120eec4051..70c82a2684 100644 --- a/server/update-api.ts +++ b/server/update-api.ts @@ -71,7 +71,7 @@ export function connectUpdater(channel?: SupervisorChannel): UpdateService { // an install goes as a message an older supervisor does not know, never as its unbound `install` request: (request) => supervisor?.send(request.command === "install" ? { type: "update-install-revision", expected_revision: request.expected_revision, ...(request.install_id ? { install_id: request.install_id } : {}) } - : request.command === "cancel" ? { type: "update-cancel", install_id: request.install_id } : { type: "update-command", command: "check" }), + : request.command === "cancel" ? { type: "update-cancel-v2", install_id: request.install_id } : { type: "update-command", command: "check" }), // the supervisor starts the bridge in the directory it serves: the release's, or the source checkout ...(managed ? { servedRevision: servedRevision(process.cwd()) } : {}), }; @@ -135,6 +135,9 @@ async function smallBody(request: Request): Promise | nu } catch { return null; } } +/** the install_id of the install each update service was last asked for here: the only one a cancel may name */ +const issued = new WeakMap(); + const running = (status: UpdateStatus) => ["checking", "building", "restarting"].includes(status.phase); /** @@ -170,8 +173,9 @@ export function handleUpdateRequest(request: Request, pathname: string, service? return cancelledInstall(request).then((id) => { if (id === null) return fail("invalid_update_cancel", "Name the install_id of the install to cancel.", 400); if (status.install_cancellable !== true) return fail("update_restart_required", "Restart the app before cancelling an update from here.", 409); - // building, or still checking what it installs: stopped before anything is built (the supervisor stops only that install) - if (status.phase !== "building" && status.phase !== "checking") return fail("update_not_cancellable", "No update is building that could be cancelled.", 409); + // only the install this server was last asked for, while it still checks or builds: a wrong or expired id is refused + // exactly as when no update runs, so the answer tells nothing of another's install (the supervisor checks the id too) + if (id !== issued.get(service) || (status.phase !== "building" && status.phase !== "checking")) return fail("update_not_cancellable", "No update asked for with that install_id is running here.", 409); service.request({ command: "cancel", install_id: id }); return reply({ accepted: true }, 202); }); @@ -191,6 +195,7 @@ export function handleUpdateRequest(request: Request, pathname: string, service? if (now.latest_revision !== expected) return fail("update_offer_changed", "A different update is on offer now. Review it, then update again.", 409); // its own id, for a cancel by the one that asked for it const id = randomUUID(); + issued.set(service, id); service.request({ command: "install", expected_revision: expected, install_id: id }); return reply({ accepted: true, install_id: id }, 202); }); diff --git a/server/update-ipc.test.ts b/server/update-ipc.test.ts index 7378e522d3..c6404ee52a 100644 --- a/server/update-ipc.test.ts +++ b/server/update-ipc.test.ts @@ -70,8 +70,10 @@ describe("the update IPC between the bridge and its supervisor", () => { expect(readUpdateIpcRequest({ type: "update-command", command: "check" })).toEqual({ command: "check" }); // a cancel goes as a type of its own, which an older supervisor ignores // naming the install it stops; one that names none, or not an id, is no cancel - expect(readUpdateIpcRequest({ type: "update-cancel", install_id: "11111111-2222-4333-8444-555555555555" })).toEqual({ command: "cancel", install_id: "11111111-2222-4333-8444-555555555555" }); - for (const install_id of [undefined, "", "x", 7]) expect(readUpdateIpcRequest({ type: "update-cancel", install_id })).toBeNull(); + expect(readUpdateIpcRequest({ type: "update-cancel-v2", install_id: "11111111-2222-4333-8444-555555555555" })).toEqual({ command: "cancel", install_id: "11111111-2222-4333-8444-555555555555" }); + for (const install_id of [undefined, "", "x", 7]) expect(readUpdateIpcRequest({ type: "update-cancel-v2", install_id })).toBeNull(); + // the name a supervisor that cancels without an id might know: never read as a cancel + expect(readUpdateIpcRequest({ type: "update-cancel", install_id: "11111111-2222-4333-8444-555555555555" })).toBeNull(); }); it("reads only a full lowercase commit id, and nothing from a message out of shape", () => { diff --git a/shared/protocol.ts b/shared/protocol.ts index c1f1ae80b7..bf4f0afc57 100644 --- a/shared/protocol.ts +++ b/shared/protocol.ts @@ -187,7 +187,8 @@ export type { DevicePairedEvent, DeviceRemovedEvent, PairingIntent, RelayPairing * POST /api/updates/install -> 202 { accepted: true, install_id }; UpdateInstallRequest JSON body * POST /api/updates/cancel -> 202 { accepted: true }; { install_id } JSON body, as the install's 202 gave it: stops that * install only, while it checks or builds (install_cancellable); 400 invalid_update_cancel, - * 409 update_not_cancellable or update_restart_required + * 409 update_restart_required, or 409 update_not_cancellable alike for a wrong or expired + * install_id and for no update running (supervisor IPC: update-cancel-v2) * (the bridge token may cancel any install that builds there, as it may start one: it is this * PC owner's own connection server, which asks only for an install it requested) * (at most 1 KiB): 400 invalid_update_install for a missing or malformed revision, diff --git a/shared/update.ts b/shared/update.ts index 923bf58825..ae1a7d621a 100644 --- a/shared/update.ts +++ b/shared/update.ts @@ -99,8 +99,8 @@ export function readUpdateIpcRequest(message: unknown): UpdateRequest | null { const value = message as { type?: unknown; command?: unknown; expected_revision?: unknown; install_id?: unknown } | null | undefined; if (typeof value !== "object" || value === null) return null; if (value.type === "update-command" && value.command === "check") return { command: "check" }; - // a cancel travels as a type an older supervisor does not know, and so ignores - if (value.type === "update-cancel") return typeof value.install_id === "string" && INSTALL_ID.test(value.install_id) ? { command: "cancel", install_id: value.install_id } : null; + // a cancel travels as a type no older supervisor knows (one that cancelled without an id knew "update-cancel"), and so ignores + if (value.type === "update-cancel-v2") return typeof value.install_id === "string" && INSTALL_ID.test(value.install_id) ? { command: "cancel", install_id: value.install_id } : null; if (value.type === "update-install-revision" && typeof value.expected_revision === "string" && REVISION.test(value.expected_revision)) { return { command: "install", expected_revision: value.expected_revision, ...(typeof value.install_id === "string" && INSTALL_ID.test(value.install_id) ? { install_id: value.install_id } : {}) }; } From ea3b62d79572d0356812ed8e1f62e4602a719a59 Mon Sep 17 00:00:00 2001 From: devswha Date: Mon, 9 Nov 2026 04:09:57 +0900 Subject: [PATCH 4/5] test(relay-pc): an install request carries its install_id The relay PC's install path is unchanged and still installs: the PC is asked for the release shown, now with the install_id its 202 gives (the cancel contract of this PR). The contract test pinned the request without that id; it now expects it. --- server/relay-pc.contract.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/relay-pc.contract.test.ts b/server/relay-pc.contract.test.ts index c87300fbdc..9b2a4bf289 100644 --- a/server/relay-pc.contract.test.ts +++ b/server/relay-pc.contract.test.ts @@ -12,7 +12,7 @@ import { decodeOffer } from "../shared/relay-offer.ts"; import type { RelayOffer } from "../shared/relay-offer.ts"; import { bounded, deferred } from "./relay/test-helpers.ts"; import type { UpdateService } from "./update-api.ts"; -import { noInstalledNotes, noUpdateNotes, unmanagedUpdateStatus, type UpdateStatus } from "../shared/update.ts"; +import { noInstalledNotes, noUpdateNotes, unmanagedUpdateStatus, type UpdateStatus, INSTALL_ID } from "../shared/update.ts"; const cleanup: (() => void | Promise)[] = []; afterEach(async () => { for (const close of cleanup.splice(0).reverse()) await close(); }); @@ -606,7 +606,9 @@ it("shows a relay PC's own update on its card and installs it from here, with it expect(started.status).toBe(202); const { job_id } = await started.json() as { job_id: string }; expect((await step).updating).toMatchObject({ job_id, ovis: true }); - expect(b.requests).toEqual([{ command: "install", expected_revision: OFFERED_RELEASE }]); + // the install names the release shown, and carries the id its 202 gave (POST /api/updates/install), which only a cancel by + // the one that asked for it names + expect(b.requests).toEqual([{ command: "install", expected_revision: OFFERED_RELEASE, install_id: expect.stringMatching(INSTALL_ID) }]); const done = await machineWhen(a.base, (pc) => pc.id === machine.id && !pc.updating && pc.ovis_update === null); expect(done.update_error ?? null).toBeNull(); // nothing more on offer: asked again, it is refused in plain words From 51a26ece691b1cd131cc241b5c11265503a58137 Mon Sep 17 00:00:00 2001 From: devswha Date: Mon, 9 Nov 2026 04:32:47 +0900 Subject: [PATCH 5/5] test(relay-shell): an install request carries its install_id scripts/relay-shell-browser-qa.ts (run by relay-e2e-regression.ts) pinned the install request the PC's update service received without the install_id this PR's cancel contract adds. It now checks one install of the release on offer, by name, with an id in INSTALL_ID's shape. No other script or test pins that request or the install's 202 body; the clients' POST bodies are unchanged ({ expected_revision }). --- scripts/relay-shell-browser-qa.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/relay-shell-browser-qa.ts b/scripts/relay-shell-browser-qa.ts index 01423152ca..d3585e2e75 100644 --- a/scripts/relay-shell-browser-qa.ts +++ b/scripts/relay-shell-browser-qa.ts @@ -39,7 +39,7 @@ import { shellContentSecurityPolicy } from "../server/static.ts"; import { REGIONAL_APPS } from "../shared/relay-regions.ts"; import type { UpdateService } from "../server/update-api.ts"; import { MIN_SERVER_VERSION } from "../shared/relay-mux.ts"; -import { noInstalledNotes, noUpdateNotes } from "../shared/update.ts"; +import { noInstalledNotes, noUpdateNotes, INSTALL_ID } from "../shared/update.ts"; import type { UpdateRequest, UpdateStatus } from "../shared/update.ts"; import { version as shellVersion } from "../package.json"; import { exportPublicKey, generateKeyPair, openDaemonSecureChannel } from "../shared/relay-e2ee.ts"; @@ -1140,7 +1140,11 @@ export async function relayShellQa(tree: RelayServerTree): Promise { assert.deepEqual(updates.requests, [], "nothing is installed before the tap"); await page.getByRole("button", { name: "Update now" }).click(); await bounded(updates.installed, "the PC's update service received the install"); - assert.deepEqual(updates.requests, [{ command: "install", expected_revision: "b".repeat(40) }], "one install of the release on offer, no check first"); + // one install of the release on offer, named by it, with the install_id its 202 gave (a cancel names that id) + assert.equal(updates.requests.length, 1, "one install of the release on offer, no check first"); + const [asked] = updates.requests as { command?: string; expected_revision?: string; install_id?: string }[]; + assert.deepEqual([asked?.command, asked?.expected_revision], ["install", "b".repeat(40)], "one install of the release on offer, no check first"); + assert.match(asked?.install_id ?? "", INSTALL_ID, "the install carries the id its 202 gave"); await page.getByText("Step 4 of 5: Building the app").waitFor(); await shot19(page, "update"); note(`pc-outdated (managed): WELCOME ${below} < ${MIN_SERVER_VERSION} -> update screen; Update now -> POST /api/updates/install over the relay -> fake service recorded ${JSON.stringify(updates.requests)}; progress from GET /api/updates`);