Skip to content

test(cloudflare): Skip retries for expected failures #22991

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

Open
JPeer264 wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

test(cloudflare): Skip retries for expected failures #22991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 11 additions & 2 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 @@ -72,11 +72,20 @@
}
}

type RetryOptions = { maxRetries?: number; retryDelayMs?: number };

// Wrangler can report "Ready" before it can actually handle requests.
// This retries fetch on connection errors and transient 500 responses to handle this race condition.
// The budget (maxRetries * retryDelayMs) must cover the "ready-but-not-serving" window, which can be
// several seconds on a loaded CI runner — hence a generous default.
async function fetchWithRetry(url: string, init: RequestInit, maxRetries = 25, retryDelayMs = 200): Promise<Response> {
//
// Requests expected to fail must disable retries (`maxRetries: 1`), because their 500 or connection
// reset is indistinguishable from a transient startup failure and retrying only repeats the exception.
async function fetchWithRetry(
url: string,
init: RequestInit,
{ maxRetries = 25, retryDelayMs = 200 }: RetryOptions = {},
): Promise<Response> {
for (let attempt = 0; attempt < maxRetries; attempt++) {
try {
const res = await fetch(url, init);
Expand Down Expand Up @@ -342,7 +351,7 @@
});

childProcess.on('close', (code, sig) => {
reject(new Error(`wrangler exited with code ${code} (signal ${sig}) before becoming ready`));

Check failure on line 354 in dev-packages/cloudflare-integration-tests/runner.ts

View workflow job for this annotation

GitHub Actions / Cloudflare Integration Tests

suites/durableobject/error/test.ts > captures errors thrown by a Durable Object fetch handler 

Error: wrangler exited with code 1 (signal null) before becoming ready
 ❯ ChildProcess.<anonymous> runner.ts:354:24
});
});
}
Expand Down Expand Up @@ -429,7 +438,7 @@
if (process.env.DEBUG) log('making request', method, url, headers, body);

