fix(web): scope warehouse atom cache keys by org - #348
Merged
Conversation
Three defs: the recent-window probe, the unbounded fallback probe, and the pruned hierarchy read. All three carry cache: undefined on purpose. The handler keeps a single cachedDirect around the whole probe-then-read sequence, so the probe only fires on an outer cache miss. Caching them individually would run the probe on every request and cache a result nobody asked for -- and the probe exists precisely because trace_detail_spans is partitioned by toDate(Timestamp), where an unpruned hierarchy read is p95 ~8.8s against ~2.3s pruned. Adds a `withDeps` helper. cachedDirect takes an Effect with no requirements, while runQuery reads WarehouseQueryService and QueryEngineService from context; withDeps supplies the instances already bound at the top of the group so a registry query can run INSIDE a cache wrapper. Without it the only options were hoisting the probe out of the cache (losing the miss-only property) or threading services through every call site. Verified: apps/api typecheck; SQL baseline byte-identical; routes + warehouse suites 310 pass.
withDeps was an anti-pattern. It re-provided WarehouseQueryService and QueryEngineService into an Effect that read them from context, using the very instances already bound as values three lines above -- laundering a requirement purely to satisfy cachedDirect's R = never. makeQueryRunners takes those services as values once per handler group and returns runQuery/runQueryFirst closed over them. Every call site is unchanged (`runQuery(def, tenant, payload)`), but the effects now carry R = never, so a registry query composes inside a cache wrapper with no ceremony. spanHierarchy's probe-inside-cachedDirect works for the right reason rather than by re-injection. Verified: apps/api typecheck; SQL baseline byte-identical; routes + warehouse suites 310 pass.
Network reads a counter family, everything else a gauge family, so they are two defs rather than one def with a branch -- the row shapes differ and the handler maps them differently. Both keep the id "hostInfraTimeseries", which is what their spans already report; renaming would break continuity of existing telemetry for no gain. hostMetricSpec joins the pod/node/workload specs in query-helpers: the def needs metricName and groupByAttributeKey, the handler needs unit and isNetwork. Verified: apps/api typecheck; SQL baseline byte-identical. Local test suites not re-run for this commit -- CI covers them.
cloudflareInfraZoneBreakdown and serviceOperations, the two that were left because their control flow is not query construction. ZoneBreakdown runs three queries in parallel and then a fourth whose grouping keys come from the first's ranked output. `topKeys` therefore rides in the PAYLOAD rather than being derived inside compile -- a def has no way to see a previous query's result. The caller still skips the fourth entirely when the key list is empty. Its coverage sub-query stays deliberately unfiltered: it answers "what did the poller collect here", which is how the UI distinguishes "not collected yet" from "no traffic". serviceOperations has a rollup form and a raw form of each of its two queries. The CHOICE stays in the handler because it is policy: a feature flag selects the rollup, and a typed isMissingServiceOperationsRollup error falls back to raw at runtime, flipping a flag the timeseries query then honors too. The defs own compile, profile and context; the handler owns the fallback. Rollup/raw pairs share an id, matching the context their spans already report -- the fallback is recorded separately as query.rollup.fallback. Also removes dead code the earlier passes left behind: an unused `params` object in each of the two handlers above, and a whole conditional CH.compile in planetscaleInfraTimeseries that its def had already superseded. query-engine.http.ts now contains ZERO CH.compile calls and is down from 3275 to 1774 lines. The three remaining handlers -- execute, executeQueryBuilder and executeRawSql -- are the QuerySpec and raw-SQL surfaces and should not become QueryDefs. Verified: both packages typecheck. Test suites not run locally by request; CI covers them, including the SQL baseline and the ClickHouse DESCRIBE sweep.
Atom family keys carried only filters and time range. The org rode along invisibly in the auth header, so after an org switch every cached entry stayed addressable by the new org and the UI re-rendered the PREVIOUS org's rows until the idle TTL expired -- up to 30 minutes on some atoms. Electric collections never had this bug because their ids already embed the org. The server side was already correct: buildDirectRouteCacheKey includes orgId, so this was purely the client cache. The org is PREFIXED rather than folded into the encoded object, because the atom decodes its key back into the query input -- an extra field there would travel to the server as part of the request payload. NUL cannot occur in encodeKey output (it is JSON) or in a Clerk org id, so the first occurrence always marks the boundary. Applied to both key-building sites: the warehouse atom factory and the dashboard widget family, which had the same defect. Verified: apps/web typecheck. Tests not run locally by request.
🍁 Maple PR previewNote Preview resources were removed when this pull request closed. Final commit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #347 (which is stacked on #346) — review those first.
The bug
Warehouse atom family keys carried only filters and time range. The org rode along invisibly in the auth header, so after an org switch every cached entry stayed addressable by the new org — the UI re-rendered the previous org's rows until the idle TTL expired. That's up to 30 minutes on some atoms (
getServiceHealthBaselineResultAtom).Electric collections never had this bug, because their collection ids already embed the org (
<shape>:<org>).The server side was already correct —
buildDirectRouteCacheKeyincludesorgId. This was purely the client cache.The fix
encodeOrgScopedKey/orgScopedKeyPayloadinapps/web/src/lib/cache-key.ts.The org is prefixed rather than folded into the encoded object, because the atom decodes its key back into the query input — an extra field there would travel to the server as part of the request payload. NUL can't occur in
encodeKeyoutput (it's JSON) or in a Clerk org id, so the first occurrence always marks the boundary.Applied at both key-building sites:
warehouse-query-atoms.ts(~90 atom families)use-widget-data.ts, which had the identical defectTwo things I removed from this PR after checking them
I'd previously listed these as quick wins. Neither survived inspection, and I'd rather say so than ship churn:
explore-attributes"3 sequential queries" — wrong. The threeexecutor.querycalls live across two exported functions, and the two insideexploreAttributeKeysare on mutually-exclusive branches (one returns early). Nothing runs sequentially; there is nothing to parallelize.inspect-widget.tsconcurrency: 1— deliberate. It makes the push order intoformulaBaseInputsdeterministic for formula evaluation. Parallelizing means restructuring the side-effecting push, in an MCP inspection tool that isn't on a user-facing latency path.Testing
apps/webtypecheck cleanWorth a manual check on merge: sign in, load
/traces, switch org, and confirm a fresh request fires rather than stale rows rendering.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.