Skip to content

fix(loader): label loader_cache with the block key, not the resolve chain #1220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nicacioliveira wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(loader): label loader_cache with the block key, not the resolve chain #1220

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions blocks/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,13 @@ const wrapLoader = (
}: LoaderModule,
resolveChain: FieldResolver[],
release: DecofileProvider,
/**
* The block key — the loader's module path, e.g.
* `vtex/loaders/legacy/productListingPage.ts`. Used as the metric label
* instead of `ctx.resolverId`, which carries the full resolve chain and is
* unbounded. See the `loader` constant below.
*/
blockKey: string,
) => {
const [cacheMaxAge, mode] = typeof cache === "string"
? [MAX_AGE_S, cache]
Expand All @@ -208,7 +215,21 @@ const wrapLoader = (
req: Request,
ctx: FnContext<State, any>,
): Promise<ReturnType<typeof handler>> => {
const loader = ctx.resolverId || "unknown";
// Metric label. Deliberately the block key and NOT `ctx.resolverId`:
// resolverId carries the full resolve chain, e.g.
// `Categories@sections.variants.1.value.5.sections.0.section.page`, so
// every section position of every variant of every page becomes its own
// time series. Measured in production that reached 3,684 distinct values
// on a single site and 21,849 across the fleet, against a documented
// budget of 1,000 per site and 100 fleet-wide — and `loader_cache` alone
// became 76.6% of all rows in otel_metrics_sum.
//
// The block key is bounded by the number of loader modules in the app,
// which is what the @decocms/start runtime already uses for the
// equivalent metric (13 distinct values fleet-wide). The chain is not
// lost: it still travels on error logs, where high cardinality is fine
// because they are read by point lookup rather than aggregated.
const loader = blockKey || ctx.resolverId || "unknown";
Comment on lines +218 to +232

@coderabbitai coderabbitai Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve the resolver chain for cache identity.

Line 232 changes loader, but this value is also used by ctx.vary?.push and as the resolver query parameter in the cache URL at Lines 282-305. Two instances of the same loader can therefore collide when their cacheKeyValue values match.

Keep the block key for metric labels, but retain ctx.resolverId for cache identity.

Suggested fix 
       const loader = blockKey || ctx.resolverId || "unknown";
+      const cacheResolver = ctx.resolverId || "unknown";
...
-        ctx.vary?.push(loader, cacheKeyValue);
+        ctx.vary?.push(cacheResolver, cacheKeyValue);
...
-          resolver: loader,
+          resolver: cacheResolver,
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@blocks/loader.ts` around lines 218 - 232, Separate the metric label from the
cache identity in the loader flow: keep the bounded block key for the metric’s
loader value, but use ctx.resolverId for ctx.vary?.push and the resolver query
parameter when constructing the cache URL. Update the relevant cache-key
handling around ctx.vary and cache URL generation without changing the existing
fallback behavior for metrics.

@cubic-dev-ai cubic-dev-ai Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Cached loader calls now collide across distinct resolve chains because loader is used as the cache identity as well as the metric label, so a loader with the optional/default cache key can serve one section or page's result to another. Keep a separate block-key label for metrics and retain ctx.resolverId for the cache URL and vary key.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At blocks/loader.ts, line 232:

<comment>Cached loader calls now collide across distinct resolve chains because `loader` is used as the cache identity as well as the metric label, so a loader with the optional/default cache key can serve one section or page's result to another. Keep a separate block-key label for metrics and retain `ctx.resolverId` for the cache URL and vary key.</comment>

<file context>
@@ -208,7 +215,21 @@ const wrapLoader = (
+      // equivalent metric (13 distinct values fleet-wide). The chain is not
+      // lost: it still travels on error logs, where high cardinality is fine
+      // because they are read by point lookup rather than aggregated.
+      const loader = blockKey || ctx.resolverId || "unknown";
       const start = performance.now();
       let status: "bypass" | "miss" | "stale" | "hit" | undefined;
</file context>

const start = performance.now();
let status: "bypass" | "miss" | "stale" | "hit" | undefined;

Expand Down Expand Up @@ -365,7 +386,7 @@ const loaderBlock: Block<LoaderModule> = {
wrapCaughtErrors,
(props: TProps, ctx: HttpContext<{ global: any } & RequestState>) =>
applyProps(
wrapLoader(mod, ctx.resolveChain, ctx.context.state.release),
wrapLoader(mod, ctx.resolveChain, ctx.context.state.release, key),
)(
props,
ctx,
Expand Down
Loading