fix(query-orchestrator): Drop touch/used keys when pre-aggregation bu…

[ovr]

No description provided.

[claude]

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

---

Code Review

Verdict: Fix looks correct and well-tested. One race and one semantics question worth addressing.

Details

• Read the diff
• Read surrounding code (strategy dispatch, cache helpers, addTableUsed call sites)
• Review tests

1. Race between fire-and-forget updateLastTouch and removeTableTouched (medium)

PreAggregationLoader.ts:467

const targetTableName = this.targetTableName(newVersionEntry);
this.updateLastTouch(targetTableName);   // fire-and-forget wrapper (line 367-373)

this.updateLastTouch is fire-and-forget — it kicks off preAggregations.updateLastTouch(...), which sets the in-memory guard synchronously but writes to the cache driver asynchronously. On the failure path we then await removeTableTouched(targetTableName).

If the refresh strategy throws quickly (e.g. synchronously, or before the touch's cache-driver write settles), the sequence is:

1. touchCache.set(name, true) — sync
2. cacheDriver.set(touchKey, …) — pending
3. strategy throws
4. touchCache.delete(name) — sync
5. await cacheDriver.remove(touchKey)
6. pending set from step 2 now resolves → key persists, no future writer will re-add it because touchCache no longer has it, but nothing removes it either

For most drivers requests are FIFO on one connection so this is unlikely in practice, but the whole point of this PR is to guarantee no orphan touch keys. An await this.preAggregations.updateLastTouch(targetTableName) at the top (instead of the fire-and-forget wrapper on this path) closes the race deterministically. That change is safe because the strategy already runs async inside cancelCombinator.

2. dropTableUsedKey is false for refreshWriteStrategy — intentional? (low)

PreAggregationLoader.ts:481-484

For the non-readOnly external write path, dropTableUsedKey is set to false, same as readOnly. For readOnly this makes sense (we didn't create the table, the used marker tracks something we don't own). But for refreshWriteStrategy we are writing to the external store, and addTableUsed (line 1028) runs inside dropOrphanedTables on the success path. If a failing write leaves a partial table and the used key is written by another concurrent path, this branch will skip cleanup. Worth a one-line comment explaining why write-strategy also skips, or unifying with the source-strategy behavior.

Also note that on the source-strategy path, addTableUsed only fires after successful build, so removeTableUsed in the catch is usually a no-op. That's fine as belt-and-suspenders, but the asymmetry vs. the external branches deserves a comment.

3. Comment wording (nit)

PreAggregationLoader.ts:497-498

// It's required to remove touch keys, because they are unique per run/table, and it causes a large number of touch keys in the cache store

"unique per run/table" reads oddly — a given tableName has one touch key. The real accumulation happens because each failed attempt produces a new targetTableName (version suffix in newVersionEntry), so retries pile up distinct keys until TTL. Suggest:

// Each failed attempt uses a fresh targetTableName (version suffix), so
// without this cleanup the cache accumulates one dead touch key per retry
// until touchTablePersistTime expires.

4. Tests (nit)

test/unit/PreAggregations.test.ts additions are solid — they exercise both the direct helpers and the end-to-end failure path via PreAggregationLoader.refresh. The LRU-eviction check (re-add after remove) is a nice touch.

Two minor things:

• Consider also asserting the underlying cache driver no longer has the raw redis-key (e.g. queryCache.getCacheDriver().get(preAggregations.tablesTouchRedisKey(table))) — right now the assertions go through tablesTouched()/tablesUsed() which is fine, but a direct read would prove the driver-level remove happened too.
• The failed-build test only covers refreshStoreInSourceStrategy (external: false). Consider adding a case with external: true (readOnly and/or writeStrategy) to lock in the "touch is dropped, used is preserved" behavior for those branches, since that's the branchier part of the new code.

Summary

• 1× medium: touch-key race with fire-and-forget updateLastTouch
• 1× low: dropTableUsedKey=false for write strategy — intent unclear
• 2× nit: comment wording, extra test coverage for external branches

Nothing blocking; the fix does what the title says and CORE-646 will stop bleeding touch keys on repeated build failures.

· branch `fix/refresh-scheduler-used-flag`

[codecov]

Codecov Report

❌ Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.48%. Comparing base (78bb27f) to head (54bed1c).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...hestrator/src/orchestrator/PreAggregationLoader.ts	64.70%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11314      +/-   ##
==========================================
+ Coverage   59.46%   59.48%   +0.02%     
==========================================
  Files         216      216              
  Lines       17482    17501      +19     
  Branches     3541     3544       +3     
==========================================
+ Hits        10395    10411      +16     
- Misses       6580     6583       +3     
  Partials      507      507              

Flag	Coverage Δ
cube-backend	59.48% <71.42%> (+0.02%)	⬆️

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.