[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 10, 2026 10:49pm UTC
hyperdx-storybook	Ready	Preview	Nov 10, 2026 10:49pm UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: 698c541

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 (+ 33 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 110 in total (1.20%).
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

The change extracts getMapKeys's lookup into a private queryMapKeys and wraps the whole thing in the pre-existing cache.getOrFetch, so concurrent identical calls share one in-flight ClickHouse read. The perf goal is sound and the refactor is mostly behavior-preserving — but wrapping every return path in getOrFetch(cacheKey, …) changes what the rollup path caches, which diverges from main in one reachable case.

🟡 P2 — recommended

• packages/common-utils/src/core/metadata.ts:1125 — On main the rollup path did if (rollupKeys.length > 0) return rollupKeys; and was cached only under the range-scoped .rollup key, never under the outer cacheKey; the new outer getOrFetch(cacheKey, …) now also caches a non-empty rollup result under cacheKey, which is range-less and TTL-less when timestampValueExpression is absent (alignedCacheSuffix/dateRangeCacheSuffix both collapse to '' at lines 1105–1116). A later getMapKeys for the same table/column with a different dateRange then hits cacheKey and returns the first range's rollup keys, so the filter/group-by field list shows stale keys for the new window. This precondition (metadataMVs present + timestampValueExpression absent + rollup non-empty) is type-allowed on getAllFields and is exactly the case the reverted .rollup-key design guarded against.

  • Fix: Keep rollup results out of the range-less cacheKey — either skip the outer getOrFetch cache write when a rollup result is returned under a range-less key, or incorporate the aligned range into cacheKey whenever metadataMVs is present.

• packages/common-utils/src/__tests__/metadata.test.ts:3227 — The new test covers only concurrent sharing on the two text-index paths; there is no test asserting the rollup path dedups, that different date ranges do not return shared/stale keys, or that a rollup result is not served under a range-less key — the exact behavior the caching change affects.

  • Fix: Add a test with metadataMVs set and timestampValueExpression omitted that calls getMapKeys for two distinct date ranges and asserts each range re-queries rather than returning the first range's keys.

🔵 P3 nitpicks (2)

• packages/common-utils/src/core/metadata.ts:1125 — A shared in-flight read adopts the first caller's abort_signal, so if that caller aborts, concurrent callers with their own signals receive the rejection too; the search page passes no signal, but direct callers such as the MCP describeSource tool do.
  • Fix: Document this on getMapKeys/getOrFetch, or detach shared in-flight reads from any single caller's signal.
• packages/common-utils/src/core/metadata.ts:1161 — The parameter type literal is duplicated verbatim between getMapKeys and the new queryMapKeys, so future field changes must be edited in two places.
  • Fix: Extract a shared named type (e.g. QueryMapKeysArgs) and reference it from both signatures.

---

Reviewers (5): correctness, performance, kieran-typescript, testing, adversarial.

Testing gaps: No coverage for rollup-path concurrent sharing, for date-range cache scoping of rollup results, or for the abort-signal sharing behavior.

_{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 • 466 passed • 1 skipped • 1299s

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

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

🟠 major — MCP describeSource calls now share text-index reads in the process-wide cache and can silently lose Map keys when another request aborts

getMetadata (metadata.ts:3537) uses the module-level __LOCAL_CACHE__, so in the API process two concurrent describeSource calls for the same connection, table and column, from different MCP sessions, now share one in-flight read. That read uses the first caller's signal. Before this PR the text-index path never shared reads. Now, when request A hits its deadline, the read throws through if (signal?.aborted) throw e and request B gets the same rejection. B's own signal was never aborted, so describeSource.ts:316-332 only logs a warning and does not add mapAttributeKeys to skippedStages. B returns an incomplete schema that is not flagged as partial. The PR description mentions the shared-signal limit but not this unflagged partial result. Fix: when signal is set, skip dedupe or key it per signal; or run the shared read on its own controller that aborts only after every joined caller has aborted, with each caller racing its own signal. Add a test where caller A aborts and caller B still resolves.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[github-actions]

🔵 minor — Aligned-range key suffix is now built by hand in three places that must stay in sync

${alignedDateRange[0].getTime()}.${alignedDateRange[1].getTime()} now appears in alignedCacheSuffix (L1106), inFlightKey (L1127) and the rollup key (L1304). When timestampValueExpression is set, inFlightKey repeats a range that cacheKey already contains. The in-flight key is only correct while it uses the same range as the rollup key it protects. Compute alignedRangeSuffix once in getMapKeys and pass it to queryMapKeys for the rollup key, so both keys come from one value.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[github-actions]

🟠 major — Rollup keys are now cached forever under a cacheKey with no date range, so other ranges get stale keys

The outer getOrFetch stores whatever queryMapKeys returns under cacheKey, rollup results included. When timestampValueExpression is not set, alignedCacheSuffix/dateRangeCacheSuffix are '', so cacheKey has no date range. The rollup query still filters on Timestamp using alignedDateRange (lines 1296-1308). Sequence: call with range A (metadataMVs set, no tse, rollup returns keys) → keys cached with no TTL → call with range B hits the cache and gets range A's keys. On main, the rollup result went only to its own ${cacheKey}.<aligned range>.rollup entry. Commit e9d9dd0 guarded against this case and was reverted. The PR description says the work uses a non-caching MetadataCache.dedupe and an in-flight key that includes the aligned range, but neither is in this diff. Fix: add dedupe(key, fn) (in-flight sharing only, no caching), wrap queryMapKeys in it keyed on cacheKey plus the aligned range, and keep the cache.set(cacheKey, …) calls on the text-index and scan paths as they are on main. The other option is to leave the rollup branch out of the outer cache.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[github-actions]

🔵 minor — New test only covers the text-index path with timestampValueExpression, so the rollup cache-key regression passes

The test always passes timestampValueExpression: 'Timestamp' and never sets metadataMVs. The existing 'caches rollup keys by aligned date range' test (around line 2942) also sets tse. Nothing checks that calls without tse, with metadataMVs, for two different date ranges each query the rollup. Add that case: getMapKeys({...baseArgs, timestampValueExpression: undefined}) for range A, then for range B, and expect two rollup queries with different results. Also add a concurrent case for the scan fallback.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}