From 008d259ddc511d5d8b7123e56c979f1844c028e4 Mon Sep 17 00:00:00 2001 From: Prabhat Sharma Date: Sun, 2 Aug 2026 20:51:59 -0700 Subject: [PATCH] fix(editor): tenant-scope the PromQL schema cache, and drop overtaken lookups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from external review, both reproduced before being believed. A CROSS-TENANT CACHE. metricLabelCache was keyed on the metric name alone, and organisations are switched IN PLACE in this SPA — no reload, so module state survives. Metric names are not unique across organisations, and label names are that tenant's schema. Open the same metric name in a second organisation and it served the first one's labels, without ever asking the server. This is the same class as the get_all_transform leak fixed earlier in this workstream, and the field-value cache next to it was already keyed org|type|stream|field for exactly this reason. Now keyed by organisation and metric. STALE LOOKUPS OVERWRITING CURRENT ONES. Every completed lookup published its result without asking whether it was still the current question. Values are a network call with a ten-second ceiling, so `{service="` can answer after the user has moved on to `{region="` — and the reproduction shows exactly that: region's values on screen, replaced by service names when the abandoned request landed. A generation counter now gates all three publish points, matching the requestSeq guard the SQL catalog fetch already uses. Verified by mutation, not by inspection: removing the guard from the value branch turns the stale-lookup test red, and only that test. Full suite: 40468 total, 40037 passed, 23 failed — 14 synthetics journey specs and the 9 load-flaky CodeQueryEditor ones, all pre-existing. type-check, eslint and prettier clean. --- ...usePromqlSuggestions.streamSources.spec.ts | 77 ++++++++++++++++++- web/src/composables/usePromqlSuggestions.ts | 23 +++++- 2 files changed, 94 insertions(+), 6 deletions(-) diff --git a/web/src/composables/usePromqlSuggestions.streamSources.spec.ts b/web/src/composables/usePromqlSuggestions.streamSources.spec.ts index a6b85d9fbc..a15315d4f5 100644 --- a/web/src/composables/usePromqlSuggestions.streamSources.spec.ts +++ b/web/src/composables/usePromqlSuggestions.streamSources.spec.ts @@ -43,12 +43,14 @@ vi.mock("@/composables/fieldValueStore", () => ({ getFieldValuesForSuggestion: vi.fn().mockResolvedValue([]), requestFieldValues: vi.fn().mockResolvedValue([]), })); +// A SINGLE mutable store, so a test can switch organisation the way the app +// does — in place, without a page reload. +const mockStore = vi.hoisted(() => ({ + state: { selectedOrganization: { identifier: "myorg" } }, +})); vi.mock("vuex", async (importOriginal) => { const actual = await importOriginal(); - return { - ...actual, - useStore: vi.fn(() => ({ state: { selectedOrganization: { identifier: "myorg" } } })), - }; + return { ...actual, useStore: vi.fn(() => mockStore) }; }); import searchService from "@/services/search"; @@ -100,6 +102,7 @@ const offered = (c: any) => c.autoCompletePromqlKeywords.value as any[]; beforeEach(() => { vi.clearAllMocks(); + mockStore.state.selectedOrganization.identifier = "myorg"; vi.mocked(streamService.schema).mockResolvedValue(METRIC_SCHEMA as any); vi.mocked(getFieldValuesForSuggestion).mockResolvedValue([]); vi.mocked(requestFieldValues).mockResolvedValue([]); @@ -349,6 +352,72 @@ describe("while a lookup is in flight", () => { }); }); +describe("caches and races", () => { + it("does not serve one organisation's labels to another", async () => { + // The same class of bug as the cross-tenant transform leak fixed earlier in + // this workstream: a cache keyed on the metric alone. Organisations are + // switched IN PLACE in this SPA, metric names are not unique across them, + // and label names are that tenant's schema. + const c = await freshComposable(); + const query = "cpu_utilization_percent{"; + const ask = async () => { + c.autoCompleteData.value.query = query; + c.autoCompleteData.value.position.cursorIndex = query.length - 1; + await c.getSuggestions(); + await flushPromises(); + return offered(c).map((k: any) => k.label); + }; + + vi.mocked(streamService.schema).mockResolvedValue({ + data: { schema: [{ name: "tenant_a_only", type: "Utf8" }] }, + } as any); + expect(await ask()).toContain("tenant_a_only"); + + mockStore.state.selectedOrganization.identifier = "otherorg"; + vi.mocked(streamService.schema).mockResolvedValue({ + data: { schema: [{ name: "tenant_b_only", type: "Utf8" }] }, + } as any); + + const afterSwitch = await ask(); + expect(afterSwitch, "served the previous tenant's labels").not.toContain("tenant_a_only"); + expect(afterSwitch).toContain("tenant_b_only"); + expect(vi.mocked(streamService.schema).mock.calls.at(-1)?.[0]).toBe("otherorg"); + }); + + it("ignores a slow lookup that lands after a newer one", async () => { + // Values are a network call with a ten-second ceiling. Type `{service="`, + // change your mind to `{region="`, and the first answer can arrive last and + // overwrite the second — offering service names for a region filter. + const c = await freshComposable(); + let resolveSlow: (v: string[]) => void = () => {}; + vi.mocked(getFieldValuesForSuggestion) + .mockImplementationOnce(() => new Promise((resolve) => (resolveSlow = resolve)) as any) + .mockResolvedValue(["us-east-1"]); + + const first = 'cpu_utilization_percent{service="'; + c.autoCompleteData.value.query = first; + c.autoCompleteData.value.position.cursorIndex = first.length - 1; + void c.getSuggestions(); + await flushPromises(); + + const second = 'cpu_utilization_percent{region="'; + c.autoCompleteData.value.query = second; + c.autoCompleteData.value.position.cursorIndex = second.length - 1; + await c.getSuggestions(); + await flushPromises(); + expect(offered(c).map((k: any) => k.label)).toEqual(["us-east-1"]); + + // The abandoned request finally answers. + resolveSlow(["api-gateway", "chat-service"]); + await flushPromises(); + + expect( + offered(c).map((k: any) => k.label), + "a stale lookup overwrote the current one", + ).toEqual(["us-east-1"]); + }); +}); + describe("what must not change", () => { it("still offers the catalog when the cursor is not in a label position", async () => { const c = await freshComposable(); diff --git a/web/src/composables/usePromqlSuggestions.ts b/web/src/composables/usePromqlSuggestions.ts index 1d40a2940c..f6b74c6ab3 100644 --- a/web/src/composables/usePromqlSuggestions.ts +++ b/web/src/composables/usePromqlSuggestions.ts @@ -11,7 +11,12 @@ const NON_LABEL_COLUMNS = new Set(["value", "_timestamp", "__hash__", "__name__" // One schema per metric per page, shared by every editor. A metrics stream is // named for its metric, so this is also the label list. +// +// Keyed by ORGANISATION and metric, because organisations are switched in place +// in this SPA and metric names are not unique across them — the field-value +// cache is scoped the same way, for the same reason. const metricLabelCache = new Map(); +const metricLabelCacheKey = (org: string, metric: string) => `${org}|${metric}`; import { useStore } from "vuex"; const usePromqlSuggestions = () => { @@ -177,7 +182,15 @@ const usePromqlSuggestions = () => { return labelMeta; } + // Bumped by every suggestion pass. A lookup that finishes after a newer one + // started has been overtaken and must not publish: values are a network call + // with a ten-second ceiling, so `{service="` can easily answer after the user + // has moved on to `{region="`. + let suggestionGeneration = 0; + const getSuggestions = async () => { + const generation = ++suggestionGeneration; + const isCurrent = () => generation === suggestionGeneration; try { const parsedQuery: any = parsePromQlQuery(autoCompleteData.value.query); const metricName = parsedQuery?.metricName || ""; @@ -229,16 +242,19 @@ const usePromqlSuggestions = () => { // with all of its labels for the client to dedupe — 5903 bytes where // the schema answers in 1699, and it is metadata, so no scan at all. try { - let labels = metricLabelCache.get(metricName); + const cacheKey = metricLabelCacheKey(org, metricName); + let labels = metricLabelCache.get(cacheKey); if (!labels) { const response: any = await streamService.schema(org, metricName, "metrics"); const columns = response?.data?.schema ?? response?.data?.uds_schema ?? []; labels = columns .map((column: any) => column?.name) .filter((name: string) => name && !NON_LABEL_COLUMNS.has(name)); - metricLabelCache.set(metricName, labels as string[]); + metricLabelCache.set(cacheKey, labels as string[]); } + if (!isCurrent()) return; + const alreadyFiltered = formattedLabels.join(","); updatePromqlKeywords( (labels as string[]) @@ -254,6 +270,7 @@ const usePromqlSuggestions = () => { ); } catch { // The labels are unavailable; the language is not. + if (!isCurrent()) return; updatePromqlKeywords([]); autoCompleteData.value.popup.close(""); } @@ -271,6 +288,8 @@ const usePromqlSuggestions = () => { values = []; } + if (!isCurrent()) return; + // Quoting, decided from what is actually around the cursor. Monaco // auto-closes `"`, so the model is `service=""` with the cursor between // them; inserting a fully quoted value there produced