diff --git a/src/update/index.ts b/src/update/index.ts index a8af29661dc..cfe1c237b19 100644 --- a/src/update/index.ts +++ b/src/update/index.ts @@ -11,7 +11,10 @@ import { readPid, readRuntimePort } from "../config/process-state"; import { pendingTeardownOutstanding } from "../config/pending-teardown"; import type { ServiceOwnership } from "../service/state"; import { planUpdateRuntimeHandling } from "./runtime-ownership.mjs"; -import { acquireOwnershipMutationLease } from "../service/ownership-mutation-lease.mjs"; +import { + acquireOwnershipMutationLease, + ownershipMutationLeaseChildEnvironment, +} from "../service/ownership-mutation-lease.mjs"; import { npmInvocation } from "./npm-invocation.mjs"; import { pnpmInvocation, pnpmInvocationForPath, resolvePnpmCommands } from "./pnpm-invocation.mjs"; import { detectInstallFromPath } from "./install-detection.mjs"; @@ -421,7 +424,7 @@ export async function runUpdate(): Promise { // What this update may do to the runtime. A desktop takeover vetoes both the stop and the // service refresh below; see `planUpdateRuntimeHandling` for why each half is wrong. const initialOwnership = await resolvedRuntimeOwnership(); - const runtimePlan = planUpdateRuntimeHandling({ + let runtimePlan = planUpdateRuntimeHandling({ ...initialOwnership, serviceInstalled: serviceWasInstalled, }); @@ -466,6 +469,36 @@ export async function runUpdate(): Promise { ...(runtimeTrusted && livePid ? { oldPid: livePid } : {}), }; + const { serviceStatePaths } = await import("../service"); + const replacementLease = acquireOwnershipMutationLease(serviceStatePaths()); + const lockedOwnership = await resolvedRuntimeOwnership(); + const lockedPlan = planUpdateRuntimeHandling({ + ...lockedOwnership, + serviceInstalled: serviceWasInstalled, + }); + if (lockedOwnership.subjectToken !== initialOwnership.subjectToken || !lockedPlan.mayReplacePackage) { + replacementLease.release(); + console.error(lockedPlan.notice + ?? "⚠️ Update stopped because runtime ownership changed before stop authorization; rerun from the beginning."); + process.exit(1); + } + runtimePlan = lockedPlan; + const stopEnvironment = ownershipMutationLeaseChildEnvironment(process.env, replacementLease.token); + let replacementRefusal: string | null = null; + let stopAttempted = false; + const installStdio = updateChildStdio(); + let postUpdateLauncher = installer === "pnpm" && owner + ? join(owner.packagePath, "bin", "ocx.mjs") + : join(packageRoot(), "bin", "ocx.mjs"); + let postUpdateLauncherUsable = true; + let r: { + status: number | null; + signal?: NodeJS.Signals | null; + stdout?: string | Buffer | null; + stderr?: string | Buffer | null; + } | null = null; + try { + // Never replace package files under a live proxy: the running server dynamic-imports // modules after startup, so an in-place update leaves it executing mixed old/new code. // Gate on the service and the runtime-port record too, not just the pid file — a @@ -476,7 +509,6 @@ export async function runUpdate(): Promise { // shared client config still points at a proxy that is gone; installing over that // silently skips the recovery the receipt was written to trigger (#3008). // Full `ocx stop` semantics (drain, service stop, restore). - let stopAttempted = false; if (runtimePlan.mayStopRuntime && (serviceWasInstalled || readPid() || readRuntimePort() || pendingTeardownOutstanding())) { stopAttempted = true; console.log("⏹ Stopping the running proxy before updating..."); @@ -485,6 +517,7 @@ export async function runUpdate(): Promise { stdio: stopStdio, encoding: stopStdio === "pipe" ? "utf8" : undefined, windowsHide: true, + env: stopEnvironment, }); if (stopStdio === "pipe") logSpawnOutput("", stop); // One decision, shared with the package launcher (#3008). The two lanes disagreeing about @@ -517,6 +550,7 @@ export async function runUpdate(): Promise { ? `⚠️ Could not confirm the proxy on ${capturedListen.hostname}:${capturedListen.port} is stopped; aborting the update. Run 'ocx stop' and retry.` : "⚠️ Could not stop the running proxy; aborting the update. Run 'ocx stop' and retry."); } + replacementLease.release(); process.exit(1); } if (historyOnlyStop || historyRestoreIncomplete()) { @@ -538,24 +572,8 @@ export async function runUpdate(): Promise { } } - const installStdio = updateChildStdio(); - let postUpdateLauncher = installer === "pnpm" && owner - ? join(owner.packagePath, "bin", "ocx.mjs") - : join(packageRoot(), "bin", "ocx.mjs"); - let postUpdateLauncherUsable = true; - let r: { - status: number | null; - signal?: NodeJS.Signals | null; - stdout?: string | Buffer | null; - stderr?: string | Buffer | null; - } | null = null; - const { serviceStatePaths } = await import("../service"); - const replacementLease = acquireOwnershipMutationLease(serviceStatePaths()); - let replacementRefusal: string | null = null; - try { - // Ownership can change while registry and stop work is in flight. Unknown at this exact - // boundary blocks replacement; a confirmed desktop claim still permits updating the idle - // npm installation while leaving the bundled sidecar alone. + // Re-read even though cooperating ownership changes are fenced by the lease: an unreadable + // or externally replaced record still blocks replacement at the final package boundary. const replacementOwnership = await resolvedRuntimeOwnership(); const replacementPlan = planUpdateRuntimeHandling({ ...replacementOwnership, diff --git a/tests/cli/cli-status-json.test.ts b/tests/cli/cli-status-json.test.ts index 4d8d393e8db..1c31d8e1dc6 100644 --- a/tests/cli/cli-status-json.test.ts +++ b/tests/cli/cli-status-json.test.ts @@ -12,6 +12,7 @@ import * as statusFacade from "../../src/cli/status"; import * as statusProbes from "../../src/cli/status-probes"; import { packageVersion } from "../../src/cli/help"; import { getDefaultConfig } from "../../src/config"; +import { isProcessAlive } from "../../src/lib/process-control"; import { findDeadPid } from "../helpers/dead-pid"; import { COLD_SPAWN_WARMUP_HOOK_BUDGET_MS, warmColdSpawn } from "../helpers/cold-spawn-warmup"; import { removeTreeWithRetry } from "../helpers/remove-tree"; @@ -1061,7 +1062,7 @@ describe("status reports stale process records end to end", () => { await new Promise(resolve => { occupied.listen(0, "127.0.0.1", () => resolve()); }); const occupiedPort = (occupied.address() as AddressInfo).port; try { - const pid = findDeadPid(); + let pid = findDeadPid(); writeFileSync(join(home, "config.json"), JSON.stringify({ port: occupiedPort, codexAutoStart: false }), "utf8"); writeFileSync(join(home, "ocx.pid"), String(pid), "utf8"); @@ -1069,14 +1070,24 @@ describe("status reports stale process records end to end", () => { // hands back a port it has already released. Confirm refusal immediately before and // immediately after the probe, and re-allocate when something took it in between, so // a stolen port retries instead of failing an assertion it never exercised. + // + // The seeded dead pid carries the same hazard: `findDeadPid` proves it free once, + // and a pid reclaimed later makes the probe correctly judge the records as owned by + // a live process on every remaining attempt — the same misreading as a stolen port. + // Re-verify liveness around the probe and re-seed the records when it is taken. let parsed: { proxy?: { staleProcessState?: unknown } } | undefined; for (let attempt = 0; attempt < 5 && parsed === undefined; attempt++) { + if (isProcessAlive(pid)) { + pid = findDeadPid(); + writeFileSync(join(home, "ocx.pid"), String(pid), "utf8"); + } const recordedPort = await allocateFreePort(); if (recordedPort === occupiedPort) continue; if (!await refusesConnection(recordedPort)) continue; writeFileSync(join(home, "runtime-port.json"), JSON.stringify({ pid, port: recordedPort, hostname: "127.0.0.1" }), "utf8"); const observed = JSON.parse(runStatusJson(home).stdout) as { proxy?: { staleProcessState?: unknown } }; if (!await refusesConnection(recordedPort)) continue; + if (isProcessAlive(pid)) continue; // The TCP check can refuse while the HTTP /healthz probe aborts at 800ms // without an ECONNREFUSED code. That leaves staleProcessState false even // though the recorded port is still empty; retry instead of treating a diff --git a/tests/update/update-desktop-owner.test.ts b/tests/update/update-desktop-owner.test.ts index 004266a7df4..e96d9c51f34 100644 --- a/tests/update/update-desktop-owner.test.ts +++ b/tests/update/update-desktop-owner.test.ts @@ -176,6 +176,20 @@ describe("both updaters consult the shared rule", () => { expect(bunPath).toContain("if (postInstallPlan.mayRestoreService) {"); }); + test("the Bun updater fences and delegates its final stop authorization", () => { + const leaseAt = bunPath.indexOf("const replacementLease = acquireOwnershipMutationLease"); + const lockedReadAt = bunPath.indexOf("const lockedOwnership = await resolvedRuntimeOwnership()", leaseAt); + const stopAt = bunPath.indexOf('selfLaunchArgv(["stop"])', lockedReadAt); + const releaseAt = bunPath.indexOf("replacementLease.release()", stopAt); + expect(leaseAt).toBeGreaterThan(-1); + expect(lockedReadAt).toBeGreaterThan(leaseAt); + expect(stopAt).toBeGreaterThan(lockedReadAt); + expect(releaseAt).toBeGreaterThan(stopAt); + expect(bunPath.slice(lockedReadAt, stopAt)).toContain("lockedOwnership.subjectToken !== initialOwnership.subjectToken"); + expect(bunPath.slice(lockedReadAt, stopAt)).toContain("ownershipMutationLeaseChildEnvironment"); + expect(bunPath.slice(stopAt, releaseAt)).toContain("env: stopEnvironment"); + }); + test("the npm launcher gates its stop, its refresh and its failure recovery", () => { expect(launcher).toContain("from \"../src/update/runtime-ownership.mjs\""); expect(launcher).toContain("if (stopNeeded && !runtimePlan.mayStopRuntime)");