From 5123654b0defe75cea187cabf14dc5ec3017300f Mon Sep 17 00:00:00 2001 From: Vito Date: Thu, 22 Oct 2026 19:49:14 -0700 Subject: [PATCH] fix(admin): start no list or stats read while the admin page leaves (#290) On WebKit, a reload cancels the page's reads between beforeunload and pagehide. After an in-place edit, finishEdit awaits loadStatistics, which swallows its cancelled read, and then calls refreshList. That /recent read starts while the page is leaving, and WebKit fails it with "Fetch API cannot load ... due to access control checks." Playwright reports that console error as a page error. This failed workflows.spec on WebKit in CI. It is not an unhandled rejection: the page catches the failure. The read should never start. The admin page now has a lifetime (AbortController) that ends on beforeunload, unless a running scan asks to stay, and on pagehide. - ApiClient.latestOnly({lifetime}) starts no request once the lifetime has ended, aborts the one in flight, and resolves both to STALE. - loadStatistics skips and silences a read that ended with the page. - Real errors still propagate and show as before. - A page restored from the back/forward cache, or one whose navigation never left, gets a new lifetime on pageshow or its next pointer or key input. Tests: - The test server can hold a real request at the server (request_hold.py), because a page.route hold is cancelled differently by WebKit. - A WebKit regression test holds /prompt_manager/stats after an in-place edit and reloads. It failed 5/5 before the fix with the CI message and passes 5/5 after it. - A test checks that a called-off leave reads the list again. - api-client unit tests cover the lifetime. --- tests/browser/request_hold.py | 106 +++++++++++++++++++++++++++++++ tests/browser/server.py | 3 + tests/browser/workflows.spec.mjs | 31 +++++++++ tests/js/api-client.test.js | 29 +++++++++ web/js/admin.js | 37 +++++++++-- web/js/api-client.js | 14 +++- 6 files changed, 213 insertions(+), 7 deletions(-) create mode 100644 tests/browser/request_hold.py diff --git a/tests/browser/request_hold.py b/tests/browser/request_hold.py new file mode 100644 index 0000000..eef5fd0 --- /dev/null +++ b/tests/browser/request_hold.py @@ -0,0 +1,106 @@ +"""Hold real requests at this test server, so a page can leave while one is in flight. + +A Playwright ``page.route`` hold answers inside the browser's interception layer, +which WebKit cancels differently from a request the server has not answered yet. +These commands hold the request at the server itself: + +- ``POST /__test__/hold`` with ``{"path": "/prompt_manager/..."}`` holds every + later request for that path until it is released. With ``"stage": "body"`` + the server answers first and holds after sending the headers and the first + half of the body, so the browser has a response whose body is still loading. +- ``GET /__test__/hold`` answers ``{"held": {path: count}}``: how many requests + for each held path have arrived and are waiting. +- ``POST /__test__/release`` lets every held request go on, and stops holding. +""" + +import asyncio + +from aiohttp import web + +HOLDABLE_PREFIX = "/prompt_manager/" + + +class RequestHolds: + def __init__(self) -> None: + self._gates: dict[str, asyncio.Event] = {} + self._waiting: dict[str, int] = {} + self._stages: dict[str, str] = {} + + def hold(self, path: str, stage: str = "request") -> None: + self._gates.setdefault(path, asyncio.Event()) + self._waiting.setdefault(path, 0) + self._stages[path] = stage + + def stage(self, path: str) -> str | None: + return self._stages.get(path) if path in self._gates else None + + def held(self) -> dict[str, int]: + return dict(self._waiting) + + def release(self) -> None: + for gate in self._gates.values(): + gate.set() + self._gates.clear() + self._waiting.clear() + self._stages.clear() + + async def wait(self, path: str) -> None: + gate = self._gates.get(path) + if gate is None: + return + self._waiting[path] = self._waiting.get(path, 0) + 1 + try: + await gate.wait() + finally: + if path in self._waiting: + self._waiting[path] -= 1 + + +def register_request_holds(app: web.Application, routes) -> RequestHolds: + holds = RequestHolds() + + @web.middleware + async def hold_middleware(request: web.Request, handler): + stage = holds.stage(request.path) + if stage == "request": + await holds.wait(request.path) + response = await handler(request) + if stage != "body" or not isinstance(response, web.Response): + return response + body = response.body if isinstance(response.body, bytes) else b"" + streamed = web.StreamResponse(status=response.status, headers=response.headers) + streamed.content_length = len(body) + await streamed.prepare(request) + await streamed.write(body[: len(body) // 2]) + await holds.wait(request.path) + await streamed.write(body[len(body) // 2 :]) + await streamed.write_eof() + return streamed + + app.middlewares.append(hold_middleware) + + @routes.post("/__test__/hold") + async def hold(request: web.Request) -> web.Response: + data = await request.json() if request.can_read_body else {} + path = data.get("path") if isinstance(data, dict) else None + stage = data.get("stage", "request") if isinstance(data, dict) else None + if ( + not isinstance(path, str) + or not path.startswith(HOLDABLE_PREFIX) + or set(data) - {"path", "stage"} + or stage not in ("request", "body") + ): + raise web.HTTPBadRequest() + holds.hold(path, stage) + return web.json_response({"success": True}) + + @routes.get("/__test__/hold") + async def held(request: web.Request) -> web.Response: + return web.json_response({"success": True, "held": holds.held()}) + + @routes.post("/__test__/release") + async def release(request: web.Request) -> web.Response: + holds.release() + return web.json_response({"success": True}) + + return holds diff --git a/tests/browser/server.py b/tests/browser/server.py index 0d202da..9764aa7 100644 --- a/tests/browser/server.py +++ b/tests/browser/server.py @@ -18,6 +18,7 @@ from aiohttp.web_runner import GracefulExit from PIL import Image from PIL.PngImagePlugin import PngInfo +from tests.browser.request_hold import register_request_holds from tests.browser.workspace_activity_fixture import register_activity_commands from tests.browser.workspace_concept_fixture import ( MAX_EXTRA_IMAGES as CONCEPT_MAX_EXTRA_IMAGES, @@ -195,6 +196,7 @@ def create_app(root: Path) -> web.Application: register_tagging_commands(routes, api, root, model) reveal_stub = register_reveal_stub(routes, api, root) register_activity_commands(app, routes, api) + request_holds = register_request_holds(app, routes) async def canonical_asset(request: web.Request) -> web.Response: filename, content_type = CANONICAL_ASSETS[request.path] @@ -374,6 +376,7 @@ def create_app(root: Path) -> web.Application: @routes.post("/__test__/reset") async def reset(request: web.Request) -> web.Response: nonlocal history_fixture + request_holds.release() library_editor_commands.reset() data = await request.json() if request.can_read_body else {} if not isinstance(data, dict): diff --git a/tests/browser/workflows.spec.mjs b/tests/browser/workflows.spec.mjs index ab5d842..2f31eda 100644 --- a/tests/browser/workflows.spec.mjs +++ b/tests/browser/workflows.spec.mjs @@ -43,6 +43,37 @@ test('actual gallery form saves a prompt that the admin page can edit and reload expect((await saved.json()).prompt_id).not.toBe(Number(id)); }); +// WebKit fails a fetch that a leaving page starts ("Fetch API cannot load … due to access control checks."), and +// Playwright reports that console error as a page error. A save's follow-up reads must not start once the page hides: +// here the statistics read after an in-place edit is held at the server while the page reloads. +test('an in-place edit whose follow-up reads outlive the page starts no read while it leaves', async ({ page, request }) => { + await page.goto('/prompt_manager/admin'); + const text = page.locator('.prompt-text').filter({ hasText: 'Cat portrait' }); + const id = await text.getAttribute('data-id'); + await expect(await request.post('/__test__/hold', { data: { path: '/prompt_manager/stats' } })).toBeOK(); + await page.locator(`[data-action="edit"][data-prompt-id="${id}"]`).click(); + const editor = page.locator(`.prompt-text[data-id="${id}"]`); + await editor.fill('Cat portrait at dusk'); + await editor.press('Enter'); + const held = async () => (await (await request.get('/__test__/hold')).json()).held['/prompt_manager/stats']; + await expect.poll(held).toBe(1); + await page.reload({ waitUntil: 'commit' }); + await expect(await request.post('/__test__/release')).toBeOK(); + await expect(page.locator('.prompt-text').filter({ hasText: 'Cat portrait at dusk' })).toBeVisible(); +}); + +// A leave that never happens (a stopped navigation, or one answered with a download) must not leave the list dead. +test('a page whose leave was called off reads its list again on the next input', async ({ page }) => { + await page.goto('/prompt_manager/admin'); + await expect(page.locator('.prompt-text').first()).toBeVisible(); + await page.evaluate(() => window.dispatchEvent(new Event('beforeunload'))); + await page.mouse.click(1, 1); + const reread = page.waitForResponse(response => new URL(response.url()).pathname === '/prompt_manager/recent'); + await page.locator('#sortBy').selectOption('created_asc'); + expect(new URL((await reread).url()).searchParams.get('sort')).toBe('created_asc'); + await expect(page.locator('#resultsTitle')).toHaveText('Recent Prompts'); +}); + test('actual gallery review generates and persists tags for the selected root', async ({ page, request }) => { await page.goto('/prompt_manager/gallery'); await expect(page.locator('#galleryGrid .image-item')).toHaveCount(2); diff --git a/tests/js/api-client.test.js b/tests/js/api-client.test.js index 01b45b4..df2fa21 100644 --- a/tests/js/api-client.test.js +++ b/tests/js/api-client.test.js @@ -236,6 +236,35 @@ test("latestOnly: errors from the current call propagate, errors from superseded assert.equal(await first, STALE); }); +test("latestOnly with a lifetime: once it ends, no request starts and the call is STALE", async () => { + const fetch = fakeFetch(); + const api = createApiClient(fetch); + const page = new AbortController(); + const latest = latestOnly({ lifetime: page.signal }); + page.abort(); + assert.equal(await latest((signal) => api.get("/recent", { signal })), STALE); + assert.equal(fetch.calls.length, 0, "a leaving page starts no read"); +}); + +test("latestOnly with a lifetime: its end aborts the request in flight, which is STALE, not an error", async () => { + const fetch = fakeFetch(); + const api = createApiClient(fetch); + const page = new AbortController(); + const latest = latestOnly({ lifetime: page.signal }); + const call = latest((signal) => api.get("/recent", { signal })); + page.abort(); + assert.equal(fetch.calls[0].aborted, true, "the read in flight is aborted"); + fetch.calls[0].reject(new DOMException("The operation was aborted.", "AbortError")); + assert.equal(await call, STALE); +}); + +test("latestOnly with a lifetime: real errors still propagate while the page lives", async () => { + const page = new AbortController(); + const latest = latestOnly({ lifetime: page.signal }); + await assert.rejects(latest(async () => { throw new Error("server down"); }), /server down/); + assert.equal(await latest(async () => "fine"), "fine"); +}); + test("ApiError exposes only validated history recovery actions on all error paths", async () => { const permitted = [null, { action: "refresh-history" }, { action: "reload-outputs" }, { action: "retry-read" }, { action: "smaller-limit", limit: 1 }, diff --git a/web/js/admin.js b/web/js/admin.js index 5aba7aa..da699ac 100644 --- a/web/js/admin.js +++ b/web/js/admin.js @@ -25,8 +25,10 @@ this.tagsPage = null; this.api = ApiClient.createApiClient(); - // Search and page loads share one guard: whichever list request is newest wins - this.listRequest = ApiClient.latestOnly(); + // The page's lifetime ends when it starts to leave. WebKit cancels the page's reads + // between beforeunload and pagehide; a save's follow-up read started then fails and is + // logged as a page error ("Fetch API cannot load … due to access control checks."). + this.startPageLifetime(); this.infiniteScroll = false; // session value; seeded from settings, toggled in the header this.infiniteScrollChosen = false; this.loadingMore = false; @@ -150,7 +152,18 @@ // Output scan: a server-side job that survives modal dismissal and reloads this.scanState = ScanProgress.initialState(); this.scanSource = null; - window.addEventListener("beforeunload", (e) => this.warnIfScanRunning(e)); + window.addEventListener("beforeunload", (e) => { + this.warnIfScanRunning(e); + // A page that asks to stay (a running scan) keeps its reads until it really goes. + if (!e.defaultPrevented) this.pageLifetime.abort(); + }); + window.addEventListener("pagehide", () => this.pageLifetime.abort()); + // A page restored from the back/forward cache lives again, and so does one whose + // navigation never left (stopped, or answered with a download): its next input revives it. + window.addEventListener("pageshow", (e) => { if (e.persisted) this.revivePageLifetime(); }); + for (const type of ["pointerdown", "keydown"]) { + window.addEventListener(type, () => this.revivePageLifetime(), true); + } this.bindModalEvents(); @@ -319,9 +332,24 @@ } } + /** A new lifetime once the old one has ended; a living page keeps its list guard. */ + revivePageLifetime() { + if (this.pageLifetime.signal.aborted) this.startPageLifetime(); + } + + /** A fresh page lifetime, and the list guard bound to it. */ + startPageLifetime() { + this.pageLifetime = new AbortController(); + // Search and page loads share one guard: whichever list request is newest wins + this.listRequest = ApiClient.latestOnly({ lifetime: this.pageLifetime.signal }); + } + async loadStatistics() { + // A leaving page starts no read; one in flight is aborted with the page and not reported. + const { signal } = this.pageLifetime; + if (signal.aborted) return; try { - const response = await ApiClient.fetchUrl("/prompt_manager/stats"); + const response = await ApiClient.fetchUrl("/prompt_manager/stats", { signal }); if (response.ok) { const data = await response.json(); @@ -340,6 +368,7 @@ console.error("Stats API HTTP error:", response.status, response.statusText); } } catch (error) { + if (signal.aborted) return; console.error("Stats error:", error); // Set some default values if API fails document.getElementById("totalPrompts").textContent = this.prompts.length || 0; diff --git a/web/js/api-client.js b/web/js/api-client.js index df1f7a0..b334467 100644 --- a/web/js/api-client.js +++ b/web/js/api-client.js @@ -107,20 +107,28 @@ } /** + * @param {{lifetime?: AbortSignal}} [options] - `lifetime` ends with the page (its pagehide): + * then no call starts a request, the one in flight is aborted, and both resolve to STALE. + * WebKit fails a fetch that a leaving page starts and logs it as a page error. * @returns {(run: (signal: AbortSignal) => Promise<*>) => Promise<*>} wrapper that * aborts the previous call's signal and resolves superseded calls to STALE */ - function latestOnly() { + function latestOnly({ lifetime } = {}) { let current = null; + lifetime?.addEventListener("abort", () => current?.abort(), { once: true }); return async function (run) { if (current) current.abort(); + if (lifetime?.aborted) { + current = null; + return STALE; + } const controller = new AbortController(); current = controller; try { const result = await run(controller.signal); - return controller === current ? result : STALE; + return controller === current && !lifetime?.aborted ? result : STALE; } catch (error) { - if (controller !== current) return STALE; + if (controller !== current || lifetime?.aborted) return STALE; throw error; } };