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-`, 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, 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),