Race Condition from `beforeLoad`/`context` when rapidly changing Search

[KiwiKilian]

Which project does this relate to?

Router

Describe the bug

When rapidly changing search params, the context returned from a promise becomes undefined intermittently. This bug was probably introduced through #4961. It works fine on <=1.131.14 and started breaking from >=1.131.16 onwards.

Your Example Website or App

https://stackblitz.com/edit/github-tg7crfha?file=src%2Froutes%2F__root.tsx

Steps to Reproduce the Bug or Issue

Rapidly move the slider.

Expected behavior

The context shall always be defined.

Screenshots or Videos

Version 1.131.14

14.mp4

Version 1.131.37

latest.mp4

Platform

• Router / Start Version: ^1.131.37

Additional context

No response

[lucasflomuller]

I think its related to #4998

[KiwiKilian]

@Sheraff I've attempted to create a failing unit test but wasn't quite successful. Maybe you can make it break like the reproduction 😅.

test('reproducer #5130', async () => {
  const rootRoute = createRootRoute({})

  const indexRoute = createRoute({
    getParentRoute: () => rootRoute,
    path: '/',
    beforeLoad: async () => {
      await sleep(10)

      return { value: true }
    },

    component: () => {
      const context = indexRoute.useRouteContext()
      const search = indexRoute.useSearch()

      return (
        <div>
          {context.value ? (
            <div data-testid="context"></div>
          ) : (
            <div data-testid="context-missing"></div>
          )}

          {search.value ? (
            <div data-testid={`search-${search.value}`}></div>
          ) : null}

          <Link to="/" search={{ value: 'foo' }} data-testid="link-to-foo">
            foo
          </Link>
          <Link to="/" search={{ value: 'bar' }} data-testid="link-to-bar">
            bar
          </Link>
        </div>
      )
    },
  })

  const routeTree = rootRoute.addChildren([indexRoute])
  const router = createRouter({ routeTree })

  render(<RouterProvider router={router} />)
  await act(() => router.load())

  const fooLink = await screen.findByTestId('link-to-foo')
  expect(fooLink).toBeInTheDocument()
  const barLink = await screen.findByTestId('link-to-bar')
  expect(barLink).toBeInTheDocument()
  const context = await screen.findByTestId('context')
  expect(context).toBeInTheDocument()

  for (let i = 0; i < 100; i++) {
    fireEvent.click(fooLink)

    const contextFoo = screen.queryByTestId('context')
    expect(contextFoo).toBeInTheDocument()
    const searchFoo = await screen.findByTestId('search-foo')
    expect(searchFoo).toBeInTheDocument()

    fireEvent.click(barLink)

    const contextBar = screen.queryByTestId('context')
    expect(contextBar).toBeInTheDocument()
    const searchBar = await screen.findByTestId('search-bar')
    expect(searchBar).toBeInTheDocument()
  }
})

[Sheraff]

I still haven't had time to look into this issue, but I did make a quick repro (i think)

import { describe, expect, it, vi } from "vitest"
import { createMemoryHistory } from "@tanstack/history"
import { BaseRootRoute, BaseRoute, RouterCore } from "../src"

async function setup() {
  const before = vi.fn()
  const rootRoute = new BaseRootRoute({
    beforeLoad: async () => {
      await new Promise((r) => setTimeout(r, 10))
      before()
      return { foo: 'bar' }
    },
  })

  const fooRoute = new BaseRoute({
    getParentRoute: () => rootRoute,
    path: '/foo',
  })

  const routeTree = rootRoute.addChildren([fooRoute])

  const router = new RouterCore({
    routeTree,
    history: createMemoryHistory(),
  })

  await router.navigate({ to: '/foo' })
  before.mockReset()

  const counters = { failed: 0, updates: 0 }
  router.__store.subscribe((e) => {
    if (e.currentVal.isLoading) return
    counters.updates++
    const { foo } = e.currentVal.matches.at(-1)!.context
    if (!foo) counters.failed++
  })

  return { before, router, counters }
}

const ITER = 20

describe('foo', () => {
  it('fast switching search, before load sometimes undefined', async () => {
    const { before, router, counters } = await setup()

    const promises = []
    for (let i = 0; i < ITER; i++) {
      promises.push(router.navigate({ to: '/foo', search: { value: i } }))
      await new Promise((r) => setTimeout(r, 0))
    }
    await Promise.all(promises)

    expect(before).toHaveBeenCalledTimes(ITER)
    expect(counters.updates).toBeGreaterThan(0)
    expect(counters.failed).toBe(0)
  })

  it('slow switching search, before load never undefined', async () => {
    const { before, router, counters } = await setup()

    for (let i = 0; i < ITER; i++) {
      await router.navigate({ to: '/foo', search: { value: i } })
    }

    expect(before).toHaveBeenCalledTimes(ITER)
    expect(counters.updates).toBeGreaterThan(0)
    expect(counters.failed).toBe(0)
  })
})

This doesn't need anything other then router-core, which is always nice for testing.

[sharonyogev]

