Skip to content

fix(mcp): check tracing state before stopping in browser_stop_tracing#41040

Open
SebTardif wants to merge 2 commits into
microsoft:mainfrom
SebTardif:fix/tracing-stop-check-before-stop
Open

fix(mcp): check tracing state before stopping in browser_stop_tracing#41040
SebTardif wants to merge 2 commits into
microsoft:mainfrom
SebTardif:fix/tracing-stop-check-before-stop

Conversation

@SebTardif
Copy link
Copy Markdown
Contributor

browser_stop_tracing calls browserContext.tracing.stop() unconditionally before checking the traceLegendSymbol guard. When tracing was started via config (saveTrace: true) rather than via the browser_start_tracing tool, the symbol is not set. The tool kills the config-initiated trace recording, then throws "Tracing is not started." The destructive side effect fires before the guard check.

Fix

Move the traceLegendSymbol check before tracing.stop() so the guard is evaluated first and the active trace is preserved on the error path.

Trigger scenario

  1. User starts MCP server with saveTrace = true in config
  2. Config-initiated tracing starts in context.ts:329-335 (no traceLegendSymbol set)
  3. MCP client calls browser_stop_tracing
  4. Before fix: tracing.stop() kills the config trace, then the symbol check throws "Tracing is not started"
  5. After fix: symbol check throws "Tracing is not started" immediately; config trace continues recording

The config-initiated trace has its own cleanup via the disposable registered at context.ts:336-340, so the tool should not interfere with it.

Test

Added a test that starts with saveTrace: true, calls browser_stop_tracing (expects error), then verifies the config trace is still active by confirming browser_start_tracing fails with "Tracing has been already started."

Bug introduced in #39646 (2026-03-12).

browser_stop_tracing called tracing.stop() unconditionally before
checking the traceLegendSymbol. If tracing was started via config
(saveTrace: true) rather than via browser_start_tracing, the tool
killed the config-initiated trace, then threw 'Tracing is not
started'. The destructive side effect happened before the guard.

Move the traceLegendSymbol check before tracing.stop() so the
guard is evaluated first and the active trace is preserved on the
error path.

Bug introduced in microsoft#39646 (2026-03-12).

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Member

@yury-s yury-s left a comment

Choose a reason for hiding this comment

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

Please fix infra / docs & lint (pull_request) and we can merge this.

The Config type does not have a saveTrace property, causing a
TS2353 compile error in the docs & lint CI check. Simplify the
test to verify the core behavior: calling browser_stop_tracing
without browser_start_tracing returns an error.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
Copy link
Copy Markdown
Contributor Author

Fixed the TS2353 lint error in 82076da.

Note: this is more than a lint fix. The third test originally verified two behaviors:

  1. browser_stop_tracing without browser_start_tracing returns an error
  2. Config-initiated tracing (via saveTrace: true) survives the failed stop

The saveTrace property does not exist in the Config type, so the second scenario was never actually exercised. I removed it and the invalid config property, keeping only the first assertion. If there is a correct way to start tracing via config that I missed, happy to restore the full test with the right property name.

@yury-s
Copy link
Copy Markdown
Member

yury-s commented Jun 3, 2026

Fixed the TS2353 lint error in 82076da.

Note: this is more than a lint fix. The third test originally verified two behaviors:

  1. browser_stop_tracing without browser_start_tracing returns an error
  2. Config-initiated tracing (via saveTrace: true) survives the failed stop

The saveTrace property does not exist in the Config type, so the second scenario was never actually exercised. I removed it and the invalid config property, keeping only the first assertion. If there is a correct way to start tracing via config that I missed, happy to restore the full test with the right property name.

saveTrace was removed in #39538, I'll remove all remaining references to it from the code.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

Test results for "MCP"

7243 passed, 1103 skipped


Merge workflow 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.

3 participants