fix(jira): reactive token refresh + retry on 401 feedback-comment POST (#370) (#375)

* fix(jira): reactive token refresh + retry on 401 feedback-comment POST (#370)

The Jira webhook processor's feedback-comment POST could 401 even though
the gateway base (api.atlassian.com/ex/jira/<cloudId>) and proactive,
expiry-based token refresh were already in place. Root cause: refresh was
*proactive only*. A token can be rejected with 401/403 before its stored
`expires_at` — server-side revocation, a re-issued token after a scope
change, or a value still inside the 60s in-memory cache after an
out-of-band rotation. In those cases `postComment` logged
"Jira feedback REST non-2xx", status: 401 and gave up, so every upstream
failure (REPO_NOT_ONBOARDED, project not onboarded, user not linked)
became silent to the operator — the feedback comment is the only
operator-visible signal.

Fix:
- `resolveJiraOauthToken` gains a `forceRefresh` option that bypasses both
  the in-memory token cache and the proactive-expiry short-circuit, minting
  a guaranteed-fresh token regardless of `expires_at`.
- `postComment` now reports an outcome ('ok' | 'auth' | 'error') so the
  caller can distinguish a recoverable auth rejection from a terminal one.
- `postIssueComment` reacts to a 401/403 with exactly one forced refresh +
  one retry. The retry is bounded (a second 401 means the credential is
  genuinely unusable), and is skipped when the refresh returns an unchanged
  token (refresh-token revoked → retry would only 401 again).

Best-effort/never-throws semantics are preserved. Adds coverage for the
forceRefresh resolver path and the 401→refresh→retry feedback path
(success, 403, unchanged-token, refresh-fails, second-401).

* test(jira): cover resolveTenantToken catch path (codecov #375)

Codecov flagged 5 uncovered lines on PR #375 — the catch block in
resolveTenantToken (jira-feedback.ts), which the existing tests never
exercised (they only made the resolver resolve to a value or null, never
throw). Add two tests that make resolveJiraOauthToken throw: once on the
initial resolve and once on the forced-refresh retry after a 401, covering
both the force_refresh:false and force_refresh:true log branches. Brings
jira-feedback.ts to 100% line coverage.

---------

Co-authored-by: bgagent <bgagent@noreply.github.com>

Author: mayakost

cdk/src/handlers/shared/jira-feedback.ts

@@ -66,12 +66,19 @@ function toAdfDocument(message: string): Record<string, unknown> {
   };
 }
 
+/**
+ * Outcome of a single comment POST. We distinguish auth rejection (401/403)
+ * from other failures so the caller can react to the former with a forced
+ * token refresh + retry, and treat the latter as terminal.
+ */
+type PostOutcome = 'ok' | 'auth' | 'error';
+
 async function postComment(
   accessToken: string,
   cloudId: string,
   issueIdOrKey: string,
   message: string,
-): Promise<boolean> {
+): Promise<PostOutcome> {
   // The 3LO token (audience=api.atlassian.com) is only valid against the
   // gateway base scoped by cloudId — see JIRA_API_BASE. Posting to the raw
   // site host (`*.atlassian.net`) would 401. Both path segments are
@@ -92,17 +99,20 @@ async function postComment(
       body: JSON.stringify({ body: toAdfDocument(message) }),
       signal: controller.signal,
     });
-    if (!resp.ok) {
-      logger.warn('Jira feedback REST non-2xx', { status: resp.status, url });
-      return false;
-    }
-    return true;
+    if (resp.ok) return 'ok';
+    // 401/403 are recoverable: the stored access token may be dead despite a
+    // not-yet-reached `expires_at` (server-side revocation, scope re-issue, or
+    // a value cached past its out-of-band rotation). Signal the caller to
+    // force-refresh and retry once. Any other non-2xx is terminal.
+    const outcome: PostOutcome = resp.status === 401 || resp.status === 403 ? 'auth' : 'error';
+    logger.warn('Jira feedback REST non-2xx', { status: resp.status, url, outcome });
+    return outcome;
   } catch (err) {
     logger.warn('Jira feedback request failed', {
       error: err instanceof Error ? err.message : String(err),
       url,
     });
-    return false;
+    return 'error';
   } finally {
     clearTimeout(timer);
   }
@@ -123,14 +133,20 @@ export interface JiraFeedbackContext {
 
 async function resolveTenantToken(
   ctx: JiraFeedbackContext,
+  forceRefresh = false,
 ): Promise<{ accessToken: string } | null> {
   try {
-    const resolved = await resolveJiraOauthToken(ctx.cloudId, ctx.registryTableName);
+    const resolved = await resolveJiraOauthToken(ctx.cloudId, ctx.registryTableName, { forceRefresh });
     if (!resolved) return null;
     return { accessToken: resolved.accessToken };
   } catch (err) {
+    // `force_refresh` discriminates the initial resolve from the post-401
+    // retry resolve: a failure here on the retry is an infra error (DDB/SM),
+    // distinct from "refresh-token revoked", and triage needs to tell them
+    // apart.
     logger.warn('Jira feedback could not resolve OAuth token', {
       jira_cloud_id: ctx.cloudId,
+      force_refresh: forceRefresh,
       error: err instanceof Error ? err.message : String(err),
     });
     return null;
@@ -141,6 +157,17 @@ async function resolveTenantToken(
  * Post a comment onto a Jira issue. Returns true on success, false on any
  * failure (network, auth, REST errors). Never throws — callers proceed
  * regardless.
+ *
+ * Auth resilience: the access token from the resolver can already be stale
+ * (cached within its TTL, or revoked/re-issued server-side before its
+ * advertised `expires_at`). The proactive expiry check can't catch those, so
+ * a 401/403 on the first POST triggers exactly one forced token refresh
+ * (`forceRefresh: true`) and one retry. This is the path that makes feedback
+ * comments — the only operator-visible failure signal — actually land after a
+ * token goes bad, rather than silently 401ing (issue #370). The retry is
+ * bounded at one attempt: a second 401 means the credential is genuinely
+ * unusable (refresh-token revoked, scope removed), so we stop and let the
+ * caller no-op.
  */
 export async function postIssueComment(
   ctx: JiraFeedbackContext,
@@ -149,7 +176,30 @@ export async function postIssueComment(
 ): Promise<boolean> {
   const resolved = await resolveTenantToken(ctx);
   if (!resolved) return false;
-  return postComment(resolved.accessToken, ctx.cloudId, issueIdOrKey, body);
+
+  const outcome = await postComment(resolved.accessToken, ctx.cloudId, issueIdOrKey, body);
+  if (outcome === 'ok') return true;
+  if (outcome === 'error') return false;
+
+  // outcome === 'auth': the stored access token was rejected. Force a refresh
+  // (bypassing the resolver's cache and proactive-expiry short-circuit) and
+  // retry once with the freshly-minted token.
+  logger.info('Jira feedback got auth rejection — forcing token refresh and retrying once', {
+    jira_cloud_id: ctx.cloudId,
+    issue_id_or_key: issueIdOrKey,
+  });
+  const refreshed = await resolveTenantToken(ctx, true);
+  if (!refreshed) return false;
+  // If the refresh handed back the same access token, the retry can only
+  // reproduce the 401 — skip the redundant network call.
+  if (refreshed.accessToken === resolved.accessToken) {
+    logger.warn('Jira feedback refresh returned an unchanged token — not retrying', {
+      jira_cloud_id: ctx.cloudId,
+      issue_id_or_key: issueIdOrKey,
+    });
+    return false;
+  }
+  return (await postComment(refreshed.accessToken, ctx.cloudId, issueIdOrKey, body)) === 'ok';
 }
 
 /**

cdk/src/handlers/shared/jira-oauth-resolver.ts

@@ -97,6 +97,21 @@ export interface ResolverOptions {
   readonly dynamoDbClient?: DynamoDBDocumentClient;
   /** Override fetch for token-endpoint refresh in tests. */
   readonly fetchImpl?: typeof fetch;
+  /**
+   * Force a token refresh even when the stored `expires_at` claims the token
+   * is still valid. Used by the reactive-401 retry path: Atlassian can reject
+   * an access token (401) before its advertised expiry — server-side
+   * invalidation, a re-issued token after a scope change, or a token cached
+   * within {@link SECRET_CACHE_TTL_MS} that was rotated out-of-band. The
+   * proactive expiry check can't see those, so on a 401 the caller re-resolves
+   * with `forceRefresh: true` to bypass the in-memory *token* cache and the
+   * expiry short-circuit and mint a guaranteed-fresh token before retrying.
+   *
+   * The registry-row cache is intentionally NOT bypassed: a revoked access
+   * token does not change the row (`site_url`, `oauth_secret_arn`), so forcing
+   * a DDB read on every 401 would add load without changing the outcome.
+   */
+  readonly forceRefresh?: boolean;
 }
 
 interface CacheEntry<T> {
@@ -137,6 +152,10 @@ export interface ResolvedJiraToken {
  * Refreshes silently if the cached token is expiring. Returns null on any
  * failure (registry miss, secret missing, refresh-token revoked) so callers
  * can gracefully no-op rather than blowing up.
+ *
+ * Pass `options.forceRefresh` to mint a guaranteed-fresh token even when the
+ * stored `expires_at` looks valid — see {@link ResolverOptions.forceRefresh}
+ * (the reactive-401 retry path).
  */
 export async function resolveJiraOauthToken(
   cloudId: string,
@@ -146,6 +165,7 @@ export async function resolveJiraOauthToken(
   const region = options.region ?? process.env.AWS_REGION ?? 'us-east-1';
   const ddb = options.dynamoDbClient ?? DynamoDBDocumentClient.from(new DynamoDBClient({ region }));
   const sm = options.secretsManagerClient ?? new SecretsManagerClient({ region });
+  const forceRefresh = options.forceRefresh ?? false;
 
   // ─── Step 1: Registry row ────────────────────────────────────────
   const row = await getRegistryRow(ddb, registryTableName, cloudId);
@@ -162,7 +182,11 @@ export async function resolveJiraOauthToken(
   }
 
   // ─── Step 2: Cached or fresh token JSON ──────────────────────────
-  const cached = tokenCache.get(row.oauth_secret_arn);
+  // `forceRefresh` bypasses the in-memory token cache: the cached access token
+  // is the one a caller just saw 401, so we must re-read the secret (and
+  // refresh below) rather than hand the same dead token back. (The registry
+  // row above still uses its own cache — see ResolverOptions.forceRefresh.)
+  const cached = forceRefresh ? undefined : tokenCache.get(row.oauth_secret_arn);
   let token: StoredOauthToken;
   if (cached && cached.expiresAt > Date.now() && !isTokenExpiring(cached.value.expires_at)) {
     token = cached.value;
@@ -178,8 +202,11 @@ export async function resolveJiraOauthToken(
     token = fetched;
   }
 
-  // ─── Step 3: Refresh if expiring ─────────────────────────────────
-  if (isTokenExpiring(token.expires_at)) {
+  // ─── Step 3: Refresh if expiring (or forced) ─────────────────────
+  // `forceRefresh` refreshes regardless of `expires_at`: a 401 on a
+  // not-yet-expired token means the stored access token is dead despite the
+  // timestamp, so trust the 401 over the clock and rotate.
+  if (forceRefresh || isTokenExpiring(token.expires_at)) {
     const refreshed = await refreshJiraToken(token, sm, row.oauth_secret_arn, options);
     if (!refreshed) {
       return null;

cdk/test/handlers/shared/jira-feedback.test.ts

@@ -103,12 +103,48 @@ describe('jira-feedback: postIssueComment', () => {
     expect(fetchMock).not.toHaveBeenCalled();
   });
 
-  test('returns false on a non-2xx response and swallows the error', async () => {
-    global.fetch = jest.fn().mockResolvedValue(mockResponse(401)) as unknown as typeof fetch;
+  test('returns false (never throws) when the initial resolve throws an infra error', async () => {
+    // The resolver can throw on DDB/SM failure (not just resolve null). The
+    // catch in resolveTenantToken must swallow it and no-op.
+    resolveJiraOauthTokenMock.mockRejectedValueOnce(new Error('DDB throttle'));
+    const fetchMock = jest.fn();
+    global.fetch = fetchMock as unknown as typeof fetch;
+
+    const ok = await postIssueComment(CTX, 'ENG-42', 'hello');
+
+    expect(ok).toBe(false);
+    expect(fetchMock).not.toHaveBeenCalled();
+  });
+
+  test('returns false when the forced-refresh resolve throws an infra error after a 401', async () => {
+    // First resolve succeeds; the POST 401s; the forced-refresh resolve then
+    // throws (SM unavailable). Must swallow and no-op — never throw.
+    resolveJiraOauthTokenMock
+      .mockResolvedValueOnce({ accessToken: 'stale_token', scope: '', siteUrl: '', oauthSecretArn: 'x' })
+      .mockRejectedValueOnce(new Error('SecretsManager unavailable'));
+    const fetchMock = jest.fn().mockResolvedValueOnce(mockResponse(401));
+    global.fetch = fetchMock as unknown as typeof fetch;
+
+    const ok = await postIssueComment(CTX, 'ENG-42', 'hello');
+
+    expect(ok).toBe(false);
+    // Only the first POST happened; the retry never got a token.
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+    expect(resolveJiraOauthTokenMock).toHaveBeenCalledTimes(2);
+    // The forced-refresh resolve carried forceRefresh: true.
+    expect(resolveJiraOauthTokenMock.mock.calls[1][2]).toEqual({ forceRefresh: true });
+  });
+
+  test('returns false on a non-auth non-2xx response and does NOT refresh', async () => {
+    const fetchMock = jest.fn().mockResolvedValue(mockResponse(500));
+    global.fetch = fetchMock as unknown as typeof fetch;
 
     const ok = await postIssueComment(CTX, 'ENG-42', 'hello');
 
     expect(ok).toBe(false);
+    // 5xx is terminal — no forced-refresh retry, no second POST.
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+    expect(resolveJiraOauthTokenMock).toHaveBeenCalledTimes(1);
   });
 
   test('reportIssueFailure never throws even when fetch rejects', async () => {
@@ -119,3 +155,103 @@ describe('jira-feedback: postIssueComment', () => {
     await expect(reportIssueFailure(CTX, 'ENG-42', '❌ nope')).resolves.toBeUndefined();
   });
 });
+
+describe('jira-feedback: 401 → forced refresh → retry (issue #370)', () => {
+  test('refreshes the token and retries once on 401, succeeding the second time', async () => {
+    // First resolve hands back a (stale) token; the forced-refresh resolve
+    // hands back a different, freshly-minted token.
+    resolveJiraOauthTokenMock
+      .mockResolvedValueOnce({
+        accessToken: 'stale_token',
+        scope: 'write:jira-work',
+        siteUrl: 'https://acme.atlassian.net',
+        oauthSecretArn: 'arn:aws:secretsmanager:us-east-1:111:secret:x',
+      })
+      .mockResolvedValueOnce({
+        accessToken: 'fresh_token',
+        scope: 'write:jira-work',
+        siteUrl: 'https://acme.atlassian.net',
+        oauthSecretArn: 'arn:aws:secretsmanager:us-east-1:111:secret:x',
+      });
+    const fetchMock = jest
+      .fn()
+      .mockResolvedValueOnce(mockResponse(401)) // first POST: stale token rejected
+      .mockResolvedValueOnce(mockResponse(201)); // retry: fresh token accepted
+    global.fetch = fetchMock as unknown as typeof fetch;
+
+    const ok = await postIssueComment(CTX, 'ENG-42', 'hello');
+
+    expect(ok).toBe(true);
+    // Two POSTs, and the second carried the refreshed bearer token.
+    expect(fetchMock).toHaveBeenCalledTimes(2);
+    const firstHeaders = (fetchMock.mock.calls[0][1] as RequestInit).headers as Record<string, string>;
+    const secondHeaders = (fetchMock.mock.calls[1][1] as RequestInit).headers as Record<string, string>;
+    expect(firstHeaders.Authorization).toBe('Bearer stale_token');
+    expect(secondHeaders.Authorization).toBe('Bearer fresh_token');
+    // The forced-refresh resolve must pass forceRefresh: true.
+    expect(resolveJiraOauthTokenMock).toHaveBeenCalledTimes(2);
+    expect(resolveJiraOauthTokenMock.mock.calls[0][2]).toEqual({ forceRefresh: false });
+    expect(resolveJiraOauthTokenMock.mock.calls[1][2]).toEqual({ forceRefresh: true });
+  });
+
+  test('also retries on 403 (insufficient-permission style auth rejection)', async () => {
+    resolveJiraOauthTokenMock
+      .mockResolvedValueOnce({ accessToken: 'stale_token', scope: '', siteUrl: '', oauthSecretArn: 'x' })
+      .mockResolvedValueOnce({ accessToken: 'fresh_token', scope: '', siteUrl: '', oauthSecretArn: 'x' });
+    const fetchMock = jest
+      .fn()
+      .mockResolvedValueOnce(mockResponse(403))
+      .mockResolvedValueOnce(mockResponse(201));
+    global.fetch = fetchMock as unknown as typeof fetch;
+
+    const ok = await postIssueComment(CTX, 'ENG-42', 'hello');
+
+    expect(ok).toBe(true);
+    expect(fetchMock).toHaveBeenCalledTimes(2);
+  });
+
+  test('returns false without a second POST when the refresh yields the same token', async () => {
+    // Both resolves return the identical access token (e.g. refresh-token
+    // revoked, so the resolver couldn't rotate). Retrying would only 401
+    // again, so we skip it.
+    const fetchMock = jest.fn().mockResolvedValue(mockResponse(401));
+    global.fetch = fetchMock as unknown as typeof fetch;
+
+    const ok = await postIssueComment(CTX, 'ENG-42', 'hello');
+
+    expect(ok).toBe(false);
+    expect(fetchMock).toHaveBeenCalledTimes(1); // no retry with an unchanged token
+    expect(resolveJiraOauthTokenMock).toHaveBeenCalledTimes(2);
+  });
+
+  test('returns false when the forced refresh cannot resolve a token', async () => {
+    resolveJiraOauthTokenMock
+      .mockResolvedValueOnce({ accessToken: 'stale_token', scope: '', siteUrl: '', oauthSecretArn: 'x' })
+      .mockResolvedValueOnce(null); // refresh-token revoked → resolver gives up
+    const fetchMock = jest.fn().mockResolvedValueOnce(mockResponse(401));
+    global.fetch = fetchMock as unknown as typeof fetch;
+
+    const ok = await postIssueComment(CTX, 'ENG-42', 'hello');
+
+    expect(ok).toBe(false);
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+  });
+
+  test('does not retry a second time if the refreshed token is also rejected', async () => {
+    resolveJiraOauthTokenMock
+      .mockResolvedValueOnce({ accessToken: 'stale_token', scope: '', siteUrl: '', oauthSecretArn: 'x' })
+      .mockResolvedValueOnce({ accessToken: 'fresh_token', scope: '', siteUrl: '', oauthSecretArn: 'x' });
+    const fetchMock = jest
+      .fn()
+      .mockResolvedValueOnce(mockResponse(401)) // stale rejected
+      .mockResolvedValueOnce(mockResponse(401)); // fresh also rejected → give up
+    global.fetch = fetchMock as unknown as typeof fetch;
+
+    const ok = await postIssueComment(CTX, 'ENG-42', 'hello');
+
+    expect(ok).toBe(false);
+    // Exactly two POSTs — the retry is bounded at one attempt.
+    expect(fetchMock).toHaveBeenCalledTimes(2);
+    expect(resolveJiraOauthTokenMock).toHaveBeenCalledTimes(2);
+  });
+});