fix(browser): give the DevToolsActivePort wait one deadline owner (#2265)
* Single owner for the Chrome DevToolsActivePort startup deadline Cold Chrome startup tolerance was spelled three ways for one wait: the helper fell back to 15,000 ms, the runtime fixture lifecycle passed a literal 60,000, and the real-browser verifier kept a local 60,000 constant. A per-caller literal quietly changes how long a lane tolerates a cold browser before it fails, and the failure reported only elapsed_ms, so a reader could not see which ceiling was hit. Export BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS as the owner, use it for the helper fallback and both call sites, and report startup_timeout_ms in BrowserStartupError. A source guard keeps callers from restating the number. * Pin the DevToolsActivePort deadline guard to the startup call argument --------- Co-authored-by: morluto <76467478+morluto@users.noreply.github.com>
This commit is contained in:
The one -Niu
GitHub
parent
1e38a038c6
commit
edd9939879
|
|
@ -8,7 +8,10 @@ import { spawnOwnedProviderProcess } from "../../dist/process/ProviderProcess.js
|
|||
import { createSystemProcessOwnershipHost } from "../../dist/process/ProcessOwnershipObservation.js";
|
||||
import { liveProcesses } from "../../dist/process/ProcessOwnershipProcessTree.js";
|
||||
import { CdpConnection } from "../../dist/browser/CdpConnection.js";
|
||||
import { waitForBrowserDevtoolsPort } from "../../dist/browser/BrowserProcessStartup.js";
|
||||
import {
|
||||
BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS,
|
||||
waitForBrowserDevtoolsPort,
|
||||
} from "../../dist/browser/BrowserProcessStartup.js";
|
||||
|
||||
/** Launch the synthetic fixture browser and await CDP in its own verified process group. */
|
||||
export async function startRuntimeFixtureBrowser(executable, origin) {
|
||||
|
|
@ -50,7 +53,7 @@ export async function startRuntimeFixtureBrowser(executable, origin) {
|
|||
stderr: () => stderr,
|
||||
// Match the broader real-browser verifier's allowance for cold Chrome
|
||||
// startup on shared runners; early process failures still reject at once.
|
||||
timeoutMs: 60_000,
|
||||
timeoutMs: BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS,
|
||||
});
|
||||
return {
|
||||
port,
|
||||
|
|
|
|||
|
|
@ -9,7 +9,10 @@ import { join } from "node:path";
|
|||
import { fileURLToPath } from "node:url";
|
||||
|
||||
import { CdpBrowserProvider } from "../dist/browser/CdpBrowserProvider.js";
|
||||
import { waitForBrowserDevtoolsPort } from "../dist/browser/BrowserProcessStartup.js";
|
||||
import {
|
||||
BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS,
|
||||
waitForBrowserDevtoolsPort,
|
||||
} from "../dist/browser/BrowserProcessStartup.js";
|
||||
import { createBrowserScenarioProvider } from "../dist/composition/browserScenario.js";
|
||||
import {
|
||||
inspectWebPageInputSchema,
|
||||
|
|
@ -47,7 +50,6 @@ import { artifactCliEvidence, artifactMcpResult } from "./lib/artifact-e2e.mjs";
|
|||
import { verifyScenarioEnvironment } from "./lib/browser-scenario-environment-e2e.mjs";
|
||||
import { verifyScenarioStorage } from "./lib/browser-scenario-storage-e2e.mjs";
|
||||
|
||||
const REAL_BROWSER_STARTUP_TIMEOUT_MS = 60_000;
|
||||
const SCENARIO_SECRET_VALUE = "rea-browser-verifier-secret";
|
||||
const SCENARIO_URL_SECRET_VALUE = "rea-browser-url-verifier-secret";
|
||||
process.env.REA_BROWSER_VERIFIER_SECRET = SCENARIO_SECRET_VALUE;
|
||||
|
|
@ -92,7 +94,7 @@ try {
|
|||
executable,
|
||||
activePortPath: join(profile, "DevToolsActivePort"),
|
||||
stderr: () => stderr,
|
||||
timeoutMs: REAL_BROWSER_STARTUP_TIMEOUT_MS,
|
||||
timeoutMs: BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS,
|
||||
});
|
||||
const endpoint = `http://127.0.0.1:${String(port)}`;
|
||||
const provider = new CdpBrowserProvider();
|
||||
|
|
|
|||
|
|
@ -14,6 +14,7 @@ export class BrowserStartupError extends Error {
|
|||
readonly failure: BrowserStartupFailure;
|
||||
readonly executable: string;
|
||||
readonly elapsedMs: number;
|
||||
readonly startupTimeoutMs: number;
|
||||
readonly exitCode: number | null;
|
||||
readonly signalCode: NodeJS.Signals | null;
|
||||
readonly stderr: string;
|
||||
|
|
@ -23,6 +24,7 @@ export class BrowserStartupError extends Error {
|
|||
readonly failure: BrowserStartupFailure;
|
||||
readonly executable: string;
|
||||
readonly elapsedMs: number;
|
||||
readonly startupTimeoutMs: number;
|
||||
readonly exitCode: number | null;
|
||||
readonly signalCode: NodeJS.Signals | null;
|
||||
readonly stderr: string;
|
||||
|
|
@ -33,12 +35,22 @@ export class BrowserStartupError extends Error {
|
|||
this.failure = diagnostics.failure;
|
||||
this.executable = diagnostics.executable;
|
||||
this.elapsedMs = diagnostics.elapsedMs;
|
||||
this.startupTimeoutMs = diagnostics.startupTimeoutMs;
|
||||
this.exitCode = diagnostics.exitCode;
|
||||
this.signalCode = diagnostics.signalCode;
|
||||
this.stderr = diagnostics.stderr;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Single owner of the wait for Chrome to publish its DevToolsActivePort.
|
||||
*
|
||||
* Callers that need a different budget pass `timeoutMs`; nobody else should
|
||||
* restate this number, because a per-caller literal silently changes how long
|
||||
* a cold Chrome launch is tolerated before the failure names its ceiling.
|
||||
*/
|
||||
export const BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS = 60_000;
|
||||
|
||||
/** Owned child process and startup policy for Chrome CDP discovery. */
|
||||
export interface BrowserStartupOptions {
|
||||
readonly child: ChildProcess;
|
||||
|
|
@ -53,23 +65,23 @@ export interface BrowserStartupOptions {
|
|||
export const waitForBrowserDevtoolsPort = async (
|
||||
options: BrowserStartupOptions,
|
||||
): Promise<number> => {
|
||||
const timeoutMs = options.timeoutMs ?? 15_000;
|
||||
const timeoutMs = options.timeoutMs ?? BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS;
|
||||
const pollIntervalMs = options.pollIntervalMs ?? 25;
|
||||
const startedAt = performance.now();
|
||||
const terminal = childTermination(options, startedAt);
|
||||
const terminal = childTermination(options, startedAt, timeoutMs);
|
||||
const deadline = startedAt + timeoutMs;
|
||||
|
||||
while (true) {
|
||||
const terminalState = currentTerminalState(options.child);
|
||||
if (terminalState !== null)
|
||||
throw startupError(options, startedAt, terminalState);
|
||||
throw startupError(options, startedAt, timeoutMs, terminalState);
|
||||
|
||||
const port = await readDevtoolsPort(options.activePortPath);
|
||||
if (port !== null) return port;
|
||||
|
||||
const remainingMs = deadline - performance.now();
|
||||
if (remainingMs <= 0)
|
||||
throw startupError(options, startedAt, { failure: "timeout" });
|
||||
throw startupError(options, startedAt, timeoutMs, { failure: "timeout" });
|
||||
|
||||
const outcome = await Promise.race([
|
||||
terminal,
|
||||
|
|
@ -88,11 +100,12 @@ type TerminalState =
|
|||
const childTermination = (
|
||||
options: BrowserStartupOptions,
|
||||
startedAt: number,
|
||||
startupTimeoutMs: number,
|
||||
): Promise<BrowserStartupError> =>
|
||||
new Promise((resolve) => {
|
||||
options.child.once("error", (cause: unknown) => {
|
||||
resolve(
|
||||
startupError(options, startedAt, {
|
||||
startupError(options, startedAt, startupTimeoutMs, {
|
||||
failure: "spawn-error",
|
||||
cause,
|
||||
}),
|
||||
|
|
@ -103,6 +116,7 @@ const childTermination = (
|
|||
startupError(
|
||||
options,
|
||||
startedAt,
|
||||
startupTimeoutMs,
|
||||
options.child.signalCode === null
|
||||
? { failure: "exited" }
|
||||
: { failure: "signalled" },
|
||||
|
|
@ -120,6 +134,7 @@ const currentTerminalState = (child: ChildProcess): TerminalState | null => {
|
|||
const startupError = (
|
||||
options: BrowserStartupOptions,
|
||||
startedAt: number,
|
||||
startupTimeoutMs: number,
|
||||
state: TerminalState,
|
||||
): BrowserStartupError =>
|
||||
new BrowserStartupError(
|
||||
|
|
@ -127,6 +142,7 @@ const startupError = (
|
|||
failure: state.failure,
|
||||
executable: options.executable,
|
||||
elapsedMs: Math.max(0, Math.round(performance.now() - startedAt)),
|
||||
startupTimeoutMs,
|
||||
exitCode: options.child.exitCode,
|
||||
signalCode: options.child.signalCode,
|
||||
stderr: options.stderr(),
|
||||
|
|
@ -156,9 +172,10 @@ const browserStartupMessage = (
|
|||
diagnostics: {
|
||||
readonly executable: string;
|
||||
readonly elapsedMs: number;
|
||||
readonly startupTimeoutMs: number;
|
||||
readonly exitCode: number | null;
|
||||
readonly signalCode: NodeJS.Signals | null;
|
||||
readonly stderr: string;
|
||||
},
|
||||
): string =>
|
||||
`Chrome CDP startup ${failure}; executable=${diagnostics.executable}; elapsed_ms=${String(diagnostics.elapsedMs)}; exit_code=${diagnostics.exitCode === null ? "null" : String(diagnostics.exitCode)}; signal=${diagnostics.signalCode ?? "null"}; stderr=${diagnostics.stderr.length === 0 ? "<empty>" : diagnostics.stderr}`;
|
||||
`Chrome CDP startup ${failure}; executable=${diagnostics.executable}; elapsed_ms=${String(diagnostics.elapsedMs)}; startup_timeout_ms=${String(diagnostics.startupTimeoutMs)}; exit_code=${diagnostics.exitCode === null ? "null" : String(diagnostics.exitCode)}; signal=${diagnostics.signalCode ?? "null"}; stderr=${diagnostics.stderr.length === 0 ? "<empty>" : diagnostics.stderr}`;
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
import { readFile } from "node:fs/promises";
|
||||
import { join } from "node:path";
|
||||
|
||||
import { describe, expect } from "vitest";
|
||||
|
|
@ -9,6 +10,7 @@ import {
|
|||
} from "../../support/process/processFixture.js";
|
||||
|
||||
import {
|
||||
BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS,
|
||||
BrowserStartupError,
|
||||
waitForBrowserDevtoolsPort,
|
||||
} from "../../../src/browser/BrowserProcessStartup.js";
|
||||
|
|
@ -86,15 +88,68 @@ describe("browser process startup", () => {
|
|||
failure: "timeout",
|
||||
exitCode: null,
|
||||
signalCode: null,
|
||||
startupTimeoutMs: 20,
|
||||
});
|
||||
expect(String(failure)).toContain("stderr=<empty>");
|
||||
expect(String(failure)).toContain("startup_timeout_ms=20");
|
||||
} finally {
|
||||
child.kill("SIGKILL");
|
||||
expect(await waitForExit(child, 5_000)).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it("names the owner's deadline when a caller omits one", async ({
|
||||
processes,
|
||||
}) => {
|
||||
const root = await temporaryRoot();
|
||||
const child = processes.spawn(process.execPath, [
|
||||
"-e",
|
||||
"setTimeout(() => process.exit(3), 10)",
|
||||
]);
|
||||
|
||||
const failure = await waitForBrowserDevtoolsPort({
|
||||
child,
|
||||
executable: process.execPath,
|
||||
activePortPath: join(root, "DevToolsActivePort"),
|
||||
stderr: () => "owner default fixture",
|
||||
pollIntervalMs: 5,
|
||||
}).catch((cause: unknown) => cause);
|
||||
|
||||
expect(failure).toBeInstanceOf(BrowserStartupError);
|
||||
expect(failure).toMatchObject({
|
||||
failure: "exited",
|
||||
startupTimeoutMs: BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS,
|
||||
});
|
||||
expect(String(failure)).toContain(
|
||||
`startup_timeout_ms=${String(BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS)}`,
|
||||
);
|
||||
});
|
||||
|
||||
it("keeps one owner for the DevToolsActivePort deadline", async () => {
|
||||
expect(BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS).toBe(60_000);
|
||||
const callers = [
|
||||
"../../../scripts/verify-real-browser.mjs",
|
||||
"../../../scripts/lib/browser-runtime-fixture-lifecycle.mjs",
|
||||
];
|
||||
for (const caller of callers) {
|
||||
const source = await readFile(new URL(caller, import.meta.url), "utf8");
|
||||
expect(devtoolsPortDeadlines(source)).toEqual([
|
||||
["BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS"],
|
||||
]);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
const temporaryRoot = async (): Promise<string> => {
|
||||
return createTestTempDirectory("rea-browser-startup-");
|
||||
};
|
||||
|
||||
const devtoolsPortDeadlines = (source: string): string[][] => {
|
||||
const calls = /waitForBrowserDevtoolsPort\(\{(?<body>[\s\S]*?)\}\)/gu;
|
||||
const deadlines = /\btimeoutMs:\s*(?<value>[^\n,]+)/gu;
|
||||
return [...source.matchAll(calls)].map((call) =>
|
||||
[...String(call.groups?.body).matchAll(deadlines)].map((deadline) =>
|
||||
String(deadline.groups?.value).trim(),
|
||||
),
|
||||
);
|
||||
};
|
||||
|
|
|
|||
Loading…
Reference in New Issue