From 7125681a986e22577fecbdba00f510d1fb9675f4 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Wed, 21 Oct 2026 11:49:14 -0700 Subject: [PATCH] fix(activity): stop the job store on pagehide and let proxied routes outlive teardown CI's touch coverage run failed "a resolver failure for the ComfyUI link never stops the shell from mounting" with "route.fulfill: Fetch response has been disposed": its trace shows two Library thumbnail requests started by the last route still in the spec's proxy-mount handler while the page fixture was torn down. - The proxy-mount handler ignores only that teardown error, and the workspace and Activity specs unroute with ignoreErrors before teardown. - The Activity store now ends its work on pagehide: the poll and backoff timers stop, the job streams close and a list read in flight is aborted (an older read is also aborted when a newer one starts). A pageshow from the back-forward cache reads the list again. --- tests/browser/workspace-activity.spec.mjs | 2 + tests/browser/workspace.spec.mjs | 12 +++++- tests/js/workspace-activity-store.test.js | 43 +++++++++++++++++++ web/js/workspace/activity-store.mjs | 50 ++++++++++++++++++----- 4 files changed, 94 insertions(+), 13 deletions(-) diff --git a/tests/browser/workspace-activity.spec.mjs b/tests/browser/workspace-activity.spec.mjs index f89c606..2c6b072 100644 --- a/tests/browser/workspace-activity.spec.mjs +++ b/tests/browser/workspace-activity.spec.mjs @@ -68,6 +68,8 @@ test.beforeEach(async ({ page, request, browserName }) => { }); test.afterEach(async ({ page, request, browserName }) => { + // The store's own reads can meet a route handler at teardown; that must not fail the test it outlived. + await page.unrouteAll({ behavior: 'ignoreErrors' }); await saveCoverage(page, browserName); expect(page.activityErrors).toEqual([]); await request.post('/__test__/jobs/restart'); diff --git a/tests/browser/workspace.spec.mjs b/tests/browser/workspace.spec.mjs index 3a71a3d..e353121 100644 --- a/tests/browser/workspace.spec.mjs +++ b/tests/browser/workspace.spec.mjs @@ -48,6 +48,8 @@ test.beforeEach(async ({ page, request, browserName }) => { }); test.afterEach(async ({ page, browserName }) => { + // A route handler still running at teardown must not fail the test it outlived. + await page.unrouteAll({ behavior: 'ignoreErrors' }); await saveCoverage(page, browserName); expect(page.workspaceErrors).toEqual([]); }); @@ -688,8 +690,14 @@ async function serveUnderProxyMount(page) { await page.route(url => url.pathname.startsWith(`${PROXY_MOUNT}/`), async route => { const url = new URL(route.request().url()); url.pathname = url.pathname.slice(PROXY_MOUNT.length); - const response = await route.fetch({ url: url.href }); - await route.fulfill({ response }); + // A request still in flight when the test ends (a thumbnail the last route started) meets a page that is + // closing: its fetched response is disposed under it. Only that teardown error is ignored. + try { + const response = await route.fetch({ url: url.href }); + await route.fulfill({ response }); + } catch (error) { + if (!/has been disposed|has been closed|Target page, context or browser has been closed/.test(String(error?.message))) throw error; + } }); return escaped; } diff --git a/tests/js/workspace-activity-store.test.js b/tests/js/workspace-activity-store.test.js index 5ded899..cd6e367 100644 --- a/tests/js/workspace-activity-store.test.js +++ b/tests/js/workspace-activity-store.test.js @@ -281,3 +281,46 @@ test('summaries are validated and the copy follows the spec', async () => { assert.equal(outcomeLine(other), ''); assert.deepEqual([-1, 0, 1, 4, 5, 6, 1e9, 0.5].map(backoffDelay), [1000, 1000, 2000, 16000, 30000, 30000, 30000, 1000]); }); + +test('pagehide stops polling, closes streams and aborts the in-flight read; pageshow resumes', async () => { + const { createActivityStore } = await source; + const { world, api, ...deps } = harness(); + const lifecycle = new EventTarget(); + const signals = []; + let answer = null; + const pendingApi = { ...api, get: (url, options) => { + signals.push(options.signal); + world.gets += 1; + if (world.gets === 1) return Promise.resolve({ success: true, jobs: [summary()] }); + return new Promise((resolve, reject) => { + answer = resolve; + options.signal.addEventListener('abort', () => reject(Object.assign(new Error('aborted'), { name: 'AbortError' }))); + }); + } }; + const store = createActivityStore({ ...deps, api: pendingApi, pollMs: 15000, lifecycle }); + await store.load(); + assert.equal(world.open().length, 1); + const poll = world.timers.find(timer => !timer.cleared && timer.delay === 15000); + assert.ok(poll, 'a visible tab polls'); + // A read in flight when the page goes away (a closed tab, a test teardown) is aborted, nothing is scheduled again. + const read = store.load(); + lifecycle.dispatchEvent(new Event('pagehide')); + await read; + assert.equal(signals.at(-1).aborted, true); + assert.equal(world.open().length, 0); + assert.ok(world.timers.every(timer => timer.cleared || timer.fired)); + assert.equal(store.find(ID_A).state, 'running', 'a hidden page marks nothing lost'); + await store.load(); + assert.equal(world.gets, 2, 'a suspended store reads nothing'); + // Back from the cache, it reads the list again and follows the job. + const resumed = Object.assign(new Event('pageshow'), { persisted: true }); + lifecycle.dispatchEvent(resumed); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(world.gets, 3); + answer({ success: true, jobs: [summary()] }); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(world.open().length, 1); + world.dispose(); + assert.equal(signals.at(-1).aborted, true, 'disposing aborts the store reads'); + assert.equal(world.open().length, 0); +}); diff --git a/web/js/workspace/activity-store.mjs b/web/js/workspace/activity-store.mjs index 259cc8d..3a248a1 100644 --- a/web/js/workspace/activity-store.mjs +++ b/web/js/workspace/activity-store.mjs @@ -101,10 +101,15 @@ function defaultConnect(url) { * @param {{api: {get: Function, post: Function}, scope: {signal: AbortSignal, cleanup: Function}, * connect?: (url: string) => {onmessage: Function|null, onerror: Function|null, close: Function}, * notify?: (message: string) => void, timers?: {setTimeout: Function, clearTimeout: Function}, - * pollMs?: number, isVisible?: () => boolean}} options + * pollMs?: number, isVisible?: () => boolean, lifecycle?: EventTarget}} options + * + * `lifecycle` (the window) ends the store's work on `pagehide`: the periodic and backoff reads stop, the streams close + * and a read in flight is aborted, so nothing answers into a page that is going away (a closed tab, a test teardown). + * A `pageshow` from the back-forward cache reads the list again. */ export function createActivityStore({ api, scope, connect = defaultConnect, notify = showWorkspaceToast, - timers = globalThis, pollMs = POLL_MS, isVisible = () => globalThis.document?.visibilityState !== 'hidden' } = {}) { + timers = globalThis, pollMs = POLL_MS, isVisible = () => globalThis.document?.visibilityState !== 'hidden', + lifecycle = globalThis.window ?? null } = {}) { let jobs = []; let status = 'idle'; let attempt = 0; @@ -112,6 +117,8 @@ export function createActivityStore({ api, scope, connect = defaultConnect, noti let pollTimer = null; let reads = 0; let disposed = false; + let suspended = false; + let reading = null; const streams = new Map(); const listeners = new Set(); @@ -181,7 +188,7 @@ export function createActivityStore({ api, scope, connect = defaultConnect, noti } function scheduleResync() { - if (disposed || retryTimer !== null) return; + if (disposed || suspended || retryTimer !== null) return; const delay = backoffDelay(attempt); attempt += 1; retryTimer = timers.setTimeout(() => { @@ -192,7 +199,7 @@ export function createActivityStore({ api, scope, connect = defaultConnect, noti /** The next periodic read; a hidden tab skips its turn. */ function schedulePoll() { - if (disposed || pollMs <= 0) return; + if (disposed || suspended || pollMs <= 0) return; if (pollTimer !== null) timers.clearTimeout(pollTimer); pollTimer = timers.setTimeout(() => { pollTimer = null; @@ -203,18 +210,22 @@ export function createActivityStore({ api, scope, connect = defaultConnect, noti /** Read this session's jobs. A failed read marks running jobs ended and tries again after the backoff. */ async function load() { - if (disposed) return; + if (disposed || suspended) return; const read = ++reads; + // Only the newest read applies, so an older one still in flight is aborted. + reading?.abort(); + const controller = new AbortController(); + reading = controller; try { - const result = await api.get(JOBS_URL, { signal: scope.signal, cache: 'no-store' }); - // Only the newest read applies; an older one answering late is dropped. - if (disposed || read !== reads) return; + const result = await api.get(JOBS_URL, { signal: controller.signal, cache: 'no-store' }); + // An older read answering late is dropped. + if (disposed || suspended || read !== reads) return; if (!Array.isArray(result?.jobs)) throw new TypeError('Invalid job list'); merge(result.jobs); status = 'ready'; schedulePoll(); } catch (error) { - if (disposed || scope.signal?.aborted || read !== reads) return; + if (disposed || suspended || scope.signal?.aborted || controller.signal.aborted || read !== reads) return; markLost(); status = 'error'; scheduleResync(); @@ -254,13 +265,30 @@ export function createActivityStore({ api, scope, connect = defaultConnect, noti }, }; - scope.cleanup(() => { - disposed = true; + /** Stop every timer, stream and read in flight. */ + function halt() { if (retryTimer !== null) timers.clearTimeout(retryTimer); if (pollTimer !== null) timers.clearTimeout(pollTimer); retryTimer = null; pollTimer = null; + reading?.abort(); + reading = null; for (const id of [...streams.keys()]) closeStream(id); + } + const onPageHide = () => { suspended = true; halt(); }; + const onPageShow = event => { + if (!suspended || !event.persisted) return; + suspended = false; + load(); + }; + lifecycle?.addEventListener('pagehide', onPageHide); + lifecycle?.addEventListener('pageshow', onPageShow); + + scope.cleanup(() => { + disposed = true; + halt(); + lifecycle?.removeEventListener('pagehide', onPageHide); + lifecycle?.removeEventListener('pageshow', onPageShow); listeners.clear(); }); return store;