Skip to content

fix(queue): treat 200 + non-REVALIDATED response as success #1272

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
alex-all3dp wants to merge 1 commit into
base: main
Choose a base branch
from
+35 −24
Open

fix(queue): treat 200 + non-REVALIDATED response as success #1272

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
7 changes: 7 additions & 0 deletions .changeset/queue-treat-already-revalidated-as-success.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@opennextjs/cloudflare": patch
---

fix: treat 200 + non-REVALIDATED response as success in DO queue handler

`DOQueueHandler.executeRevalidation` previously threw `FatalError` when the HEAD self-fetch returned 200 with `x-nextjs-cache` other than `REVALIDATED`. This commonly happens under load when in-isolate stale-while-revalidate has already regenerated the page before the queued HEAD reaches the ISR handler — the page is fresh in cache but Next.js returns `HIT`/`STALE` rather than `REVALIDATED`. The handler now logs a debug message and falls through to the success path (sync table updated, failed state cleared). Addresses the `FatalError: ... cannot be done. This error should never happen.` reports.
32 changes: 19 additions & 13 deletions packages/cloudflare/src/api/durable-objects/queue.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,25 @@ describe("DurableObjectQueue", () => {
expect(queue.ongoingRevalidations.has("id")).toBe(true);
});

it("should treat 200 + non-REVALIDATED response as success (page already regenerated by another path)", async () => {
process.env.__NEXT_PREVIEW_MODE_ID = "test";
const queue = createDurableObjectQueue({
fetchDuration: 10,
statusCode: 200,
headers: new Headers([["x-nextjs-cache", "HIT"]]),
});
await queue.revalidate(createMessage("id"));

await queue.ongoingRevalidations.get("id");

expect(queue.routeInFailedState.size).toBe(0);
expect(queue.sql.exec).toHaveBeenCalledWith(
"INSERT OR REPLACE INTO sync (id, lastSuccess, buildId) VALUES (?, unixepoch(), ?)",
"test.local/test",
process.env.__OPEN_NEXT_BUILD_ID
);
});

it("should block concurrency", async () => {
const queue = createDurableObjectQueue({ fetchDuration: 10 });
await queue.revalidate(createMessage("id"));
Expand All @@ -121,19 +140,6 @@ describe("DurableObjectQueue", () => {
});

describe("failed revalidation", () => {
it("should not put it in failed state for an incorrect 200", async () => {
const queue = createDurableObjectQueue({
fetchDuration: 10,
statusCode: 200,
headers: new Headers([["x-nextjs-cache", "MISS"]]),
});
await queue.revalidate(createMessage("id"));

await queue.ongoingRevalidations.get("id");

expect(queue.routeInFailedState.size).toBe(0);
});

it("should not put it in failed state for a failed revalidation with 404", async () => {
const queue = createDurableObjectQueue({
fetchDuration: 10,
Expand Down
20 changes: 9 additions & 11 deletions packages/cloudflare/src/api/durable-objects/queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import { debug, error, warn } from "@opennextjs/aws/adapters/logger.js";
import type { QueueMessage } from "@opennextjs/aws/types/overrides.js";
import {
FatalError,
IgnorableError,
isOpenNextError,
RecoverableError,
} from "@opennextjs/aws/utils/error.js";
import { IgnorableError, isOpenNextError, RecoverableError } from "@opennextjs/aws/utils/error.js";
import { DurableObject } from "cloudflare:workers";

const DEFAULT_MAX_REVALIDATION = 5;
Expand Down Expand Up @@ -132,11 +127,14 @@ export class DOQueueHandler extends DurableObject<CloudflareEnv> {
signal: AbortSignal.timeout(this.revalidationTimeout),
});
// Now we need to handle errors from the fetch
if (response.status === 200 && response.headers.get("x-nextjs-cache") !== "REVALIDATED") {
this.routeInFailedState.delete(msg.MessageDeduplicationId);
throw new FatalError(
`The revalidation for ${host}${url} cannot be done. This error should never happen.`
);
if (response.status === 200) {
const cacheStatus = response.headers.get("x-nextjs-cache");
if (cacheStatus !== "REVALIDATED") {
Copy link
Copy Markdown
Collaborator

@conico974 conico974 May 19, 2026

Choose a reason for hiding this comment

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

We shouldn't treat STALE as a success that's for sure.
But even HIT should never happen (maybe in PPR, haven't tested that), we are supposed to remove the in isolate revalidation, if not it means there is a bug that we need to fix

Copy link
Copy Markdown
Contributor Author

@alex-all3dp alex-all3dp May 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review @conico974

Not PPR-only, we hit it on regular ISR routes (dynamic = 'error' revalidate from 1 week to 1 year), no cacheComponents/PPR enabled. Our open-next.config.ts is

const openNextConfig: OpenNextConfig = {
  ...defineCloudflareConfig({
    incrementalCache: withRegionalCache(r2IncrementalCache, {
      mode: 'long-lived',
    }),
    queue: queueCache(doQueue, {regionalCacheTtlSec: 30, waitForQueueAck: false}),
    tagCache: doShardedTagCache({
      baseShardSize: 4,
      regionalCache: true,
    }),
    cachePurge: purgeCache({type: 'durableObject'}),
  }),
  dangerous: {
    middlewareHeadersOverrideNextConfigHeaders: true,
  },
  cloudflare: {
    skewProtection: {
      enabled: true,
      maxNumberOfVersions: 10,
      maxVersionAgeDays: 20
    },
  },
}

You're right on both points though. If in-isolate SWR-triggered regen shouldn't exist alongside the queue's HEAD regen, then this PR would just paper over an underlying bug it seems.

How do ypu prefer to handle this PR? Closing it would be reasonable. Alternatively narrow it to handle HIT only, drop STALE, add a guard rail ((e.g. only treat HIT as success when the cache freshness is verifiable) and mark the change as a defense for potential race conditions?

// Page already regenerated by another path (e.g. in-isolate SWR). Treat as success.
debug(
`Revalidation for ${host}${url} observed x-nextjs-cache=${cacheStatus}; treating as success.`
);
}
} else if (response.status === 404) {
// The page is not found, we should not revalidate it
// We remove the route from the failed state because it might be expected (i.e. a route that was deleted)
Expand Down
Loading