diff --git a/packages/playwright-core/src/client/tracing.ts b/packages/playwright-core/src/client/tracing.ts index b0257c5258191..4f403f5679a1c 100644 --- a/packages/playwright-core/src/client/tracing.ts +++ b/packages/playwright-core/src/client/tracing.ts @@ -95,8 +95,10 @@ export class Tracing extends ChannelOwner 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 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,7 +224,7 @@ export class Tracing extends ChannelOwner 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; } @@ -217,17 +232,23 @@ export class Tracing extends ChannelOwner implements ap // 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() { diff --git a/packages/playwright-core/src/server/trace/recorder/tracing.ts b/packages/playwright-core/src/server/trace/recorder/tracing.ts index b1dbca080ecb6..4c9b282782c24 100644 --- a/packages/playwright-core/src/server/trace/recorder/tracing.ts +++ b/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,11 +459,8 @@ 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); @@ -442,34 +468,7 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps 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 { diff --git a/tests/library/tracing.spec.ts b/tests/library/tracing.spec.ts index 53ea505d735aa..1af0215d244e6 100644 --- a/tests/library/tracing.spec.ts +++ b/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();