[knudtty]

Summary

Concurrent getMapKeys calls with the same arguments now share one ClickHouse query.

The problem

getMapKeys checked its cache with cache.get, ran the query, and stored the result with cache.set only after the query finished. A second call with the same arguments that arrived while the first was still running found nothing in the cache and sent its own query. cache.getOrFetch shares running requests, but getMapKeys only used it on its last fallback (the raw table scan), not on the text index or rollup paths.

On a 537M-row otel_logs table (26 parts, ClickHouse 26.6), opening the search page ran every Map key text index read twice, at the same time:

Index	Per query	In flight
idx_log_attr_key	154M rows read, 9.6 GiB memory, ~8 s	2 copies
idx_res_attr_key	162M rows read, 14.0 GiB memory, ~14 s	2 copies

That is about 47 GiB of memory in use at once for one user opening one page. With this change each read runs once.

Which two callers overlap isn't confirmed. The search page's field lookups build the same React Query key, which React Query already shares, so the duplicates need two keys that differ only in values getMapKeys ignores. A likely candidate is useMultipleAllFields: its key includes the timestampValueExpression option, but getAllFields falls back to the table connection's own value. If that option is undefined on the first render, the key changes once the source loads, while getMapKeys gets the same arguments both times. getAllFields doesn't pass React Query's abort signal on, so the first query keeps running. That's a separate fix.

The fix

• MetadataCache.dedupe(key, fn) shares one running call per key without caching its result. getOrFetch now uses it, with no change in behavior.
• getMapKeys wraps its lookup (queryMapKeys) in dedupe. What gets cached, and under which key, is unchanged from main. The in-flight key includes the aligned date range: without timestampValueExpression, cacheKey has no range, but a rollup answer depends on it, so lookups for different ranges never share a rollup read.

Known limit, same as every other getOrFetch lookup: a shared read uses the first caller's abort signal, so if that caller aborts, the others get the rejection too. getAllFields, the search page path, passes no signal, so this only applies to direct callers such as the MCP describeSource tool.

