fix(cancellable): suppress non-Abort errors from superseded predecessors (0.87.24)
CancellableLoader.run() previously swallowed AbortError but propagated every other rejection. A predecessor whose fetch survived its abort (timing race, or fn that ignores signal) and then 5xx'd would bubble the error into the caller's catch, clearing the successor's spinner and writing its `error` field — exactly the flicker the loader was supposed to prevent. Strengthen the contract: a call that no longer owns this.current returns null regardless of how it died. Symmetric with the existing late-resolve suppression at the success path. The loadCoverage catch in admin/analysis/+page.svelte now only fires when this call genuinely failed AND still owns the loader — so clearing the spinner there is sound. Three new unit tests pin all three contract corners: a superseded predecessor's non-Abort rejection resolves to null; a standalone call with no successor still throws normally; the AbortError swallow on a still-owning call (synthetic AbortError from fn with no successor or cancel()) remains protected, since the new supersede check would otherwise have eaten every existing AbortError-path test and left that branch as silently-removable dead code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "mangalord-frontend",
|
||||
"version": "0.87.23",
|
||||
"version": "0.87.24",
|
||||
"private": true,
|
||||
"type": "module",
|
||||
"scripts": {
|
||||
|
||||
@@ -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<string>((_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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user