chore(protocol): make timeout a protocol-level field by Skn0tt · Pull Request #41712 · microsoft/playwright

Skn0tt · 2026-07-09T14:31:00Z

Follows #41141, which made signal a protocol-level field. This does the same for timeout: every message carries it on Metadata, computed once on the client and read from progress.timeout on the server, instead of each method declaring its own timeout: float.

Metadata:
  timeout: float  # 👈 0 means "no timeout"

0 is the sentinel for "no deadline", matching what ProgressController.run already does. It's required on sendMessageToServer so the 0 is spelled out where it's genuinely meant, rather than hidden behind a ?? 0.

Most of the diff is mechanical - spec deletions plus regenerated channels/validator, staged separately for review.

Follows #41141, which made signal a protocol-level field. This does the same for timeout: every message carries it on Metadata, computed once on the client and read from progress.timeout on the server, instead of each method declaring its own timeout: float. 0 is the sentinel for no deadline.

…ut-field # Conflicts: # packages/playwright-core/src/client/channels.d.ts # packages/playwright-core/src/server/channels.d.ts # packages/protocol/src/validator.ts

dgozman · 2026-07-10T14:39:32Z

            addScheme(`${derived}${titleCase(methodName)}ErrorDetails`, `tType('${errorDetailsName}')`);
        }
-        channels_ts.push(`  ${methodName}(params: ${paramsName}, ${isDispatcher ? `progress: Progress` : `signal: AbortSignal | undefined`}): Promise<${resultName}>;`);
+        channels_ts.push(`  ${methodName}(params: ${paramsName}, ${isDispatcher ? `progress: Progress` : `options?: CommandOptions`}): Promise<${resultName}>;`);


Why are options now optional? Let's make them mandatory (like we had with the signal), so every caller thinks about it?

