refactor(cache): cap hierarchy walk at owner level + drop defensive catch

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.

Author: BYK

src/lib/cache-keys.ts

@@ -125,22 +125,35 @@ 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<string> {
   const trimmed = relPath.endsWith("/") ? relPath.slice(0, -1) : relPath;
   if (trimmed === "") {
     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("/")}/`;
   }
 }

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;
     }

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,22 +76,23 @@ 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", () => {
     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/`);
+    // 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/"
     );
   });
 });