Bound Playwright daemon executions

[IlyaasK]

Summary

Bound each Playwright daemon request with one request-level timeout at the socket handler. The timeout now covers transform, browser connection/setup, and user code, and the timer is cleared after completion.

The daemon also carries the request deadline into setup and checks it before starting user code, so a request that times out during setup cannot later run the submitted script after the caller already received a timeout response.

Also make the cached browser disconnect handler instance-aware so a stale disconnected event cannot clear a newer live browser connection.

For the display resize e2e path, retry the initial about:blank Playwright navigation with require.Eventually instead of failing on one transient daemon/socket timeout.

Validation

Local:

• git diff --check
• bun build server/runtime/playwright-daemon.ts --outfile=/tmp/playwright-daemon.js --target=node --external playwright-core --external patchright --external esbuild
• cd server && go test ./e2e -run TestDoesNotExist -count=0
• ~/.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main clean after the deadline guard fix

GitHub Actions:

• scan: passed
• chromium-launcher test: passed
• Socket checks: passed
• test-server-unit: passed
• build-headful / docker: passed
• build-headless / docker: passed
• test-server-e2e: passed

---

Note

Medium Risk
Changes execution and timeout semantics on the Playwright automation path used by the instance API; behavior is improved but mis-timed aborts could affect long-running scripts or slow CDP setup.

Overview
Playwright daemon now enforces one request-level deadline via AbortSignal.timeout and withTimeout, so transform, CDP connect/setup, and user script all share the same bound. The handler aborts before running user code if setup already exceeded the limit, avoiding late script execution after the client got a timeout. Per-request setTimeout racing inside executeCode is removed.

CDP reconnect only clears the cached browser when the disconnected event comes from the current connection instance, so a stale disconnect cannot wipe a newer live session.

E2e: navigateBlank retries about:blank with require.EventuallyWithT (30s) instead of a single attempt. New TestPlaywrightExecuteTimeoutReturnsPromptlyAndRecovers asserts a 2s timeout returns within 5s with a proper timeout error (not socket I/O timeout) and that the next execute succeeds.

^{Reviewed by Cursor Bugbot for commit 3f3b5ad. Bugbot is set up for automated code reviews on this repo. Configure here.}

[rgarcia]

asks

• server/runtime/playwright-daemon.ts:150-190 — reduce the five requireWithinDeadline checkpoints to the single one before userFunction(...). the PR body states the goal precisely: a request that timed out during setup must not later run the submitted script. the check after new AsyncFunction(...) achieves that by itself. the checks after transform, connection, newContext, and newPage guard nothing observable — once the outer race is lost the response is already sent and the orphaned promise's result is discarded; a context or page created a beat late is harmless. each extra checkpoint is a line a future reader has to justify.
• server/runtime/playwright-daemon.ts:53-63 — with the checkpoints gone, delayWithinDeadline reduces to the plain setTimeout sleep it replaced. sleeping past the deadline inside an orphaned promise has no effect; drop the helper.

nits

• server/e2e/e2e_display_resize_window_test.go:468-485 — the if err != nil { require.NoError(collect, err); return } guards are redundant: require against a CollectT already aborts the closure via FailNow, so the three branches collapse to three consecutive require calls. the trailing return in the !Success branch is dead either way.

[IlyaasK]

Reduced the timeout setup checks to the single guard immediately before userFunction(...), removed the now-redundant delayWithinDeadline helper, and collapsed the display resize EventuallyWithT assertions.

[rgarcia]

follow-up on the deadline guard: requireWithinDeadline(deadlineMs, timeoutMs) has two legibility problems, and timeoutMs is dead weight besides.

• deadlineMs is an epoch timestamp, but nothing in the name or type says so — it reads like a duration at the call site, especially next to timeoutMs which is one.
• timeoutMs exists only to format the error message — and that message is effectively unobservable. the deadline and the outer withTimeout timer start at the same moment with the same duration, so by the time Date.now() >= deadlineMs the race has already rejected and the socket has already written the timeout response; the guard's response object loses the race and is discarded. its only real job is "don't run user code."

replace both with an AbortSignal — the platform primitive for exactly this:

