fix: accurate rate-limit timestamps, dynamic token attempts, env-var reconciliation

Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>

Author: joanagmaia

services/apps/security_best_practices_worker/src/activities/index.ts

@@ -187,12 +187,18 @@ export async function saveOSPSBaselineInsightsToRedis(
 // Only rate-limit responses should trigger the 1h token-rotation cooldown; permission failures
 // must be marked as invalid so the token isn't retried in a loop.
 function classifyTokenError(output: string, source: string): void {
+  // Wall-clock timestamp of the failure. Passed via ApplicationFailure.details so the
+  // workflow can persist an accurate rateLimitedAt — workflow code can't call Date.now()
+  // itself (Temporal determinism rule; workflowInfo().startTime skews on long batches).
+  const failedAtMs = Date.now()
+
   if (output.includes('401 Unauthorized') || output.includes('401 Bad credentials')) {
     svc.log.warn(`Detected 401 error in ${source} - token invalid or expired!`)
     throw ApplicationFailure.create({
       message: 'GitHub token invalid or expired',
       type: 'TokenAuthError',
       nonRetryable: true,
+      details: [failedAtMs],
     })
   }
   if (!output.includes('403')) return
@@ -207,13 +213,15 @@ function classifyTokenError(output: string, source: string): void {
       message: 'GitHub token rate-limited',
       type: 'Token403Error',
       nonRetryable: true,
+      details: [failedAtMs],
     })
   }
   svc.log.warn(`Detected 403 permission error in ${source} - token lacks required access!`)
   throw ApplicationFailure.create({
     message: 'GitHub token lacks required permissions',
     type: 'TokenAuthError',
     nonRetryable: true,
+    details: [failedAtMs],
   })
 }
 
