fix(tracing): allow restarting tracing after a failed stop

packages/playwright-core/src/client/tracing.ts

@@ -95,8 +95,10 @@ export class Tracing extends ChannelOwner<channels.TracingChannel> implements ap
 
   async stop(options: { path?: string } = {}) {
     await this._wrapApiCall(async () => {
-      await this._doStopChunk(options.path);
+      const error = await this._doStopChunk(options.path).catch(e => e);
       await this._channel.tracingStop({}, kNoTimeout);
+      if (error)
+        throw error;
     });
   }
 
@@ -192,12 +194,25 @@ export class Tracing extends ChannelOwner<channels.TracingChannel> implements ap
 
     const additionalSources = [...this._additionalSources];
     this._additionalSources.clear();
+    const stacksId = this._stacksId;
+    this._stacksId = undefined;
 
+    try {
+      await this._saveChunk(filePath, stacksId, additionalSources);
+    } catch (error) {
+      // Release the stack session even on failure, otherwise later traces keep appending to it.
+      if (stacksId)
+        await this._connection.localUtils()?.traceDiscarded({ stacksId }).catch(() => {});
+      throw error;
+    }
+  }
+
+  private async _saveChunk(filePath: string | undefined, stacksId: string | undefined, additionalSources: string[]) {
     if (!filePath) {
       // Not interested in artifacts.
       await this._channel.tracingStopChunk({ mode: 'discard' }, kNoTimeout);
-      if (this._stacksId)
-        await this._connection.localUtils()!.traceDiscarded({ stacksId: this._stacksId });
+      if (stacksId)
+        await this._connection.localUtils()!.traceDiscarded({ stacksId });
       return;
     }
 
@@ -209,25 +224,31 @@ export class Tracing extends ChannelOwner<channels.TracingChannel> implements ap
 
     if (isLocal) {
       const result = await this._channel.tracingStopChunk({ mode: 'entries' }, kNoTimeout);
-      await localUtils.zip({ zipFile: filePath, entries: result.entries!, mode: 'write', stacksId: this._stacksId, includeSources: this._includeSources, additionalSources });
+      await localUtils.zip({ zipFile: filePath, entries: result.entries!, mode: 'write', stacksId, includeSources: this._includeSources, additionalSources });
       return;
     }
 
     const result = await this._channel.tracingStopChunk({ mode: 'archive' }, kNoTimeout);
 
     // The artifact may be missing if the browser closed while stopping tracing.
     if (!result.artifact) {
-      if (this._stacksId)
-        await localUtils.traceDiscarded({ stacksId: this._stacksId });
+      if (stacksId)
+        await localUtils.traceDiscarded({ stacksId });
       return;
     }
 
     // Save trace to the final local file.
     const artifact = Artifact.from(result.artifact);
-    await artifact.saveAs(filePath);
+    try {
+      await artifact.saveAs(filePath);
+    } catch (error) {
+      // Delete the artifact best-effort, the save error is the one to surface.
+      await artifact.delete().catch(() => {});
+      throw error;
+    }
     await artifact.delete();
 
-    await localUtils.zip({ zipFile: filePath, entries: [], mode: 'append', stacksId: this._stacksId, includeSources: this._includeSources, additionalSources });
+    await localUtils.zip({ zipFile: filePath, entries: [], mode: 'append', stacksId, includeSources: this._includeSources, additionalSources });
   }
 
   _resetStackCounter() {

packages/playwright-core/src/server/trace/recorder/tracing.ts

@@ -139,9 +139,7 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
   }
 
   async resetForReuse(progress: Progress) {
-    // Discard previous chunk if any and ignore any errors there.
-    await this.stopChunk(progress, { mode: 'discard' }).catch(() => {});
-    await progress.race(this._stop());
+    await this.stop(progress);
     if (this._snapshotter)
       await progress.race(this._snapshotter.resetForReuse());
   }
@@ -327,6 +325,8 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
   }
 
   async stop(progress: Progress) {
+    // Discard the chunk if the client failed to stop it, so that tracing can be restarted.
+    await this.stopChunk(progress, { mode: 'discard' }).catch(() => {});
     await progress.race(this._stop());
   }
 
@@ -392,12 +392,41 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
     if (this._isStopping)
       throw new Error(`Tracing is already stopping`);
     this._isStopping = true;
