fix(appkit): decouple cache in-flight execution from per-caller abort…#398
Open
MarioCadenas wants to merge 5 commits into
Open
fix(appkit): decouple cache in-flight execution from per-caller abort…#398MarioCadenas wants to merge 5 commits into
MarioCadenas wants to merge 5 commits 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>
…atures - cache-manager.test.ts — add 5 abort/ref-counting tests covering: caller abort while another waits, all callers abort triggers shared abort, pre-aborted signal rejects immediately, single caller abort mid-flight, deduped caller abort doesn't poison first caller - cache-manager.test.ts — regression test for fn ignoring signal param - cache.test.ts — MockCacheManager passes shared signal to fn(), add signal-swap test verifying context.signal is swapped during execution and restored afterward - 16 plugin test files — update stale getOrExecute mock signatures from fn: () => Promise<T> to fn: (signal?: AbortSignal) => Promise<T> Signed-off-by: Pawel Kosiec <pawel.kosiec@databricks.com>
pkosiec
reviewed
May 22, 2026
Member
pkosiec
left a comment
There was a problem hiding this comment.
Tested this PR and it works! 👍
- Skip joining in-flight entries whose sharedController is already aborted - Guard finally-block delete to only remove its own entry, not a replacement - Remove early span.end() in dedup path (let outer finally handle it) - Add edge-case tests: fn rejection, post-resolve abort, fresh-after-abort Signed-off-by: Pawel Kosiec <pawel.kosiec@databricks.com>
When all callers leave an in-flight cache entry, delay the sharedController.abort() by 100ms instead of firing immediately. This allows React StrictMode remounts to join the existing execution before it is cancelled, preventing unnecessary SQL query aborts and error logs in development. - Add abortTimer to InFlightEntry with 100ms grace period - Clear abort timer when a new caller joins the dedup path - Skip joining aborted entries (guard against expired grace period) - Guard finally-block delete to only remove own entry - Remove early span.end() in dedup path - Add grace period tests + edge-case abort tests Signed-off-by: Pawel Kosiec <pawel.kosiec@databricks.com>
StreamManager.abort() was passing string reasons ("All clients
disconnected", "Server shutdown") which caused _categorizeError to
misclassify client disconnects as INTERNAL_ERROR instead of
STREAM_ABORTED. Use DOMException with name "AbortError" so the
error is correctly categorized and logged at info level.
Signed-off-by: Pawel Kosiec <pawel.kosiec@databricks.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).