fix: add SWR non-blocking guard tests for route handler ISR#975
fix: add SWR non-blocking guard tests for route handler ISR#975james-elicx merged 2 commits intocloudflare:mainfrom
Conversation
…re#959) Audit confirms vinext correctly uses ctx.waitUntil() for all background cache-write and regeneration work — no equivalent of the Next.js pendingWaitUntil / pipe-readable blocking pattern exists. ## Changes - Add slow ISR route handler fixture (revalidate=1, 1s handler delay) - Add response-time assertion to existing STALE test (< 500ms) - Add new test verifying stale response completes fast despite slow background regeneration Ported from Next.js: test/e2e/app-dir/use-cache-swr/use-cache-swr.test.ts
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Good audit work. The PR correctly identifies that vinext's architecture avoids the Next.js bug (#93189) by design — triggerBackgroundRegeneration() fires the regen promise without awaiting it, and ctx.waitUntil() keeps the isolate alive without blocking the response. The tests are a useful regression guard.
CI is all green. A few observations below, nothing blocking.
|
|
||
| // The stale response must arrive promptly; background regen runs | ||
| // out-of-band via ctx.waitUntil(). Allow 500ms for cold-start latency. | ||
| expect(staleDuration).toBeLessThan(500); |
There was a problem hiding this comment.
The 500ms threshold is reasonable for the existing static-data handler since its handler body is near-instant — the only latency is test infra overhead. But note that this assertion is sensitive to CI load spikes. If this starts flaking, the fix is to raise this to 1000ms (not remove it), since the important invariant is "much less than any hypothetical regen duration." Worth keeping in mind.
| expect(cold.status).toBe(200); | ||
| const coldBody = await cold.json(); | ||
| const coldDuration = Date.now() - coldStart; | ||
| expect(coldDuration).toBeGreaterThanOrEqual(700); // roughly 1s handler delay |
There was a problem hiding this comment.
700ms lower bound for a 1000ms setTimeout is generous, which is good for CI. One edge case: if the test process is under heavy load and the cold request takes significantly longer than 1s (e.g., 3s due to GC or Vite overhead), the 1500ms stale window below might not be enough for the entry to expire (since revalidate=1 is measured from when the cache was written, not from test start). In practice this probably won't happen because the prod server is lightweight, but if this test ever flakes, the symptom would be a HIT instead of STALE at line 2323. The fix would be to increase the sleep at line 2315 or to poll for STALE instead of sleeping a fixed duration.
| expect(stale.headers.get("x-vinext-cache")).toBe("STALE"); | ||
| const staleBody = await stale.json(); | ||
| expect(staleBody.timestamp).toBe(coldBody.timestamp); // Still the old data | ||
| expect(staleDuration).toBeLessThan(500); |
There was a problem hiding this comment.
This is the key assertion — the whole point of the test. The 500ms threshold works well here because the handler has a deliberate 1000ms delay, so there's a clear 2x gap between "stale response served from cache" (~0ms) and "blocked on regen" (~1000ms). Good design.
| }); | ||
|
|
||
| // Ported from Next.js: | ||
| // test/e2e/app-dir/use-cache-swr/use-cache-swr.test.ts |
There was a problem hiding this comment.
Nit: The Next.js use-cache-swr test tests the "use cache" directive with SWR semantics, which is a different mechanism from vinext's export const revalidate ISR. The porting attribution is accurate in the sense that the test pattern (slow handler + timing assertion) is borrowed from there, but a reader might think vinext implements "use cache". Consider adjusting the comment to clarify:
| // test/e2e/app-dir/use-cache-swr/use-cache-swr.test.ts | |
| // Test pattern ported from Next.js: | |
| // test/e2e/app-dir/use-cache-swr/use-cache-swr.test.ts | |
| // https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/use-cache-swr/use-cache-swr.test.ts | |
| // (adapted from "use cache" SWR to route handler ISR with export const revalidate) |
| * Slow GET route handler for SWR non-blocking test. | ||
| * | ||
| * Ported from Next.js test: | ||
| * test/e2e/app-dir/use-cache-swr/app/delayed-route/route.ts |
There was a problem hiding this comment.
Same note as the test file — the Next.js delayed-route fixture uses "use cache" + cacheLife("seconds"), not export const revalidate. The comment could clarify that this is an adapted port, not a direct translation:
| * test/e2e/app-dir/use-cache-swr/app/delayed-route/route.ts | |
| * Test pattern adapted from Next.js fixture: | |
| * test/e2e/app-dir/use-cache-swr/app/delayed-route/route.ts |
|
Review posted. Here's a summary of the feedback: Overall: The PR is sound. vinext's ISR architecture correctly avoids the Next.js blocking bug by design — Inline comments:
|
Adjust comments to note test pattern is adapted from Next.js "use cache" SWR tests, not a direct translation. The mechanism differs (export const revalidate vs "use cache").
|
/bigbonk review |
There was a problem hiding this comment.
Review
LGTM. The PR is well-scoped and the tests are correctly designed.
Architecture verification: I traced the full STALE response path through readAppRouteHandlerCacheResponse → scheduleBackgroundRegeneration → triggerBackgroundRegeneration. The regen promise is created synchronously and registered with ctx.waitUntil() but never awaited — the stale response is returned immediately. The claim in the PR description is accurate: vinext has no equivalent of the Next.js pendingWaitUntil / pipe-readable blocking bug.
Test design: The slow-ISR test creates a clean 2x gap (500ms threshold vs 1000ms handler delay) that makes the assertion robust. The cold-request lower bound (>= 700ms) provides a sanity check that the delay is actually running. The polling loop for fresh data with a 5s deadline is the right pattern for CI.
Previous review feedback: The second commit (309688a) correctly addresses all the attribution nits from the prior review — both the test file and fixture comments now clarify that the pattern is adapted from "use cache" SWR, not a direct translation.
CI is all green across all 21 checks. No blocking issues.
| expect(coldDuration).toBeGreaterThanOrEqual(700); // roughly 1s handler delay | ||
|
|
||
| // Wait for the 1s revalidate window to expire. | ||
| await new Promise((resolve) => setTimeout(resolve, 1500)); |
There was a problem hiding this comment.
Noting for future reference: the 1500ms sleep assumes the cold request completes in roughly 1s, so the cache entry (written at cold-request completion) has been stale for ~500ms by this point. If the cold request ever takes >1500ms under extreme CI load (e.g., 3s due to GC), the entry would still be within its 1s TTL when the stale request fires, producing a HIT instead of STALE.
This was already flagged in the prior review and I agree it's unlikely in practice. If it does flake, switching to a poll-for-STALE pattern (similar to the poll-for-HIT below) would be the robust fix. Not blocking.
| // regeneration, this will take ≥ 1s and fail. | ||
| const staleStart = Date.now(); | ||
| const stale = await fetch(`${baseUrl}/api/slow-isr`); | ||
| const staleDuration = Date.now() - staleStart; |
There was a problem hiding this comment.
Minor: staleDuration is measured across only the fetch() call, not the .json() body read. That's fine — response headers (including x-vinext-cache) are available before the body is consumed, and .json() on a small in-memory body is negligible. Just noting this is correct as-is, since some reviewers might wonder whether the body read should be inside the timing window.
|
Review submitted. Here's the summary: Approved — the PR is well-scoped and correctly designed. I verified the ISR architecture by tracing the full STALE response path: The test design is solid — the 500ms threshold vs 1000ms handler delay creates a clean 2x gap that's robust for CI. The second commit addressed all attribution nits from the prior review. CI is all green across 21 checks. Two inline observations (non-blocking):
|
2 participants
Fixes #959
Summary
pendingWaitUntil/pipe-readableblocking pattern from the Next.js fix (vercel/next.js#93189)ctx.waitUntil()for all background cache writes and regenerations — no equivalent of the Next.jspendingWaitUntil/pipe-readableblocking existstest/e2e/app-dir/use-cache-swr/use-cache-swr.test.tsTest plan
it("route handler ISR: STALE serves stale data and triggers background regen")— addedstaleDuration < 500msassertionit("route handler ISR: STALE completes quickly without blocking on background regen")— new test with slow handler fixture, verifies cold request ≥ 700ms, stale response < 500ms, background regen completes successfully