Skip to content

fix(cache): keep updateTag misuse catchable - #2311

Merged
james-elicx merged 1 commit into
mainfrom
codex/fix-update-tag-route-handler-crash
Jun 26, 2026
Merged

james-elicx merged 1 commit into
mainfrom
codex/fix-update-tag-route-handler-crash

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • throw updateTag() invalid-context errors synchronously
  • keep valid Server Action invalidation asynchronous and awaitable
  • prevent caught Route Handler misuse from terminating the deployed server

Next.js parity

Fixes the server crash behind the connection-refused failures in test/e2e/app-dir/app-static/app-static.test.ts from run 28143992598.

Validation

  • focused cache shim tests
  • production Route Handler health regression
  • targeted Next.js Route Handler and later Server Action assertions passed, concurrency 1
  • scoped checks and package build
  • independent review: no actionable findings

Full suites are deferred to CI.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Jun 25, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2311
npm i https://pkg.pr.new/vinext@2311

commit: fa97022

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@ask-bonk

ask-bonk Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Working tree clean. My review is complete.

Review Summary

This is a correct, well-scoped fix. No actionable issues found.

What the PR does

Changes updateTag from async function to a plain function returning Promise<void>. The invalid-context guard now throws synchronously, while the valid Server Action path still returns an awaitable promise via return _invalidateEncodedTag(...).

Why it's correct

The previous async wrapper turned the validation throw into a rejected promise. A Route Handler calling updateTag() inside a synchronous try/catch could not catch it — the rejection escaped as an unhandled promise rejection and crashed the deployed server (the connection-refused failures referenced in the PR body).

I verified this matches Next.js exactly. In packages/next/src/server/web/spec-extension/revalidate.ts, Next.js declares export function updateTag(tag) (non-async), throws synchronously on the guard, and returns revalidate(...) on the valid path:

export function updateTag(tag: string) {
  const workStore = workAsyncStorage.getStore()
  if (!workStore || workStore.page.endsWith('/route')) {
    throw new Error('updateTag can only be called from within a Server Action. ...')
  }
  return revalidate([encodeCacheTag(tag)], `updateTag ${tag}`, undefined)
}

The PR's shape is now a faithful match.

Verification performed

  • Call-site compatibility: Valid callers in tests/shims.test.ts:4814 and tests/cdn-cache.test.ts:229 still await updateTag(...) — works fine since the valid path returns a promise. Server Action execution (app-server-action-execution.ts) awaits user code, so immediate invalidation still completes before the response.
  • Type signature unchanged: Return type stays Promise<void>, matching the existing declaration in next-shims.d.ts:740 — no .d.ts update needed.
  • Ran the tests: tests/shims.test.ts -t updateTag (4 passed) and tests/app-router-production-server.test.ts -t updateTag (1 passed, including the health-check regression).

Minor observations (non-blocking)

  • The fixture route returns the message string but not a stack; the test asserts on error message content only, which is sufficient.
  • The doc comment at cache.ts:157 ("invalidates synchronously within the same request context") is slightly imprecise — the guard is synchronous but the invalidation itself is still async/awaitable. Not worth changing.

Solid fix with correct Next.js parity, a ported regression test, and a focused fixture.

github run

@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared fa97022 against base de014d4 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 117.1 KB 117.1 KB ⚫ -0.0%
Client entry size (gzip) vinext 112.5 KB 112.5 KB ⚫ -0.0%
Dev server cold start vinext 2.61 s 2.58 s ⚫ -1.1%
Production build time vinext 3.11 s 3.10 s ⚫ -0.2%
RSC entry closure size (gzip) vinext 93.0 KB 93.0 KB ⚫ +0.0%
Server bundle size (gzip) vinext 156.9 KB 156.9 KB ⚫ -0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx
james-elicx marked this pull request as ready for review June 25, 2026 09:39
@james-elicx
james-elicx merged commit f05eeb8 into main Jun 26, 2026
94 of 97 checks passed
@james-elicx
james-elicx deleted the codex/fix-update-tag-route-handler-crash branch June 26, 2026 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant