Skip to content

fix(screencast): acknowledge frames after callbacks#41898

Merged
Skn0tt merged 2 commits into
microsoft:mainfrom
Skn0tt:skn0tt-screencast-frame-ack
Jul 21, 2026
Merged

fix(screencast): acknowledge frames after callbacks#41898
Skn0tt merged 2 commits into
microsoft:mainfrom
Skn0tt:skn0tt-screencast-frame-ack

Conversation

@Skn0tt

@Skn0tt Skn0tt commented Jul 21, 2026

Copy link
Copy Markdown
Member

This PR makes page.screencast.start({ onFrame }) use callback completion as backpressure. Fixes a bug in the OR logic (one sync ack was swallowed by pending clients), and replaced ManualPromise with LongStandingScope so we can use the non-leaking safeRace.

Re microsoft/playwright-python#3145

Re: microsoft/playwright-python#3145

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2848e8f0-1e34-4996-a6d6-7e48c00bb8f3
@Skn0tt
Skn0tt requested a review from dgozman July 21, 2026 10:07
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@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 looks clear — both real failures are pre-existing flakes

This PR only touches screencast frame acknowledgement in packages/playwright-core (server/client screencast, page dispatcher, the cr/ff/wk/bidi hooks, protocol wiring, and tests/library/screencast.spec.ts). Neither failing test goes near that code, and each one already fails on unrelated PRs.

Details

Pre-existing flake / infra

  • [playwright-test] › reporter-html.spec.ts:3402 › created › speedboard › clicking on label should not exit speedboard (macos-latest-node22) — pre-existing flake. Same test, same bot, same signature (toMatchAriaSnapshot timing out at 75s on the "Slowest Tests" speedboard) also failed on PR fix(test): increase Intel macOS timeout #41851 (sha fd9b0e9ab6), where a retry rescued it. Across the results DB it's 1 hard failure vs 911 passing runs — a macOS HTML-reporter UI test that has nothing to do with screencast.

  • [firefox] › mcp/annotate.spec.ts:173 › user-initiated annotate downloads zip with feedback.md (ubuntu + macos) — pre-existing flake. This one flakes broadly on unrelated work: firefox 12 failures across 12 SHAs / 9 PRs (315 passes), and it also flakes on webkit (12 fails / 7 PRs), chrome, and chromium. The MCP annotate download path doesn't exercise any screencast code.

The 9 "flaky" entries in the tests-1 report (including several library/video.spec.ts screencast/video cases) were all rescued on retry, so there's nothing failing there to triage.

Triaged by the Playwright bot

Triaged by the Playwright bot - agent run

Comment thread tests/library/screencast.spec.ts
Comment thread tests/library/screencast.spec.ts
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2848e8f0-1e34-4996-a6d6-7e48c00bb8f3
@Skn0tt
Skn0tt merged commit bc97a13 into microsoft:main Jul 21, 2026
45 of 47 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

2 flaky ⚠️ [chromium-library] › library/video.spec.ts:699 › screencast › should capture full viewport on hidpi `@chromium-ubuntu-22.04-arm-node20`
⚠️ [firefox-page] › page/page-goto.spec.ts:81 › should work with Cross-Origin-Opener-Policy `@firefox-ubuntu-22.04-node20`

49998 passed, 1189 skipped


Merge workflow run.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "MCP"

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

7759 passed, 1249 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 looks clear — the one failure is a pre-existing flake

The latest merged report has a single red: [chromium] › mcp/annotate.spec.ts:446 › should switch screencast to -s session on show --annotate on mcp-macos-latest-chromium. Despite the "screencast" in its name, this is an MCP annotate test that flakes broadly on unrelated work, so it isn't this PR.

Details

Pre-existing flake / infra

This PR's diff is confined to screencast frame acknowledgement in packages/playwright-coreserver/screencast.ts, dispatchers/pageDispatcher.ts, the cr/ff/wk/bidi page hooks, client/screencast.ts, the protocol/validator wiring — plus tests/library/screencast.spec.ts. The MCP annotate CLI path doesn't exercise that code, and the earlier merged reports on this PR (the library screencast/video cases, reporter-html, and annotate.spec.ts:173) were all rescued on retry or are the same broadly-flaking annotate test.

Triaged by the Playwright bot - agent run

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants