fix(tracing): allow restarting tracing after a failed stop by yury-s · Pull Request #42425 · microsoft/playwright

Yury Semikhatsky (yury-s) · 2026-08-26T21:07:57Z

Summary

When tracing.stop() failed to save the trace (e.g. a disk write error), tracing on that context was left permanently wedged: every later tracing.start() threw "Tracing has been already started" and stop() threw "Must start tracing before stopping", with no recovery API. A failed stop also leaked the client-side stack session, so later traces kept appending calls to it.

client: stop() always sends tracingStop, then surfaces the save error.
server: stop() discards a chunk the client failed to stop; stopChunk() always releases the recording state.
client: release the stack session and delete the artifact when saving the trace fails.

Fixes#42423

Devin Rousso (dcrousso) · 2026-08-26T22:06:38Z

+ // Stop tracing even when saving the trace failed, otherwise tracing can
+ // never be started again on this context.
+ let error: Error | undefined = await this._doStopChunk(options.path).catch(e => e);
+ await this._channel.tracingStop({}, kNoTimeout).catch(e => error ??= e);


this still leaves tracing stuck if _doStopChunk() fails before stopping the chunk because tracingStop() rejects while the chunk is recording

Fixed — server-side stop() now discards a still-recording chunk before stopping, so tracingStop no longer rejects. Covered by "should stop tracing when the chunk was not stopped".

Devin Rousso (dcrousso) · 2026-08-26T22:11:27Z

- await this._channel.tracingStop({}, kNoTimeout);
+ // Stop tracing even when saving the trace failed, otherwise tracing can
+ // never be started again on this context.
+ let error: Error | undefined = await this._doStopChunk(options.path).catch(e => e);


if writing the trace archive fails, its client call stack state is not removed, so later traces continue appending calls to an abandoned trace record

Fixed — _doStopChunk now releases the stack session when saving fails. Covered by "should release the stack session when saving the trace fails".

Devin Rousso (dcrousso) · 2026-08-26T22:11:29Z

+ let error: Error | undefined = await this._doStopChunk(options.path).catch(e => e);
+ await this._channel.tracingStop({}, kNoTimeout).catch(e => error ??= e);
+ if (error)
+ throw error;


NIT: unless we specifically care about exposing an error from _doStopChunk over tracingStop then we could just do this
Suggested change
throwerror;
try{
awaitthis._doStopChunk(options.path);
}finally{
awaitthis._channel.tracingStop({},kNoTimeout);
}

We do want the save error to surface, and with the server-side discard tracingStop is not expected to throw anymore, so this is now const error = await ...catch(e => e); await tracingStop(); if (error) throw error; — same shape as browserContext.close().

Dmitry Gozman (dgozman) · 2026-08-27T07:47:56Z

I agree with Devin, if we want to make stop always stop, we have to audit the whole call-tree and ensure everything is cleaned up even on error, e.g. a StackSession, artifact, any trace counters, internal state on the server, etc.

github-actions · 2026-08-28T00:47:50Z

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

🔴 The failures are caused by this PR — its own new tracing tests fail on every browser

The latest "tests 1" run (33130700334) has 16 failures, and every one is a test this PR adds — library/tracing.spec.ts:423 and :442 — failing deterministically on chromium, firefox and webkit across all bots. That's the change under test, not noise. Everything else in the report is flaky (passed on retry), so there's nothing else to triage.

Details

Overall: one real, PR-introduced failure story — the two recovery tests this PR adds fail everywhere. No other true failures in this run. Worst call is red.

Caused by this PR

[chromium/firefox/webkit-library] › library/tracing.spec.ts:423 › should recover tracing after a failed stop and :442 › should stop tracing when the chunk was not stopped — the two tests this PR adds. They fail on all three browsers and across every bot (16 red: chromium @frozen-time, @realtime, arm-node20, node20/22/24; firefox-library; webkit-library) with no browser/platform divergence. Because they're brand new, there's no history elsewhere to call this a flake — the "always stop tracing after a failed stop" fix in client/tracing.ts + server/trace/recorder/tracing.ts isn't recovering the recording state the way these tests assert. This lines up with Dmitry Gozman (@dgozman)'s review note that making stop always stop needs the whole call-tree (StackSession, artifact, counters, server state) audited for cleanup on error. Reproduce locally with npm run ctest tests/library/tracing.spec.ts:423 before merging.

Pre-existing flake / infra

[chromium-library] › library/video.spec.ts:736 › screencast › should work with video+trace — flaked on four chromium bots (@realtime, node20/22/24) but passed on retry in this same run. It's tracing-adjacent, so worth a glance, but it recovered every time and isn't a hard failure.
[firefox-page] › page/page-event-request.spec.ts:181 › should return response body when Cross-Origin-Opener-Policy is set — flaked and passed on retry. Network response test, unrelated to the tracing change.

_{Triaged by the Playwright bot - agent run}

Dmitry Gozman (dgozman) · 2026-08-28T15:37:35Z

+ }
+ }

