diff --git a/backend/Cargo.lock b/backend/Cargo.lock index a8a5899..86645f3 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1517,7 +1517,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.87.23" +version = "0.87.24" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 2e601b8..52bb837 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.87.23" +version = "0.87.24" edition = "2021" default-run = "mangalord" diff --git a/frontend/package.json b/frontend/package.json index 8d9296f..9f4100f 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.87.23", + "version": "0.87.24", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/lib/util/cancellable.test.ts b/frontend/src/lib/util/cancellable.test.ts index 3db1be8..d990341 100644 --- a/frontend/src/lib/util/cancellable.test.ts +++ b/frontend/src/lib/util/cancellable.test.ts @@ -170,4 +170,63 @@ describe('CancellableLoader', () => { const v2 = await loader.run(async () => 'second'); expect(v2).toBe('second'); }); + + it('suppresses a non-AbortError rejection from a superseded predecessor', async () => { + // Real-world shape: a predecessor's `fetch` survives the abort + // (timing-of-cancel race or an `fn` that ignores `signal`) and + // then rejects with a NON-Abort error — a 5xx, a TypeError, a + // JSON parse failure. Before this fix the rejection bubbled + // out of `run()`, which let the caller's catch-block clear the + // spinner / overwrite `error` for the successor that already + // took over. After the fix, a call that no longer owns + // `this.current` returns `null` regardless of how it died — + // symmetric with the late-resolve suppression at line ~58. + const loader = new CancellableLoader(); + let predecessorReject: (e: unknown) => void = () => {}; + const predecessor = loader.run( + () => + new Promise((_resolve, reject) => { + predecessorReject = reject; + }) + ); + await Promise.resolve(); + // Successor takes ownership. + const successor = loader.run(async () => 'fresh'); + expect(await successor).toBe('fresh'); + // NOW the predecessor's body throws — well after it lost + // ownership. Must NOT propagate to the caller; must resolve + // to null. + predecessorReject(new TypeError('5xx after supersede')); + await expect(predecessor).resolves.toBeNull(); + }); + + it('still propagates a non-AbortError rejection when no successor exists', async () => { + // Regression-guard the other side of the supersede check: a + // standalone call that fails with a real error must still + // throw — otherwise the caller never sees the network/JSON + // failure on the only in-flight request. + const loader = new CancellableLoader(); + const err = new Error('the only call kaboomed'); + await expect(loader.run(async () => { throw err; })).rejects.toBe(err); + }); + + it('swallows AbortError on a call that still owns the controller', async () => { + // After the 0.87.24 strengthening, the supersede check returns + // null FIRST for any call that lost ownership — so the existing + // AbortError-suppression tests (predecessor aborted, cancel() + // fired) hit the supersede branch and never reach the + // AbortError branch. Without this test, removing the + // AbortError swallow entirely would pass the suite. + // + // Real-world path: an underlying library (e.g. fetch backed by + // an upstream cancellation we didn't initiate) surfaces an + // AbortError to `fn` without any successor `run()` or + // `cancel()` having fired. The owning call must still resolve + // to null per invariant #3 in the class docstring. + const loader = new CancellableLoader(); + const result = await loader.run(async () => { + throw new DOMException('aborted', 'AbortError'); + }); + expect(result).toBeNull(); + }); }); diff --git a/frontend/src/lib/util/cancellable.ts b/frontend/src/lib/util/cancellable.ts index bc51276..2da0a5c 100644 --- a/frontend/src/lib/util/cancellable.ts +++ b/frontend/src/lib/util/cancellable.ts @@ -14,8 +14,9 @@ * 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. + * OR its rejection — `run()` returns `null` ("superseded") on + * both paths so the caller never overwrites the successor's + * state with stale data or with an error from the dead call. * 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 @@ -58,8 +59,14 @@ export class CancellableLoader { 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. + // A superseded call returns null regardless of HOW it + // died — Abort, a 5xx, a JSON parse failure, anything. + // Otherwise the caller's catch-block would clear the + // successor's spinner / overwrite its `error`. Symmetric + // with the late-resolve suppression above. + if (this.current !== ctrl) return null; + // AbortError on the still-owning call is normal + // cancellation (e.g. consumer called cancel()); swallow. if ((e as { name?: string } | null | undefined)?.name === 'AbortError') { return null; } diff --git a/frontend/src/routes/admin/analysis/+page.svelte b/frontend/src/routes/admin/analysis/+page.svelte index ff3248a..cdad181 100644 --- a/frontend/src/routes/admin/analysis/+page.svelte +++ b/frontend/src/routes/admin/analysis/+page.svelte @@ -369,11 +369,12 @@ loading = false; loadingMore = false; } catch (e) { + // Reaching this catch means run() rejected for a call + // that still owned the loader — a superseded call would + // have returned null instead (CancellableLoader contract, + // 0.87.24). So this call genuinely failed and is allowed + // to surface its error / clear the spinner. error = e instanceof ApiError ? e.message : 'Failed to load coverage.'; - // 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; }