fix(appkit): decouple cache in-flight execution from per-caller abort…#398
Open
MarioCadenas wants to merge 1 commit into
Open
fix(appkit): decouple cache in-flight execution from per-caller abort…#398MarioCadenas wants to merge 1 commit into
MarioCadenas wants to merge 1 commit into
Conversation
… signal The cache in-flight deduplication map shared a single Promise across concurrent callers with the same cacheKey. When one caller's signal aborted (e.g. a React StrictMode mount/cleanup pair), fn()'s rejection cascaded through the shared promise to every other awaiter — including still-connected SSE consumers, which broadcast the abort as UPSTREAM_ERROR to the browser. Replace the bare promise map with a reference-counted in-flight entry owning its own AbortController. Callers pass their own callerSignal; the shared controller aborts only when every caller has bailed (refCount -> 0). Each caller's await is raced against its own signal so local aborts reject locally without poisoning the shared result. The catch block no longer wraps abort errors as ExecutionError.statementFailed when the shared controller has already aborted, since no live awaiter would observe them anyway. CacheInterceptor forwards context.signal as callerSignal and swaps context.signal to the cache's shared signal for the duration of fn() so the inner interceptor chain (timeout/retry/telemetry) and the underlying I/O (e.g. analytics SDK calls) observe the shared lifecycle rather than the per-request stream signal. Existing cache + plugin + stream + analytics test suites pass unchanged (715 tests). Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Under React 19
<StrictMode>, navigating to/analyticsin dev surfaced this error in the browser console:Root cause
CacheManager.getOrExecutededupes concurrent callers with the samecacheKeyby sharing a singlePromiseininFlightRequests. Under StrictMode:useAnalyticsQueryfires a request → server registers an in-flight promise for the query'scacheKey.fetch→ server'sres.on("close")aborts the stream'sAbortControllerwith"Client disconnected"→ the SDK'sexecutor.query(..., signal)rejects withStatement failed: The operation was aborted.(anExecutionErrorcarryingstatusCode).stream-manager._processGeneratorInBackgroundcatches that rejection on mount 2's stream entry (which still has a connected client), categorizes it asUPSTREAM_ERROR(becausestatusCodeis set anderror.name !== "AbortError"), and broadcasts it to the browser.Smoking-gun log from the catch on mount 2's stream:
{"clientsRemaining": 1, "errorCode": "UPSTREAM_ERROR", "abortReason": }
clientsRemaining: 1+ noabortReasonproves the abort came in via the shared in-flight promise, not via this stream's own controller.This is impossible without StrictMode (only one mount → no inflight join) and impossible on the reconnect view (different code path, no
CacheInterceptor).Fix
Reference-count the cache's in-flight execution behind a cache-owned
AbortController:inFlightRequestsnow holds{ promise, refCount, sharedController }instead of barePromises.getOrExecute(key, fn, userKey, options)acceptsoptions.callerSignaland passes a cache-ownedsharedController.signalintofn(sharedSignal). The shared controller is aborted only whenrefCountdrops to 0 — i.e. every caller has bailed._waitWithRefCount(entry, callerSignal)races the entry's promise against each caller's own signal. A caller-side abort rejects that caller's promise locally and decrementsrefCount; other still-connected awaiters continue to share the underlying execution.ExecutionError.statementFailedwhen the shared controller has already aborted (no live awaiter would observe them anyway), andentry.promise.catch(() => {})suppresses unhandled-rejection warnings when every caller leaves beforefn()resolves.CacheInterceptorforwardscontext.signalascallerSignaland swapscontext.signalto the cache'ssharedSignalfor the duration offn()so the inner interceptor chain (timeout/retry/telemetry) and the underlying I/O (e.g. analyticsexecutor.query(..., signal)) observe the shared lifecycle instead of the per-request stream signal.External callers (e.g.
cache.getOrExecute(key, async () => {...}, userKey)) keep working unchanged because the newsharedSignalparameter is optional.Test plan
pnpm exec vitest run --project appkit src/cache src/plugin src/stream src/plugins/analytics— 715 tests, all green.apps/dev-playground: load/analyticsunder<StrictMode>— noUPSTREAM_ERRORin console; tiles load with data.telemetry-example-plugin.ts's directcache.getOrExecuteuse without a signal).