Skip to content

fix(next): isolate per-request state to prevent concurrent-request race#1124

Merged
wangsijie merged 2 commits into
logto-io:masterfrom
darcyYe:yemq-fix-next-concurrent-storage-race
Jun 16, 2026
Merged

fix(next): isolate per-request state to prevent concurrent-request race#1124
wangsijie merged 2 commits into
logto-io:masterfrom
darcyYe:yemq-fix-next-concurrent-storage-race

Conversation

@darcyYe

@darcyYe darcyYe commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Summary

@logto/next kept per-request state — the request's CookieStorage and the navigation URL — on instance fields (this.storage / this.navigateUrl) of LogtoClient. Apps instantiate this client as a module-level singleton (the pattern our own docs recommend), so all requests share those fields.

createNodeClientFromNextApi (and the Edge / server-actions equivalents) did:

this.storage = new CookieStorage(...) // ① write shared field
await this.storage.init()             // ② await — yields the event loop
return new NodeClient({ storage: this.storage }) // ③ read shared field back

The await at ② is a yield point. With two concurrent requests A and B on the same singleton:

A①: this.storage = storageA
A②: await storageA.init()      // suspends
B①: this.storage = storageB    // overwrites the shared field
B②: await storageB.init()      // suspends
A③: new NodeClient(this.storage)  // this.storage is now storageB!

Request A then builds its client with B's storage. If B's init() (async cookie decrypt) hasn't resolved, the session is still empty, so getContext() returns isAuthenticated: false (and empty claims) for a fully authenticated user. The shared navigate callback has the same flaw, so concurrent sign-in/out can redirect to another request's target. User-visible symptom: the session intermittently "flickers" to signed-out on refresh and recovers after a reload.

Why this is an SDK defect, not misuse

Concurrency is the normal operating condition for a server-side request handler, not an edge case — a single page firing two parallel auth-gated requests (e.g. two useSWR hooks) is enough to trigger it, and the singleton usage that triggers it is exactly what the docs recommend. A per-request handler is expected to be reentrant. This fix removes the shared mutable state rather than serializing access to it, so concurrent requests are isolated with zero added latency or locking.

Fix

Keep all per-request state in request-scoped locals, threaded through the return value of the internal createNodeClient* helpers instead of this.*. A small NavigationStore (in src/utils.ts) captures the navigate callback's URL per request. The shared instance fields are removed from the base client. Applied consistently across all three entry points: Pages Router (@logto/next), Edge (@logto/next/edge), and server-actions (@logto/next/server-actions).

Note: the separate refresh-token-rotation race (concurrent getAccessToken() calls each presenting the same single-use refresh token) is not addressed here — it shares the "parallel requests" trigger but is a distinct issue.

Testing

Unit tests

Added a deterministic regression test (keeps storage isolated across concurrent requests) that starts two createNodeClientFromNextApi calls before awaiting either, so request B interleaves through request A's init() yield point, and asserts each request receives its own storage instance. Existing suites for all three entry points pass.

Checklist

  • .changeset
  • unit tests
  • integration tests
  • necessary TSDoc comments

Per-request state (CookieStorage and the navigation URL) was stored on the
LogtoClient instance, which apps use as a module-level singleton. Concurrent
requests clobbered each other across the `await storage.init()` yield point,
intermittently reporting authenticated users as signed out. Keep all
per-request state in request-scoped locals threaded through the return value
of the internal createNodeClient* helpers across the Pages Router, Edge, and
server-actions entry points.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@darcyYe darcyYe marked this pull request as ready for review June 13, 2026 12:17
@darcyYe darcyYe requested a review from wangsijie as a code owner June 13, 2026 12:17
@devin-ai-integration

Copy link
Copy Markdown

Great fix — the race condition analysis is spot-on and the per-request isolation approach is the right solution.

One concern: createNodeClientFromNextApi (Pages Router), createNodeClient (server-actions), and createNodeClientFromEdgeRequest (Edge) are all public methods whose return types changed from NodeClient (or the Edge equivalent) to { nodeClient, storage, getNavigateUrl }. If any consumer calls these methods directly (which is a valid use case for advanced scenarios), this is a breaking change despite the patch changeset.

I'd suggest splitting each into a public wrapper (preserving the original return type) + a private implementation method. See the inline comments for a concrete example.

This keeps the concurrent-request fix intact for all internal handler methods while maintaining full backward compatibility for external consumers.

Comment thread packages/next/src/index.ts
…ible

The per-request isolation fix changed the return type of the public
createNodeClientFromNextApi / createNodeClient helpers from the node client to a
{ nodeClient, storage, getNavigateUrl } bundle, which breaks direct callers
(including the documented "fetch organization tokens" snippet) and is not a valid
patch-level change. Keep the public methods' original return types and move the
per-request bundle into a private createRequestScopedClient used by internal callers.
@wangsijie wangsijie merged commit b03106f into logto-io:master Jun 16, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants