fix(web): never cache a degraded SSR render; guard the feed card's cover check - #1882
Conversation
The card calls getEntryCardImageRawUrl on the same entry as its two catchPostImageSafely calls, during an SSR render with no boundary above it on the profile tabs, feed and community routes. A throw from it would undo the thumbnail guard and fail the whole document. Degrade it to null (treated as not animated) and report once.
A prefetch that outlives the SSR timeout resolves undefined and the page renders without its data, but the middleware chose Cache-Control before the render began, so the degraded document kept the route's s-maxage and was stored at the origin and the edge for the whole tier. A new dependency-free preload gives each request an async context. The timeout marks the request it ran under and the response head is rewritten to private, no-store with a -degraded x-cache-tier, so no layer stores it and an expired good copy stays available to be served stale. Timeouts after the head was flushed are counted as late, and both counts reach the container log once a minute.
… caches A prefetch that fails (RPC 5xx, every node exhausted) renders the same entry-less page as one that times out and was still cached: prefetchQuery swallows the error inside react-query, and withSsrTimeout's catch resolved undefined without marking. Both now mark the response prefetch-error; the page's status is untouched, so a notFound() after a failed lookup still sends its 404, uncached. The preload now counts each outcome by reason with its own sample paths: sent (head written as no-store), late (head already flushed, still cached, which names the streamed routes) and abandoned (client gone before any head, so nothing was sent).
A missing post, account, tag or community comes back from a Hive node as an assert error, but it is a real answer, not a degraded render. Marking it no-store sent every dead-link and crawler hit on a missing post back to the renderer. The not-found asserts the RSS handler already recognised move into a shared isHiveNotFoundError (now also covering the post and community shapes), and both prefetch-error marks skip it. Transport, 5xx, rate-limit, timeout and node-exhaustion errors are still marked.
… state from its own client
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoPrevent caching degraded SSR renders and guard feed cover checks
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 356e020481
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| res.once("close", () => { | ||
| if (ctx.reason && !res.headersSent) record("abandoned", ctx.reason, req); |
There was a problem hiding this comment.
When a client disconnects while a prefetch is still pending, the close event runs while ctx.reason is null, so nothing is recorded; if the prefetch later reaches its 10-second timeout, mark() sets the reason after this one-shot listener has already fired and the request never appears in abandoned. This is especially likely for the async render tails this metric is intended to expose, and causes both the lifetime totals and minute logs to underreport them; retain a closed flag and record when a later mark arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 51212bd: close now sets ctx.closed, and a mark that arrives afterwards with no head ever written records abandoned, once. Specced with the real preload: the client leaves at 50ms, the mark arrives at 150ms, and the result is abandoned: 1 with nothing in sent.
Code Review by Qodo
1. Query tests bypass type checking
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
The entry metadata and the oEmbed/agent loader read condenser get_content first and fall back to bridge.get_post. A condenser failure alone marked the response no-store even when bridge then returned the post, so a complete page went out uncacheable during a partial outage. prefetchQuery takes degradeOnFailure (default true); the preferred source opts out and the fallback's own prefetch marks the response if it fails too. The opt-out is scoped to that one call, so it never clears another query's mark. The preload also counts a client that left while a prefetch was still pending: close now sets a flag, and a mark that arrives afterwards is recorded as abandoned once.
bridge.get_post omits root_author/root_permlink, so a reply at depth 2 or more served from the condenser fallback renders with no canonical, is noindexed, and the discussion route answers its subtree. With the condenser source opted out of marking, that render was cached for the entry tier. Both entry loaders now mark it fallback-incomplete when the served entry has no recoverable root. The root rule moves out of canonicalTarget into an exported threadRoot (a post is its own root, root_* when present, a depth-1 reply's parent), which canonicalTarget and the loaders share. markSsrDegraded is exported from the query helpers for this caller.
1 participant
Two web SSR fixes, one or more commits per issue. Touches the production container command (
apps/web/Dockerfileadds a second--requirepreload), so worth a look at the startup log after deploy.Degraded SSR renders are never stored (#1558)
When a server prefetch times out or fails (RPC 5xx, every node exhausted), the page renders without that data and the client fetches it after hydration. The middleware picked the route's Cache-Control before the render started, so that degraded document kept its
s-maxageand was stored at the origin and the edge for the whole tier: five minutes of an entry-less profile for every visitor, longer on post pages.The App Router gives a server component no way to change a response header once rendering has begun, and throwing turns into a 500 that keeps the same Cache-Control. So this adds
apps/web/ssr-degraded.js, a dependency-free preload next tossr-admission.js:fetchQueryrejects, or whenprefetchQueryleaves its query in error state (react-query swallows that error);isHiveNotFoundError, shared with the RSS handler's Sentry filter) is a real answer and keeps its normal caching, so dead links stay cached;get_contentwithprefetchQuery(..., { degradeOnFailure: false })and thenbridge.get_postwith the default, so a condenser failure alone leaves a page the bridge resolved completely cacheable; nothing is ever unmarked, so one query's success cannot clear another's failure. The fallback is not lossless for replies (bridge omitsroot_*), so a bridge-served reply at depth 2+ whose thread root cannot be recovered (threadRoot, shared withcanonicalTarget) is markedfallback-incomplete;private, no-storewith-degradedappended tox-cache-tier, so neither nginx nor the edge stores it and an expired good copy stays available to be served stale; the status is untouched;[ssr-degraded]line countingsent,late(head already flushed) andabandoned(client gone before any head, including a mark that arrives after the client left), each by reason with sample paths.Known limit, tracked in #1881: a render that already flushed its head (RSC navigations, routes with a
loading.tsxabove their prefetch) cannot be changed and is still cached; those show up aslate.Tradeoff: during an upstream slowdown a URL with no cached copy is no longer shielded by a cached empty page. Stale serving (
proxy_cache_use_stale updating, stale-while-revalidate) and the per-process admission cap still apply; a shorts-maxagefor degraded pages was rejected because it would replace a good stale copy with the empty one.Closes #1558
Profile tabs: last unguarded reader on the card path (#1805)
The route work #1805 asks for (delete
[section]/loading.tsx, structure and stream specs) already landed in #1811 (f517fdd), which did not reference the issue. This closes the one untrusted reader still unguarded on those tabs' card path:EntryListItemThumbnailcalledgetEntryCardImageRawUrl(entry)raw, one line after the guardedcatchPostImageSafelyon the same entry and the same throw surface, so a body that broke one would rethrow from the other with no boundary above the cards.getEntryCardImageRawUrlSafelydegrades a throw tonull("not animated", srcset kept) and reports once per post. No known input throws today; this is the same defence in depth as #1790 and #1814.Please measure
/@ecency/comments,/@ecency/repliesand/@ecency/blogon production with the recipe in #1805 after deploy.Closes #1805
Test plan
tsc --noEmitand eslint cleanx-cache-tier: <tier>-degradedandcache-control: private, no-store;[ssr-degraded]lines appear in the web service logSummary by CodeRabbit