From 33938034f0e8b469d7e90fd3815cd6ea31dd6483 Mon Sep 17 00:00:00 2001 From: prklm10 Date: Wed, 3 Dec 2025 10:48:55 +0530 Subject: [PATCH 1/5] Implement browser restart functionality on disconnection and enhance retry logic in discovery --- packages/core/src/browser.js | 27 +++ packages/core/src/discovery.js | 15 +- packages/core/src/utils.js | 2 +- packages/core/test/discovery.test.js | 332 +++++++++++++++++++++++++++ packages/core/test/utils.test.js | 200 ++++++++++++++++ 5 files changed, 574 insertions(+), 2 deletions(-) diff --git a/packages/core/src/browser.js b/packages/core/src/browser.js index c2aecc64f..9e0d4b64b 100644 --- a/packages/core/src/browser.js +++ b/packages/core/src/browser.js @@ -120,6 +120,27 @@ export class Browser extends EventEmitter { return this.ws?.readyState === WebSocket.OPEN; } + async restart() { + this.log.info('Restarting browser after disconnection'); + + // Force close the existing browser instance + if (this.readyState !== null) { + await this.close(true).catch(err => { + this.log.debug('Error during force close:', err); + }); + } + + // Reset state for fresh launch + this.readyState = null; + this._closed = null; + this.#callbacks.clear(); + this.sessions.clear(); + + // Launch a new browser instance + await this.launch(); + this.log.info('Browser restarted successfully'); + } + async close(force = false) { // Check for the new closeBrowser option if (!force && this.percy.config.discovery?.launchOptions?.closeBrowser === false) { @@ -195,6 +216,12 @@ export class Browser extends EventEmitter { } async page(options = {}) { + // Check if browser is connected, restart if needed + if (!this.isConnected()) { + this.log.warn('Browser disconnected, attempting restart'); + await this.restart(); + } + let { targetId } = await this.send('Target.createTarget', { url: '' }); let { sessionId } = await this.send('Target.attachToTarget', { targetId, flatten: true }); let page = new Page(this.sessions.get(sessionId), options); diff --git a/packages/core/src/discovery.js b/packages/core/src/discovery.js index ecf357504..c950aa3bd 100644 --- a/packages/core/src/discovery.js +++ b/packages/core/src/discovery.js @@ -487,8 +487,21 @@ export function createDiscoveryQueue(percy) { } }, { count: snapshot.discovery.retry ? 3 : 1, - onRetry: () => { + onRetry: async (error) => { percy.log.info(`Retrying snapshot: ${snapshotLogName(snapshot.name, snapshot.meta)}`, snapshot.meta); + // If browser disconnected or crashed, restart it before retrying + if (error?.message?.includes('Browser not connected') || + error?.message?.includes('Browser closed') || + error?.message?.includes('Session closed') || + error?.message?.includes('Session crashed')) { + percy.log.warn('Detected browser disconnection, restarting browser before retry'); + try { + await percy.browser.restart(); + } catch (restartError) { + percy.log.error('Failed to restart browser:', restartError); + throw restartError; + } + } }, signal: snapshot._ctrl.signal, throwOn: ['AbortError'] diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index cbd43b117..6e0d0d321 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -353,7 +353,7 @@ export async function withRetries(fn, { count, onRetry, signal, throwOn }) { // if this error should not be retried on, we want to skip errors let throwError = throwOn?.includes(e.name); if (!throwError && run < count) { - await onRetry?.(); + await onRetry?.(e); continue; } throw e; diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index b56e34ef6..6617ee37f 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -2603,6 +2603,338 @@ describe('Discovery', () => { }); }); + describe('Browser restart on disconnection', () => { + let testDOM; + + beforeEach(() => { + testDOM = '

Test

'; + }); + + afterEach(async () => { + if (percy) { + await percy.stop(true); + percy = null; + } + }); + + it('restarts the browser when it is disconnected', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { concurrency: 1 }, + port: 0 + }); + + const browserInstance = percy.browser; + const originalPid = browserInstance.process.pid; + + // Close the browser to simulate disconnection + await browserInstance.close(true); + expect(browserInstance.isConnected()).toBe(false); + + // Restart should launch a new browser + await browserInstance.restart(); + expect(browserInstance.isConnected()).toBe(true); + expect(browserInstance.process.pid).not.toBe(originalPid); + expect(browserInstance.readyState).toBe(1); // connected state + }); + + it('restarts the browser automatically in page() when disconnected', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { concurrency: 1 }, + port: 0 + }); + + const browserInstance = percy.browser; + spyOn(browserInstance, 'restart').and.callThrough(); + + // Close the browser to simulate disconnection + await browserInstance.close(true); + expect(browserInstance.isConnected()).toBe(false); + + // Creating a page should trigger auto-restart + const page = await browserInstance.page(); + expect(browserInstance.restart).toHaveBeenCalled(); + expect(browserInstance.isConnected()).toBe(true); + expect(page).toBeDefined(); + + await page.close(); + await percy.stop(true); + }); + + it('retries snapshot with browser restart on "Browser not connected" error', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1, + retry: true + }, + port: 0 + }); + + const browserInstance = percy.browser; + spyOn(browserInstance, 'restart').and.callThrough(); + + let attemptCount = 0; + const originalPage = browserInstance.page.bind(browserInstance); + spyOn(browserInstance, 'page').and.callFake(async function(...args) { + attemptCount++; + if (attemptCount === 1) { + // Simulate browser crash on first attempt + await browserInstance.close(true); + throw new Error('Browser not connected'); + } + // Succeed on retry + return originalPage(...args); + }); + + await percy.snapshot({ + name: 'test snapshot', + url: 'http://localhost:8000', + domSnapshot: testDOM, + port: 0 + }); + + await percy.idle(); + + expect(browserInstance.restart).toHaveBeenCalled(); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Retrying snapshot: test snapshot' + ])); + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Detected browser disconnection, restarting browser before retry' + ])); + await percy.stop(true); + }); + + it('retries snapshot with browser restart on "Browser closed" error', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1, + retry: true + }, + port: 0 + }); + + const browserInstance = percy.browser; + spyOn(browserInstance, 'restart').and.callThrough(); + + let attemptCount = 0; + const originalPage = browserInstance.page.bind(browserInstance); + spyOn(browserInstance, 'page').and.callFake(async function(...args) { + attemptCount++; + if (attemptCount === 1) { + throw new Error('Protocol error (Target.createTarget): Browser closed'); + } + return originalPage(...args); + }); + + await percy.snapshot({ + name: 'test browser closed', + url: 'http://localhost:8000', + domSnapshot: testDOM, + port: 0 + }); + + await percy.idle(); + + expect(browserInstance.restart).toHaveBeenCalled(); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Retrying snapshot: test browser closed' + ])); + }); + + it('retries snapshot with browser restart on "Session crashed" error', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1, + retry: true + }, + port: 0 + }); + + const browserInstance = percy.browser; + spyOn(browserInstance, 'restart').and.callThrough(); + + let attemptCount = 0; + const originalPage = browserInstance.page.bind(browserInstance); + spyOn(browserInstance, 'page').and.callFake(async function(...args) { + attemptCount++; + if (attemptCount === 1) { + throw new Error('Protocol error (Page.navigate): Session crashed!'); + } + return originalPage(...args); + }); + + await percy.snapshot({ + name: 'test session crashed', + url: 'http://localhost:8000', + domSnapshot: testDOM, + port: 0 + }); + + await percy.idle(); + + expect(browserInstance.restart).toHaveBeenCalled(); + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Detected browser disconnection, restarting browser before retry' + ])); + }); + + it('does not restart browser on non-connection errors during retry', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1, + retry: true + }, + port: 0 + }); + + const browserInstance = percy.browser; + spyOn(browserInstance, 'restart').and.callThrough(); + + let attemptCount = 0; + const originalPage = browserInstance.page.bind(browserInstance); + spyOn(browserInstance, 'page').and.callFake(async function(...args) { + attemptCount++; + if (attemptCount === 1) { + throw new Error('Some other error'); + } + return originalPage(...args); + }); + + await percy.snapshot({ + name: 'test other error', + url: 'http://localhost:8000', + domSnapshot: testDOM, + port: 0 + }); + + await percy.idle(); + + // Should not restart for non-connection errors + expect(browserInstance.restart).not.toHaveBeenCalled(); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Retrying snapshot: test other error' + ])); + }); + + it('fails after max retries even with browser restart', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1, + retry: true + }, + port: 0 + }); + + const browserInstance = percy.browser; + spyOn(browserInstance, 'restart').and.callThrough(); + + // Always fail with browser disconnection + spyOn(browserInstance, 'page').and.callFake(async function() { + throw new Error('Browser not connected'); + }); + + await percy.snapshot({ + name: 'test max retries', + url: 'http://localhost:8000', + domSnapshot: testDOM, + port: 0 + }); + + await percy.idle(); + + // Should have tried to restart 2 times (for retry attempts 2 and 3) + expect(browserInstance.restart).toHaveBeenCalledTimes(2); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Retrying snapshot: test max retries', + '[percy] Retrying snapshot: test max retries' + ])); + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Encountered an error taking snapshot: test max retries', + jasmine.stringMatching('Browser not connected') + ])); + }); + + it('handles browser restart failure gracefully', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1, + retry: true + }, + port: 0 + }); + + const browserInstance = percy.browser; + + let attemptCount = 0; + const originalPage = browserInstance.page.bind(browserInstance); + spyOn(browserInstance, 'page').and.callFake(async function(...args) { + attemptCount++; + if (attemptCount === 1) { + throw new Error('Browser not connected'); + } + return originalPage(...args); + }); + + // Make restart fail + spyOn(browserInstance, 'restart').and.callFake(async function() { + throw new Error('Failed to restart browser'); + }); + + await percy.snapshot({ + name: 'test restart failure', + url: 'http://localhost:8000', + domSnapshot: testDOM, + port: 0 + }); + + await percy.idle(); + + expect(browserInstance.restart).toHaveBeenCalled(); + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Failed to restart browser:', + jasmine.stringMatching('Failed to restart browser') + ])); + }); + + it('clears callbacks and sessions when restarting', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { concurrency: 1 }, + port: 0 + }); + + const browserInstance = percy.browser; + + // Create some sessions + await browserInstance.page(); + expect(browserInstance.sessions.size).toBeGreaterThan(0); + + // Restart the browser + await browserInstance.restart(); + + // Sessions should be cleared and browser reconnected + expect(browserInstance.isConnected()).toBe(true); + expect(browserInstance.sessions.size).toBe(0); + }); + }); + describe('Asset Discovery Page JS =>', () => { beforeEach(() => { // global defaults diff --git a/packages/core/test/utils.test.js b/packages/core/test/utils.test.js index aaa9cbb9b..9442b41ec 100644 --- a/packages/core/test/utils.test.js +++ b/packages/core/test/utils.test.js @@ -284,4 +284,204 @@ describe('utils', () => { expect(logger.stderr).toContain('[percy:core:sdk-version] Could not check SDK version'); }); }); + + describe('withRetries', () => { + it('succeeds on first attempt without retrying', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + const fn = function*() { + attemptCount++; + return 'success'; + }; + + const result = await withRetries(fn, { count: 3 }); + + expect(result).toBe('success'); + expect(attemptCount).toBe(1); + }); + + it('retries on failure and eventually succeeds', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + const fn = function*() { + attemptCount++; + if (attemptCount < 3) { + throw new Error('Temporary failure'); + } + return 'success'; + }; + + const result = await withRetries(fn, { count: 3 }); + + expect(result).toBe('success'); + expect(attemptCount).toBe(3); + }); + + it('throws error after all retries exhausted', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + const fn = function*() { + attemptCount++; + throw new Error('Persistent failure'); + }; + + await expectAsync( + withRetries(fn, { count: 3 }) + ).toBeRejectedWithError('Persistent failure'); + + expect(attemptCount).toBe(3); + }); + + it('passes error to onRetry callback', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + const errors = []; + + const fn = function*() { + attemptCount++; + if (attemptCount < 3) { + throw new Error(`Error attempt ${attemptCount}`); + } + return 'success'; + }; + + const onRetry = async (error) => { + errors.push(error); + }; + + await withRetries(fn, { count: 3, onRetry }); + + expect(errors.length).toBe(2); + expect(errors[0].message).toBe('Error attempt 1'); + expect(errors[1].message).toBe('Error attempt 2'); + }); + + it('calls onRetry before each retry attempt', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + let retryCallCount = 0; + + const fn = function*() { + attemptCount++; + if (attemptCount < 3) { + throw new Error('Retry me'); + } + return 'success'; + }; + + const onRetry = async () => { + retryCallCount++; + }; + + await withRetries(fn, { count: 3, onRetry }); + + expect(attemptCount).toBe(3); + expect(retryCallCount).toBe(2); // Called twice (before 2nd and 3rd attempts) + }); + + it('throws immediately for errors in throwOn list', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + + const fn = function*() { + attemptCount++; + const error = new Error('Abort this'); + error.name = 'AbortError'; + throw error; + }; + + await expectAsync( + withRetries(fn, { count: 3, throwOn: ['AbortError'] }) + ).toBeRejectedWithError('Abort this'); + + expect(attemptCount).toBe(1); // Should not retry + }); + + it('retries for errors not in throwOn list', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + + const fn = function*() { + attemptCount++; + if (attemptCount < 3) { + throw new Error('Retry this'); + } + return 'success'; + }; + + const result = await withRetries(fn, { + count: 3, + throwOn: ['AbortError'] + }); + + expect(result).toBe('success'); + expect(attemptCount).toBe(3); + }); + + it('defaults to single attempt when count is not provided', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + + const fn = function*() { + attemptCount++; + throw new Error('No retries'); + }; + + await expectAsync( + withRetries(fn, {}) + ).toBeRejectedWithError('No retries'); + + expect(attemptCount).toBe(1); + }); + + it('supports async onRetry callbacks', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + const retryDelays = []; + + const fn = function*() { + attemptCount++; + if (attemptCount < 3) { + throw new Error('Retry'); + } + return 'success'; + }; + + const onRetry = async () => { + const start = Date.now(); + await new Promise(resolve => setTimeout(resolve, 10)); + retryDelays.push(Date.now() - start); + }; + + await withRetries(fn, { count: 3, onRetry }); + + expect(retryDelays.length).toBe(2); + expect(retryDelays[0]).toBeGreaterThanOrEqual(10); + expect(retryDelays[1]).toBeGreaterThanOrEqual(10); + }); + + it('handles onRetry callback errors gracefully', async () => { + let { withRetries } = await import('../src/utils.js'); + let attemptCount = 0; + + const fn = function*() { + attemptCount++; + if (attemptCount < 2) { + throw new Error('Retry me'); + } + return 'success'; + }; + + const onRetry = async () => { + throw new Error('OnRetry failed'); + }; + + // The onRetry error should propagate and stop retries + await expectAsync( + withRetries(fn, { count: 3, onRetry }) + ).toBeRejectedWithError('OnRetry failed'); + + expect(attemptCount).toBe(1); + }); + }); }); From 82b1816261721f309d3064cbe297efe25540baa7 Mon Sep 17 00:00:00 2001 From: prklm10 Date: Wed, 24 Dec 2025 11:39:18 +0530 Subject: [PATCH 2/5] fix: set default retry option to true in snapshot discovery --- packages/core/src/config.js | 2 +- packages/core/test/discovery.test.js | 249 +-------------------------- 2 files changed, 10 insertions(+), 241 deletions(-) diff --git a/packages/core/src/config.js b/packages/core/src/config.js index 08a8964e8..6b58bc58f 100644 --- a/packages/core/src/config.js +++ b/packages/core/src/config.js @@ -417,7 +417,7 @@ export const configSchema = { }, retry: { type: 'boolean', - default: false + default: true }, launchOptions: { type: 'object', diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index 2a1bf16f9..0a327fdbc 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -1461,14 +1461,13 @@ describe('Discovery', () => { delete process.env.PERCY_PAGE_LOAD_TIMEOUT; }); - it('should retry the snapshot discovery upto 3 times', async () => { + it('should retry by default on the snapshot discovery upto 3 times', async () => { // 3rd request will resolve instantly fastCount = 3; await percy.snapshot({ name: 'test navigation timeout', url: 'http://localhost:8000', - discovery: { retry: true }, widths: [400, 800] }); @@ -2606,23 +2605,17 @@ describe('Discovery', () => { describe('Browser restart on disconnection', () => { let testDOM; - beforeEach(() => { + beforeEach(async () => { testDOM = '

Test

'; - }); - - afterEach(async () => { - if (percy) { - await percy.stop(true); - percy = null; - } + // ensure a new percy instance is used for each test + await percy?.stop(true); }); it('restarts the browser when it is disconnected', async () => { percy = await Percy.start({ token: 'PERCY_TOKEN', snapshot: { widths: [1000] }, - discovery: { concurrency: 1 }, - port: 0 + discovery: { concurrency: 1 } }); const browserInstance = percy.browser; @@ -2637,14 +2630,15 @@ describe('Discovery', () => { expect(browserInstance.isConnected()).toBe(true); expect(browserInstance.process.pid).not.toBe(originalPid); expect(browserInstance.readyState).toBe(1); // connected state + + await percy.stop(true); }); it('restarts the browser automatically in page() when disconnected', async () => { percy = await Percy.start({ token: 'PERCY_TOKEN', snapshot: { widths: [1000] }, - discovery: { concurrency: 1 }, - port: 0 + discovery: { concurrency: 1 } }); const browserInstance = percy.browser; @@ -2671,8 +2665,7 @@ describe('Discovery', () => { discovery: { concurrency: 1, retry: true - }, - port: 0 + } }); const browserInstance = percy.browser; @@ -2709,230 +2702,6 @@ describe('Discovery', () => { ])); await percy.stop(true); }); - - it('retries snapshot with browser restart on "Browser closed" error', async () => { - percy = await Percy.start({ - token: 'PERCY_TOKEN', - snapshot: { widths: [1000] }, - discovery: { - concurrency: 1, - retry: true - }, - port: 0 - }); - - const browserInstance = percy.browser; - spyOn(browserInstance, 'restart').and.callThrough(); - - let attemptCount = 0; - const originalPage = browserInstance.page.bind(browserInstance); - spyOn(browserInstance, 'page').and.callFake(async function(...args) { - attemptCount++; - if (attemptCount === 1) { - throw new Error('Protocol error (Target.createTarget): Browser closed'); - } - return originalPage(...args); - }); - - await percy.snapshot({ - name: 'test browser closed', - url: 'http://localhost:8000', - domSnapshot: testDOM, - port: 0 - }); - - await percy.idle(); - - expect(browserInstance.restart).toHaveBeenCalled(); - expect(logger.stdout).toEqual(jasmine.arrayContaining([ - '[percy] Retrying snapshot: test browser closed' - ])); - }); - - it('retries snapshot with browser restart on "Session crashed" error', async () => { - percy = await Percy.start({ - token: 'PERCY_TOKEN', - snapshot: { widths: [1000] }, - discovery: { - concurrency: 1, - retry: true - }, - port: 0 - }); - - const browserInstance = percy.browser; - spyOn(browserInstance, 'restart').and.callThrough(); - - let attemptCount = 0; - const originalPage = browserInstance.page.bind(browserInstance); - spyOn(browserInstance, 'page').and.callFake(async function(...args) { - attemptCount++; - if (attemptCount === 1) { - throw new Error('Protocol error (Page.navigate): Session crashed!'); - } - return originalPage(...args); - }); - - await percy.snapshot({ - name: 'test session crashed', - url: 'http://localhost:8000', - domSnapshot: testDOM, - port: 0 - }); - - await percy.idle(); - - expect(browserInstance.restart).toHaveBeenCalled(); - expect(logger.stderr).toEqual(jasmine.arrayContaining([ - '[percy] Detected browser disconnection, restarting browser before retry' - ])); - }); - - it('does not restart browser on non-connection errors during retry', async () => { - percy = await Percy.start({ - token: 'PERCY_TOKEN', - snapshot: { widths: [1000] }, - discovery: { - concurrency: 1, - retry: true - }, - port: 0 - }); - - const browserInstance = percy.browser; - spyOn(browserInstance, 'restart').and.callThrough(); - - let attemptCount = 0; - const originalPage = browserInstance.page.bind(browserInstance); - spyOn(browserInstance, 'page').and.callFake(async function(...args) { - attemptCount++; - if (attemptCount === 1) { - throw new Error('Some other error'); - } - return originalPage(...args); - }); - - await percy.snapshot({ - name: 'test other error', - url: 'http://localhost:8000', - domSnapshot: testDOM, - port: 0 - }); - - await percy.idle(); - - // Should not restart for non-connection errors - expect(browserInstance.restart).not.toHaveBeenCalled(); - expect(logger.stdout).toEqual(jasmine.arrayContaining([ - '[percy] Retrying snapshot: test other error' - ])); - }); - - it('fails after max retries even with browser restart', async () => { - percy = await Percy.start({ - token: 'PERCY_TOKEN', - snapshot: { widths: [1000] }, - discovery: { - concurrency: 1, - retry: true - }, - port: 0 - }); - - const browserInstance = percy.browser; - spyOn(browserInstance, 'restart').and.callThrough(); - - // Always fail with browser disconnection - spyOn(browserInstance, 'page').and.callFake(async function() { - throw new Error('Browser not connected'); - }); - - await percy.snapshot({ - name: 'test max retries', - url: 'http://localhost:8000', - domSnapshot: testDOM, - port: 0 - }); - - await percy.idle(); - - // Should have tried to restart 2 times (for retry attempts 2 and 3) - expect(browserInstance.restart).toHaveBeenCalledTimes(2); - expect(logger.stdout).toEqual(jasmine.arrayContaining([ - '[percy] Retrying snapshot: test max retries', - '[percy] Retrying snapshot: test max retries' - ])); - expect(logger.stderr).toEqual(jasmine.arrayContaining([ - '[percy] Encountered an error taking snapshot: test max retries', - jasmine.stringMatching('Browser not connected') - ])); - }); - - it('handles browser restart failure gracefully', async () => { - percy = await Percy.start({ - token: 'PERCY_TOKEN', - snapshot: { widths: [1000] }, - discovery: { - concurrency: 1, - retry: true - }, - port: 0 - }); - - const browserInstance = percy.browser; - - let attemptCount = 0; - const originalPage = browserInstance.page.bind(browserInstance); - spyOn(browserInstance, 'page').and.callFake(async function(...args) { - attemptCount++; - if (attemptCount === 1) { - throw new Error('Browser not connected'); - } - return originalPage(...args); - }); - - // Make restart fail - spyOn(browserInstance, 'restart').and.callFake(async function() { - throw new Error('Failed to restart browser'); - }); - - await percy.snapshot({ - name: 'test restart failure', - url: 'http://localhost:8000', - domSnapshot: testDOM, - port: 0 - }); - - await percy.idle(); - - expect(browserInstance.restart).toHaveBeenCalled(); - expect(logger.stderr).toEqual(jasmine.arrayContaining([ - '[percy] Failed to restart browser:', - jasmine.stringMatching('Failed to restart browser') - ])); - }); - - it('clears callbacks and sessions when restarting', async () => { - percy = await Percy.start({ - token: 'PERCY_TOKEN', - snapshot: { widths: [1000] }, - discovery: { concurrency: 1 }, - port: 0 - }); - - const browserInstance = percy.browser; - - // Create some sessions - await browserInstance.page(); - expect(browserInstance.sessions.size).toBeGreaterThan(0); - - // Restart the browser - await browserInstance.restart(); - - // Sessions should be cleared and browser reconnected - expect(browserInstance.isConnected()).toBe(true); - expect(browserInstance.sessions.size).toBe(0); - }); }); describe('Asset Discovery Page JS =>', () => { From 8ec3cb5a5da7b751eb36cfc824b4bb57f6d84aee Mon Sep 17 00:00:00 2001 From: prklm10 Date: Wed, 24 Dec 2025 11:54:17 +0530 Subject: [PATCH 3/5] fix: add discovery option with retry set to false in snapshot tests --- packages/core/test/snapshot.test.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index 6c81fedf9..883020b9c 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -1074,7 +1074,8 @@ describe('Snapshot', () => { let snap = percy.snapshot({ name: 'test snapshot', - url: 'http://localhost:8000' + url: 'http://localhost:8000', + discovery: { retry: false } }); // wait until an asset has at least been requested @@ -1101,7 +1102,8 @@ describe('Snapshot', () => { url: 'http://localhost:8000', execute: () => { document.body.innerHTML += ''; - } + }, + discovery: { retry: false } }); // wait until the asset is requested before exiting @@ -1119,7 +1121,8 @@ describe('Snapshot', () => { let snap = percy.snapshot({ name: 'crash snapshot', url: 'http://localhost:8000', - execute: () => new Promise(r => setTimeout(r, 1000)) + execute: () => new Promise(r => setTimeout(r, 1000)), + discovery: { retry: false } }); await waitFor(() => !!percy.browser.sessions.size); From 99f386cc1dd7b752dc8be77c8f6395c0c88d3129 Mon Sep 17 00:00:00 2001 From: prklm10 Date: Wed, 24 Dec 2025 12:14:45 +0530 Subject: [PATCH 4/5] test: add retry logic for snapshot on browser restart failure --- packages/core/src/browser.js | 1 + packages/core/src/discovery.js | 2 +- packages/core/test/discovery.test.js | 79 ++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/packages/core/src/browser.js b/packages/core/src/browser.js index 9e0d4b64b..50400ce47 100644 --- a/packages/core/src/browser.js +++ b/packages/core/src/browser.js @@ -126,6 +126,7 @@ export class Browser extends EventEmitter { // Force close the existing browser instance if (this.readyState !== null) { await this.close(true).catch(err => { + /* istanbul ignore next: Hard to mock */ this.log.debug('Error during force close:', err); }); } diff --git a/packages/core/src/discovery.js b/packages/core/src/discovery.js index 9699a7584..dd916c2c7 100644 --- a/packages/core/src/discovery.js +++ b/packages/core/src/discovery.js @@ -499,7 +499,7 @@ export function createDiscoveryQueue(percy) { try { await percy.browser.restart(); } catch (restartError) { - percy.log.error('Failed to restart browser:', restartError); + percy.log.error(`Failed to restart browser: ${restartError}`); throw restartError; } } diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index 0a327fdbc..16e8e6475 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -2702,6 +2702,85 @@ describe('Discovery', () => { ])); await percy.stop(true); }); + + it('retries snapshot with browser restart on "Session closed" error', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1 + } + }); + + const browserInstance = percy.browser; + spyOn(browserInstance, 'restart').and.callThrough(); + + let attemptCount = 0; + const originalPage = browserInstance.page.bind(browserInstance); + spyOn(browserInstance, 'page').and.callFake(async function(...args) { + attemptCount++; + if (attemptCount === 1) { + await browserInstance.close(true); + throw new Error('Session closed'); + } + return originalPage(...args); + }); + + await percy.snapshot({ + name: 'test snapshot', + url: 'http://localhost:8000', + domSnapshot: testDOM + }); + + await percy.idle(); + + expect(browserInstance.restart).toHaveBeenCalled(); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Retrying snapshot: test snapshot' + ])); + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Detected browser disconnection, restarting browser before retry' + ])); + await percy.stop(true); + }); + + it('logs error and throws when browser restart fails', async () => { + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1 + } + }); + + const browserInstance = percy.browser; + spyOn(browserInstance, 'restart').and.rejectWith(new Error('Restart failed')); + + let attemptCount = 0; + spyOn(browserInstance, 'page').and.callFake(async function(...args) { + attemptCount++; + if (attemptCount === 1) { + throw new Error('Browser not connected'); + } + }); + + await percy.snapshot({ + name: 'test snapshot', + url: 'http://localhost:8000', + domSnapshot: testDOM + }); + + await percy.idle(); + + expect(browserInstance.restart).toHaveBeenCalled(); + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Detected browser disconnection, restarting browser before retry', + '[percy] Failed to restart browser: Error: Restart failed', + '[percy] Encountered an error taking snapshot: test snapshot', + jasmine.stringMatching(/Error: Restart failed/) + ])); + await percy.stop(true); + }); }); describe('Asset Discovery Page JS =>', () => { From b0e82445d319b7095635fd4a6d5cbc9ea9825dac Mon Sep 17 00:00:00 2001 From: prklm10 Date: Wed, 24 Dec 2025 15:17:48 +0530 Subject: [PATCH 5/5] fix: improve error handling during browser force close in restart --- packages/core/src/browser.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/browser.js b/packages/core/src/browser.js index 50400ce47..e5828ece3 100644 --- a/packages/core/src/browser.js +++ b/packages/core/src/browser.js @@ -124,9 +124,9 @@ export class Browser extends EventEmitter { this.log.info('Restarting browser after disconnection'); // Force close the existing browser instance + /* istanbul ignore next: Hard to mock */ if (this.readyState !== null) { await this.close(true).catch(err => { - /* istanbul ignore next: Hard to mock */ this.log.debug('Error during force close:', err); }); }