Fix navigation getting reverted when a Server Action is in flight (#95391)

Alternative to https://github.com/vercel/next.js/pull/95188. Fixes https://github.com/vercel/next.js/issues/86151#issuecomment-4802297986. This fixes a bug where a navigation would spuriously get reverted once a Server Action settles. ## Test Plan New e2e tests that fail before the fix and pass after the fix. I've steered the agent to cover both the broken case now *and* the case that the regressing PR (#82674) was originally trying to fix. The test itself is reduced from https://github.com/vercel/next.js/pull/95188. Also, verified before/after in a real tiny app based on the tests. Before the fix, it reverts to the previous route; after the fix, it stays on the last route as it should. The tests themselves are a bit wonky and do some timing assertions. I had no luck reducing them further. ## Summary *Note: Everything below is written by Fable.* Fixes a regression where a navigation could get reverted shortly after it committed if a Server Action was still in flight when the navigation started: the destination page renders, then the router snaps back to the previous URL. Regressed in 15.5.1 via #82674. Reported in #86151, with a repro and initial investigation by @errmakov in #95188. ### Why? When a navigation starts while a Server Action is in flight, the router discards the action and the navigation takes its place at the head of the action queue. The discarded action still runs to completion. Since #82674, its completion also called `runRemainingActions`, which advances the queue — but the discarded action is no longer at the head of the queue, the navigation is. This started the next queued action while the navigation was still running, computed against router state that didn't include the navigation yet. Those actions send their requests with the previous URL, and applying their responses reverts the navigation. The promise rejection path had the same problem even before #82674: a discarded action that rejected would also advance the queue. ### How? Only the settlement of the action at the head of the queue may advance the queue. A discarded action's settlement (resolve or reject) now only records whether the action revalidated, and the deferred refresh is flushed once the queue goes idle. This keeps the behavior #82674 was after: no refresh for discarded actions that didn't revalidate, and a refresh for those that did — including when they settle mid-navigation, a case that could previously lose the refresh entirely. --------- Co-authored-by: errmakov <[email protected]>
2026-09-24 17:00:51 +00:00 · 2026-07-02 23:52:24 +01:00
parent b420536fe8
commit db4e6231e1
8 changed files with 367 additions and 15 deletions
@@ -67,25 +67,30 @@ export type ActionQueueNode = {

 function runRemainingActions(
  actionQueue: AppRouterActionQueue,
+  settledAction: ActionQueueNode,
  setState: DispatchStatePromise
 ) {
-  if (actionQueue.pending !== null) {
-    actionQueue.pending = actionQueue.pending.next
+  // Only advance the queue if the settled action is still at its head. If a
+  // navigation discarded this action, the navigation took its place and is
+  // still in flight — starting the next queued action now would run it
+  // against router state that doesn't include the navigation yet.
+  if (actionQueue.pending === settledAction) {
+    actionQueue.pending = settledAction.next
    if (actionQueue.pending !== null) {
      runAction({
        actionQueue,
        action: actionQueue.pending,
        setState,
      })
+      return
    }
-  } else {
-    // Check for refresh when pending is already null
-    // This handles the case where a discarded server action completes
-    // after the navigation has already finished and the queue is empty
-    if (actionQueue.needsRefresh) {
-      actionQueue.needsRefresh = false
-      actionQueue.dispatch({ type: ACTION_REFRESH }, setState)
-    }
+  }
+
+  if (actionQueue.pending === null && actionQueue.needsRefresh) {
+    // The queue is idle; flush the refresh requested by a discarded server
+    // action that revalidated data.
+    actionQueue.needsRefresh = false
+    actionQueue.dispatch({ type: ACTION_REFRESH }, setState)
  }
 }

@@ -117,22 +122,22 @@ async function runAction({
        // mark that we need to refresh after all actions complete
        actionQueue.needsRefresh = true
      }
-      // Still need to run remaining actions even for discarded actions
-      // to potentially trigger the refresh
-      runRemainingActions(actionQueue, setState)
+      // This can't advance the queue (this action is no longer its head), but
+      // if the queue has already drained, it flushes the refresh now.
+      runRemainingActions(actionQueue, action, setState)
      return
    }

    actionQueue.state = nextState

-    runRemainingActions(actionQueue, setState)
+    runRemainingActions(actionQueue, action, setState)
    action.resolve(nextState)
  }

  // if the action is a promise, set up a callback to resolve it
  if (isThenable(actionResult)) {
    actionResult.then(handleResult, (err) => {
-      runRemainingActions(actionQueue, setState)
+      runRemainingActions(actionQueue, action, setState)
      action.reject(err)
    })
  } else {
@@ -197,6 +202,10 @@ function dispatchAction(
    // (Note that it can't contain any earlier navigations, because we always put those into `actionQueue.pending` by calling `runAction`)
    newAction.next = actionQueue.pending.next

+    if (actionQueue.last === actionQueue.pending) {
+      actionQueue.last = newAction
+    }
+
    runAction({
      actionQueue,
      action: newAction,
@@ -0,0 +1,167 @@
+import { nextTestSetup } from 'e2e-utils'
+import type * as Playwright from 'playwright'
+import { createRouterAct } from 'router-act'
+
+describe('discarded action settling while a navigation is pending (#86151)', () => {
+  const { next } = nextTestSetup({ files: __dirname })
+
+  // Navigating while a server action is in flight discards that action:
+  // its result is never applied. These tests cover the window where the
+  // discarded action's response arrives while the navigation is still
+  // ongoing. The navigation must not be reverted, and later actions must
+  // not run until it finishes — if one ran earlier, it would see the
+  // pre-navigation page and undo the navigation.
+  async function interleave(
+    dispatchButton: string,
+    options?: { allowErrorStatusCodes: number[] }
+  ) {
+    let page: Playwright.Page
+    const browser = await next.browser('/', {
+      beforePageLoad(p: Playwright.Page) {
+        page = p
+      },
+    })
+    const act = createRouterAct(page, options)
+
+    // data-render counts how many times the client received new data for
+    // the root layout. Navigations preserve the root layout, so it only
+    // changes when the router refreshes.
+    const initialRender = await browser
+      .elementById('stamp')
+      .getAttribute('data-render')
+
+    await act(
+      async () => {
+        // Start server action A. Its response is withheld, so it stays in
+        // flight throughout this scope.
+        await act(async () => {
+          await browser.elementById(dispatchButton).click()
+        }, 'block')
+
+        // Start server action B. Actions run one at a time, so B waits for
+        // A and no request is issued yet.
+        await act(async () => {
+          await browser.elementById('dispatch-b').click()
+        }, 'no-requests')
+
+        // Navigate before A or B finished. This discards A. The navigation
+        // response is withheld too, so the navigation is still ongoing when
+        // A's response arrives below.
+        await act(
+          async () => {
+            await browser.elementById('go-dest').click()
+          },
+          { includes: 'Destination page', block: true }
+        )
+
+        // Exiting this scope delivers the withheld responses in the same
+        // order: first A's, mid-navigation — the moment under test — then
+        // the navigation's. Only then may B run.
+      },
+      // B ran and completed.
+      { includes: 'b-result' }
+    )
+
+    // B completing means all three operations have settled.
+    await browser.waitForElementByCss('#status-b[data-status="b-result"]')
+
+    // If B ran too early, it saw the pre-navigation page — but that is not
+    // necessarily visible yet. Running one more action makes it visible:
+    // with the bug, the URL flips back to "/" here instead of staying
+    // on /dest.
+    await act(
+      async () => {
+        await browser.elementById('dispatch-c').click()
+      },
+      { includes: 'c-result' }
+    )
+    await browser.waitForElementByCss('#status-c[data-status="c-result"]')
+
+    expect(new URL(await browser.url()).pathname).toBe('/dest')
+    expect(await browser.elementById('dest').text()).toBe('Destination page')
+
+    return { browser, initialRender }
+  }
+
+  it('keeps the navigation when the discarded action resolves', async () => {
+    const { browser, initialRender } = await interleave('dispatch-resolve')
+
+    // The discarded action didn't revalidate anything, so there must be
+    // no refresh.
+    expect(await browser.elementById('stamp').getAttribute('data-render')).toBe(
+      initialRender
+    )
+  })
+
+  // Same scenario, but the discarded action fails. Failure is handled
+  // separately from success, so cover both.
+  it('keeps the navigation when the discarded action rejects', async () => {
+    const { browser, initialRender } = await interleave('dispatch-reject', {
+      // A server action that throws responds with a 500.
+      allowErrorStatusCodes: [500],
+    })
+
+    await browser.waitForElementByCss('#status-a[data-status="rejected"]')
+    expect(await browser.elementById('stamp').getAttribute('data-render')).toBe(
+      initialRender
+    )
+  })
+
+  // An action can also be dispatched *while* the navigation is ongoing.
+  // It must run once the navigation finishes. (At one point it never ran:
+  // its caller never settled, and the router stopped applying updates
+  // entirely — the URL never even changed to the navigation target.)
+  it('runs an action dispatched during the navigation once it finishes', async () => {
+    let page: Playwright.Page
+    const browser = await next.browser('/', {
+      beforePageLoad(p: Playwright.Page) {
+        page = p
+      },
+    })
+    const act = createRouterAct(page)
+
+    await act(
+      async () => {
+        // Start server action A. Its response is withheld, so it stays in
+        // flight throughout this scope.
+        await act(async () => {
+          await browser.elementById('dispatch-resolve').click()
+        }, 'block')
+
+        // Navigate before A finished. This discards A. The navigation
+        // response is withheld too, so the navigation is still ongoing
+        // when B is dispatched below.
+        await act(
+          async () => {
+            await browser.elementById('go-dest').click()
+          },
+          { includes: 'Destination page', block: true }
+        )
+
+        // Dispatch server action B while the navigation is ongoing. It
+        // must wait for the navigation, so no request is issued yet.
+        await act(async () => {
+          await browser.elementById('dispatch-b').click()
+        }, 'no-requests')
+      },
+      // B ran once the withheld responses were delivered.
+      { includes: 'b-result' }
+    )
+
+    await browser.waitForElementByCss('#status-b[data-status="b-result"]')
+    expect(new URL(await browser.url()).pathname).toBe('/dest')
+    expect(await browser.elementById('dest').text()).toBe('Destination page')
+  })
+
+  it('defers the refresh from a discarded revalidating action until the queue drains', async () => {
+    const { browser, initialRender } = await interleave('dispatch-revalidate')
+
+    // The discarded action revalidated, so a refresh must still happen —
+    // after everything else, and without undoing the navigation.
+    await browser.waitForElementByCss(
+      `#stamp:not([data-render="${initialRender}"])`
+    )
+    expect(new URL(await browser.url()).pathname).toBe('/dest')
+    expect(await browser.elementById('dest').text()).toBe('Destination page')
+  })
+})
@@ -0,0 +1,16 @@
+'use server'
+
+import { revalidatePath } from 'next/cache'
+
+export async function echo(value: string) {
+  return value
+}
+
+export async function reject() {
+  throw new Error('intentional test error')
+}
+
+export async function revalidate() {
+  revalidatePath('/', 'layout')
+  return 'revalidated'
+}
@@ -0,0 +1,91 @@
+'use client'
+
+import { useState } from 'react'
+import { useRouter } from 'next/navigation'
+import { echo, reject, revalidate } from './actions'
+
+// Rendered in the root layout so the statuses survive navigations.
+export function Controls() {
+  const router = useRouter()
+  const [a, setA] = useState('idle')
+  const [b, setB] = useState('idle')
+  const [c, setC] = useState('idle')
+
+  async function dispatchResolve() {
+    setA('pending')
+    try {
+      setA(await echo('a-result'))
+    } catch {
+      setA('rejected')
+    }
+  }
+
+  async function dispatchReject() {
+    setA('pending')
+    try {
+      await reject()
+      setA('resolved')
+    } catch {
+      setA('rejected')
+    }
+  }
+
+  async function dispatchRevalidate() {
+    setA('pending')
+    try {
+      setA(await revalidate())
+    } catch {
+      setA('rejected')
+    }
+  }
+
+  async function dispatchB() {
+    setB('pending')
+    try {
+      setB(await echo('b-result'))
+    } catch {
+      setB('rejected')
+    }
+  }
+
+  async function dispatchC() {
+    setC('pending')
+    try {
+      setC(await echo('c-result'))
+    } catch {
+      setC('rejected')
+    }
+  }
+
+  return (
+    <div>
+      <button id="dispatch-resolve" onClick={dispatchResolve}>
+        dispatch resolving action
+      </button>
+      <button id="dispatch-reject" onClick={dispatchReject}>
+        dispatch rejecting action
+      </button>
+      <button id="dispatch-revalidate" onClick={dispatchRevalidate}>
+        dispatch revalidating action
+      </button>
+      <button id="dispatch-b" onClick={dispatchB}>
+        dispatch queued action
+      </button>
+      <button id="go-dest" onClick={() => router.push('/dest')}>
+        go to destination
+      </button>
+      <button id="dispatch-c" onClick={dispatchC}>
+        dispatch probe action
+      </button>
+      <div id="status-a" data-status={a}>
+        a:{a}
+      </div>
+      <div id="status-b" data-status={b}>
+        b:{b}
+      </div>
+      <div id="status-c" data-status={c}>
+        c:{c}
+      </div>
+    </div>
+  )
+}
@@ -0,0 +1,17 @@
+import { Suspense } from 'react'
+import { connection } from 'next/server'
+
+// connection() makes the page dynamic in all modes, so navigating here
+// always produces a router request.
+async function Content() {
+  await connection()
+  return <div id="dest">Destination page</div>
+}
+
+export default function DestPage() {
+  return (
+    <Suspense fallback={null}>
+      <Content />
+    </Suspense>
+  )
+}
@@ -0,0 +1,29 @@
+import { Suspense } from 'react'
+import { connection } from 'next/server'
+import { Controls } from './controls'
+import { RenderCounter } from './render-counter'
+
+// The stamp only changes when the router refreshes: navigations preserve the
+// root layout, so a new stamp on the client means a refresh happened.
+async function Stamp() {
+  await connection()
+  return <RenderCounter uuid={crypto.randomUUID()} />
+}
+
+export default function RootLayout({
+  children,
+}: {
+  children: React.ReactNode
+}) {
+  return (
+    <html lang="en">
+      <body>
+        <Suspense fallback={null}>
+          <Stamp />
+        </Suspense>
+        <Controls />
+        {children}
+      </body>
+    </html>
+  )
+}
@@ -0,0 +1,3 @@
+export default function Page() {
+  return <div id="origin">Origin page</div>
+}
@@ -0,0 +1,20 @@
+'use client'
+
+import { useState } from 'react'
+
+// Counts how many times the client received new dynamic data for the root
+// layout. The count lives in client state: a server-side counter would break
+// in deploy tests, where the server is stateless.
+export function RenderCounter({ uuid }: { uuid: string }) {
+  const [count, setCount] = useState(0)
+  const [prevUuid, setPrevUuid] = useState(uuid)
+  if (prevUuid !== uuid) {
+    setPrevUuid(uuid)
+    setCount(count + 1)
+  }
+  return (
+    <div id="stamp" data-render={count}>
+      renders:{count}
+    </div>
+  )
+}