Repository navigation
🐛 Rule pages - Retry throttled Tina requests and render the fallback only for a missing record - #2802
Conversation
…only for a missing record (#2794, #2801) A throttled TinaCloud request during next build made getRuleData return null, so the page rendered ClientFallbackPage and was cached as NOT FOUND until the next deploy. A throttled ruleData request also showed a false "Broken Related Rule Reference" banner. On 12 Nov two production builds shipped 134 and 20 NOT FOUND rules, and 11 false banners. - lib/tina/tinaRequest.ts classifies a Tina error as not-found, transient (throttling, 429/5xx, network) or fatal, and retries transient errors with backoff. - getRuleData and getCategoryData (now in app/[filename]/pageData.ts) return null only for "Unable to find record". Other errors are thrown, so next build retries or fails the page, and at run time Next keeps the previous version of the page. - getRuleData calls ruleData first and calls ruleDataBasic only after a missing record, which tells a missing rule from a missing related rule. - Both loaders use React cache, so the page and generateMetadata share one result. generateMetadata no longer hides a Tina error behind the default title. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDrXaLRhgZb1G9fPaq4
- The build makes one attempt for each page (staticGenerationRetryCount defaults to 1), so a thrown error fails the build. At run time a background revalidation keeps the stale page, and a render with no usable cached page returns a 500. - ruleDataBasic resolves categories, so it separates a missing related rule only from a missing rule or a missing category. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDrXaLRhgZb1G9fPaq4
|
/deploy |
The /deploy build of this PR failed after 3 s of static generation: /categories got a throttled mainCategoryQuery, and that call had no retry. The layout `global` query, /categories, the rules and discussions services and /orphaned now retry transient errors with withTinaRetry. - These pages are ISR, so they keep their cache settings. Attempt 1 is unchanged. Each retry adds an `x-tina-retry` header, which changes the Next data cache key, so a retry cannot read a cached throttled response. - withTinaRetryAllowingErrors covers errorPolicy "all" (categoryRuleCountsQuery, getCategoryData). It throws on a transient error in the response, and each retry adds "# attempt n" to the query, because the Tina disk cache keys on the request body. - The catch blocks that return a default value are unchanged. They now apply only after the last attempt. Local next build (5% throttling on page queries, plus a throttled first request for each covered query): passed, 0 fallback pages, 285 retries, 2.6 min static generation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDrXaLRhgZb1G9fPaq4
|
🎬 The Case of the Vanishing Rules: what this PR fixes, in 38 seconds.
🤖 Generated with Claude Code https://claude.ai/code/session_01UTtNDrXaLRhgZb1G9fPaq4 vanishing-rules-pr2802.mp4 |
|
/deploy |
…gress (#2801) Each comment on a pull request starts pr-deploy.yml, and GitHub applies the workflow concurrency group before the validate job checks for /deploy. With the group pr-<number>, any comment cancelled a /deploy of that pull request. For example, run 43323425588 on #2802 stopped at page 1,921 of 4,211. Only a /deploy comment now shares the group of its pull request. Another comment gets a group with its own run ID. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDpWnuaUUQ2winSSgMb
PR Preview Deployed
This preview will be automatically deleted when the PR is closed. |
0xharkirat
left a comment
There was a problem hiding this comment.
Nice work Brady. The not-found / transient / fatal split is clean, and the before and after tables make the change easy to trust.
I reviewed 753e958 and sent a few requests to the preview slot.
I could reproduce 2 issues there, and I found 2 more in the code. I also have a few questions.
Josh already covered the comment text at pageData.ts:7 and :121, so I skip those.
Must fix
1. A URL with an odd character now gives a 500, not the NOT FOUND page
app/[filename]/pageData.ts:115
- Preview:
GET /rules/foo%20barreturns 500. Production: the same URL returns 200 (the fallback page). GET /rules/zz-no-such-rule-2802returns 200 on both, so a normal missing slug is fine.- I think Tina rejects the path (
relativePath contains invalid characters) before it looks for the record. That text is not "Unable to find record", soclassifyTinaErrorreturnsfatalandgetRuleDatathrows.generateMetadatahas no catch now, so the render fails. - Real cases: a shared link with a stray character, or scanner URLs. Each one now logs an error and returns a 500.
- Fix: return
nullearly for afilenameoutside Tina's allowed set ([a-zA-Z0-9\-_.]). Or classify Tina's invalid-path error asnot-found. The preview slot log shows the exact error text.
2. A slug that contains an error code is retried 5 times
lib/tina/tinaRequest.ts:38
classifyTinaErrortests the transient patterns on the whole message, and a not-found message contains the path. SoUnable to find record .../ECONNRESET/rule.mdxmatches\bECONNRESET\b, and "transient wins".- Preview:
GET /rules/ECONNRESETreturns 500 after 20.2 s. A normal missing slug returns 200 in about 8 s. - So 1 crafted request costs 5 TinaCloud requests and a 20 s hold, on the backend that throttles us.
ETIMEDOUT,EPIPE,UND_ERR_XandProvisionedThroughputExceededdo the same. - Fix: remove the
Unable to find record <path>lines before the transient test, so the echoed path cannot match.
3. The Tina disk cache can give a throttled response to the default-policy request
app/[filename]/pageData.ts:78 and :83, lib/tina/tinaRequest.ts:103
In tinacms 3.15.0 dist/client.js:
- The disk-cache key is
sha256(JSON.stringify({query, variables})). The error policy is not in the key. - On a cache hit,
request()returns the stored value and skips theerrorPolicy === "throw"check, which is only inrequestFromServer. cache.setwrites withflag: "wx", so the first write stays.
The default-policy request on line 78 and attempt 1 of the "all" request on line 83 send the same body, so they share 1 key:
- A category has a missing rule. Line 78 throws not-found, and the client stores nothing.
- Line 83 attempt 1 comes back throttled. The client stores it under the shared key.
- Attempt 2 passes, and this render is fine.
- The next render of that category with the same cache dir: line 78 gets a cache hit,
{ errors: [throttled] }, with no throw.withTinaRetrysees a success, and line 91 throws[getCategoryData] failed. There is no retry, and it fails the same way until someone clears the cache dir.
package.json runs tinacms build without --no-client-build-cache, so the generated client sets cacheDir.
I did not check if the runtime keeps that dir, so I am not sure how often step 4 happens. The shared key is clear from the client code.
Fix (1 line): add a marker to every "all" request, attempt 1 too, for example `${query}\n# errorPolicy all, attempt ${attempt}`. This also covers fetchCategoryRuleData.
Should fix
4. fetchCategoryRuleData ignores fatal errors in an "all" response
lib/services/rules/rules.service.ts:118-125
withTinaRetryAllowingErrorsreturns a fatal error "for the caller to handle" (your test says so). This caller only logs "Unable to find record", then readsres?.data?.categoryConnection.- With
data: nullit gets 0 edges, ends the loop, and returns{ totalUniqueRules: 0, categoryRuleCounts: {} }as a success. unstable_cachestores that with tags and norevalidate, so/categoriescan show no categories until something revalidates the tag.- This is old behaviour, but
getCategoryDatanow throws in the same case (pageData.ts:91). Copy that check here, and usetinaResponseErrorMessagesin place of the copy on line 118.
Questions
5. Can attempt 1 of the ISR callers still read a throttled response from the Next data cache?
lib/tina/tinaRequest.ts:86
pageData.tsusesno-store, because a cached throttled 200 "would fail every retry and every later page". The other callers (layoutglobal,mainCategoryQuery, homepage, orphaned) send attempt 1 unchanged. So a throttled attempt 1 stays under that key for the revalidate window: 60 s forglobal, 6 h for/categories.- Every later page then loses attempt 1 and sleeps about 1 s. All callers also share the
x-tina-retry: 2..5keys, so a long burst can fill all 5, and the call fails after TinaCloud recovers. - Check: in your proxy test, throttle
globalfor 20 s, then count[tina] global failedlines from pages that render after that. If the count is more than 0, use a per-call nonce in the retry header, not the bare attempt number.
6. Do you want 5 retries at request time too?
lib/services/rules/rules.service.ts:239
fetchPaginatedRules and the other service calls also run at request time. Under throttling, 1 API request can now wait about 15 s and send 5 Tina requests. For request-time callers, attempts: 2 could be enough.
7. How many more Tina requests does no-store on topCategoryWithIndexQuery cause at run time?
app/[filename]/pageData.ts:37
Before, the Next data cache shared this query between all rule pages. At build the Tina disk cache still dedupes it. At run time each ISR render and each unknown URL now walks every top category live. If the preview slot shows a lot more Tina requests, a module-level memo of the filename-to-path map would fix it.
8. One deleted category makes every category page NOT FOUND (old behaviour)
app/[filename]/pageData.ts:39-42
topCategoryWithIndexQuery resolves every index[].category. If 1 top category lists a deleted category, the whole query throws not-found. getFullRelativePathFromFilename then returns null for every filename, so every category page renders NOT FOUND.
This is not from this PR, but the new comment on line 13 says null means "the record does not exist". errorPolicy: "all" here, like getCategoryData, would keep the lookup working. A follow-up is fine.
Nits
tinaRequest.ts:6-8says that every default value after an error gets cached.fetchRuleCount(0),fetchCategoryRuleCounts({}),fetchActivityLatestRulesData([]) andfetchHomepageData(null) still do that after the last retry. You say this is on purpose, so maybe reword the comment, or raise a PBI.pageData.test.ts:46hard-codeshasNextPage: false, so the pagination loop and the not-found branch ofgetFullRelativePathFromFilenamehave no test. Theno-storetest on line 84 checks 3 of the 5 request sites.ruleDataBasicand the"all"request are missing.tinaRequest.ts:64: ±25% jitter on a 1 s base keeps parallel workers close together. Full jitter (Math.random() * cap) spreads them out more.
I don't raise what you already list under "Not in this PR" (generateStaticParams, lib/og/target.ts, /api/tina/*).
So: changes requested for 1 to 3. 1 and 2 are small, and 3 is a 1-line change. The rest can be follow-ups.
0xharkirat
left a comment
There was a problem hiding this comment.
Inline notes for my review above, 1 note on each line.
From the review of 0xharkirat: 1. A filename outside [A-Za-z0-9._-] returns null with no Tina request, and Tina's "Invalid path:" error is not-found. /rules/foo%20bar gives the fallback page again, not a 500. Every rule folder and category file name uses only these characters. 2. classifyTinaError removes the path from "Unable to find record" lines before the transient test, so /rules/ECONNRESET is not retried 5 times. 3. Every errorPolicy "all" request adds a marker to the query, attempt 1 too. The Tina disk cache keys on the body only and skips the "throw" check on a hit, so a default-policy request could read a stored "all" response with a throttling error. 4. fetchCategoryRuleData throws on an error that is not not-found, the same as getCategoryData, instead of returning empty counts as a success. 5. Each retry header and retry marker has a random part, so a retry cannot read a response that another call's retry stored. 6. fetchPaginatedRules runs only at request time and retries once. Tests: the pagination and not-found branches of the top category lookup, no-store on every pageData request, and a new rulesService test. Local check: next build with throttling passed (0 fallback pages). next start: /foo%20bar and /ECONNRESET return 200 with the fallback page in under 0.4 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDrXaLRhgZb1G9fPaq4
Dismissed by Brady. Items 1 to 6 are fixed in 7255149, and each thread has a reply. Items 7, 8 and the jitter nit are follow-ups.
0xharkirat
left a comment
There was a problem hiding this comment.
Re-reviewed 7255149. All 4 fixes look right, thanks Brady.
- 1.
isPossibleFilenamestops an odd name before any Tina request.Invalid path:is now not-found too, so that guard also catches..(Invalid path: path escapes the collection directory). - 2. I checked
@tinacms/graphql:Unable to find record <path>holds only the path. So removing that line cannot hide a real throttling error. - 3. The
# errorPolicy allmarker gives the"all"request its own disk-cache key. The random retry id also stops the shared retry keys. - 4.
fetchCategoryRuleDatanow throws, sounstable_cachestores nothing. - 5 and 6. The random retry header and
attempts: 2on the paginated API look good. 24 retries of about 1 s each is a fair price to keep ISR.
7, 8 and the jitter as follow-ups work for me. CI passes.
The preview slot still runs 753e958 (/rules/foo%20bar still returns 500 there). A /deploy before merge would confirm 1 and 2 on the slot.
| expect(request).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it("does not return empty counts as a success after a fatal error", async () => { |
There was a problem hiding this comment.
This test is named for the opposite of what it asserts. The title says empty counts are not returned as a success, but the body asserts the result is {}. It documents the swallowed error as correct.
| it("does not return empty counts as a success after a fatal error", async () => { | |
| it("logs a fatal error and returns an empty result", async () => { |
| withTinaRetry( | ||
| label, | ||
| async (attempt) => { | ||
| const response = await send(attempt === 1 ? `${query}\n# errorPolicy all` : `${query}\n# errorPolicy all, retry ${retryId(attempt)}`); |
There was a problem hiding this comment.
The PR body describes cache-busting markers this code does not use. The body says each errorPolicy: "all" attempt adds # attempt n to the query. The code adds # errorPolicy all on attempt 1 and # errorPolicy all, retry <random id> on retries. Anyone grepping logs or the proxy for # attempt finds nothing.
Fix: reword the body (the "Each \"all\" attempt adds # attempt n" bullet, the withTinaRetryAllowingErrors paragraph and the build-retries check line) to match the code.
| errorPolicy: "all", | ||
| }); | ||
| const variables = { first: 50, after }; | ||
| const res: any = await withTinaRetryAllowingErrors(`categoryRuleCountsQuery after=${after}`, String(CategoryRuleCountsQueryDocument), (query) => |
There was a problem hiding this comment.
The PR's "Not in this PR" list contradicts this diff. It says fetchCategoryRuleData has "the same error handling as before". Here it is wrapped in withTinaRetryAllowingErrors and now throws on any error that is not "not found" (below), where it used to only log. It also says the API routes have no retry, but fetchPaginatedRules now retries with { attempts: 2 }, and the paginated rules API route calls it.
Fix: move these two out of "Not in this PR" and describe the new behaviour.
…validation period (#2801) The root layout sent the global settings query with `next: { revalidate: 60 }`. Each page that the server rendered at run time got a 60 second revalidation period from that request. This applies to an edited rule or category, to a path that is not a rule, and to the home, categories, search, user, archived and orphaned pages, which declare 5 minutes or 6 hours. The layout now reads the settings through `unstable_cache` with the tag `global-settings` and no period. A page changes only after a revalidation on demand, after its own period, or in a deploy. The Tina webhook route marks the tag stale when a file in `global/` changes. A fallback page that the server renders at run time now also has no period. Merge this commit after #2802, which stops the fallback for a failed Tina request. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDpWnuaUUQ2winSSgMb
…ucture job, send less to Docker, and crawl at the full rate (#2806) * ⚡ Build workflow - Run the disk cleanup only when free disk space is less than 30 GB (#2801) The runner had 80 GB of free disk space before the cleanup in 6 of 6 runs. The image build writes approximately 18 GB. The two cleanup steps took 2.0 to 3.6 minutes in each run. - Add a step that reads the free space of the file system that holds the Docker data directory, and prints it. The pre-build cleanup runs only when the free space is less than 30 GB, or when the step cannot read it. The cleanup removes the same items as before. - Remove the post-build cleanup. GitHub discards the runner after the job. - Set cleanup: false for docker/setup-buildx-action. With the default value, the action removes the builder at the end of the job, and that deletes the same build cache that the post-build cleanup deleted. - Print the disk usage after the image build, to show how much space the build used. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDpWnuaUUQ2winSSgMb * ⚡ Deploy workflows - Build the image at the same time as the infrastructure job (#2801) The build job waited 2.1 to 3.3 minutes for the infrastructure job, and it used only the registry name from that job. - Remove the infrastructure job from the needs of the build job in build-and-deploy.yml and pr-deploy.yml. - Calculate acr_name with the naming rule of infra/deploy.ps1. The result is acrsswrulesprod for production, and acrsswrulesstaging for staging and for pull request slots. - The deploy job continues to need the two jobs. Its first step stops the deploy when the image is not in the registry that the infrastructure job reports. If the infrastructure job fails, the build job now continues and pushes the image. The deploy job does not run. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDpWnuaUUQ2winSSgMb * ⚡ Docker context - Send only the MDX and JSON files of the content checkout (#2801) The build context was 3.33 GB, because the content checkout was in it with its git data (1.7 GB) and its images (1.5 GB). The image build does not read those files. The context is now approximately 40 MB. The MDX and JSON files stay in the context. Tailwind detects class names in the text files of the build folder, and that includes the rule text. In a test build with all of the content checkout excluded, the CSS had 10 fewer utility rules. With the MDX and JSON files in the context, each file of the build output was identical to the build with the full checkout, except the files that differ between two builds of the same input. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDpWnuaUUQ2winSSgMb * ⚡ Sitemap crawl - Run the crawler without the dependency install (#2801) scripts/crawl-sitemap.js imports only modules that are part of Node.js. - Remove the pnpm setup step and the install step. They took approximately 21 seconds. - Call the script with node. This is the command that the pnpm script ran. - Remove fetch-depth: 0 from the checkout. The job does not read the git history. The request rate of the crawl does not change. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDpWnuaUUQ2winSSgMb * ⚡ Sitemap crawl - Use the full request rate for the crawl after a deploy (#2801) The crawl after a production deploy took 11.8 minutes for 3,707 URLs. It sent 5 requests and then waited 250 ms, which is approximately 5 requests each second. The limit of the crawler is 20. - crawl-sitemap.yml gets two optional inputs for a caller, batch_size and delay_ms. The crawl step gives them to the crawler as SITEMAP_BATCH_SIZE and SITEMAP_DELAY_MS. - build-and-deploy.yml passes 20 and 0. The crawler then sends 20 requests, and starts the next 20 one second after the start of the batch. SITEMAP_MAX_RPS stays 20. - A crawl that a person starts has no such inputs, so it keeps the rate of 5 requests and 250 ms. Reason for the two rates: the crawl after a deploy starts minutes after the build, when each page is newer than its revalidate period. The server answers from its cache and starts no render. A crawl that a person starts can find pages that are older than that period. Each request for such a page starts a render that reads TinaCloud, and a fast crawl can then cause TinaCloud throttling. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDpWnuaUUQ2winSSgMb * 🐛 PR deploy - Stop a comment from cancelling a /deploy that is in progress (#2801) Each comment on a pull request starts pr-deploy.yml, and GitHub applies the workflow concurrency group before the validate job checks for /deploy. With the group pr-<number>, any comment cancelled a /deploy of that pull request. For example, run 43323425588 on #2802 stopped at page 1,921 of 4,211. Only a /deploy comment now shares the group of its pull request. Another comment gets a group with its own run ID. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTtNDpWnuaUUQ2winSSgMb --------- Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
Description
✏️ Relates to #2794 and #2801 (the first three tasks of phase 2). After a TinaCloud throttling error, a production build no longer ships NOT FOUND rule pages or false "Broken Related Rule Reference" banners.
Problem. On 12 Nov, two production builds were throttled by TinaCloud (
Throughput exceeds the current capacity). They shipped 134 and 20 rules as NOT FOUND, cached for a year, and 11 rules with a false banner.getRuleDataandgetCategoryDatacaught every Tina error and returned null, so the page renderedClientFallbackPage.ruleDatarequest was read as a broken related reference.Changes
lib/tina/tinaRequest.tsputs each Tina error into one of three classes:not-found: "Unable to find record"transient: throttling, 429/5xx or a network failurefatal: all other errorswithTinaRetryretries transient errors with backoff (about 1, 2, 4 and 8 s).app/[filename]/pageData.tsnow holds the loaders frompage.tsx:not-found. For any other error they throw, and Next.js caches nothing for that render:staticGenerationRetryCountdefaults to 1.revalidatePath) returns a 500, and the next request renders again.getRuleDatacallsruleDatafirst. It callsruleDataBasiconly after "Unable to find record".ruleDataBasicdoes not resolve related rules, so it separates a missing related rule from a missing rule or a missing category. A rule whose category file is missing still renders as NOT FOUND, as before.getCategoryDatauses the defaultthrowpolicy first, anderrorPolicy: "all"only when rules are missing. Any error other than a missing record fails the page, so rules cannot drop from a category without a trace.cache(), so the page andgenerateMetadatashare one result.Two caches could replay a throttled response. TinaCloud sends throttling as a GraphQL error with HTTP 200:
.next/cache/fetch-cache. The page requests now usecache: "no-store". The route stays SSG because ofdynamic = "force-static", and the route table is unchanged.cacheDir, added bytinacms build). It storeserrorPolicy: "all"responses with their errors. Each"all"attempt adds# attempt nto the query, so the cache key changes.generateMetadatano longer catches errors, so a Tina error cannot put the default title on a cached page.The other build-time pages retry too (commit 753e958). The first
/deploybuild of this PR failed after 3 s:/categoriesgot a throttledmainCategoryQuerywith no retry. These calls now usewithTinaRetry:globalquery (every page)/categoriesmainCategoryQueryrules.service.ts: activity and latest rules, category rule counts, archived rules, homepage, paginated rulesdiscussions.service.tsrulesByGuidQuery(home page) and/orphanedThese pages are ISR, so they do not use
no-store. Attempt 1 is unchanged, and each retry adds anx-tina-retryheader, which gives it a new Next data cache key. ForerrorPolicy: "all",withTinaRetryAllowingErrorsthrows on a transient error in the response and adds# attempt nto each retry. The catch blocks that return a default value are unchanged, and now apply only after the last attempt.Checks
CI
/deploybuild of 753e958 against real TinaCloud (run 43328493737, 3 workers, 4,211 pages):release/v1.8.18(run 43165212271)avoid-repetition)[filename]was throttled in this run, so TinaCloud has not yet received thex-tina-retryheader.run-llms-locally,upgrade-your-laptopand the categoryrules-to-better-user-authentication. They are NOT FOUND on production now.Local fault test:
Local
next buildagainst the local Tina server, through a proxy that throttles 5% ofruleData,ruleDataBasic,topCategoryWithIndexQueryandcategoryWithRulesQueryrequests with the real TinaCloud error text.The client was generated by
tinacms build(with its disk cache), and.nextwas empty, as in CI. Content: SSW.Rules.Content at 5 Nov. Each build prerendered 4,138 pages.2741dc4)ruleData+ruleDataBasicrequests/[filename]●x-tina-retryheader and the# attemptcomment on the retries.Not in this PR
generateStaticParams(changed by ⚡ Performance - Decrease the production deploy time from 42 minutes to 20 minutes #2801's rule-list task),lib/og/target.ts(OG images are not prerendered) and the API routes have no retry.generateStaticParams,fetchCategoryRuleData(errorPolicy: "all") and/api/tina/rulehave the same error handling as before. The rule list from a file is a separate task in ⚡ Performance - Decrease the production deploy time from 42 minutes to 20 minutes #2801./api/tina/categoryuseserrorPolicy: "all"insideunstable_cache(app/api/tina/category/route.ts), so a throttled response can be cached for an hour.🤖 Generated with Claude Code
https://claude.ai/code/session_01UTtNDrXaLRhgZb1G9fPaq4