From 39152770165dc1db9d22ab1319592b83f505472c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 16 Oct 2026 07:47:16 +0000 Subject: [PATCH 1/3] fix(setup): use a heartbeat lease for the config lock Replace PID and process-start-time liveness with a worker-thread mtime refresh. A lock is stale five seconds after its last refresh, including locks written by the previous version, and a live holder is not stolen while that lease is renewed. Refs #862 Co-authored-by: tt-a1i --- extensions/shared/setup-config.ts | 473 +++++++++--------- extensions/shared/win32-process-started-at.js | 39 -- tests/extensions/shared/setup-config.test.ts | 463 ++++++++++------- 3 files changed, 516 insertions(+), 459 deletions(-) delete mode 100644 extensions/shared/win32-process-started-at.js diff --git a/extensions/shared/setup-config.ts b/extensions/shared/setup-config.ts index 429416843..3de69a9fd 100644 --- a/extensions/shared/setup-config.ts +++ b/extensions/shared/setup-config.ts @@ -1,4 +1,3 @@ -import { execFile } from "node:child_process"; import { randomUUID } from "node:crypto"; import { existsSync, readFileSync, type Stats, watch } from "node:fs"; import { @@ -12,8 +11,8 @@ import { writeFile, } from "node:fs/promises"; import { basename, join } from "node:path"; -import { fileURLToPath } from "node:url"; import { isDeepStrictEqual } from "node:util"; +import { Worker } from "node:worker_threads"; import { getAgentDir } from "@earendil-works/pi-coding-agent"; import { isWebSearchModelSupport, @@ -338,13 +337,57 @@ export const DEFAULT_SETUP_CONFIG: MyPiSetupConfig = { export const SETUP_CONFIG_PATH = join(getAgentDir(), "my-pi-setup.json"); const SETUP_CONFIG_LOCK_PATH = `${SETUP_CONFIG_PATH}.lock`; +/** + * The holder refreshes the lock file's mtime from a worker thread every second. + * The lease expires five seconds after the last refresh, so one delayed tick + * does not look like a crash. A waiter stops after five seconds when a refresh + * was observed during the wait and another landed inside the last two seconds. + * A waiter that sees no further refresh keeps the lease already on disk, and + * can run one stale period past that patience bound. + */ +const SETUP_CONFIG_LOCK_HEARTBEAT_MS = 1_000; +const SETUP_CONFIG_LOCK_STALE_MS = 5_000; const SETUP_CONFIG_LOCK_TIMEOUT_MS = 5_000; const SETUP_CONFIG_LOCK_RECHECK_MS = 500; -const SETUP_CONFIG_LOCK_VERSION = 1; -const ESTIMATED_PROCESS_STARTED_AT = Math.max( - 1, - Math.round(Date.now() - process.uptime() * 1_000), -); +const SETUP_CONFIG_LOCK_ACTIVE_MS = SETUP_CONFIG_LOCK_HEARTBEAT_MS * 2; +const SETUP_CONFIG_LOCK_VERSION = 2 as const; +/** + * Opens the lock once and refreshes that descriptor. A replacement created at + * the same path keeps its own timestamps. The worker has its own thread, so a + * synchronous critical section on the holder still renews the lease. + */ +const LOCK_HEARTBEAT_WORKER_SOURCE = ` +import { parentPort, workerData } from "node:worker_threads"; +import { closeSync, futimesSync, openSync } from "node:fs"; + +const port = parentPort; +if (!port) throw new Error("setup lock heartbeat has no parent port"); +const fd = openSync(workerData.path, "r+"); +let stopped = false; +const beat = () => { + if (stopped) return; + try { + const now = new Date(); + futimesSync(fd, now, now); + } catch { + // A failed refresh retries on the next tick. Repeated failure lets the + // lease expire instead of touching some other file at this path. + } +}; +const timer = setInterval(beat, workerData.intervalMs); +port.on("message", (message) => { + if (message !== "stop") return; + stopped = true; + clearInterval(timer); + try { + closeSync(fd); + } catch { + // The descriptor is already closed. + } + port.postMessage("stopped"); +}); +port.postMessage("ready"); +`; const isRecord = (value: unknown): value is Record => typeof value === "object" && value !== null && !Array.isArray(value); @@ -1145,40 +1188,59 @@ const isErrno = (error: unknown, code: string) => interface LockIdentity { readonly pid: number; + readonly token: string; +} + +interface LockOwnerV1 extends LockIdentity { + readonly version: 1; readonly processStartedAt: number; readonly processStartedAtVerified: boolean; - readonly token: string; + readonly createdAt: number; } -interface LockOwner extends LockIdentity { - readonly version: typeof SETUP_CONFIG_LOCK_VERSION; +interface LockOwnerV2 extends LockIdentity { + readonly version: 2; readonly createdAt: number; } +type LockOwner = LockOwnerV1 | LockOwnerV2; + type LockOwnerRead = | { kind: "owner"; owner: LockOwner } | { kind: "missing" } | { kind: "unknown" }; -type ProcessLiveness = "live" | "dead" | "unknown"; - const UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; const isPositiveInteger = (value: unknown): value is number => typeof value === "number" && Number.isSafeInteger(value) && value > 0; +const isStaleLock = (mtimeMs: number, now = Date.now()) => + now - mtimeMs >= SETUP_CONFIG_LOCK_STALE_MS; + function parseLockOwner(value: unknown): LockOwner | undefined { if (!isRecord(value)) return undefined; - if (value.version !== SETUP_CONFIG_LOCK_VERSION) return undefined; if (!isPositiveInteger(value.pid)) return undefined; - if (!isPositiveInteger(value.processStartedAt)) return undefined; - if (typeof value.processStartedAtVerified !== "boolean") return undefined; if (!isPositiveInteger(value.createdAt)) return undefined; - if (typeof value.token !== "string" || !UUID_PATTERN.test(value.token)) + if (typeof value.token !== "string" || !UUID_PATTERN.test(value.token)) { return undefined; + } + if (value.version === SETUP_CONFIG_LOCK_VERSION) { + return { + version: SETUP_CONFIG_LOCK_VERSION, + pid: value.pid, + createdAt: value.createdAt, + token: value.token, + }; + } + // Version 1 recorded a process start time and never refreshed its lease. + // Honor it until that original timestamp goes stale, then recover it. + if (value.version !== 1) return undefined; + if (!isPositiveInteger(value.processStartedAt)) return undefined; + if (typeof value.processStartedAtVerified !== "boolean") return undefined; return { - version: SETUP_CONFIG_LOCK_VERSION, + version: 1, pid: value.pid, processStartedAt: value.processStartedAt, processStartedAtVerified: value.processStartedAtVerified, @@ -1187,212 +1249,136 @@ function parseLockOwner(value: unknown): LockOwner | undefined { }; } -const sameOwner = (left: LockOwner, right: LockOwner) => - left.version === right.version && - left.pid === right.pid && - left.processStartedAt === right.processStartedAt && - left.processStartedAtVerified === right.processStartedAtVerified && - left.createdAt === right.createdAt && - left.token === right.token; - -const sameFile = (left: Stats, right: Stats) => - left.dev === right.dev && left.ino === right.ino; - -// CIM datetime is local time plus a UTC offset in minutes. Truncate the -// microsecond field so the integer matches FILETIME / ToUnixTimeMilliseconds. -const CIM_PROCESS_STARTED_AT = - /^(\d{4})(\d{2})(\d{2})(\d{2})(\d{2})(\d{2})\.(\d{6})([+-])(\d{3})$/; - -// Redirected cscript writes UTF-16 LE, often with a BOM. Reading that -// pipe as UTF-8 leaves NULs or U+FFFD and the CIM parser rejects it, so -// a live PID stays unknown until the 5s lock expires. -export function decodeProcessStartedAtOutput(output: Buffer | string) { - const buffer = Buffer.isBuffer(output) ? output : Buffer.from(output); - const utf16 = - (buffer.length >= 2 && buffer[0] === 0xff && buffer[1] === 0xfe) || - (buffer.length >= 4 && buffer[1] === 0 && buffer[3] === 0); - const text = utf16 ? buffer.toString("utf16le") : buffer.toString("utf8"); - return text - .replace(/^\uFEFF/, "") - .replaceAll("\0", "") - .trim(); -} - -const parseCimProcessStartedAt = (stdout: string) => { - const match = CIM_PROCESS_STARTED_AT.exec(stdout.trim()); - if (!match) return undefined; - const year = Number(match[1]); - const month = Number(match[2]); - const day = Number(match[3]); - const hour = Number(match[4]); - const minute = Number(match[5]); - const second = Number(match[6]); - const millisecond = Number(match[7]?.slice(0, 3)); - const offsetMinutes = (match[8] === "+" ? 1 : -1) * Number(match[9]); +const sameOwner = (left: LockOwner, right: LockOwner) => { if ( - month < 1 || - month > 12 || - day < 1 || - day > 31 || - hour > 23 || - minute > 59 || - second > 59 || - !Number.isInteger(offsetMinutes) + left.version !== right.version || + left.pid !== right.pid || + left.createdAt !== right.createdAt || + left.token !== right.token ) { - return undefined; + return false; } - const localAsUtc = Date.UTC( - year, - month - 1, - day, - hour, - minute, - second, - millisecond, - ); - if (!Number.isFinite(localAsUtc)) return undefined; - const parsed = new Date(localAsUtc); - if ( - parsed.getUTCFullYear() !== year || - parsed.getUTCMonth() !== month - 1 || - parsed.getUTCDate() !== day - ) { - return undefined; + if (left.version === 1 && right.version === 1) { + return ( + left.processStartedAt === right.processStartedAt && + left.processStartedAtVerified === right.processStartedAtVerified + ); } - const startedAt = localAsUtc - offsetMinutes * 60_000; - return isPositiveInteger(startedAt) ? startedAt : undefined; + return true; }; -const parsePsProcessStartedAt = (stdout: string) => { - const startedAt = Date.parse(stdout.trim()); - return Number.isFinite(startedAt) ? startedAt : undefined; -}; +const sameFile = (left: Stats, right: Stats) => + left.dev === right.dev && left.ino === right.ino; -export function processStartedAtQuery( - pid: number, - platform = process.platform, -) { - if (platform === "win32") { - // Resolve on use. Vitest loads this module without a file URL, so a - // top-level fileURLToPath throws during import. WMI via cscript avoids - // the PowerShell CLR cold start. The 1s query budget stays; failure is - // still unknown ownership, and the 5s lock does not recover. - const script = fileURLToPath( - new URL("./win32-process-started-at.js", import.meta.url), - ); - return { - command: "cscript.exe", - args: ["//Nologo", script, String(pid)], - env: undefined, - parseOutput: parseCimProcessStartedAt, +function startLockHeartbeat(path: string) { + const worker = new Worker(LOCK_HEARTBEAT_WORKER_SOURCE, { + eval: true, + workerData: { + path, + intervalMs: SETUP_CONFIG_LOCK_HEARTBEAT_MS, + }, + }); + worker.unref(); + let readySettled = false; + const ready = new Promise((resolve, reject) => { + let timer: ReturnType | undefined; + const fail = (error: unknown) => { + if (readySettled) return; + readySettled = true; + if (timer) clearTimeout(timer); + reject(error instanceof Error ? error : new Error(String(error))); }; - } - return { - command: "ps", - args: ["-o", "lstart=", "-p", String(pid)], - env: { LC_ALL: "C" }, - parseOutput: parsePsProcessStartedAt, - }; -} - -// One query. A timeout, non-zero exit, or unparseable value is unknown -// ownership. The 5s lock then expires without a second command. -const PROCESS_START_QUERY_TIMEOUT_MS = 1_000; - -const runProcessStartedAtQuery = ( - query: { - command: string; - args: string[]; - env: Record | undefined; - parseOutput: (stdout: string) => number | undefined; - }, - timeout: number, -) => - new Promise((resolve) => { - try { - execFile( - query.command, - query.args, - { - encoding: "buffer", - env: query.env ? { ...process.env, ...query.env } : process.env, - timeout, - windowsHide: true, - // cscript treats //Nologo as a host switch. Verbatim keeps Node - // from quoting it into an ordinary argument. - windowsVerbatimArguments: process.platform === "win32", - }, - (error, stdout) => { - resolve( - error || stdout == null - ? undefined - : query.parseOutput(decodeProcessStartedAtOutput(stdout)), - ); - }, - ); - } catch { - resolve(undefined); - } + timer = setTimeout(() => { + fail(new Error("Setup config lock heartbeat did not start.")); + }, SETUP_CONFIG_LOCK_STALE_MS); + worker.on("message", (message: unknown) => { + if (message !== "ready") return; + if (readySettled) return; + readySettled = true; + clearTimeout(timer); + resolve(); + }); + worker.once("error", fail); + worker.once("exit", (code) => { + if (!readySettled && code !== 0) { + fail(new Error(`Setup config lock heartbeat exited ${code}.`)); + } + }); }); -const queryProcessStartedAt = (pid: number) => - runProcessStartedAtQuery( - processStartedAtQuery(pid), - PROCESS_START_QUERY_TIMEOUT_MS, - ); - -const makeLockIdentity = async (): Promise => { - const processStartedAt = await queryProcessStartedAt(process.pid); return { - pid: process.pid, - processStartedAt: processStartedAt ?? ESTIMATED_PROCESS_STARTED_AT, - processStartedAtVerified: processStartedAt !== undefined, - token: randomUUID(), + ready, + async stop() { + readySettled = true; + await new Promise((resolve) => { + const timer = setTimeout(resolve, SETUP_CONFIG_LOCK_HEARTBEAT_MS); + const done = () => { + clearTimeout(timer); + resolve(); + }; + worker.on("message", (message: unknown) => { + if (message === "stopped") done(); + }); + worker.once("exit", done); + worker.once("error", done); + try { + worker.postMessage("stop"); + } catch { + done(); + } + }); + await worker.terminate().catch(() => undefined); + }, }; -}; +} + +const makeLockIdentity = (): LockIdentity => ({ + pid: process.pid, + token: randomUUID(), +}); /** * The owner document is fully written before `link()` atomically publishes the * lock path. Its companion hard link remains for the whole critical section. - * A dead-owner recovery atomically renames that companion to its own PID/token, - * so another recovery can take over if the recovering process is also killed. + * Stale-owner recovery atomically renames that companion onto the recoverer's + * own pid and token, so another recovery can take over if this one is killed + * before it deletes the lock. */ -const makeLockOwner = async (): Promise => ({ +const makeLockOwner = (): LockOwnerV2 => ({ version: SETUP_CONFIG_LOCK_VERSION, - ...(await makeLockIdentity()), + ...makeLockIdentity(), createdAt: Date.now(), }); const claimPathFor = (owner: LockOwner) => - `${SETUP_CONFIG_LOCK_PATH}.owner.${owner.pid}.${owner.processStartedAt}.${owner.token}`; + owner.version === 1 + ? `${SETUP_CONFIG_LOCK_PATH}.owner.${owner.pid}.${owner.processStartedAt}.${owner.token}` + : `${SETUP_CONFIG_LOCK_PATH}.owner.${owner.pid}.${owner.token}`; const recoveryPathFor = (owner: LockOwner, recovery: LockIdentity) => - `${claimPathFor(owner)}.recovering.${recovery.pid}.${recovery.processStartedAt}.${recovery.processStartedAtVerified ? 1 : 0}.${recovery.token}`; + `${claimPathFor(owner)}.recovering.${recovery.pid}.${recovery.token}`; const parseRecoveryIdentity = (owner: LockOwner, name: string) => { const prefix = `${basename(claimPathFor(owner))}.recovering.`; if (!name.startsWith(prefix)) return undefined; - const [rawPid, rawStartedAt, rawVerified, token, ...extra] = name - .slice(prefix.length) - .split("."); - const pid = Number(rawPid); - const processStartedAt = Number(rawStartedAt); + const parts = name.slice(prefix.length).split("."); + // Current recoveries are `pid.token`. A version 1 recovery still in progress + // is `pid.processStartedAt.verified.token`. + const token = parts.at(-1); + const pid = Number(parts[0]); + const legacy = parts.length === 4; if ( - extra.length > 0 || + (parts.length !== 2 && !legacy) || !isPositiveInteger(pid) || - !isPositiveInteger(processStartedAt) || - (rawVerified !== "0" && rawVerified !== "1") || !token || - !UUID_PATTERN.test(token) + !UUID_PATTERN.test(token) || + (legacy && + (!isPositiveInteger(Number(parts[1])) || + (parts[2] !== "0" && parts[2] !== "1"))) ) { return undefined; } - return { - pid, - processStartedAt, - processStartedAtVerified: rawVerified === "1", - token, - } satisfies LockIdentity; + return { pid, token } satisfies LockIdentity; }; async function readLockOwner(path: string): Promise { @@ -1404,18 +1390,13 @@ async function readLockOwner(path: string): Promise { } } -async function processLiveness( - identity: LockIdentity, -): Promise { +async function lockMtime(path: string) { try { - process.kill(identity.pid, 0); + return (await stat(path)).mtimeMs; } catch (error) { - return isErrno(error, "ESRCH") ? "dead" : "unknown"; + if (isErrno(error, "ENOENT")) return undefined; + throw error; } - if (!identity.processStartedAtVerified) return "unknown"; - const processStartedAt = await queryProcessStartedAt(identity.pid); - if (processStartedAt === undefined) return "unknown"; - return processStartedAt === identity.processStartedAt ? "live" : "dead"; } const lockTimeoutError = () => @@ -1425,32 +1406,23 @@ const lockTimeoutError = () => function waitForSetupConfigLock(deadline: number) { const remaining = deadline - Date.now(); - if (remaining <= 0) return Promise.reject(lockTimeoutError()); + if (remaining <= 0) return Promise.resolve(); return new Promise((resolve, reject) => { let settled = false; let watcher: ReturnType | undefined; - // A dead owner does not change the lock file, so periodically retry the - // existing fail-closed recovery path before the shared deadline expires. - const recheckTimer = setTimeout( - () => finish(), - Math.min(remaining, SETUP_CONFIG_LOCK_RECHECK_MS), - ); - const timer = setTimeout(() => { - // fs.watch may coalesce or drop events. Recheck the atomic lock path at - // the deadline so a released lock cannot become a false timeout. - if (!existsSync(SETUP_CONFIG_LOCK_PATH)) finish(); - else finish(lockTimeoutError()); - }, remaining); + let timer: ReturnType | undefined; const finish = (error?: unknown) => { if (settled) return; settled = true; - clearTimeout(timer); - clearTimeout(recheckTimer); + if (timer) clearTimeout(timer); watcher?.close(); if (error) reject(error); else resolve(); }; + // A silent owner does not change the lock file. Wake even when fs.watch + // drops the event so a lease can be rechecked on the caller's schedule. + timer = setTimeout(() => finish(), remaining); try { const lockName = basename(SETUP_CONFIG_LOCK_PATH); @@ -1485,7 +1457,7 @@ async function findRecoveryClaim(owner: LockOwner) { async function takeRecoveryClaim(owner: LockOwner, lockStat: Stats) { const ownerClaimPath = claimPathFor(owner); - const recovery = await makeLockIdentity(); + const recovery = makeLockIdentity(); const recoveryPath = recoveryPathFor(owner, recovery); const claimOwner = await readLockOwner(ownerClaimPath); @@ -1503,7 +1475,10 @@ async function takeRecoveryClaim(owner: LockOwner, lockStat: Stats) { const existing = await findRecoveryClaim(owner); if (!existing) return undefined; - if ((await processLiveness(existing.recovery)) !== "dead") return undefined; + const existingMtime = await lockMtime(existing.path); + if (existingMtime === undefined || !isStaleLock(existingMtime)) { + return undefined; + } const existingOwner = await readLockOwner(existing.path); if ( existingOwner.kind !== "owner" || @@ -1533,12 +1508,7 @@ async function restoreOwnerClaim(owner: LockOwner, recoveryPath: string) { async function recoverStaleSetupConfigLock() { const lockRead = await readLockOwner(SETUP_CONFIG_LOCK_PATH); if (lockRead.kind === "missing") return true; - if ( - lockRead.kind !== "owner" || - (await processLiveness(lockRead.owner)) !== "dead" - ) { - return false; - } + if (lockRead.kind !== "owner") return false; let lockStat: Stats; try { @@ -1546,6 +1516,7 @@ async function recoverStaleSetupConfigLock() { } catch (error) { return isErrno(error, "ENOENT"); } + if (!isStaleLock(lockStat.mtimeMs)) return false; const recoveryPath = await takeRecoveryClaim(lockRead.owner, lockStat); if (recoveryPath === null) return true; @@ -1554,13 +1525,14 @@ async function recoverStaleSetupConfigLock() { const currentLock = await readLockOwner(SETUP_CONFIG_LOCK_PATH); const recoveryOwner = await readLockOwner(recoveryPath); try { + const currentStat = await stat(SETUP_CONFIG_LOCK_PATH); if ( currentLock.kind !== "owner" || recoveryOwner.kind !== "owner" || !sameOwner(currentLock.owner, lockRead.owner) || !sameOwner(recoveryOwner.owner, lockRead.owner) || - (await processLiveness(currentLock.owner)) !== "dead" || - !sameFile(await stat(SETUP_CONFIG_LOCK_PATH), await stat(recoveryPath)) + !isStaleLock(currentStat.mtimeMs) || + !sameFile(currentStat, await stat(recoveryPath)) ) { await restoreOwnerClaim(lockRead.owner, recoveryPath); return false; @@ -1599,15 +1571,20 @@ async function releaseSetupConfigLock(owner: LockOwner, claimPath: string) { async function withSetupConfigLock(action: () => Promise) { await mkdir(getAgentDir(), { recursive: true }); - const owner = await makeLockOwner(); + const owner = makeLockOwner(); const claimPath = claimPathFor(owner); await writeFile(claimPath, `${JSON.stringify(owner)}\n`, { encoding: "utf8", flag: "wx", mode: 0o600, }); - const deadline = Date.now() + SETUP_CONFIG_LOCK_TIMEOUT_MS; + const started = Date.now(); + const patienceDeadline = started + SETUP_CONFIG_LOCK_TIMEOUT_MS; + const hardDeadline = patienceDeadline + SETUP_CONFIG_LOCK_STALE_MS; let acquired = false; + let heartbeat: ReturnType | undefined; + let observedMtime: number | undefined; + let lastRenewedAt = 0; try { while (true) { @@ -1618,18 +1595,48 @@ async function withSetupConfigLock(action: () => Promise) { } catch (error) { if (!isErrno(error, "EEXIST")) throw error; if (await recoverStaleSetupConfigLock()) continue; - await waitForSetupConfigLock(deadline); + + const now = Date.now(); + if (now >= hardDeadline) throw lockTimeoutError(); + const lockRead = await readLockOwner(SETUP_CONFIG_LOCK_PATH); + if (lockRead.kind === "missing") continue; + if (lockRead.kind !== "owner") { + if (now >= patienceDeadline) throw lockTimeoutError(); + } else { + const mtimeMs = await lockMtime(SETUP_CONFIG_LOCK_PATH); + if (mtimeMs === undefined) continue; + if ( + observedMtime !== undefined && + mtimeMs > observedMtime + SETUP_CONFIG_LOCK_HEARTBEAT_MS / 2 + ) { + lastRenewedAt = now; + } + observedMtime = mtimeMs; + if (now >= patienceDeadline) { + const holderStillRefreshing = + lastRenewedAt > 0 && + now - lastRenewedAt < SETUP_CONFIG_LOCK_ACTIVE_MS; + // A quiet owner keeps the lease it already published. A holder + // that refreshed during this wait is still working. A stale lock + // we could not prove from its companion stops here too. + if (holderStillRefreshing || isStaleLock(mtimeMs, now)) { + throw lockTimeoutError(); + } + } + } + await waitForSetupConfigLock( + Math.min(hardDeadline, now + SETUP_CONFIG_LOCK_RECHECK_MS), + ); } } - try { - return await action(); - } finally { - await releaseSetupConfigLock(owner, claimPath); - acquired = false; - } + heartbeat = startLockHeartbeat(SETUP_CONFIG_LOCK_PATH); + await heartbeat.ready; + return await action(); } finally { - if (!acquired) await unlink(claimPath).catch(() => undefined); + await heartbeat?.stop(); + if (acquired) await releaseSetupConfigLock(owner, claimPath); + else await unlink(claimPath).catch(() => undefined); } } diff --git a/extensions/shared/win32-process-started-at.js b/extensions/shared/win32-process-started-at.js deleted file mode 100644 index 8f4254362..000000000 --- a/extensions/shared/win32-process-started-at.js +++ /dev/null @@ -1,39 +0,0 @@ -// Windows Script Host JScript. Node does not execute this file. -// setup-config asks cscript.exe for one process CreationDate. A missing -// process or a failed query exits non-zero so the lock stays fail-closed. -// JScript rejects trailing commas, so keep every call list tight. -var raw = - WScript.Arguments.length === 1 ? String(WScript.Arguments.Item(0)) : ""; -if (!/^[1-9][0-9]*$/.test(raw)) { - WScript.StdOut.Write("wmi-args"); - WScript.Quit(1); -} -var startedAt = ""; -var failure = "wmi-error"; -try { - var locator = new ActiveXObject("WbemScripting.SWbemLocator"); - var service = locator.ConnectServer(".", "root\\cimv2"); - var processes = service.ExecQuery( - "SELECT CreationDate FROM Win32_Process WHERE ProcessId=" + raw - ); - var enumerator = new Enumerator(processes); - if (enumerator.atEnd()) { - failure = "wmi-empty"; - } else { - var value = enumerator.item().CreationDate; - if (value) { - startedAt = String(value); - failure = ""; - } - } -} catch (error) { - failure = "wmi-error"; - startedAt = ""; -} -// Quit outside try. JScript turns WScript.Quit into a catchable exception, -// so a failure exit inside try/catch can fall off the end with status 0. -if (!startedAt) { - WScript.StdOut.Write(failure); - WScript.Quit(1); -} -WScript.Echo(startedAt); diff --git a/tests/extensions/shared/setup-config.test.ts b/tests/extensions/shared/setup-config.test.ts index 3d9374812..49e4424a4 100644 --- a/tests/extensions/shared/setup-config.test.ts +++ b/tests/extensions/shared/setup-config.test.ts @@ -1,5 +1,5 @@ import assert from "node:assert/strict"; -import { execFile, spawn } from "node:child_process"; +import { spawn } from "node:child_process"; import { once } from "node:events"; import { existsSync, @@ -8,9 +8,11 @@ import { mkdirSync, readdirSync, readFileSync, + renameSync, rmSync, rmdirSync, statSync, + utimesSync, writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; @@ -32,8 +34,6 @@ const { formatSetupDiagnostics, parseSetupConfig, POST_EDIT_COMMAND_MAX_CHARS, - decodeProcessStartedAtOutput, - processStartedAtQuery, saveSetupConfig, SETUP_CONFIG_PATH, updateSetupConfig, @@ -245,66 +245,15 @@ test("FAIL: session child execution admission is opt-in and bounded", () => { ); }); -test("process start-time queries are platform-specific and conservative", () => { - const windowsQuery = processStartedAtQuery(123, "win32"); - assert.equal(windowsQuery.command, "cscript.exe"); - assert.deepEqual(windowsQuery.args.slice(0, 1), ["//Nologo"]); - assert.match(windowsQuery.args[1] ?? "", /win32-process-started-at\.js$/); - assert.equal(windowsQuery.args[2], "123"); - assert.equal( - windowsQuery.args.some((arg) => arg.toLowerCase().includes("powershell")), - false, - ); - const cim = "20231114221320.000000+000"; - const utf16Pipe = Buffer.from(`\uFEFF${cim}\r\n`, "utf16le"); - assert.equal( - windowsQuery.parseOutput(decodeProcessStartedAtOutput(utf16Pipe)), - 1_700_000_000_000, - ); - assert.equal(windowsQuery.parseOutput(utf16Pipe.toString("utf8")), undefined); - assert.equal( - windowsQuery.parseOutput( - decodeProcessStartedAtOutput(Buffer.from(`${cim}\n`, "utf8")), - ), - 1_700_000_000_000, - ); - assert.equal( - windowsQuery.parseOutput("20231114221320.000000+000"), - 1_700_000_000_000, - ); - assert.equal( - windowsQuery.parseOutput("20231115061320.000000+480"), - 1_700_000_000_000, - ); - assert.equal( - windowsQuery.parseOutput("20231114171320.000000-300"), - 1_700_000_000_000, - ); - assert.equal( - windowsQuery.parseOutput("20231114221320.123999+000\n"), - 1_700_000_000_123, - ); - assert.equal(windowsQuery.parseOutput("1700000000000"), undefined); - assert.equal(windowsQuery.parseOutput("not a timestamp"), undefined); - assert.equal( - windowsQuery.parseOutput("20231314221320.000000+000"), - undefined, - ); - assert.equal( - windowsQuery.parseOutput("19700101000000.000000+000"), - undefined, - ); +const delay = (ms: number) => + new Promise((resolve) => { + setTimeout(resolve, ms); + }); - const posixQuery = processStartedAtQuery(123, "linux"); - assert.equal(posixQuery.command, "ps"); - assert.deepEqual(posixQuery.args, ["-o", "lstart=", "-p", "123"]); - assert.deepEqual(posixQuery.env, { LC_ALL: "C" }); - assert.equal( - posixQuery.parseOutput("Mon Jan 1 00:00:00 2024\n"), - Date.parse("Mon Jan 1 00:00:00 2024"), - ); - assert.equal(posixQuery.parseOutput("not a timestamp"), undefined); -}); +const expireLock = (path: string) => { + const expired = new Date(Date.now() - 6_000); + utimesSync(path, expired, expired); +}; test("every one-line footer preset keeps model context left and cwd right", () => { for (const preset of Object.values(FOOTER_PRESET_DEFINITIONS)) { @@ -526,7 +475,9 @@ await updateSetupConfig((current) => { }); }); -test("a killed setup writer releases ownership for the next process", async () => { +test("a killed setup writer is recovered from its last heartbeat", { + timeout: 15_000, +}, async () => { await saveSetupConfig(DEFAULT_SETUP_CONFIG); const source = ` const { writeFileSync } = await import("node:fs"); @@ -569,6 +520,12 @@ await updateSetupConfig((current) => { ); }); assert.equal(existsSync(setupConfigLockPath), true); + const frozen = statSync(setupConfigLockPath).mtimeMs; + const killedAt = Date.now(); + assert.ok( + killedAt - frozen < 3_000, + `heartbeat was already stale when the holder was killed (${killedAt - frozen}ms)`, + ); const exited = once(holder, "exit"); holder.kill("SIGKILL"); @@ -578,9 +535,18 @@ await updateSetupConfig((current) => { ...current, ui: { ...current.ui, showHeader: true }, })); + const recoveredAt = Date.now(); assert.equal(config.ui.showHeader, true); assert.equal(loadSetupConfig().ui.showHeader, true); assert.equal(existsSync(setupConfigLockPath), false); + assert.ok( + recoveredAt >= frozen + 5_000 - 100, + `recovered ${recoveredAt - frozen}ms after the last heartbeat`, + ); + assert.ok( + recoveredAt - killedAt < 8_000, + `recovery took ${recoveredAt - killedAt}ms`, + ); } finally { if (holder.exitCode === null && holder.signalCode === null) { const exited = once(holder, "exit"); @@ -672,18 +638,27 @@ process.stdout.write(JSON.stringify({ result: "success", ms: Date.now() - starte await holder.waitFor("holding\n"); waiter = start(waiterSource); await waiter.waitFor("watching\n"); + const frozen = statSync(setupConfigLockPath).mtimeMs; + const killedAt = Date.now(); + assert.ok(killedAt - frozen < 3_000); holder.child.kill("SIGKILL"); await holder.exited; await waiter.exited; + const recoveredAt = Date.now(); const { stdout, stderr } = waiter.output(); assert.equal(stderr, ""); const result = JSON.parse(stdout.split("\n").at(-2) ?? ""); assert.equal(result.result, "success"); assert.ok( - result.ms < 5_000, - "waiter used the full deadline: " + result.ms + "ms", + recoveredAt >= frozen + 5_000 - 100, + `waiter recovered ${recoveredAt - frozen}ms after the last heartbeat`, + ); + assert.ok( + result.ms < 12_000, + "waiter exceeded the lease cap: " + result.ms, ); + assert.equal(existsSync(setupConfigLockPath), false); } finally { for (const process of [holder, waiter]) { if (!process) continue; @@ -697,43 +672,93 @@ process.stdout.write(JSON.stringify({ result: "success", ms: Date.now() - starte } }); -test("Windows reads this process start time from WMI alone", { - skip: process.platform !== "win32", -}, async () => { - const query = processStartedAtQuery(process.pid); - assert.equal(query.command, "cscript.exe"); - const { stdout, stderr, error } = await new Promise<{ - stdout: Buffer; - stderr: Buffer; - error: Error | null; - }>((resolve) => { - execFile( - query.command, - query.args, - { - encoding: "buffer", - timeout: 1_000, - windowsHide: true, - windowsVerbatimArguments: true, - }, - (execError, execStdout, execStderr) => { - resolve({ - stdout: Buffer.isBuffer(execStdout) ? execStdout : Buffer.alloc(0), - stderr: Buffer.isBuffer(execStderr) ? execStderr : Buffer.alloc(0), - error: execError, - }); - }, +test("a stale setup lock is recovered even when its recorded pid is alive", async () => { + removeLockArtifacts(); + await saveSetupConfig(DEFAULT_SETUP_CONFIG); + const owner = { + version: 2, + pid: process.pid, + createdAt: Date.now(), + token: "33333333-3333-4333-8333-333333333333", + }; + const metadata = `${JSON.stringify(owner)}\n`; + const claimPath = `${setupConfigLockPath}.owner.${owner.pid}.${owner.token}`; + writeFileSync(claimPath, metadata); + linkSync(claimPath, setupConfigLockPath); + expireLock(setupConfigLockPath); + + try { + const started = Date.now(); + const { config } = await updateSetupConfig((current) => ({ + ...current, + ui: { ...current.ui, showHeader: true }, + })); + assert.equal(config.ui.showHeader, true); + assert.equal(loadSetupConfig().ui.showHeader, true); + assert.ok( + Date.now() - started < 2_000, + "stale recovery waited out a live pid", ); + assert.equal(existsSync(setupConfigLockPath), false); + } finally { + removeLockArtifacts(); + } +}); + +test("a fresh setup lock is kept until its timestamp is past the deadline", async () => { + removeLockArtifacts(); + await saveSetupConfig(DEFAULT_SETUP_CONFIG); + const dead = spawn(process.execPath, ["--eval", ""]); + const deadPid = dead.pid; + await once(dead, "exit"); + assert.equal(typeof deadPid, "number"); + const owner = { + version: 2, + pid: deadPid, + createdAt: Date.now(), + token: "55555555-5555-4555-8555-555555555555", + }; + const metadata = `${JSON.stringify(owner)}\n`; + const claimPath = `${setupConfigLockPath}.owner.${owner.pid}.${owner.token}`; + writeFileSync(claimPath, metadata); + linkSync(claimPath, setupConfigLockPath); + + let settled = false; + const pending = updateSetupConfig((current) => ({ + ...current, + ui: { ...current.ui, showHeader: true }, + })).then((result) => { + settled = true; + return result; }); - const text = decodeProcessStartedAtOutput(stdout); - const startedAt = error ? undefined : query.parseOutput(text); - assert.ok( - startedAt !== undefined && startedAt > 1, - `WMI start time missing: ${error?.message ?? "unparseable"}; decoded=${JSON.stringify(text)}; stderr=${JSON.stringify(decodeProcessStartedAtOutput(stderr))}`, - ); + pending.catch(() => undefined); + + try { + await delay(1_000); + assert.equal(settled, false); + assert.equal(readFileSync(setupConfigLockPath, "utf8"), metadata); + expireLock(setupConfigLockPath); + let timeout: ReturnType | undefined; + const result = await Promise.race([ + pending, + new Promise((_, reject) => { + timeout = setTimeout( + () => reject(new Error("stale lock was not recovered")), + 3_000, + ); + }), + ]); + if (timeout) clearTimeout(timeout); + assert.equal(settled, true); + assert.equal(result.config.ui.showHeader, true); + assert.equal(existsSync(setupConfigLockPath), false); + } finally { + removeLockArtifacts(); + await pending.catch(() => undefined); + } }); -test("a reused PID does not impersonate the dead lock owner", async () => { +test("a legacy setup lock is protected while fresh and recovered once stale", async () => { removeLockArtifacts(); await saveSetupConfig(DEFAULT_SETUP_CONFIG); const owner = { @@ -742,134 +767,198 @@ test("a reused PID does not impersonate the dead lock owner", async () => { processStartedAt: 1, processStartedAtVerified: true, createdAt: Date.now(), - token: "33333333-3333-4333-8333-333333333333", + token: "66666666-6666-4666-8666-666666666666", + }; + const metadata = `${JSON.stringify(owner)}\n`; + const claimPath = `${setupConfigLockPath}.owner.${owner.pid}.${owner.processStartedAt}.${owner.token}`; + writeFileSync(claimPath, metadata); + linkSync(claimPath, setupConfigLockPath); + + let settled = false; + const pending = updateSetupConfig((current) => ({ + ...current, + ui: { ...current.ui, showHeader: true }, + })).then((result) => { + settled = true; + return result; + }); + pending.catch(() => undefined); + + try { + await delay(1_000); + assert.equal(settled, false); + assert.equal(readFileSync(setupConfigLockPath, "utf8"), metadata); + expireLock(setupConfigLockPath); + let timeout: ReturnType | undefined; + const result = await Promise.race([ + pending, + new Promise((_, reject) => { + timeout = setTimeout( + () => reject(new Error("stale legacy lock was not recovered")), + 3_000, + ); + }), + ]); + if (timeout) clearTimeout(timeout); + assert.equal(settled, true); + assert.equal(result.config.ui.showHeader, true); + } finally { + removeLockArtifacts(); + await pending.catch(() => undefined); + } +}); + +test("a stale legacy recovery claim is taken over even if its pid is alive", async () => { + removeLockArtifacts(); + await saveSetupConfig(DEFAULT_SETUP_CONFIG); + const owner = { + version: 1, + pid: process.pid, + processStartedAt: 1, + processStartedAtVerified: true, + createdAt: Date.now(), + token: "77777777-7777-4777-8777-777777777777", }; + const metadata = `${JSON.stringify(owner)}\n`; const claimPath = `${setupConfigLockPath}.owner.${owner.pid}.${owner.processStartedAt}.${owner.token}`; - writeFileSync(claimPath, `${JSON.stringify(owner)}\n`); + const recoveryPath = `${claimPath}.recovering.${process.pid}.1.1.88888888-8888-4888-8888-888888888888`; + writeFileSync(claimPath, metadata); linkSync(claimPath, setupConfigLockPath); + renameSync(claimPath, recoveryPath); + expireLock(setupConfigLockPath); try { + const started = Date.now(); const { config } = await updateSetupConfig((current) => ({ ...current, ui: { ...current.ui, showHeader: true }, })); assert.equal(config.ui.showHeader, true); + assert.ok(Date.now() - started < 2_000); + assert.equal(existsSync(setupConfigLockPath), false); + assert.equal(existsSync(recoveryPath), false); } finally { removeLockArtifacts(); } }); -test("process start-time query failures do not authorize lock recovery", async () => { +test("a live holder keeps the setup lock across a blocked main thread", { + timeout: 20_000, +}, async () => { + removeLockArtifacts(); + await saveSetupConfig(DEFAULT_SETUP_CONFIG); const source = ` -const childProcess = await import("node:child_process"); -const { syncBuiltinESMExports } = await import("node:module"); -const commands = []; -childProcess.default.execFile = (...args) => { - commands.push(String(args[0])); - const callback = args.at(-1); - if (typeof callback !== "function") { - throw new Error("expected execFile callback"); - } - queueMicrotask(() => callback(new Error("mock process start query failure"), "")); -}; -syncBuiltinESMExports(); - -const { existsSync, linkSync, readFileSync, rmSync, writeFileSync } = await import("node:fs"); -const { - DEFAULT_SETUP_CONFIG, - SETUP_CONFIG_PATH, - saveSetupConfig, - updateSetupConfig, -} = await import(process.env.SETUP_CONFIG_MODULE_URL); - -await saveSetupConfig(DEFAULT_SETUP_CONFIG); -const lockPath = SETUP_CONFIG_PATH + ".lock"; -const owner = { - version: 1, - pid: process.pid, - processStartedAt: 1, - processStartedAtVerified: true, - createdAt: Date.now(), - token: "44444444-4444-4444-8444-444444444444", -}; -const metadata = JSON.stringify(owner) + "\\n"; -const claimPath = lockPath + ".owner." + owner.pid + "." + owner.processStartedAt + "." + owner.token; -writeFileSync(claimPath, metadata); -linkSync(claimPath, lockPath); - -let mutated = false; -try { - await updateSetupConfig((current) => { - mutated = true; - return { ...current, ui: { ...current.ui, showHeader: true } }; - }); - throw new Error("lock was recovered after an unknown process start-time query"); -} catch (error) { - if (!(error instanceof Error) || !error.message.includes("Timed out waiting")) throw error; -} -if (mutated) throw new Error("mutator ran under unknown lock ownership"); -if (!existsSync(lockPath) || readFileSync(lockPath, "utf8") !== metadata) { - throw new Error("unknown lock ownership was modified"); -} -if (commands.some((command) => command.toLowerCase().includes("powershell"))) { - throw new Error("failed start-time read fell through to PowerShell: " + commands.join(",")); -} -if (process.platform === "win32" && !commands.includes("cscript.exe")) { - throw new Error("Windows lock recovery did not query WMI: " + commands.join(",")); -} -if (process.platform === "win32" && commands.some((command) => command !== "cscript.exe")) { - throw new Error("Windows lock recovery used another start-time command: " + commands.join(",")); -} -rmSync(process.env.PI_CODING_AGENT_DIR, { recursive: true, force: true }); -process.stdout.write("preserved\\n"); +const { writeFileSync } = await import("node:fs"); +const { updateSetupConfig } = await import(process.env.SETUP_CONFIG_MODULE_URL); +await updateSetupConfig((current) => { + writeFileSync(1, "holding\\n"); + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 15000); + return { ...current, ui: { ...current.ui, showHeader: true } }; +}); +writeFileSync(1, "released\\n"); `; - - await runSetupConfigChildScenario({ - source, - tempPrefix: "my-pi-setup-query-failure-lock-", - failureLabel: "query failure owner check", + const holder = spawn( + process.execPath, + ["--experimental-strip-types", "--input-type=module", "--eval", source], + { + env: { + ...process.env, + PI_CODING_AGENT_DIR: agentDir, + SETUP_CONFIG_MODULE_URL: setupConfigModuleUrl, + }, + stdio: ["ignore", "pipe", "pipe"], + }, + ); + holder.stdout.setEncoding("utf8"); + holder.stderr.setEncoding("utf8"); + let stderr = ""; + holder.stderr.on("data", (chunk: string) => { + stderr += chunk; }); + const exited = once(holder, "exit"); + void exited.catch(() => {}); + + try { + await new Promise((resolve, reject) => { + let stdout = ""; + holder.stdout.on("data", (chunk: string) => { + stdout += chunk; + if (stdout.includes("holding\n")) resolve(); + }); + holder.once("error", reject); + holder.once("exit", (code) => + reject(new Error(`lock holder exited ${code}: ${stderr}`)), + ); + }); + const before = statSync(setupConfigLockPath).mtimeMs; + const held = readFileSync(setupConfigLockPath, "utf8"); + await delay(2_200); + const after = statSync(setupConfigLockPath).mtimeMs; + assert.ok( + after >= before + 500, + `lock timestamp did not refresh during a blocked hold (${after - before}ms)`, + ); + + const waiterStarted = Date.now(); + await assert.rejects( + updateSetupConfig((current) => ({ + ...current, + ui: { ...current.ui, customFooter: false }, + })), + /Timed out waiting/, + ); + const waiterMs = Date.now() - waiterStarted; + assert.ok(waiterMs < 9_000, `live lock wait took ${waiterMs}ms`); + assert.equal(readFileSync(setupConfigLockPath, "utf8"), held); + assert.equal(loadSetupConfig().ui.customFooter, true); + assert.equal(holder.exitCode, null); + } finally { + if (holder.exitCode === null && holder.signalCode === null) { + holder.kill("SIGKILL"); + await exited; + } + removeLockArtifacts(); + } }); -test("live and uncertain lock owners are never stolen", async () => { +test("unreadable, mismatched, and newer setup locks are never stolen", async () => { const source = ` -const { spawn } = await import("node:child_process"); -const { once } = await import("node:events"); -const { existsSync, linkSync, readFileSync, rmSync, writeFileSync } = await import("node:fs"); +const { existsSync, linkSync, readFileSync, rmSync, utimesSync, writeFileSync } = await import("node:fs"); const { join } = await import("node:path"); const scenario = process.env.SETUP_LOCK_SCENARIO; const agentDir = process.env.PI_CODING_AGENT_DIR; const lockPath = join(agentDir, "my-pi-setup.json.lock"); -if (scenario === "live") { +const expire = () => { + const expired = new Date(Date.now() - 10_000); + utimesSync(lockPath, expired, expired); +}; +if (scenario === "future") { const owner = { - version: 1, + version: 99, pid: process.pid, - processStartedAt: Math.max(1, Math.round(Date.now() - process.uptime() * 1000)), - processStartedAtVerified: false, createdAt: Date.now(), - token: "11111111-1111-4111-8111-111111111111", + token: "99999999-9999-4999-8999-999999999999", }; - const claimPath = lockPath + ".owner." + owner.pid + "." + owner.processStartedAt + "." + owner.token; - writeFileSync(claimPath, JSON.stringify(owner) + "\\n"); + const metadata = JSON.stringify(owner) + "\\n"; + const claimPath = lockPath + ".owner." + owner.pid + "." + owner.token; + writeFileSync(claimPath, metadata); linkSync(claimPath, lockPath); + expire(); } else if (scenario === "mismatched") { - const dead = spawn(process.execPath, ["--eval", ""]); - const deadPid = dead.pid; - await once(dead, "exit"); const owner = { - version: 1, - pid: deadPid, - processStartedAt: 1, - processStartedAtVerified: false, + version: 2, + pid: process.pid, createdAt: Date.now(), token: "22222222-2222-4222-8222-222222222222", }; - const claimPath = lockPath + ".owner." + owner.pid + "." + owner.processStartedAt + "." + owner.token; const metadata = JSON.stringify(owner) + "\\n"; + const claimPath = lockPath + ".owner." + owner.pid + "." + owner.token; writeFileSync(lockPath, metadata); writeFileSync(claimPath, metadata); + expire(); } else { writeFileSync(lockPath, "not trustworthy\\n"); + expire(); } const before = readFileSync(lockPath, "utf8"); const { updateSetupConfig } = await import(process.env.SETUP_CONFIG_MODULE_URL); @@ -891,7 +980,7 @@ rmSync(agentDir, { recursive: true, force: true }); process.stdout.write("preserved\\n"); `; - const runs = ["live", "unknown", "mismatched"].map((scenario) => { + const runs = ["unknown", "mismatched", "future"].map((scenario) => { return runSetupConfigChildScenario({ source, tempPrefix: `my-pi-setup-${scenario}-lock-`, From 2f6bd2847b8136669a530ca5521775e5eb906188 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 16 Oct 2026 08:15:36 +0000 Subject: [PATCH 2/3] fix(test): observe workbar readiness after React commits Windows CI can stall long enough that Testing Library's wait timer and the deferred file or review read expire in the same timer batch. The timeout then rejects before React commits, so the README button is reported missing and the review restore case burns the 5s test budget. Poll on a check-phase flush inside the existing 1s budget. Co-authored-by: tt-a1i --- tests/web/workbar-session-position.spec.ts | 80 ++++++++++++++++++---- 1 file changed, 65 insertions(+), 15 deletions(-) diff --git a/tests/web/workbar-session-position.spec.ts b/tests/web/workbar-session-position.spec.ts index 77e7114fe..f63c3b83e 100644 --- a/tests/web/workbar-session-position.spec.ts +++ b/tests/web/workbar-session-position.spec.ts @@ -18,12 +18,12 @@ import type { } from "../../web/protocol/types.ts"; import { App } from "../../web/ui/src/app/App.tsx"; import { Providers } from "../../web/ui/src/app/providers.tsx"; -import { i18n } from "../../web/ui/src/i18n.ts"; import { loadWorkbarPositions, saveWorkbarPositions, WORKBAR_POSITION_STORAGE_KEY, } from "../../web/ui/src/features/workbar/workbar-position-storage.ts"; +import { i18n } from "../../web/ui/src/i18n.ts"; import { WebClient } from "../../web/ui/src/protocol/client.ts"; import { createWebStore, webStore } from "../../web/ui/src/store/web-store.ts"; import { installCheckVisibilityFixture } from "./check-visibility-fixture.ts"; @@ -178,6 +178,56 @@ async function open(tool: "files" | "browser" | "review") { ); } +// File listings and git review both publish through a timer, then React commits +// on a later task. waitFor/findByRole arm their deadline on that same timer +// phase. After a Windows runner stalls past the deadline, the timeout callback +// is already queued behind the read and rejects before the commit. A check-phase +// turn plus act() sees a result that landed in that batch. Absence still fails +// at the same 1s budget; this does not extend it. +async function paintFrame() { + await new Promise((resolve) => { + setTimeout(resolve, 20); + }); + await act(async () => { + await new Promise((resolve) => { + setImmediate(resolve); + }); + }); +} + +async function paintedRole( + container: { + getByRole: typeof screen.getByRole; + queryByRole: typeof screen.queryByRole; + }, + role: Parameters[0], + name?: string | RegExp, +) { + const options = name === undefined ? undefined : { name }; + const deadline = Date.now() + 1_000; + for (;;) { + await paintFrame(); + const found = container.queryByRole(role, options); + if (found) return found; + if (Date.now() >= deadline) return container.getByRole(role, options); + } +} + +async function painted(assert: () => void) { + const deadline = Date.now() + 1_000; + let failure: unknown; + for (;;) { + await paintFrame(); + try { + assert(); + return; + } catch (error) { + failure = error; + if (Date.now() >= deadline) throw failure; + } + } +} + it("dismisses only the focused Files tooltip before its preview and Workbar, leaving outside Settings Escape alone", async () => { const current = snapshot("/workspace/tooltip-escape.jsonl", "tooltip-escape"); webStore.setState({ @@ -203,8 +253,8 @@ it("dismisses only the focused Files tooltip before its preview and Workbar, lea ), ); await open("files"); - fireEvent.click(await screen.findByRole("button", { name: "README.md" })); - await screen.findByRole("heading", { name: "Read me" }); + fireEvent.click(await paintedRole(screen, "button", "README.md")); + await paintedRole(screen, "heading", "Read me"); const workbar = view.container.querySelector(".workbar-panel")!; const preview = view.container.querySelector(".artifact-panel")!; const copy = within(preview).getByRole("button", { @@ -217,7 +267,7 @@ it("dismisses only the focused Files tooltip before its preview and Workbar, lea const tooltip = (await screen.findByText(i18n.t("copyFilePath"))).closest( '[role="tooltip"]', )!; - await waitFor(() => expect(tooltip.matches(":popover-open")).toBe(true)); + await painted(() => expect(tooltip.matches(":popover-open")).toBe(true)); expect(fireEvent.keyDown(copy, { key: "Escape" })).toBe(false); expect(tooltip.isConnected && tooltip.matches(":popover-open")).toBe(false); expect(view.container.querySelector(".artifact-panel")).toBe(preview); @@ -289,7 +339,7 @@ it("restores a comparison source, filter and reading position after reloading th fireEvent.change(scope().getByRole("combobox"), { target: { value: "staged" }, }); - await waitFor(() => + await painted(() => expect( vi.mocked(WebClient.prototype.gitReview).mock.calls.at(-1)?.[3]?.source, ).toBe("staged"), @@ -297,8 +347,8 @@ it("restores a comparison source, filter and reading position after reloading th fireEvent.change(scope().getByRole("searchbox"), { target: { value: "example" }, }); - fireEvent.click(await scope().findByRole("button", { name: /example\.ts/u })); - await scope().findByRole("figure"); + fireEvent.click(await paintedRole(scope(), "button", /example\.ts/u)); + await paintedRole(scope(), "figure"); const preview = view.container.querySelector( ".review-file-preview", )!; @@ -309,17 +359,17 @@ it("restores a comparison source, filter and reading position after reloading th fireEvent.change(scope().getByRole("combobox"), { target: { value: "branch" }, }); - await waitFor(() => + await painted(() => expect( vi.mocked(WebClient.prototype.gitReview).mock.calls.at(-1)?.[3]?.source, ).toBe("branch"), ); restoring = true; await select("/workspace/a.jsonl"); - await waitFor(() => expect(resolveRestoredFile).toBeTypeOf("function")); + await painted(() => expect(resolveRestoredFile).toBeTypeOf("function")); expect(scope().queryByRole("figure")).toBeNull(); await act(async () => resolveRestoredFile(file)); - await scope().findByRole("figure"); + await paintedRole(scope(), "figure"); expect(scope().getByRole("combobox").value).toBe("staged"); expect( view.container.querySelector(".review-file-preview")! @@ -370,7 +420,7 @@ it("quotes an exact deleted line through the workbar into the existing chat draf view.container.querySelector(".composer textarea")!; fireEvent.change(composer, { target: { value: "Please reconsider this." } }); await open("review"); - fireEvent.click(await screen.findByRole("button", { name: /example\.ts/u })); + fireEvent.click(await paintedRole(screen, "button", /example\.ts/u)); fireEvent.click( screen.getByRole("button", { name: i18n.t("reviewSelectOldLine", { line: 10 }), @@ -577,8 +627,8 @@ it("bounds remembered tools to 32 exact Session identities while keeping a recen it("reattaches a Session's file position after a copied same-ID Session, releasing the old preview", async () => { const view = render(createElement(Providers, null, createElement(App))); await open("files"); - fireEvent.click(await screen.findByRole("button", { name: "README.md" })); - await screen.findByRole("heading", { name: "Read me" }); + fireEvent.click(await paintedRole(screen, "button", "README.md")); + await paintedRole(screen, "heading", "Read me"); const tree = view.container.querySelector(".file-tree")!; const preview = view.container.querySelector( ".artifact-panel-body", @@ -604,14 +654,14 @@ it("reattaches a Session's file position after a copied same-ID Session, releasi }).value, ).toBe(""); await select("/workspace/a.jsonl"); - await screen.findByRole("heading", { name: "Read me" }); + await paintedRole(screen, "heading", "Read me"); expect(view.container.querySelectorAll(".workbar-panel")).toHaveLength(1); expect( screen.getByRole("textbox", { name: i18n.t("filesSearch"), }).value, ).toBe("README"); - await waitFor(() => + await painted(() => expect( view.container.querySelector(".file-tree")!.scrollTop, ).toBe(84), From 947e36e4aa65b1a27d84716d69a85b30936173cb Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 16 Oct 2026 08:31:07 +0000 Subject: [PATCH 3/3] fix(test): drain snapshot routes before the request context closes route.fetch() bodies live on the per-test request context. Closing that context drops them, so a refresh still between fetch() and json() fails a finished test with "Response has been disposed". Co-authored-by: tt-a1i --- tests/web/openpi-web.e2e.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/web/openpi-web.e2e.ts b/tests/web/openpi-web.e2e.ts index 3f06f5f77..933290a2b 100644 --- a/tests/web/openpi-web.e2e.ts +++ b/tests/web/openpi-web.e2e.ts @@ -26,6 +26,17 @@ if (!token) throw new Error("OPENPI_WEB_E2E_TOKEN is required"); const authenticatedPath = "/"; +// route.fetch() keeps the upstream body on the per-test request context. +// Closing that context ("Test ended") drops the body, so a snapshot refresh +// still between fetch() and json() rejects a finished test with +// "Response has been disposed". Drain handlers before fixture teardown. +// A handler that fails for any other reason still fails the test. Tests that +// already closed the fixture page have finished their own handlers. +test.afterEach(async ({ page }) => { + if (page.isClosed()) return; + await page.unrouteAll({ behavior: "wait" }); +}); + for (const firstControl of ["settings", "thinking"] as const) { test(`first-use ${firstControl} works before sending a message and preserves the draft`, async ({ page,