Add withSignal to reactive action store for per-dispatch cancellation#1614
Add withSignal to reactive action store for per-dispatch cancellation#1614mcintyre94 wants to merge 1 commit into
withSignal to reactive action store for per-dispatch cancellation#1614Conversation
🦋 Changeset detectedLatest commit: 4d72b50 The changes in this PR will be included in the next version bump. This PR includes changesets to release 47 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BundleMonFiles updated (16)
Unchanged files (131)
Total files change +3.68KB +0.7% Final result: ✅ View report in BundleMon website ➡️ |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds perRequestSignal / perConnectionSignal factory options to the reactive stores so callers can compose a fresh signal per dispatch / per connection (e.g. () => AbortSignal.timeout(N)) while still expressing a kill switch by returning the same cancellable signal each call. Internally rewires reactive-stream-store.ts to use AbortSignal.any for signal composition (replacing the manual outer/inner forwarder), preserves the existing abortSignal option with @deprecated, and polyfills AbortSignal.any in the shared jsdom test environment. Changeset is minor for both packages.
The overall shape is good — the factory API is a clean generalization over a single signal, and the action/stream stores end up with parallel mental models. The internal refactor of connect() is genuinely simpler than the previous outerController + forwardAbort dance.
Key things to look at
- Possible test bug in
reactive-action-store-test.ts—'lets later dispatches recover when the factory returns a fresh signal each call'. The first dispatch supplies an already-abortedperRequestSignal, butfn = () => Promise.resolve('ok')doesn't read the signal and resolves successfully. The post-awaitcheck isif (controller.signal.aborted)— only the per-dispatch controller, not the combined signal — so the dispatch falls intosetState({ status: 'success' }). I'd expect the assertionexpect(store.getState().status).toBe('error')to fail. Either the test needs anfnthat actually rejects onsignal.aborted, or the implementation should also surface aborts on the combined signal. Worth running this one in isolation to confirm. (Left an inline comment.) - Asymmetry between
perRequestSignalandperConnectionSignalreturn types. Action-store factory returnsAbortSignal | undefined; stream-store factory returnsAbortSignal. Both are defensible but it's inconsistent in the public surface — easy to unify onAbortSignal | undefinedfor both, with the stream-store'sconnect()already handling theundefinedbranch viaif (perConnection) signals.push(perConnection). PendingRpcSubscriptionsRequest.reactiveStoreisn't migrated. The wiring inpackages/rpc-subscriptions-spec/src/rpc-subscriptions.tsstill passes{ abortSignal }through tocreateReactiveStoreFromDataPublisherFactory. Backwards-compatible, but the symmetric ergonomic win for subscription consumers (per-connection timeouts on RPC subs) isn't actually exposed end-to-end here — only the action-sidePendingRpcRequest.reactiveStoregot the new option. Intentional follow-up?@deprecated abortSignalis still load-bearing inside the repo. The deprecation is correct but worth being aware that the only first-party caller (rpc-subscriptions-spec) still uses it, so the deprecation warning will fire in our own builds the moment anyone enables@typescript-eslint/no-deprecatedor similar.
Notes for subsequent reviewers
- The behavioral change in the action-store catch block is subtle and worth a second look: previously
if (signal.aborted) throw signal.reasoncovered both supersession and any external abort becausesignalwas justcontroller.signal. Nowsignalis the combined signal, so the post-await check was correctly narrowed tocontroller.signal.abortedonly — that's howperRequestSignalaborts get surfaced aserrorstate rather than silently dropped. The comments document this well, just confirm the intent matches your mental model. - The
FactoryConfigdiscriminated union ({ abortSignal; perConnectionSignal? } | { abortSignal?; perConnectionSignal }) requires at least one of the two to be present at the type level, which is what we want.{}correctly fails to typecheck. Good. - The jsdom
AbortSignal.anypolyfill is scoped to a single static-method patch on jsdom's existing class — this is the right call (cross-realm replacement would break jsdom's brand checks onaddEventListener({ signal }), as the comment notes). The implementation looks correct, including the early-abort short-circuit and{ once: true, signal: controller.signal }cleanup so input listeners go away once the combined signal aborts. AbortSignal.anycleanup semantics: confirmed thesignalsarray passed intoAbortSignal.anydoesn't retain listener references after the combined signal aborts — so retries don't leak listeners onto a long-livedlongLivedAbortSignalacross manyretry()calls. That was the main concern with the oldforwardAbortapproach and it's resolved here.
| it('lets later dispatches recover when the factory returns a fresh signal each call', async () => { | ||
| expect.assertions(2); | ||
| const signals = [AbortSignal.abort(new Error('first')), new AbortController().signal]; | ||
| const store = createReactiveActionStore(() => Promise.resolve('ok'), { | ||
| perRequestSignal: () => signals.shift(), | ||
| }); | ||
| await store.dispatchAsync().catch(() => {}); | ||
| expect(store.getState().status).toBe('error'); | ||
| await store.dispatchAsync(); | ||
| expect(store.getState()).toStrictEqual({ | ||
| data: 'ok', | ||
| error: undefined, | ||
| status: 'success', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I think this test as written should fail. Tracing through:
- First dispatch:
signals.shift()returnsAbortSignal.abort(new Error('first'))— already aborted. AbortSignal.any([controller.signal, abortedSignal])returns an already-aborted combinedsignal.fn = () => Promise.resolve('ok')— never readssignal, just resolves to'ok'.- Post-
awaitcheck isif (controller.signal.aborted)—controllerwas just created and hasn't been aborted, so this isfalse. - Falls through to
setState({ ..., status: 'success' }).
So the first expect(...).toBe('error') should be 'success'. Either:
- the
fnneeds to actually honor the signal (e.g.(signal) => signal.aborted ? Promise.reject(signal.reason) : Promise.resolve('ok')), making this a real test of the recovery path, or - the implementation should also check the combined
signal.abortedpost-await and surface that as an error (which would be a behavior change — currently only the per-dispatch controller can produce that throw).
Could you double-check this one runs green locally? If it does I'm missing something.
There was a problem hiding this comment.
@trevor-cortex check the updated implementation of dispatchAsync in createReactiveActionStore:
kit/packages/subscribable/src/reactive-action-store.ts
Lines 148 to 174 in a0c59f8
To clarify, the test does pass in CI and locally
Here's what I think is happening:
- Signal is set to an aborted signal because
perRequestis aborted:const signal = perRequest ? AbortSignal.any([controller.signal, perRequest]) : controller.signal; - The abortable promise with signal throws:
const result = await getAbortablePromise(fn(signal, ...args), signal); - In the catch block, we haven't aborted the controller (just per-request), so:
if (controller.signal.aborted)is false - We fall through to setting the error:
// Real failure or `perRequestSignal` firing — surface as error state.
setState({ data: previousData, error, status: 'error' });
throw error;Can you double check I've got all this right?
There was a problem hiding this comment.
You're right, my mistake — I missed that fn(signal) is wrapped in getAbortablePromise(..., signal), which is what makes the already-aborted combined signal reject even though fn itself ignores it. Tracing again:
signals.shift()→ already-abortedAbortSignal.abort(new Error('first')).signal = AbortSignal.any([controller.signal, perRequest])is already aborted.getAbortablePromise(fn(signal), signal)rejects withsignal.reasonbecausesignal.abortedis true —fnresolving to'ok'doesn't matter, the race is decided by the signal.- Catch block:
controller.signal.abortedis false (onlyperRequestaborted), so we fall through tosetState({ status: 'error' }). ✅
Second dispatch then gets a fresh non-aborted signal and succeeds. So the test is correct and exercises exactly the recovery path it claims to. Sorry for the noise — that was the piece I was missing.
| function connect() { | ||
| if (outerController.signal.aborted) return; | ||
| // Inner signal is passed to data publisher | ||
| if (longLivedAbortSignal?.aborted) return; | ||
| // Factory called per connection — fresh signal per attempt for `perConnectionSignal`, | ||
| // same kill-able signal returned on every call for the kill-switch pattern. |
There was a problem hiding this comment.
Sanity-checking the deprecated abortSignal path under the new wiring: previously, aborting the caller's abortSignal propagated through outerController → forwardAbort → innerController, which removed the publisher listeners (since they were { signal: innerController.signal }). Now longLivedAbortSignal is one of the inputs to AbortSignal.any, so the combined signal aborts when the caller aborts, and the publisher listeners (registered with { signal }) are removed the same way. ✓
One difference worth confirming: in the old code, aborting the caller's abortSignal would also propagate to innerController directly, so any code reading innerController.signal.aborted would see true. The new code reads the combined signal everywhere instead, which is correct — but if any future code path branches on innerController.signal.aborted specifically (e.g. "was this aborted by the error channel, vs by the caller?"), that signal stays false on caller-driven aborts now. Not a problem today, just a heads-up for whoever maintains this next.
There was a problem hiding this comment.
This is intentional, they're now just treated as independent signals
| JsdomAbortSignal.any = function any(signals: readonly AbortSignal[]): AbortSignal { | ||
| const controller = new JsdomAbortController(); | ||
| const alreadyAborted = signals.find(s => s.aborted); | ||
| if (alreadyAborted) { | ||
| controller.abort(alreadyAborted.reason); | ||
| return controller.signal; | ||
| } | ||
| for (const inputSignal of signals) { | ||
| inputSignal.addEventListener( | ||
| 'abort', | ||
| function () { | ||
| if (!controller.signal.aborted) controller.abort(this.reason); | ||
| }, | ||
| { once: true, signal: controller.signal }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Polyfill looks correct. Two small things:
- Inside the listener,
this.reasonrelies onthisbeing bound to the input signal that fired the event. That works becauseaddEventListenerinvokes listeners withthis === currentTarget, but it's the kind of thing TypeScript-strict reviewers tend to flag.event.currentTarget(or capturing the input signal in the closure) is slightly more explicit. Style call. JsdomAbortControlleris typed astypeof globalThis.AbortController— fine for the polyfill, but note that thecontroller.signalreturned here is a jsdomAbortSignal. That's correct (we want it to pass jsdom's brand checks), just worth a mental note if anyone debugs realm issues later.
|
Documentation Preview: https://kit-docs-dxql0otuv-anza-tech.vercel.app |
mcintyre94
force-pushed
the
reactive-store/abort-signal
branch
from
a0c59f8 to
b04eae2
Compare
|
This PR has been simplified to only add |
mcintyre94
changed the title
perRequestSignal / perConnectionSignal factory options to reactive storesperRequestSignal factory option to reactive action store
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
LGTM (commenting since I can't approve). Scope tightened nicely — stream-store changes deferred to the follow-up, so this PR is now just the action-store perRequestSignal factory plus the wiring through PendingRpcRequest.reactiveStore and the jsdom AbortSignal.any polyfill. The factory shape is a clean generalization: per-attempt timeouts via () => AbortSignal.timeout(N) and kill-switch semantics via () => killCtrl.signal both fall out of the same primitive.
From my prior review, the three substantive points are resolved/scoped out:
- The test I flagged (
'lets later dispatches recover when the factory returns a fresh signal each call') is correct —getAbortablePromise(fn(signal), signal)races against the combined signal, so an already-abortedperRequestSignalrejects the await even whenfnitself doesn't read the signal. Thanks for the walkthrough. - The
perRequestSignal/perConnectionSignalreturn-type asymmetry is moot now thatperConnectionSignalisn't in this PR. PendingRpcSubscriptionsRequest.reactiveStorenot being migrated is explicitly deferred to the upstack stream-store PR.
Notes for subsequent reviewers
- The narrowing in the catch block from
signal.abortedtocontroller.signal.abortedis the load-bearing behavior change: previouslysignalwas just the per-dispatch controller, so it covered supersession only; nowsignalis the combined signal, and the post-await check is correctly narrowed tocontroller.signalso thatperRequestSignalaborts surface aserrorstate rather than being silently dropped as supersession. The inline comments document this well. - The jsdom
AbortSignal.anypolyfill is scoped to a single static-method patch on jsdom's existing class (cross-realm replacement would break jsdom's brand checks foraddEventListener({ signal }), as the comment notes). Implementation is correct: early-abort short-circuit, and{ once: true, signal: controller.signal }so input-signal listeners are cleaned up once the combined signal aborts — no listener leak on long-livedperRequestSignals across many dispatches. - Changeset is
minorfor both@solana/subscribableand@solana/rpc-spec; both are pre-1.0 so reviewers should treat that as breaking-equivalent per the repo's convention.
I think the line about pre-1.0 in our claude.md is confusing trevor, I've seen this locally too. But to clarify, minor because non breaking! |
lorisleiva
left a comment
lorisleiva
left a comment
There was a problem hiding this comment.
Hmm I'm not a massive fan of how decoupled the AbortSignal is from the actual dispatch call. It feels like a significant indirection when ideally you'd want dispatch(...args, { signal }). Could you tell me what we'd be missing if we made each dispatchable function responsible for their own AbortSignal? That is, they must add them explicitly to their function signature.
|
@lorisleiva Agreed, thanks for the suggestion! This design is similar to what we have in the stream store ATM, which is an But I think it's better to add The only downside is that if you do want the same kill-switch signal, or the same signal factory, then you need to repeat it on every dispatch. But it's definitely more obvious what's going on! Will update this. |
mcintyre94
force-pushed
the
reactive-store/abort-signal
branch
from
b04eae2 to
1c2b3e9
Compare
mcintyre94
force-pushed
the
graphite-base/1614
branch
from
6f0326e to
000d8bb
Compare
|
I went with the fluent helper idea to cleanly separate the signal from the args: This is non-breaking, The |
|
Worth noting the API has already moved in this direction since the PR description was written — the current head no longer exposes a
So modulo shape, this is pretty close to For
Against:
On the alternative you're floating (each dispatchable function responsible for its own signal, no store-level plumbing): that works for the bare So I think the real question to weigh is whether the bind-once ergonomic + the variadic-tuple-friendliness of |
mcintyre94
force-pushed
the
reactive-store/abort-signal
branch
from
1c2b3e9 to
3471172
Compare
mcintyre94
changed the base branch from
graphite-base/1614
to
reactive-store/no-auto-dispatch
mcintyre94
changed the title
perRequestSignal factory option to reactive action storewithSignal to reactive action store for per-dispatch cancellation
|
With #1628 this PR is simplified, and no longer needs to change the @trevor-cortex Please do a fresh code review of this PR as-is. Earlier comments refer to a previous version of the API |
|
Fresh review — LGTM (commenting since I don't have write access). Scope is tight and the API is materially cleaner than where this started. What changed since the prior round
Behavior trace (for the record)The two abort-check sites in
This was the load-bearing thing I asked about in the prior round and it's still correct under the simplified shape. Notes for subsequent reviewers
|
mcintyre94
force-pushed
the
reactive-store/abort-signal
branch
from
3471172 to
f22b562
Compare
mcintyre94
force-pushed
the
reactive-store/abort-signal
branch
2 times, most recently
from
674c112 to
f2bb5d1
Compare
mcintyre94
force-pushed
the
reactive-store/no-auto-dispatch
branch
from
23bc249 to
5fac7a4
Compare
| const JsdomAbortSignal = this.global.AbortSignal as typeof globalThis.AbortSignal & { | ||
| any?: typeof globalThis.AbortSignal.any; | ||
| }; | ||
| const JsdomAbortController = this.global.AbortController as typeof globalThis.AbortController; |
There was a problem hiding this comment.
Should we consider bumping jsdom to version 24 in a subsequent PR?
…tion
`ReactiveActionStore` now exposes `withSignal(signal)` — a thin wrapper that returns `{ dispatch, dispatchAsync }` bindings composing the caller's signal with the store's internal per-dispatch controller via `AbortSignal.any`. Aborting either cancels the in-flight call and surfaces the abort reason on state. The bare `dispatch` / `dispatchAsync` signatures are unchanged, so this is additive — no existing caller breaks.
The wrapper supports two patterns. Per-attempt timeout: `store.withSignal(AbortSignal.timeout(5_000)).dispatch(args)` — a fresh clock per call, with different call sites free to pass different timeouts without rebuilding the store. Shared kill switch: hold one `AbortController`, bind the wrapper once (`const killable = store.withSignal(killCtrl.signal)`), use `killable.dispatch(...)` everywhere; aborting the controller cancels the in-flight call and short-circuits future calls on that wrapper.
`PendingRpcRequest.reactiveStore()` (and the `ReactiveActionSource` duck-type) also accepts an optional `{ signal?: AbortSignal }` so a caller-provided cancellation source can be attached to the initial dispatch fired implicitly by `reactiveStore()` — same role `abortSignal` plays for `send()`. The `signal` is not a store-level setting; subsequent dispatches on the returned store go through `store.withSignal(...).dispatch(...)` like any other store.
The shared `@solana/test-config` browser environment polyfills `AbortSignal.any` because jsdom 22 (the version pinned here) doesn't ship it. Replacing the AbortSignal class wholesale would break jsdom's brand checks for `addEventListener({ signal })`, so the patch is limited to the missing static method.
mcintyre94
force-pushed
the
reactive-store/no-auto-dispatch
branch
from
5fac7a4 to
593face
Compare
mcintyre94
force-pushed
the
reactive-store/abort-signal
branch
from
f2bb5d1 to
4d72b50
Compare
3 participants
Summary of Changes
Previously I didn't add an abort signal to the
ReactiveActionStore, but I've realised that as this can be used with egPendingRpcRequestit is useful for consumers to be able to control the abort logic - eg to passAbortSignal.timeout(N).I think it makes sense for this signal to be per dispatch, ie not a single abort signal passed into the store on create.
This PR adds an optional API:
store.withSignal(signal).dispatch(...)to calldispatchordispatchAsyncwith an additional abort signal. It's combined with the store controller signal usingAbortSignal.any.store.dispatch(...)andstore.dispatchAsync(...)remain unchanged.The same per-request logic also applies for the
ReactiveStreamStore, this will be addressed in a later PR.Note that we use
AbortSignal.anyfor combining signals. This required Claude to polyfill it for jsdom because Jest, but it's widely supported and is in Baseline 2024. Much better than the signal juggling we have in the stream store (which will be refactored in the upstack PR too)