// handleConnection
const timeoutMs = request.timeout_ms ?? 60000;
const signal = AbortSignal.timeout(timeoutMs);
let response: ExecuteResponse;
try {
  response = await withTimeout(executeCode(request, signal), timeoutMs);
} catch (error: any) {
  ...

// executeCode — drop requireWithinDeadline entirely
async function executeCode(request: ExecuteRequest, signal: AbortSignal): Promise<ExecuteResponse> {
  ...
  const userFunction = new AsyncFunction('page', 'context', 'browser', jsCode);
  signal.throwIfAborted();

  const result = await userFunction(page, context, browserInstance);

same line count, but: the type says what the parameter is, the message-only param disappears (throwIfAborted throws a standard TimeoutError), and executeCode(request, signal) needs no explanation.

[IlyaasK]

The daemon now passes an AbortSignal into executeCode, creates it with AbortSignal.timeout(timeoutMs), and uses signal.throwIfAborted() immediately before userFunction(...)

Thank you for your feedback :)

[rgarcia]

one last polish on the same thread: there are now two independent timers for the same deadline — the signal's internal one (drives throwIfAborted) and withTimeout's setTimeout (drives the race). they fire within milliseconds of each other but not atomically, so there's a small window where the race has rejected and the timeout response is already written, but the signal hasn't aborted yet — throwIfAborted() passes and user code starts anyway. that's the exact edge the guard exists to close.

drive the race off the signal so "response written" and "signal aborted" are the same event:

function withTimeout<T>(promise: Promise<T>, signal: AbortSignal): Promise<T> {
  return Promise.race([
    promise,
    new Promise<never>((_, reject) => {
      signal.addEventListener('abort', () => reject(signal.reason), { once: true });
    }),
  ]);
}

// handleConnection
const signal = AbortSignal.timeout(request.timeout_ms ?? 60000);
let response: ExecuteResponse;
try {
  response = await withTimeout(executeCode(request, signal), signal);
} catch (error: any) {
  ...

one deadline authority, the clearTimeout/.finally bookkeeping disappears (the platform owns the timer), and the race rejects with the same TimeoutError the guard throws.

[IlyaasK]

withTimeout now races on the same AbortSignal passed to executeCode, so the response timeout and signal abortion are driven by the same event

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 5a8b33d. Configure here.}

[cursor]

Late abort promise unhandled rejections

Medium Severity

withTimeout uses Promise.race with an abort listener that still rejects after the main work promise settles. When executeCode finishes before AbortSignal.timeout fires, the timer later rejects the losing racer with no handler, which can emit unhandled promise rejections under steady Playwright traffic.

 

^{Reviewed by Cursor Bugbot for commit 5a8b33d. Configure here.}

[IlyaasK]

Not changing code for this one. Promise.race registers handlers on each input promise, so a later rejection from the losing abort branch is still handled by the race and does not emit an unhandledRejection. I verified this locally with the exact AbortSignal.timeout + withTimeout shape from this PR; the abort promise rejected after the work promise resolved and Node emitted no unhandledRejection. The response timeout and pre-user-code guard are now driven by the same signal as intended.

[rgarcia]

one gap before this lands: test-server-e2e never exercises the timeout actually firing. TestPlaywrightExecuteAPI covers the happy path through the new plumbing and TestPlaywrightDaemonRecovery covers the reconnect/disconnected path, but no test submits code that exceeds timeout_ms and asserts a prompt success=false response — which is the behavior this PR exists to add. the withTimeout race and throwIfAborted() guard only run their non-triggering branches in CI, so a green run verifies the change didn't break normal execution but not that the fix fixes the bug.

add a test alongside the existing ones in e2e_playwright_test.go:

1. execute await new Promise(r => setTimeout(r, 30000)); with TimeoutSec: 2
2. assert the response arrives within a few seconds with success=false and a timeout error — this is the regression test for the original read unix @->/tmp/playwright-daemon.sock: i/o timeout flake
3. execute a second normal request (e.g. the existing navigator.userAgent snippet) and assert it succeeds — locking in the "daemon stays responsive after a timed-out request" behavior deferred from the bugbot thread

[IlyaasK]

one gap before this lands: test-server-e2e never exercises the timeout actually firing. TestPlaywrightExecuteAPI covers the happy path through the new plumbing and TestPlaywrightDaemonRecovery covers the reconnect/disconnected path, but no test submits code that exceeds timeout_ms and asserts a prompt success=false response — which is the behavior this PR exists to add. the withTimeout race and throwIfAborted() guard only run their non-triggering branches in CI, so a green run verifies the change didn't break normal execution but not that the fix fixes the bug.

add a test alongside the existing ones in e2e_playwright_test.go:

1. execute `await new Promise(r => setTimeout(r, 30000));` with `TimeoutSec: 2`

2. assert the response arrives within a few seconds with `success=false` and a timeout error — this is the regression test for the original `read unix @->/tmp/playwright-daemon.sock: i/o timeout` flake

3. execute a second normal request (e.g. the existing `navigator.userAgent` snippet) and assert it succeeds — locking in the "daemon stays responsive after a timed-out request" behavior deferred from the bugbot thread

added this in TestPlaywrightExecuteTimeoutReturnsPromptlyAndRecovers.

It submits await new Promise(r => setTimeout(r, 30000)); with TimeoutSec: 2, asserts the response comes back within a few seconds with success=false and a timeout error rather than the API socket i/o timeout, then sends a follow-up navigator.userAgent request and asserts it succeeds.