dgozman · 2026-07-10T14:46:05Z


  async uncheck(options: channels.ElementHandleUncheckOptions & TimeoutOptions = {}) {
-    return await this._elementChannel.uncheck({ ...options, timeout: this._frame._timeout(options) }, options.signal);
+    return await this._elementChannel.uncheck({ ...options }, { signal: options.signal, timeout: this._frame._timeout(options) });


Let's make all the helper methods like this._frame._timeout(options) handle both timeout and signal - this way the code will be much easier everywhere.

dgozman · 2026-07-10T14:46:48Z

 export type { Binary, Channel, ${structNames.join(', ')} } from '@protocol/structs';
-`];
+` + (target === 'Dispatcher' ? '' : `
+export type CommandOptions = { signal?: AbortSignal, timeout?: number };


Make both properties required: { signal: AbortSignal | undefined, timeout: number } to ensure all callers doing the right thing?

Make signal and timeout required fields on the generated CommandOptions struct, and pass them explicitly at every channel call site. Internal calls that opt out of a deadline use timeout: 0 as the disabled sentinel.

dgozman · 2026-07-13T13:19:25Z

    this._timeoutSettings.setDefaultTimeout(timeout);
  }

+  _timeout(options?: types.TimeoutOptions): channels.CommandOptions {


Call this _timeoutOptions() now?

Rename channels.CommandOptions to channels.TimeoutOptions?

dgozman · 2026-07-13T13:21:21Z

    if (this._connection.isRemote())
      throw new Error(`Path is not available when connecting remotely. Use saveAs() to save a local copy.`);
-    return (await this._channel.pathAfterFinished({}, undefined)).value;
+    return (await this._channel.pathAfterFinished({}, { signal: undefined, timeout: 0 })).value;


WDYT about exporting a kNoTimeout constant from some common helper, e.g. TimeoutSettings.ts?

dgozman · 2026-07-13T13:22:04Z

    };
    return await this._wrapApiCall(async () => {
-      const browser = Browser.from((await this._channel.launch(launchOptions, options.signal)).browser);
+      const browser = Browser.from((await this._channel.launch(launchOptions, { signal: options.signal, timeout: new TimeoutSettings().launchTimeout(options) })).browser);


TimeoutSettings.launchTimeout might as well handle the signal now.

dgozman · 2026-07-13T13:22:22Z

      noDefaults: params.noDefaults,
      artifactsDir: params.artifactsDir,
-    }, undefined);
+    }, { signal: undefined, timeout: new TimeoutSettings().timeout(params) });


Same here, let TimeoutSettings.timeout() handle the signal as well.

dgozman · 2026-07-13T13:23:17Z

 }

-export async function connectToEndpoint(parentConnection: Connection, params: channels.LocalUtilsConnectParams): Promise<Connection> {
+export async function connectToEndpoint(parentConnection: Connection, params: channels.LocalUtilsConnectParams, timeout: number): Promise<Connection> {


Accept TimeoutOptions instead of a number?

Addresses review feedback: rename CommandOptions to TimeoutOptions, add a shared kNoTimeout sentinel, have TimeoutSettings resolvers return the full { signal, timeout } options, and thread TimeoutOptions through connectToEndpoint. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 110d728d-5d20-42f6-93b9-e80ab89b3f66

Inline options.signal in the resolvers and rename the loose input alias to TimeoutInput to distinguish it from the resolved channels.TimeoutOptions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 110d728d-5d20-42f6-93b9-e80ab89b3f66

_timeoutOptions was a pure alias for _timeoutSettings.timeout(options); inline it at the call sites. Frame keeps its helpers since they pick between the timeout and navigationTimeout resolvers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 110d728d-5d20-42f6-93b9-e80ab89b3f66

Rename _timeoutOptions/_navigationTimeoutOptions back to _timeout/_navigationTimeout on Frame and update cross-file callers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 110d728d-5d20-42f6-93b9-e80ab89b3f66

reload/goBack/goForward/ariaSnapshot resolved their timeout via this._mainFrame, an unnecessary indirection since Page has its own _timeoutSettings. Call it directly, matching the pre-existing style. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 110d728d-5d20-42f6-93b9-e80ab89b3f66

Remove the per-dispatch metadataWithDefaults spread (the client always sends metadata.timeout, no method passes timeout via params, and the validator produces a fresh validMetadata) and the paramsForMetadata copy. Timeout is still folded into callMetadata.params for trace/inspector visibility, now isolated in a clearly-commented block to follow up on. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 110d728d-5d20-42f6-93b9-e80ab89b3f66

Non-JS and raw protocol consumers (e.g. the DebugController test backend) no longer have to send a timeout just to satisfy the schema. The JS client still always sends one; a missing timeout falls back to the disabled sentinel. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 110d728d-5d20-42f6-93b9-e80ab89b3f66

Skn0tt · 2026-07-14T07:27:22Z

      log: [],
    };
+
+    // TODO(skn0tt): promote to top-level metadata instead of smuggling through params.


i'd like to do this in a follow-up so this one doesn't balloon

dgozman · 2026-07-14T08:59:36Z

    waiter.rejectOnEvent<Frame>(this._page!, Events.Page.FrameDetached, new Error('Navigating frame was detached!'), frame => frame === this);
-    const timeout = this._page!._timeoutSettings.navigationTimeout(options);
+    const { timeout } = this._page!._timeoutSettings.navigationTimeout(options);
    waiter.rejectOnTimeout(timeout, `Timeout ${timeout}ms exceeded.`);


As a follow up, we can merge rejectOnTimeout and rejectOnSignal.

dgozman · 2026-07-14T09:01:55Z

-  async _expect(expression: string, options: Omit<channels.FrameExpectParams, 'expression'>, signal: AbortSignal | undefined): Promise<ExpectResult> {
-    const params: channels.FrameExpectParams = { expression, ...options, isNot: !!options.isNot };
-    params.expectedValue = serializeArgument(options.expectedValue);
+  async _expect(expression: string, options: Omit<channels.FrameExpectParams, 'expression'> & { timeout: number }, signal: AbortSignal | undefined): Promise<ExpectResult> {


As a follow up, either put both signal and timeout into options, or put them both into a separate TimeoutOptions argument.

dgozman · 2026-07-14T09:04:13Z

  }

  async _expectScreenshot(options: ExpectScreenshotOptions): Promise<{ actual?: Buffer, previous?: Buffer, diff?: Buffer, errorMessage?: string, log?: string[], timedOut?: boolean}> {
+    const { timeout, ...optionsWithoutTimeout } = options;


Follow up with supporting the signal here?

- unify signal into the timeout() destructure across waitForEvent methods - pass _timeout()/timeout() directly for screenshots - use kNoTimeout sentinel for isHidden/isVisible/newContext Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 110d728d-5d20-42f6-93b9-e80ab89b3f66

github-actions · 2026-07-14T12:22:50Z

Test results for "MCP"

1 failed
❌ [firefox] › mcp/annotate.spec.ts:447 › should switch screencast to -s session on show --annotate @mcp-macos-latest-firefox

7759 passed, 1249 skipped

Merge workflow run.

github-actions · 2026-07-14T12:29:37Z

Test results for "tests 1"

4 flaky

⚠️ [chromium-library] › library/video.spec.ts:645 › screencast › should capture full viewport `@chromium-ubuntu-22.04-arm-node20`
⚠️ [chromium-library] › library/chromium/oopif.spec.ts:282 › should click `@chromium-ubuntu-22.04-node24`
⚠️ [chromium-library] › library/beforeunload.spec.ts:130 › should support dismissing the dialog multiple times `@chromium-ubuntu-22.04-node20`
⚠️ [chromium-library] › library/video.spec.ts:717 › screencast › should work with video+trace `@chromium-ubuntu-22.04-node22`

49579 passed, 1156 skipped

Merge workflow run.

Skn0tt requested a review from dgozman July 9, 2026 14:31