@@ -297,27 +305,35 @@ export async function initializeTokenInfos(): Promise<ITokenInfo[]> {
   const redisCache = new RedisCache(`osps-baseline-insights`, svc.redis, svc.log)
 
   const tokenInfosInRedis = await redisCache.get('tokenInfos')
-
-  if (tokenInfosInRedis) {
-    const parsed: ITokenInfo[] = JSON.parse(tokenInfosInRedis)
-    return parsed.map((t) => ({
-      ...t,
-      // Reset inUse — workflow processes may have crashed leaving stale in-use flags.
+  const cached: ITokenInfo[] = tokenInfosInRedis ? JSON.parse(tokenInfosInRedis) : []
+  const cachedByToken = new Map(cached.map((t) => [t.token, t]))
+
+  // Env var is authoritative for pool membership so PAT rotation is picked up on next run:
+  // a new token in CROWD_GITHUB_PERSONAL_ACCESS_TOKENS joins the pool fresh, and a removed
+  // token drops out even if its cached entry is still in Redis.
+  const envTokens = process.env['CROWD_GITHUB_PERSONAL_ACCESS_TOKENS'].split(',')
+
+  return envTokens.map((token) => {
+    const t = cachedByToken.get(token)
+    if (t) {
+      return {
+        ...t,
+        // Reset inUse — workflow processes may have crashed leaving stale in-use flags.
+        inUse: false,
+        // Backward compat: legacy entries have isRateLimited=true with no rateLimitedAt timestamp.
+        // Without a timestamp the 1-hour expiry can't apply and the token would be stuck forever.
+        // Clear stale rate-limits that lack a timestamp so they can be retried on this run.
+        isRateLimited: t.isRateLimited && !!t.rateLimitedAt,
+        rateLimitedAt: t.isRateLimited && t.rateLimitedAt ? t.rateLimitedAt : undefined,
+      }
+    }
+    return {
+      token,
       inUse: false,
-      // Backward compat: legacy entries have isRateLimited=true with no rateLimitedAt timestamp.
-      // Without a timestamp the 1-hour expiry can't apply and the token would be stuck forever.
-      // Clear stale rate-limits that lack a timestamp so they can be retried on this run.
-      isRateLimited: t.isRateLimited && !!t.rateLimitedAt,
-      rateLimitedAt: t.isRateLimited && t.rateLimitedAt ? t.rateLimitedAt : undefined,
-    }))
-  }
-
-  return process.env['CROWD_GITHUB_PERSONAL_ACCESS_TOKENS'].split(',').map((token) => ({
-    token,
-    inUse: false,
-    lastUsed: new Date(),
-    isRateLimited: false,
-  }))
+      lastUsed: new Date(),
+      isRateLimited: false,
+    }
+  })
 }
 
 export async function updateTokenInfos(tokenInfos: ITokenInfo[]): Promise<void> {

services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts

@@ -28,21 +28,25 @@ export async function triggerSecurityInsightsCheckForRepos(
   const REPOS_OBSOLETE_AFTER_SECONDS = 30 * 24 * 60 * 60 // 30 days
   const LIMIT_REPOS_TO_CHECK_PER_RUN = 100
   const MAX_PARALLEL_CHILDREN = 5
-  const MAX_TOKEN_ATTEMPTS = 5
 
   const info = workflowInfo()
   const failedRepoUrls = args?.failedRepoUrls || []
 
-  // Use workflowInfo().startTime as the replay-safe time reference — Temporal workflows must
-  // be deterministic, so Date.now()/new Date() are banned by services-checklist.md:9-12.
-  // startTime is stable within a run and advances on each continueAsNew batch.
+  // Use workflowInfo().startTime as the replay-safe time reference for rate-limit expiry
+  // checks — Temporal workflows must be deterministic, so Date.now()/new Date() are banned
+  // (services-checklist.md:9-12). Actual rate-limit failure timestamps come from the
+  // ApplicationFailure.details payload set inside the activity, so long batches don't skew
+  // the persisted rateLimitedAt.
   const nowMs = info.startTime.getTime()
-  const nowIso = info.startTime.toISOString()
 
   // Token state is persisted in Redis via updateTokenInfos so isRateLimited (with rateLimitedAt
   // for expiry) and isInvalid survive across continueAsNew batches.
   const tokenInfos: ITokenInfo[] = await initializeTokenInfos()
 
+  // Scale attempts to pool size so a repo isn't marked failed while untried tokens remain
+  // (e.g. 6 PATs where the first 5 attempts all 403 on different tokens).
+  const MAX_TOKEN_ATTEMPTS = Math.max(5, tokenInfos.length)
+
   const repos = await findObsoleteRepos(
     REPOS_OBSOLETE_AFTER_SECONDS,
     failedRepoUrls,
@@ -105,7 +109,10 @@ export async function triggerSecurityInsightsCheckForRepos(
         const appFailure = unwrapApplicationFailure(error)
         if (appFailure?.type === 'Token403Error') {
           tokenInfo.isRateLimited = true
-          tokenInfo.rateLimitedAt = nowIso
+          // Activity captures the wall-clock time of the 403 in details[0]; fall back to the
+          // batch startTime when details are missing so we still record something.
+          tokenInfo.rateLimitedAt =
+            extractFailureTimestamp(appFailure) ?? info.startTime.toISOString()
           attempts++
           continue // retry with a different token
         } else if (appFailure?.type === 'TokenAuthError') {
@@ -124,7 +131,7 @@ export async function triggerSecurityInsightsCheckForRepos(
           break
         }
       } finally {
-        await releaseToken(tokenInfos, tokenInfo.token)
+        await releaseToken(tokenInfos, tokenInfo.token, nowMs)
       }
     }
 
@@ -228,11 +235,19 @@ function unwrapApplicationFailure(error: unknown): ApplicationFailure | null {
   return null
 }
 
-async function releaseToken(tokenInfos: ITokenInfo[], token: string): Promise<void> {
+// Activity records the wall-clock failure time via ApplicationFailure.details[0] (ms).
+function extractFailureTimestamp(appFailure: ApplicationFailure): string | null {
+  const details = appFailure.details as unknown[] | undefined
+  const ts = details?.[0]
+  if (typeof ts !== 'number' || !Number.isFinite(ts)) return null
+  return new Date(ts).toISOString()
+}
+
+async function releaseToken(tokenInfos: ITokenInfo[], token: string, nowMs: number): Promise<void> {
   const tokenInfo = tokenInfos.find((tokenInfo) => tokenInfo.token === token)
   if (tokenInfo) {
     tokenInfo.inUse = false
-    tokenInfo.lastUsed = new Date()
+    tokenInfo.lastUsed = new Date(nowMs)
   }
 }