From 74ce592792261661b116be093195680eba64e96a Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 11 Nov 2026 20:43:59 +0900 Subject: [PATCH] fix(update): repair the Windows installer tests broken by #8534 The dev batch went red on four Windows shards (run 43000940642). Two causes: - The Windows supervisor tests run a private copy of npm-install.mjs from a data: URL and rewrote only "./mutation-guard.mjs" to a file URL. #8534 added "./windows-installer-marker.mjs" and "./test-home-guard.mjs", which a data: URL cannot resolve. A shared test loader now pins every relative ./*.mjs import and refuses a copy with an unpinned one, and a new case loads the copy on every host so CI catches this without the Windows lane. - #8534 stopped probing a recorded installerPid on win32 unless terminationUnconfirmed was set, so a marker with a live installer read free on Windows. confirmInstallerExit already drops the Windows witness, so the rule was unnecessary: the pid is probed again on every platform. Refs #8524 --- src/update/sync-update-mutex.mjs | 7 +++---- structure/ops/service-and-sidecars.md | 2 +- tests/helpers/update-module-data-url.ts | 18 +++++++++++++++++ .../update/update-npm-supervisor-exit.test.ts | 11 ++++++++-- tests/update/update-npm-timeout.test.ts | 4 ++-- .../update-windows-installer-witness.test.ts | 20 +++++++++++++++---- 6 files changed, 49 insertions(+), 13 deletions(-) create mode 100644 tests/helpers/update-module-data-url.ts diff --git a/src/update/sync-update-mutex.mjs b/src/update/sync-update-mutex.mjs index 311298afee3..5bb0a954316 100644 --- a/src/update/sync-update-mutex.mjs +++ b/src/update/sync-update-mutex.mjs @@ -177,10 +177,9 @@ export function inspectUpdateMarker(home, { now = Date.now, platform = process.p for (const key of ["installerPid", "installerPgid"]) { if (marker[key] === undefined) continue; if (!Number.isSafeInteger(marker[key]) || marker[key] <= 0 || (key === "installerPgid" && windows)) return "unknown"; - // On Windows installerPid names the Job Object supervisor and witnesses only an installer whose - // exit was not confirmed (#8524). A confirmed one needs no witness, and probing a stale pid - // that Windows has already reused would report a finished update as still in flight. - if (key === "installerPid" && windows && marker.terminationUnconfirmed !== true) continue; + // A recorded installer pid is probed whenever present, on every platform: a live installer + // keeps the marker fenced. On Windows confirmInstallerExit drops the supervisor pid, so a + // finished update leaves none to be held by a reused pid (#8524). ids.push(key === "installerPgid" ? -marker[key] : marker[key]); } let unknown = false; diff --git a/structure/ops/service-and-sidecars.md b/structure/ops/service-and-sidecars.md index e60bd039537..407a426a007 100644 --- a/structure/ops/service-and-sidecars.md +++ b/structure/ops/service-and-sidecars.md @@ -416,7 +416,7 @@ For orchestrated Bun installs, `src/update/orchestrated-entry.ts` starts an exac stop, package replacement and recovery work. The parent never puts its token in the global environment. Fixed stop/service/direct-recovery children can join it; package-manager and ancillary children receive environments without the capability. Refusals return through the -lease boundary before exiting, and thrown failures release it after owner-aware recovery. Before installer launch the mutation marker carries a 15-minute fence from worker start; a dead worker, timed-out or killed installer, or unconfirmed exit retains that fence, while live or uninspectable recorded installers remain fenced independently. For POSIX npm, pnpm and Bun installs, `src/update/posix-install-supervisor.mjs` starts a gated process-group leader, publishes its PID and group ID before allowing the installer to run, and terminates the group on the 180-second deadline. On normal installer exit and confirmed group disappearance, finalization clears the fence. Process groups cannot contain detached descendants: an escaped descendant can outlive a normally exiting installer and continue after the fence clears, an accepted residual; it can also outlive a timeout fence. Windows uses the Job Object supervisor in `src/update/npm-install.mjs` and clears the fence only after normal exit and an empty job. Before the supervisor receives its request, `src/update/windows-installer-marker.mjs` records its PID as `installerPid` on a marker awaiting an installer. Because the job is kill-on-close and only the supervisor holds its handle, that PID is the Windows process-tree witness (#8524). `inspectUpdateMarker` reports an unconfirmed Windows installer free once the fence has passed and both the launcher PID and that supervisor PID are gone. A confirmed exit drops the Windows-shaped witness, and a confirmed Windows marker (no `terminationUnconfirmed`) never probes its old PID. A supervisor whose PID cannot be recorded receives no request and reports an unstarted installer. A live earlier supervisor keeps its witness and the new run stays unconfirmed. An unconfirmed marker without a witness, such as one written by an older release, stays uncertain. The inspection reports that case through its `detail` out-parameter, and only a refusal it causes prints `staleUpdateMarkerHint`: the marker path and a reversible rename, after the unchanged `update-in-flight` line. This covers the legacy session start through a non-wire `staleMarker` flag and the sync guards through `refuseSync`. +lease boundary before exiting, and thrown failures release it after owner-aware recovery. Before installer launch the mutation marker carries a 15-minute fence from worker start; a dead worker, timed-out or killed installer, or unconfirmed exit retains that fence, while live or uninspectable recorded installers remain fenced independently. For POSIX npm, pnpm and Bun installs, `src/update/posix-install-supervisor.mjs` starts a gated process-group leader, publishes its PID and group ID before allowing the installer to run, and terminates the group on the 180-second deadline. On normal installer exit and confirmed group disappearance, finalization clears the fence. Process groups cannot contain detached descendants: an escaped descendant can outlive a normally exiting installer and continue after the fence clears, an accepted residual; it can also outlive a timeout fence. Windows uses the Job Object supervisor in `src/update/npm-install.mjs` and clears the fence only after normal exit and an empty job. Before the supervisor receives its request, `src/update/windows-installer-marker.mjs` records its PID as `installerPid` on a marker awaiting an installer. Because the job is kill-on-close and only the supervisor holds its handle, that PID is the Windows process-tree witness (#8524). `inspectUpdateMarker` reports an unconfirmed Windows installer free once the fence has passed and both the launcher PID and that supervisor PID are gone. A confirmed exit drops the Windows-shaped witness, so a finished update leaves no PID for a reused number to hold; a recorded installer PID is otherwise probed on every platform. A supervisor whose PID cannot be recorded receives no request and reports an unstarted installer. A live earlier supervisor keeps its witness and the new run stays unconfirmed. An unconfirmed marker without a witness, such as one written by an older release, stays uncertain. The inspection reports that case through its `detail` out-parameter, and only a refusal it causes prints `staleUpdateMarkerHint`: the marker path and a reversible rename, after the unchanged `update-in-flight` line. This covers the legacy session start through a non-wire `staleMarker` flag and the sync guards through `refuseSync`. Replacement and recovery inspect both the captured endpoint and the freshly read runtime record. Malformed or unreadable records remain unknown. Recovery requires the same complete owner identity and proven-dead liveness; unknown or transferred ownership never starts another proxy. diff --git a/tests/helpers/update-module-data-url.ts b/tests/helpers/update-module-data-url.ts new file mode 100644 index 00000000000..1a4c983665c --- /dev/null +++ b/tests/helpers/update-module-data-url.ts @@ -0,0 +1,18 @@ +import { pathToFileURL } from "node:url"; +import { repoPath } from "./repo-root"; + +/** + * Import a private, instrumented copy of a src/update module from its source text. + * + * A data: URL has no base to resolve relative specifiers against, so every relative + * "./x.mjs" import is pinned to the shipped file's file: URL. A copy that still carries an + * unpinned relative import is refused here rather than failing only on the Windows lane + * (#8534 added "./windows-installer-marker.mjs" and "./test-home-guard.mjs" to npm-install.mjs, + * and the copies that rewrote only "./mutation-guard.mjs" broke on every Windows shard). + */ +export async function importUpdateModuleCopy(source: string): Promise { + const pinned = source.replace(/(from\s+)"\.\/([\w.-]+\.mjs)"/g, + (_match, from: string, file: string) => `${from}${JSON.stringify(pathToFileURL(repoPath("src/update", file)).href)}`); + if (/(?:from\s+|import\s*\()["']\.\.?\//.test(pinned)) throw new Error("update module copy still has an unpinned relative import"); + return await import("data:text/javascript;base64," + Buffer.from(pinned).toString("base64")) as T; +} diff --git a/tests/update/update-npm-supervisor-exit.test.ts b/tests/update/update-npm-supervisor-exit.test.ts index 6540aa685ef..eb61f93bc5f 100644 --- a/tests/update/update-npm-supervisor-exit.test.ts +++ b/tests/update/update-npm-supervisor-exit.test.ts @@ -1,4 +1,3 @@ -import { pathToFileURL } from "node:url"; import { expect, test } from "bun:test"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -7,6 +6,7 @@ import { spawnSync } from "node:child_process"; import { transactionalNpmUpdateAsync, UPDATE_OWNER_MARKER } from "../../src/update/transactional-install.mjs"; import type { runNpmInstall } from "../../src/update/npm-install.mjs"; import { fixturePath, repoPath } from "../helpers/repo-root"; +import { importUpdateModuleCopy } from "../helpers/update-module-data-url"; const alive = (pid: number) => { try { process.kill(pid, 0); return true; } @@ -25,9 +25,16 @@ async function runner(root: string, transform: (source: string) => string): Prom const source = readFileSync(repoPath("src/update/npm-install.mjs"), "utf8"); const anchor = 'const helper = join(dirname(fileURLToPath(import.meta.url)), "npm-install-job.ps1");'; expect(source.split(anchor)).toHaveLength(2); - return (await import("data:text/javascript;base64," + Buffer.from(source.replace(anchor, `const helper = ${JSON.stringify(helper)};`).replaceAll('"./mutation-guard.mjs"', JSON.stringify(pathToFileURL(repoPath("src/update/mutation-guard.mjs")).href))).toString("base64"))).runNpmInstall; + return (await importUpdateModuleCopy<{ runNpmInstall: typeof runNpmInstall }>(source.replace(anchor, `const helper = ${JSON.stringify(helper)};`))).runNpmInstall; } +test("a private copy of npm-install.mjs resolves every import on every host", async () => { + // The Windows-only cases below load this copy too; a relative import that a data: URL cannot + // resolve must fail here, on every CI host, instead of only on the Windows lane. + const copy = await importUpdateModuleCopy<{ runNpmInstall: unknown }>(readFileSync(repoPath("src/update/npm-install.mjs"), "utf8")); + expect(typeof copy.runNpmInstall).toBe("function"); +}); + test.skipIf(process.platform !== "win32").each(["created", "tree"])("abrupt supervisor exit at %s kills installer and retains unproven stage", async point => { const root = mkdtempSync(join(tmpdir(), "ocx-supervisor-exit-")); const packageDir = join(root, "opencodex"); diff --git a/tests/update/update-npm-timeout.test.ts b/tests/update/update-npm-timeout.test.ts index e6292f7bde1..00c60ea1f47 100644 --- a/tests/update/update-npm-timeout.test.ts +++ b/tests/update/update-npm-timeout.test.ts @@ -1,4 +1,3 @@ -import { pathToFileURL } from "node:url"; import { expect, test } from "bun:test"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -8,6 +7,7 @@ import { launcherUsableAfterNpmUpdate, transactionalNpmUpdateAsync, UPDATE_OWNER import { npmInvocation } from "../../src/update/npm-invocation.mjs"; import { runNpmInstall } from "../../src/update/npm-install.mjs"; import { fixturePath, repoPath } from "../helpers/repo-root"; +import { importUpdateModuleCopy } from "../helpers/update-module-data-url"; function fixture() { const root = mkdtempSync(join(tmpdir(), "ocx-npm-timeout-é-업데이트-")); @@ -37,7 +37,7 @@ async function readyTimeoutRunner(root: string, receipt: string): Promise(instrumented)).runNpmInstall; } test("timeout preserves ETIMEDOUT and permits recovery through the untouched launcher", async () => { diff --git a/tests/update/update-windows-installer-witness.test.ts b/tests/update/update-windows-installer-witness.test.ts index ff0aefe33cb..5ffc6333ff5 100644 --- a/tests/update/update-windows-installer-witness.test.ts +++ b/tests/update/update-windows-installer-witness.test.ts @@ -57,14 +57,26 @@ test("win32: a marker without a supervisor witness, or with a POSIX group, stays expect(inspectUpdateMarker(root, { now: afterFence, platform: "win32", signal: signalWith(new Set()) })).toBe("unknown"); }); -test("win32: a confirmed installer never probes its old supervisor pid", () => { - // Windows reuses pids quickly; a finished update must not read as in flight because some - // unrelated process now holds the supervisor's old number. +test("win32: a finished session leaves no supervisor pid for a reused pid to hold", async () => { + // Windows reuses pids quickly. A finished update must not read as in flight because an + // unrelated process now holds the supervisor's old number, so the confirmed exit drops it. const root = home(); - writeMarker(root, { pid: LAUNCHER, phase: "installed", startedAt: 1_000, installerPid: SUPERVISOR }); + const session = createOrchestratedChildSession({ home: root, verifyIdentity: () => true }); + session.start(); await session.enterMutation(); + session.noteUntrackedInstaller(); + publishWindowsInstallerPid(markerPath(root), SUPERVISOR); + session.confirmInstallerExit(); + session.finish({ phase: "installed" }); + expect(readMarker(root).installerPid).toBeUndefined(); expect(inspectUpdateMarker(root, { now: afterFence, platform: "win32", signal: signalWith(new Set([SUPERVISOR])) })).toBe("free"); }); +test("win32: a recorded live installer keeps a marker fenced, as on POSIX", () => { + const root = home(); + writeMarker(root, { pid: 2147483647, phase: "failed", startedAt: 1_000, installerPid: SUPERVISOR }); + expect(inspectUpdateMarker(root, { now: afterFence, platform: "win32", signal: signalWith(new Set([SUPERVISOR])) })).toBe("active"); +}); + test("POSIX keeps requiring the installer process group", () => { const root = home(); writeMarker(root, { ...interrupted, installerPid: SUPERVISOR });