Any news with that?

[Sheraff]

Any news with that?

I tried for a couple hours to make a convincing repro inside router-core just using vitest (no E2E, no playwright, no react) and I couldn't manage it.

If someone wants to open a PR w/ a reliable repro for this issue it'd be greatly appreciated.

[sharonyogev]

What is wrong with your snippet above? I can try sometime to take it and make it work
Anyhow, I've tried to debug the issue myself, and I have some observations:

• I think the main issue is that concurrent async navigation both access the global router state, so one navigation can update a match and another navigation will get old / new match instead of the right one (for example, in executeBeforeLoad when you call inner.router.getMatch(parentMatchId))
• The bug occur specifically when match._nonReactive.beforeLoadPromise is defined, so we wait for it inside preBeforeLoadSetup and while we wait a new navigation starts and sets the parent match into pending mode
• My first try was to resolve and clear the match._nonReactive.beforeLoadPromise inside the cancelMatch function, that made the bug much less probable, especially if I have only single async beforeLoad at the root route. but I still was able to reproduce the problem with multiple async beforeLoad (on the root route and a nested route). and also, I was a bit afraid to just clean up the promise
• This made me think, what we should aim for concurrent navigation? we see 3 options
  • Each navigation should wait for the previous navigations (with something like navigation queue) - which means we run navigation sequentially (hence no need for state isolation)
  • Run concurrently, but cancel the previous navigations somehow, so there are no concurrent beforeLoad/loaders. this adds complexity since loader might have side effects so cancelling might be problematic, or maybe it's a good practice not to have side effects in loaders
  • Run concurrently with per navigation state isolation so calls to inner.router.getMatch won't conflict with other concurrent navigations, here we might need to address a situation where navigation N finished after navigation N+1

I took the stackblitz repro from above and added some logs to help me understand the problem, if you need me to make attach some more and make it more clear tell me (the line numbers for example are wrong as I edited the node_modules files 😆

load-matches.ts:880 loadMatches 0 0 run
load-matches.ts:471 beforeLoadContext 0 __root__ awaiting
load-matches.ts:883 loadMatches 0 0 awaiting
load-matches.ts:433 updateContext 0 __root__
load-matches.ts:461 updateContext 0 __root__ save context {auth: true}
load-matches.ts:887 loadMatches 0 0 awaited
load-matches.ts:880 loadMatches 0 1 run
load-matches.ts:407 updateContext 0 / no beforeLoad
load-matches.ts:890 loadMatches 0 1 returned
index.tsx:47 loadMatches 1 0 run
index.tsx:47 beforeLoadContext 1 __root__ awaiting
index.tsx:47 loadMatches 1 0 awaiting
load-matches.ts:433 updateContext 1 __root__
load-matches.ts:461 updateContext 1 __root__ save context {auth: true}
load-matches.ts:887 loadMatches 1 0 awaited
load-matches.ts:880 loadMatches 1 1 run
load-matches.ts:407 updateContext 1 / no beforeLoad
load-matches.ts:890 loadMatches 1 1 returned
index.tsx:47 loadMatches 2 0 run
index.tsx:47 beforeLoadContext 2 __root__ awaiting
index.tsx:47 loadMatches 2 0 awaiting
index.tsx:47 cancelMatch __root__
index.tsx:47 cancelMatch /
index.tsx:47 loadMatches 3 0 run
index.tsx:47 loadMatches 3 0 awaiting
load-matches.ts:433 updateContext 2 __root__
load-matches.ts:461 updateContext 2 __root__ save context {auth: true}
load-matches.ts:887 loadMatches 2 0 awaited
load-matches.ts:880 loadMatches 2 1 run
load-matches.ts:407 updateContext 2 / no beforeLoad
load-matches.ts:890 loadMatches 2 1 returned
load-matches.ts:471 beforeLoadContext 3 __root__ awaiting
index.tsx:47 loadMatches 4 0 run
index.tsx:47 loadMatches 4 0 awaiting
load-matches.ts:433 updateContext 3 __root__
load-matches.ts:461 updateContext 3 __root__ save context {auth: true}
load-matches.ts:471 beforeLoadContext 4 __root__ awaiting
load-matches.ts:887 loadMatches 3 0 awaited
load-matches.ts:880 loadMatches 3 1 run
load-matches.ts:407 updateContext 3 / no beforeLoad
load-matches.ts:890 loadMatches 3 1 returned
index.tsx:18 boom!!!!!
index.tsx:18 boom!!!!!
load-matches.ts:433 updateContext 4 __root__
load-matches.ts:461 updateContext 4 __root__ save context {auth: true}
load-matches.ts:887 loadMatches 4 0 awaited
load-matches.ts:880 loadMatches 4 1 run
load-matches.ts:407 updateContext 4 / no beforeLoad
load-matches.ts:890 loadMatches 4 1 returned

This issue is very important to us and we can help if needed, even hop on on a call if needed
cc @avshalomt2

[schiller-manuel]

fixed in https://github.com/TanStack/router/releases/tag/v1.140.1