diff --git a/.github/workflows/server-test.yaml b/.github/workflows/server-test.yaml index 6369b76bd..b3bc43e60 100644 --- a/.github/workflows/server-test.yaml +++ b/.github/workflows/server-test.yaml @@ -36,6 +36,11 @@ jobs: cache: true cache-dependency-path: server/go.sum + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: 22 + # categorygen's checks (unclassified route, category that isn't control or # platform, classified route with no handler) only run when the generator # does, and its only other caller is `make oapi-generate`, which needs the @@ -50,6 +55,10 @@ jobs: run: make test-unit working-directory: server + - name: Run Playwright daemon unit tests + run: make test-runtime + working-directory: server + test-server-e2e: runs-on: ubuntu-latest needs: [build-headful, build-headless] diff --git a/images/chromium-headful/Dockerfile b/images/chromium-headful/Dockerfile index 971ac4023..452e0db52 100644 --- a/images/chromium-headful/Dockerfile +++ b/images/chromium-headful/Dockerfile @@ -372,7 +372,7 @@ COPY --from=server-builder /out/kernel-images-supervisord-shim /usr/local/bin/ke COPY --from=server-builder /out/wrapper /wrapper # Copy and compile the Playwright daemon -COPY server/runtime/playwright-daemon.ts /tmp/playwright-daemon.ts +COPY server/runtime/playwright-daemon.ts server/runtime/page-target-id-cache.ts /tmp/ RUN esbuild /tmp/playwright-daemon.ts \ --bundle \ --platform=node \ @@ -382,7 +382,7 @@ RUN esbuild /tmp/playwright-daemon.ts \ --external:playwright-core \ --external:patchright \ --external:esbuild \ - && rm /tmp/playwright-daemon.ts + && rm /tmp/playwright-daemon.ts /tmp/page-target-id-cache.ts RUN useradd -m -s /bin/bash kernel diff --git a/images/chromium-headless/image/Dockerfile b/images/chromium-headless/image/Dockerfile index c118fc542..920c988be 100644 --- a/images/chromium-headless/image/Dockerfile +++ b/images/chromium-headless/image/Dockerfile @@ -268,7 +268,7 @@ COPY --from=server-builder /out/chromium-launcher /usr/local/bin/chromium-launch COPY --from=server-builder /out/kernel-images-supervisord-shim /usr/local/bin/kernel-images-supervisord-shim # Copy and compile the Playwright daemon -COPY server/runtime/playwright-daemon.ts /tmp/playwright-daemon.ts +COPY server/runtime/playwright-daemon.ts server/runtime/page-target-id-cache.ts /tmp/ RUN esbuild /tmp/playwright-daemon.ts \ --bundle \ --platform=node \ @@ -278,6 +278,6 @@ RUN esbuild /tmp/playwright-daemon.ts \ --external:playwright-core \ --external:patchright \ --external:esbuild \ - && rm /tmp/playwright-daemon.ts + && rm /tmp/playwright-daemon.ts /tmp/page-target-id-cache.ts ENTRYPOINT [ "/wrapper" ] diff --git a/server/Makefile b/server/Makefile index 0ce673328..5934802e0 100644 --- a/server/Makefile +++ b/server/Makefile @@ -1,5 +1,5 @@ SHELL := /bin/bash -.PHONY: oapi-generate build dev test test-unit test-e2e clean +.PHONY: oapi-generate build dev test test-unit test-runtime test-e2e clean BIN_DIR ?= $(CURDIR)/bin RECORDING_DIR ?= $(CURDIR)/recordings @@ -30,15 +30,18 @@ build: | $(BIN_DIR) dev: build $(RECORDING_DIR) OUTPUT_DIR=$(RECORDING_DIR) DISPLAY_NUM=$(DISPLAY_NUM) ./bin/api -# `test` runs unit + e2e. The two are split so callers (e.g. the Hypeman CI job) -# can run just the e2e suite, and so e2e logs stream as they run instead of -# waiting for all unit tests to complete. -test: test-unit test-e2e +# `test` runs Go unit, runtime unit, and e2e tests. The suites are split so +# callers (e.g. the Hypeman CI job) can run just the e2e suite, and so e2e logs +# stream as they run instead of waiting for all unit tests to complete. +test: test-unit test-runtime test-e2e test-unit: go vet ./... go test -v -race $$(go list ./... | grep -v /e2e$$) +test-runtime: + node --test runtime/*.test.ts + test-e2e: @echo "" @echo "=== Running e2e tests (this may take a few minutes) ===" diff --git a/server/runtime/page-target-id-cache.test.ts b/server/runtime/page-target-id-cache.test.ts new file mode 100644 index 000000000..24ab0df7d --- /dev/null +++ b/server/runtime/page-target-id-cache.test.ts @@ -0,0 +1,157 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { PageTargetIdCache } from './page-target-id-cache.ts'; + +test('reuses a successfully discovered target ID', async () => { + const page = {}; + let discoveries = 0; + const cache = new PageTargetIdCache(async () => { + discoveries++; + return 'target-1'; + }); + + assert.equal(await cache.get(page), 'target-1'); + assert.equal(await cache.get(page), 'target-1'); + assert.equal(discoveries, 1); +}); + +test('refreshes a cached target ID', async () => { + const page = {}; + let targetId = 'stale-target'; + let discoveries = 0; + const cache = new PageTargetIdCache(async () => { + discoveries++; + return targetId; + }); + + assert.equal(await cache.get(page), 'stale-target'); + targetId = 'fresh-target'; + assert.equal(await cache.get(page), 'stale-target'); + assert.equal(await cache.get(page, { refresh: true }), 'fresh-target'); + assert.equal(await cache.get(page), 'fresh-target'); + assert.equal(discoveries, 2); +}); + +test('does not cache failed discovery', async () => { + const page = {}; + let discoveries = 0; + const cache = new PageTargetIdCache(async () => { + discoveries++; + if (discoveries === 1) throw new Error('page closed'); + return 'target-1'; + }); + + await assert.rejects(cache.get(page), /page closed/); + assert.equal(await cache.get(page), 'target-1'); + assert.equal(discoveries, 2); +}); + +test('shares an in-flight discovery between concurrent callers', async () => { + const page = {}; + const pending = Promise.withResolvers(); + let discoveries = 0; + const cache = new PageTargetIdCache(() => { + discoveries++; + return pending.promise; + }); + + const first = cache.get(page); + const second = cache.get(page); + assert.equal(discoveries, 1); + + pending.resolve('target-1'); + assert.deepEqual(await Promise.all([first, second]), ['target-1', 'target-1']); + assert.equal(discoveries, 1); +}); + +test('failed refresh evicts the stale target ID', async () => { + const page = {}; + const results = ['stale-target', new Error('target replaced'), 'fresh-target']; + let discoveries = 0; + const cache = new PageTargetIdCache(async () => { + const result = results[discoveries++]; + if (result instanceof Error) throw result; + return result; + }); + + assert.equal(await cache.get(page), 'stale-target'); + await assert.rejects(cache.get(page, { refresh: true }), /target replaced/); + assert.equal(await cache.get(page), 'fresh-target'); + assert.equal(discoveries, 3); +}); + +test('builds an index while skipping pages that cannot be inspected', async () => { + const firstPage = {}; + const closedPage = {}; + const secondPage = {}; + const targetIds = new Map([ + [firstPage, 'target-1'], + [secondPage, 'target-2'], + ]); + const cache = new PageTargetIdCache(async page => { + const targetId = targetIds.get(page); + if (targetId === undefined) throw new Error('page closed'); + return targetId; + }); + + const pageByTargetId = await cache.buildPageByTargetId([firstPage, closedPage, secondPage]); + + assert.deepEqual([...pageByTargetId.entries()], [ + ['target-1', firstPage], + ['target-2', secondPage], + ]); +}); + +test('refreshes cached IDs while rebuilding the index', async () => { + const page = {}; + let targetId = 'stale-target'; + let discoveries = 0; + const cache = new PageTargetIdCache(async () => { + discoveries++; + return targetId; + }); + + assert.equal((await cache.buildPageByTargetId([page])).get('stale-target'), page); + targetId = 'fresh-target'; + const refreshed = await cache.buildPageByTargetId([page], { refresh: true }); + + assert.equal(refreshed.has('stale-target'), false); + assert.equal(refreshed.get('fresh-target'), page); + assert.equal(discoveries, 2); +}); + +test('reset discards every cached target ID', async () => { + const firstPage = {}; + const secondPage = {}; + let discoveries = 0; + const cache = new PageTargetIdCache(async () => `target-${++discoveries}`); + + assert.equal(await cache.get(firstPage), 'target-1'); + assert.equal(await cache.get(secondPage), 'target-2'); + cache.reset(); + assert.equal(await cache.get(firstPage), 'target-3'); + assert.equal(await cache.get(secondPage), 'target-4'); +}); + +test('reset prevents an in-flight discovery from repopulating the cache', async () => { + const page = {}; + const pending = Promise.withResolvers(); + let discoveries = 0; + const cache = new PageTargetIdCache(async () => { + discoveries++; + if (discoveries === 1) return pending.promise; + return 'fresh-target'; + }); + + const staleDiscovery = cache.get(page); + cache.reset(); + const freshDiscovery = cache.get(page); + assert.equal(discoveries, 2); + + pending.resolve('stale-target'); + assert.equal(await staleDiscovery, 'stale-target'); + assert.equal(await freshDiscovery, 'fresh-target'); + assert.equal(await cache.get(page), 'fresh-target'); + assert.equal(discoveries, 2); +}); diff --git a/server/runtime/page-target-id-cache.ts b/server/runtime/page-target-id-cache.ts new file mode 100644 index 000000000..2d86aea33 --- /dev/null +++ b/server/runtime/page-target-id-cache.ts @@ -0,0 +1,67 @@ +interface CacheOptions { + refresh?: boolean; +} + +// A CDP page target ID is stable for the lifetime of its Playwright Page. +// Weak keys let closed pages and their IDs be collected without explicit +// eviction; refresh and reset cover target replacement and browser reconnects. +export class PageTargetIdCache { + #targetIdMemo = new WeakMap(); + #targetIdInFlight = new WeakMap>(); + #generation = 0; + readonly #discoverTargetId: (page: Page) => Promise; + + constructor(discoverTargetId: (page: Page) => Promise) { + this.#discoverTargetId = discoverTargetId; + } + + async get(page: Page, options: CacheOptions = {}): Promise { + if (!options.refresh) { + const cached = this.#targetIdMemo.get(page); + if (cached !== undefined) return cached; + } else { + this.#targetIdMemo.delete(page); + } + + const inFlight = this.#targetIdInFlight.get(page); + if (inFlight !== undefined) return inFlight; + + const generation = this.#generation; + const discovery = this.#discoverTargetId(page) + .then(targetId => { + if (generation === this.#generation) { + this.#targetIdMemo.set(page, targetId); + } + return targetId; + }) + .finally(() => { + if (this.#targetIdInFlight.get(page) === discovery) { + this.#targetIdInFlight.delete(page); + } + }); + + this.#targetIdInFlight.set(page, discovery); + return discovery; + } + + async buildPageByTargetId(pages: readonly Page[], options: CacheOptions = {}): Promise> { + const pageByTargetId = new Map(); + + for (const page of pages) { + try { + pageByTargetId.set(await this.get(page, options), page); + } catch { + // A crashed or closing page can fail target discovery. Exclude it from + // this snapshot without preventing other live pages from resolving. + } + } + + return pageByTargetId; + } + + reset(): void { + this.#targetIdMemo = new WeakMap(); + this.#targetIdInFlight = new WeakMap>(); + this.#generation++; + } +} diff --git a/server/runtime/playwright-daemon.ts b/server/runtime/playwright-daemon.ts index 78a93ee89..9823f350a 100644 --- a/server/runtime/playwright-daemon.ts +++ b/server/runtime/playwright-daemon.ts @@ -12,9 +12,11 @@ import { createServer, Socket } from 'net'; import { unlinkSync, existsSync } from 'fs'; import { transform } from 'esbuild'; -import { chromium as chromiumPW, Browser, Page } from 'playwright-core'; +import { chromium as chromiumPW, Browser, CDPSession, Page } from 'playwright-core'; import { chromium as chromiumPR } from 'patchright'; +import { PageTargetIdCache } from './page-target-id-cache'; + const SOCKET_PATH = process.env.PLAYWRIGHT_DAEMON_SOCKET || '/tmp/playwright-daemon.sock'; const CDP_ENDPOINT = process.env.CDP_ENDPOINT || 'ws://127.0.0.1:9222'; const USE_PATCHRIGHT = process.env.PLAYWRIGHT_ENGINE !== 'playwright-core'; @@ -25,6 +27,16 @@ let browser: Browser | null = null; let connecting = false; let reconnectAttempts = 0; +const pageTargetIdCache = new PageTargetIdCache(async page => { + const session = await page.context().newCDPSession(page); + try { + const { targetInfo } = await session.send('Target.getTargetInfo'); + return targetInfo.targetId; + } finally { + await session.detach().catch(() => {}); + } +}); + interface ExecuteRequest { id: string; code: string; @@ -76,6 +88,7 @@ async function transformCode(code: string): Promise { async function disconnectBrowser(): Promise { const connectedBrowser = browser; browser = null; + pageTargetIdCache.reset(); if (!connectedBrowser) return; try { @@ -110,6 +123,7 @@ async function ensureBrowserConnection(): Promise { // Ignore } browser = null; + pageTargetIdCache.reset(); } console.error(`[playwright-daemon] Connecting to CDP: ${CDP_ENDPOINT}`); @@ -121,6 +135,7 @@ async function ensureBrowserConnection(): Promise { console.error('[playwright-daemon] Browser disconnected'); if (browser === connectedBrowser) { browser = null; + pageTargetIdCache.reset(); } }); @@ -131,59 +146,46 @@ async function ensureBrowserConnection(): Promise { } } -async function activeTabTargetIds(browser: Browser): Promise { - const root = await browser.newBrowserCDPSession(); - - try { - const { targetInfos } = await root.send('Target.getTargets', { - filter: [{ type: 'tab', exclude: false }, { exclude: true }], - }); +async function activeTabTargetIds(root: CDPSession): Promise { + const { targetInfos } = await root.send('Target.getTargets', { + filter: [{ type: 'tab', exclude: false }, { exclude: true }], + }); - return targetInfos - .filter(target => (target.embedderData as any)?.tabActive === true) - .map(target => target.targetId); - } finally { - await root.detach().catch(() => {}); - } + return targetInfos + .filter(target => (target.embedderData as any)?.tabActive === true) + .map(target => target.targetId); } -async function pageForTabTarget(browser: Browser, targetId: string, pages: Page[]): Promise { - const root = await browser.newBrowserCDPSession(); +async function pageForTabTarget( + root: CDPSession, + targetId: string, + pageByTargetId: Map, +): Promise { + // The listener is scoped to this call so page targets attached for one tab + // never leak into another tab's candidate set. + const relatedPageIds = new Set(); + const collectRelatedPage = (event: { targetInfo: { type: string; subtype?: string; targetId: string } }) => { + if (event.targetInfo.type === 'page' && !event.targetInfo.subtype) { + relatedPageIds.add(event.targetInfo.targetId); + } + }; + root.on('Target.attachedToTarget', collectRelatedPage); try { - const relatedPageIds = new Set(); - root.on('Target.attachedToTarget', event => { - if (event.targetInfo.type === 'page' && !event.targetInfo.subtype) { - relatedPageIds.add(event.targetInfo.targetId); - } - }); - await root.send('Target.autoAttachRelated', { targetId, waitForDebuggerOnStart: false, filter: [{ type: 'page', exclude: false }, { exclude: true }], }); - for (const page of pages) { - try { - const context = page.context(); - const session = await context.newCDPSession(page); - try { - const { targetInfo } = await session.send('Target.getTargetInfo'); - if (relatedPageIds.has(targetInfo.targetId)) return page; - } finally { - await session.detach().catch(() => {}); - } - } catch { - // A crashed or closing page can fail CDP session setup/queries; skip it - // rather than aborting the search for the real foreground tab. - continue; - } + for (const relatedPageId of relatedPageIds) { + const page = pageByTargetId.get(relatedPageId); + if (page && !page.isClosed()) return page; } return null; } finally { - await root.detach().catch(() => {}); + root.off('Target.attachedToTarget', collectRelatedPage); } } @@ -215,16 +217,25 @@ async function resolveActivePage(browser: Browser): Promise { .contexts() .flatMap(context => context.pages()) .filter(page => !page.isClosed()); + const pageByTargetId = await pageTargetIdCache.buildPageByTargetId(pages, { refresh: attempt > 0 }); - activeTabIds = await activeTabTargetIds(browser); - for (const targetId of activeTabIds) { - try { - const page = await pageForTabTarget(browser, targetId, pages); - if (page) return page; - } catch { - // This tab may have changed while it was inspected. Keep trying the - // other active tabs reported by the same browser snapshot. + // One browser-level session serves the whole attempt: the active-tab + // listing and every tab-to-page join. Detaching it also cleans up the + // page sessions auto-attached by pageForTabTarget. + const root = await browser.newBrowserCDPSession(); + try { + activeTabIds = await activeTabTargetIds(root); + for (const targetId of activeTabIds) { + try { + const page = await pageForTabTarget(root, targetId, pageByTargetId); + if (page) return page; + } catch { + // This tab may have changed while it was inspected. Keep trying the + // other active tabs reported by the same browser snapshot. + } } + } finally { + await root.detach().catch(() => {}); } // No active tab matched this page snapshot. Retry with fresh snapshots.