diff --git a/backend/Cargo.lock b/backend/Cargo.lock index fa34138..a8a5899 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1517,7 +1517,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.87.22" +version = "0.87.23" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 9c02428..2e601b8 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.87.22" +version = "0.87.23" edition = "2021" default-run = "mangalord" diff --git a/frontend/package.json b/frontend/package.json index 11fcffa..8d9296f 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.87.22", + "version": "0.87.23", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/hooks.server.test.ts b/frontend/src/hooks.server.test.ts index 226da79..a2c7b9d 100644 --- a/frontend/src/hooks.server.test.ts +++ b/frontend/src/hooks.server.test.ts @@ -207,7 +207,7 @@ describe('hooks.server proxy', () => { // 0.87.9 the upstream fetch kept running until the proxy wall-clock // timeout fired. it('chains an already-aborted client signal into the upstream signal immediately', async () => { - // Capture the signal the proxy hands fetch; resolve when called. + // Capture the signal the proxy hands fetch. let upstream: AbortSignal | undefined; fetchSpy.mockImplementation(async (_url, init) => { upstream = (init as RequestInit).signal ?? undefined; @@ -225,6 +225,26 @@ describe('hooks.server proxy', () => { expect(upstream?.aborted).toBe(true); }); + it('returns 502 when fetch itself rejects with AbortError on an already-aborted signal', async () => { + // Production Node fetch rejects synchronously with AbortError if + // the signal is already aborted on entry — the prior test mocks + // fetch to resolve 200, which leaves the rejection-path + // observation uncovered. Mirror Node's real behaviour and + // confirm the handler maps the rejection to 502 / upstream_unavailable + // (same envelope as any other network failure). + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + fetchSpy.mockRejectedValueOnce(new DOMException('aborted', 'AbortError')); + const resolve = vi.fn(); + const ctrl = new AbortController(); + ctrl.abort(); + const event = makeEventWithSignal('/api/v1/health', ctrl.signal); + const resp = await handle({ event, resolve }); + expect(resp.status).toBe(502); + const body = await resp.json(); + expect(body.error.code).toBe('upstream_unavailable'); + expect(errSpy).toHaveBeenCalled(); + }); + it('chains a mid-flight client-disconnect into the upstream signal', async () => { // The mid-flight shape: request.signal is live when the handler // starts, then aborts while the upstream fetch is in progress. @@ -232,13 +252,13 @@ describe('hooks.server proxy', () => { let upstream: AbortSignal | undefined; fetchSpy.mockImplementation(async (_url, init) => { upstream = (init as RequestInit).signal ?? undefined; - // Let the handler's chaining listener attach, then trip the - // client signal — the upstream signal should reflect. + // Let the handler's chaining listener attach (it runs on + // the next microtask after `handle()` calls fetch), THEN + // trip the client signal. `AbortSignal#dispatchEvent` is + // synchronous, so the listener runs in-line — no further + // tick needed; we observe `upstream.aborted` immediately. await Promise.resolve(); clientCtrl.abort(); - // Tick once so the listener observes the abort before we - // check. - await Promise.resolve(); return new Response('[]', { status: 200 }); }); const resolve = vi.fn(); diff --git a/frontend/src/lib/api/admin.test.ts b/frontend/src/lib/api/admin.test.ts index c8320a3..173230a 100644 --- a/frontend/src/lib/api/admin.test.ts +++ b/frontend/src/lib/api/admin.test.ts @@ -731,28 +731,19 @@ describe('admin crawler api client', () => { }); it('getAnalysisMangaCoverage propagates an abort rejection', async () => { - // Mid-flight abort: caller flips the AbortController, the fetch - // promise rejects, and the api client lets the rejection - // surface to its caller (the debounce-race handler treats - // AbortError as silent cancellation rather than an error toast). + // Production Node fetch rejects synchronously with AbortError + // when called with an already-aborted signal. Mirror that + // (instead of wiring our own listener and asserting the + // rejection we self-fire) so the test exercises the contract + // the consumer relies on — the CancellableLoader at + // $lib/util/cancellable.ts treats AbortError as `null` and + // skips the state update. const ctrl = new AbortController(); - fetchSpy.mockImplementationOnce( - (_url, init) => - new Promise((_resolve, reject) => { - const signal = (init as RequestInit).signal; - signal?.addEventListener( - 'abort', - () => reject(new DOMException('aborted', 'AbortError')), - { once: true } - ); - }) - ); - const inflight = getAnalysisMangaCoverage( - { limit: 25 }, - { signal: ctrl.signal } - ); ctrl.abort(); - await expect(inflight).rejects.toMatchObject({ name: 'AbortError' }); + fetchSpy.mockRejectedValueOnce(new DOMException('aborted', 'AbortError')); + await expect( + getAnalysisMangaCoverage({ limit: 25 }, { signal: ctrl.signal }) + ).rejects.toMatchObject({ name: 'AbortError' }); }); it('getAnalysisChapterCoverage unwraps items', async () => { diff --git a/frontend/src/lib/util/cancellable.test.ts b/frontend/src/lib/util/cancellable.test.ts new file mode 100644 index 0000000..3db1be8 --- /dev/null +++ b/frontend/src/lib/util/cancellable.test.ts @@ -0,0 +1,173 @@ +import { describe, it, expect, vi } from 'vitest'; +import { CancellableLoader } from './cancellable'; + +/** + * Helper: a Promise that resolves when `signal` aborts. Mirrors the + * shape a real `fetch(url, { signal })` rejects on cancellation. + */ +function rejectsOnAbort(signal: AbortSignal, value?: T): Promise { + return new Promise((resolve, reject) => { + if (signal.aborted) { + reject(new DOMException('aborted', 'AbortError')); + return; + } + signal.addEventListener( + 'abort', + () => reject(new DOMException('aborted', 'AbortError')), + { once: true } + ); + // The caller wires its own resolve via the AbortSignal listeners + // in the test; if a value is provided, resolve after a tick. + if (value !== undefined) { + // microtask so the test can call run() again first. + queueMicrotask(() => resolve(value)); + } + }); +} + +describe('CancellableLoader', () => { + it('returns the resolved value when no successor cancels the call', async () => { + const loader = new CancellableLoader(); + const result = await loader.run(async () => 42); + expect(result).toBe(42); + }); + + it('aborts the predecessor signal when a successor starts', async () => { + const loader = new CancellableLoader(); + + // Predecessor: long-running, observes its own abort. + let predecessorSignal: AbortSignal | null = null; + const predecessor = loader.run(({ signal }) => { + predecessorSignal = signal; + return rejectsOnAbort(signal); + }); + + // Tick the microtask queue so the predecessor's signal is set. + await Promise.resolve(); + expect(predecessorSignal).toBeInstanceOf(AbortSignal); + expect(predecessorSignal!.aborted).toBe(false); + + // Successor: resolves immediately. + const successor = loader.run(async () => 'B'); + // The predecessor's signal must have been aborted. + expect(predecessorSignal!.aborted).toBe(true); + + // Successor wins; predecessor is swallowed. + expect(await successor).toBe('B'); + expect(await predecessor).toBeNull(); + }); + + it('suppresses a predecessor whose body resolves AFTER the successor started', async () => { + // Race: predecessor's fetch hasn't been cancelled in time (e.g. + // it had already started writing the response). Its resolved + // value must still NOT leak into the loader's caller — the + // successor owns the state. + const loader = new CancellableLoader(); + + // Predecessor: ignores its signal, resolves slowly. + let predecessorResolve: (v: string) => void = () => {}; + const predecessor = loader.run( + () => + new Promise((resolve) => { + predecessorResolve = resolve; + }) + ); + await Promise.resolve(); + + // Successor: replaces predecessor, resolves quickly. + const successor = loader.run(async () => 'fresh'); + expect(await successor).toBe('fresh'); + + // Now resolve the (ignored) predecessor late. + predecessorResolve('stale'); + expect(await predecessor).toBeNull(); + }); + + it('returns null on AbortError without rethrowing', async () => { + const loader = new CancellableLoader(); + // Start a never-resolving call. We can fire its abort by + // starting a fresh run() — the AbortError swallow is what + // we're pinning here. Crucially, DON'T await `pending` before + // starting the successor; the await must come after. + const pending = loader.run(({ signal }) => rejectsOnAbort(signal)); + await Promise.resolve(); + const successor = loader.run(async () => 'B'); + expect(await pending).toBeNull(); + await successor; + }); + + it('propagates non-AbortError rejections', async () => { + const loader = new CancellableLoader(); + const err = new Error('network kaboom'); + await expect(loader.run(async () => { throw err; })).rejects.toBe(err); + }); + + it('isOwnerOf is true only for the current in-flight call', async () => { + const loader = new CancellableLoader(); + // No call in flight: every signal is non-owner. + expect(loader.isOwnerOf(new AbortController().signal)).toBe(false); + + let signal: AbortSignal | null = null; + const pending = loader.run(({ signal: s }) => { + signal = s; + return rejectsOnAbort(s); + }); + await Promise.resolve(); + + // Mid-flight: the active call's signal is the owner. + expect(loader.isOwnerOf(signal!)).toBe(true); + // A stranger signal is not. + expect(loader.isOwnerOf(new AbortController().signal)).toBe(false); + + // Starting a successor flips ownership. + const successor = loader.run(async () => 'next'); + expect(loader.isOwnerOf(signal!)).toBe(false); + await pending; + await successor; + }); + + it('cancel() aborts the current call and leaves the slot empty', async () => { + const loader = new CancellableLoader(); + let signal: AbortSignal | null = null; + const pending = loader.run(({ signal: s }) => { + signal = s; + return rejectsOnAbort(s); + }); + await Promise.resolve(); + expect(signal!.aborted).toBe(false); + + loader.cancel(); + expect(signal!.aborted).toBe(true); + expect(await pending).toBeNull(); + // After cancel, isOwnerOf is false for the old signal — slot + // empty, no successor queued. + expect(loader.isOwnerOf(signal!)).toBe(false); + }); + + it('rapid burst of three calls: only the last resolves', async () => { + // The exact debounce-race shape the dashboard's search box used + // to suffer from. Three back-to-back calls; only the third's + // value wins; predecessors return null. + const loader = new CancellableLoader(); + + const a = loader.run(({ signal }) => rejectsOnAbort(signal)); + await Promise.resolve(); + const b = loader.run(({ signal }) => rejectsOnAbort(signal)); + await Promise.resolve(); + const c = loader.run(async () => 'WIN'); + + const [ra, rb, rc] = await Promise.all([a, b, c]); + expect(ra).toBeNull(); + expect(rb).toBeNull(); + expect(rc).toBe('WIN'); + }); + + it('cleanup: late re-call after a no-cancellation completion finds an empty slot', async () => { + const loader = new CancellableLoader(); + const v = await loader.run(async () => 'first'); + expect(v).toBe('first'); + // No predecessor abort fires on the second run; ownership flows. + const v2 = await loader.run(async () => 'second'); + expect(v2).toBe('second'); + }); +}); diff --git a/frontend/src/lib/util/cancellable.ts b/frontend/src/lib/util/cancellable.ts new file mode 100644 index 0000000..bc51276 --- /dev/null +++ b/frontend/src/lib/util/cancellable.ts @@ -0,0 +1,95 @@ +/** + * A loader that cancels its in-flight predecessor on each call. + * + * Background — 0.87.9 added per-loader AbortControllers to the admin + * analysis dashboard so a debounced search-burst couldn't race itself + * (slow first response overwriting a faster second). The 0.87.16 + * rereview noted that the contract (predecessor-abort + result- + * ownership check + spinner guard) was only inlined into a single + * Svelte component and untested. + * + * Lift the four invariants into a reusable, testable helper: + * + * 1. **Predecessor abort.** Every `run()` aborts whatever came before + * via the prior AbortController. + * 2. **Late-result suppression.** A predecessor whose fetch finishes + * AFTER the successor's run() started must not surface its value + * — `run()` returns `null` ("superseded") on that path so the + * caller can skip the state update. + * 3. **AbortError swallowed.** Aborts are normal cancellation, not + * errors to toast; `run()` returns `null` rather than rethrowing. + * 4. **Ownership-aware finally.** Whichever call still owns the + * controller clears `this.current` on completion; a later call + * that overwrote it leaves the marker alone (or the spinner would + * flicker on the older call's abort). + * + * Usage: + * const loader = new CancellableLoader(); + * const result = await loader.run(({ signal }) => + * fetch(url, { signal }).then(r => r.json()) + * ); + * if (result === null) return; // superseded or aborted + * // …apply result… + * + * Note the `null` outcome is INTENTIONAL — the caller MUST decide + * whether to flip its spinner off (yes if no other call is in flight, + * no if a successor is). The `isOwnerOf` helper makes that decision + * explicit: + * if (loader.isOwnerOf(signal)) loading = false; + */ +export class CancellableLoader { + private current: AbortController | null = null; + + /** + * Run `fn` with a fresh AbortController, aborting any prior in- + * flight call. Returns the resolved value, or `null` when this + * call was superseded mid-flight OR fn rejected with AbortError. + * Other errors propagate. + */ + async run(fn: (ctx: { signal: AbortSignal }) => Promise): Promise { + this.current?.abort(); + const ctrl = new AbortController(); + this.current = ctrl; + try { + const value = await fn({ signal: ctrl.signal }); + // Successor may have replaced us between the await above + // and here. Drop the result silently — the caller should + // not write stale data over the successor's state. + if (this.current !== ctrl) return null; + return value; + } catch (e) { + // AbortError is normal cancellation; non-Abort errors + // (network, 5xx, JSON parse) propagate to the caller. + if ((e as { name?: string } | null | undefined)?.name === 'AbortError') { + return null; + } + throw e; + } finally { + // Only clear the marker if this call still owns it. A + // predecessor that lost ownership leaves the successor's + // controller untouched. + if (this.current === ctrl) this.current = null; + } + } + + /** + * True iff the given signal belongs to the current in-flight call. + * Use from the caller's finally{} to decide whether to flip + * loading=false. Reading `signal.aborted` directly works for the + * abort case but not for the superseded-without-abort case (only + * `null` from `run()` covers both). + */ + isOwnerOf(signal: AbortSignal): boolean { + return this.current?.signal === signal; + } + + /** + * Abort the current in-flight call without queueing a successor. + * Used from onDestroy so a late response can't write into a dead + * page. + */ + cancel(): void { + this.current?.abort(); + this.current = null; + } +} diff --git a/frontend/src/routes/admin/analysis/+page.svelte b/frontend/src/routes/admin/analysis/+page.svelte index 1a61b8b..ff3248a 100644 --- a/frontend/src/routes/admin/analysis/+page.svelte +++ b/frontend/src/routes/admin/analysis/+page.svelte @@ -17,6 +17,7 @@ type AnalysisEvent } from '$lib/api/admin'; import { chapterLabel } from '$lib/api/chapters'; + import { CancellableLoader } from '$lib/util/cancellable'; import CoverageBadge from '$lib/components/CoverageBadge.svelte'; import Modal from '$lib/components/Modal.svelte'; import AnalysisHistoryTable from '$lib/components/analysis/AnalysisHistoryTable.svelte'; @@ -71,11 +72,14 @@ let live = $state(false); let source: EventSource | null = null; let sseErrors = 0; - // Per-loader AbortController so a debounced search-keystroke burst - // (or unmount) can cancel its predecessor's response — without this, - // a slow first request would overwrite a faster second one, mirroring - // the issue the crawler page already solved at line 55. - let coverageAbort: AbortController | null = null; + // Per-loader cancellation guard. A debounced search-keystroke burst + // (or unmount) cancels its predecessor's response — without this, + // a slow first request would overwrite a faster second one. + // `CancellableLoader` lifts the four invariants (predecessor abort, + // late-result suppression, AbortError swallow, ownership-aware + // finally) into $lib/util/cancellable, where they're unit-tested. + // The inlined version in 0.87.9 was structurally untested. + const coverageLoader = new CancellableLoader(); // Global "now analyzing" pointer, driven by the `started` SSE event so // the operator sees what the worker is on even when nothing is @@ -135,7 +139,7 @@ if (clearTimer) clearTimeout(clearTimer); // Cancel any in-flight coverage fetch so a late response can't // overwrite state after the page is gone. - coverageAbort?.abort(); + coverageLoader.cancel(); }); function pushTicker(text: string) { @@ -342,35 +346,36 @@ loadingMore = true; } error = null; - // Cancel any in-flight predecessor — a debounced search burst - // would otherwise race; the slow first response could overwrite - // the faster second result. - coverageAbort?.abort(); - const ctrl = new AbortController(); - coverageAbort = ctrl; try { - const r = await getAnalysisMangaCoverage( - { - search: query.trim() || undefined, - limit: LIMIT, - offset - }, - { signal: ctrl.signal } + const r = await coverageLoader.run(({ signal }) => + getAnalysisMangaCoverage( + { + search: query.trim() || undefined, + limit: LIMIT, + offset + }, + { signal } + ) ); + // `null` = superseded by a successor call, or AbortError. + // Skip state updates AND leave the spinner alone — the + // successor (which is still in-flight) owns it. Without + // this guard, an unconditional `loading = false` in a + // finally{} would clear the spinner while the successor + // is still loading, producing a flicker. + if (r === null) return; mangas = reset ? r.items : [...mangas, ...r.items]; total = r.page.total ?? mangas.length; + loading = false; + loadingMore = false; } catch (e) { - // Aborts are normal cancellation, not errors to surface. - if ((e as { name?: string })?.name === 'AbortError') return; error = e instanceof ApiError ? e.message : 'Failed to load coverage.'; - } finally { - // Only clear the in-flight markers if this call still owns - // them — a later call may have superseded us already. - if (coverageAbort === ctrl) coverageAbort = null; - if (!ctrl.signal.aborted) { - loading = false; - loadingMore = false; - } + // Non-Abort error: this call genuinely failed and OWNS + // (any successor would have flipped run() to null before + // throwing). Clear the spinner so the error message is + // readable. + loading = false; + loadingMore = false; } }