mirror of
https://github.com/morluto/rea.git
synced 2026-11-11 17:28:08 +00:00
fix(browser): distinguish shutdown admission from failed cleanup (#10038)
This commit is contained in:
1 parent
81d4f6b42a
commit
2e67d57d0f
4 files changed
+109
-16
No files matched your search
@@ -8,7 +8,10 @@ import { AnalysisError } from "../domain/analysisErrorBase.js";
|
||||
import { BrowserObservationError } from "../domain/browserObservationError.js";
|
||||
import type { BrowserObservationOperation } from "../domain/browserObservationErrors.js";
|
||||
import { ProviderCleanupError } from "../domain/providerCleanupError.js";
|
||||
import { AnalysisCancelledError } from "../domain/analysisErrorCore.js";
|
||||
import {
|
||||
AnalysisCancelledError,
|
||||
AnalysisCapabilityUnavailableError,
|
||||
} from "../domain/analysisErrorCore.js";
|
||||
import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js";
|
||||
import { CdpConnection } from "./CdpConnection.js";
|
||||
import type { ExecutionOptions } from "../application/AnalysisProvider.js";
|
||||
@@ -119,7 +122,7 @@ export class CdpTargetSessionOwner {
|
||||
options: ExecutionOptions,
|
||||
limits: BrowserSocketLimits,
|
||||
): Promise<{ readonly connection: CdpConnection; readonly reused: boolean }> {
|
||||
this.#assertOpen(operation, "browser-endpoint");
|
||||
this.#assertOpen(operation);
|
||||
await this.retryPending();
|
||||
const leased = await this.#leaseExistingBrowserSocket(
|
||||
url,
|
||||
@@ -140,7 +143,7 @@ export class CdpTargetSessionOwner {
|
||||
{ readonly connection: CdpConnection; readonly reused: boolean } | undefined
|
||||
> {
|
||||
while (true) {
|
||||
this.#assertOpen(operation, "browser-endpoint");
|
||||
this.#assertOpen(operation);
|
||||
if (options.signal?.aborted === true)
|
||||
throw new AnalysisCancelledError(operation, {
|
||||
cause: options.signal.reason,
|
||||
@@ -427,9 +430,9 @@ export class CdpTargetSessionOwner {
|
||||
readonly handshakeTimeoutMs?: number;
|
||||
},
|
||||
): Promise<CdpConnection> {
|
||||
this.#assertOpen(operation, resource);
|
||||
this.#assertOpen(operation);
|
||||
await this.retryPending();
|
||||
this.#assertOpen(operation, resource);
|
||||
this.#assertOpen(operation);
|
||||
if (options.signal?.aborted === true)
|
||||
throw new AnalysisCancelledError(operation, {
|
||||
cause: options.signal.reason,
|
||||
@@ -576,13 +579,12 @@ export class CdpTargetSessionOwner {
|
||||
if (failures.length > 1) throw aggregateCleanupFailures(failures);
|
||||
}
|
||||
|
||||
#assertOpen(operation: BrowserObservationOperation, resource: string): void {
|
||||
#assertOpen(operation: BrowserObservationOperation): void {
|
||||
if (!this.#closing) return;
|
||||
throw new ProviderCleanupError(
|
||||
throw new AnalysisCapabilityUnavailableError(
|
||||
"rea-cdp-browser",
|
||||
[resource],
|
||||
{ reason: "CDP provider is closing and cannot admit another session." },
|
||||
{ operation },
|
||||
operation,
|
||||
"CDP provider is closing and cannot admit another session.",
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -1429,3 +1429,57 @@ it.each([false, true])(
|
||||
expect(screenshot).not.toHaveBeenCalled();
|
||||
},
|
||||
);
|
||||
|
||||
it.each(["clean", "pending", "queued"] as const)(
|
||||
"refuses scenario admission during %s shutdown without claiming cleanup failed",
|
||||
async (stage) => {
|
||||
let opened = 0;
|
||||
let finish: () => void = () => {};
|
||||
let started: () => void = () => {};
|
||||
const release = new Promise<void>((resolve) => {
|
||||
finish = resolve;
|
||||
});
|
||||
const closingStarted = new Promise<void>((resolve) => {
|
||||
started = resolve;
|
||||
});
|
||||
const session = new FakeSession("launch");
|
||||
session.close = async () => {
|
||||
started();
|
||||
await release;
|
||||
return "terminated-owned-process";
|
||||
};
|
||||
const provider = new PlaywrightBrowserScenarioProvider(async () => {
|
||||
opened += 1;
|
||||
return session;
|
||||
});
|
||||
let running: ReturnType<typeof provider.captureScenario> | undefined;
|
||||
let closing: Promise<void> | undefined;
|
||||
try {
|
||||
if (stage !== "clean") {
|
||||
running = provider.captureScenario(scenario());
|
||||
await closingStarted;
|
||||
}
|
||||
const queued =
|
||||
stage === "queued" ? provider.captureScenario(scenario()) : undefined;
|
||||
closing = provider.close();
|
||||
if (stage === "clean") await closing;
|
||||
const refusal = queued ?? provider.captureScenario(scenario());
|
||||
if (stage === "queued") finish();
|
||||
const result = await refusal;
|
||||
expect(result.ok).toBe(false);
|
||||
if (result.ok)
|
||||
throw new Error("Expected closed provider admission refusal");
|
||||
expect(result.error.cleanupIncomplete).toBe(false);
|
||||
expect(projectAnalysisError(result.error)).toMatchObject({
|
||||
code: "capability_unavailable",
|
||||
details: { operation: "capture_browser_scenario" },
|
||||
});
|
||||
expect(opened).toBe(stage === "clean" ? 0 : 1);
|
||||
} finally {
|
||||
finish();
|
||||
await running;
|
||||
await closing;
|
||||
await provider.close();
|
||||
}
|
||||
},
|
||||
);
|
||||
@@ -34,7 +34,10 @@ import {
|
||||
type BrowserStepArtifacts,
|
||||
} from "../domain/browserScenarioCaptureValues.js";
|
||||
import { AnalysisError } from "../domain/analysisErrorBase.js";
|
||||
import { AnalysisCancelledError } from "../domain/analysisErrorCore.js";
|
||||
import {
|
||||
AnalysisCancelledError,
|
||||
AnalysisCapabilityUnavailableError,
|
||||
} from "../domain/analysisErrorCore.js";
|
||||
import {
|
||||
BrowserObservationError,
|
||||
browserCleanupObservation,
|
||||
@@ -496,10 +499,11 @@ export class PlaywrightBrowserScenarioProvider implements BrowserScenarioCapture
|
||||
}
|
||||
|
||||
#closedError(): AnalysisError {
|
||||
return new BrowserObservationError(OPERATION, "cleanup_failed", {
|
||||
detail: "Browser scenario provider is closing.",
|
||||
cleanup: { reason: "Provider is closing", resources: [] },
|
||||
});
|
||||
return new AnalysisCapabilityUnavailableError(
|
||||
PLAYWRIGHT_BROWSER_SCENARIO_PROVIDER_IDENTITY.id,
|
||||
OPERATION,
|
||||
"Browser scenario provider is closing.",
|
||||
);
|
||||
}
|
||||
|
||||
#bindCleanup(
|
||||
|
||||
@@ -9,11 +9,44 @@ import { CdpConnection } from "../../../src/browser/CdpConnection.js";
|
||||
import { CdpBrowserProvider } from "../../../src/browser/CdpBrowserProvider.js";
|
||||
import { inspectWebPageInputSchema } from "../../../src/domain/browserObservation.js";
|
||||
import { AnalysisError } from "../../../src/domain/analysisErrorBase.js";
|
||||
import { AnalysisCancelledError } from "../../../src/domain/analysisErrorCore.js";
|
||||
import {
|
||||
AnalysisCancelledError,
|
||||
AnalysisCapabilityUnavailableError,
|
||||
} from "../../../src/domain/analysisErrorCore.js";
|
||||
import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js";
|
||||
import { startFakeCdpBrowser } from "../../fixtures/fakeCdpBrowser.js";
|
||||
import { awaitAcknowledgement } from "../../fixtures/lifecycleAcknowledgement.js";
|
||||
|
||||
it.each(["connect", "acquireBrowserSocket"] as const)(
|
||||
"refuses %s after clean shutdown without inventing retained cleanup",
|
||||
async (method) => {
|
||||
const owner = new CdpTargetSessionOwner();
|
||||
await owner.close();
|
||||
const endpoint = "ws://127.0.0.1:9/devtools/browser/closed";
|
||||
const failure = await (
|
||||
method === "connect"
|
||||
? owner.connect(endpoint, "inspect_web_page", {}, "not-acquired")
|
||||
: owner.acquireBrowserSocket(
|
||||
endpoint,
|
||||
"inspect_web_page",
|
||||
{},
|
||||
{
|
||||
commandTimeoutMs: 100,
|
||||
handshakeTimeoutMs: 100,
|
||||
},
|
||||
)
|
||||
).catch((cause: unknown) => cause);
|
||||
expect(failure).toBeInstanceOf(AnalysisCapabilityUnavailableError);
|
||||
expect(failure).toMatchObject({
|
||||
providerId: "rea-cdp-browser",
|
||||
operation: "inspect_web_page",
|
||||
cleanupIncomplete: false,
|
||||
cleanupResources: [],
|
||||
});
|
||||
await owner.close();
|
||||
},
|
||||
);
|
||||
|
||||
it("joins a late owned handshake and waits for its active operation", async () => {
|
||||
const http = createServer();
|
||||
const webSockets = new WebSocketServer({ noServer: true });
|
||||
|
||||
Reference in new issue
Block a user