Skip to content

fix(trace): keep client paths out of remote launch options - #42436

Open
Devin Rousso (dcrousso) wants to merge 1 commit into
microsoft:mainfrom
dcrousso:fix-42394
Open

fix(trace): keep client paths out of remote launch options#42436
Devin Rousso (dcrousso) wants to merge 1 commit into
microsoft:mainfrom
dcrousso:fix-42394

Conversation

@dcrousso

Copy link
Copy Markdown
Contributor

remote browser servers can receive Playwright Test worker paths and try to write trace files on the client filesystem

remove artifactsDir and tracesDir from remote launch options

fixes#42394

remote browser servers can receive Playwright Test worker paths and try to write trace files on the client filesystem
remove `artifactsDir` and `tracesDir` from remote launch options
@github-actions

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

5 flaky⚠️ [chromium-library] › library/video.spec.ts:699 › screencast › should capture full viewport on hidpi `@chromium-ubuntu-22.04-node20`
⚠️ [chromium-library] › library/video.spec.ts:736 › screencast › should work with video+trace `@chromium-ubuntu-22.04-node20`
⚠️ [chromium-library] › library/chromium/chromium.spec.ts:436 › should produce network events, routing, and annotations for Service Worker (advanced) `@chromium-ubuntu-22.04-node22`
⚠️ [firefox-page] › page/page-event-request.spec.ts:181 › should return response body when Cross-Origin-Opener-Policy is set `@firefox-ubuntu-22.04-node20`
⚠️ [playwright-test] › ui-mode-trace.spec.ts:778 › should partition action tree state by test `@windows-latest-node22`

51325 passed, 1239 skipped


Merge workflow run.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "MCP"

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

8258 passed, 1361 skipped


Merge workflow run.

@github-actions

Copy link
Copy Markdown
Contributor

Hi, I'm the Playwright bot and I took a look at the CI failures here.

🟢 The PR is clear — the one failure is a pre-existing Firefox flake

The single red test, mcp/annotate.spec.ts:446 › should switch screencast to -s session on show --annotate, has nothing to do with this change. This PR only strips artifactsDir/tracesDir from remote launch options in the Playwright Test connect path; it doesn't touch the MCP screencast/annotate flow. And the test flips verdict on its own: on Firefox it failed 31 of 715 runs (~4%) and passed 684, across SHAs unrelated to this PR. The tests 1 report has no failures at all — only flakes rescued on retry.

Details

Overall: no failure is attributable to this PR. One real red, confirmed pre-existing flake; the rest is retry-rescued noise.

Pre-existing flake / infra

  • [firefox] › mcp/annotate.spec.ts:446 › should switch screencast to -s session on show --annotate (@mcp-windows-latest-firefox) — pre-existing Firefox flake. Cross-run history for this test: chrome 3/731 fail, chromium 2/727, webkit 1/731, msedge 0/717, and firefox 31/715 (~4%) — bimodal, on unrelated SHAs. The PR's diff (packages/playwright/src/index.ts remote launch options + a connect test) never reaches the MCP screencast/annotate code path. Recent Firefox run history, oldest→newest:

    🟩🟩🟩🟩🟩🟩🟩🟩🟩🟥🟥🟥🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟥🟩🟩🟩🟩🟩🟩🟩🟥🟩🟩🟩🟩🟩🟥🟩🟥

  • The tests 1 report's 5 ⚠️ entries (video.spec.ts:699/:736, chromium.spec.ts:436, page-event-request.spec.ts:181, ui-mode-trace.spec.ts:778) are flaky-passed-on-retry, not failures — nothing to triage.

Triaged by the Playwright bot - agent run

@aczekajski

Copy link
Copy Markdown

A bit of a nitpick, but it seems like the options coming from the x-playwright-launch-options header are filtered on the server side to begin with here:

launchOptions=filterLaunchOptions(launchOptions,isExtension||!!this._options.unsafe);

and the filtering function looking like this:
functionfilterLaunchOptions(options: LaunchOptionsWithTimeout,allowUnsafe: boolean): LaunchOptionsWithTimeout{
return{
channel: options.channel,
args: allowUnsafe ? options.args : undefined,
ignoreAllDefaultArgs: allowUnsafe ? options.ignoreAllDefaultArgs : undefined,
ignoreDefaultArgs: allowUnsafe ? options.ignoreDefaultArgs : undefined,
timeout: options.timeout,
headless: options.headless,
proxy: options.proxy,
chromiumSandbox: allowUnsafe ? options.chromiumSandbox : undefined,
firefoxUserPrefs: (isUnderTest()||allowUnsafe) ? options.firefoxUserPrefs : undefined,
slowMo: options.slowMo,
executablePath: (isUnderTest()||allowUnsafe) ? options.executablePath : undefined,
downloadsPath: allowUnsafe ? options.downloadsPath : undefined,
artifactsDir: (isUnderTest()||allowUnsafe) ? options.artifactsDir : undefined,
};
}

so the tracesDir was being filtered out anyway and artifactsDir was in fact allowed only when isUnsafe was being passed. And it is true that in my case where I had an error, I was launching the server with --usafe flag. My guess is that passing artifactsDir as a launch option is something used by the extension so it cannot be completely filtered out in there. Maybe the that filtering fn should make a distinction between "extension mode" and "--unsafe"? 🤔

@aczekajski

Copy link
Copy Markdown

Btw backporting just this small change to 1.62.1 also makes the apiRequestContext._wrapApiCall: End of central directory record signature not found. Either not a zip file, or file is truncated error gone for me ^^

headers: {
// HTTP headers are ASCII only (not UTF-8).
'x-playwright-launch-options': jsonStringifyForceASCII(_browserOptions),
'x-playwright-launch-options': jsonStringifyForceASCII({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filters like this should be done on the server side. In fact, we already do something along these lines in packages/playwright-core/src/remote/playwrightServer.ts (see filterLaunchOptions) - why doesn't that work?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants

@dcrousso@aczekajski@dgozman