feat(query-orchestrator): honour refreshKeyRenewalThreshold on locally evaluated refresh keys

[ovr]

Description of Changes Made

Allow CUBEJS_REFRESH_KEY_LOCAL_TIME to work together with queryCacheOptions.refreshKeyRenewalThreshold. Previously, configuring a renewal threshold disabled local evaluation and sent time-based refresh key queries to the database. Eligible every-based keys now use the instance clock while retaining the shared cache's expiration and renewal rules.

With a renewal threshold configured, fetchAndCacheQuery evaluates the local descriptor directly on a cache miss or renewal and writes the result through the existing cache path. The cache identity, caller-supplied TTL, renewal timestamp, background renewal, and cache logging are preserved. Entries remain reusable across instances and when switching between SQL and local evaluation. Local computations never enter the SQL queue or create a database client, so they cannot wait behind long-running SQL queries. Concurrent misses and renewals are not deduplicated. Last-writer-wins is an accepted trade-off of direct local evaluation: if callers compute on opposite sides of an interval boundary and their writes arrive out of order, an older key can overwrite a newer key already returned to another caller. The stored value can remain until renewal or TTL expiry and may cause an extra pre-aggregation build; monotonic key values across callers are not guaranteed.

Without a threshold override, including an override of zero, eligible keys are computed directly without a shared cache entry. SQL-defined and incremental keys, and keys with invalid or missing local descriptors, retain SQL execution. A shared usesUncachedLocalRefreshKey() method keeps cache bypass and scheduled-refresh warming consistent: the scheduler warms cached local keys and skips uncached ones.

Remove the unused localRefreshKeyResult method and its duplicate tests, make descriptor validation a TypeScript type guard, and identify warmed interval keys by cache identity in scheduler tests. The environment-variable reference documents threshold behavior, and REFRESH_KEY_CACHE_TTL_SECONDS names the pre-aggregation loader's existing one-hour TTL.

Validation

• 210 query-orchestrator unit tests passed, covering cache TTL and renewal, mixed-TTL readers, background renewal, cache failures, SQL/local mode changes, fallback cases, and pre-aggregation loading.
• 33 Cube Store cache tests passed, including shared local entries and evaluation without a queue on misses and renewals.
• 4 local-refresh scheduler tests passed, including zero/unset thresholds and SQL results that also contain a refresh_key column.
• TypeScript build passed; lint completed without errors.

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated
cubed3	🟢 Ready	View Preview	Sep 25, 2026, 2:35 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

[claude]

Claude finished @ovr's task in 2m 38s —— View job

---

Review: approach is sound — 0 blocking, 1 medium (synchronized refresh boundaries), 3 low

Full review — expand for details

Todo

• Gather context (diff, changed files, call sites)
• Review snapToRenewalThreshold semantics & edge cases
• Review QueryCache integration
• Review RefreshScheduler behaviour change
• Review tests & docs
• Post findings

Summary

The core idea — reproduce the cache-derived rate bound arithmetically instead of vetoing local evaluation — is correct and a genuine improvement over the TODO it replaces. I verified the pieces that could silently be wrong:

• Units line up. refreshKeyRenewalThreshold is seconds everywhere (optionsValidate.ts:128, QueryCache.ts:526); snapToRenewalThreshold multiplies by 1000 before flooring and evaluateLocalRefreshKey takes ms. ✅
• The "don't snap to the per-key threshold" comment is accurate. BaseQuery.refreshKeyRenewalThresholdForInterval returns min(max(round(interval/10), 1), 300) — a fraction of the interval — so snapping to it would indeed only delay the boundary without changing the rate. It also matches cacheRefreshKeyResult, where the global option already takes precedence over the per-key one. ✅
• threshold = 0 is a no-op in both paths. Joi permits 0, snapToRenewalThreshold returns nowMs unchanged, and cacheRefreshKeyResult's || chain already treated 0 as unset. Consistent. ✅
• Monotonicity holds — flooring a monotonic clock is monotonic, and the test asserts it.
• T < interval is harmless, T > interval gives the intended once-per-window advance, and snapping keeps the value inside the key's own series (the 144 case) rather than renumbering it — that property is what makes turning the threshold on/off not trigger a mass rebuild.

Findings below, none blocking.

Medium — refresh keys now all flip at the same instant

snapToRenewalThreshold aligns to the Unix epoch, so with refreshKeyRenewalThreshold: 86400 every every-based key across every cube, tenant and timezone advances at exactly 00:00 UTC. The SQL path bounded the rate identically but phased each key by when its cache entry was written, spreading rebuilds out. Cross-instance determinism is a real win; the burst is the cost. A per-key phase offset seeded from QueryCache.refreshKeyIdentity would buy both — see the inline comment on utils.ts.

Low

1. Stale comment. packages/cubejs-server-core/src/core/RefreshScheduler.ts:363 still reads "and so do interval keys whenever the cache declines to evaluate them locally", which was written for the threshold veto this PR removes. It's not false — localRefreshKeyResult still declines on an invalid/absent descriptor — but the case it was describing is gone, so it now points at nothing a reader can find.
2. Warming gap widens slightly. isLocalRefreshKeyActive() is a global flag, while localRefreshKeyResult also requires a valid per-key descriptor. Where those disagree (an every key that yields no descriptor), the scheduler skips warming but the cache still falls back to SQL, so the key query moves onto the user query path. Pre-existing for the threshold-unset case; this PR extends it to threshold-set deployments too. Probably fine given isValidLocalRefreshKey should hold for any real every key — flagging so it's a deliberate call rather than an accident.
3. Test reads the real clock after the fact — inline comment on QueryCache.abstract.ts:644. Once-a-day flake at the UTC boundary; the sibling test you added in PreAggregations.test.ts already mocks Date.now and reads better for it.

