From 6e62a11643ed5b8adcea9e14895f4128fcb9f257 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Thu, 19 Oct 2023 11:14:17 -0700
Subject: [PATCH] fix(trace): EPERM on windows (#27693)

When merging trace files, we sometimes left open read streams from the
zip, which prevents it from being removed.

Fixes #27286.
---
 .../dispatchers/localUtilsDispatcher.ts       |  2 +-
 packages/playwright/src/worker/testTracing.ts | 13 ++++--
 .../playwright-test/playwright.trace.spec.ts  | 46 +++++++++++++++++++
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/packages/playwright-core/src/server/dispatchers/localUtilsDispatcher.ts b/packages/playwright-core/src/server/dispatchers/localUtilsDispatcher.ts
index 16b863707b..c90110a460 100644
--- a/packages/playwright-core/src/server/dispatchers/localUtilsDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/localUtilsDispatcher.ts
@@ -139,7 +139,7 @@ export class LocalUtilsDispatcher extends Dispatcher<{ guid: string }, channels.
               zipFile.outputStream.pipe(fs.createWriteStream(params.zipFile)).on('close', () => {
                 fs.promises.unlink(tempFile).then(() => {
                   promise.resolve();
-                });
+                }).catch(error => promise.reject(error));
               });
             });
           }
diff --git a/packages/playwright/src/worker/testTracing.ts b/packages/playwright/src/worker/testTracing.ts
index c6d8007d1e..270c29468b 100644
--- a/packages/playwright/src/worker/testTracing.ts
+++ b/packages/playwright/src/worker/testTracing.ts
@@ -204,15 +204,18 @@ export async function mergeTraceFiles(fileName: string, temporaryTraceFiles: str
         } else if (entry.fileName.match(/[\d-]*trace\./)) {
           entryName = i + '-' + entry.fileName;
         }
+        if (entryNames.has(entryName)) {
+          if (--pendingEntries === 0)
+            promise.resolve();
+          return;
+        }
+        entryNames.add(entryName);
         inZipFile.openReadStream(entry, (err, readStream) => {
           if (err) {
             promise.reject(err);
             return;
           }
-          if (!entryNames.has(entryName)) {
-            entryNames.add(entryName);
-            zipFile.addReadStream(readStream!, entryName);
-          }
+          zipFile.addReadStream(readStream!, entryName);
           if (--pendingEntries === 0)
             promise.resolve();
         });
@@ -225,7 +228,7 @@ export async function mergeTraceFiles(fileName: string, temporaryTraceFiles: str
     zipFile.outputStream.pipe(fs.createWriteStream(fileName)).on('close', () => {
       void Promise.all(temporaryTraceFiles.map(tempFile => fs.promises.unlink(tempFile))).then(() => {
         mergePromise.resolve();
-      });
+      }).catch(error => mergePromise.reject(error));
     }).on('error', error => mergePromise.reject(error));
   });
   await mergePromise;
diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts
index c9b874aad6..5ac1c1fb31 100644
--- a/tests/playwright-test/playwright.trace.spec.ts
+++ b/tests/playwright-test/playwright.trace.spec.ts
@@ -753,3 +753,49 @@ test('should use custom expect message in trace', async ({ runInlineTest }, test
     '  fixture: context',
   ]);
 });
+
+test('should not throw when merging traces multiple times', async ({ runInlineTest }, testInfo) => {
+  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/27286' });
+
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = { use: { trace: 'on' } };
+    `,
+    'a.spec.ts': `
+      import { BrowserContext, expect, Page, test as baseTest } from '@playwright/test';
+
+      let pg: Page;
+      let ctx: BrowserContext;
+
+      const test = baseTest.extend({
+        page: async ({}, use) => {
+          await use(pg);
+        },
+        context: async ({}, use) => {
+          await use(ctx);
+        },
+      });
+
+      test.beforeAll(async ({ browser }) => {
+        ctx = await browser.newContext();
+        pg = await ctx.newPage();
+      });
+
+      test.beforeAll(async ({ page }) => {
+        await page.goto('https://playwright.dev');
+      });
+
+      test.afterAll(async ({ context }) => {
+        await context.close();
+      });
+
+      test('foo', async ({ page }) => {
+        await expect(page.locator('h1')).toContainText('Playwright');
+      });
+    `,
+  }, { workers: 1 });
+
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+  expect(fs.existsSync(testInfo.outputPath('test-results', 'a-foo', 'trace.zip'))).toBe(true);
+});