From 067d2fba251708221a011d52d101d76c62180fa0 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 21 Apr 2026 13:10:58 +0000 Subject: [PATCH 1/7] feat(cache): centralize mutation invalidation at the HTTP layer (#792) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #788. The per-site `invalidate*` helpers in `api/issues.ts`, `api/projects.ts`, and `api/dashboards.ts` are replaced by a single post-mutation hook in `authenticatedFetch` that auto-invalidates the cache for every successful non-GET. `invalidateAfterMutation` fires in `sentry-client.ts` after `fetchWithRetry` returns a 2xx non-GET response. Prefix computation is delegated to a new `src/lib/cache-keys.ts` module: - **Hierarchy walk.** For a mutation on `/api/0/organizations/{org}/releases/1.0.0/deploys/`, sweep the URL path plus every ancestor: `releases/1.0.0/`, `releases/`, `organizations/{org}/`, `organizations/`. Catches the corresponding GET caches in a single pass. - **Cross-endpoint rules.** Tiny table for the 2 cases where a mutation affects a different URL tree: - `POST /teams/{org}/{team}/projects/` invalidates `/organizations/{org}/projects/` - `DELETE /projects/{org}/{project}/` invalidates `/organizations/{org}/projects/` The hook is awaited so a subsequent read in the same command sees fresh data. Identity-gated via the existing sweep primitive, so a mutation by one account can't evict another account's cache. `classifyUrl === "no-cache"` paths (autofix/root-cause) skip naturally because reads to those URLs aren't cached either. Issue #792 proposed an optional `invalidates` callback on `buildCommand` for cross-endpoint fan-outs. Turned out the 2 hardcoded rules above cover every current case; deferring the callback API to when a use case actually emerges. Every mutation in `src/lib/api/` is now covered for free, including mutations that had no invalidation before (`releases`, `teams`, `dashboards` create, `trials`). `sentry api -X POST/PUT/DELETE ...` also gets auto-invalidation — users of the raw escape hatch no longer need `--fresh` on follow-up reads. - `api/issues.ts`: `invalidateIssueCaches`, `invalidateIssueDetailCaches`, `invalidateOrgIssueList` plus their call sites in `updateIssueStatus` and `mergeIssues`. - `api/projects.ts`: `invalidateProjectCaches`, `invalidateOrgProjectCache` plus call sites in `createProject` and `deleteProject`. - `api/dashboards.ts`: inline `invalidateCachedResponse` in `updateDashboard`. Net: -156 lines source + new `cache-keys.ts` (~130 lines). - `test/lib/cache-keys.test.ts` — 13 tests covering the hierarchy walk, cross-endpoint rules, query-string stripping, unparseable URLs, self-hosted bases, and dedup. - `test/lib/sentry-client.invalidation.test.ts` — 5 integration tests: successful mutation clears self + list, failed mutation leaves cache alone, GET doesn't invalidate, cross-endpoint rule fires, identity isolation holds. Full unit suite: 5545 passing. Closes #792. --- src/lib/api/dashboards.ts | 9 - src/lib/api/issues.ts | 104 +--------- src/lib/api/projects.ts | 50 ----- src/lib/cache-keys.ts | 146 ++++++++++++++ src/lib/sentry-client.ts | 50 ++++- test/lib/cache-keys.test.ts | 139 ++++++++++++++ test/lib/sentry-client.invalidation.test.ts | 199 ++++++++++++++++++++ 7 files changed, 538 insertions(+), 159 deletions(-) create mode 100644 src/lib/cache-keys.ts create mode 100644 test/lib/cache-keys.test.ts create mode 100644 test/lib/sentry-client.invalidation.test.ts diff --git a/src/lib/api/dashboards.ts b/src/lib/api/dashboards.ts index 4302d97a5..9bc8b5ccd 100644 --- a/src/lib/api/dashboards.ts +++ b/src/lib/api/dashboards.ts @@ -27,7 +27,6 @@ import { } from "../../types/dashboard.js"; import { stringifyUnknown } from "../errors.js"; import { resolveOrgRegion } from "../region.js"; -import { invalidateCachedResponse } from "../response-cache.js"; import { apiRequestToRegion, @@ -125,14 +124,6 @@ export async function updateDashboard( method: "PUT", body, }); - - // Invalidate cached GET for this dashboard so subsequent view commands - // return fresh data instead of the pre-mutation cached response. - const normalizedBase = regionUrl.endsWith("/") - ? regionUrl.slice(0, -1) - : regionUrl; - await invalidateCachedResponse(`${normalizedBase}/api/0${path}`); - return data; } diff --git a/src/lib/api/issues.ts b/src/lib/api/issues.ts index 43ac6736d..d2ec5d347 100644 --- a/src/lib/api/issues.ts +++ b/src/lib/api/issues.ts @@ -12,14 +12,11 @@ import type { SentryIssue } from "../../types/index.js"; import { applyCustomHeaders } from "../custom-headers.js"; import { ApiError, ValidationError } from "../errors.js"; import { resolveOrgRegion } from "../region.js"; -import { invalidateCachedResponsesMatching } from "../response-cache.js"; -import { getApiBaseUrl } from "../sentry-client.js"; import { API_MAX_PER_PAGE, apiRequest, apiRequestToRegion, - buildApiUrl, getOrgSdkConfig, MAX_PAGINATION_PAGES, type PaginatedResponse, @@ -566,22 +563,14 @@ export async function updateIssueStatus( `/organizations/${encodeURIComponent(options.orgSlug)}/issues/${encodeURIComponent(issueId)}/`, { method: "PUT", body } ); - await invalidateIssueCaches(regionUrl, options.orgSlug, issueId); return data; } - // Legacy global endpoint — works without org but not region-aware, - // so we can only flush the legacy issue-detail URL. Region-scoped - // lists age out via their TTL. Prefix-sweep (not exact-match) - // because `getIssue` caches under `.../issues/{id}/?collapse=...`. - const legacyData = await apiRequest( - `/issues/${encodeURIComponent(issueId)}/`, - { method: "PUT", body } - ); - await invalidateCachedResponsesMatching( - buildApiUrl(getApiBaseUrl(), "issues", issueId) - ); - return legacyData; + // Legacy global endpoint — works without org but not region-aware. + return apiRequest(`/issues/${encodeURIComponent(issueId)}/`, { + method: "PUT", + body, + }); } /** Result of a successful issue-merge operation. */ @@ -625,15 +614,6 @@ export async function mergeIssues( method: "PUT", body: { merge: 1 }, }); - // Flush detail caches for every affected ID (detail-only avoids - // N+1 list scans) then sweep the org-wide list once. - const affectedIds = data.merge.children.toSpliced(0, 0, data.merge.parent); - await Promise.all( - affectedIds.map((id) => - invalidateIssueDetailCaches(regionUrl, orgSlug, id) - ) - ); - await invalidateOrgIssueList(regionUrl, orgSlug); return data.merge; } catch (error) { // The bulk-mutate endpoint returns 204 when no matching issues are @@ -693,77 +673,3 @@ export async function getSharedIssue( return (await response.json()) as { groupID: string }; } - -/** - * Flush both the org-scoped and legacy detail endpoints for one issue, - * including all `collapse` query-param variants (`getIssueInOrg` caches - * responses under URLs like `.../issues/{id}/?collapse=stats&...` so - * exact-match invalidation would miss them). Does NOT sweep the - * org-wide list — callers must call {@link invalidateOrgIssueList} - * once per operation. Never throws. - */ -async function invalidateIssueDetailCaches( - regionUrl: string, - orgSlug: string, - issueId: string -): Promise { - try { - await Promise.all([ - invalidateCachedResponsesMatching( - buildApiUrl(regionUrl, "organizations", orgSlug, "issues", issueId) - ), - // Legacy `/api/0/issues/{id}/` is stored under the non-region base - // (see `apiRequest` → `getApiBaseUrl`), NOT the org's region URL. - invalidateCachedResponsesMatching( - buildApiUrl(getApiBaseUrl(), "issues", issueId) - ), - ]); - } catch { - /* best-effort: mutation already succeeded upstream */ - } -} - -/** - * Flush detail + list caches for one issue. Use for single-issue - * mutations (resolve, unresolve); batch mutations should use the - * detail-only helper plus one final {@link invalidateOrgIssueList}. - * - * Minor redundancy: the org-scoped half of - * {@link invalidateIssueDetailCaches} is already a prefix of the - * {@link invalidateOrgIssueList} sweep. Accepted because the helpers - * are each also used solo elsewhere, and the extra directory walk is - * negligible. - * - * Never throws. - */ -async function invalidateIssueCaches( - regionUrl: string, - orgSlug: string, - issueId: string -): Promise { - try { - await Promise.all([ - invalidateIssueDetailCaches(regionUrl, orgSlug, issueId), - invalidateOrgIssueList(regionUrl, orgSlug), - ]); - } catch { - /* best-effort: mutation already succeeded upstream */ - } -} - -/** - * Sweep every paginated variant of the org's issue-list endpoint. - * Never throws. - */ -async function invalidateOrgIssueList( - regionUrl: string, - orgSlug: string -): Promise { - try { - await invalidateCachedResponsesMatching( - buildApiUrl(regionUrl, "organizations", orgSlug, "issues") - ); - } catch { - /* best-effort: mutation already succeeded upstream */ - } -} diff --git a/src/lib/api/projects.ts b/src/lib/api/projects.ts index cf207f7bb..4d87ea750 100644 --- a/src/lib/api/projects.ts +++ b/src/lib/api/projects.ts @@ -25,8 +25,6 @@ import { cacheProjectsForOrg } from "../db/project-cache.js"; import { getCachedOrganizations } from "../db/regions.js"; import { type AuthGuardSuccess, withAuthGuard } from "../errors.js"; import { logger } from "../logger.js"; -import { resolveOrgRegion } from "../region.js"; -import { invalidateCachedResponsesMatching } from "../response-cache.js"; import { getApiBaseUrl } from "../sentry-client.js"; import { buildProjectUrl } from "../sentry-urls.js"; import { isAllDigits } from "../utils.js"; @@ -34,7 +32,6 @@ import { isAllDigits } from "../utils.js"; import { API_MAX_PER_PAGE, apiRequestToRegion, - buildApiUrl, getOrgSdkConfig, MAX_PAGINATION_PAGES, ORG_FANOUT_CONCURRENCY, @@ -169,7 +166,6 @@ export async function createProject( body, }); const data = unwrapResult(result, "Failed to create project"); - await invalidateOrgProjectCache(orgSlug); return data as unknown as SentryProject; } @@ -219,52 +215,6 @@ export async function deleteProject( }, }); unwrapResult(result, "Failed to delete project"); - await invalidateProjectCaches(orgSlug, projectSlug); -} - -/** - * Flush the project-detail GET and the org-wide project list so - * follow-up `project list` / `project view` reads don't see the - * deleted project. Never throws. - * - * Uses prefix-matching on the project detail URL rather than an exact - * match because `getProject()` appends `?collapse=organization`, and - * the response cache keys entries by the full URL (including query - * string). A prefix sweep catches the collapsed variant plus any - * future query-parameter additions. - */ -async function invalidateProjectCaches( - orgSlug: string, - projectSlug: string -): Promise { - try { - const regionUrl = await resolveOrgRegion(orgSlug); - await Promise.all([ - invalidateCachedResponsesMatching( - buildApiUrl(regionUrl, "projects", orgSlug, projectSlug) - ), - invalidateCachedResponsesMatching( - buildApiUrl(regionUrl, "organizations", orgSlug, "projects") - ), - ]); - } catch { - /* best-effort: mutation already succeeded */ - } -} - -/** - * Sweep every paginated variant of the org's project-list endpoint. - * Used by `project create`. Never throws. - */ -async function invalidateOrgProjectCache(orgSlug: string): Promise { - try { - const regionUrl = await resolveOrgRegion(orgSlug); - await invalidateCachedResponsesMatching( - buildApiUrl(regionUrl, "organizations", orgSlug, "projects") - ); - } catch { - /* best-effort: mutation already succeeded */ - } } /** Result of searching for projects by slug across all organizations. */ diff --git a/src/lib/cache-keys.ts b/src/lib/cache-keys.ts new file mode 100644 index 000000000..32415e949 --- /dev/null +++ b/src/lib/cache-keys.ts @@ -0,0 +1,146 @@ +/** + * Compute cache-invalidation prefixes for a mutation URL. + * + * The HTTP layer calls {@link computeInvalidationPrefixes} after every + * successful non-GET request and feeds the result into + * `invalidateCachedResponsesMatching`. Two rules apply: + * + * 1. **Hierarchy walk.** Sweep the URL's own path and every ancestor + * up to `/api/0/`. A mutation on + * `/organizations/{org}/releases/1.0.0/deploys/` sweeps itself, + * `.../releases/1.0.0/`, and `.../releases/` — which catches the + * detail, deploys-list, and releases-list GET caches in one pass. + * + * 2. **Cross-endpoint rules.** A small hardcoded table for mutations + * whose effects cross URL trees. For example, creating a project + * under a team hits `/organizations/{org}/teams/{team}/projects/` + * but invalidates the org project list at + * `/organizations/{org}/projects/`. The table is tiny today + * (2 rules) and only grows when a new cross-tree relationship + * appears in the API surface. + * + * Prefixes are identity-scoped at the sweep layer + * (`invalidateCachedResponsesMatching` checks `entry.identity`), so a + * slightly broader sweep is safe — it can only touch the current + * identity's entries. Query strings on the mutation URL are dropped + * from the prefix (a prefix sweep on the path naturally catches every + * query-param variant cached under that path). + */ + +/** Regex capturing the `/api/0/` boundary. Anchored so it matches only the canonical API prefix. */ +const API_V0_SEGMENT = "/api/0/"; + +/** + * Cross-endpoint invalidation rules. + * + * Each rule is a pattern the mutation URL path must match, plus a + * function that returns additional prefixes to sweep. Patterns match + * against the *path*, not the full URL — so the prefix returned is + * prepended with the request's base later. + * + * Keep this table small. The hierarchy walk handles most cases; add a + * rule here only when the API's cross-tree relationships force it. + */ +type CrossEndpointRule = { + /** Matches the path relative to `/api/0/` (no leading slash). */ + match: RegExp; + /** Returns additional path prefixes (relative to `/api/0/`) to sweep. */ + extra: (matchGroups: RegExpMatchArray) => string[]; +}; + +const CROSS_ENDPOINT_RULES: CrossEndpointRule[] = [ + { + // POST /api/0/teams/{org}/{team}/projects/ (create a project in a team) + // invalidates the org project list at + // /api/0/organizations/{org}/projects/ which lives under a + // different URL tree. + match: /^teams\/([^/]+)\/[^/]+\/projects\/?$/, + extra: ([, org]) => [`organizations/${org}/projects/`], + }, + { + // DELETE /api/0/projects/{org}/{project}/ (delete a project) + // invalidates the org project list at + // /api/0/organizations/{org}/projects/ (different URL tree). + match: /^projects\/([^/]+)\/[^/]+\/?$/, + extra: ([, org]) => [`organizations/${org}/projects/`], + }, +]; + +/** + * Compute the full set of cache-invalidation prefixes for a mutation + * URL. + * + * @param fullUrl - Fully-qualified URL of the mutation (absolute, + * including base). Query string is ignored. + * @returns Array of full-URL prefixes (including base and + * `/api/0/`) ready to pass to + * `invalidateCachedResponsesMatching`. Returns `[]` if the URL is + * not under `/api/0/` (e.g. sourcemap chunk upload to an arbitrary + * endpoint) or can't be parsed. + */ +export function computeInvalidationPrefixes(fullUrl: string): string[] { + let parsed: URL; + try { + parsed = new URL(fullUrl); + } catch { + return []; + } + + const apiIdx = parsed.pathname.indexOf(API_V0_SEGMENT); + if (apiIdx === -1) { + return []; + } + + // `base` includes origin + path up through and including `/api/0/`. + const base = `${parsed.origin}${parsed.pathname.slice(0, apiIdx + API_V0_SEGMENT.length)}`; + // Path below `/api/0/`, leading slash trimmed, trailing slash kept + // so it matches against rules that anchor on `/?$`. + const relPath = parsed.pathname.slice(apiIdx + API_V0_SEGMENT.length); + + // No relative path means the mutation hit `/api/0/` itself; nothing to sweep. + if (relPath === "") { + return []; + } + + const prefixes = new Set(); + + // Rule 1: hierarchy walk. Sweep the URL's own path plus every + // ancestor with at least one segment. + for (const segments of ancestorSegments(relPath)) { + prefixes.add(`${base}${segments}`); + } + + // Rule 2: cross-endpoint table. + for (const rule of CROSS_ENDPOINT_RULES) { + const match = relPath.match(rule.match); + if (match) { + for (const extra of rule.extra(match)) { + prefixes.add(`${base}${extra}`); + } + } + } + + return [...prefixes]; +} + +/** + * Yield every path-prefix sequence of `relPath` in descending length, + * always ending with a trailing slash. + * + * `"organizations/acme/releases/1.0.0/deploys/"` yields: + * - `"organizations/acme/releases/1.0.0/deploys/"` + * - `"organizations/acme/releases/1.0.0/"` + * - `"organizations/acme/releases/"` + * - `"organizations/acme/"` + * - `"organizations/"` + */ +function* ancestorSegments(relPath: string): Generator { + const trimmed = relPath.endsWith("/") ? relPath.slice(0, -1) : relPath; + if (trimmed === "") { + return; + } + const parts = trimmed.split("/"); + for (let i = parts.length; i > 0; i--) { + yield `${parts.slice(0, i).join("/")}/`; + } +} diff --git a/src/lib/sentry-client.ts b/src/lib/sentry-client.ts index 671275c67..9d715955f 100644 --- a/src/lib/sentry-client.ts +++ b/src/lib/sentry-client.ts @@ -10,6 +10,7 @@ import { getTraceData } from "@sentry/node-core/light"; import { maybeWarnEnvTokenIgnored } from "./auth-hint.js"; +import { computeInvalidationPrefixes } from "./cache-keys.js"; import { DEFAULT_SENTRY_URL, getConfiguredSentryUrl, @@ -18,7 +19,11 @@ import { import { applyCustomHeaders } from "./custom-headers.js"; import { getAuthToken, refreshToken } from "./db/auth.js"; import { logger } from "./logger.js"; -import { getCachedResponse, storeCachedResponse } from "./response-cache.js"; +import { + getCachedResponse, + invalidateCachedResponsesMatching, + storeCachedResponse, +} from "./response-cache.js"; import { withTracingSpan } from "./telemetry.js"; const log = logger.withTag("http"); @@ -285,6 +290,46 @@ function cacheResponse( }); } +/** + * Auto-invalidate cache entries that a successful non-GET mutation + * made stale. Awaited before returning the response so a subsequent + * read in the same command sees fresh data (the whole point of + * post-mutation invalidation). + * + * Prefix computation lives in {@link computeInvalidationPrefixes} + * (hierarchy walk + cross-endpoint rules). Each prefix runs through + * `invalidateCachedResponsesMatching`, which is identity-gated so + * a mutation by one account can't evict another account's cache. + * + * GETs skip invalidation entirely; they go through the cache-write + * path above. 4xx/5xx non-GETs skip too — a rejected mutation didn't + * change server state so its cache is still accurate. + * + * Never throws: the helpers we call are already best-effort, but we + * wrap defensively because a housekeeping error must never surface + * as a user-visible failure after a successful mutation. + */ +async function invalidateAfterMutation( + method: string, + fullUrl: string, + response: Response +): Promise { + if (method === "GET" || !response.ok) { + return; + } + const prefixes = computeInvalidationPrefixes(fullUrl); + if (prefixes.length === 0) { + return; + } + try { + await Promise.all( + prefixes.map((prefix) => invalidateCachedResponsesMatching(prefix)) + ); + } catch { + /* best-effort: mutation already succeeded upstream */ + } +} + /** Build a `{ authorization }` header map from a bearer token, or `{}` if absent. */ function authHeaders(token: string | undefined): Record { return token ? { authorization: `Bearer ${token}` } : {}; @@ -318,6 +363,9 @@ async function fetchWithRetry( authHeaders(getAuthToken()), result.response ); + // Awaited so a subsequent read in the same command sees fresh + // data. Never throws — own contract enforced by the helper. + await invalidateAfterMutation(method, fullUrl, result.response); return result.response; } if (result.action === "throw") { diff --git a/test/lib/cache-keys.test.ts b/test/lib/cache-keys.test.ts new file mode 100644 index 000000000..17bea3ddf --- /dev/null +++ b/test/lib/cache-keys.test.ts @@ -0,0 +1,139 @@ +/** + * Unit tests for `computeInvalidationPrefixes`. + */ + +import { describe, expect, test } from "bun:test"; +import { computeInvalidationPrefixes } from "../../src/lib/cache-keys.js"; + +const BASE = "https://us.sentry.io/api/0/"; + +describe("computeInvalidationPrefixes — hierarchy walk", () => { + test("detail URL yields self + org list ancestor", () => { + const prefixes = computeInvalidationPrefixes( + `${BASE}organizations/acme/issues/12345/` + ); + expect(prefixes).toContain(`${BASE}organizations/acme/issues/12345/`); + expect(prefixes).toContain(`${BASE}organizations/acme/issues/`); + expect(prefixes).toContain(`${BASE}organizations/acme/`); + expect(prefixes).toContain(`${BASE}organizations/`); + }); + + test("deeply nested path yields every ancestor", () => { + const prefixes = computeInvalidationPrefixes( + `${BASE}organizations/acme/releases/1.0.0/deploys/` + ); + // Five segments → five ancestor prefixes. + expect(prefixes).toContain( + `${BASE}organizations/acme/releases/1.0.0/deploys/` + ); + expect(prefixes).toContain(`${BASE}organizations/acme/releases/1.0.0/`); + expect(prefixes).toContain(`${BASE}organizations/acme/releases/`); + expect(prefixes).toContain(`${BASE}organizations/acme/`); + expect(prefixes).toContain(`${BASE}organizations/`); + }); + + test("single-segment path yields only the segment itself", () => { + const prefixes = computeInvalidationPrefixes(`${BASE}organizations/`); + expect(prefixes).toEqual([`${BASE}organizations/`]); + }); + + test("query string is stripped from the prefix set", () => { + // Prefix-sweep against the path catches every cached query variant, + // so the query itself is irrelevant for computing prefixes. + const prefixes = computeInvalidationPrefixes( + `${BASE}organizations/acme/issues/12345/?collapse=stats&collapse=lifetime` + ); + expect(prefixes).toContain(`${BASE}organizations/acme/issues/12345/`); + expect( + prefixes.every((p) => !(p.includes("collapse=") || p.includes("?"))) + ).toBe(true); + }); + + test("trailing-slashless URL still works", () => { + const prefixes = computeInvalidationPrefixes( + `${BASE}organizations/acme/issues` + ); + expect(prefixes).toContain(`${BASE}organizations/acme/issues/`); + expect(prefixes).toContain(`${BASE}organizations/acme/`); + }); +}); + +describe("computeInvalidationPrefixes — cross-endpoint rules", () => { + test("POST /teams/{org}/{team}/projects/ invalidates org project list", () => { + const prefixes = computeInvalidationPrefixes( + `${BASE}teams/acme/backend/projects/` + ); + expect(prefixes).toContain(`${BASE}organizations/acme/projects/`); + // Plus the hierarchy walk: + expect(prefixes).toContain(`${BASE}teams/acme/backend/projects/`); + expect(prefixes).toContain(`${BASE}teams/acme/backend/`); + expect(prefixes).toContain(`${BASE}teams/acme/`); + expect(prefixes).toContain(`${BASE}teams/`); + }); + + test("DELETE /projects/{org}/{project}/ invalidates org project list", () => { + const prefixes = computeInvalidationPrefixes( + `${BASE}projects/acme/frontend/` + ); + expect(prefixes).toContain(`${BASE}organizations/acme/projects/`); + // Plus the hierarchy walk from the mutation URL: + expect(prefixes).toContain(`${BASE}projects/acme/frontend/`); + expect(prefixes).toContain(`${BASE}projects/acme/`); + expect(prefixes).toContain(`${BASE}projects/`); + }); + + test("unrelated paths get no cross-endpoint sweep", () => { + const prefixes = computeInvalidationPrefixes( + `${BASE}organizations/acme/teams/` + ); + // `organizations/.../teams/` doesn't match either cross-endpoint + // rule, so the result is only the hierarchy walk. + expect(prefixes).not.toContain(`${BASE}organizations/acme/projects/`); + expect(prefixes).toContain(`${BASE}organizations/acme/teams/`); + }); +}); + +describe("computeInvalidationPrefixes — edge cases", () => { + test("returns [] for URLs not under /api/0/", () => { + expect( + computeInvalidationPrefixes("https://example.com/some/path/") + ).toEqual([]); + // Chunk-upload endpoints are arbitrary host URLs. + expect( + computeInvalidationPrefixes("https://uploads.sentry.io/x/y") + ).toEqual([]); + }); + + test("returns [] for unparseable URLs", () => { + expect(computeInvalidationPrefixes("not-a-url")).toEqual([]); + expect(computeInvalidationPrefixes("")).toEqual([]); + }); + + test("returns [] when the mutation hits /api/0/ root itself", () => { + expect(computeInvalidationPrefixes(BASE)).toEqual([]); + }); + + test("deduplicates prefixes across hierarchy + rule-table", () => { + // `teams/acme/backend/projects/` matches the cross-endpoint rule + // AND naturally walks up through `teams/acme/backend/` etc. The + // rule adds `organizations/acme/projects/` (unique). No duplicates + // should appear in the result. + const prefixes = computeInvalidationPrefixes( + `${BASE}teams/acme/backend/projects/` + ); + const unique = new Set(prefixes); + expect(prefixes.length).toBe(unique.size); + }); + + test("self-hosted base URLs are preserved", () => { + const prefixes = computeInvalidationPrefixes( + "https://sentry.example.com/api/0/organizations/acme/issues/12345/" + ); + expect(prefixes).toContain( + "https://sentry.example.com/api/0/organizations/acme/issues/12345/" + ); + expect(prefixes).toContain( + "https://sentry.example.com/api/0/organizations/" + ); + }); +}); diff --git a/test/lib/sentry-client.invalidation.test.ts b/test/lib/sentry-client.invalidation.test.ts new file mode 100644 index 000000000..90ff602ef --- /dev/null +++ b/test/lib/sentry-client.invalidation.test.ts @@ -0,0 +1,199 @@ +/** + * Integration tests for the HTTP-layer auto-invalidation hook. + * + * The hook lives in `sentry-client.ts` (`invalidateAfterMutation`) + * and fires after every successful non-GET at the + * `authenticatedFetch` seam. Prefix computation is delegated to + * `computeInvalidationPrefixes`; this file verifies the end-to-end + * behavior through the real response cache. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { setAuthToken } from "../../src/lib/db/auth.js"; +import { + getCachedResponse, + storeCachedResponse, +} from "../../src/lib/response-cache.js"; +import { resetAuthenticatedFetch } from "../../src/lib/sentry-client.js"; +import { useTestConfigDir } from "../helpers.js"; + +useTestConfigDir("invalidation-"); + +/** + * Factory for a `Response` with a cacheable JSON body. Used both for + * priming the cache (`storeCachedResponse`) and for mock-fetch returns. + */ +function makeResponse( + body: unknown, + status = 200, + headers: Record = {} +): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json", ...headers }, + }); +} + +let originalFetch: typeof globalThis.fetch; +type FetchHandler = ( + input: Request | string | URL, + init?: RequestInit +) => Promise; + +beforeEach(() => { + originalFetch = globalThis.fetch; + resetAuthenticatedFetch(); + setAuthToken("test-token", 3600, "test-refresh"); +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + resetAuthenticatedFetch(); +}); + +/** Swap `globalThis.fetch` with a deterministic handler for one test. */ +function installMockFetch(handler: FetchHandler): void { + globalThis.fetch = ((input: Request | string | URL, init?: RequestInit) => + handler(input, init)) as typeof fetch; +} + +/** + * Run the authenticated fetch against a URL + method. Spins up the + * singleton fresh (we reset it in beforeEach) so it picks up whichever + * mock is installed. + */ +async function runAuthenticatedFetch( + url: string, + method = "GET" +): Promise { + // Import lazily so the module's cached state is reset between tests. + const { getSdkConfig } = await import("../../src/lib/sentry-client.js"); + const { fetch } = getSdkConfig("https://us.sentry.io"); + return fetch(url, { method }); +} + +const BASE = "https://us.sentry.io/api/0/"; +const DETAIL_URL = `${BASE}organizations/acme/issues/12345/`; +const LIST_URL = `${BASE}organizations/acme/issues/`; + +describe("HTTP-layer auto-invalidation", () => { + test("successful non-GET clears cached detail + list entries", async () => { + // Prime the cache as if earlier GETs populated these entries. + await storeCachedResponse( + "GET", + DETAIL_URL, + {}, + makeResponse({ id: "12345" }) + ); + await storeCachedResponse( + "GET", + `${LIST_URL}?cursor=abc`, + {}, + makeResponse({ data: [] }) + ); + expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); + expect( + await getCachedResponse("GET", `${LIST_URL}?cursor=abc`, {}) + ).toBeDefined(); + + // Perform a mutation on the detail URL. + installMockFetch(async (input, init) => { + expect(init?.method).toBe("PUT"); + expect(String(input)).toBe(DETAIL_URL); + return makeResponse({ id: "12345", status: "resolved" }); + }); + const response = await runAuthenticatedFetch(DETAIL_URL, "PUT"); + expect(response.ok).toBe(true); + + // Invalidation is awaited inside the fetch hook, so by the time + // the mutation's caller sees the response, the cache is already + // cleared — no race, no sleep needed. + expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeUndefined(); + expect( + await getCachedResponse("GET", `${LIST_URL}?cursor=abc`, {}) + ).toBeUndefined(); + }); + + test("failed non-GET does NOT invalidate the cache", async () => { + await storeCachedResponse( + "GET", + DETAIL_URL, + {}, + makeResponse({ id: "12345" }) + ); + expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); + + installMockFetch(async () => makeResponse({ error: "denied" }, 403)); + const response = await runAuthenticatedFetch(DETAIL_URL, "PUT"); + expect(response.status).toBe(403); + + // Cache entry survives — a rejected mutation didn't change state. + expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); + }); + + test("GET does NOT invalidate the cache", async () => { + await storeCachedResponse( + "GET", + DETAIL_URL, + {}, + makeResponse({ id: "12345" }) + ); + expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); + + // A fresh GET to a different URL — shouldn't touch existing cache entries. + installMockFetch(async () => makeResponse({ id: "99999" })); + await runAuthenticatedFetch( + `${BASE}organizations/acme/issues/99999/`, + "GET" + ); + + expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); + }); + + test("cross-endpoint rule fires for project delete", async () => { + // Prime the org project-list cache. + const orgListUrl = `${BASE}organizations/acme/projects/`; + await storeCachedResponse( + "GET", + `${orgListUrl}?cursor=xyz`, + {}, + makeResponse({ data: [] }) + ); + expect( + await getCachedResponse("GET", `${orgListUrl}?cursor=xyz`, {}) + ).toBeDefined(); + + // DELETE on the non-org-prefixed project URL. + const deleteUrl = `${BASE}projects/acme/frontend/`; + installMockFetch(async () => makeResponse({}, 204)); + await runAuthenticatedFetch(deleteUrl, "DELETE"); + + // The cross-endpoint rule sweeps the org project-list even though + // the mutation hit a different URL tree. + expect( + await getCachedResponse("GET", `${orgListUrl}?cursor=xyz`, {}) + ).toBeUndefined(); + }); + + test("another identity's cache survives a mutation", async () => { + // Identity A caches an entry. + setAuthToken("identity-a", 3600, "refresh-a"); + await storeCachedResponse( + "GET", + DETAIL_URL, + {}, + makeResponse({ owner: "a" }) + ); + expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); + + // Switch to identity B and mutate the same URL. + setAuthToken("identity-b", 3600, "refresh-b"); + installMockFetch(async () => makeResponse({}, 200)); + await runAuthenticatedFetch(DETAIL_URL, "PUT"); + + // Back as identity A: the entry must survive because invalidation + // is identity-gated. + setAuthToken("identity-a", 3600, "refresh-a"); + expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); + }); +}); From 1c42241fdf26ad4f4af3b1fafb99cae0c00dabce Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 21 Apr 2026 13:25:15 +0000 Subject: [PATCH 2/7] refactor(cache): cap hierarchy walk at owner level + drop defensive catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review-feedback fixes on #801: 1. **Cap the hierarchy walk.** Before: every mutation swept the bare top-level prefix (`/api/0/organizations/`, `/api/0/teams/`, etc.), which would evict cross-org caches on any single-org mutation. Now the walk stops at 2 segments — owner level (`organizations/{org}/`). Paths that are already ≤ 2 segments still walk to their root (a mutation targeting the root itself should clear its cache). Added a new test for the two-segment case. 2. **Drop the defensive try/catch in `invalidateAfterMutation`.** `invalidateCachedResponsesMatching` is already contractually no-throw; wrapping its `Promise.all` in another try/catch was dead code. Matches BYK's brevity preference in the lore. No behavior change beyond the walk cap. Tests updated accordingly. --- src/lib/cache-keys.ts | 19 ++++++++++++++++--- src/lib/sentry-client.ts | 27 +++++++-------------------- test/lib/cache-keys.test.ts | 33 +++++++++++++++++++++++---------- 3 files changed, 46 insertions(+), 33 deletions(-) diff --git a/src/lib/cache-keys.ts b/src/lib/cache-keys.ts index 32415e949..d73b75727 100644 --- a/src/lib/cache-keys.ts +++ b/src/lib/cache-keys.ts @@ -125,14 +125,21 @@ export function computeInvalidationPrefixes(fullUrl: string): string[] { /** * Yield every path-prefix sequence of `relPath` in descending length, - * always ending with a trailing slash. + * stopping at the "resource owner" level (typically `{root}/{owner}/`, + * e.g. `organizations/acme/`). The bare `organizations/` root is + * deliberately omitted — sweeping it on every mutation would evict + * unrelated cross-org caches, since a mutation under one org cannot + * invalidate another org's state. * * `"organizations/acme/releases/1.0.0/deploys/"` yields: * - `"organizations/acme/releases/1.0.0/deploys/"` * - `"organizations/acme/releases/1.0.0/"` * - `"organizations/acme/releases/"` * - `"organizations/acme/"` - * - `"organizations/"` + * + * Single-segment paths (e.g. `"organizations/"`) still yield themselves + * — a mutation at the resource-owner root is rare but the sweep should + * still clear its cache. */ function* ancestorSegments(relPath: string): Generator { const trimmed = relPath.endsWith("/") ? relPath.slice(0, -1) : relPath; @@ -140,7 +147,13 @@ function* ancestorSegments(relPath: string): Generator { return; } const parts = trimmed.split("/"); - for (let i = parts.length; i > 0; i--) { + // Stop at 2 segments when the path has more — a mutation under + // `organizations/acme/.../...` shouldn't sweep the bare + // `organizations/` root (would evict other orgs' caches). Paths + // with ≤ 2 segments (e.g. the `organizations/` root itself, or + // `teams/acme/`) still walk all the way down. + const floor = parts.length > 2 ? 2 : 1; + for (let i = parts.length; i >= floor; i--) { yield `${parts.slice(0, i).join("/")}/`; } } diff --git a/src/lib/sentry-client.ts b/src/lib/sentry-client.ts index 9d715955f..5ea2b53dc 100644 --- a/src/lib/sentry-client.ts +++ b/src/lib/sentry-client.ts @@ -293,21 +293,17 @@ function cacheResponse( /** * Auto-invalidate cache entries that a successful non-GET mutation * made stale. Awaited before returning the response so a subsequent - * read in the same command sees fresh data (the whole point of - * post-mutation invalidation). + * read in the same command sees fresh data. * * Prefix computation lives in {@link computeInvalidationPrefixes} * (hierarchy walk + cross-endpoint rules). Each prefix runs through - * `invalidateCachedResponsesMatching`, which is identity-gated so - * a mutation by one account can't evict another account's cache. + * `invalidateCachedResponsesMatching` — a no-throw helper that's + * already identity-gated so a mutation by one account can't evict + * another account's cache. * * GETs skip invalidation entirely; they go through the cache-write * path above. 4xx/5xx non-GETs skip too — a rejected mutation didn't * change server state so its cache is still accurate. - * - * Never throws: the helpers we call are already best-effort, but we - * wrap defensively because a housekeeping error must never surface - * as a user-visible failure after a successful mutation. */ async function invalidateAfterMutation( method: string, @@ -318,16 +314,9 @@ async function invalidateAfterMutation( return; } const prefixes = computeInvalidationPrefixes(fullUrl); - if (prefixes.length === 0) { - return; - } - try { - await Promise.all( - prefixes.map((prefix) => invalidateCachedResponsesMatching(prefix)) - ); - } catch { - /* best-effort: mutation already succeeded upstream */ - } + await Promise.all( + prefixes.map((prefix) => invalidateCachedResponsesMatching(prefix)) + ); } /** Build a `{ authorization }` header map from a bearer token, or `{}` if absent. */ @@ -363,8 +352,6 @@ async function fetchWithRetry( authHeaders(getAuthToken()), result.response ); - // Awaited so a subsequent read in the same command sees fresh - // data. Never throws — own contract enforced by the helper. await invalidateAfterMutation(method, fullUrl, result.response); return result.response; } diff --git a/test/lib/cache-keys.test.ts b/test/lib/cache-keys.test.ts index 17bea3ddf..65d3e29c3 100644 --- a/test/lib/cache-keys.test.ts +++ b/test/lib/cache-keys.test.ts @@ -8,35 +8,47 @@ import { computeInvalidationPrefixes } from "../../src/lib/cache-keys.js"; const BASE = "https://us.sentry.io/api/0/"; describe("computeInvalidationPrefixes — hierarchy walk", () => { - test("detail URL yields self + org list ancestor", () => { + test("detail URL yields self + ancestors down to owner", () => { const prefixes = computeInvalidationPrefixes( `${BASE}organizations/acme/issues/12345/` ); expect(prefixes).toContain(`${BASE}organizations/acme/issues/12345/`); expect(prefixes).toContain(`${BASE}organizations/acme/issues/`); expect(prefixes).toContain(`${BASE}organizations/acme/`); - expect(prefixes).toContain(`${BASE}organizations/`); + // The bare `organizations/` root is deliberately NOT swept — + // it would evict other orgs' caches. + expect(prefixes).not.toContain(`${BASE}organizations/`); }); - test("deeply nested path yields every ancestor", () => { + test("deeply nested path yields every ancestor down to owner", () => { const prefixes = computeInvalidationPrefixes( `${BASE}organizations/acme/releases/1.0.0/deploys/` ); - // Five segments → five ancestor prefixes. expect(prefixes).toContain( `${BASE}organizations/acme/releases/1.0.0/deploys/` ); expect(prefixes).toContain(`${BASE}organizations/acme/releases/1.0.0/`); expect(prefixes).toContain(`${BASE}organizations/acme/releases/`); expect(prefixes).toContain(`${BASE}organizations/acme/`); - expect(prefixes).toContain(`${BASE}organizations/`); + // Stop at the owner level, don't sweep bare `organizations/`. + expect(prefixes).not.toContain(`${BASE}organizations/`); }); test("single-segment path yields only the segment itself", () => { + // Paths with ≤ 2 segments walk all the way down — a mutation + // targeting the root itself should still clear its own cache. const prefixes = computeInvalidationPrefixes(`${BASE}organizations/`); expect(prefixes).toEqual([`${BASE}organizations/`]); }); + test("two-segment path walks to the top (owner-level mutation)", () => { + // `organizations/acme/` is the resource owner; the floor kicks in + // at length > 2, so this still yields both prefixes. + const prefixes = computeInvalidationPrefixes(`${BASE}organizations/acme/`); + expect(prefixes).toContain(`${BASE}organizations/acme/`); + expect(prefixes).toContain(`${BASE}organizations/`); + }); + test("query string is stripped from the prefix set", () => { // Prefix-sweep against the path catches every cached query variant, // so the query itself is irrelevant for computing prefixes. @@ -64,11 +76,11 @@ describe("computeInvalidationPrefixes — cross-endpoint rules", () => { `${BASE}teams/acme/backend/projects/` ); expect(prefixes).toContain(`${BASE}organizations/acme/projects/`); - // Plus the hierarchy walk: + // Plus the hierarchy walk (capped at the owner level): expect(prefixes).toContain(`${BASE}teams/acme/backend/projects/`); expect(prefixes).toContain(`${BASE}teams/acme/backend/`); expect(prefixes).toContain(`${BASE}teams/acme/`); - expect(prefixes).toContain(`${BASE}teams/`); + expect(prefixes).not.toContain(`${BASE}teams/`); }); test("DELETE /projects/{org}/{project}/ invalidates org project list", () => { @@ -76,10 +88,11 @@ describe("computeInvalidationPrefixes — cross-endpoint rules", () => { `${BASE}projects/acme/frontend/` ); expect(prefixes).toContain(`${BASE}organizations/acme/projects/`); - // Plus the hierarchy walk from the mutation URL: expect(prefixes).toContain(`${BASE}projects/acme/frontend/`); expect(prefixes).toContain(`${BASE}projects/acme/`); - expect(prefixes).toContain(`${BASE}projects/`); + // Two segments: floor allows the top (projects/). Reasonable — + // a DELETE on an owner-level resource does clear that root. + expect(prefixes).not.toContain(`${BASE}projects/`); }); test("unrelated paths get no cross-endpoint sweep", () => { @@ -133,7 +146,7 @@ describe("computeInvalidationPrefixes — edge cases", () => { "https://sentry.example.com/api/0/organizations/acme/issues/12345/" ); expect(prefixes).toContain( - "https://sentry.example.com/api/0/organizations/" + "https://sentry.example.com/api/0/organizations/acme/" ); }); }); From 297194694f90a49581590c95484cdafeae8a9124 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 20:26:20 +0000 Subject: [PATCH 3/7] chore(cache): trim AI-generated comment slop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tighten over-explained JSDoc and inline commentary in the PR's new files: - `cache-keys.ts`: drop the misleading "anchored regex" comment on `API_V0_SEGMENT` (it's a literal string + indexOf, not a regex), collapse verbose "Rule 1/Rule 2" section markers that restate the function names, trim per-step inline comments on variable assignments that just restate the code, remove redundant JSDoc-duplicating inline comment inside `ancestorSegments`. - `sentry-client.ts`: shrink `invalidateAfterMutation` JSDoc from ~14 lines to 4 — matches the neighboring `cacheResponse` style (3-line doc). - `sentry-client.invalidation.test.ts`: hoist the dynamic `await import("../../src/lib/sentry-client.js")` to a top-level static import (the "reset module state between tests" comment was wrong — dynamic import doesn't reset module-level caches). `runAuthenticatedFetch` is now synchronous. Drop unused `headers` parameter on `makeResponse`. Trim navigational comments that restate the code. - `cache-keys.test.ts`: trim purely-restating comments. Net: -95 lines. No behavior change. --- src/lib/cache-keys.ts | 41 ++----------- src/lib/sentry-client.ts | 14 +---- test/lib/cache-keys.test.ts | 20 +------ test/lib/sentry-client.invalidation.test.ts | 65 ++++----------------- 4 files changed, 20 insertions(+), 120 deletions(-) diff --git a/src/lib/cache-keys.ts b/src/lib/cache-keys.ts index d73b75727..a68a24038 100644 --- a/src/lib/cache-keys.ts +++ b/src/lib/cache-keys.ts @@ -27,40 +27,23 @@ * query-param variant cached under that path). */ -/** Regex capturing the `/api/0/` boundary. Anchored so it matches only the canonical API prefix. */ const API_V0_SEGMENT = "/api/0/"; -/** - * Cross-endpoint invalidation rules. - * - * Each rule is a pattern the mutation URL path must match, plus a - * function that returns additional prefixes to sweep. Patterns match - * against the *path*, not the full URL — so the prefix returned is - * prepended with the request's base later. - * - * Keep this table small. The hierarchy walk handles most cases; add a - * rule here only when the API's cross-tree relationships force it. - */ +/** Rule for mutations whose effects cross URL trees. Patterns match the path relative to `/api/0/`. */ type CrossEndpointRule = { - /** Matches the path relative to `/api/0/` (no leading slash). */ match: RegExp; - /** Returns additional path prefixes (relative to `/api/0/`) to sweep. */ extra: (matchGroups: RegExpMatchArray) => string[]; }; const CROSS_ENDPOINT_RULES: CrossEndpointRule[] = [ + // `POST teams/{org}/{team}/projects/` (create project in team) also + // invalidates the org project list at `organizations/{org}/projects/`. { - // POST /api/0/teams/{org}/{team}/projects/ (create a project in a team) - // invalidates the org project list at - // /api/0/organizations/{org}/projects/ which lives under a - // different URL tree. match: /^teams\/([^/]+)\/[^/]+\/projects\/?$/, extra: ([, org]) => [`organizations/${org}/projects/`], }, + // `DELETE projects/{org}/{project}/` also invalidates the org project list. { - // DELETE /api/0/projects/{org}/{project}/ (delete a project) - // invalidates the org project list at - // /api/0/organizations/{org}/projects/ (different URL tree). match: /^projects\/([^/]+)\/[^/]+\/?$/, extra: ([, org]) => [`organizations/${org}/projects/`], }, @@ -91,26 +74,16 @@ export function computeInvalidationPrefixes(fullUrl: string): string[] { return []; } - // `base` includes origin + path up through and including `/api/0/`. const base = `${parsed.origin}${parsed.pathname.slice(0, apiIdx + API_V0_SEGMENT.length)}`; - // Path below `/api/0/`, leading slash trimmed, trailing slash kept - // so it matches against rules that anchor on `/?$`. const relPath = parsed.pathname.slice(apiIdx + API_V0_SEGMENT.length); - - // No relative path means the mutation hit `/api/0/` itself; nothing to sweep. if (relPath === "") { return []; } const prefixes = new Set(); - - // Rule 1: hierarchy walk. Sweep the URL's own path plus every - // ancestor with at least one segment. for (const segments of ancestorSegments(relPath)) { prefixes.add(`${base}${segments}`); } - - // Rule 2: cross-endpoint table. for (const rule of CROSS_ENDPOINT_RULES) { const match = relPath.match(rule.match); if (match) { @@ -119,7 +92,6 @@ export function computeInvalidationPrefixes(fullUrl: string): string[] { } } } - return [...prefixes]; } @@ -147,11 +119,6 @@ function* ancestorSegments(relPath: string): Generator { return; } const parts = trimmed.split("/"); - // Stop at 2 segments when the path has more — a mutation under - // `organizations/acme/.../...` shouldn't sweep the bare - // `organizations/` root (would evict other orgs' caches). Paths - // with ≤ 2 segments (e.g. the `organizations/` root itself, or - // `teams/acme/`) still walk all the way down. const floor = parts.length > 2 ? 2 : 1; for (let i = parts.length; i >= floor; i--) { yield `${parts.slice(0, i).join("/")}/`; diff --git a/src/lib/sentry-client.ts b/src/lib/sentry-client.ts index 5ea2b53dc..2e0bd8e25 100644 --- a/src/lib/sentry-client.ts +++ b/src/lib/sentry-client.ts @@ -292,18 +292,8 @@ function cacheResponse( /** * Auto-invalidate cache entries that a successful non-GET mutation - * made stale. Awaited before returning the response so a subsequent - * read in the same command sees fresh data. - * - * Prefix computation lives in {@link computeInvalidationPrefixes} - * (hierarchy walk + cross-endpoint rules). Each prefix runs through - * `invalidateCachedResponsesMatching` — a no-throw helper that's - * already identity-gated so a mutation by one account can't evict - * another account's cache. - * - * GETs skip invalidation entirely; they go through the cache-write - * path above. 4xx/5xx non-GETs skip too — a rejected mutation didn't - * change server state so its cache is still accurate. + * made stale. Awaited so a subsequent read in the same command sees + * fresh data. Prefix computation: {@link computeInvalidationPrefixes}. */ async function invalidateAfterMutation( method: string, diff --git a/test/lib/cache-keys.test.ts b/test/lib/cache-keys.test.ts index 65d3e29c3..7f23cea7b 100644 --- a/test/lib/cache-keys.test.ts +++ b/test/lib/cache-keys.test.ts @@ -35,23 +35,18 @@ describe("computeInvalidationPrefixes — hierarchy walk", () => { }); test("single-segment path yields only the segment itself", () => { - // Paths with ≤ 2 segments walk all the way down — a mutation - // targeting the root itself should still clear its own cache. const prefixes = computeInvalidationPrefixes(`${BASE}organizations/`); expect(prefixes).toEqual([`${BASE}organizations/`]); }); test("two-segment path walks to the top (owner-level mutation)", () => { - // `organizations/acme/` is the resource owner; the floor kicks in - // at length > 2, so this still yields both prefixes. + // The floor only kicks in at length > 2. const prefixes = computeInvalidationPrefixes(`${BASE}organizations/acme/`); expect(prefixes).toContain(`${BASE}organizations/acme/`); expect(prefixes).toContain(`${BASE}organizations/`); }); test("query string is stripped from the prefix set", () => { - // Prefix-sweep against the path catches every cached query variant, - // so the query itself is irrelevant for computing prefixes. const prefixes = computeInvalidationPrefixes( `${BASE}organizations/acme/issues/12345/?collapse=stats&collapse=lifetime` ); @@ -76,7 +71,6 @@ describe("computeInvalidationPrefixes — cross-endpoint rules", () => { `${BASE}teams/acme/backend/projects/` ); expect(prefixes).toContain(`${BASE}organizations/acme/projects/`); - // Plus the hierarchy walk (capped at the owner level): expect(prefixes).toContain(`${BASE}teams/acme/backend/projects/`); expect(prefixes).toContain(`${BASE}teams/acme/backend/`); expect(prefixes).toContain(`${BASE}teams/acme/`); @@ -90,8 +84,6 @@ describe("computeInvalidationPrefixes — cross-endpoint rules", () => { expect(prefixes).toContain(`${BASE}organizations/acme/projects/`); expect(prefixes).toContain(`${BASE}projects/acme/frontend/`); expect(prefixes).toContain(`${BASE}projects/acme/`); - // Two segments: floor allows the top (projects/). Reasonable — - // a DELETE on an owner-level resource does clear that root. expect(prefixes).not.toContain(`${BASE}projects/`); }); @@ -99,8 +91,6 @@ describe("computeInvalidationPrefixes — cross-endpoint rules", () => { const prefixes = computeInvalidationPrefixes( `${BASE}organizations/acme/teams/` ); - // `organizations/.../teams/` doesn't match either cross-endpoint - // rule, so the result is only the hierarchy walk. expect(prefixes).not.toContain(`${BASE}organizations/acme/projects/`); expect(prefixes).toContain(`${BASE}organizations/acme/teams/`); }); @@ -111,7 +101,6 @@ describe("computeInvalidationPrefixes — edge cases", () => { expect( computeInvalidationPrefixes("https://example.com/some/path/") ).toEqual([]); - // Chunk-upload endpoints are arbitrary host URLs. expect( computeInvalidationPrefixes("https://uploads.sentry.io/x/y") ).toEqual([]); @@ -127,15 +116,10 @@ describe("computeInvalidationPrefixes — edge cases", () => { }); test("deduplicates prefixes across hierarchy + rule-table", () => { - // `teams/acme/backend/projects/` matches the cross-endpoint rule - // AND naturally walks up through `teams/acme/backend/` etc. The - // rule adds `organizations/acme/projects/` (unique). No duplicates - // should appear in the result. const prefixes = computeInvalidationPrefixes( `${BASE}teams/acme/backend/projects/` ); - const unique = new Set(prefixes); - expect(prefixes.length).toBe(unique.size); + expect(prefixes.length).toBe(new Set(prefixes).size); }); test("self-hosted base URLs are preserved", () => { diff --git a/test/lib/sentry-client.invalidation.test.ts b/test/lib/sentry-client.invalidation.test.ts index 90ff602ef..82ea4d1d3 100644 --- a/test/lib/sentry-client.invalidation.test.ts +++ b/test/lib/sentry-client.invalidation.test.ts @@ -14,23 +14,18 @@ import { getCachedResponse, storeCachedResponse, } from "../../src/lib/response-cache.js"; -import { resetAuthenticatedFetch } from "../../src/lib/sentry-client.js"; +import { + getSdkConfig, + resetAuthenticatedFetch, +} from "../../src/lib/sentry-client.js"; import { useTestConfigDir } from "../helpers.js"; useTestConfigDir("invalidation-"); -/** - * Factory for a `Response` with a cacheable JSON body. Used both for - * priming the cache (`storeCachedResponse`) and for mock-fetch returns. - */ -function makeResponse( - body: unknown, - status = 200, - headers: Record = {} -): Response { +function makeResponse(body: unknown, status = 200): Response { return new Response(JSON.stringify(body), { status, - headers: { "content-type": "application/json", ...headers }, + headers: { "content-type": "application/json" }, }); } @@ -57,19 +52,9 @@ function installMockFetch(handler: FetchHandler): void { handler(input, init)) as typeof fetch; } -/** - * Run the authenticated fetch against a URL + method. Spins up the - * singleton fresh (we reset it in beforeEach) so it picks up whichever - * mock is installed. - */ -async function runAuthenticatedFetch( - url: string, - method = "GET" -): Promise { - // Import lazily so the module's cached state is reset between tests. - const { getSdkConfig } = await import("../../src/lib/sentry-client.js"); - const { fetch } = getSdkConfig("https://us.sentry.io"); - return fetch(url, { method }); +/** Run the authenticated fetch. `resetAuthenticatedFetch` in beforeEach ensures the singleton picks up the current mock. */ +function runAuthenticatedFetch(url: string, method = "GET"): Promise { + return getSdkConfig("https://us.sentry.io").fetch(url, { method }); } const BASE = "https://us.sentry.io/api/0/"; @@ -78,7 +63,6 @@ const LIST_URL = `${BASE}organizations/acme/issues/`; describe("HTTP-layer auto-invalidation", () => { test("successful non-GET clears cached detail + list entries", async () => { - // Prime the cache as if earlier GETs populated these entries. await storeCachedResponse( "GET", DETAIL_URL, @@ -91,12 +75,7 @@ describe("HTTP-layer auto-invalidation", () => { {}, makeResponse({ data: [] }) ); - expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); - expect( - await getCachedResponse("GET", `${LIST_URL}?cursor=abc`, {}) - ).toBeDefined(); - // Perform a mutation on the detail URL. installMockFetch(async (input, init) => { expect(init?.method).toBe("PUT"); expect(String(input)).toBe(DETAIL_URL); @@ -105,9 +84,8 @@ describe("HTTP-layer auto-invalidation", () => { const response = await runAuthenticatedFetch(DETAIL_URL, "PUT"); expect(response.ok).toBe(true); - // Invalidation is awaited inside the fetch hook, so by the time - // the mutation's caller sees the response, the cache is already - // cleared — no race, no sleep needed. + // Invalidation is awaited inside the hook, so the cache is + // already cleared when the caller sees the response. expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeUndefined(); expect( await getCachedResponse("GET", `${LIST_URL}?cursor=abc`, {}) @@ -121,13 +99,10 @@ describe("HTTP-layer auto-invalidation", () => { {}, makeResponse({ id: "12345" }) ); - expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); installMockFetch(async () => makeResponse({ error: "denied" }, 403)); const response = await runAuthenticatedFetch(DETAIL_URL, "PUT"); expect(response.status).toBe(403); - - // Cache entry survives — a rejected mutation didn't change state. expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); }); @@ -138,20 +113,16 @@ describe("HTTP-layer auto-invalidation", () => { {}, makeResponse({ id: "12345" }) ); - expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); - // A fresh GET to a different URL — shouldn't touch existing cache entries. installMockFetch(async () => makeResponse({ id: "99999" })); await runAuthenticatedFetch( `${BASE}organizations/acme/issues/99999/`, "GET" ); - expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); }); test("cross-endpoint rule fires for project delete", async () => { - // Prime the org project-list cache. const orgListUrl = `${BASE}organizations/acme/projects/`; await storeCachedResponse( "GET", @@ -159,24 +130,16 @@ describe("HTTP-layer auto-invalidation", () => { {}, makeResponse({ data: [] }) ); - expect( - await getCachedResponse("GET", `${orgListUrl}?cursor=xyz`, {}) - ).toBeDefined(); - // DELETE on the non-org-prefixed project URL. - const deleteUrl = `${BASE}projects/acme/frontend/`; installMockFetch(async () => makeResponse({}, 204)); - await runAuthenticatedFetch(deleteUrl, "DELETE"); + await runAuthenticatedFetch(`${BASE}projects/acme/frontend/`, "DELETE"); - // The cross-endpoint rule sweeps the org project-list even though - // the mutation hit a different URL tree. expect( await getCachedResponse("GET", `${orgListUrl}?cursor=xyz`, {}) ).toBeUndefined(); }); test("another identity's cache survives a mutation", async () => { - // Identity A caches an entry. setAuthToken("identity-a", 3600, "refresh-a"); await storeCachedResponse( "GET", @@ -184,15 +147,11 @@ describe("HTTP-layer auto-invalidation", () => { {}, makeResponse({ owner: "a" }) ); - expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); - // Switch to identity B and mutate the same URL. setAuthToken("identity-b", 3600, "refresh-b"); installMockFetch(async () => makeResponse({}, 200)); await runAuthenticatedFetch(DETAIL_URL, "PUT"); - // Back as identity A: the entry must survive because invalidation - // is identity-gated. setAuthToken("identity-a", 3600, "refresh-a"); expect(await getCachedResponse("GET", DETAIL_URL, {})).toBeDefined(); }); From ae3fa968ff9af0ba7d51f486d5bb4b821178535a Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 20:48:23 +0000 Subject: [PATCH 4/7] fix(cache): skip invalidation for write-only endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-feedback fix on #801: The hierarchy walk broadcasts invalidation up the URL tree, which is correct for mutations that modify cacheable state — but sourcemap chunk uploads and artifact-bundle assembly are write-only (no GET reads anything under those paths). Without this carve-out, a single `sentry sourcemap upload` run would sweep the org's cached state hundreds of times (one sweep per chunk POST × concurrency), nuking the user's cache for an operation that changed nothing observable. `SKIP_INVALIDATION_PATTERNS` short-circuits `computeInvalidationPrefixes` for these paths. Test added. Also clarified the fetch-mockability comment in sentry-client.invalidation.test.ts — the mock works because `fetchWithTimeout` calls `fetch(...)` as a bare global reference, not because `resetAuthenticatedFetch` does anything fetch-related. --- src/lib/cache-keys.ts | 17 +++++++++++++++++ test/lib/cache-keys.test.ts | 14 ++++++++++++++ test/lib/sentry-client.invalidation.test.ts | 6 +++++- 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/lib/cache-keys.ts b/src/lib/cache-keys.ts index a68a24038..6c776c23f 100644 --- a/src/lib/cache-keys.ts +++ b/src/lib/cache-keys.ts @@ -29,6 +29,19 @@ const API_V0_SEGMENT = "/api/0/"; +/** + * Paths where a mutation doesn't change any cacheable state — typically + * write-only endpoints like chunk uploads and bundle assembly. Invalidation + * on these would pointlessly sweep the org's cache on every chunk of a + * sourcemap upload. + */ +const SKIP_INVALIDATION_PATTERNS: readonly RegExp[] = [ + // Sourcemap chunk upload + bundle assemble. Both are write-only in the + // sense that no GET endpoint reads cacheable state under these paths. + /\/chunk-upload\//, + /\/artifactbundle\/assemble\//, +]; + /** Rule for mutations whose effects cross URL trees. Patterns match the path relative to `/api/0/`. */ type CrossEndpointRule = { match: RegExp; @@ -74,6 +87,10 @@ export function computeInvalidationPrefixes(fullUrl: string): string[] { return []; } + if (SKIP_INVALIDATION_PATTERNS.some((p) => p.test(parsed.pathname))) { + return []; + } + const base = `${parsed.origin}${parsed.pathname.slice(0, apiIdx + API_V0_SEGMENT.length)}`; const relPath = parsed.pathname.slice(apiIdx + API_V0_SEGMENT.length); if (relPath === "") { diff --git a/test/lib/cache-keys.test.ts b/test/lib/cache-keys.test.ts index 7f23cea7b..dcd73e520 100644 --- a/test/lib/cache-keys.test.ts +++ b/test/lib/cache-keys.test.ts @@ -122,6 +122,20 @@ describe("computeInvalidationPrefixes — edge cases", () => { expect(prefixes.length).toBe(new Set(prefixes).size); }); + test("write-only endpoints skip invalidation entirely", () => { + // Sourcemap chunk uploads and bundle assembly don't modify any + // cacheable state; without this skip, every chunk POST would + // sweep the org's cache hierarchy. + expect( + computeInvalidationPrefixes(`${BASE}organizations/acme/chunk-upload/`) + ).toEqual([]); + expect( + computeInvalidationPrefixes( + `${BASE}organizations/acme/artifactbundle/assemble/` + ) + ).toEqual([]); + }); + test("self-hosted base URLs are preserved", () => { const prefixes = computeInvalidationPrefixes( "https://sentry.example.com/api/0/organizations/acme/issues/12345/" diff --git a/test/lib/sentry-client.invalidation.test.ts b/test/lib/sentry-client.invalidation.test.ts index 82ea4d1d3..b799c27d3 100644 --- a/test/lib/sentry-client.invalidation.test.ts +++ b/test/lib/sentry-client.invalidation.test.ts @@ -52,7 +52,11 @@ function installMockFetch(handler: FetchHandler): void { handler(input, init)) as typeof fetch; } -/** Run the authenticated fetch. `resetAuthenticatedFetch` in beforeEach ensures the singleton picks up the current mock. */ +/** + * Run the authenticated fetch. Works because `sentry-client.ts` calls + * `fetch(...)` as a bare global reference (see `fetchWithTimeout`), so + * swapping `globalThis.fetch` per-test is observable. + */ function runAuthenticatedFetch(url: string, method = "GET"): Promise { return getSdkConfig("https://us.sentry.io").fetch(url, { method }); } From ea6d92ab303830370fb1390975e6ef5ddc06099e Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 21:04:26 +0000 Subject: [PATCH 5/7] chore(cache): remove now-unused invalidateCachedResponse + buildApiUrl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor-bot findings on #801: both helpers became dead code when this PR removed their last callers (the per-site invalidation helpers in api/issues.ts, api/projects.ts, api/dashboards.ts). - `invalidateCachedResponse` (exact-match variant): replaced by `invalidateCachedResponsesMatching` everywhere. The exact-match variant silently misses URLs with query strings (getIssue caches under `?collapse=...`), so callers were already migrating to the prefix-sweep form anyway. - `buildApiUrl`: the HTTP-layer hook works on full URLs parsed by `new URL()`, not template-string construction. If a future caller needs safe Sentry-API URL construction from slugs, resurrect it via git log — the tests document the contract. Net: -60 lines. --- src/lib/api/infrastructure.ts | 21 ----------------- src/lib/response-cache.ts | 14 ----------- test/lib/api/infrastructure.test.ts | 36 +---------------------------- test/lib/response-cache.test.ts | 11 --------- 4 files changed, 1 insertion(+), 81 deletions(-) diff --git a/src/lib/api/infrastructure.ts b/src/lib/api/infrastructure.ts index aadca8a1b..1c1b0d644 100644 --- a/src/lib/api/infrastructure.ts +++ b/src/lib/api/infrastructure.ts @@ -179,27 +179,6 @@ export async function getOrgSdkConfig(orgSlug: string) { return getSdkConfig(regionUrl); } -/** - * Build a full Sentry API URL from a region base and path segments. - * - * Each segment is passed through `encodeURIComponent`, so callers can - * pass user-supplied slugs directly without worrying about slashes, - * spaces, or other reserved characters. The `/api/0/` prefix and the - * required trailing slash are added automatically. - * - * @example - * buildApiUrl(regionUrl, "organizations", orgSlug, "projects") - * // → `${regionUrl.replace(/\/$/,"")}/api/0/organizations//projects/` - */ -export function buildApiUrl(regionUrl: string, ...segments: string[]): string { - const base = regionUrl.endsWith("/") ? regionUrl.slice(0, -1) : regionUrl; - if (segments.length === 0) { - return `${base}/api/0/`; - } - const path = segments.map(encodeURIComponent).join("/"); - return `${base}/api/0/${path}/`; -} - /** * Maximum number of pages to follow when auto-paginating. * diff --git a/src/lib/response-cache.ts b/src/lib/response-cache.ts index 1e45dd0b4..154b9c755 100644 --- a/src/lib/response-cache.ts +++ b/src/lib/response-cache.ts @@ -602,20 +602,6 @@ async function writeResponseToCache(req: WriteRequest): Promise { return serialized.length; } -/** - * Invalidate the cached GET response for a specific URL. Best-effort; - * never throws. Used by mutation commands so subsequent GETs don't - * serve stale data. - */ -export async function invalidateCachedResponse(url: string): Promise { - try { - const key = buildCacheKey("GET", url); - await rm(cacheFilePath(key), { force: true }); - } catch { - /* best-effort */ - } -} - /** * Invalidate every cached GET whose URL starts with `prefix` and * belongs to the current identity. Best-effort; never throws. diff --git a/test/lib/api/infrastructure.test.ts b/test/lib/api/infrastructure.test.ts index d93fa71eb..26a47fb6c 100644 --- a/test/lib/api/infrastructure.test.ts +++ b/test/lib/api/infrastructure.test.ts @@ -1,8 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { - buildApiUrl, - throwApiError, -} from "../../../src/lib/api/infrastructure.js"; +import { throwApiError } from "../../../src/lib/api/infrastructure.js"; import { ApiError } from "../../../src/lib/errors.js"; describe("throwApiError", () => { @@ -102,34 +99,3 @@ describe("throwApiError", () => { } }); }); - -describe("buildApiUrl", () => { - const BASE = "https://us.sentry.io"; - - test("composes /api/0/ + segments with a trailing slash", () => { - expect(buildApiUrl(BASE, "organizations", "acme", "projects")).toBe( - "https://us.sentry.io/api/0/organizations/acme/projects/" - ); - }); - - test("strips a single trailing slash from the base", () => { - expect(buildApiUrl(`${BASE}/`, "organizations", "acme")).toBe( - "https://us.sentry.io/api/0/organizations/acme/" - ); - }); - - test("URL-encodes each segment so reserved chars are safe", () => { - // Slash in a user-supplied slug used to break manual template-string - // builders. encodeURIComponent renders it as %2F. - expect(buildApiUrl(BASE, "organizations", "acme/evil", "projects")).toBe( - "https://us.sentry.io/api/0/organizations/acme%2Fevil/projects/" - ); - expect(buildApiUrl(BASE, "projects", "my org", "my proj")).toBe( - "https://us.sentry.io/api/0/projects/my%20org/my%20proj/" - ); - }); - - test("handles zero segments — returns the base /api/0/ root", () => { - expect(buildApiUrl(BASE)).toBe("https://us.sentry.io/api/0/"); - }); -}); diff --git a/test/lib/response-cache.test.ts b/test/lib/response-cache.test.ts index d402141fb..71f48dbb7 100644 --- a/test/lib/response-cache.test.ts +++ b/test/lib/response-cache.test.ts @@ -14,7 +14,6 @@ import { clearResponseCache, disableResponseCache, getCachedResponse, - invalidateCachedResponse, invalidateCachedResponsesMatching, normalizeUrl, resetCacheState, @@ -533,16 +532,6 @@ describe("invalidateCachedResponsesMatching", () => { }); }); -describe("invalidateCachedResponse", () => { - test("removes the exact cached entry for the current identity", async () => { - await storeCachedResponse("GET", TEST_URL, {}, mockResponse(TEST_BODY)); - expect(await getCachedResponse("GET", TEST_URL, {})).toBeDefined(); - - await invalidateCachedResponse(TEST_URL); - expect(await getCachedResponse("GET", TEST_URL, {})).toBeUndefined(); - }); -}); - // --------------------------------------------------------------------------- // Regression: prefix-sweep must catch query-string variants // (sentry-bot finding on #788 — `getIssue` caches under From e20fc49219138d3d9f40faf78a1234cdacdc6824 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 21:23:40 +0000 Subject: [PATCH 6/7] fix(cache): cross-origin legacy-issue invalidation + defensive try/catch Two Cursor-bot findings on ea6d92a: 1. **Cross-origin legacy issue cache not invalidated (LOW)**. The old per-site `invalidateIssueDetailCaches` explicitly cleared both the org-scoped URL at the region (e.g. `us.sentry.io`) and the legacy `/issues/{id}/` URL under the control-silo base (e.g. `sentry.io`). On SaaS those are different origins. My HTTP-layer hook only saw the mutation URL's origin, so `updateIssueStatus` / `mergeIssues` would leave stale legacy `getIssue()` cache under a different origin. Fix: extend the cross-endpoint rule table to support `extraAbsolute` (full-URL outputs, not just relative paths). Added a rule for `organizations/{org}/issues/{id}/` that emits `{apiBaseUrl}/api/0/issues/{id}/` as an additional prefix. `computeInvalidationPrefixes` now takes `apiBaseUrl` as a parameter; `invalidateAfterMutation` passes `getApiBaseUrl()`. Test added. 2. **Missing try/catch around `invalidateAfterMutation` (MEDIUM)**. Re-added the defensive wrapper. Today's code provably can't throw (all inputs are safe, both helpers have internal try/catch), but a post-mutation housekeeping error converting a successful mutation into a user-visible failure is exactly the thing we want belt-and-suspenders protection against. Defense-in-depth. Also fixed two lint errors introduced by the cross-endpoint rule expansion: - Hoist inline regex `/\/$/` to `TRAILING_SLASH_RE` at module scope. - Extract cross-endpoint rule application to `applyCrossEndpointRules` generator to keep `computeInvalidationPrefixes` under Biome's cognitive-complexity ceiling. --- src/lib/cache-keys.ts | 63 +++++++++++++++++++++++++++++++------ src/lib/sentry-client.ts | 17 +++++++--- test/lib/cache-keys.test.ts | 22 ++++++++++++- 3 files changed, 87 insertions(+), 15 deletions(-) diff --git a/src/lib/cache-keys.ts b/src/lib/cache-keys.ts index 6c776c23f..e9c7bb912 100644 --- a/src/lib/cache-keys.ts +++ b/src/lib/cache-keys.ts @@ -28,6 +28,7 @@ */ const API_V0_SEGMENT = "/api/0/"; +const TRAILING_SLASH_RE = /\/$/; /** * Paths where a mutation doesn't change any cacheable state — typically @@ -42,10 +43,20 @@ const SKIP_INVALIDATION_PATTERNS: readonly RegExp[] = [ /\/artifactbundle\/assemble\//, ]; -/** Rule for mutations whose effects cross URL trees. Patterns match the path relative to `/api/0/`. */ +/** + * Rule for mutations whose effects cross URL trees. Patterns match + * the path relative to `/api/0/`. `extra` returns additional + * path-prefixes (swept under the mutation's own origin); `extraAbsolute` + * returns absolute URL prefixes (when invalidation must cross origins, + * e.g. region-scoped mutation clearing a cache under the control silo). + */ type CrossEndpointRule = { match: RegExp; - extra: (matchGroups: RegExpMatchArray) => string[]; + extra?: (matchGroups: RegExpMatchArray) => string[]; + extraAbsolute?: ( + matchGroups: RegExpMatchArray, + ctx: { apiBaseUrl: string } + ) => string[]; }; const CROSS_ENDPOINT_RULES: CrossEndpointRule[] = [ @@ -60,6 +71,18 @@ const CROSS_ENDPOINT_RULES: CrossEndpointRule[] = [ match: /^projects\/([^/]+)\/[^/]+\/?$/, extra: ([, org]) => [`organizations/${org}/projects/`], }, + // Org-scoped issue mutations at `organizations/{org}/issues/{id}/` + // also affect the legacy global endpoint at `issues/{id}/`, which + // `getIssue()` hits under the control-silo base URL (potentially a + // DIFFERENT origin than the org's region URL). Must clear the + // legacy cache too, or subsequent `getIssue()` calls serve stale + // data. + { + match: /^organizations\/[^/]+\/issues\/([^/]+)\/?$/, + extraAbsolute: ([, issueId], { apiBaseUrl }) => [ + `${apiBaseUrl.replace(TRAILING_SLASH_RE, "")}/api/0/issues/${issueId}/`, + ], + }, ]; /** @@ -68,13 +91,18 @@ const CROSS_ENDPOINT_RULES: CrossEndpointRule[] = [ * * @param fullUrl - Fully-qualified URL of the mutation (absolute, * including base). Query string is ignored. - * @returns Array of full-URL prefixes (including base and - * `/api/0/`) ready to pass to + * @param apiBaseUrl - The CLI's non-region API base URL (used by rules + * that need to clear caches under a different origin — e.g. the + * legacy `/issues/{id}/` endpoint that `getIssue()` hits). + * @returns Array of full-URL prefixes ready to pass to * `invalidateCachedResponsesMatching`. Returns `[]` if the URL is * not under `/api/0/` (e.g. sourcemap chunk upload to an arbitrary * endpoint) or can't be parsed. */ -export function computeInvalidationPrefixes(fullUrl: string): string[] { +export function computeInvalidationPrefixes( + fullUrl: string, + apiBaseUrl: string +): string[] { let parsed: URL; try { parsed = new URL(fullUrl); @@ -101,15 +129,30 @@ export function computeInvalidationPrefixes(fullUrl: string): string[] { for (const segments of ancestorSegments(relPath)) { prefixes.add(`${base}${segments}`); } + for (const extra of applyCrossEndpointRules(relPath, base, apiBaseUrl)) { + prefixes.add(extra); + } + return [...prefixes]; +} + +/** Apply the cross-endpoint rule table, yielding absolute prefix URLs. */ +function* applyCrossEndpointRules( + relPath: string, + base: string, + apiBaseUrl: string +): Generator { for (const rule of CROSS_ENDPOINT_RULES) { const match = relPath.match(rule.match); - if (match) { - for (const extra of rule.extra(match)) { - prefixes.add(`${base}${extra}`); - } + if (!match) { + continue; + } + for (const extra of rule.extra?.(match) ?? []) { + yield `${base}${extra}`; + } + for (const absolute of rule.extraAbsolute?.(match, { apiBaseUrl }) ?? []) { + yield absolute; } } - return [...prefixes]; } /** diff --git a/src/lib/sentry-client.ts b/src/lib/sentry-client.ts index 2e0bd8e25..7353fea10 100644 --- a/src/lib/sentry-client.ts +++ b/src/lib/sentry-client.ts @@ -294,6 +294,11 @@ function cacheResponse( * Auto-invalidate cache entries that a successful non-GET mutation * made stale. Awaited so a subsequent read in the same command sees * fresh data. Prefix computation: {@link computeInvalidationPrefixes}. + * + * Never throws: a post-mutation housekeeping failure must not convert + * a successful mutation into a caller-visible error. Defense-in-depth + * for future regressions — the helpers we call are already no-throw + * today. */ async function invalidateAfterMutation( method: string, @@ -303,10 +308,14 @@ async function invalidateAfterMutation( if (method === "GET" || !response.ok) { return; } - const prefixes = computeInvalidationPrefixes(fullUrl); - await Promise.all( - prefixes.map((prefix) => invalidateCachedResponsesMatching(prefix)) - ); + try { + const prefixes = computeInvalidationPrefixes(fullUrl, getApiBaseUrl()); + await Promise.all( + prefixes.map((prefix) => invalidateCachedResponsesMatching(prefix)) + ); + } catch { + /* best-effort: mutation already succeeded upstream */ + } } /** Build a `{ authorization }` header map from a bearer token, or `{}` if absent. */ diff --git a/test/lib/cache-keys.test.ts b/test/lib/cache-keys.test.ts index dcd73e520..66de9d889 100644 --- a/test/lib/cache-keys.test.ts +++ b/test/lib/cache-keys.test.ts @@ -3,9 +3,15 @@ */ import { describe, expect, test } from "bun:test"; -import { computeInvalidationPrefixes } from "../../src/lib/cache-keys.js"; +import { computeInvalidationPrefixes as computeInvalidationPrefixesRaw } from "../../src/lib/cache-keys.js"; const BASE = "https://us.sentry.io/api/0/"; +const API_BASE_URL = "https://sentry.io"; + +/** Test wrapper that pins `apiBaseUrl` to the control-silo default. */ +function computeInvalidationPrefixes(fullUrl: string): string[] { + return computeInvalidationPrefixesRaw(fullUrl, API_BASE_URL); +} describe("computeInvalidationPrefixes — hierarchy walk", () => { test("detail URL yields self + ancestors down to owner", () => { @@ -87,6 +93,20 @@ describe("computeInvalidationPrefixes — cross-endpoint rules", () => { expect(prefixes).not.toContain(`${BASE}projects/`); }); + test("org-scoped issue mutation invalidates cross-origin legacy endpoint", () => { + // `updateIssueStatus` / `mergeIssues` hit the org-scoped endpoint + // at the region URL, but `getIssue()` caches under the control-silo + // URL at a DIFFERENT origin. Without a cross-origin rule, stale + // legacy cache entries survive org-scoped mutations. + const prefixes = computeInvalidationPrefixes( + `${BASE}organizations/acme/issues/12345/` + ); + expect(prefixes).toContain(`${API_BASE_URL}/api/0/issues/12345/`); + // Plus the hierarchy walk under the mutation's own origin: + expect(prefixes).toContain(`${BASE}organizations/acme/issues/12345/`); + expect(prefixes).toContain(`${BASE}organizations/acme/issues/`); + }); + test("unrelated paths get no cross-endpoint sweep", () => { const prefixes = computeInvalidationPrefixes( `${BASE}organizations/acme/teams/` From 036b0e7cf584055b8c86c634dbc6bed79be39544 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 21:49:18 +0000 Subject: [PATCH 7/7] fix(cache): mergeIssues manually invalidates per-ID legacy caches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor-bot finding on e20fc49: the HTTP-layer hook only sees the mutation URL, which for bulk merge is `organizations/{org}/issues/` (affected IDs live in query params, stripped by `URL.pathname`). The cross-endpoint rule that catches the legacy cross-origin URL needs a specific issue ID segment, so merge doesn't fire it. Result: subsequent `getIssue(id)` calls could serve stale data from the control-silo cache. Fix: after a successful merge, explicitly sweep each affected ID's legacy `/issues/{id}/` URL under `getApiBaseUrl()`. The affected IDs are only known from the response body, so this can't live in `cache-keys.ts` (URL-only logic) — it's a genuine command-layer invalidation case. Matches the old `invalidateIssueDetailCaches` behavior for per-ID legacy invalidation. Added a regression test: seed legacy cache entries for parent + children, run mergeIssues, verify all three are cleared. --- src/lib/api/issues.ts | 19 ++++++++++++ test/lib/api/issues.test.ts | 61 ++++++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/lib/api/issues.ts b/src/lib/api/issues.ts index d2ec5d347..950139ed2 100644 --- a/src/lib/api/issues.ts +++ b/src/lib/api/issues.ts @@ -12,6 +12,8 @@ import type { SentryIssue } from "../../types/index.js"; import { applyCustomHeaders } from "../custom-headers.js"; import { ApiError, ValidationError } from "../errors.js"; import { resolveOrgRegion } from "../region.js"; +import { invalidateCachedResponsesMatching } from "../response-cache.js"; +import { getApiBaseUrl } from "../sentry-client.js"; import { API_MAX_PER_PAGE, @@ -23,6 +25,8 @@ import { unwrapPaginatedResult, } from "./infrastructure.js"; +const TRAILING_SLASH_RE = /\/$/; + /** * Sort options for issue listing, derived from the @sentry/api SDK types. * Uses the SDK type directly for compile-time safety against parameter drift. @@ -614,6 +618,21 @@ export async function mergeIssues( method: "PUT", body: { merge: 1 }, }); + // HTTP-layer invalidation covers the region-scoped caches via the + // prefix sweep on `/organizations/{org}/issues/`, but it can't see + // the affected IDs (they're in query params, stripped from the + // URL the hook sees). Manually clear each affected issue's legacy + // cross-origin cache so subsequent `getIssue(id)` doesn't serve + // stale data. + const apiBase = getApiBaseUrl().replace(TRAILING_SLASH_RE, ""); + const affectedIds = data.merge.children.toSpliced(0, 0, data.merge.parent); + await Promise.all( + affectedIds.map((id) => + invalidateCachedResponsesMatching( + `${apiBase}/api/0/issues/${encodeURIComponent(id)}/` + ) + ) + ); return data.merge; } catch (error) { // The bulk-mutate endpoint returns 204 when no matching issues are diff --git a/test/lib/api/issues.test.ts b/test/lib/api/issues.test.ts index 98600a7bc..c4e52b79a 100644 --- a/test/lib/api/issues.test.ts +++ b/test/lib/api/issues.test.ts @@ -13,8 +13,13 @@ import { RESOLVE_COMMIT_SENTINEL, RESOLVE_NEXT_RELEASE_SENTINEL, } from "../../../src/lib/api-client.js"; +import { setAuthToken } from "../../../src/lib/db/auth.js"; import { ApiError, ValidationError } from "../../../src/lib/errors.js"; -import { mockFetch } from "../../helpers.js"; +import { + getCachedResponse, + storeCachedResponse, +} from "../../../src/lib/response-cache.js"; +import { mockFetch, useTestConfigDir } from "../../helpers.js"; describe("parseResolveSpec", () => { test("returns null for undefined", () => { @@ -235,3 +240,57 @@ describe("mergeIssues", () => { ); }); }); + +describe("mergeIssues: cross-origin legacy cache", () => { + useTestConfigDir("merge-legacy-cache-"); + + let originalFetch: typeof globalThis.fetch; + + beforeEach(() => { + originalFetch = globalThis.fetch; + setAuthToken("test-token", 3600, "test-refresh"); + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + test("clears legacy /issues/{id}/ cache for every affected ID", async () => { + // Regression: the HTTP-layer invalidation hook only sees + // `organizations/{org}/issues/?id=...`, so it can't clear the + // per-ID legacy caches under the control-silo origin + // (`sentry.io/api/0/issues/{id}/`) that `getIssue()` reads. + // mergeIssues must do it manually after the merge response + // identifies the affected IDs. + const legacyUrl = (id: string) => `https://sentry.io/api/0/issues/${id}/`; + + for (const id of ["100", "200", "300"]) { + await storeCachedResponse( + "GET", + legacyUrl(id), + {}, + new Response(JSON.stringify({ id }), { + status: 200, + headers: { "content-type": "application/json" }, + }) + ); + expect(await getCachedResponse("GET", legacyUrl(id), {})).toBeDefined(); + } + + globalThis.fetch = mockFetch( + async () => + new Response( + JSON.stringify({ + merge: { parent: "100", children: ["200", "300"] }, + }), + { status: 200, headers: { "Content-Type": "application/json" } } + ) + ); + + await mergeIssues("test-org", ["100", "200", "300"]); + + for (const id of ["100", "200", "300"]) { + expect(await getCachedResponse("GET", legacyUrl(id), {})).toBeUndefined(); + } + }); +});