fix(agent-browser): bound crashed preview cleanup
This commit is contained in:
1 parent
23c49aa3d2
commit
c52a559b46
3 files changed
+146
-10
No files matched your search
@@ -0,0 +1,77 @@
|
|||||||
|
# Task: Remediate X-01 crashed CDP cleanup
|
||||||
|
|
||||||
|
## Identity
|
||||||
|
|
||||||
|
- Task ID: 20260827-x01-crash-cleanup-7d4b1e92
|
||||||
|
- Mode: Feature
|
||||||
|
- Branch: codex/20260827-x01-crash-cleanup-7d4b1e92-x01-crash-cleanup-7d4b1e92
|
||||||
|
- Worktree: D:\Datas\OthersProjects\makelore-x01-crash-cleanup-7d4b1e92
|
||||||
|
- Base commit: 23c49aa3d28730be657387cf9c061fb3a3bb53a8
|
||||||
|
- Owner: codex
|
||||||
|
- Status: Ready for Integration
|
||||||
|
|
||||||
|
## Scope
|
||||||
|
|
||||||
|
- Bound best-effort preview-data cleanup CDP commands in
|
||||||
|
`electron/agent-browser/module.ts`, preserving normal removal, child release,
|
||||||
|
and auto-attach ordering.
|
||||||
|
- Add the focused crashed-renderer/never-settling-removal regression in
|
||||||
|
`tests/unit/agent-browser-core.test.ts`.
|
||||||
|
- Own only this task record and the two files above; do not modify the
|
||||||
|
coordinator, open a PR, or perform live acceptance.
|
||||||
|
|
||||||
|
## Intent And Constraints
|
||||||
|
|
||||||
|
- X-01 packaged evidence showed a crashed renderer can leave
|
||||||
|
`Page.removeScriptToEvaluateOnNewDocument` pending forever, blocking the next
|
||||||
|
public data-enabled `open` during cleanup.
|
||||||
|
- Race each cleanup command against the existing bounded best-effort cleanup
|
||||||
|
convention (1,000 ms), swallow command failure/timeout, and clear timers.
|
||||||
|
- Preserve successful cleanup ordering and the existing preview token
|
||||||
|
invalidation semantics; avoid a broad CDP refactor.
|
||||||
|
|
||||||
|
## Planning Gate
|
||||||
|
|
||||||
|
- Result: Passed.
|
||||||
|
- Evidence: concurrent task gate started this isolated feature task from exact
|
||||||
|
base `23c49aa3d28730be657387cf9c061fb3a3bb53a8`; current task record,
|
||||||
|
project-memory entry documents, coordinator scope, lifecycle/X-01 pointers,
|
||||||
|
and relevant Agent Browser seams were read; no conflicting owned path was
|
||||||
|
found.
|
||||||
|
|
||||||
|
## Plan
|
||||||
|
|
||||||
|
1. Add a narrow bounded debugger-cleanup helper and apply it to preview script
|
||||||
|
removal, child waiting-target release, and cleanup auto-attach restoration.
|
||||||
|
2. Prove a never-settling removal response cannot block crash invalidation and
|
||||||
|
the next data-enabled open.
|
||||||
|
3. Run focused tests, typecheck, scoped lint, project-docs gates, and commit
|
||||||
|
once from the exact base.
|
||||||
|
|
||||||
|
## Outcome
|
||||||
|
|
||||||
|
- Added a 1,000 ms timer-bounded best-effort debugger command helper and used it
|
||||||
|
for preview script removal, document-child waiting-target release, and
|
||||||
|
cleanup `Target.setAutoAttach`. Normal successful cleanup retains its prior
|
||||||
|
ordering. A never-settling script-removal regression now proves renderer crash
|
||||||
|
invalidation does not block the next data-enabled open.
|
||||||
|
|
||||||
|
## Verification
|
||||||
|
|
||||||
|
- PASS: `corepack pnpm vitest run tests/unit/agent-browser-core.test.ts` (78/78).
|
||||||
|
- PASS: `corepack pnpm run typecheck`.
|
||||||
|
- PASS: `corepack pnpm exec eslint electron/agent-browser/module.ts tests/unit/agent-browser-core.test.ts`.
|
||||||
|
- PASS: `git diff --check`.
|
||||||
|
- PASS: `check_doc_drift.py --task-id 20260827-x01-crash-cleanup-7d4b1e92`.
|
||||||
|
- The isolated worktree required `corepack pnpm install --frozen-lockfile
|
||||||
|
--offline`; it changed no tracked dependency files.
|
||||||
|
|
||||||
|
## Follow-ups
|
||||||
|
|
||||||
|
- No live/E2E run was requested for this bounded X-01 remediation; coordinator
|
||||||
|
should rerun live acceptance after integrating the commit.
|
||||||
|
|
||||||
|
## Promotion Candidates
|
||||||
|
|
||||||
|
- None; this is a feature-task implementation with no canonical project-memory
|
||||||
|
change.
|
||||||
@@ -36,6 +36,7 @@ const DEFAULT_CDP_TIMEOUT_MS = 10_000;
|
|||||||
const MAX_CDP_TIMEOUT_MS = 30_000;
|
const MAX_CDP_TIMEOUT_MS = 30_000;
|
||||||
const OPEN_TIMEOUT_MS = 30_000;
|
const OPEN_TIMEOUT_MS = 30_000;
|
||||||
const RENDERER_PRIME_URL = 'about:blank';
|
const RENDERER_PRIME_URL = 'about:blank';
|
||||||
|
const PREVIEW_CLEANUP_TIMEOUT_MS = 1_000;
|
||||||
const PUBLISH_PREFLIGHT_TIMEOUT_MS = 30_000;
|
const PUBLISH_PREFLIGHT_TIMEOUT_MS = 30_000;
|
||||||
const PUBLISH_PREFLIGHT_SETTLE_MS = 500;
|
const PUBLISH_PREFLIGHT_SETTLE_MS = 500;
|
||||||
const PUBLISH_PREFLIGHT_VIEWPORTS = [
|
const PUBLISH_PREFLIGHT_VIEWPORTS = [
|
||||||
@@ -927,21 +928,17 @@ export class AgentBrowserModule {
|
|||||||
record.previewDataRequested = false;
|
record.previewDataRequested = false;
|
||||||
const scripts = binding.scripts.splice(0);
|
const scripts = binding.scripts.splice(0);
|
||||||
await Promise.all(scripts.map(async ({ identifier, sessionRef }) => {
|
await Promise.all(scripts.map(async ({ identifier, sessionRef }) => {
|
||||||
try {
|
await bestEffortDebuggerCommand(() => record.view.webContents.debugger.sendCommand(
|
||||||
await record.view.webContents.debugger.sendCommand(
|
|
||||||
'Page.removeScriptToEvaluateOnNewDocument',
|
'Page.removeScriptToEvaluateOnNewDocument',
|
||||||
{ identifier },
|
{ identifier },
|
||||||
sessionRef,
|
sessionRef,
|
||||||
);
|
));
|
||||||
} catch {
|
|
||||||
// The debugger or child target may already be detached.
|
|
||||||
}
|
|
||||||
if (sessionRef) {
|
if (sessionRef) {
|
||||||
await record.view.webContents.debugger.sendCommand(
|
await bestEffortDebuggerCommand(() => record.view.webContents.debugger.sendCommand(
|
||||||
'Runtime.runIfWaitingForDebugger',
|
'Runtime.runIfWaitingForDebugger',
|
||||||
undefined,
|
undefined,
|
||||||
sessionRef,
|
sessionRef,
|
||||||
).catch(() => undefined);
|
));
|
||||||
}
|
}
|
||||||
}));
|
}));
|
||||||
if (
|
if (
|
||||||
@@ -949,11 +946,11 @@ export class AgentBrowserModule {
|
|||||||
&& !record.view.webContents.isDestroyed()
|
&& !record.view.webContents.isDestroyed()
|
||||||
&& record.view.webContents.debugger.isAttached()
|
&& record.view.webContents.debugger.isAttached()
|
||||||
) {
|
) {
|
||||||
await record.view.webContents.debugger.sendCommand('Target.setAutoAttach', {
|
await bestEffortDebuggerCommand(() => record.view.webContents.debugger.sendCommand('Target.setAutoAttach', {
|
||||||
autoAttach: true,
|
autoAttach: true,
|
||||||
waitForDebuggerOnStart: false,
|
waitForDebuggerOnStart: false,
|
||||||
flatten: true,
|
flatten: true,
|
||||||
}).catch(() => undefined);
|
}));
|
||||||
}
|
}
|
||||||
if (record.previewData === binding) record.previewData = undefined;
|
if (record.previewData === binding) record.previewData = undefined;
|
||||||
})();
|
})();
|
||||||
@@ -1994,6 +1991,20 @@ function delay(milliseconds: number): Promise<void> {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async function bestEffortDebuggerCommand(operation: () => Promise<unknown>): Promise<void> {
|
||||||
|
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||||
|
try {
|
||||||
|
await Promise.race([
|
||||||
|
Promise.resolve().then(operation).catch(() => undefined),
|
||||||
|
new Promise<void>((resolvePromise) => {
|
||||||
|
timer = setTimeout(resolvePromise, PREVIEW_CLEANUP_TIMEOUT_MS);
|
||||||
|
}),
|
||||||
|
]);
|
||||||
|
} finally {
|
||||||
|
if (timer) clearTimeout(timer);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
function isTimeoutError(error: unknown): boolean {
|
function isTimeoutError(error: unknown): boolean {
|
||||||
return error instanceof Error && error.message === 'PUBLISH_PREFLIGHT_TIMEOUT';
|
return error instanceof Error && error.message === 'PUBLISH_PREFLIGHT_TIMEOUT';
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -924,6 +924,54 @@ describe('AgentBrowserModule', () => {
|
|||||||
expect(adapter.views).toHaveLength(2);
|
expect(adapter.views).toHaveLength(2);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('bounds crashed preview cleanup so a subsequent data-enabled open completes', async () => {
|
||||||
|
vi.useFakeTimers();
|
||||||
|
try {
|
||||||
|
const adapter = new FakeAdapter();
|
||||||
|
const preview = new FakePreviewDataSession();
|
||||||
|
adapter.onCreate = (view) => {
|
||||||
|
view.webContents.debugger.responders.set('Page.addScriptToEvaluateOnNewDocument', async () => ({
|
||||||
|
identifier: 'script-root',
|
||||||
|
}));
|
||||||
|
};
|
||||||
|
const module = new AgentBrowserModule(adapter, { previewDataSession: preview });
|
||||||
|
|
||||||
|
await module.open({
|
||||||
|
projectId: 'clock',
|
||||||
|
projectPath,
|
||||||
|
url: 'http://127.0.0.1:4173/',
|
||||||
|
injectProjectData: true,
|
||||||
|
});
|
||||||
|
const crashedView = adapter.views[0];
|
||||||
|
crashedView.webContents.debugger.responders.set(
|
||||||
|
'Page.removeScriptToEvaluateOnNewDocument',
|
||||||
|
async () => await new Promise<unknown>(() => undefined),
|
||||||
|
);
|
||||||
|
|
||||||
|
crashedView.webContents.emit('render-process-gone', {}, { reason: 'crashed' });
|
||||||
|
expect(preview.invalidations).toContain('browser_crashed');
|
||||||
|
expect(preview.getInjectionValue()).toBeNull();
|
||||||
|
|
||||||
|
const reopening = module.open({
|
||||||
|
projectId: 'clock',
|
||||||
|
projectPath,
|
||||||
|
url: 'http://127.0.0.1:4173/',
|
||||||
|
injectProjectData: true,
|
||||||
|
});
|
||||||
|
await vi.advanceTimersByTimeAsync(1_000);
|
||||||
|
|
||||||
|
await expect(reopening).resolves.toMatchObject({
|
||||||
|
state: 'attached',
|
||||||
|
generation: 2,
|
||||||
|
});
|
||||||
|
expect(preview.invalidations).toContain('browser_crashed');
|
||||||
|
expect(preview.opens).toHaveLength(2);
|
||||||
|
expect(adapter.views).toHaveLength(2);
|
||||||
|
} finally {
|
||||||
|
vi.useRealTimers();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
it('replaces a data-enabled browser when a later ordinary open omits the opt-in', async () => {
|
it('replaces a data-enabled browser when a later ordinary open omits the opt-in', async () => {
|
||||||
const adapter = new FakeAdapter();
|
const adapter = new FakeAdapter();
|
||||||
const preview = new FakePreviewDataSession();
|
const preview = new FakePreviewDataSession();
|
||||||
|
|||||||
Reference in new issue
Block a user