fix: resolve code review findings for PR #658

P1 — Non-2xx cache responses bypassed fallback (authz.ts + fga.ts):
  `fetch` resolves (does not throw) for 4xx/5xx responses, so the
  existing `catch` block never triggered on HTTP errors. Added an
  explicit `response.ok` guard — the fallback to `httpClient.post`
  now fires for any non-2xx response from the cache, not just network-
  level exceptions. Fixed in both authz.ts and fga.ts.

P1 — Timer leak: `clearTimeout` missing in error path (authz.ts + fga.ts):
  Moved `const controller/timeoutId` before the try block and moved
  `clearTimeout` into a `finally` clause so the timer is always
  cancelled regardless of success, HTTP error, or exception. Fixed in
  both authz.ts and fga.ts.

P3 — Missing tests for non-2xx cache response fallback (authz.test.ts):
  Added two new test cases covering the scenario where `fetch` resolves
  with `{ ok: false, status: 503 }` for `whoCanAccess` and
  `whatCanTargetAccess`. These tests would have caught the P1 bug.

Dismissed:
  P3 code duplication (postWithOptionalCache identical in authz.ts and
  fga.ts): intentional pattern match per PR description; extraction to
  a shared helper is a follow-up refactor.

Verification: 309 tests pass, lint clean, build successful.

Author: orius123

lib/management/authz.test.ts

@@ -685,6 +685,54 @@ describe('Management Authz', () => {
       expect(fetchMock).not.toHaveBeenCalled();
     });
 
+    it('should fallback to httpClient when cache returns non-OK response for whoCanAccess', async () => {
+      const targets = { targets: ['u1'] };
+      const httpResponse = {
+        ok: true,
+        json: () => targets,
+        clone: () => ({
+          json: () => Promise.resolve(targets),
+        }),
+        status: 200,
+      };
+      mockHttpClient.post.mockResolvedValue(httpResponse);
+      fetchMock.mockResolvedValue({ ok: false, status: 503, json: async () => ({}) });
+
+      const managementWithCache = withManagement(mockHttpClient, fgaConfig);
+      const resp = await managementWithCache.authz.whoCanAccess('r', 'owner', 'doc');
+
+      expect(fetchMock).toHaveBeenCalled();
+      expect(mockHttpClient.post).toHaveBeenCalledWith(apiPaths.authz.who, {
+        resource: 'r',
+        relationDefinition: 'owner',
+        namespace: 'doc',
+      });
+      expect(resp.data).toEqual(['u1']);
+    });
+
+    it('should fallback to httpClient when cache returns non-OK response for whatCanTargetAccess', async () => {
+      const relationsResponse = { relations: [mockRelation] };
+      const httpResponse = {
+        ok: true,
+        json: () => relationsResponse,
+        clone: () => ({
+          json: () => Promise.resolve(relationsResponse),
+        }),
+        status: 200,
+      };
+      mockHttpClient.post.mockResolvedValue(httpResponse);
+      fetchMock.mockResolvedValue({ ok: false, status: 503, json: async () => ({}) });
+
+      const managementWithCache = withManagement(mockHttpClient, fgaConfig);
+      const resp = await managementWithCache.authz.whatCanTargetAccess('t');
+
+      expect(fetchMock).toHaveBeenCalled();
+      expect(mockHttpClient.post).toHaveBeenCalledWith(apiPaths.authz.targetAll, {
+        target: 't',
+      });
+      expect(resp.data).toEqual([mockRelation]);
+    });
+
     it('should fallback to httpClient when cache URL fetch fails for whoCanAccess', async () => {
       const targets = { targets: ['u1'] };
       const httpResponse = {

lib/management/authz.ts

@@ -18,11 +18,10 @@ const WithAuthz = (httpClient: HttpClient, config?: FGAConfig) => {
   const postWithOptionalCache = async (path: string, body: unknown): Promise<Response> => {
     if (config?.fgaCacheUrl && config.managementKey) {
       const url = `${config.fgaCacheUrl}${path}`;
+      const controller = new AbortController();
+      const timeoutId = setTimeout(() => controller.abort(), DEFAULT_CACHE_TIMEOUT_MS);
 
       try {
-        const controller = new AbortController();
-        const timeoutId = setTimeout(() => controller.abort(), DEFAULT_CACHE_TIMEOUT_MS);
-
         const response = await fetch(url, {
           method: 'POST',
           headers: {
@@ -35,10 +34,13 @@ const WithAuthz = (httpClient: HttpClient, config?: FGAConfig) => {
           signal: controller.signal,
         });
 
-        clearTimeout(timeoutId);
-        return response;
+        if (response.ok) {
+          return response;
+        }
       } catch {
-        return httpClient.post(path, body);
+        // Cache request failed (network error or timeout); fall back to origin
+      } finally {
+        clearTimeout(timeoutId);
       }
     }
     return httpClient.post(path, body);

lib/management/fga.ts

@@ -16,11 +16,10 @@ const WithFGA = (httpClient: HttpClient, config?: FGAConfig) => {
   const postWithOptionalCache = async (path: string, body: unknown): Promise<Response> => {
     if (config?.fgaCacheUrl && config.managementKey) {
       const url = `${config.fgaCacheUrl}${path}`;
+      const controller = new AbortController();
+      const timeoutId = setTimeout(() => controller.abort(), DEFAULT_CACHE_TIMEOUT_MS);
 
       try {
-        const controller = new AbortController();
-        const timeoutId = setTimeout(() => controller.abort(), DEFAULT_CACHE_TIMEOUT_MS);
-
         const response = await fetch(url, {
           method: 'POST',
           headers: {
@@ -33,10 +32,13 @@ const WithFGA = (httpClient: HttpClient, config?: FGAConfig) => {
           signal: controller.signal,
         });
 
-        clearTimeout(timeoutId);
-        return response;
+        if (response.ok) {
+          return response;
+        }
       } catch {
-        return httpClient.post(path, body);
+        // Cache request failed (network error or timeout); fall back to origin
+      } finally {
+        clearTimeout(timeoutId);
       }
     }
     return httpClient.post(path, body);