fix(atlassian): key discovery answers by credential and retry timeouts

Review round 1. Three fixes.

The cache keyed on the normalized domain alone, so a caller joining a lookup
already in flight inherited whichever credential started it — taking that token's
authorization failure, or its single-site fallback pointing at a different site.
Retaining only exact matches closed that for settled entries but not for the
in-flight window, which is where it actually bites. Keys now carry a digest of
the access token, so an answer is only ever reused by the credential that earned
it. That also removes the reason the cache needed a `retain` channel.

The request's own `AbortSignal.timeout` rejects with a `TimeoutError` that has no
status and no message the shared predicate matches, so a slow site failed on the
first attempt despite the retry budget. It is now explicitly retryable — only
`TimeoutError`, since an `AbortError` means a caller cancelled — and the per-
request timeout drops to 5s so four attempts stay bounded.

Jira bulk read had been pointed at the cached resolver, but the tool's own
configured request IS the discovery call and `transformResponse` only runs on a
2xx. It was therefore re-issuing a request whose answer it already held. It now
matches against that payload through the shared selector, so the matching logic
stays in one place without a second round trip.

Author: waleedlatif1

apps/sim/lib/atlassian/discovery.test.ts

@@ -142,14 +142,45 @@ describe('resolveAtlassianCloudId', () => {
     ).rejects.toThrow(/Failed to fetch Confluence accessible resources: 500/)
   })
 
-  it('does not retain a single-site fallback, since it is token-specific', async () => {
-    fetchMock.mockImplementation(async () =>
-      sites([{ id: 'other-cloud', url: 'https://other.atlassian.net' }])
+  it('does not serve one credential answer to another', async () => {
+    fetchMock
+      .mockResolvedValueOnce(sites([{ id: 'token-a-cloud', url: SITE }]))
+      .mockResolvedValueOnce(sites([{ id: 'token-b-cloud', url: SITE }]))
+
+    await expect(resolveAtlassianCloudId(options({ accessToken: 'a' }))).resolves.toBe(
+      'token-a-cloud'
     )
+    await expect(resolveAtlassianCloudId(options({ accessToken: 'b' }))).resolves.toBe(
+      'token-b-cloud'
+    )
+    expect(fetchMock).toHaveBeenCalledTimes(2)
+  })
 
-    await expect(resolveAtlassianCloudId(options())).resolves.toBe('other-cloud')
-    await resolveAtlassianCloudId(options())
+  it('does not let a concurrent caller inherit another credential lookup', async () => {
+    // Token A sees only a different site, so it falls back; token B matches exactly.
+    // Joining A's in-flight promise would hand B the wrong site.
+    fetchMock
+      .mockResolvedValueOnce(sites([{ id: 'a-only-cloud', url: 'https://other.atlassian.net' }]))
+      .mockResolvedValueOnce(sites([{ id: CLOUD_ID, url: SITE }]))
+
+    const [a, b] = await Promise.all([
+      resolveAtlassianCloudId(options({ accessToken: 'a' })),
+      resolveAtlassianCloudId(options({ accessToken: 'b' })),
+    ])
+
+    expect(a).toBe('a-only-cloud')
+    expect(b).toBe(CLOUD_ID)
+    expect(fetchMock).toHaveBeenCalledTimes(2)
+  })
+
+  it('retries a request that timed out', async () => {
+    fetchMock
+      .mockRejectedValueOnce(
+        Object.assign(new Error('The operation timed out.'), { name: 'TimeoutError' })
+      )
+      .mockResolvedValueOnce(sites([{ id: CLOUD_ID, url: SITE }]))
 
+    await expect(resolveAtlassianCloudId(options({ retryOptions: FAST }))).resolves.toBe(CLOUD_ID)
     expect(fetchMock).toHaveBeenCalledTimes(2)
   })
 

apps/sim/lib/atlassian/discovery.ts

@@ -1,3 +1,4 @@
+import { sha256Hex } from '@sim/security/hash'
 import { parseRetryAfter } from '@sim/utils/retry'
 import { LRUCache } from 'lru-cache'
 import {
@@ -9,7 +10,7 @@ import {
 
 const ACCESSIBLE_RESOURCES_URL = 'https://api.atlassian.com/oauth/token/accessible-resources'
 
-const DISCOVERY_REQUEST_TIMEOUT_MS = 10_000
+const DISCOVERY_REQUEST_TIMEOUT_MS = 5_000
 
 /**
  * A site's `cloudId` is a property of the site rather than of the caller — the
@@ -41,25 +42,20 @@ export function createAtlassianDiscoveryCache() {
      *
      * The promise — not the resolved value — is stored, so callers arriving while
      * a lookup is in flight join it instead of starting their own, and a rejection
-     * is evicted rather than pinned for the TTL. `retain: false` drops the entry
-     * once it settles, so a token-specific result is not served to later callers.
+     * is evicted rather than pinned for the TTL.
+     *
+     * `key` must identify the credential as well as the resource — a joined caller
+     * receives the first caller's outcome, so a domain-only key would let one
+     * token's authorization failure or single-site fallback answer another's.
      */
-    resolve(
-      key: string,
-      resolver: () => Promise<{ value: string; retain: boolean }>
-    ): Promise<string> {
+    resolve(key: string, resolver: () => Promise<string>): Promise<string> {
       const cached = cache.get(key)
       if (cached) return cached
 
-      const promise = resolver()
-        .then(({ value, retain }) => {
-          if (!retain) cache.delete(key)
-          return value
-        })
-        .catch((error) => {
-          cache.delete(key)
-          throw error
-        })
+      const promise = resolver().catch((error) => {
+        cache.delete(key)
+        throw error
+      })
 
       cache.set(key, promise)
       return promise
@@ -91,6 +87,11 @@ export const ATLASSIAN_DISCOVERY_RETRY_OPTIONS: RetryOptions = {
   maxDelayMs: 8000,
   retryCondition: (error) => {
     if (isRetryableError(error)) return true
+    // The request's own `AbortSignal.timeout` rejects with a `TimeoutError` that
+    // carries no status and no message the shared predicate matches, so without
+    // this a slow site would fail on the first attempt. Only `TimeoutError` — an
+    // `AbortError` would mean a caller cancelled and does not want a replay.
+    if (error instanceof Error && error.name === 'TimeoutError') return true
     const status = (error as { status?: unknown } | null)?.status
     return typeof status === 'number' && status >= 500
   },
@@ -181,34 +182,47 @@ function fetchAccessibleResources(
   )
 }
 
-async function discoverCloudId(
-  siteUrl: string,
-  { domain, accessToken, product, retryOptions }: ResolveAtlassianCloudIdOptions
-): Promise<{ value: string; retain: boolean }> {
-  const resources = await fetchAccessibleResources(accessToken, product, retryOptions)
+/**
+ * Cache key for a discovery answer, scoped to the credential that produced it.
+ *
+ * What `accessible-resources` reports depends on the token, so an answer is only
+ * reusable by the same token. The digest keeps the raw token out of the key.
+ */
+export function atlassianDiscoveryKey(resource: string, accessToken: string): string {
+  return `${resource}:${sha256Hex(accessToken).slice(0, 16)}`
+}
 
+/**
+ * Picks the `cloudId` for `domain` out of an `accessible-resources` payload.
+ *
+ * Separate from the fetch so a caller that already holds the payload can match
+ * against it instead of issuing the request a second time.
+ */
+export function selectAtlassianCloudId(
+  resources: unknown,
+  domain: string,
+  product: string
+): string {
   if (!Array.isArray(resources) || resources.length === 0) {
     throw new Error(`No ${product} resources found`)
   }
 
-  const match = resources.find((r) => normalizeAtlassianSiteUrl(r.url) === siteUrl)
-  if (match) {
-    return { value: match.id, retain: true }
-  }
+  const siteUrl = normalizeAtlassianSiteUrl(domain)
+  const match = (resources as AccessibleResource[]).find(
+    (r) => normalizeAtlassianSiteUrl(r.url) === siteUrl
+  )
+  if (match) return match.id
 
-  // A single-site fallback is a property of this token, not of the domain.
-  if (resources.length === 1) {
-    return { value: resources[0].id, retain: false }
-  }
+  if (resources.length === 1) return (resources as AccessibleResource[])[0].id
 
   throw new Error(
     `Could not match ${product} domain "${domain}" to any accessible resource. ` +
-      `Available sites: ${resources.map((r) => r.url).join(', ')}`
+      `Available sites: ${(resources as AccessibleResource[]).map((r) => r.url).join(', ')}`
   )
 }
 
 /**
- * Resolves the Atlassian `cloudId` for a site domain, memoized across callers.
+ * Resolves the Atlassian `cloudId` for a site domain, memoized per credential.
  *
  * Jira, Confluence, and JSM all read the same `accessible-resources` endpoint, so
  * a run touching several Atlassian blocks shares one round trip and one retry
@@ -217,8 +231,16 @@ async function discoverCloudId(
 export async function resolveAtlassianCloudId(
   options: ResolveAtlassianCloudIdOptions
 ): Promise<string> {
-  const siteUrl = normalizeAtlassianSiteUrl(options.domain)
-  return cloudIdCache.resolve(siteUrl, () => discoverCloudId(siteUrl, options))
+  const { domain, accessToken, product, retryOptions } = options
+  const key = atlassianDiscoveryKey(normalizeAtlassianSiteUrl(domain), accessToken)
+
+  return cloudIdCache.resolve(key, async () =>
+    selectAtlassianCloudId(
+      await fetchAccessibleResources(accessToken, product, retryOptions),
+      domain,
+      product
+    )
+  )
 }
 
 /** Drops every memoized `cloudId`. Exists for tests. */

apps/sim/tools/jira/bulk_read.ts

@@ -1,6 +1,7 @@
+import { selectAtlassianCloudId } from '@/lib/atlassian/discovery'
 import type { JiraRetrieveBulkParams, JiraRetrieveResponseBulk } from '@/tools/jira/types'
 import { TIMESTAMP_OUTPUT } from '@/tools/jira/types'
-import { extractAdfText, getJiraCloudId } from '@/tools/jira/utils'
+import { extractAdfText } from '@/tools/jira/utils'
 import type { ToolConfig } from '@/tools/types'
 
 export const jiraBulkRetrieveTool: ToolConfig<JiraRetrieveBulkParams, JiraRetrieveResponseBulk> = {
@@ -51,7 +52,7 @@ export const jiraBulkRetrieveTool: ToolConfig<JiraRetrieveBulkParams, JiraRetrie
     }),
   },
 
-  transformResponse: async (_response: Response, params?: JiraRetrieveBulkParams) => {
+  transformResponse: async (response: Response, params?: JiraRetrieveBulkParams) => {
     const MAX_TOTAL = 1000
     const PAGE_SIZE = 100
 
@@ -68,8 +69,11 @@ export const jiraBulkRetrieveTool: ToolConfig<JiraRetrieveBulkParams, JiraRetrie
       return project?.key || refTrimmed
     }
 
+    // The dispatcher's configured request IS the discovery call, and it only
+    // reaches here on a 2xx — so match against that payload rather than issuing
+    // the same request again through the cached resolver.
     const cloudId =
-      params?.cloudId ?? (await getJiraCloudId(params?.domain ?? '', params!.accessToken))
+      params?.cloudId ?? selectAtlassianCloudId(await response.json(), params?.domain ?? '', 'Jira')
     const projectKey = await resolveProjectKey(cloudId, params!.accessToken, params!.projectId)
     if (!/^[A-Za-z][A-Za-z0-9_]*$/.test(projectKey)) {
       throw new Error(

apps/sim/tools/jsm/utils.ts

@@ -3,6 +3,7 @@
  */
 
 import {
+  atlassianDiscoveryKey,
   createAtlassianDiscoveryCache,
   fetchAtlassianDiscoveryJson,
 } from '@/lib/atlassian/discovery'
@@ -11,7 +12,8 @@ import type { AssetObject, RawAssetObject } from '@/tools/jsm/types'
 
 /**
  * Atlassian provisions a single Assets workspace per site, so the answer is a
- * property of the `cloudId` and safe to share across callers.
+ * property of the `cloudId` — but whether a token may read it is not, so entries
+ * are keyed by credential like every other discovery answer.
  */
 const assetsWorkspaceCache = createAtlassianDiscoveryCache()
 
@@ -124,7 +126,7 @@ export function getAssetsApiBaseUrl(cloudId: string, workspaceId: string): strin
  * @throws If discovery fails or no workspace is provisioned
  */
 export function getAssetsWorkspaceId(cloudId: string, accessToken: string): Promise<string> {
-  return assetsWorkspaceCache.resolve(cloudId, async () => {
+  return assetsWorkspaceCache.resolve(atlassianDiscoveryKey(cloudId, accessToken), async () => {
     const data = await fetchAtlassianDiscoveryJson<{ values?: Array<{ workspaceId?: string }> }>(
       `https://api.atlassian.com/ex/jira/${cloudId}/rest/servicedeskapi/assets/workspace`,
       getJsmHeaders(accessToken),
@@ -139,6 +141,6 @@ export function getAssetsWorkspaceId(cloudId: string, accessToken: string): Prom
       )
     }
 
-    return { value: workspaceId, retain: true }
+    return workspaceId
   })
 }