Skip to content

fix(dispatcher): do not lose abort requests between progress controllers - #42247

Open
Pavel Feldman (pavelfeldman) wants to merge 2 commits into
microsoft:mainfrom
pavelfeldman:fix-abort-during-instrumentation
Open

fix(dispatcher): do not lose abort requests between progress controllers#42247
Pavel Feldman (pavelfeldman) wants to merge 2 commits into
microsoft:mainfrom
pavelfeldman:fix-abort-during-instrumentation

Conversation

@pavelfeldman

Copy link
Copy Markdown
Member

Summary

  • An abort arriving while the before/after instrumentation controller runs was dropped, since the controller for the actual call is created later.
  • Track the pending abort error per call and pass it to every progress controller created for that call.

An abort that arrives while the before/after instrumentation controller is
running was dropped, because the controller handling the actual call was
created afterwards. Track the pending abort error per call and pass it to
every controller created for that call.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

4 flaky ⚠️ [chromium-library] › library/popup.spec.ts:260 › should not throw when click closes popup `@chromium-ubuntu-22.04-node22`
⚠️ [chromium-library] › library/video.spec.ts:736 › screencast › should work with video+trace `@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`
⚠️ [firefox-page] › page/page-goto.spec.ts:90 › should work with Cross-Origin-Opener-Policy `@firefox-ubuntu-22.04-node20`

51121 passed, 1208 skipped


Merge workflow run.

createProgressController(callMetadata: CallMetadata): ProgressController {
return ProgressController.createForSdkObject(this._object, callMetadata);
createProgressController(callMetadata: CallMetadata, pendingAbortError?: Error): ProgressController {
return ProgressController.createForSdkObject(this._object, callMetadata, pendingAbortError);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why can't it just throw here if there is a pending error?

@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

8100 passed, 1311 skipped


Merge workflow run.

@github-actions

Copy link
Copy Markdown
Contributor

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

🟢 The one failure is a pre-existing flake — this PR looks clear

mcp/annotate.spec.ts:446 › should switch screencast to -s session on show --annotate failed on mcp-windows-latest-firefox, but the same test times out on unrelated SHAs, and your diff (dispatcher abort plumbing) doesn't touch the MCP screencast path.

Details

The latest reports show no real failures in the library suites (only flaky/interrupted), and a single MCP failure. Your change is confined to dispatcher.ts / progress.ts — how a pending abort error is threaded through progress controllers — which doesn't reach the show --annotate screencast flow.

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 flake. The exact same test timed out on PR feat(trace-viewer): render action point and target box in aria mode #42230 (mcp-macos-latest-chromium, run 31665582622), where this PR can't be responsible. More broadly, mcp/annotate.spec.ts flips across the results DB — 67 failures over 10,359 runs on SHAs unrelated to this change. The signature (a timeout on a single browser/OS, not reproducible on the engine your diff touches) fits a flake, and the abort-plumbing change doesn't exercise the screencast session switch this test asserts on.

Flaky (rescued on retry), not caused by this PR

  • The tests 1 report lists only flaky results — library/popup.spec.ts:260, library/video.spec.ts:736, library/chromium/chromium.spec.ts:373, page/page-event-request.spec.ts:181, page/page-goto.spec.ts:90 — all passed on retry and are unrelated to dispatcher aborts.

I'm a first pass, not the final word — if the annotate failure recurs on reruns of this exact commit, it's worth a second look, but right now nothing points at this PR.

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