-
Notifications
You must be signed in to change notification settings - Fork 142
fix(api): guard initOpenNextCloudflareForDev against duplicate calls #1276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
spokodev
wants to merge
2
commits into
opennextjs:main
Choose a base branch
from
spokodev:fix/init-dev-clear-error-on-duplicate-call
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
9 changes: 9 additions & 0 deletions
9
.changeset/init-open-next-cloudflare-for-dev-duplicate-call-guard.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| "@opennextjs/cloudflare": patch | ||
| --- | ||
|
|
||
| fix: throw a user-actionable error when `initOpenNextCloudflareForDev` is called more than once in the same process | ||
|
|
||
| Calling `initOpenNextCloudflareForDev` twice in the Next.js config file used to spawn two competing miniflare instances and fail with an obscure workerd `SQLITE_BUSY` error. The function now tracks its own invocation and throws an error that points the user at the config file the moment the second call happens, instead of letting the failure surface deep inside the Workers runtime. | ||
|
|
||
| Closes #1251 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| import type { initOpenNextCloudflareForDev as InitOpenNextCloudflareForDev } from "./cloudflare-context.js"; | ||
|
|
||
| /** | ||
| * Regression test for https://github.com/opennextjs/opennextjs-cloudflare/issues/1251. | ||
| * | ||
| * Calling `initOpenNextCloudflareForDev` more than once in the same Node.js process previously | ||
| * surfaced as an obscure `SQLITE_BUSY` workerd error because two miniflare instances raced for the | ||
| * same on-disk state. The function now tracks its own invocation and throws a user-actionable | ||
| * error on the second call. | ||
| */ | ||
| describe("initOpenNextCloudflareForDev duplicate-call guard", () => { | ||
| let initOpenNextCloudflareForDev: typeof InitOpenNextCloudflareForDev; | ||
|
|
||
| beforeEach(async () => { | ||
| // Re-import fresh per test so the module-level "has been called" flag resets between cases. | ||
| vi.resetModules(); | ||
|
|
||
| // AsyncLocalStorage must be absent on globalThis so `shouldContextInitializationRun` short- | ||
| // circuits before any wrangler / miniflare setup tries to run, isolating the duplicate-call | ||
| // guard from the rest of the initialization path. | ||
| vi.stubGlobal("AsyncLocalStorage", undefined); | ||
|
|
||
| ({ initOpenNextCloudflareForDev } = await import("./cloudflare-context.js")); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| }); | ||
|
|
||
| it("resolves on the first call", async () => { | ||
| await expect(initOpenNextCloudflareForDev()).resolves.toBeUndefined(); | ||
| }); | ||
|
|
||
| it("throws a user-actionable error on the second call in the same process", async () => { | ||
| await initOpenNextCloudflareForDev(); | ||
| await expect(initOpenNextCloudflareForDev()).rejects.toThrow( | ||
| /`initOpenNextCloudflareForDev` was called more than once in the same process/ | ||
| ); | ||
| }); | ||
|
|
||
| it("mentions the SQLITE_BUSY workerd symptom and points the user at the config file", async () => { | ||
| await initOpenNextCloudflareForDev(); | ||
| await expect(initOpenNextCloudflareForDev()).rejects.toThrowError( | ||
| expect.objectContaining({ | ||
| message: expect.stringMatching(/Next\.js config file/), | ||
| }) | ||
| ); | ||
| await expect(initOpenNextCloudflareForDev()).rejects.toThrowError( | ||
| expect.objectContaining({ | ||
| message: expect.stringMatching(/SQLITE_BUSY/), | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it("treats each fresh module load as its own process for the purpose of the guard", async () => { | ||
| await initOpenNextCloudflareForDev(); | ||
| await expect(initOpenNextCloudflareForDev()).rejects.toThrow(); | ||
|
|
||
| // Simulating a new process boundary: re-import the module and call once - it should resolve | ||
| // because the module-level flag is fresh. | ||
| vi.resetModules(); | ||
| const fresh = await import("./cloudflare-context.js"); | ||
| await expect(fresh.initOpenNextCloudflareForDev()).resolves.toBeUndefined(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚩 Unhandled rejection when the guard throws in unawaited calls
The function's JSDoc at line 253 says "although async it doesn't need to be
awaited". If a user accidentally writesinitOpenNextCloudflareForDev()twice withoutawait, the second call returns a rejected promise that is never caught, producing anunhandledRejectionevent (which crashes Node.js 15+ by default). This is arguably the desired behavior — a loud crash forces the user to notice and fix their config — but it's a change from the previous behavior where a duplicate no-op call would silently succeed. Worth confirming this is the intended DX tradeoff.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks. Fixed in caac90a: the guard now lives in a synchronous wrapper that delegates the async work to an internal impl, so a duplicate call throws at the call site instead of producing an unhandled rejection when the call isn't awaited. The duplicate-call tests now assert a synchronous throw.