diff --git a/package-lock.json b/package-lock.json index 09bcccd..c194959 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "snapapi", - "version": "0.7.0", + "version": "0.8.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "snapapi", - "version": "0.7.0", + "version": "0.8.0", "dependencies": { "compression": "^1.8.1", "express": "^4.21.0", diff --git a/src/routes/screenshot.ts b/src/routes/screenshot.ts index 0461782..2bd2f44 100644 --- a/src/routes/screenshot.ts +++ b/src/routes/screenshot.ts @@ -591,6 +591,7 @@ async function handleScreenshotRequest(req: any, res: any) { res.setHeader("Content-Length", result.buffer.length); res.setHeader("Cache-Control", "no-store"); res.setHeader("X-Cache", "MISS"); + res.setHeader("X-Retry-Count", String(result.retryCount ?? 0)); res.send(result.buffer); } catch (err: any) { logger.error({ err: err.message, url }, "Screenshot failed"); diff --git a/src/services/__tests__/retry.test.ts b/src/services/__tests__/retry.test.ts new file mode 100644 index 0000000..e2ea6b1 --- /dev/null +++ b/src/services/__tests__/retry.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect } from 'vitest' +import { isRetryableError } from '../retry.js' + +describe('isRetryableError', () => { + it('returns true for TimeoutError', () => { + const err = new Error('TimeoutError: Navigation timeout of 20000ms exceeded') + err.name = 'TimeoutError' + expect(isRetryableError(err)).toBe(true) + }) + + it('returns true for Protocol error', () => { + expect(isRetryableError(new Error('Protocol error (Runtime.callFunctionOn): Session closed.'))).toBe(true) + }) + + it('returns true for Target closed', () => { + expect(isRetryableError(new Error('Target closed'))).toBe(true) + }) + + it('returns true for Session closed', () => { + expect(isRetryableError(new Error('Session closed. Most likely the page has been closed.'))).toBe(true) + }) + + it('returns true for Navigation failed', () => { + expect(isRetryableError(new Error('Navigation failed because browser has disconnected!'))).toBe(true) + }) + + it('returns true for net::ERR_ errors', () => { + expect(isRetryableError(new Error('net::ERR_CONNECTION_RESET'))).toBe(true) + expect(isRetryableError(new Error('net::ERR_CONNECTION_REFUSED'))).toBe(true) + }) + + it('returns false for SCREENSHOT_TIMEOUT (overall timeout, not transient)', () => { + expect(isRetryableError(new Error('SCREENSHOT_TIMEOUT'))).toBe(false) + }) + + it('returns false for SSRF_BLOCKED', () => { + expect(isRetryableError(new Error('SSRF_BLOCKED'))).toBe(false) + }) + + it('returns false for validation errors', () => { + expect(isRetryableError(new Error('hideSelector contains dangerous characters'))).toBe(false) + expect(isRetryableError(new Error('selector and fullPage are mutually exclusive'))).toBe(false) + }) + + it('returns false for SELECTOR_NOT_FOUND', () => { + expect(isRetryableError(new Error('SELECTOR_NOT_FOUND'))).toBe(false) + }) + + it('returns false for JS_EXECUTION_ERROR', () => { + expect(isRetryableError(new Error('JS_EXECUTION_ERROR: foo is not defined'))).toBe(false) + }) + + it('returns false for generic errors', () => { + expect(isRetryableError(new Error('Something went wrong'))).toBe(false) + }) +}) diff --git a/src/services/__tests__/screenshot.test.ts b/src/services/__tests__/screenshot.test.ts index 2e7b77b..393a18a 100644 --- a/src/services/__tests__/screenshot.test.ts +++ b/src/services/__tests__/screenshot.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' // All mocks must use inline values since vi.mock is hoisted vi.mock('../browser.js', () => ({ @@ -590,4 +590,79 @@ describe('Screenshot Service', () => { expect(mockPage.$).not.toHaveBeenCalled() }) }) + + describe('Retry logic', () => { + it('returns retryCount 0 on first-attempt success', async () => { + const result = await takeScreenshot({ url: 'https://example.com' }) + expect(result.retryCount).toBe(0) + }) + + it('retries on transient error and returns retryCount 1', async () => { + vi.useFakeTimers() + try { + mockPage.goto + .mockImplementationOnce(async () => { throw new Error('net::ERR_CONNECTION_RESET') }) + .mockResolvedValueOnce(undefined) + + const promise = takeScreenshot({ url: 'https://example.com' }) + await vi.advanceTimersByTimeAsync(2000) + const result = await promise + expect(result.retryCount).toBe(1) + expect(result.buffer).toBeTruthy() + } finally { + vi.useRealTimers() + } + }) + + it('returns original error after all 3 attempts fail with retryCount 2', async () => { + vi.useFakeTimers() + try { + mockPage.goto + .mockRejectedValueOnce(new Error('Target closed')) + .mockRejectedValueOnce(new Error('Target closed')) + .mockRejectedValueOnce(new Error('Target closed')) + + const promise = takeScreenshot({ url: 'https://example.com' }).catch((err: any) => err) + await vi.advanceTimersByTimeAsync(5000) + + const err = await promise + expect(err).toBeInstanceOf(Error) + expect(err.message).toBe('Target closed') + expect(err.retryCount).toBe(2) + } finally { + vi.useRealTimers() + } + }) + + it('does NOT retry validation errors', async () => { + await expect(takeScreenshot({ + url: 'https://example.com', + selector: '#content', + fullPage: true, + })).rejects.toThrow('selector and fullPage are mutually exclusive') + expect(acquirePage).not.toHaveBeenCalled() + }) + + it('does NOT retry SSRF errors', async () => { + vi.mocked(validateUrl).mockRejectedValueOnce(new Error('SSRF_BLOCKED')) + await expect(takeScreenshot({ url: 'http://10.0.0.1' })).rejects.toThrow('SSRF_BLOCKED') + expect(acquirePage).not.toHaveBeenCalled() + }) + + it('releases page on each failed attempt before retry', async () => { + vi.useFakeTimers() + try { + mockPage.goto + .mockRejectedValueOnce(new Error('Protocol error')) + .mockResolvedValueOnce(undefined) + + const promise = takeScreenshot({ url: 'https://example.com' }) + await vi.advanceTimersByTimeAsync(2000) + await promise + expect(releasePage).toHaveBeenCalledTimes(2) + } finally { + vi.useRealTimers() + } + }) + }) }) diff --git a/src/services/retry.ts b/src/services/retry.ts new file mode 100644 index 0000000..76fbda5 --- /dev/null +++ b/src/services/retry.ts @@ -0,0 +1,17 @@ +const RETRYABLE_PATTERNS = [ + 'TimeoutError', + 'Protocol error', + 'Target closed', + 'Session closed', + 'Navigation failed', + 'net::ERR_', +]; + +export function isRetryableError(error: Error): boolean { + // Check error name + if (error.name === 'TimeoutError') return true; + + // Check message patterns + const msg = error.message || ''; + return RETRYABLE_PATTERNS.some(pattern => msg.includes(pattern)); +} diff --git a/src/services/screenshot.ts b/src/services/screenshot.ts index 6321c3e..df0b966 100644 --- a/src/services/screenshot.ts +++ b/src/services/screenshot.ts @@ -1,6 +1,7 @@ import { Page } from "puppeteer"; import { acquirePage, releasePage } from "./browser.js"; import { validateUrl } from "./ssrf.js"; +import { isRetryableError } from "./retry.js"; import logger from "./logger.js"; export interface ScreenshotOptions { @@ -26,6 +27,7 @@ export interface ScreenshotOptions { export interface ScreenshotResult { buffer: Buffer; contentType: string; + retryCount: number; } const MAX_WIDTH = 3840; @@ -73,11 +75,14 @@ function validateSelector(selector: string): void { } } +const MAX_RETRIES = 2; +const BACKOFF_MS = [500, 1000]; + export async function takeScreenshot(opts: ScreenshotOptions): Promise { - // Validate URL for SSRF + // Validate URL for SSRF — not retried await validateUrl(opts.url); - // Validate CSS injection prevention + // Validate CSS injection prevention — not retried if (opts.hideSelectors && opts.hideSelectors.length > 0) { validateHideSelectors(opts.hideSelectors); } @@ -104,6 +109,31 @@ export async function takeScreenshot(opts: ScreenshotOptions): Promise 0) { + const delay = BACKOFF_MS[attempt - 1]; + logger.warn({ attempt, delay, url: opts.url, error: lastError?.message }, "Retrying screenshot"); + await new Promise(r => setTimeout(r, delay)); + } + + try { + const result = await executeBrowserScreenshot(opts); + return { ...result, retryCount: attempt }; + } catch (err: any) { + lastError = err; + if (!isRetryableError(err)) { + throw err; + } + } + } + + // All attempts exhausted + lastError.retryCount = MAX_RETRIES; + throw lastError; +} + +async function executeBrowserScreenshot(opts: ScreenshotOptions): Promise> { const format = opts.format || "png"; const width = Math.min(opts.width || 1280, MAX_WIDTH); const height = Math.min(opts.height || 800, MAX_HEIGHT); @@ -173,19 +203,16 @@ export async function takeScreenshot(opts: ScreenshotOptions): Promise