From d242a018ee45e52dde142473faf8a9153b1aaee8 Mon Sep 17 00:00:00 2001 From: tsushanth <78000697+tsushanth@users.noreply.github.com> Date: Sat, 13 Jun 2026 08:50:02 -0700 Subject: [PATCH 1/2] fix(signals): defer onSettled fallback cleanup to owner disposal (closes #2766) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `onSettled` is called from a children-forbidden scope (most commonly from inside another `onSettled` callback, which runs as a tracked effect), the implementation fell back to enqueueing a one-shot callback on the global queue and invoked the returned cleanup eagerly inside the same flush: globalQueue.enqueue(EFFECT_USER, () => { const cleanup = callback(); … cleanup?.(); }); That immediately tore down helpers whose intent was "install on settle, tear down on dispose" — composable helpers using `onSettled` internally saw their subscription set up and ripped down in the same tick (#2766). Capture the calling owner and, when the callback returns a function, attach it via `cleanup()` under that owner with `runWithOwner`. The cleanup now fires when the captured owner is disposed, matching the contract of the non-forbidden path. Orphaned helpers (no owner at all) drop the cleanup rather than run it immediately — there is no point at which firing it would be correct. Add a regression test in `tests/onSettled.test.ts` that wraps the reporter's pattern (a nested `onSettled` inside an outer `onSettled`) and asserts the subscription stays alive until `dispose()`. --- packages/solid-signals/src/signals.ts | 40 ++++++++++++++----- .../solid-signals/tests/onSettled.test.ts | 27 +++++++++++++ 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/packages/solid-signals/src/signals.ts b/packages/solid-signals/src/signals.ts index 2ba909a3d..67fc5d6b1 100644 --- a/packages/solid-signals/src/signals.ts +++ b/packages/solid-signals/src/signals.ts @@ -745,15 +745,33 @@ export function createOptimistic( */ export function onSettled(callback: () => void | (() => void)): void { const owner = getOwner(); - owner && !(owner._config & CONFIG_CHILDREN_FORBIDDEN) - ? createTrackedEffect(() => untrack(callback), __DEV__ ? { name: "onSettled" } : undefined) - : globalQueue.enqueue(EFFECT_USER, () => { - const cleanup = callback(); - if (__DEV__ && cleanup !== undefined && typeof cleanup !== "function") { - throw new Error( - "onSettled callback returned an invalid cleanup value. Return a cleanup function or undefined." - ); - } - cleanup?.(); - }); + if (owner && !(owner._config & CONFIG_CHILDREN_FORBIDDEN)) { + createTrackedEffect(() => untrack(callback), __DEV__ ? { name: "onSettled" } : undefined); + return; + } + // Fallback path: either no owner, or the current owner is a children- + // forbidden scope (the most common case is `onSettled()` called from + // inside another `onSettled()` callback, which itself runs as a + // tracked effect). The original implementation invoked the returned + // cleanup eagerly inside the same flush, which immediately tore down + // setup helpers (#2766). Defer cleanup via the captured owner's + // disposal chain so it fires on real disposal, not on settle. + const capturedOwner = owner; + globalQueue.enqueue(EFFECT_USER, () => { + const result = callback(); + if (__DEV__ && result !== undefined && typeof result !== "function") { + throw new Error( + "onSettled callback returned an invalid cleanup value. Return a cleanup function or undefined." + ); + } + if (typeof result === "function") { + if (capturedOwner) { + runWithOwner(capturedOwner, () => cleanup(result as Disposable)); + } else { + // No owner to scope the cleanup to — the helper is genuinely + // orphaned, so we have no point at which to fire it. Drop it + // rather than run it immediately and break the setup contract. + } + } + }); } diff --git a/packages/solid-signals/tests/onSettled.test.ts b/packages/solid-signals/tests/onSettled.test.ts index d36c7e089..14be90a03 100644 --- a/packages/solid-signals/tests/onSettled.test.ts +++ b/packages/solid-signals/tests/onSettled.test.ts @@ -224,3 +224,30 @@ it("should call cleanup on re-run after async settles", async () => { // Now callback runs expect(effect).toHaveBeenCalledWith("done"); }); + +it("defers cleanup from nested onSettled to owner disposal (#2766)", () => { + // Helper that uses onSettled internally to install + tear down a + // subscription. The parent calls the helper from inside another + // onSettled — under the bug, the helper's cleanup was invoked eagerly + // in the same flush, immediately tearing down the subscription. + const log: string[] = []; + function useSubscription() { + onSettled(() => { + log.push("subscribed"); + return () => log.push("unsubscribed"); + }); + } + + const dispose = createRoot(stop => { + onSettled(() => { + useSubscription(); + }); + return stop; + }); + + flush(); + expect(log).toEqual(["subscribed"]); + + dispose(); + expect(log).toEqual(["subscribed", "unsubscribed"]); +}); From 32ba5bf694eb30a2261fe6ecd37eb4680d8632d1 Mon Sep 17 00:00:00 2001 From: tsushanth <78000697+tsushanth@users.noreply.github.com> Date: Sat, 13 Jun 2026 08:56:09 -0700 Subject: [PATCH 2/2] fix(signals): surface sync rejection from thenables (closes #2764) --- packages/solid-signals/src/core/async.ts | 16 +++++++-- .../solid-signals/tests/syncThenable.test.ts | 34 +++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/packages/solid-signals/src/core/async.ts b/packages/solid-signals/src/core/async.ts index b2f0c9da2..74e965b6b 100644 --- a/packages/solid-signals/src/core/async.ts +++ b/packages/solid-signals/src/core/async.ts @@ -233,6 +233,8 @@ export function handleAsync( if (isThenable) { let resolved = false, + rejected = false, + syncError: any, isSync = true; (result as PromiseLike).then( v => { @@ -242,11 +244,21 @@ export function handleAsync( } else asyncWrite(v); }, e => { - if (!isSync) handleError(e); + if (isSync) { + // Library/cache thenables can invoke the rejection handler + // synchronously during `.then(…)`. Capturing the error here and + // surfacing it after `.then` returns is what lets `` see + // it instead of being stuck on the pending path forever (#2764). + syncError = e; + rejected = true; + } else handleError(e); } ); isSync = false; - if (!resolved) { + if (rejected) { + handleError(syncError); + throw syncError; + } else if (!resolved) { globalQueue.initTransition(resolveTransition(el as any)); throw new NotReadyError(context!); } diff --git a/packages/solid-signals/tests/syncThenable.test.ts b/packages/solid-signals/tests/syncThenable.test.ts index b6775e5ad..64f4cc04d 100644 --- a/packages/solid-signals/tests/syncThenable.test.ts +++ b/packages/solid-signals/tests/syncThenable.test.ts @@ -1,4 +1,5 @@ import { + createErrorBoundary, createMemo, createRenderEffect, createRoot, @@ -515,3 +516,36 @@ describe("fromObservable pattern", () => { expect(signal()).toBe(42); }); }); + +describe("sync rejecting thenable", () => { + // Helper: a thenable that calls its rejection handler synchronously + function syncRejectingThenable(reason: unknown): PromiseLike { + return { + then( + _onfulfilled?: ((v: never) => R1 | PromiseLike) | null, + onrejected?: ((reason: any) => R2 | PromiseLike) | null + ): PromiseLike { + try { + const result = onrejected ? onrejected(reason) : (reason as any); + return syncThenable(result) as PromiseLike; + } catch (err) { + return syncRejectingThenable(err) as PromiseLike; + } + } + }; + } + + it("reaches Errored on synchronous rejection (#2764)", () => { + const error = new Error("sync-reject"); + const result = createRoot(() => + createErrorBoundary( + () => { + const value = createMemo(() => syncRejectingThenable(error)); + return value(); + }, + (err: () => unknown) => `caught: ${(err() as Error).message}` + ) + ); + expect(result()).toBe("caught: sync-reject"); + }); +});