-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix potential EBADF error when restarting workerd process in Miniflare #12025
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| "miniflare": patch | ||
| --- | ||
|
|
||
| Fix potential EBADF error when restarting workerd process | ||
|
|
||
| Previously, when the workerd process was restarted (e.g., via `setOptions()` or Vite server restart), the stdio pipes from the previous process were not explicitly destroyed. This could lead to `EBADF` (Bad File Descriptor) errors during spawn on some systems. | ||
|
|
||
| The `Runtime#dispose()` method now explicitly destroys all stdio streams (stdin, stdout, stderr, and the control pipe) before killing the process to ensure file descriptors are properly released. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3401,6 +3401,46 @@ test("Miniflare: custom Node outbound service", async () => { | |
| ); | ||
| }); | ||
|
|
||
| test("Miniflare: setOptions: can restart workerd multiple times in succession", async () => { | ||
| // Regression test for https://github.com/cloudflare/workers-sdk/issues/11675 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. annotate ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think annotations are too noisy for this sort of thing. We don't need this to be displayed in every test run report.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://vitest.dev/guide/test-annotations.html#built-in-reporters
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes but I regularly use verbose reporter.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And more importantly, this link to the original issue is not needed in the reporter outpiut. If the test fails it is because something has broken that needs fixing. The original issue is only one of the sources for looking into what could be the cause. |
||
| // EBADF errors can occur when spawning a new workerd process if the stdio | ||
| // pipes from the previous process are not properly cleaned up. This is | ||
| // especially relevant when other parts of the system (like worker_threads | ||
| // used by Miniflare or other Vite plugins) are also managing file descriptors. | ||
| // While this test doesn't reliably reproduce the race condition, it validates | ||
| // that the restart mechanism works correctly after the fix. | ||
| const mf = new Miniflare({ | ||
| port: 0, | ||
| modules: true, | ||
| script: `export default { | ||
| fetch() { | ||
| return new Response("version 1"); | ||
| } | ||
| }`, | ||
| }); | ||
| useDispose(mf); | ||
|
|
||
| // First request to ensure initial startup is complete | ||
| let res = await mf.dispatchFetch("http://localhost"); | ||
| expect(await res.text()).toBe("version 1"); | ||
|
|
||
| // Perform multiple rapid setOptions calls to trigger workerd restarts. | ||
| // This tests that the stdio pipe cleanup in dispose() works correctly. | ||
| for (let i = 2; i <= 5; i++) { | ||
| await mf.setOptions({ | ||
| port: 0, | ||
| modules: true, | ||
| script: `export default { | ||
| fetch() { | ||
| return new Response("version ${i}"); | ||
| } | ||
| }`, | ||
| }); | ||
| res = await mf.dispatchFetch("http://localhost"); | ||
| expect(await res.text()).toBe(`version ${i}`); | ||
| } | ||
| }); | ||
|
|
||
| test("Miniflare: MINIFLARE_WORKERD_CONFIG_DEBUG controls workerd config file creation", async () => { | ||
| const originalEnv = process.env.MINIFLARE_WORKERD_CONFIG_DEBUG; | ||
| const configFilePath = "workerd-config.json"; | ||
|
|
||
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.
That's not consistent with l335?
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.
I guess this could be: