fix(performance): reset trace-running flag when start_trace setup fails (#2420)

Fixes #2419

## Problem

`performance_start_trace` sets the context "trace is running" flag
before several fallible `await`s (the `about:blank` navigation,
`tracing.start`, and the navigation back to the page URL). The flag is
only cleared inside `stopTracingAndAppendOutput`'s `finally`, which is
never reached if any of those awaits reject. With `reload`/`autoStop`
defaulting to `true`, a failed initial navigation leaves the flag stuck
`true` for the rest of the session, so every later
`performance_start_trace` short-circuits with "a performance trace is
already running" — although no trace is running.

## Fix

Wrap the start body in `try/catch`. On failure, stop tracing if it was
started (ignoring "not started" errors) and clear the flag before
rethrowing, so the original error still reaches the client. When
`autoStop` already ran `stopTracingAndAppendOutput`, the flag is already
`false` and the catch is a no-op — no double-stop, no dangling tracer.

## Testing

- Added a regression test (`resets the running flag if a setup step
throws`) that stubs the initial navigation to reject and asserts the
flag is reset and that a follow-up `performance_start_trace` proceeds.
It fails on `main` (`true !== false`) and passes with this change.
- `npm run test tests/tools/performance.test.ts` — all 19 tests pass.
- ESLint + Prettier clean on both changed files.

src/tools/performance.ts

@@ -61,57 +61,75 @@ export const startTrace = definePageTool({
     const page = request.page;
     const pageUrlForTracing = page.pptrPage.url();
 
-    if (request.params.reload) {
-      // Before starting the recording, navigate to about:blank to clear out any state.
-      // We use `load` because `networkidle0` is known to be flaky for about:blank in Puppeteer.
-      await page.pptrPage.goto('about:blank', {
-        waitUntil: 'load',
+    try {
+      if (request.params.reload) {
+        // Before starting the recording, navigate to about:blank to clear out any state.
+        // We use `load` because `networkidle0` is known to be flaky for about:blank in Puppeteer.
+        await page.pptrPage.goto('about:blank', {
+          waitUntil: 'load',
+        });
+      }
+
+      // Keep in sync with the categories arrays in:
+      // https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/timeline/TimelineController.ts
+      // https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/gatherers/trace.js
+      const categories = [
+        '-*',
+        'blink.console',
+        'blink.user_timing',
+        'devtools.timeline',
+        'disabled-by-default-devtools.screenshot',
+        'disabled-by-default-devtools.timeline',
+        'disabled-by-default-devtools.timeline.invalidationTracking',
+        'disabled-by-default-devtools.timeline.frame',
+        'disabled-by-default-devtools.timeline.stack',
+        'disabled-by-default-v8.cpu_profiler',
+        'disabled-by-default-v8.cpu_profiler.hires',
+        'latencyInfo',
+        'loading',
+        'disabled-by-default-lighthouse',
+        'v8.execute',
+        'v8',
+      ];
+      await page.pptrPage.tracing.start({
+        categories,
       });
-    }
 
-    // Keep in sync with the categories arrays in:
-    // https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/timeline/TimelineController.ts
-    // https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/gatherers/trace.js
-    const categories = [
-      '-*',
-      'blink.console',
-      'blink.user_timing',
-      'devtools.timeline',
-      'disabled-by-default-devtools.screenshot',
-      'disabled-by-default-devtools.timeline',
-      'disabled-by-default-devtools.timeline.invalidationTracking',
-      'disabled-by-default-devtools.timeline.frame',
-      'disabled-by-default-devtools.timeline.stack',
-      'disabled-by-default-v8.cpu_profiler',
-      'disabled-by-default-v8.cpu_profiler.hires',
-      'latencyInfo',
-      'loading',
-      'disabled-by-default-lighthouse',
-      'v8.execute',
-      'v8',
-    ];
-    await page.pptrPage.tracing.start({
-      categories,
-    });
+      if (request.params.reload) {
+        await page.pptrPage.goto(pageUrlForTracing, {
+          waitUntil: ['load'],
+        });
+      }
 
-    if (request.params.reload) {
-      await page.pptrPage.goto(pageUrlForTracing, {
-        waitUntil: ['load'],
-      });
-    }
-
-    if (request.params.autoStop) {
-      await new Promise(resolve => setTimeout(resolve, 5_000));
-      await stopTracingAndAppendOutput(
-        page,
-        response,
-        context,
-        request.params.filePath,
-      );
-    } else {
-      response.appendResponseLine(
-        `The performance trace is being recorded. Use performance_stop_trace to stop it.`,
-      );
+      if (request.params.autoStop) {
+        await new Promise(resolve => setTimeout(resolve, 5_000));
+        await stopTracingAndAppendOutput(
+          page,
+          response,
+          context,
+          request.params.filePath,
+        );
+      } else {
+        response.appendResponseLine(
+          `The performance trace is being recorded. Use performance_stop_trace to stop it.`,
+        );
+      }
+    } catch (error) {
+      // If a setup step (navigation, tracing.start) throws before
+      // stopTracingAndAppendOutput runs, the running flag would otherwise stay
+      // stuck `true` for the rest of the session, blocking all future traces.
+      // Unwind here: stop tracing if it was started and clear the flag. When
+      // autoStop already ran stopTracingAndAppendOutput the flag is already
+      // false and this is a no-op.
+      if (context.isRunningPerformanceTrace()) {
+        try {
+          await page.pptrPage.tracing.stop();
+        } catch {
+          // Tracing may not have started yet; ignore.
+        }
+        context.setIsRunningPerformanceTrace(false);
+      }
+      throw error;
     }
   },
 });

tests/tools/performance.test.ts

@@ -168,6 +168,47 @@ describe('performance', () => {
       });
     });
 
+    it('resets the running flag if a setup step throws', async () => {
+      await withMcpContext(async (response, context) => {
+        const selectedPage = context.getSelectedMcpPage().pptrPage;
+        sinon.stub(selectedPage, 'url').callsFake(() => 'https://www.test.com');
+        const gotoStub = sinon
+          .stub(selectedPage, 'goto')
+          .rejects(new Error('Navigation failed'));
+        const startTracingStub = sinon.stub(selectedPage.tracing, 'start');
+        sinon
+          .stub(selectedPage.tracing, 'stop')
+          .rejects(new Error('Cannot stop recording: tracing was not started'));
+        await assert.rejects(
+          startTrace.handler(
+            {
+              params: {reload: true, autoStop: true},
+              page: context.getSelectedMcpPage(),
+            },
+            response,
+            context,
+          ),
+          /Navigation failed/,
+        );
+        sinon.assert.notCalled(startTracingStub);
+        assert.strictEqual(context.isRunningPerformanceTrace(), false);
+
+        // A follow-up start_trace must proceed instead of reporting that a
+        // trace is already running.
+        gotoStub.resolves(null);
+        await startTrace.handler(
+          {
+            params: {reload: true, autoStop: false},
+            page: context.getSelectedMcpPage(),
+          },
+          response,
+          context,
+        );
+        sinon.assert.calledOnce(startTracingStub);
+        assert.ok(context.isRunningPerformanceTrace());
+      });
+    });
+
     it('supports filePath', async () => {
       const rawData = loadTraceAsBuffer('basic-trace.json.gz');
       // rawData is the decompressed buffer (based on loadTraceAsBuffer implementation).