Reading the keys index across every part is still expensive. A follow-up (#3418) reads a sample of parts instead.

How to reproduce

Starting point: yarn dev against a ClickHouse 26.3+ table with a Map key text index (e.g. idx_log_attr_key).

On main:

1. Open Search on the logs source.
2. Run SYSTEM FLUSH LOGS, then:

   SELECT event_time, query_duration_ms, read_rows, memory_usage, query
   FROM system.query_log
   WHERE type = 'QueryFinish' AND query LIKE '%mergeTreeTextIndex(%SELECT token AS key%'
     AND query NOT LIKE '%system.query_log%' AND event_time > now() - INTERVAL 5 MINUTE
   ORDER BY event_time

3. Each Map key index appears twice, at the same time.

On this branch:

1. Repeat the steps above. Each index appears once.

References

• Related PRs: perf: sample text index key values instead of loading all parts in time range #3418

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Nov 13, 2026 5:07pm UTC
hyperdx-storybook	Ready	Preview	Nov 13, 2026 5:07pm UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: f2f4682

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@hyperdx/common-utils	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

• Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats

• Production files changed: 1
• Production lines changed: 130 (+ 66 in test files, excluded from tier calculation)
• Branch: claude/dedupe-map-keys
• Author: knudtty

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

Duplicate code analysis

1 new clone introduced by this PR.

• packages/common-utils/src/core/metadata.ts:1213-1237 ↔ packages/common-utils/src/core/metadata.ts:1259-1280 — 25 lines

If one of these duplicates a helper that already exists, import it instead
of redefining it. If the duplication is deliberate, ignore this comment —
the check is advisory and never fails the build.

---

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 108 in total (1.18%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[github-actions]

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

2 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 1 minor

2 posted as inline comment(s) on the changed lines.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[github-actions]

Deep Review

✅ No critical issues found.

The refactor (extract queryMapKeys, wrap it in cache.getOrFetch(cacheKey, …)) correctly dedupes concurrent reads: getOrFetch runs no await between the get/pendingQueries.has checks and storing the promise, and queryMapKeys yields its promise on first suspension, so two same-cacheKey callers share one in-flight read. Removed cache.get/cache.set/fetchKeys code is fully orphan-free and TTL semantics are preserved. One behavioral change from base warrants attention before merge.

🟡 P2 -- recommended

• packages/common-utils/src/core/metadata.ts:1125 -- The outer getOrFetch now persists queryMapKeys's entire return under cacheKey, including the rollup path's result, whereas base returned rollup keys directly and cached them only under a range-scoped .rollup key; when timestampValueExpression is absent cacheKey carries no date range, so a non-empty rollup result (which filters on Timestamp via alignedDateRange) is cached with no TTL and served for a later call using a different range.
  • Fix: Keep the rollup result out of the range-less cacheKey-backed store — have queryMapKeys route rollup hits through a range-scoped key as base did, or include alignedDateRange in cacheKey whenever alignedDateRange is set rather than only when timestampValueExpression is truthy — and add a test calling getMapKeys twice with metadataMVs set, timestampValueExpression omitted, and two aligned-distinct ranges returning distinct rollup keys.
  • _{correctness-reviewer, performance-reviewer, kieran-typescript-reviewer, testing-reviewer, previous-comments-reviewer}

🔵 P3 nitpicks (4)

• packages/common-utils/src/core/metadata.ts:1125 -- The whole read (text-index + rollup + scan) now shares the first caller's abort_signal, so if that caller aborts, concurrent sharers receive the rejection; the PR notes this only reaches direct callers like the MCP describeSource tool since getAllFields passes no signal.
  • Fix: Decouple the shared read's lifetime from any single caller's signal, or document the constraint at the getOrFetch call site so direct callers know aborts are shared.
  • _{correctness-reviewer}
• packages/common-utils/src/core/metadata.ts:1296 -- On a rollup hit the same keys array is stored under both the outer cacheKey and the inner .rollup key, leaving a redundant TTL-less entry the outer layer makes unreachable.
  • Fix: Drop the inner rollup getOrFetch now that the outer layer caches successful results, keeping it only if the empty-rollup fast path still needs its own entry.
  • _{performance-reviewer}
• packages/common-utils/src/core/metadata.ts:1161 -- queryMapKeys duplicates a ~15-field inline param object-type that mirrors getMapKeys's params plus alignedDateRange/cacheKey, creating a field-by-field drift surface between the two signatures.
  • Fix: Extract a shared named type so the two methods cannot diverge silently.
  • _{kieran-typescript-reviewer, maintainability-reviewer}
• packages/common-utils/src/core/metadata.ts:1140 -- cacheKey is threaded into queryMapKeys solely so the rollup path can build ${cacheKey}.<range>.rollup, coupling the query method to the outer cache-key format.
  • Fix: Precompute the rollup key in getMapKeys and pass it in, or add a cross-reference comment near unscopedCacheKey noting the rollup path depends on appending its own range (the exact area of the previously reverted range-less-key bug).
  • _{maintainability-reviewer}

---

Reviewers (6): correctness, performance, kieran-typescript, testing, maintainability, previous-comments.

Testing gaps:

• No test covers the rollup path with metadataMVs present and timestampValueExpression absent across two different date ranges — the exact combination that would expose range-less cacheKey staleness.
• No test exercises concurrent getMapKeys dedup on the rollup path (both new concurrency tests strip/omit metadataMVs, covering only the scan and text-index paths).
• No test asserts an aborted text-index read leaves no value cached under cacheKey for the new nesting.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

[github-actions]

E2E Test Results

✅ All tests passed • 465 passed • 1 skipped • 1180s

Status	Count
✅ Passed	465
❌ Failed	0
⚠️ Flaky	3
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →