From 22a9f673d34f1df6de66c986e96476cbf63f42b9 Mon Sep 17 00:00:00 2001 From: devgbmuyiwa <142212982+devgbmuyiwa@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:17:19 +0100 Subject: [PATCH] fix(safe-operations): stop giving each withBoundedParallel item a dead AbortSignal Closes #221. lib/safe-operations.ts:164 built the signal handed to each item's handler as `options?.signal ?? new AbortController().signal`. When the caller didn't pass options.signal, this allocated a fresh AbortController per item and immediately discarded it -- nothing ever held a reference to call .abort() on it, so signal.aborted was permanently false and per-item cancellation silently never worked whenever a caller relied on the default (no outer signal). Fix: compute the fallback signal once per withBoundedParallel call, before the worker loop starts, instead of once per item inside it. Every item in a call without an outer signal now receives the same AbortSignal instance -- still not abortable from outside this function (there's no way to do that without changing the return type or exposing the controller, which is out of scope for this bug), but no longer a wasteful, definitionally-dead object manufactured and thrown away per item. Checked the one real call site (components/stream/BulkWithdrawButton.tsx) before choosing this fix over the issue's other suggested option (relaxing the handler's signal parameter to optional/undefined): it always passes its own options.signal and doesn't even use the handler's third parameter, so this change is fully backward compatible with real usage -- the handler type signature (signal: AbortSignal, never undefined) is unchanged. Tests (lib/safe-operations.test.ts): - New: 'shares a single fallback signal across items when no outer signal is provided' -- 3 items, asserts all three receive the exact same AbortSignal instance. This is the direct regression test for the bug: the old code would have given each item a distinct, dead AbortController().signal here. - Existing 'passes the abort signal to individual handlers' and 'passes the same parent signal to handlers when provided' are unchanged and still pass -- they already covered, respectively, that a signal is passed at all, and that the outer-signal case shares one signal across items. - Existing 'stops processing new items when signal is aborted' already covers cancellation reaching handlers in the outer-signal case and is unchanged. Note on the acceptance criteria's "cancellation actually reaches handlers ... in the no-outer-signal case": with the chosen fix (a shared-but-unexposed fallback AbortController), there is no way for anything outside withBoundedParallel to actually trigger that fallback signal's abort -- by design, per the issue's own "at least a caller could theoretically abort" phrasing for this option. I tested the achievable, meaningful half of that criterion (the signal is now real and shared, not dead-per-item) rather than writing a test that fakes an abort event without actually flipping signal.aborted, which would have demonstrated nothing real. Flagging this rather than silently claiming full coverage: if genuine external cancellation of the no-outer-signal fallback is actually required, that needs a small API change (e.g. returning the controller, or accepting an optional onAbort callback) beyond this bug's stated scope. Validation: ran the full lib/safe-operations.test.ts suite (npx vitest run) -- 98 passed, 4 pre-existing todo. Ran npx tsc --noEmit across the project -- one pre-existing error in lib/tokens.test.ts, unrelated to and untouched by this change. Note on assignment: this issue is assigned to devgbmuyiwa under the Stellar Wave Program; this commit is authored as devgbmuyiwa directly, at their request, using previously-granted collaborator access. --- lib/safe-operations.test.ts | 24 ++++++++++++++++++++++++ lib/safe-operations.ts | 13 ++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/lib/safe-operations.test.ts b/lib/safe-operations.test.ts index c2b67b8..7184047 100644 --- a/lib/safe-operations.test.ts +++ b/lib/safe-operations.test.ts @@ -636,6 +636,30 @@ describe('withBoundedParallel', () => { expect(receivedSignal!.aborted).toBe(false); }); + // Regression test for #221: withBoundedParallel used to build + // `new AbortController().signal` inline per item when no outer signal was + // passed, so every handler got its own signal that nothing could ever + // abort. The fix shares one AbortController across the whole batch. + it('shares a single fallback signal across items when no outer signal is provided', async () => { + const items = [1, 2, 3]; + const receivedSignals: AbortSignal[] = []; + + await withBoundedParallel( + items, + async (_item, _index, signal) => { + receivedSignals.push(signal); + return { success: true, data: 1 } as SafeOperationResult; + }, + { maxConcurrency: 1 }, // force sequential execution for a deterministic order + ); + + expect(receivedSignals).toHaveLength(3); + // Every item must receive the exact same AbortSignal instance — not a + // fresh one per item, which is the bug this test guards against. + expect(receivedSignals[1]).toBe(receivedSignals[0]); + expect(receivedSignals[2]).toBe(receivedSignals[0]); + }); + it('passes the same parent signal to handlers when provided', async () => { const controller = new AbortController(); const items = [1, 2]; diff --git a/lib/safe-operations.ts b/lib/safe-operations.ts index b50a00b..866c84d 100644 --- a/lib/safe-operations.ts +++ b/lib/safe-operations.ts @@ -157,11 +157,22 @@ export async function withBoundedParallel( const results: SafeOperationResult[] = []; let index = 0; + // When the caller doesn't pass an outer signal, fall back to a single + // AbortController created once for the whole batch — not one per item. + // A fresh AbortController allocated per item is discarded the instant + // it's created; nothing ever holds a reference to it, so `.abort()` can + // never be called and every handler's signal reads `aborted: false` + // forever, silently defeating per-item cancellation (#221). Sharing one + // controller across the batch at least makes the signal a coherent, + // referenceable object — the same one every item receives — instead of + // a dead one manufactured and thrown away per item. + const fallbackSignal = options?.signal ?? new AbortController().signal; + const worker = async () => { while (index < items.length && !options?.signal?.aborted) { const i = index++; try { - const result = await handler(items[i]!, i, options?.signal ?? new AbortController().signal); + const result = await handler(items[i]!, i, fallbackSignal); results[i] = result; } catch (err) { results[i] = {