Nits (no action needed)

• <number>thresholdSeconds appears twice because Number.isFinite doesn't narrow number | undefined. typeof thresholdSeconds !== 'number' || !Number.isFinite(...) would narrow and drop both casts.
• Infinity falls through the isFinite guard to "no snapping", which reads as the opposite of what an infinite threshold means. Unreachable through Joi (.integer() rejects it) and the test pins the current behaviour, so it's only a semantic curiosity.

Tests & docs

Coverage is good: the helper is tested directly (boundaries, monotonicity, the every-key series, and a cron key with a dayOffset), and both QueryCache paths plus the scheduler are updated rather than deleted. The cron test in particular is the one I'd have asked for. Docs correctly drop the old "so do all refresh keys if you have set…" carve-out; adding a line that thresholds align to UTC boundaries would round it out.

Not verified

I could not run yarn unit or yarn lint — node_modules is absent in this checkout and installing the monorepo wasn't warranted for a review. All findings above come from reading the code and its call sites, not from a test run.

• branch [`refresh-key-renewal-threshold-comment`](https://github.com/cube-js/cube/tree/refresh-key-renewal-threshold-comment)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.27%. Comparing base (5dd3e2f) to head (f9784ab).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11720      +/-   ##
==========================================
+ Coverage   62.21%   62.27%   +0.06%     
==========================================
  Files         248      248              
  Lines       19979    19982       +3     
  Branches     4072     4072              
==========================================
+ Hits        12430    12444      +14     
+ Misses       6958     6948      -10     
+ Partials      591      590       -1     

Flag	Coverage Δ
cube-backend	62.27% <100.00%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @ovr's task in 1m 37s —— View job

---

Re-review after rebase: no code changes since the last round — 0 blocking, 2 prior threads re-affirmed, 2 low

Full review — expand for details

Todo

• Gather context (diff, changed files, call sites)
• Re-check prior review threads against the new commit
• Review snapToRenewalThreshold + QueryCache integration
• Review RefreshScheduler behaviour change, tests, docs
• Post findings

What changed since the last review

The synchronize was a rebase (single commit eed2627, previously e97da3a). git diff origin/master...HEAD is substantively identical to the version I reviewed on Sep 1 — same snapToRenewalThreshold, same QueryCache wiring, same tests, same docs paragraph. Nothing in the two open threads has been addressed, so I re-affirmed them rather than re-posting:

• packages/cubejs-query-orchestrator/src/orchestrator/utils.ts:70 — epoch-aligned snapping synchronises every key's boundary (medium, ops trade-off).
• packages/cubejs-query-orchestrator/test/unit/QueryCache.abstract.ts:646 — test re-reads the real clock after the call (nit, once-a-day flake).

Both left unresolved; no reply posted on them.

Re-verified this round

• Units. refreshKeyRenewalThreshold is seconds (optionsValidate.ts:128 Joi.number().min(0).integer(), QueryCache.ts:526); snapToRenewalThreshold multiplies by 1000 before flooring, evaluateLocalRefreshKey takes ms. ✅
• threshold = 0 is a no-op in both paths — snapToRenewalThreshold returns nowMs, and cacheRefreshKeyResult's || chain already treated 0 as unset. ✅
• The "don't snap to the per-key threshold" comment matches BaseQuery.refreshKeyRenewalThresholdForInterval (min(max(round(interval/10), 1), 300)) and matches the precedence in cacheRefreshKeyResult. ✅
• Cron numbering. Re-derived the utils.test.ts cron case by hand: interval: 86400, dayOffset: 36000, threshold 14400 → -1 at 09:59:59, still -1 at 10:00 (snapped to 08:00), 0 at 12:00. The 10:00 fire is observed at the 12:00 sample, exactly as a 4-hour cache would. Assertions are right. ✅
• No renumbering. Snapping picks an earlier member of the same series (144 is genuinely a 10-minute key's value at the start of day two), so flipping the threshold on or off doesn't invalidate every contentVersion. That's the property that makes this safe to ship. ✅
• Scheduler. isLocalRefreshKeyActive() is now just localRefreshKeyEnabled, so refreshCubesRefreshKey skips warming interval keys under a threshold too — correct, since there is no cache entry to warm any more.

Low

1. Stale comment. packages/cubejs-server-core/src/core/RefreshScheduler.ts:363 still reads "and so do interval keys whenever the cache declines to evaluate them locally". Written for the threshold veto this PR removes; the only remaining decline is an invalid/absent descriptor, which the scheduler does not fall through for — it returns early on the global flag regardless. A reader now can't find the case it describes. Fix this →
2. Warming gap. isLocalRefreshKeyActive() is global while localRefreshKeyResult additionally requires a valid per-key descriptor. Where they disagree (an every key yielding no descriptor), the scheduler skips warming but the cache still falls back to SQL, moving that key query onto the user query path. Pre-existing for the threshold-unset case; this PR extends it to threshold-set deployments. Likely fine since isValidLocalRefreshKey holds for any real every key — flagging so it's deliberate.

Nits (no action needed)

• <number>thresholdSeconds twice because Number.isFinite doesn't narrow number | undefined; typeof thresholdSeconds !== 'number' || !Number.isFinite(thresholdSeconds) would narrow and drop both casts.
• Infinity falls through to "no snapping", the semantic opposite of an infinite threshold. Unreachable via Joi (.integer()), and the test pins it.

Not verified

node_modules is absent in this checkout, so I did not run yarn unit or yarn lint. All findings come from reading the code and its call sites.

• branch [`refresh-key-renewal-threshold-comment`](https://github.com/cube-js/cube/tree/refresh-key-renewal-threshold-comment)

[ovr]

Review (local, ovr): all findings addressed in c4d2436 — 0 open

Full review — expand for details

Fixed in c4d2436

• Medium, QueryCache.ts:255 — snap window is now min(refreshKeyRenewalThreshold, cache entry TTL); cacheRefreshKeyResult passes the entry TTL and identity into localRefreshKeyResult. The bound matches what the SQL path actually did.
• Medium, CI thread utils.ts:70 — the window is phased per key by refreshKeyPhaseSeed (first 32 bits of the identity md5): deterministic across instances, staggered across keys.
• Low, utils.ts:55-60 — JSDoc trimmed to two lines.
• Low, CI RefreshScheduler.ts:363 — stale sentence about the removed veto dropped.
• Low, CI QueryCache.abstract.ts:646 — Date.now mocked; expectation derived from the exported helpers, plus a second call an hour later to pin the TTL cap.

Verified locally

Suite	Result
query-orchestrator utils.test.ts, QueryCache.test.ts, PreAggregations.test.ts	130 / 130 pass
server-core RefreshScheduler.test.ts (local refresh key cases)	3 / 3 pass
oxlint on the six changed files	clean

My two inline threads are resolved; the two CI threads have a reply and are left for the author/CI to resolve. PR description updated to describe the TTL cap and per-key phase.

[claude]

Claude finished @ovr's task in 4m 9s —— View job

---

Re-review of c4d2436: both prior threads addressed — 0 blocking, 1 low (new), 2 threads resolved

Full review — expand for details

Todo

• Gather context (diff, changed files, call sites)
• Check prior review threads against the new commit
• Review the TTL cap + per-key phasing
• Review tests & docs
• Post findings

What changed

c4d2436 does two things the previous round asked for, and one thing it didn't:

1. Caps the snap window at the cache entry TTL. I re-verified the premise independently: cacheQueryResult returns fetchAndCacheQuery on Missing cache, and the in-memory shortcut is bounded by the same TTL (isMemoryEntryUsable, QueryCache.ts:1073: renewedAgo > expiration * 1000 → unusable). So the SQL path's effective re-read period really was min(threshold, expiration), and Math.min(threshold, bound.expiration) models it faithfully. ✅
2. Phases the window per key. refreshKeyPhaseSeed = first 4 bytes of the md5 of refreshKeyIdentity, and getCacheHash only mixes in processUid for persistent keys — an identity array is never persistent, so the seed is stable across instances and restarts. The offset arithmetic still yields snapped ≤ now for any offset, so the value stays a real past instant in the key's own series, and monotonicity survives (now asserted across three phases). ✅
3. Drops the stale RefreshScheduler.ts sentence and trims the snapToRenewalThreshold JSDoc from six lines to two — both outstanding low items.

Resolved my two prior threads (utils.ts epoch alignment, QueryCache.abstract.ts real-clock read); ovr's two were already resolved by them. One new low posted inline.

Low (new) — QueryCache.ts:263

The window is now derived from the caller's TTL while the cache key still deliberately ignores it, so one refresh key identity can return two values at the same instant: 3600 via PreAggregationLoadCache.keyQueryResult, expireSecs || 86400 via loadRefreshKey. Both paths are reachable for the same SQL — a pre-aggregation without its own refreshKey gets preAggregationQueryForSql.cacheKeyQueries() verbatim (BaseQuery.js:5202). Each consumer is self-consistent (pre-agg reads are always 3600), so I could not construct a rebuild loop; what it does break is the "one entry per identity" invariant the comments at QueryCache.ts:56-57 and :493-497 assert. Details and two fix directions in the inline comment.

Folded in there: bound is optional, but every production call site passes it and the ?? threshold / : 0 fallbacks silently restore uncapped, unphased snapping — worth making required.

Also checked, no finding

• Phase seed edge cases. A seed larger than the window wraps; a NaN seed (possible only if a bare string CacheKey were ever passed, since parseInt of a hex prefix is otherwise finite) falls back to offset 0. Both pinned by utils.test.ts.
• threshold = 0 remains a no-op on both paths; window becomes undefined and snapToRenewalThreshold returns nowMs.
• cacheKey hoisted above the local branch in cacheRefreshKeyResult — same value, same refreshKeyIdentity call, just computed unconditionally now. No behaviour change for the SQL path.
• Docs now say the window is capped at the old re-read period and that each key is offset by its identity. Matches the code.
• Tests. PreAggregations.test.ts and QueryCache.abstract.ts both pin Date.now and assert against evaluateLocalRefreshKey(descriptor, snapToRenewalThreshold(...)); the new QueryCache.abstract.ts case walks the phased hour boundary from below, inside, and above, which is the assertion that would catch an off-by-one in the offset arithmetic. The literal '174'/'318' expectations in utils.test.ts keep at least one test independent of the implementation's own formula.

Not verified

node_modules is absent in this checkout, so yarn unit and yarn lint did not run. @ovr's earlier round reported 122/122 + 3/3 green locally on the previous commit; the assertions this commit rewrote are the ones that would need re-running. Everything above comes from reading the code and its call sites.

• branch [`refresh-key-renewal-threshold-comment`](https://github.com/cube-js/cube/tree/refresh-key-renewal-threshold-comment)

[claude]

Claude finished @ovr's task in 3m 4s —— View job

---

Re-review of b5bca77: prior thread addressed — 0 blocking, 2 low (both wording/docs)

Full review — expand for details

Todo

• Gather context (diff, changed files, call sites)
• Check prior review threads against the new commit
• Review the shared-TTL change
• Review tests & docs
• Post findings

What changed

b5bca77 takes the fix direction my last thread suggested: a shared REFRESH_KEY_CACHE_TTL = 60 * 60 exported from QueryCache.ts, used both as PreAggregationLoadCache.keyQueryResult's expiration and as the snap-window cap, so the cap and the phase both derive from the key and not from the caller. bound is gone — localRefreshKeyResult(queryOptions, cacheKey) now takes the identity as a required second argument, so the silent unphased/uncapped fallbacks are gone too. That closes the "one identity, two values" divergence; I resolved the thread.

New tests pin the property directly (yields one value per identity whatever expiration the caller passes, and the 03:10 case with literals 162/162 where the per-caller cap gave 162/144), plus a guard test asserting the loader's TTL is the constant so the two can't drift apart. That last one is the right test to have written.

Low (2)

1. environment-variables.mdx:1469-1472 — "at most once per threshold window" no longer holds. The window is min(threshold, 3600), so refreshKeyRenewalThreshold: 86400 advances a 10-minute key 24 times per window, which utils.test.ts itself asserts. The second clause is the accurate one; the one-hour ceiling isn't stated anywhere a user would look. Suggestion inline.
2. QueryCache.ts:249-253 — the JSDoc's justification covers one caller. "the SQL path re-read the entry once it expired whatever the threshold said" is true of the pre-agg loader, but loadRefreshKey writes the entry with expireSecs || 24 * 3600 (getExpireSecs, :474), so the hour cap makes that path advance faster than the SQL path did — up to 24× more raw-query re-execution with a day-long threshold. I think choosing the shorter TTL is the right call and it's what I asked for; the comment just shouldn't read as behaviour-preserving for both. Wording inline.

Verified this round

• min is always the hour in practice. queryBody.expireSecs is never produced anywhere in the repo (only QueryOrchestrator.test.js:879 sets it), so getExpireSecs is 86400 and no path ever becomes slower than the SQL path was — the cadence change is one-directional and on the safe side. ✅
• One TTL, one constant. cacheRefreshKeyResult is reached from exactly two places (PreAggregationLoadCache.ts:197, loadRefreshKey → loadRefreshKeys → loadRefreshKeysFromQuery / cachedQueryResult); the constant covers the first and caps the second. No third refresh-key TTL hiding elsewhere. ✅
• No import cycle — PreAggregationLoadCache already imported QueryCache, and the constant is a plain value, so nothing new at module-init time. ✅
• Phase seed arithmetic. refreshKeyPhaseSeed is < 2³² and offset = seed % thresholdMs wraps into the 3.6 M ms window; snapped ≤ now holds for every offset, so the value stays a real past instant in the key's own series, and monotonicity is asserted across three phases. NaN seed → offset 0. ✅
• threshold = 0 still a no-op: window is undefined, snapToRenewalThreshold returns nowMs. ✅
• REFRESH_KEY_CACHE_TTL is exported through src/index.ts (export * from './orchestrator'), which is what both test files import it from. ✅
• jest.spyOn(utils, 'refreshKeyPhaseSeed') in the 03:10 test works because module: nodenext in a CJS package emits (0, utils_1.refreshKeyPhaseSeed)(…) at the QueryCache.ts call site — the spy is intercepted. Slightly emit-dependent, but correct here, and the test needs a zero phase for its literals.

Not verified

node_modules is absent in this checkout, so yarn unit and yarn lint did not run. The assertions this commit rewrote (QueryCache.abstract.ts, PreAggregations.test.ts, utils.test.ts) are the ones worth re-running locally; @ovr reported green on the previous commit. Everything above comes from reading the code and its call sites.

• branch [`refresh-key-renewal-threshold-comment`](https://github.com/cube-js/cube/tree/refresh-key-renewal-threshold-comment)

[claude]

Claude finished @ovr's task in 2m 6s —— View job

---

Review: no blocking issues. 1 medium (PR description is out of date), 2 low (inline).

Full review: expand for details

• Read diff and changed files
• Analyze correctness, tests, docs, comments
• Reconcile prior review threads
• Post review

Findings

Medium: the PR description describes a design that was removed. The body still talks about snapToRenewalThreshold, phasing by refreshKeyPhaseSeed, and passing the TTL and identity into localRefreshKeyResult. It also says "RefreshScheduler consequently stops warming interval keys under a threshold". None of that is in the code now, and on the scheduler point the code does the opposite: RefreshScheduler.ts:347-349 keeps warming interval keys when a threshold is set, and skips them only when there is no threshold. The current design, from 5a66b86, is:

• Without a threshold, the key is evaluated directly, with no cache and no queue.
• With a threshold, the key goes through cacheQueryResult with the caller's TTL, renewal threshold and renewal key. The queue's query handler (QueryCache.ts:800-802) evaluates the descriptor instead of running SQL.

Rewrite the description to match before merging. merge-pr takes the squash body from --body, but reviewers read the description.

Low (inline): QueryCache.ts:248-252: _cacheKey is dead, and its "Retained for API compatibility" comment is wrong. The parameter was added in this PR, and the master signature is (queryOptions?).

Low (inline): QueryCache.ts:519-524: the eligibility test is duplicated with QueryCache.ts:253. Extract one helper so the two paths can't drift apart.

Note, not raised inline: REFRESH_KEY_CACHE_TTL (QueryCache.ts:39) came in with the "share refresh key snap TTL" commit. Now that snapping is gone, only the pre-aggregation loader and its test use it. It's harmless, but reverting to the inline 60 * 60 would shrink the diff.

What checks out

• Semantics match the SQL path. Routing the threshold case through cacheQueryResult and evaluating inside the queue gives the SQL path's TTL expiry, renewal measured from write time, background renewal, and queue dedup across instances. The earlier concerns about the one-hour cap and the per-instance phase are gone by construction.
• No database connection. The queue handler returns before clientFactory(), so local keys under a threshold never open a connection. Both the memory and CubeStore suites assert factory is never called.
• Safe during a mixed-version rollout. An older worker that picks up a request with localRefreshKey ignores the field and runs the SQL, which gives the same value. LocalRefreshKeyCache.test.ts:123 covers entries written in one mode and read in the other.
• Incremental keys are excluded. They stay on SQL both in the schema compiler (BaseQuery.js:5150) and, defensively, in the orchestrator (QueryCache.ts:519). A test covers it (LocalRefreshKeyCache.test.ts:136).
• Tests. They compare SQL and local mode side by side under a fake clock for TTL and threshold expiry, wall-clock boundaries, readers with different TTLs, background renewal, cache failures, and concurrent dedup across two instances. That is good coverage.
• Docs. The paragraph at environment-variables.mdx:1539-1543 matches the current behaviour.
• Comments. All new comments are three lines or fewer and explain why the code does what it does.

Tests weren't run in this pass: the review relied on reading the code, and the author's round reports them passing.

Review threads

The listing returned 2 of my own threads. I resolved both: PRRT_kwDOCOH19M6iE0RZ (the docs overstating the one-hour cap) and PRRT_kwDOCOH19M6iE0pP (the misleading JSDoc on the snap TTL). The code they referred to has been removed. I re-affirmed none, and neither new finding duplicated an existing thread. I didn't touch the threads opened by ovr.

[claude]

Review: no correctness issues found. 1 new low finding; 4 earlier threads still apply (2 low, plus 1 duplicate and 1 design note)

Details

• Read diff and changed files
• Analyze correctness, tests, docs, comments
• Reconcile prior review threads
• Post findings

Verdict

The queue-based design looks correct. Under a refreshKeyRenewalThreshold, the local key uses the same refreshKeyIdentity cache key, entry TTL, renewal key and queue key (cacheKey) as the SQL key. That keeps cross-instance dedup and SQL↔local cache reuse working. Evaluation happens in the queue handler (QueryCache.ts:800) when the job runs, not when it is enqueued, so a job that waits in a backlog doesn't return a stale value. The descriptor is plain JSON, so it survives the Cube Store queue payload. An older instance that picks up the job ignores localRefreshKey and runs the SQL instead, which gives the same result, so a mixed-version fleet is safe. Planner-independent: localRefreshKey descriptors come from BaseQuery.localRefreshKeyOptions.

New finding (posted inline)

Severity	Location	Issue
Low	packages/cubejs-query-orchestrator/src/orchestrator/QueryCache.ts:258	isValidLocalRefreshKey isn't a type predicate, so this line needs ?? null and the handler at :800 passes an untyped value.

Earlier threads that still apply (no new comments posted)

• packages/cubejs-query-orchestrator/src/orchestrator/QueryCache.ts:248: localRefreshKeyResult is no longer called outside tests.
• packages/cubejs-server-core/src/core/RefreshScheduler.ts:348 (threads on both :348 and :349): the scheduler repeats the check isLocalRefreshKeyActive() && !options.refreshKeyRenewalThreshold from QueryCache.ts:522. One QueryCache method for it would stop the two copies drifting apart.
• packages/cubejs-query-orchestrator/src/orchestrator/QueryCache.ts:800: a local key computed under a threshold still waits for a slot in the data-source queue. That matches the SQL behaviour it replaces, so it's a design note, not a regression.

Tests / docs / comments

• The tests cover the claimed behaviour, including TTL vs renewal, the wall-clock boundary, mode switching, concurrent-miss dedup with evaluation when the job runs, and warming without SQL. The incremental exclusion is tested at LocalRefreshKeyCache.test.ts:127.
• The docs paragraph in environment-variables.mdx matches the implementation.
• All new comments are ≤3 lines and explain intent. None need flagging.
• I didn't run the tests in this round; this was a read-only review.

Review threads

The listing returned 4 unresolved threads of mine. I resolved 0 and re-affirmed all 4, since each concern is still present in the current code. The threads on RefreshScheduler.ts:348 and :349 are duplicates of each other.

View job run

[claude]

Reviewing PR #11720

• Read diff and changed files
• Analyze correctness, tests, docs, comments
• Reconcile prior review threads
• Post review

View job run

[claude]

Claude finished @ovr's task in 2m 54s —— View job

---

Review: no blocking issues. 1 medium, 5 low (2 new inline, 3 re-affirmed open threads). The PR description contradicts the code.

Full review

• Gather context
• Read changed files and diff
• Analyze correctness, tests, docs, comments
• Handle prior review threads
• Post review

Medium: the PR description describes a queue design that the last commits removed

The description says local keys use "the same … query queue as SQL refresh keys" and that "the queue handler evaluates the local descriptor at execution time". It also claims "concurrent-miss deduplication", and its Test Coverage section lists "Shared queue tests … concurrent-miss deduplication".

The code does not do this:

• Since 2b55648, fetchAndCacheQuery evaluates the descriptor inline (QueryCache.ts:1132). It never calls queryWithRetryAndRelease.
• The comment at QueryCache.ts:526-527 says local evaluation "bypasses queue slots and deduplication".
• The docs say local computations "are not deduplicated across instances".
• The tests in QueryCache.abstract.ts assert that getQueue and getExternalQueue are not called.

PRs are squash-merged, so update the description before merging. Otherwise the stale design ends up in the history as the rationale.

Low (new inline threads)

• packages/cubejs-server-core/test/unit/RefreshScheduler.test.ts:1306: localEntries finds local writes by checking for a result[0].refresh_key column. It excludes sql key entries only because the mock SQL rows happen to lack that column.
• docs-mintlify/reference/configuration/environment-variables.mdx:1539-1545: the paragraph is seven sentences, two of them about queue internals. There is a suggested four-line replacement.

Low (re-affirmed, still open from earlier rounds)

• QueryCache.ts:248: the public localRefreshKeyResult has no production callers and always evaluates without the cache, ignoring refreshKeyRenewalThreshold.
• RefreshScheduler.ts:348: the scheduler repeats the "local key without a cache entry" check that QueryCache.cacheRefreshKeyResult:522 owns, so the two can drift apart.
• QueryCache.ts:258: the ?? null exists only because isValidLocalRefreshKey returns boolean instead of narrowing the type.

Checked, no issue

• Cache behaviour: with a threshold set, the local path reuses the same identity, renewalKey, TTL, decideCacheAction and in-memory rules as the SQL path. So the effective bound is still min(threshold, entry TTL), which answers the earlier snapping concern. Entries written by either mode are reusable by the other.
• Incremental keys: they are now excluded explicitly in localRefreshKeyFor, with a test.
• Threshold of 0: it is treated as "no override" in QueryCache and RefreshScheduler, which matches the Joi min(0).
• Failures: a cache get or set failure on the local path propagates the same way as on the SQL path, and Dropping Cache removes the key.

Review threads

The listing returned 4 of my own unresolved threads. I resolved 0 and re-affirmed 3; the 4th (RefreshScheduler.ts:349) duplicates the line-348 thread and stays open. There are no human-opened threads.

Tests

Tests and lint were not run: node_modules is not installed in this checkout.

· [Branch](https://github.com/cube-js/cube/tree/refresh-key-renewal-threshold-comment)

[claude]

Claude finished @ovr's task in 2m 29s —— View job

---

Review: approve once the PR description is fixed. 1 medium (PR description), 4 low (2 new, 2 still open from earlier rounds), no correctness bugs found.

Full review

• Read diff and changed files
• Analyze correctness, tests, docs, comments
• Reconcile prior review threads
• Post review

Medium: the PR description contradicts the code

The description says local keys "use the same shared cache identity, entry TTL, renewal rules, and query queue as SQL refresh keys", and that "the queue handler evaluates the local descriptor". It also lists "concurrent-miss deduplication" and "Shared queue tests" as preserved or covered. Commit 2b55648 removed that behaviour:

• fetchAndCacheQuery at packages/cubejs-query-orchestrator/src/orchestrator/QueryCache.ts:1132 evaluates the key inline, skipping queryWithRetryAndRelease.
• The tests at QueryCache.abstract.ts assert that getQueue and getExternalQueue are never called.

As a result, concurrent misses are not deduplicated. Each caller evaluates the key and issues its own cacheDriver.set. That costs little, since no database is involved, but the description is squash-merged into history. Update the description to match the comment at QueryCache.ts:527-528.

New inline findings (low)

• packages/cubejs-query-orchestrator/test/unit/PreAggregations.test.ts:665: the comment claims "with the loader TTL", but the test never asserts a TTL.
• packages/cubejs-query-orchestrator/test/unit/LocalRefreshKeyCache.test.ts:47: the comment repeats the case name and refers to the removed snap approach.

Still open from earlier rounds (re-affirmed)

• QueryCache.ts:248: localRefreshKeyResult has no production caller left, and it ignores the threshold.
• RefreshScheduler.ts:348: the scheduler repeats the "uncached local key" check from QueryCache.ts:522 instead of calling a single QueryCache method.

Verified correct

• TTL cap: the effective cap is now min(threshold, entry TTL), because the local value lives in the same entry, with the same expiration, as the SQL result. This settles the TTL-cap concern from the earlier snap-based round.
• Renewal timing: renewal is measured from the entry write time (decideCacheAction), so there is no wall-clock phase problem left.
• Incremental keys: localRefreshKeyFor now excludes them explicitly (QueryCache.ts:254). Before this change they were excluded only because they carry no descriptor.
• Docs: the environment-variables.mdx wording matches the new behaviour.

I did not run the tests in this round. The review is based on reading the code.

Review threads

The listing returned 5 unresolved threads, all mine, and I resolved none. All five concerns are still present in the code:

• QueryCache.ts:248
• QueryCache.ts:258
• RefreshScheduler.ts:348 and RefreshScheduler.ts:349 (the same concern in two threads)
• RefreshScheduler.test.ts:1306

Two current findings match open threads and were not reposted. Two new inline comments were posted.

[claude]

Claude finished @ovr's task in 1m 57s —— View job

---

Review: no blocking issues. 1 medium, 3 low. The medium finding is that the PR description still describes the queue-based design that commit 2b55648 removed.

Full review

• Gather context (diff, changed files)
• Review code, tests, docs, comments
• Handle prior review threads
• Post findings

Medium: the PR description doesn't match the code (top-level only)

The description says local keys use "the same … query queue as SQL refresh keys". It also says "the queue handler evaluates the local descriptor at execution time", and that this "preserves cross-instance sharing, concurrent-miss deduplication". It lists "concurrent-miss deduplication" under the shared queue tests.

After 2b55648 ("evaluate local refresh keys without the queue"), none of that holds:

• fetchAndCacheQuery evaluates the descriptor inline and never enqueues (QueryCache.ts:1132-1144).
• The code comment at QueryCache.ts:526-527 says local evaluation "bypasses queue slots and deduplication".
• The tests in QueryCache.abstract.ts assert that neither queue is touched.
• No dedup test exists.

PRs are squash-merged, so update the description before merging to state what now happens: the shared cache entry is kept, the queue is bypassed and there's no dedup.

Inline

Severity	Site	Finding
Low	QueryCache.ts:1132	Without queue dedup, concurrent misses or renewals across an every boundary can write K after K+1; the stale value then sticks for the renewal threshold
Low	QueryCache.ts:36	The JSDoc on REFRESH_KEY_CACHE_TTL restates the name; put the unit in the name instead
Low	QueryCache.ts:244	isLocalRefreshKeyActive() is now a trivial alias with one caller

Checked and fine

• TTL / renewal bounds: with a threshold, local keys go through cacheQueryResult with the caller's expiration. The effective bound is therefore min(threshold, TTL), the same as the SQL path, which resolves the concern from the earlier snap-based round. LocalRefreshKeyCache.test.ts covers this against a SQL stand-in, including reads from mixed TTLs and a switch between SQL and local evaluation.
• Fallback cases: incremental, malformed and SQL-defined keys still take the SQL path (localRefreshKeyFor, QueryCache.ts:253-259), and there's a test for each.
• Scheduled refresh: RefreshScheduler skips warming only when there's no threshold (usesUncachedLocalRefreshKey). With a threshold, the test asserts cache entries are written and no interval SQL runs.
• Docs: the env-var paragraph matches the new behaviour.
• Legacy planner vs Tesseract: no difference; the change is confined to the orchestrator and scheduler.

Not run

I couldn't run the tests: npx jest needed an approval the CI sandbox doesn't grant. All findings come from reading the code.

Review threads

The listing returned 0 of my own threads (no prior threads), so there was nothing to resolve or re-affirm. All 3 inline findings are new.

[claude]

Claude finished @ovr's task in 2m 18s —— View job

---

Review of #11720: looks good to merge. 0 high, 0 medium, 1 low (posted inline).

Full review

• Read the diff against 5dd3e2f and the changed files
• Review source changes
• Review tests and docs
• Handle prior review threads
• Post review

Findings

• Low, packages/cubejs-query-orchestrator/src/orchestrator/QueryCache.ts:36: REFRESH_KEY_CACHE_TTL_SECONDS reads as the TTL for every refresh key entry. It is actually only the pre-aggregation loader's TTL: loadRefreshKey writes the same entries with getExpireSecs(query), which defaults to 86400. The PR description also calls it REFRESH_KEY_CACHE_TTL. (inline)

Verified, no issues

• Cached local path (QueryCache.ts:504-529, :1124-1136): with a threshold, the renewal threshold, renewalKey, caller TTL, in-memory cache and background renewal are unchanged. Only the value source changes. None of the three fetchAndCacheQuery call sites (forceNoCache, miss, WaitForRenew) nor the background renewal enter the queue for a valid descriptor. The key still advances at most once per refreshKeyRenewalThreshold, as it did on the SQL path.
• Option leakage: localRefreshKey is set explicitly after ...options, so it is null for ineligible keys. The other cacheQueryResult callers never pass the option: QueryCache.ts:412, :961, QueryOrchestrator.ts:489 and PreAggregationPartitionRangeLoader.ts:97.
• Incremental keys: BaseQuery.js:5150 already drops the descriptor, so the new queryOptions?.incremental guard in localRefreshKeyFor is only a second line of defence. It is covered by LocalRefreshKeyCache.test.ts:126. This applies to both planners, because refresh key SQL comes from BaseQuery either way.
• Scheduler (RefreshScheduler.ts:347-362): usesUncachedLocalRefreshKey() treats 0 and unset the same way as cacheRefreshKeyResult does. The tests check which cache entries were warmed by their identity rather than by SQL text.
• Comments: every new comment is 2 lines or fewer and gives a reason (last-writer-wins, entries warmed without the queue).
• Docs: the env-var page matches the new behaviour for set, zero and unset thresholds.
• Tests: good coverage of TTL vs threshold, mixed-TTL readers, background renewal, cache get/set failures, SQL/local mode switching, and the fallback cases. I did not run the tests or the build in this job.

Review threads

The listing returned 0 of my own threads, so none were resolved or re-affirmed. There are no unresolved human threads either.

[claude]

Low: the name says this is the TTL for every refresh key entry, but it is only the pre-aggregation loader's. loadRefreshKey (QueryCache.ts:1014) writes the same entries with getExpireSecs(query), which defaults to 86400. The only caller is PreAggregationLoadCache.ts:199. Someone reading REFRESH_KEY_CACHE_TTL_SECONDS could reasonably assume it applies to all refresh key entries and reuse it in the query path. A loader-specific name such as PRE_AGGREGATION_REFRESH_KEY_TTL_SECONDS, or a non-exported constant in PreAggregationLoadCache.ts, keeps the scope visible. The PR description also calls it REFRESH_KEY_CACHE_TTL.

[claude]

Claude finished @ovr's task in 1m 55s —— View job

---

No new issues; 1 low issue from an earlier round is still open (0 high, 0 medium, 1 low).

Review details

• Read diff and changed files
• Analyse cache / local-evaluation paths
• Review tests and docs
• Reconcile prior review threads
• Post findings

Findings

No new inline comments this round. I checked:

• Cached path with a threshold (QueryCache.ts:504-528)

  • A local descriptor now becomes a fetchResult producer inside fetchAndCacheQuery (QueryCache.ts:1123).
  • Cache key, caller TTL, renewalKey, in-memory gating, waitForRenew, background renewal and cache-drop-on-error are unchanged from the SQL path.
  • Local evaluation never reaches queryWithRetryAndRelease, so it doesn't queue and doesn't open a driver.

• Uncached path (QueryCache.ts:512)

  • Unset or 0 thresholds evaluate directly.
  • usesUncachedLocalRefreshKey() is the only switch, and RefreshScheduler.ts:348-363 reads the same method, so bypass and warming can't drift apart.

• Fallbacks: incremental, sql, and missing or invalid descriptors return null from localRefreshKeyFor (QueryCache.ts:245-251) and keep SQL execution. isValidLocalRefreshKey is now a type guard, so the cast is gone.

• Duplicate requests: loadRefreshKey is @AsyncDebounced, so the last-writer-wins behaviour in the description only happens across instances or the pre-aggregation load-cache path. The description states the limits accurately.

• Tests: LocalRefreshKeyCache.test.ts covers:

  • TTL vs threshold
  • mixed-TTL readers
  • background renewal
  • cache get/set failures
  • switching between SQL and local mode in both directions
  • all four fallback kinds
  • 0/undefined thresholds, with independent SQL and local caches cross-checked

  I didn't run the tests locally because running jest in this CI sandbox needs approval.

• Comments: all within 3 lines and each carries a reason.

• Docs: the env-var paragraph matches the behaviour.

Review threads

The listing returned 1 of my own threads: 0 resolved, 1 still applies.

• Still applies: the REFRESH_KEY_CACHE_TTL_SECONDS naming thread (QueryCache.ts:36). The name still suggests a TTL for every refresh key, but its only users are the pre-aggregation loaders (PreAggregationLoadCache.ts:199, PreAggregationPartitionRangeLoader.ts:420). The query path uses getExpireSecs.
• There are no unresolved threads opened by humans.

[claude]

Claude finished @ovr's task in 2m 8s —— View job

---

No new issues. 1 low-severity point from an earlier thread still applies (the constant name).

Review details

• Read diff and changed files
• Analyze correctness, tests, docs, comments
• Reconcile prior review threads
• Post inline findings and summary

Still open

• Low, packages/cubejs-query-orchestrator/src/orchestrator/QueryCache.ts:36: REFRESH_KEY_CACHE_TTL_SECONDS is misnamed. There's already an open thread on this. There's one more case for renaming it: PreAggregationPartitionRangeLoader.ts:420 passes it as expireSecs to renewQuery in downloadLambdaTable. That value is also the TTL of the cache entry for the lambda table's CSV data (QueryCache.ts:964), not just for refresh keys. So if someone later changes the refresh-key TTL through this constant, the lambda data TTL changes with it. Either keep a literal/separate constant at the lambda call site, or use a name that fits both uses.

What I checked and found correct

• cacheRefreshKeyResult (QueryCache.ts:504-526): with a threshold, the local value takes the existing cacheQueryResult path. Identity, caller TTL, renewalKey, background renewal, and Dropping Cache on failure all stay the same. The fetchResult override is limited to fetchAndCacheQuery (QueryCache.ts:1123). RefreshKeyCacheOptions is a Pick, so callers can't inject their own producer.
• localRefreshKeyFor now rejects incremental explicitly. That covers descriptors that might start carrying an incremental flag later.
• The scheduler check (RefreshScheduler.ts:361) and the cache bypass both use usesUncachedLocalRefreshKey(). They can't drift apart the way isLocalRefreshKeyActive and localRefreshKeyResult could before. The old API has no callers left.
• Threshold 0 is falsy on both paths (!this.options.refreshKeyRenewalThreshold and || queryOptions?.renewalThreshold), so it acts as "no override". The docs and tests match that.
• Tests: LocalRefreshKeyCache.test.ts checks the local path against an independent SQL stand-in. It covers TTL shorter than the threshold, readers with different TTLs, a blocked background write, cache get/set failures, switching between SQL and local mode, and all four fallback reasons. The additions to QueryCache.abstract.ts run the no-queue/no-client assertions against Cube Store too. The scheduler test now identifies interval keys by cache identity instead of a /refresh_key/ regex. That regex would have started matching the sql key once its column was aliased.
• Comments all fit within 3 lines and each explains a real constraint. Nothing to flag.
• Docs (environment-variables.mdx): the paragraph describes the threshold and no-override behaviour accurately.

I didn't run the tests; this was a read-only review.

Review threads

The listing returned 1 of my threads (QueryCache.ts:36, the constant name). I resolved none and re-affirmed that one. I folded the lambda-TTL point above into it instead of opening a duplicate inline thread. None of ovr's threads were touched.