From 62855059fb2fe6d5ad027fc706d3de07cb59678c Mon Sep 17 00:00:00 2001 From: Josh Story Date: Thu, 16 Apr 2026 10:30:25 -0700 Subject: [PATCH] [Fizz] prevent reentrant finishedTask from calling completeAll multiple times It is possible for the fallback tasks from a Suspense boundary to trigger an early completeAll() call which is later repeated due to finishedTask reentrancy. For node.js in particular this might be problematic since we invoke a callback on each completeAll() call but in general it just isn't the right semantics since the call is running slightly earlier than the completion of the last finishedTask invocation. This change ensures that any reentrant finishedTask calls (due to soft aborting fallback tasks) omit the completeAll() call by temporarily incrementing the total pending tasks. --- .gitignore | 2 +- .../src/__tests__/ReactDOMFizzServer-test.js | 94 +++++++++++++++++++ packages/react-server/src/ReactFizzServer.js | 6 ++ 3 files changed, 101 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 438d125b47a0..16982164c421 100644 --- a/.gitignore +++ b/.gitignore @@ -21,6 +21,7 @@ chrome-user-data .idea *.iml .vscode +.zed *.swp *.swo /tmp @@ -40,4 +41,3 @@ packages/react-devtools-fusebox/dist packages/react-devtools-inline/dist packages/react-devtools-shell/dist packages/react-devtools-timeline/dist - diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 6ff107786f3b..efb7e887cf18 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -6289,6 +6289,100 @@ describe('ReactDOMFizzServer', () => { expect(getVisibleChildren(container)).toEqual('Hi'); }); + // Regression: finishedTask aborting remaining fallback tasks from a + // completed boundary could reenter itself via abortTaskSoft and fire + // onAllReady twice (the inner call drained allPendingTasks to 0 and + // called completeAll, then the outer call re-observed the same 0). + it('only fires onAllReady once when a boundary with an instrumented sync-resolving thenable completes', async () => { + // Mirrors Flight-client chunk behavior: the status-probe .then() in + // trackUsedThenable stays pending, but the ping-attaching .then() in + // renderNode's catch resolves synchronously. This reorders the work + // queue so the fallback task is still in fallbackAbortableTasks when + // the content task completes. + function createDeferredSyncThenable(value) { + let thenCallCount = 0; + return { + status: 'pending', + value: undefined, + then(resolve) { + thenCallCount++; + if (thenCallCount > 1) { + this.status = 'fulfilled'; + this.value = value; + resolve(value); + } + }, + }; + } + + const thenable = createDeferredSyncThenable('hello'); + function AsyncContent() { + return ; + } + + let allReadyCount = 0; + await act(() => { + const {pipe} = renderToPipeableStream( + }> + + , + { + onAllReady() { + allReadyCount++; + }, + }, + ); + pipe(writable); + }); + + expect(allReadyCount).toBe(1); + expect(getVisibleChildren(container)).toEqual('hello'); + }); + + // Same bug, hit without any sync-thenable trickery: if the fallback + // also suspends, its spawned sub-task lives in fallbackAbortableTasks + // and can still be there when the content task completes first. + it('only fires onAllReady once when both content and fallback suspend on real promises', async () => { + let resolveContent; + const contentPromise = new Promise(r => (resolveContent = r)); + // The fallback promise never resolves — the fallback-sub-task gets + // soft-aborted when the content completes, so we never need it. + const fallbackPromise = new Promise(() => {}); + + function AsyncContent() { + return ; + } + function AsyncFallback() { + return ; + } + + let allReadyCount = 0; + await act(() => { + const {pipe} = renderToPipeableStream( + }> + + , + { + onAllReady() { + allReadyCount++; + }, + }, + ); + pipe(writable); + }); + + // Resolving content alone is enough: the fallback-sub-task is still + // in fallbackAbortableTasks when the content task completes, and + // abortTaskSoft on it reenters finishedTask. + await act(async () => { + resolveContent('hello'); + await contentPromise; + }); + + expect(allReadyCount).toBe(1); + expect(getVisibleChildren(container)).toEqual('hello'); + }); + it('promise as node', async () => { const promise = Promise.resolve('Hi'); await act(async () => { diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 0b115199fc3b..c1fc1531d3d9 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -4955,8 +4955,14 @@ function finishedTask( hoistHoistables(boundaryRow.hoistables, boundary.contentState); } if (!isEligibleForOutlining(request, boundary)) { + // abortTaskSoft reenters finishedTask for each aborted task, which + // decrements allPendingTasks. Ensure that these reentrant finsihedTask + // calls do not call `completeAll` too early by forcing the task counter + // above zero for their duration. + request.allPendingTasks++; boundary.fallbackAbortableTasks.forEach(abortTaskSoft, request); boundary.fallbackAbortableTasks.clear(); + request.allPendingTasks--; if (boundaryRow !== null) { // If we aren't eligible for outlining, we don't have to wait until we flush it. if (--boundaryRow.pendingTasks === 0) {