Skip to content

test(cloudflare): Reduce flakiness for cloudflare with sub workers #20632

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.

Sign up for GitHub

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

Merged
JPeer264 merged 5 commits into from
May 4, 2026
+32 −20
Merged

test(cloudflare): Reduce flakiness for cloudflare with sub workers #20632

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
81c275a
test(cloudflare): Reduce flakiness for cloudflare with sub workers
mydea May 4, 2026
f96420b
Revert "test(cloudflare): Reduce flakiness for cloudflare with sub wo…
JPeer264 May 4, 2026
70535eb
test(cloudflare): Wait for the port to be ready
JPeer264 May 4, 2026
952baff
fixup! test(cloudflare): Wait for the port to be ready
JPeer264 May 4, 2026
87dbe46
fixup! fixup! test(cloudflare): Wait for the port to be ready
JPeer264 May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! test(cloudflare): Wait for the port to be ready
  • Loading branch information
@JPeer264
JPeer264 committed May 4, 2026
commit 952baff7190464b74d0190336db87f2591e2ae5a
46 changes: 27 additions & 19 deletions dev-packages/cloudflare-integration-tests/runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ export function createRunner(...paths: string[]) {
let envelopeCount = 0;
const envelopeWaiters: { expected: Expected; resolve: () => void; reject: (e: unknown) => void }[] = [];
const { resolve: setWorkerPort, promise: workerPortPromise } = deferredPromise<number>();
const { resolve: setSubWorkerPort, promise: subWorkerPortPromise, reject: rejectSubWorker } = deferredPromise<number>();
let child: ReturnType<typeof spawn> | undefined;
let childSubWorker: ReturnType<typeof spawn> | undefined;

Expand Down Expand Up @@ -209,22 +208,33 @@ export function createRunner(...paths: string[]) {

if (process.env.DEBUG) log('Starting scenario', testPath);

const stdio: ('inherit' | 'ipc' | 'ignore')[] = process.env.DEBUG
? ['inherit', 'inherit', 'inherit', 'ipc']
: ['ignore', 'ignore', 'ignore', 'ipc'];

const onChildError = (e: Error) => {
// eslint-disable-next-line no-console
console.error('Error starting child process:', e);
reject(e);
};

function onChildMessage(message: string, onReady: (port: number) => void): void {
const msg = JSON.parse(message) as { event: string; port?: number };
if (msg.event === 'DEV_SERVER_READY' && typeof msg.port === 'number') {
if (process.env.DEBUG) log('worker ready on port', msg.port);
onReady(msg.port);
}
// Inspired by workers-sdk: https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/e2e/helpers/wrangler.ts
function waitForReady(childProcess: ReturnType<typeof spawn>): Promise<number> {
return new Promise((resolve, reject) => {
const stdout = childProcess.stdout;
if (!stdout) {
reject(new Error('No stdout available'));
return;
}

let output = '';
stdout.on('data', (chunk: Buffer) => {
const text = chunk.toString();
if (process.env.DEBUG) process.stdout.write(text);
output += text;

const match = output.match(/Ready on (https?:\/\/[^\s]+)/);
if (match?.[1]) {
resolve(parseInt(new URL(match[1]).port, 10));
}
});
});
Comment on lines +227 to +237
Copy link
Copy Markdown

@sentry sentry Bot May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The waitForReady function can hang indefinitely because it lacks an exit event handler for the child process. If the process exits before it's ready, tests will time out.
Severity: MEDIUM

Suggested Fix

Add an on('exit', ...) event handler to the child process within the waitForReady function. This handler should reject the promise if the process exits before the 'Ready on' message is received, ensuring that tests fail fast with a clear error instead of timing out. A similar implementation exists for the childSubWorker which can be used as a reference.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: dev-packages/cloudflare-integration-tests/runner.ts#L218-L237

Potential issue: The `waitForReady` function lacks a handler for the child process's
'exit' event. If the `wrangler` process exits prematurely (e.g., due to a configuration
error or port conflict) before emitting its "Ready on" message, the promise returned by
`waitForReady` will never resolve or reject. This causes any test calls to `makeRequest`
to also hang, leading to a test timeout instead of a fast failure with a clear error
message. The existing 'error' event handler is insufficient to catch all premature exit
scenarios.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown

@cursor cursor Bot May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitForReady hangs forever if process exits early

Medium Severity

waitForReady only resolves when "Ready on" appears in stdout but never rejects if the process exits or errors before that. At line 264, await waitForReady(childSubWorker) blocks indefinitely if the sub-worker crashes during startup. The outer onChildError handler rejects the isComplete promise, but waitForReady's own promise (which shadows reject in its constructor) never settles, so the async function remains stuck and the main worker is never spawned. The old code properly rejected the awaited startup promise on process exit/error. This is a regression that can cause test hangs instead of fast, clear failures.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 952baff. Configure here.

Copy link
Copy Markdown
Member

@JPeer264 JPeer264 May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would only "hang" for as long as the test timeout. Also if the subworker process would exit, then it is flaky anyways.

}

if (existsSync(join(testPath, 'wrangler-sub-worker.jsonc'))) {
Expand All @@ -243,17 +253,15 @@ export function createRunner(...paths: string[]) {
'--inspector-port',
'0',
],
{ stdio, signal },
{ stdio: ['ignore', 'pipe', 'inherit'], signal },
);

childSubWorker.on('message', (msg: string) => onChildMessage(msg, setSubWorkerPort));
childSubWorker.on('error', rejectSubWorker);
childSubWorker.on('error', onChildError);
childSubWorker.on('exit', code => {
rejectSubWorker(new Error(`Sub-worker exited with code ${code}`));
onChildError(new Error(`Sub-worker exited with code ${code}`));
});

// Wait for the sub-worker to be ready before starting the main worker
await subWorkerPortPromise;
await waitForReady(childSubWorker);
}

child = spawn(
Expand All @@ -274,7 +282,7 @@ export function createRunner(...paths: string[]) {
'0',
...extraWranglerArgs,
],
{ stdio, signal },
{ stdio: ['ignore', 'pipe', 'inherit'], signal },
);

CLEANUP_STEPS.add(() => {
Expand All @@ -284,7 +292,7 @@ export function createRunner(...paths: string[]) {

childSubWorker?.on('error', onChildError);
child.on('error', onChildError);
child.on('message', (msg: string) => onChildMessage(msg, setWorkerPort));
waitForReady(child).then(setWorkerPort).catch(reject);
Copy link
Copy Markdown

@cursor cursor Bot May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate error handler registered on sub-worker process

Low Severity

The onChildError handler is registered on childSubWorker's 'error' event twice — once at line 259 inside the if (existsSync(...)) block, and again at line 293 outside it via optional chaining. When a sub-worker exists and emits an error, onChildError fires twice, logging the error and calling reject redundantly.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 952baff. Configure here.

Copy link
Copy Markdown
Member

@JPeer264 JPeer264 May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, forgot to change that part of the code

})
.catch(e => reject(e));

Expand Down
Loading