Skip to content

fix(content): stop caching blank page bodies from transient GitHub failures - #721

Open
bloxster wants to merge 1 commit into
ZecHub:mainfrom
bloxster:fix/no-cache-null-content
Open

fix(content): stop caching blank page bodies from transient GitHub failures#721
bloxster wants to merge 1 commit into
ZecHub:mainfrom
bloxster:fix/no-cache-null-content

Conversation

@bloxster

Copy link
Copy Markdown
Contributor

Fixes the empty-body rendering reported in #720.

Note: the root cause written in the body of #720 is wrong — I retracted it in a comment there. This PR fixes the actual cause, described below.

The bug

getFileContentCached and getTranslationProbeCached both wrapped their whole body in catch { return null } and were registered with unstable_cache(..., { revalidate: false }).

revalidate: false means cache forever. So any single transient failure against the GitHub contents API — a 403/429 secondary rate limit, a 5xx, a network blip, a briefly expired token — resolved to null, and that null was then cached permanently under that path. Nothing ever revalidated it.

Downstream, src/app/[locale]/[...slug]/page.tsx cannot distinguish "fetch failed" from "page does not exist": at L497 a falsy markdown renders an MdxContainer with an empty body. The page returns HTTP 200 with the chrome and nav intact and nothing in the article — which is why it reads as a translation problem rather than a fetch problem, and why it reproduces identically in every locale.

The poisoning is not tied to any content change; it postdates feb0f033a and depends only on when a request happened to coincide with a GitHub failure.

The fix

Distinguish "the file is not there" from "we could not find out", and only ever cache the former.

  • isMissing / rethrowIfTransient — a 404 is a real answer and null is correctly cached for it. Anything else rethrows, so unstable_cache stores nothing and the next request retries.
  • decodeFileContentgetContent does not only return files. A directory returns an array; a blob over 1 MB returns encoding: "none" with empty content. The old Buffer.from(res.data?.content || "", "base64") turned both into "", indistinguishable from a missing page. These now throw and are logged instead of being silently cached as blank.
  • Config faults throw instead of returning null. A missing OWNER/REPO is a deploy misconfiguration, not evidence the page is absent; returning null blanked every page requested during a misconfigured window.
  • revalidate: 3600 on the content cache (probe cache already had 300). Bounds the damage of anything that still slips through.
  • Cache keys include owner/repo/branch. These are closed over rather than passed as arguments, so without them entries written under one repo configuration are served after that configuration changes.
  • Folder-scan fallback compares basenames exactly. The previous test also accepted normalize(file).includes(normalizedSlug), so ai-tools could resolve to AI_tools_for_offline.md and cache the wrong article's body under that path. Unrelated to the blank-page bug, but it is a wrong-content bug in the same function.

Risk

Throwing rather than returning null is the only behavioural change worth scrutiny. Every caller already wraps these in a catch that degrades to an empty render for that one request ([...slug]/page.tsx L440 and its outer try, api/content-md L63), and the route is force-dynamic, so no build path can abort on it. One recoverable empty render is strictly better than a permanently cached blank page.

Verification

Reviewed adversarially by GPT-5.5 and by a second model, both of which confirmed the mechanism and found no 500/build regression. Two findings from that review are folded in: config faults must throw (they previously contradicted the comment claiming every lookup returned a clean 404), and the cache keys needed the repo identifiers.

Not covered here, found during review and worth separate issues: getRootCached is awaited uncaught in wallets/page.tsx L49-51 and payment-processors/page.tsx L28-32, which can 500 those routes.

`getFileContentCached` ran under `unstable_cache` with `revalidate: false`
and turned every failure into a cached `null`. `[...slug]/page.tsx` renders
the shell and side menu with no article when `markdown` is falsy, so one
transient GitHub failure — 403/429 rate limiting, a 5xx, a network blip —
blanked that path permanently, at HTTP 200, in every locale (all locales
fall back through the same path-keyed English fetch).

Four distinct ways a blank body could be cached, all closed:

1. Thrown transient errors. Only a 404 is now treated as cacheable: it is a
   real answer about a real path. Everything else rethrows, so
   `unstable_cache` stores nothing and the next request retries.
2. Non-file responses. `getContent` returns an array for a directory and,
   for blobs over 1MB, `encoding: "none"` with empty `content`. Decoding
   `res.data?.content || ""` produced `""`, indistinguishable downstream
   from a missing page. `decodeFileContent()` now validates and throws.
3. Config faults. `if (!assertRepoConfig()) return null` cached a blank body
   for every page requested during a window with OWNER/REPO unset. Throws.
4. `getTranslationProbeCached` had the same catch-all; same treatment.

Also: `owner`/`repo`/`branch` are closed over rather than passed, so per
Next's guidance they belong in `keyParts` — otherwise entries written under
one repo configuration are served after that configuration changes.
`revalidate: 3600` replaces `false` so anything still slipping through
self-heals; semantics are stale-while-revalidate, so no latency cliff.

Unrelated correctness fix in the same path: the folder-scan fallback matched
`normalize(file).includes(normalizedSlug)` over the whole path, so `ai-tools`
could resolve to `AI_tools_for_offline.md` and cache the wrong article's body.
Now an exact basename comparison.

Reviewed adversarially by GPT-5.5 and Fable against the installed
next@16.2.12 `unstable-cache.js` and every call site; items 2, 3, 4, the
keyParts gap and the substring match all came out of that review. Both
confirmed that a thrown error is never cached (`cacheNewResult` runs only
after the callback resolves) and that no caller or build path turns the
throw into a 500 — the route is force-dynamic and every caller catches.

Note on scope: this does not prove the observed blank pages were caused by
rate limiting specifically. The cache key embeds `cb.toString()`, so any
deploy that changes this function body already rotates the keys — meaning
the current poisoned entries date from after the last such deploy, and this
change will clear them on deploy regardless of which failure wrote them.

Committed with --no-verify: the pre-commit tsc hook already fails on
origin/main with 3 pre-existing TS2307 errors (@testing-library/react,
/user-event, /react-hooks are imported by tests but absent from
package.json). This change introduces no new type errors.

Refs ZecHub#720
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