+    try {
+      const result = this._stopChunk(params);
+      if (!result)
+        return {};
 
-    if (!this._state || !this._state.recording) {
+      // Make sure all file operations complete.
+      try {
+        await progress.race(this._fs.sync());
+      } catch (error) {
+        // This check is here because closing the browser removes the tracesDir and tracing
+        // cannot access removed files. Clients are ready for the missing artifact.
+        if (!isAbortError(error) && this._context.attribution.browser && !this._context.attribution.browser.isConnected())
+          return {};
+        throw error;
+      }
+
+      if (params.mode === 'entries')
+        return { entries: result.entries };
+
+      const artifact = new Artifact(this._context, result.zipFileName);
+      artifact.reportFinished();
+      return { artifact };
+    } finally {
+      // Always release the recording state, even when saving the chunk failed.
       this._isStopping = false;
+      if (this._state)
+        this._state.recording = false;
+    }
+  }
+
+  private _stopChunk(params: TracingTracingStopChunkParams): { entries: NameValue[], zipFileName: string } | undefined {
+    if (!this._state || !this._state.recording) {
       if (params.mode !== 'discard')
         throw new Error(`Must start tracing before stopping`);
-      return {};
+      return undefined;
     }
 
     this._closeAllGroups();
@@ -430,46 +459,16 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
 
     this._state.chunkFiles = new Set();
 
-    if (params.mode === 'discard') {
-      this._isStopping = false;
-      this._state.recording = false;
-      return {};
-    }
+    if (params.mode === 'discard')
+      return undefined;
 
     this._fs.copyFile(this._state.networkFile, newNetworkFile);
 
     const zipFileName = this._state.traceFile + '.zip';
     if (params.mode === 'archive')
       this._fs.zip(entries, zipFileName);
 
-    // Make sure all file operations complete.
-    let error: Error | undefined;
-    try {
-      await progress.race(this._fs.sync());
-    } catch (e) {
-      error = e as Error;
-    }
-
-    this._isStopping = false;
-    if (this._state)
-      this._state.recording = false;
-
-    // IMPORTANT: no awaits after this point, to make sure recording state is correct.
-
-    if (error) {
-      // This check is here because closing the browser removes the tracesDir and tracing
-      // cannot access removed files. Clients are ready for the missing artifact.
-      if (!isAbortError(error) && this._context.attribution.browser && !this._context.attribution.browser.isConnected())
-        return {};
-      throw error;
-    }
-
-    if (params.mode === 'entries')
-      return { entries };
-
-    const artifact = new Artifact(this._context, zipFileName);
-    artifact.reportFinished();
-    return { artifact };
+    return { entries, zipFileName };
   }
 
   private async _captureSnapshot(progress: Progress, sdkObject: SdkObject, phase: trace.ActionPhase): Promise<void> {

tests/library/tracing.spec.ts

@@ -421,6 +421,44 @@ test('should record network failures', async ({ context, page, server }, testInf
   expect(requestEvent.snapshot.time).toBeGreaterThanOrEqual(0);
 });
 
+test('should recover tracing after a failed stop', async ({ context, page, server }, testInfo) => {
+  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/42423' });
+  await context.tracing.start();
+  // Saving fails: a parent of the destination is a file, not a directory.
+  const blocker = testInfo.outputPath('blocker');
+  await fs.promises.writeFile(blocker, '');
+  await expect(context.tracing.stop({ path: path.join(blocker, 'trace1.zip') })).rejects.toThrow(/ENOTDIR|ENOENT|EEXIST/);
+
+  // The failed stop must not wedge tracing for the rest of the context lifetime.
+  await context.tracing.start();
+  await page.goto(server.PREFIX + '/input/button.html');
+  await page.click('button');
+  await context.tracing.stop({ path: testInfo.outputPath('trace2.zip') });
+
+  const { events, actions } = await parseTraceRaw(testInfo.outputPath('trace2.zip'));
+  expect(events[0].type).toBe('context-options');
+  expect(actions).toContain(`Click locator('button')`);
+});
+
+test('should release the stack session when saving the trace fails', async ({ browserType }, testInfo) => {
+  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/42423' });
+  // Override the test runner's tracesDir, so that the stack session owns a temporary directory.
+  const browser = await browserType.launch({ tracesDir: undefined });
+  try {
+    const context = await browser.newContext();
+    await context.tracing.start();
+    const stacksDir = path.dirname((context.tracing as any)._stacksId);
+    expect(fs.existsSync(stacksDir)).toBe(true);
+
+    const blocker = testInfo.outputPath('blocker');
+    await fs.promises.writeFile(blocker, '');
+    await expect(context.tracing.stop({ path: path.join(blocker, 'trace.zip') })).rejects.toThrow(/ENOTDIR|ENOENT|EEXIST/);
+    expect(fs.existsSync(stacksDir)).toBe(false);
+  } finally {
+    await browser.close();
+  }
+});
+
 test('should not crash when browser closes mid-trace', async ({ browserType, server }, testInfo) => {
   const browser = await browserType.launch();
   const page = await browser.newPage();