try {
const res = await fetchWithRetry(url, { headers, method, body });
const res = await fetchWithRetry(url, { headers, method, body }, expectError ? { maxRetries: 1 } : {});

@sentry sentry Bot Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: When expectError: true and maxRetries: 1 are used, a connection error during startup is silently suppressed, causing tests to hang and time out.
Severity: MEDIUM

Suggested Fix

When expectError: true, the test runner should still retry on transient connection errors (e.g., ECONNREFUSED) but should stop retrying once it successfully receives any response from the worker, including the expected error response. This will prevent connection failures from being silently swallowed while correctly handling expected application-level errors. Alternatively, only use maxRetries: 1 after verifying the worker is truly accepting connections.

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#L441

Potential issue: When a test is configured with `expectError: true` and `maxRetries: 1`,
a race condition during wrangler startup can cause a connection error. The `makeRequest`
function's `catch` block silently suppresses this specific type of error, preventing the
request from ever reaching the worker. Because the worker is never hit, the Sentry
envelope the test is waiting for is never generated. This causes the test to hang until
it eventually fails due to a timeout, creating a flaky test condition that obscures the
root cause of the failure.

Also affects:

  • dev-packages/cloudflare-integration-tests/runner.ts:84~88

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


if (!res.ok) {
if (!expectError) {
Expand Down
42 changes: 42 additions & 0 deletions dev-packages/cloudflare-integration-tests/suites/durableobject/error/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import * as Sentry from '@sentry/cloudflare';
import { DurableObject } from 'cloudflare:workers';

interface Env {
SENTRY_DSN: string;
TEST_DURABLE_OBJECT: DurableObjectNamespace;
}

class TestDurableObjectBase extends DurableObject<Env> {
public constructor(ctx: DurableObjectState, env: Env) {
super(ctx, env);
}

async fetch(_request: Request): Promise<Response> {
throw new Error('Test error from Durable Object fetch handler');
}
}

export const TestDurableObject = Sentry.instrumentDurableObjectWithSentry(
(env: Env) => ({
dsn: env.SENTRY_DSN,
traceLifecycle: 'static',
tracesSampleRate: 1.0,
}),
TestDurableObjectBase,
);

export default Sentry.withSentry(
(env: Env) => ({
dsn: env.SENTRY_DSN,
traceLifecycle: 'static',
tracesSampleRate: 1.0,
}),
{
async fetch(_request: Request, env: Env): Promise<Response> {
const id: DurableObjectId = env.TEST_DURABLE_OBJECT.idFromName('test');
const stub = env.TEST_DURABLE_OBJECT.get(id);

return stub.fetch('http://durable-object/');
},
} satisfies ExportedHandler<Env>,
);
30 changes: 30 additions & 0 deletions dev-packages/cloudflare-integration-tests/suites/durableobject/error/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { expect, it } from 'vitest';
import type { Event } from '@sentry/core';
import { createRunner } from '../../../runner';

it('captures errors thrown by a Durable Object fetch handler', async ({ signal }) => {
const runner = createRunner(__dirname)
.expect(envelope => {
const event = envelope[1]?.[0]?.[1] as Event;
expect(event.exception?.values?.[0]?.type).toBe('Error');
expect(event.exception?.values?.[0]?.value).toBe('Test error from Durable Object fetch handler');
expect(event.exception?.values?.[0]?.mechanism).toEqual({
type: 'auto.faas.cloudflare.durable_object',
handled: false,
});
})
.expect(envelope => {
const event = envelope[1]?.[0]?.[1] as Event;
expect(event.exception?.values?.[0]?.type).toBe('Error');
expect(event.exception?.values?.[0]?.value).toBe('Test error from Durable Object fetch handler');
expect(event.exception?.values?.[0]?.mechanism).toEqual({
type: 'auto.http.cloudflare',
handled: false,
});
})
.unordered()
.start(signal);

await runner.makeRequest('get', '/', { expectError: true });
await runner.completed();
});
20 changes: 20 additions & 0 deletions dev-packages/cloudflare-integration-tests/suites/durableobject/error/wrangler.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"name": "durableobject-error-worker",
"main": "index.ts",
"compatibility_date": "2025-06-17",
"migrations": [
{
"new_sqlite_classes": ["TestDurableObject"],
"tag": "v1",
},
],
"durable_objects": {
"bindings": [
{
"class_name": "TestDurableObject",
"name": "TEST_DURABLE_OBJECT",
},
],
},
"compatibility_flags": ["nodejs_als"],
}
2 changes: 1 addition & 1 deletion dev-packages/cloudflare-integration-tests/suites/tracing/headers/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ it('Tracing headers', async ({ signal }) => {
)
.start(signal);

await runner.makeRequest('get', '/');
await runner.makeRequest('get', '/', { expectError: true });
await runner.completed();
closeTestServer();
});
6 changes: 5 additions & 1 deletion dev-packages/cloudflare-integration-tests/suites/tracing/scheduled/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ export default Sentry.withSentry(
async fetch(_request, _env, _ctx) {
return new Response('OK');
},
async scheduled(_controller, _env, _ctx) {
async scheduled(controller, _env, _ctx) {
if (controller.cron === '0 0 * * *') {
throw new Error('Test error from scheduled handler');
}

// Successful scheduled handler - just does some work
await new Promise(resolve => setTimeout(resolve, 10));
},
Expand Down
20 changes: 20 additions & 0 deletions dev-packages/cloudflare-integration-tests/suites/tracing/scheduled/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect, it } from 'vitest';
import type { Event } from '@sentry/core';
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
Expand Down Expand Up @@ -44,3 +45,22 @@ it('Scheduled handler creates transaction with correct attributes', async ({ sig
await runner.makeRequest('get', '/__scheduled');
await runner.completed();
});

it('captures errors thrown by the scheduled handler', async ({ signal }) => {
const runner = createRunner(__dirname)
.withWranglerArgs('--test-scheduled')
.expect(envelope => {
const event = envelope[1]?.[0]?.[1] as Event;
expect(event.exception?.values?.[0]?.type).toBe('Error');
expect(event.exception?.values?.[0]?.value).toBe('Test error from scheduled handler');
expect(event.exception?.values?.[0]?.mechanism).toEqual({
type: 'auto.faas.cloudflare.scheduled',
handled: false,
});
})
.unordered()
.start(signal);

await runner.makeRequest('get', `/__scheduled?cron=${encodeURIComponent('0 0 * * *')}`, { expectError: true });
await runner.completed();
});
Loading