+ private async _stopChunk(progress: Progress, params: TracingTracingStopChunkParams): Promise<{ artifact?: Artifact, entries?: NameValue[] }> {


Perhaps we can make this method sync now? It has a single await that we can probably lift up to stopChunk.

Done — _stopChunk is sync now, the _fs.sync() await and result building moved into stopChunk.

Dmitry Gozman (dgozman) · 2026-08-28T15:38:50Z

+ test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/42423' });
+ await context.tracing.start();
+ await page.goto(server.PREFIX + '/input/button.html');
+ // Saving the chunk can fail before it was stopped, leaving the server recording.


Can we trigger this through public APIs instead of calling private methods on the channel?

You are right that it drives the protocol out of order — the sequence is not reachable through public APIs in a regular client (only a thin client can fail before stopping the chunk). Dropped the test; the server-side recovery in stop() stays.

When tracing.stop() failed to save the chunk (e.g. a trace file write errored), tracingStop was never sent, so the server kept its recording state and every later tracing.start() on that context threw "Tracing has been already started" for the rest of the context lifetime. Fixes: microsoft#42423

Addresses review feedback: stop() must leave tracing in a clean state even when it throws, so that a subsequent start() works. - server: stop() discards a chunk the client failed to stop, instead of rejecting with "Must stop trace file before stopping tracing". - server: stopChunk() always releases the recording state. - client: release the stack session when saving the trace fails, otherwise later traces keep appending calls to the abandoned session. - client: delete the artifact when saving it fails.

- server stop() unconditionally discards the chunk (a no-op when not recording), resetForReuse() delegates to it. - client: deleting the artifact no longer masks the saveAs error. - test: stack session test does not need a page.

The test drove the protocol out of order by calling private channel methods; the sequence is not reachable through public APIs in a regular client. The server-side recovery in stop() stays.

github-actions · 2026-08-28T18:49:58Z

Test results for "tests 1"

5 flaky

⚠️ [chromium-library] › library/video.spec.ts:699 › screencast › should capture full viewport on hidpi `@frozen-time-library-chromium-linux`
⚠️ [chromium-library] › library/browsercontext-page-event.spec.ts:173 › should work with Ctrl-clicking `@chromium-ubuntu-22.04-arm-node20`
⚠️ [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:90 › should work with Cross-Origin-Opener-Policy `@firefox-ubuntu-22.04-node20`
⚠️ [playwright-test] › ui-mode-trace.spec.ts:827 › should update state on subsequent run `@windows-latest-node22`

51264 passed, 1240 skipped

Merge workflow run.

github-actions · 2026-08-28T18:50:06Z

Test results for "MCP"

1 failed
❌ [firefox] › mcp/cli-core.spec.ts:131 › check @mcp-windows-latest-firefox

8258 passed, 1361 skipped

Merge workflow run.

github-actions · 2026-08-28T18:55:03Z

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

🟢 CI is clear — the one failure is a pre-existing flake

The only red job is windows-latest - firefox, on [firefox] › mcp/cli-core.spec.ts:131 › check. This PR only touches tracing (client/tracing.ts, server/trace/recorder/tracing.ts, and tests/library/tracing.spec.ts); the failing test exercises the MCP check action on a checkbox and never goes near tracing. It's a known flake on this bot, so nothing here is caused by your change.

Details

Pre-existing flake / infra

[firefox] › mcp/cli-core.spec.ts:131 › check — failed on mcp-windows-latest-firefox with expect(inlineSnapshot).toContain('- checkbox [checked] [ref=e2]') getting back - checkbox [ref=e2]: the checkbox snapshot was read before the check registered. Across the test-results DB this exact test + signature has failed on SHAs unrelated to this PR — PRs test(chromium): fixme flaky "should ignore page.pause when headed" on macOS #41959 and feat(locator): page-free by locators resolved with page.get() #42157, plus a push commit (4f25184) — while passing 694 of 697 runs on that bot. Firefox+Windows only; every other browser and OS is green (0 failures across 700+ runs each). The diff doesn't reach the MCP checkbox flow, so this is timing noise, not a regression.

No other jobs failed — the rest of the matrix (chromium/webkit/firefox across Linux/macOS/Windows, all Test Runner shards, installation and lint jobs) passed.

_{Triaged by the Playwright bot - agent run}

Yury Semikhatsky (yury-s) requested review from Devin Rousso (dcrousso) and Dmitry Gozman (dgozman)August 26, 2026 21:09

This comment has been minimized.
Sign in to view

Devin Rousso (dcrousso) requested changes Aug 26, 2026
View reviewed changes

JustasM (JustasMonkev) mentioned this pull request Aug 27, 2026
Track trace archive loss after page crash JustasMonkev/mcp-accessibility-scanner#190
Open

This comment has been minimized.
Sign in to view

Dmitry Gozman (dgozman) approved these changes Aug 28, 2026
View reviewed changes

Yury Semikhatsky (yury-s) added 5 commits August 28, 2026 10:30

chore(tracing): tracingStop is not expected to throw, trim comments
9af2e63

test(tracing): update action titles after step formatting change
a3b14e4

Yury Semikhatsky (yury-s)force-pushed the fix-mcp-1720 branch from 1eada8a to a3b14e4Compare August 28, 2026 17:33

chore(tracing): make _stopChunk synchronous, drop chunk-not-stopped test
c884529
The test drove the protocol out of order by calling private channel methods; the sequence is not reachable through public APIs in a regular client. The server-side recovery in stop() stays.

This comment has been minimized.
Sign in to view

Yury Semikhatsky (yury-s) requested a review from Devin Rousso (dcrousso)August 28, 2026 18:42

Devin Rousso (dcrousso) approved these changes Aug 28, 2026
View reviewed changes

Yury Semikhatsky (yury-s) merged commit a30296c into microsoft:mainAug 28, 2026
44 of 45 checks passed

Yury Semikhatsky (yury-s) deleted the fix-mcp-1720 branch August 28, 2026 22:00

-throwerror;
+try{
+awaitthis._doStopChunk(options.path);
+}finally{
+awaitthis._channel.tracingStop({},kNoTimeout);
+}

Uh oh!

Conversation

Yury Semikhatsky (yury-s) commented Aug 26, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmitry Gozman (dgozman) commented Aug 27, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actionsBot commented Aug 28, 2026

🔴 The failures are caused by this PR — its own new tracing tests fail on every browser

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

github-actionsBot commented Aug 28, 2026

Test results for "tests 1"

Uh oh!

github-actionsBot commented Aug 28, 2026

Test results for "MCP"

Uh oh!

github-actionsBot commented Aug 28, 2026

🟢 CI is clear — the one failure is a pre-existing flake

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Yury Semikhatsky (yury-s) commented Aug 26, 2026•
edited
Loading