From 81139ce9a61b3da0ea430dc92272eb826734444e Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 7 Jul 2026 15:58:57 +0100 Subject: [PATCH 01/24] chore: update pvtr plugin to v0.24.0+2fixes Bumps pvtr-github-repo-scanner from v0.23.2 (c7bd9538) to commit c1095c95, which is post-v0.24.0 and includes two required fixes: - fix(OSPS-BR-07.01): consult Security Insights when security_and_analysis is null - fix(OSPS-LE-02.01): pass deprecated but OSI/FSF-approved SPDX IDs Also updates the plugin build stage to golang:1.26.4-alpine3.22, required by go.mod at the new commit (go 1.26.4). Privateer binary stays at 0.21.2, confirmed compatible by pvtr's own Dockerfile. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: Joana Maia --- .../docker/Dockerfile.security_best_practices_worker | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/services/docker/Dockerfile.security_best_practices_worker b/scripts/services/docker/Dockerfile.security_best_practices_worker index cb250dc12f..04baff781b 100644 --- a/scripts/services/docker/Dockerfile.security_best_practices_worker +++ b/scripts/services/docker/Dockerfile.security_best_practices_worker @@ -1,4 +1,4 @@ -ARG PVTR_VERSION=v0.23.2 +ARG PVTR_VERSION=v0.24.0 FROM alpine:3.21 AS core RUN apk add --no-cache wget tar unzip @@ -10,13 +10,13 @@ ARG PLATFORM=Linux_x86_64 RUN wget https://github.com/privateerproj/privateer/releases/download/v${VERSION}/privateer_${PLATFORM}.tar.gz RUN tar -xzf privateer_${PLATFORM}.tar.gz -FROM golang:1.26.3-alpine3.23 AS plugin +FROM golang:1.26.4-alpine3.22 AS plugin RUN apk add --no-cache make git WORKDIR /plugin -ARG PVTR_COMMIT=c7bd9538d64f7eaab94a05c9b5fd05458a387b1c +ARG PVTR_COMMIT=c1095c95a1b399ec63e4f4e2b7880b0ef55e604f ARG PVTR_VERSION # To run the latest version of the plugin, we need to use the latest commit of the pvtr-github-repo-scanner repository. -# Currently using v0.23.2: https://github.com/ossf/pvtr-github-repo-scanner/commit/c7bd9538d64f7eaab94a05c9b5fd05458a387b1c +# Currently using v0.24.0+2fixes: https://github.com/ossf/pvtr-github-repo-scanner/commit/c1095c95a1b399ec63e4f4e2b7880b0ef55e604f RUN git clone https://github.com/ossf/pvtr-github-repo-scanner.git && cd pvtr-github-repo-scanner && git checkout ${PVTR_COMMIT} RUN cd pvtr-github-repo-scanner && make binary && cp github-repo ../github-repo From 338d99b47dacd41d731a757b623c36fa1c79b0da Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 15:35:37 +0100 Subject: [PATCH 02/24] fix: issues with runs Signed-off-by: Joana Maia --- .../example-config.yml | 6 +++--- .../src/activities/index.ts | 20 ++++++++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/services/apps/security_best_practices_worker/example-config.yml b/services/apps/security_best_practices_worker/example-config.yml index f6619448b1..4e965c0daa 100644 --- a/services/apps/security_best_practices_worker/example-config.yml +++ b/services/apps/security_best_practices_worker/example-config.yml @@ -12,6 +12,6 @@ services: - Maturity Level 1 vars: - owner: $REPO_OWNER - repo: $REPO_NAME - token: $GITHUB_TOKEN + owner: "$REPO_OWNER" + repo: "$REPO_NAME" + token: "$GITHUB_TOKEN" diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index e1d92a5712..9039885613 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -44,8 +44,15 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P const combinedOutput = `${stdout}\n${stderr}` + if (combinedOutput.includes('401 Unauthorized')) { + svc.log.warn('Detected 401 error in privateer output - token invalid or expired!') + throw ApplicationFailure.create({ + message: 'GitHub token invalid or expired', + type: 'Token403Error', + }) + } if (combinedOutput.includes('403')) { - svc.log.warn('Detected 403 error in privateer output!') + svc.log.warn('Detected 403 error in privateer output - token rate-limited!') throw ApplicationFailure.create({ message: 'GitHub token rate-limited', type: 'Token403Error', @@ -54,10 +61,17 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P } catch (err) { svc.log.error(`Privateer run failed: ${err.message}`) - // check for 403 in captured output if available + // check for auth errors in captured output if available const output = `${err.stdout || ''}\n${err.stderr || ''}` + if (output.includes('401 Unauthorized')) { + svc.log.warn('Detected 401 error in failed privateer output - token invalid or expired!') + throw ApplicationFailure.create({ + message: 'GitHub token invalid or expired', + type: 'Token403Error', + }) + } if (output.includes('403')) { - svc.log.warn('Detected 403 error in failed privateer output!') + svc.log.warn('Detected 403 error in failed privateer output - token rate-limited!') throw ApplicationFailure.create({ message: 'GitHub token rate-limited', type: 'Token403Error', From da949b30a2e56df9aaee0d7d1dbe82c44c935c1b Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 15:48:08 +0100 Subject: [PATCH 03/24] fix: lint Signed-off-by: Joana Maia --- .../apps/security_best_practices_worker/example-config.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/apps/security_best_practices_worker/example-config.yml b/services/apps/security_best_practices_worker/example-config.yml index 4e965c0daa..8b3b8df719 100644 --- a/services/apps/security_best_practices_worker/example-config.yml +++ b/services/apps/security_best_practices_worker/example-config.yml @@ -12,6 +12,6 @@ services: - Maturity Level 1 vars: - owner: "$REPO_OWNER" - repo: "$REPO_NAME" - token: "$GITHUB_TOKEN" + owner: '$REPO_OWNER' + repo: '$REPO_NAME' + token: '$GITHUB_TOKEN' From c2d9ba09c9100a1318b9e07fa71e54d3b3693300 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 15:49:20 +0100 Subject: [PATCH 04/24] fix: handle '401 Bad credentials' as invalid token in privateer auth check Signed-off-by: Joana Maia --- .../security_best_practices_worker/src/activities/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index 9039885613..d881a47fe5 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -44,7 +44,7 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P const combinedOutput = `${stdout}\n${stderr}` - if (combinedOutput.includes('401 Unauthorized')) { + if (combinedOutput.includes('401 Unauthorized') || combinedOutput.includes('401 Bad credentials')) { svc.log.warn('Detected 401 error in privateer output - token invalid or expired!') throw ApplicationFailure.create({ message: 'GitHub token invalid or expired', @@ -63,7 +63,7 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P // check for auth errors in captured output if available const output = `${err.stdout || ''}\n${err.stderr || ''}` - if (output.includes('401 Unauthorized')) { + if (output.includes('401 Unauthorized') || output.includes('401 Bad credentials')) { svc.log.warn('Detected 401 error in failed privateer output - token invalid or expired!') throw ApplicationFailure.create({ message: 'GitHub token invalid or expired', From 7da9a3eaea2cf8817213bd9a2b3c26bb62558237 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 16:31:24 +0100 Subject: [PATCH 05/24] fix: address rate limiting, token rotation, and timeout issues in security worker Signed-off-by: Joana Maia --- .../src/activities/index.ts | 12 +++++-- .../triggerSecurityInsightsCheckForRepos.ts | 34 ++++++++++++++----- .../upsertOSPSBaselineSecurityInsights.ts | 2 +- 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index d881a47fe5..d8707a735f 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -48,7 +48,8 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P svc.log.warn('Detected 401 error in privateer output - token invalid or expired!') throw ApplicationFailure.create({ message: 'GitHub token invalid or expired', - type: 'Token403Error', + type: 'TokenAuthError', + nonRetryable: true, }) } if (combinedOutput.includes('403')) { @@ -56,6 +57,7 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P throw ApplicationFailure.create({ message: 'GitHub token rate-limited', type: 'Token403Error', + nonRetryable: true, }) } } catch (err) { @@ -67,7 +69,8 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P svc.log.warn('Detected 401 error in failed privateer output - token invalid or expired!') throw ApplicationFailure.create({ message: 'GitHub token invalid or expired', - type: 'Token403Error', + type: 'TokenAuthError', + nonRetryable: true, }) } if (output.includes('403')) { @@ -75,6 +78,7 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P throw ApplicationFailure.create({ message: 'GitHub token rate-limited', type: 'Token403Error', + nonRetryable: true, }) } throw err @@ -292,7 +296,9 @@ export async function initializeTokenInfos(): Promise { const tokenInfosInRedis = await redisCache.get('tokenInfos') if (tokenInfosInRedis) { - return JSON.parse(tokenInfosInRedis) + const parsed: ITokenInfo[] = JSON.parse(tokenInfosInRedis) + // reset rate-limited/in-use state — GitHub limits reset hourly, each run starts fresh + return parsed.map((t) => ({ ...t, isRateLimited: false, inUse: false })) } return process.env['CROWD_GITHUB_PERSONAL_ACCESS_TOKENS'].split(',').map((token) => ({ diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index edfe0f048c..ea8f40d216 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -48,7 +48,12 @@ export async function triggerSecurityInsightsCheckForRepos( async function processRepo(repo: (typeof repos)[0]): Promise { let attempts = 0 while (attempts < MAX_TOKEN_ATTEMPTS) { - const tokenInfo = await getNextToken(tokenInfos) + const tokenInfo = getNextToken(tokenInfos) + if (!tokenInfo) { + console.error(`No usable tokens for repo ${repo.repoUrl}, skipping`) + failedRepoUrls.push(repo.repoUrl) + break + } const token = tokenInfo.token await acquireToken(tokenInfos, token) @@ -63,7 +68,7 @@ export async function triggerSecurityInsightsCheckForRepos( initialInterval: 2000, backoffCoefficient: 2, maximumInterval: 30000, - nonRetryableErrorTypes: ['Token403Error'], + nonRetryableErrorTypes: ['Token403Error', 'TokenAuthError'], }, args: [ { @@ -76,7 +81,15 @@ export async function triggerSecurityInsightsCheckForRepos( }) return // success } catch (error) { - if (error instanceof ApplicationFailure && error.type === 'Token403Error') { + // executeChild rejects with ChildWorkflowFailure wrapping ActivityFailure wrapping + // ApplicationFailure — traverse the cause chain to find the root ApplicationFailure + const appFailure = unwrapApplicationFailure(error) + if (appFailure?.type === 'Token403Error') { + tokenInfo.isRateLimited = true + attempts++ + continue // retry with a different token + } else if (appFailure?.type === 'TokenAuthError') { + console.error(`Token invalid/expired for repo ${repo.repoUrl}, excluding token for this run`) tokenInfo.isRateLimited = true attempts++ continue // retry with a different token @@ -125,7 +138,7 @@ export async function triggerSecurityInsightsCheckForRepos( }) } -async function getNextToken(tokenInfos: ITokenInfo[]): Promise { +function getNextToken(tokenInfos: ITokenInfo[]): ITokenInfo | null { const usableTokenInfos = tokenInfos.filter((token) => !token.inUse && !token.isRateLimited) // sort usable tokens by last used date from oldest to newest @@ -135,11 +148,16 @@ async function getNextToken(tokenInfos: ITokenInfo[]): Promise { return aTime - bTime }) - if (sortedTokenInfos.length === 0) { - throw new Error('No usable tokens available') - } + return sortedTokenInfos[0] ?? null +} - return sortedTokenInfos[0] +function unwrapApplicationFailure(error: unknown): ApplicationFailure | null { + let e: unknown = error + while (e) { + if (e instanceof ApplicationFailure) return e + e = (e as { cause?: unknown }).cause + } + return null } async function releaseToken(tokenInfos: ITokenInfo[], token: string): Promise { diff --git a/services/apps/security_best_practices_worker/src/workflows/upsertOSPSBaselineSecurityInsights.ts b/services/apps/security_best_practices_worker/src/workflows/upsertOSPSBaselineSecurityInsights.ts index bab2a5ff16..eddb74c74f 100644 --- a/services/apps/security_best_practices_worker/src/workflows/upsertOSPSBaselineSecurityInsights.ts +++ b/services/apps/security_best_practices_worker/src/workflows/upsertOSPSBaselineSecurityInsights.ts @@ -6,7 +6,7 @@ import { IUpsertOSPSBaselineSecurityInsightsParams } from '../types' const { getOSPSBaselineInsights, saveOSPSBaselineInsightsToDB } = proxyActivities< typeof activities >({ - startToCloseTimeout: '5 minutes', + startToCloseTimeout: '15 minutes', retry: { maximumAttempts: 5, initialInterval: 2 * 1000, backoffCoefficient: 1 }, }) From b884d895b47ae0bec4a77048524be586cdff1011 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 16:38:57 +0100 Subject: [PATCH 06/24] fix: lint Signed-off-by: Joana Maia --- .../security_best_practices_worker/src/activities/index.ts | 5 ++++- .../src/workflows/triggerSecurityInsightsCheckForRepos.ts | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index d8707a735f..a5ae1c9559 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -44,7 +44,10 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P const combinedOutput = `${stdout}\n${stderr}` - if (combinedOutput.includes('401 Unauthorized') || combinedOutput.includes('401 Bad credentials')) { + if ( + combinedOutput.includes('401 Unauthorized') || + combinedOutput.includes('401 Bad credentials') + ) { svc.log.warn('Detected 401 error in privateer output - token invalid or expired!') throw ApplicationFailure.create({ message: 'GitHub token invalid or expired', diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index ea8f40d216..b16946d70d 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -89,7 +89,9 @@ export async function triggerSecurityInsightsCheckForRepos( attempts++ continue // retry with a different token } else if (appFailure?.type === 'TokenAuthError') { - console.error(`Token invalid/expired for repo ${repo.repoUrl}, excluding token for this run`) + console.error( + `Token invalid/expired for repo ${repo.repoUrl}, excluding token for this run`, + ) tokenInfo.isRateLimited = true attempts++ continue // retry with a different token From b7e036d110d8b9a18147cede251054c971c5866e Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 16:47:14 +0100 Subject: [PATCH 07/24] fix: harden token state handling across batches and concurrent tasks - Distinguish 'in-use' from 'excluded' tokens: requeue repos when tokens are temporarily busy instead of failing them. - Track rateLimitedAt timestamp for 403 rate limits; auto-reset after 1 hour. - Introduce isInvalid flag for 401 auth failures; never auto-resets. - Preserve isRateLimited/isInvalid across continueAsNew via existing Redis state. - Record repo as failed when MAX_TOKEN_ATTEMPTS is exhausted. Signed-off-by: Joana Maia --- .../src/activities/index.ts | 5 +- .../src/types.ts | 2 + .../triggerSecurityInsightsCheckForRepos.ts | 60 +++++++++++++++++-- 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index a5ae1c9559..99f685d327 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -300,8 +300,9 @@ export async function initializeTokenInfos(): Promise { if (tokenInfosInRedis) { const parsed: ITokenInfo[] = JSON.parse(tokenInfosInRedis) - // reset rate-limited/in-use state — GitHub limits reset hourly, each run starts fresh - return parsed.map((t) => ({ ...t, isRateLimited: false, inUse: false })) + // Reset inUse (workflow processes may have crashed leaving stale in-use flags). + // Preserve isRateLimited/isInvalid — those are cleared based on rateLimitedAt (>1h) or admin action. + return parsed.map((t) => ({ ...t, inUse: false })) } return process.env['CROWD_GITHUB_PERSONAL_ACCESS_TOKENS'].split(',').map((token) => ({ diff --git a/services/apps/security_best_practices_worker/src/types.ts b/services/apps/security_best_practices_worker/src/types.ts index e50a44a4f2..6ac6191806 100644 --- a/services/apps/security_best_practices_worker/src/types.ts +++ b/services/apps/security_best_practices_worker/src/types.ts @@ -54,4 +54,6 @@ export interface ITokenInfo { inUse: boolean lastUsed: Date isRateLimited: boolean + rateLimitedAt?: string // ISO timestamp; used to auto-reset after 1 hour + isInvalid?: boolean // permanent auth failure — never auto-resets } diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index b16946d70d..1ebb5c388b 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -20,6 +20,8 @@ const { findObsoleteRepos, initializeTokenInfos, updateTokenInfos } = proxyActiv retry: { maximumAttempts: 3, backoffCoefficient: 3 }, }) +const ONE_HOUR_MS = 60 * 60 * 1000 + export async function triggerSecurityInsightsCheckForRepos( args: ITriggerSecurityInsightsCheckForReposParams, ): Promise { @@ -31,7 +33,10 @@ export async function triggerSecurityInsightsCheckForRepos( const info = workflowInfo() const failedRepoUrls = args?.failedRepoUrls || [] + // 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() + const repos = await findObsoleteRepos( REPOS_OBSOLETE_AFTER_SECONDS, failedRepoUrls, @@ -50,8 +55,16 @@ export async function triggerSecurityInsightsCheckForRepos( while (attempts < MAX_TOKEN_ATTEMPTS) { const tokenInfo = getNextToken(tokenInfos) if (!tokenInfo) { - console.error(`No usable tokens for repo ${repo.repoUrl}, skipping`) - failedRepoUrls.push(repo.repoUrl) + // Distinguish: are there tokens that will become free, or are all excluded? + const anyRecoverable = tokenInfos.some((t) => !t.isInvalid && !t.isRateLimited) + if (anyRecoverable) { + // All non-excluded tokens are currently in use by other concurrent tasks. + // Put the repo back and let the outer loop retry after a task finishes. + queue.unshift(repo) + } else { + console.error(`All tokens excluded for repo ${repo.repoUrl}, skipping`) + failedRepoUrls.push(repo.repoUrl) + } break } const token = tokenInfo.token @@ -86,13 +99,14 @@ export async function triggerSecurityInsightsCheckForRepos( const appFailure = unwrapApplicationFailure(error) if (appFailure?.type === 'Token403Error') { tokenInfo.isRateLimited = true + tokenInfo.rateLimitedAt = new Date().toISOString() attempts++ continue // retry with a different token } else if (appFailure?.type === 'TokenAuthError') { console.error( - `Token invalid/expired for repo ${repo.repoUrl}, excluding token for this run`, + `Token invalid/expired for repo ${repo.repoUrl}, permanently excluding token`, ) - tokenInfo.isRateLimited = true + tokenInfo.isInvalid = true attempts++ continue // retry with a different token } else { @@ -107,6 +121,11 @@ export async function triggerSecurityInsightsCheckForRepos( await releaseToken(tokenInfos, tokenInfo.token) } } + + if (attempts >= MAX_TOKEN_ATTEMPTS) { + console.error(`Exhausted token attempts for repo ${repo.repoUrl}, skipping`) + failedRepoUrls.push(repo.repoUrl) + } } /** @@ -119,6 +138,12 @@ export async function triggerSecurityInsightsCheckForRepos( */ while (queue.length > 0 || activeTasks.length > 0) { while (queue.length > 0 && activeTasks.length < MAX_PARALLEL_CHILDREN) { + // Only start a task if a token is currently free — avoids false "no token" failures + // when all tokens are temporarily held by other concurrent tasks. + refreshExpiredRateLimits(tokenInfos) + const hasFreeToken = tokenInfos.some((t) => !t.isInvalid && !t.isRateLimited && !t.inUse) + if (!hasFreeToken) break + const repo = queue.shift() const task = processRepo(repo).finally(() => { const index = activeTasks.indexOf(task) @@ -127,12 +152,19 @@ export async function triggerSecurityInsightsCheckForRepos( activeTasks.push(task) } - // Wait for any one to finish before refilling if (activeTasks.length > 0) { await Promise.race(activeTasks) + } else if (queue.length > 0) { + // No active tasks and no free tokens — all remaining repos cannot be processed this batch. + for (const repo of queue.splice(0)) { + failedRepoUrls.push(repo.repoUrl) + } + break } } + // Persist token state to Redis so isRateLimited (with rateLimitedAt for expiry) and isInvalid + // survive the continueAsNew handoff. await updateTokenInfos(tokenInfos) await continueAsNew({ @@ -140,8 +172,24 @@ export async function triggerSecurityInsightsCheckForRepos( }) } +function refreshExpiredRateLimits(tokenInfos: ITokenInfo[]): void { + const now = Date.now() + for (const t of tokenInfos) { + if ( + t.isRateLimited && + t.rateLimitedAt && + now - new Date(t.rateLimitedAt).getTime() > ONE_HOUR_MS + ) { + t.isRateLimited = false + t.rateLimitedAt = undefined + } + } +} + function getNextToken(tokenInfos: ITokenInfo[]): ITokenInfo | null { - const usableTokenInfos = tokenInfos.filter((token) => !token.inUse && !token.isRateLimited) + refreshExpiredRateLimits(tokenInfos) + + const usableTokenInfos = tokenInfos.filter((t) => !t.inUse && !t.isRateLimited && !t.isInvalid) // sort usable tokens by last used date from oldest to newest const sortedTokenInfos = usableTokenInfos.sort((a, b) => { From 5d9fa1950fab6293531ca05c25f5f44f908f5bab Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 17:28:34 +0100 Subject: [PATCH 08/24] fix: prevent stuck tokens and repo blacklist across continueAsNew batches Signed-off-by: Joana Maia --- .../src/activities/index.ts | 13 ++++++++++--- .../triggerSecurityInsightsCheckForRepos.ts | 10 ++++++---- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index 99f685d327..9c1c4a5f4c 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -300,9 +300,16 @@ export async function initializeTokenInfos(): Promise { if (tokenInfosInRedis) { const parsed: ITokenInfo[] = JSON.parse(tokenInfosInRedis) - // Reset inUse (workflow processes may have crashed leaving stale in-use flags). - // Preserve isRateLimited/isInvalid — those are cleared based on rateLimitedAt (>1h) or admin action. - return parsed.map((t) => ({ ...t, inUse: false })) + return parsed.map((t) => ({ + ...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 process.env['CROWD_GITHUB_PERSONAL_ACCESS_TOKENS'].split(',').map((token) => ({ diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 1ebb5c388b..4a41392747 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -155,10 +155,12 @@ export async function triggerSecurityInsightsCheckForRepos( if (activeTasks.length > 0) { await Promise.race(activeTasks) } else if (queue.length > 0) { - // No active tasks and no free tokens — all remaining repos cannot be processed this batch. - for (const repo of queue.splice(0)) { - failedRepoUrls.push(repo.repoUrl) - } + // No active tasks and no free tokens — remaining repos can't be processed this batch. + // Don't add them to failedRepoUrls: they're still obsolete and findObsoleteRepos will + // pick them up on the next batch after rate limits expire. + console.warn( + `No tokens available; deferring ${queue.length} repos to next batch (they will be re-fetched)`, + ) break } } From f25e23de151d9cbc046009a1046f4e14556b1a68 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 18:07:59 +0100 Subject: [PATCH 09/24] fix: use workflow startTime for determinism and avoid continueAsNew hot loop Signed-off-by: Joana Maia --- .../triggerSecurityInsightsCheckForRepos.ts | 32 ++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 4a41392747..3138697a9b 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -33,6 +33,12 @@ export async function triggerSecurityInsightsCheckForRepos( 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. + 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() @@ -53,7 +59,7 @@ export async function triggerSecurityInsightsCheckForRepos( async function processRepo(repo: (typeof repos)[0]): Promise { let attempts = 0 while (attempts < MAX_TOKEN_ATTEMPTS) { - const tokenInfo = getNextToken(tokenInfos) + const tokenInfo = getNextToken(tokenInfos, nowMs) if (!tokenInfo) { // Distinguish: are there tokens that will become free, or are all excluded? const anyRecoverable = tokenInfos.some((t) => !t.isInvalid && !t.isRateLimited) @@ -99,7 +105,7 @@ export async function triggerSecurityInsightsCheckForRepos( const appFailure = unwrapApplicationFailure(error) if (appFailure?.type === 'Token403Error') { tokenInfo.isRateLimited = true - tokenInfo.rateLimitedAt = new Date().toISOString() + tokenInfo.rateLimitedAt = nowIso attempts++ continue // retry with a different token } else if (appFailure?.type === 'TokenAuthError') { @@ -136,11 +142,12 @@ export async function triggerSecurityInsightsCheckForRepos( * it will be removed from the activeTasks array and the next task will be started. * This way we don't need to wait for all tasks to finish before starting new ones. */ + let deferred = false while (queue.length > 0 || activeTasks.length > 0) { while (queue.length > 0 && activeTasks.length < MAX_PARALLEL_CHILDREN) { // Only start a task if a token is currently free — avoids false "no token" failures // when all tokens are temporarily held by other concurrent tasks. - refreshExpiredRateLimits(tokenInfos) + refreshExpiredRateLimits(tokenInfos, nowMs) const hasFreeToken = tokenInfos.some((t) => !t.isInvalid && !t.isRateLimited && !t.inUse) if (!hasFreeToken) break @@ -161,6 +168,7 @@ export async function triggerSecurityInsightsCheckForRepos( console.warn( `No tokens available; deferring ${queue.length} repos to next batch (they will be re-fetched)`, ) + deferred = true break } } @@ -169,18 +177,26 @@ export async function triggerSecurityInsightsCheckForRepos( // survive the continueAsNew handoff. await updateTokenInfos(tokenInfos) + if (deferred) { + // All tokens rate-limited or invalid and repos still pending. Calling continueAsNew here + // would spin: findObsoleteRepos returns the same repos, they defer again, etc. + // Return instead — the daily schedule (scheduleCheckReposWithObsoleteSecurityInsights) + // will re-trigger the workflow; rate-limit timestamps persist in Redis so recovery is + // resumable once the 1h window elapses. + return + } + await continueAsNew({ failedRepoUrls, }) } -function refreshExpiredRateLimits(tokenInfos: ITokenInfo[]): void { - const now = Date.now() +function refreshExpiredRateLimits(tokenInfos: ITokenInfo[], nowMs: number): void { for (const t of tokenInfos) { if ( t.isRateLimited && t.rateLimitedAt && - now - new Date(t.rateLimitedAt).getTime() > ONE_HOUR_MS + nowMs - new Date(t.rateLimitedAt).getTime() > ONE_HOUR_MS ) { t.isRateLimited = false t.rateLimitedAt = undefined @@ -188,8 +204,8 @@ function refreshExpiredRateLimits(tokenInfos: ITokenInfo[]): void { } } -function getNextToken(tokenInfos: ITokenInfo[]): ITokenInfo | null { - refreshExpiredRateLimits(tokenInfos) +function getNextToken(tokenInfos: ITokenInfo[], nowMs: number): ITokenInfo | null { + refreshExpiredRateLimits(tokenInfos, nowMs) const usableTokenInfos = tokenInfos.filter((t) => !t.inUse && !t.isRateLimited && !t.isInvalid) From d2c415c4c49c35d778a48ac64623e884d42fc95b Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 13 Jul 2026 18:09:51 +0100 Subject: [PATCH 10/24] fix: distinguish 403 rate-limit from permission failures in token classifier Signed-off-by: Joana Maia --- .../src/activities/index.ts | 72 +++++++++---------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index 9c1c4a5f4c..50d5c7a261 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -44,46 +44,12 @@ export async function getOSPSBaselineInsights(repoUrl: string, token: string): P const combinedOutput = `${stdout}\n${stderr}` - if ( - combinedOutput.includes('401 Unauthorized') || - combinedOutput.includes('401 Bad credentials') - ) { - svc.log.warn('Detected 401 error in privateer output - token invalid or expired!') - throw ApplicationFailure.create({ - message: 'GitHub token invalid or expired', - type: 'TokenAuthError', - nonRetryable: true, - }) - } - if (combinedOutput.includes('403')) { - svc.log.warn('Detected 403 error in privateer output - token rate-limited!') - throw ApplicationFailure.create({ - message: 'GitHub token rate-limited', - type: 'Token403Error', - nonRetryable: true, - }) - } + classifyTokenError(combinedOutput, 'privateer output') } catch (err) { svc.log.error(`Privateer run failed: ${err.message}`) - // check for auth errors in captured output if available const output = `${err.stdout || ''}\n${err.stderr || ''}` - if (output.includes('401 Unauthorized') || output.includes('401 Bad credentials')) { - svc.log.warn('Detected 401 error in failed privateer output - token invalid or expired!') - throw ApplicationFailure.create({ - message: 'GitHub token invalid or expired', - type: 'TokenAuthError', - nonRetryable: true, - }) - } - if (output.includes('403')) { - svc.log.warn('Detected 403 error in failed privateer output - token rate-limited!') - throw ApplicationFailure.create({ - message: 'GitHub token rate-limited', - type: 'Token403Error', - nonRetryable: true, - }) - } + classifyTokenError(output, 'failed privateer output') throw err } @@ -217,6 +183,40 @@ export async function saveOSPSBaselineInsightsToRedis( await redisCache.set(key, JSON.stringify(insights), 60 * 60 * 24) // 1 day } +// GitHub returns 403 for both rate-limit exhaustion AND persistent permission/SAML failures. +// 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 { + 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, + }) + } + if (!output.includes('403')) return + + // Rate-limit 403s from GitHub carry a distinctive body: "API rate limit exceeded" or + // "secondary rate limit". Anything else (SAML enforcement, missing scopes, resource + // not accessible) is a permission problem and the token won't recover. + const isRateLimit = /rate limit|rate_limit|secondary rate/i.test(output) + if (isRateLimit) { + svc.log.warn(`Detected 403 rate-limit in ${source} - token rate-limited!`) + throw ApplicationFailure.create({ + message: 'GitHub token rate-limited', + type: 'Token403Error', + nonRetryable: true, + }) + } + 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, + }) +} + function computeRunDuration(start: string | undefined, end: string | undefined): string { if (!start || !end) return '' const startMs = new Date(start).getTime() From aeccd3edfd9f189a0dda820e9e1da0e645483378 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 11:28:54 +0100 Subject: [PATCH 11/24] fix: accurate rate-limit timestamps, dynamic token attempts, env-var reconciliation Signed-off-by: Joana Maia --- .../src/activities/index.ts | 56 ++++++++++++------- .../triggerSecurityInsightsCheckForRepos.ts | 33 ++++++++--- 2 files changed, 60 insertions(+), 29 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index 50d5c7a261..59bbbff96e 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/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,6 +213,7 @@ 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!`) @@ -214,6 +221,7 @@ function classifyTokenError(output: string, source: string): void { message: 'GitHub token lacks required permissions', type: 'TokenAuthError', nonRetryable: true, + details: [failedAtMs], }) } @@ -297,27 +305,35 @@ export async function initializeTokenInfos(): Promise { 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 { diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 3138697a9b..4a0933a094 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/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 { +// 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 { const tokenInfo = tokenInfos.find((tokenInfo) => tokenInfo.token === token) if (tokenInfo) { tokenInfo.inUse = false - tokenInfo.lastUsed = new Date() + tokenInfo.lastUsed = new Date(nowMs) } } From 5bfa6ab72038a73f327eacf4cfa2e18f6987afdb Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 12:22:27 +0100 Subject: [PATCH 12/24] fix: refresh wall-clock time in loop, requeue rate-limited pool Signed-off-by: Joana Maia --- .../src/activities.ts | 2 + .../src/activities/index.ts | 7 +++ .../triggerSecurityInsightsCheckForRepos.ts | 49 ++++++++++--------- 3 files changed, 34 insertions(+), 24 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities.ts b/services/apps/security_best_practices_worker/src/activities.ts index 15ef05952d..a0bc8587b3 100644 --- a/services/apps/security_best_practices_worker/src/activities.ts +++ b/services/apps/security_best_practices_worker/src/activities.ts @@ -1,5 +1,6 @@ import { findObsoleteRepos, + getCurrentTimeMs, getOSPSBaselineInsights, initializeTokenInfos, saveOSPSBaselineInsightsToDB, @@ -12,4 +13,5 @@ export { findObsoleteRepos, initializeTokenInfos, updateTokenInfos, + getCurrentTimeMs, } diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index 59bbbff96e..a2a4dc5b2c 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -340,3 +340,10 @@ export async function updateTokenInfos(tokenInfos: ITokenInfo[]): Promise const redisCache = new RedisCache(`osps-baseline-insights`, svc.redis, svc.log) await redisCache.set('tokenInfos', JSON.stringify(tokenInfos), 60 * 60 * 24) // 1 day } + +// Wall-clock time via activity — workflow code can't call Date.now() (determinism rule), +// but rate-limit cooldowns need real elapsed time to expire mid-run rather than only at +// the next continueAsNew batch boundary. +export async function getCurrentTimeMs(): Promise { + return Date.now() +} diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 4a0933a094..b4290938c3 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -13,12 +13,11 @@ import { ITokenInfo, ITriggerSecurityInsightsCheckForReposParams } from '../type import { upsertOSPSBaselineSecurityInsights } from './upsertOSPSBaselineSecurityInsights' -const { findObsoleteRepos, initializeTokenInfos, updateTokenInfos } = proxyActivities< - typeof activities ->({ - startToCloseTimeout: '5 minutes', - retry: { maximumAttempts: 3, backoffCoefficient: 3 }, -}) +const { findObsoleteRepos, initializeTokenInfos, updateTokenInfos, getCurrentTimeMs } = + proxyActivities({ + startToCloseTimeout: '5 minutes', + retry: { maximumAttempts: 3, backoffCoefficient: 3 }, + }) const ONE_HOUR_MS = 60 * 60 * 1000 @@ -32,12 +31,10 @@ export async function triggerSecurityInsightsCheckForRepos( const info = workflowInfo() const failedRepoUrls = args?.failedRepoUrls || [] - // 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() + // Wall-clock time comes from the getCurrentTimeMs activity — workflow code can't call + // Date.now() (Temporal determinism rule). Refreshed each outer-loop iteration so rate-limit + // cooldowns can expire mid-run instead of only at continueAsNew boundaries. + let currentTimeMs = await getCurrentTimeMs() // Token state is persisted in Redis via updateTokenInfos so isRateLimited (with rateLimitedAt // for expiry) and isInvalid survive across continueAsNew batches. @@ -63,17 +60,18 @@ export async function triggerSecurityInsightsCheckForRepos( async function processRepo(repo: (typeof repos)[0]): Promise { let attempts = 0 while (attempts < MAX_TOKEN_ATTEMPTS) { - const tokenInfo = getNextToken(tokenInfos, nowMs) + const tokenInfo = getNextToken(tokenInfos, currentTimeMs) if (!tokenInfo) { - // Distinguish: are there tokens that will become free, or are all excluded? - const anyRecoverable = tokenInfos.some((t) => !t.isInvalid && !t.isRateLimited) - if (anyRecoverable) { - // All non-excluded tokens are currently in use by other concurrent tasks. - // Put the repo back and let the outer loop retry after a task finishes. - queue.unshift(repo) - } else { - console.error(`All tokens excluded for repo ${repo.repoUrl}, skipping`) + // Only mark the repo failed if every token is permanently invalid. Rate-limited + // tokens recover after 1h and in-use tokens free up as concurrent tasks finish, so + // in both cases requeue the repo — the outer loop's hasFreeToken gate + defer path + // handles the wait. + const allPermanentlyInvalid = tokenInfos.every((t) => t.isInvalid) + if (allPermanentlyInvalid) { + console.error(`All tokens permanently invalid for repo ${repo.repoUrl}, skipping`) failedRepoUrls.push(repo.repoUrl) + } else { + queue.unshift(repo) } break } @@ -112,7 +110,7 @@ export async function triggerSecurityInsightsCheckForRepos( // 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() + extractFailureTimestamp(appFailure) ?? new Date(currentTimeMs).toISOString() attempts++ continue // retry with a different token } else if (appFailure?.type === 'TokenAuthError') { @@ -131,7 +129,7 @@ export async function triggerSecurityInsightsCheckForRepos( break } } finally { - await releaseToken(tokenInfos, tokenInfo.token, nowMs) + await releaseToken(tokenInfos, tokenInfo.token, currentTimeMs) } } @@ -151,10 +149,13 @@ export async function triggerSecurityInsightsCheckForRepos( */ let deferred = false while (queue.length > 0 || activeTasks.length > 0) { + // Refresh wall-clock time each iteration so mid-run rate-limit cooldowns can expire + // without waiting for the next continueAsNew batch. + currentTimeMs = await getCurrentTimeMs() while (queue.length > 0 && activeTasks.length < MAX_PARALLEL_CHILDREN) { // Only start a task if a token is currently free — avoids false "no token" failures // when all tokens are temporarily held by other concurrent tasks. - refreshExpiredRateLimits(tokenInfos, nowMs) + refreshExpiredRateLimits(tokenInfos, currentTimeMs) const hasFreeToken = tokenInfos.some((t) => !t.isInvalid && !t.isRateLimited && !t.inUse) if (!hasFreeToken) break From 24d0730b2212999417b80645cef0958dfc317c90 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 12:24:10 +0100 Subject: [PATCH 13/24] fix: classify 429 as rate limit and scope 403 permission failures per repo Signed-off-by: Joana Maia --- .../src/activities/index.ts | 30 +++++++++++-------- .../triggerSecurityInsightsCheckForRepos.ts | 8 ++++- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index a2a4dc5b2c..821cfa4a29 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -183,9 +183,10 @@ export async function saveOSPSBaselineInsightsToRedis( await redisCache.set(key, JSON.stringify(insights), 60 * 60 * 24) // 1 day } -// GitHub returns 403 for both rate-limit exhaustion AND persistent permission/SAML failures. -// 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. +// GitHub returns 403 for rate-limit exhaustion AND persistent permission/SAML failures, and +// 429 for primary rate-limit hits. Only 401 proves the token is globally invalid — SAML/repo +// permissions vary per org, so a permission 403 on one repo doesn't mean the token is unusable +// elsewhere. 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() @@ -201,14 +202,17 @@ function classifyTokenError(output: string, source: string): void { details: [failedAtMs], }) } - if (!output.includes('403')) return - // Rate-limit 403s from GitHub carry a distinctive body: "API rate limit exceeded" or - // "secondary rate limit". Anything else (SAML enforcement, missing scopes, resource - // not accessible) is a permission problem and the token won't recover. - const isRateLimit = /rate limit|rate_limit|secondary rate/i.test(output) + const has403 = output.includes('403') + const has429 = output.includes('429') + if (!has403 && !has429) return + + // Rate-limit responses carry a distinctive body: "API rate limit exceeded" or + // "secondary rate limit". 429 is always a rate-limit signal. 403 without those markers + // is a permission problem (SAML enforcement, missing scopes, resource not accessible). + const isRateLimit = has429 || /rate limit|rate_limit|secondary rate/i.test(output) if (isRateLimit) { - svc.log.warn(`Detected 403 rate-limit in ${source} - token rate-limited!`) + svc.log.warn(`Detected rate-limit in ${source} - token rate-limited!`) throw ApplicationFailure.create({ message: 'GitHub token rate-limited', type: 'Token403Error', @@ -216,10 +220,12 @@ function classifyTokenError(output: string, source: string): void { details: [failedAtMs], }) } - svc.log.warn(`Detected 403 permission error in ${source} - token lacks required access!`) + // Permission 403: this token can't access THIS repo, but may work for others. Signal the + // workflow to try a different token without marking this one globally invalid. + svc.log.warn(`Detected 403 permission error in ${source} - token lacks access to this repo!`) throw ApplicationFailure.create({ - message: 'GitHub token lacks required permissions', - type: 'TokenAuthError', + message: 'GitHub token lacks required permissions for this repo', + type: 'TokenRepoAccessError', nonRetryable: true, details: [failedAtMs], }) diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index b4290938c3..91f0f7a67e 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -89,7 +89,7 @@ export async function triggerSecurityInsightsCheckForRepos( initialInterval: 2000, backoffCoefficient: 2, maximumInterval: 30000, - nonRetryableErrorTypes: ['Token403Error', 'TokenAuthError'], + nonRetryableErrorTypes: ['Token403Error', 'TokenAuthError', 'TokenRepoAccessError'], }, args: [ { @@ -120,6 +120,12 @@ export async function triggerSecurityInsightsCheckForRepos( tokenInfo.isInvalid = true attempts++ continue // retry with a different token + } else if (appFailure?.type === 'TokenRepoAccessError') { + // Token can't access this repo (SAML/perms) but may work elsewhere — don't taint + // the token's global state, just try the next one for this repo. + console.warn(`Token lacks access to repo ${repo.repoUrl}, trying different token`) + attempts++ + continue } else { console.error(`Failed to process repo ${repo.repoUrl}:`, error) // we retried this error using the retry policy (because it's not a token non-retryable error) From 93a94da6622f690255f7023df84361a25aa82579 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 12:34:28 +0100 Subject: [PATCH 14/24] fix: monotonic release timestamp for LRU rotation, clarify isInvalid TTL Signed-off-by: Joana Maia --- services/apps/security_best_practices_worker/src/types.ts | 2 +- .../src/workflows/triggerSecurityInsightsCheckForRepos.ts | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/types.ts b/services/apps/security_best_practices_worker/src/types.ts index 6ac6191806..ce4578bb32 100644 --- a/services/apps/security_best_practices_worker/src/types.ts +++ b/services/apps/security_best_practices_worker/src/types.ts @@ -55,5 +55,5 @@ export interface ITokenInfo { lastUsed: Date isRateLimited: boolean rateLimitedAt?: string // ISO timestamp; used to auto-reset after 1 hour - isInvalid?: boolean // permanent auth failure — never auto-resets + isInvalid?: boolean // 401 auth failure; persists until the Redis token cache expires (24h TTL) — after a full idle day the token re-enters the pool fresh, giving re-provisioned PATs a natural recovery path } diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 91f0f7a67e..6d357b15ba 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -21,6 +21,12 @@ const { findObsoleteRepos, initializeTokenInfos, updateTokenInfos, getCurrentTim const ONE_HOUR_MS = 60 * 60 * 1000 +// Monotonic offset added to nowMs on each releaseToken so lastUsed is strictly increasing +// per release even when currentTimeMs hasn't refreshed. Without this, sequential releases in +// the same iteration all share currentTimeMs, the stable sort in getNextToken falls back to +// env order, and the first PAT gets re-selected repeatedly instead of rotating LRU. +let releaseCounter = 0 + export async function triggerSecurityInsightsCheckForRepos( args: ITriggerSecurityInsightsCheckForReposParams, ): Promise { @@ -254,7 +260,7 @@ async function releaseToken(tokenInfos: ITokenInfo[], token: string, nowMs: numb const tokenInfo = tokenInfos.find((tokenInfo) => tokenInfo.token === token) if (tokenInfo) { tokenInfo.inUse = false - tokenInfo.lastUsed = new Date(nowMs) + tokenInfo.lastUsed = new Date(nowMs + releaseCounter++) } } From 2be876324222c353ed8c22d802be9d147fc99e43 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 12:35:58 +0100 Subject: [PATCH 15/24] fix: fail repos when all tokens invalid, widen lastUsed to Date | string Signed-off-by: Joana Maia --- .../security_best_practices_worker/src/types.ts | 4 +++- .../triggerSecurityInsightsCheckForRepos.ts | 17 ++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/types.ts b/services/apps/security_best_practices_worker/src/types.ts index ce4578bb32..7735b6cdd5 100644 --- a/services/apps/security_best_practices_worker/src/types.ts +++ b/services/apps/security_best_practices_worker/src/types.ts @@ -52,7 +52,9 @@ export interface ITriggerSecurityInsightsCheckForReposParams { export interface ITokenInfo { token: string inUse: boolean - lastUsed: Date + // Date at initialization time; becomes an ISO string after JSON round-trip through Redis + // and Temporal payloads, so callers must wrap in `new Date()` before comparing. + lastUsed: Date | string isRateLimited: boolean rateLimitedAt?: string // ISO timestamp; used to auto-reset after 1 hour isInvalid?: boolean // 401 auth failure; persists until the Redis token cache expires (24h TTL) — after a full idle day the token re-enters the pool fresh, giving re-provisioned PATs a natural recovery path diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 6d357b15ba..231fa54134 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -182,9 +182,20 @@ export async function triggerSecurityInsightsCheckForRepos( if (activeTasks.length > 0) { await Promise.race(activeTasks) } else if (queue.length > 0) { - // No active tasks and no free tokens — remaining repos can't be processed this batch. - // Don't add them to failedRepoUrls: they're still obsolete and findObsoleteRepos will - // pick them up on the next batch after rate limits expire. + // No active tasks and no free tokens. If every token is permanently invalid, deferring + // would loop forever across scheduled runs — fail the remaining repos so ops sees the + // problem instead of a silent stall. + const allPermanentlyInvalid = tokenInfos.every((t) => t.isInvalid) + if (allPermanentlyInvalid) { + console.error( + `All tokens permanently invalid; marking ${queue.length} remaining repos as failed`, + ) + for (const repo of queue) failedRepoUrls.push(repo.repoUrl) + queue.length = 0 + break + } + // Otherwise defer: remaining repos are still obsolete and findObsoleteRepos will pick + // them up on the next batch after rate limits expire. console.warn( `No tokens available; deferring ${queue.length} repos to next batch (they will be re-fetched)`, ) From cdc45de8214b4679a8b2102fa3881fbdef8f4385 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 13:51:35 +0100 Subject: [PATCH 16/24] fix: harden token error handling and workflow determinism Signed-off-by: Joana Maia --- .../src/activities/index.ts | 8 ++-- .../src/types.ts | 2 +- .../triggerSecurityInsightsCheckForRepos.ts | 47 +++++++------------ 3 files changed, 22 insertions(+), 35 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index 821cfa4a29..fb7212c4ff 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -293,13 +293,13 @@ async function runBinary( resolve({ stdout, stderr }) } else { const truncated = (s: string) => (s.length > 500 ? s.slice(0, 500) + '…' : s) - const truncStdout = truncated(stdout) - const truncStderr = truncated(stderr) + // Attach full stdout/stderr so classifyTokenError sees rate-limit markers that may + // appear after the truncation cut. Message is still truncated for log readability. const err = Object.assign( new Error( - `Binary exited with code ${code}\nStderr:\n${truncStderr}\nStdout:\n${truncStdout}`, + `Binary exited with code ${code}\nStderr:\n${truncated(stderr)}\nStdout:\n${truncated(stdout)}`, ), - { stdout: truncStdout, stderr: truncStderr }, + { stdout, stderr }, ) reject(err) } diff --git a/services/apps/security_best_practices_worker/src/types.ts b/services/apps/security_best_practices_worker/src/types.ts index 7735b6cdd5..43277126e2 100644 --- a/services/apps/security_best_practices_worker/src/types.ts +++ b/services/apps/security_best_practices_worker/src/types.ts @@ -57,5 +57,5 @@ export interface ITokenInfo { lastUsed: Date | string isRateLimited: boolean rateLimitedAt?: string // ISO timestamp; used to auto-reset after 1 hour - isInvalid?: boolean // 401 auth failure; persists until the Redis token cache expires (24h TTL) — after a full idle day the token re-enters the pool fresh, giving re-provisioned PATs a natural recovery path + isInvalid?: boolean // 401 auth failure. Effectively permanent while the scheduler keeps running (`updateTokenInfos` rewrites the Redis key with a fresh 24h TTL on every batch, so the cache doesn't expire on its own). Recovery paths: rotate the PAT out of `CROWD_GITHUB_PERSONAL_ACCESS_TOKENS` (env is authoritative for pool membership) or let the cache expire during a >24h scheduler outage. } diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 231fa54134..69e1cd83f4 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -21,12 +21,6 @@ const { findObsoleteRepos, initializeTokenInfos, updateTokenInfos, getCurrentTim const ONE_HOUR_MS = 60 * 60 * 1000 -// Monotonic offset added to nowMs on each releaseToken so lastUsed is strictly increasing -// per release even when currentTimeMs hasn't refreshed. Without this, sequential releases in -// the same iteration all share currentTimeMs, the stable sort in getNextToken falls back to -// env order, and the first PAT gets re-selected repeatedly instead of rotating LRU. -let releaseCounter = 0 - export async function triggerSecurityInsightsCheckForRepos( args: ITriggerSecurityInsightsCheckForReposParams, ): Promise { @@ -34,6 +28,11 @@ export async function triggerSecurityInsightsCheckForRepos( const LIMIT_REPOS_TO_CHECK_PER_RUN = 100 const MAX_PARALLEL_CHILDREN = 5 + // Monotonic offset added to nowMs on each release so lastUsed is strictly increasing per + // release even when currentTimeMs hasn't refreshed. Kept function-scoped (not module-level) + // so Temporal workflow isolate reuse across executions doesn't leak state between runs. + let releaseCounter = 0 + const info = workflowInfo() const failedRepoUrls = args?.failedRepoUrls || [] @@ -82,8 +81,7 @@ export async function triggerSecurityInsightsCheckForRepos( break } const token = tokenInfo.token - - await acquireToken(tokenInfos, token) + tokenInfo.inUse = true try { await executeChild(upsertOSPSBaselineSecurityInsights, { @@ -141,7 +139,8 @@ export async function triggerSecurityInsightsCheckForRepos( break } } finally { - await releaseToken(tokenInfos, tokenInfo.token, currentTimeMs) + tokenInfo.inUse = false + tokenInfo.lastUsed = new Date(currentTimeMs + releaseCounter++) } } @@ -195,7 +194,10 @@ export async function triggerSecurityInsightsCheckForRepos( break } // Otherwise defer: remaining repos are still obsolete and findObsoleteRepos will pick - // them up on the next batch after rate limits expire. + // them up on the next scheduled run (daily cron `0 8 * * *`). We don't `sleep()` here + // to the earliest cooldown because the 30-day obsolescence window makes a ~23h delay + // negligible, and holding a worker slot idle for hours per rate-limit event isn't worth + // the cost. `rateLimitedAt` persists in Redis so the 1h expiry still applies on resume. console.warn( `No tokens available; deferring ${queue.length} repos to next batch (they will be re-fetched)`, ) @@ -224,11 +226,11 @@ export async function triggerSecurityInsightsCheckForRepos( function refreshExpiredRateLimits(tokenInfos: ITokenInfo[], nowMs: number): void { for (const t of tokenInfos) { - if ( - t.isRateLimited && - t.rateLimitedAt && - nowMs - new Date(t.rateLimitedAt).getTime() > ONE_HOUR_MS - ) { + if (!t.isRateLimited) continue + // Clear rate-limit when the timestamp is missing, unparseable, or older than 1h. + // Guarding against NaN avoids permanently wedging a token due to malformed cache data. + const rateMs = t.rateLimitedAt ? new Date(t.rateLimitedAt).getTime() : NaN + if (!Number.isFinite(rateMs) || nowMs - rateMs > ONE_HOUR_MS) { t.isRateLimited = false t.rateLimitedAt = undefined } @@ -266,18 +268,3 @@ function extractFailureTimestamp(appFailure: ApplicationFailure): string | null if (typeof ts !== 'number' || !Number.isFinite(ts)) return null return new Date(ts).toISOString() } - -async function releaseToken(tokenInfos: ITokenInfo[], token: string, nowMs: number): Promise { - const tokenInfo = tokenInfos.find((tokenInfo) => tokenInfo.token === token) - if (tokenInfo) { - tokenInfo.inUse = false - tokenInfo.lastUsed = new Date(nowMs + releaseCounter++) - } -} - -async function acquireToken(tokenInfos: ITokenInfo[], token: string): Promise { - const tokenInfo = tokenInfos.find((tokenInfo) => tokenInfo.token === token) - if (tokenInfo) { - tokenInfo.inUse = true - } -} From ea84fef16404b939f6a784c8cc8a558ff1596ead Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 14:07:09 +0100 Subject: [PATCH 17/24] fix: requeue repo on rate-limit exhaustion instead of failing Signed-off-by: Joana Maia --- .../triggerSecurityInsightsCheckForRepos.ts | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 69e1cd83f4..5dd29306e1 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -64,6 +64,12 @@ export async function triggerSecurityInsightsCheckForRepos( async function processRepo(repo: (typeof repos)[0]): Promise { let attempts = 0 + // Distinguishes attempt exhaustion caused by transient rate-limits from exhaustion caused + // by per-repo access failures. With MAX_TOKEN_ATTEMPTS == poolSize, a pool where every + // token 403s in this run exits the loop via `attempts >= MAX_TOKEN_ATTEMPTS` rather than + // via `!tokenInfo`, so requeue decisions have to be made here too — otherwise a rate-limit + // wave would falsely fail the repo for the rest of the continueAsNew chain. + let sawRateLimit = false while (attempts < MAX_TOKEN_ATTEMPTS) { const tokenInfo = getNextToken(tokenInfos, currentTimeMs) if (!tokenInfo) { @@ -115,6 +121,7 @@ export async function triggerSecurityInsightsCheckForRepos( // batch startTime when details are missing so we still record something. tokenInfo.rateLimitedAt = extractFailureTimestamp(appFailure) ?? new Date(currentTimeMs).toISOString() + sawRateLimit = true attempts++ continue // retry with a different token } else if (appFailure?.type === 'TokenAuthError') { @@ -145,8 +152,19 @@ export async function triggerSecurityInsightsCheckForRepos( } if (attempts >= MAX_TOKEN_ATTEMPTS) { - console.error(`Exhausted token attempts for repo ${repo.repoUrl}, skipping`) - failedRepoUrls.push(repo.repoUrl) + // If any attempt hit a rate-limit and not every token is permanently invalid, the + // exhaustion is transient — requeue so the repo can be retried once cooldowns expire + // (via the outer loop's `refreshExpiredRateLimits`) or on a later scheduled run. + const allPermanentlyInvalid = tokenInfos.every((t) => t.isInvalid) + if (sawRateLimit && !allPermanentlyInvalid) { + console.warn( + `Exhausted token attempts (rate-limit) for repo ${repo.repoUrl}, requeuing`, + ) + queue.unshift(repo) + } else { + console.error(`Exhausted token attempts for repo ${repo.repoUrl}, skipping`) + failedRepoUrls.push(repo.repoUrl) + } } } From c5c864688aa97e03663996b991c72caf42e65879 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 14:15:58 +0100 Subject: [PATCH 18/24] docs: correct stale references to workflowInfo().startTime in comments Signed-off-by: Joana Maia --- .../security_best_practices_worker/src/activities/index.ts | 2 +- .../src/workflows/triggerSecurityInsightsCheckForRepos.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index fb7212c4ff..32ec2794f0 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -190,7 +190,7 @@ export async function saveOSPSBaselineInsightsToRedis( 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). + // directly (Temporal determinism rule). const failedAtMs = Date.now() if (output.includes('401 Unauthorized') || output.includes('401 Bad credentials')) { diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 5dd29306e1..358b27e8e7 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -117,8 +117,9 @@ export async function triggerSecurityInsightsCheckForRepos( const appFailure = unwrapApplicationFailure(error) if (appFailure?.type === 'Token403Error') { tokenInfo.isRateLimited = true - // 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. + // Activity captures the wall-clock time of the 403 in details[0]; fall back to + // the iteration's currentTimeMs (from the getCurrentTimeMs activity) so we still + // record something when details are missing. tokenInfo.rateLimitedAt = extractFailureTimestamp(appFailure) ?? new Date(currentTimeMs).toISOString() sawRateLimit = true From 351c47c36049d2e33cf218227133601920b6fb65 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 14:19:10 +0100 Subject: [PATCH 19/24] fix: track per-repo attempted tokens and match HTTP status by word boundary Signed-off-by: Joana Maia --- .../src/activities/index.ts | 6 ++-- .../triggerSecurityInsightsCheckForRepos.ts | 35 ++++++++++++++----- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index 32ec2794f0..c9b51acd05 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -203,8 +203,10 @@ function classifyTokenError(output: string, source: string): void { }) } - const has403 = output.includes('403') - const has429 = output.includes('429') + // Word-boundary match so unrelated numbers (e.g. "processed 4291 records") don't get + // misclassified as HTTP 429/403. + const has403 = /\b403\b/.test(output) + const has429 = /\b429\b/.test(output) if (!has403 && !has429) return // Rate-limit responses carry a distinctive body: "API rate limit exceeded" or diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 358b27e8e7..fb40d58dc2 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -70,16 +70,25 @@ export async function triggerSecurityInsightsCheckForRepos( // via `!tokenInfo`, so requeue decisions have to be made here too — otherwise a rate-limit // wave would falsely fail the repo for the rest of the continueAsNew chain. let sawRateLimit = false + // Per-repo attempted-tokens set. TokenRepoAccessError releases the token in "clean" + // global state, so without this filter `getNextToken` could re-pick the same token + // while the others are held by concurrent children — burning attempts without ever + // reaching a token that might have access. + const attemptedTokens = new Set() while (attempts < MAX_TOKEN_ATTEMPTS) { - const tokenInfo = getNextToken(tokenInfos, currentTimeMs) + const tokenInfo = getNextToken(tokenInfos, currentTimeMs, attemptedTokens) if (!tokenInfo) { - // Only mark the repo failed if every token is permanently invalid. Rate-limited - // tokens recover after 1h and in-use tokens free up as concurrent tasks finish, so - // in both cases requeue the repo — the outer loop's hasFreeToken gate + defer path - // handles the wait. + // No untried, usable token remains for this repo. Distinguish two cases: + // - all tokens permanently invalid, OR every non-invalid token has been tried for + // this repo → this repo genuinely can't be scanned, fail it + // - some non-invalid, non-attempted tokens exist but are currently in-use or + // rate-limited → requeue so a later iteration can retry when they free up const allPermanentlyInvalid = tokenInfos.every((t) => t.isInvalid) - if (allPermanentlyInvalid) { - console.error(`All tokens permanently invalid for repo ${repo.repoUrl}, skipping`) + const anyUnattemptedNonInvalid = tokenInfos.some( + (t) => !t.isInvalid && !attemptedTokens.has(t.token), + ) + if (allPermanentlyInvalid || !anyUnattemptedNonInvalid) { + console.error(`No untried tokens remain for repo ${repo.repoUrl}, skipping`) failedRepoUrls.push(repo.repoUrl) } else { queue.unshift(repo) @@ -87,6 +96,7 @@ export async function triggerSecurityInsightsCheckForRepos( break } const token = tokenInfo.token + attemptedTokens.add(token) tokenInfo.inUse = true try { @@ -256,10 +266,17 @@ function refreshExpiredRateLimits(tokenInfos: ITokenInfo[], nowMs: number): void } } -function getNextToken(tokenInfos: ITokenInfo[], nowMs: number): ITokenInfo | null { +function getNextToken( + tokenInfos: ITokenInfo[], + nowMs: number, + excludeTokens?: Set, +): ITokenInfo | null { refreshExpiredRateLimits(tokenInfos, nowMs) - const usableTokenInfos = tokenInfos.filter((t) => !t.inUse && !t.isRateLimited && !t.isInvalid) + const usableTokenInfos = tokenInfos.filter( + (t) => + !t.inUse && !t.isRateLimited && !t.isInvalid && !(excludeTokens && excludeTokens.has(t.token)), + ) // sort usable tokens by last used date from oldest to newest const sortedTokenInfos = usableTokenInfos.sort((a, b) => { From 0b8b786a57b024fdd7268747f7d631a71927fea2 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 14:33:03 +0100 Subject: [PATCH 20/24] fix: lint Signed-off-by: Joana Maia --- .../workflows/triggerSecurityInsightsCheckForRepos.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index fb40d58dc2..ccaa542ad9 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -168,9 +168,7 @@ export async function triggerSecurityInsightsCheckForRepos( // (via the outer loop's `refreshExpiredRateLimits`) or on a later scheduled run. const allPermanentlyInvalid = tokenInfos.every((t) => t.isInvalid) if (sawRateLimit && !allPermanentlyInvalid) { - console.warn( - `Exhausted token attempts (rate-limit) for repo ${repo.repoUrl}, requeuing`, - ) + console.warn(`Exhausted token attempts (rate-limit) for repo ${repo.repoUrl}, requeuing`) queue.unshift(repo) } else { console.error(`Exhausted token attempts for repo ${repo.repoUrl}, skipping`) @@ -275,7 +273,10 @@ function getNextToken( const usableTokenInfos = tokenInfos.filter( (t) => - !t.inUse && !t.isRateLimited && !t.isInvalid && !(excludeTokens && excludeTokens.has(t.token)), + !t.inUse && + !t.isRateLimited && + !t.isInvalid && + !(excludeTokens && excludeTokens.has(t.token)), ) // sort usable tokens by last used date from oldest to newest From 7ed743288af79c8a7a90e8a6448aa9da3d4cbdca Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 14:44:31 +0100 Subject: [PATCH 21/24] fix: persist per-repo attempted tokens across requeues Signed-off-by: Joana Maia --- .../triggerSecurityInsightsCheckForRepos.ts | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index ccaa542ad9..361faea94f 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -62,6 +62,14 @@ export async function triggerSecurityInsightsCheckForRepos( const queue = [...repos] const activeTasks: Promise[] = [] + // Per-repo attempted-token state kept across in-run requeues. Only TokenRepoAccessError + // records into this set — Token403Error and TokenAuthError already mark the token + // isRateLimited/isInvalid, so getNextToken filters them out globally. Preserving the set + // across requeues prevents the same TokenRepoAccessError token from being immediately + // re-picked when other tokens are held by concurrent children. The map is function-scoped + // so continueAsNew starts each batch with a clean slate. + const repoAttemptedTokens = new Map>() + async function processRepo(repo: (typeof repos)[0]): Promise { let attempts = 0 // Distinguishes attempt exhaustion caused by transient rate-limits from exhaustion caused @@ -70,33 +78,36 @@ export async function triggerSecurityInsightsCheckForRepos( // via `!tokenInfo`, so requeue decisions have to be made here too — otherwise a rate-limit // wave would falsely fail the repo for the rest of the continueAsNew chain. let sawRateLimit = false - // Per-repo attempted-tokens set. TokenRepoAccessError releases the token in "clean" - // global state, so without this filter `getNextToken` could re-pick the same token - // while the others are held by concurrent children — burning attempts without ever - // reaching a token that might have access. - const attemptedTokens = new Set() + // Look up (or create) the persisted attempted-token set for this repo. Persistence + // across requeues is the fix for a concurrency race: a requeued repo with a fresh set + // could immediately re-pick the same token that just returned TokenRepoAccessError + // while other tokens are held by concurrent children. + let attemptedTokens = repoAttemptedTokens.get(repo.repoUrl) + if (!attemptedTokens) { + attemptedTokens = new Set() + repoAttemptedTokens.set(repo.repoUrl, attemptedTokens) + } while (attempts < MAX_TOKEN_ATTEMPTS) { const tokenInfo = getNextToken(tokenInfos, currentTimeMs, attemptedTokens) if (!tokenInfo) { - // No untried, usable token remains for this repo. Distinguish two cases: - // - all tokens permanently invalid, OR every non-invalid token has been tried for - // this repo → this repo genuinely can't be scanned, fail it - // - some non-invalid, non-attempted tokens exist but are currently in-use or - // rate-limited → requeue so a later iteration can retry when they free up + // No untried, usable token remains for this repo. Fail only when either every token + // is permanently invalid, or every non-invalid token has been tried AND none of the + // attempts hit a rate-limit that could cool down. Otherwise requeue: some tokens are + // rate-limited (may recover this run) or in-use by concurrent children (may free up). const allPermanentlyInvalid = tokenInfos.every((t) => t.isInvalid) const anyUnattemptedNonInvalid = tokenInfos.some( (t) => !t.isInvalid && !attemptedTokens.has(t.token), ) - if (allPermanentlyInvalid || !anyUnattemptedNonInvalid) { + if (allPermanentlyInvalid || (!anyUnattemptedNonInvalid && !sawRateLimit)) { console.error(`No untried tokens remain for repo ${repo.repoUrl}, skipping`) failedRepoUrls.push(repo.repoUrl) + repoAttemptedTokens.delete(repo.repoUrl) } else { queue.unshift(repo) } break } const token = tokenInfo.token - attemptedTokens.add(token) tokenInfo.inUse = true try { @@ -120,6 +131,7 @@ export async function triggerSecurityInsightsCheckForRepos( }, ], }) + repoAttemptedTokens.delete(repo.repoUrl) return // success } catch (error) { // executeChild rejects with ChildWorkflowFailure wrapping ActivityFailure wrapping @@ -144,8 +156,10 @@ export async function triggerSecurityInsightsCheckForRepos( continue // retry with a different token } else if (appFailure?.type === 'TokenRepoAccessError') { // Token can't access this repo (SAML/perms) but may work elsewhere — don't taint - // the token's global state, just try the next one for this repo. + // the token's global state. Record it in attemptedTokens (persisted across + // requeues) so getNextToken won't hand this token back for this repo again. console.warn(`Token lacks access to repo ${repo.repoUrl}, trying different token`) + attemptedTokens.add(token) attempts++ continue } else { @@ -154,6 +168,7 @@ export async function triggerSecurityInsightsCheckForRepos( // but it failed in all retries. // to proceed with processing, we don't wanna try this repo again in this run failedRepoUrls.push(repo.repoUrl) + repoAttemptedTokens.delete(repo.repoUrl) break } } finally { @@ -173,6 +188,7 @@ export async function triggerSecurityInsightsCheckForRepos( } else { console.error(`Exhausted token attempts for repo ${repo.repoUrl}, skipping`) failedRepoUrls.push(repo.repoUrl) + repoAttemptedTokens.delete(repo.repoUrl) } } } From 95a17e907e9ce48004db71621dcd05161bf0341c Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 14:48:12 +0100 Subject: [PATCH 22/24] fix: fail fast on empty token pool and refresh clock per-attempt Signed-off-by: Joana Maia --- .../triggerSecurityInsightsCheckForRepos.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 361faea94f..219c7264f7 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -45,6 +45,17 @@ export async function triggerSecurityInsightsCheckForRepos( // for expiry) and isInvalid survive across continueAsNew batches. const tokenInfos: ITokenInfo[] = await initializeTokenInfos() + // Empty pool means CROWD_GITHUB_PERSONAL_ACCESS_TOKENS is misconfigured. Without this guard + // an empty array would satisfy `tokenInfos.every(t => t.isInvalid)` and silently mark every + // obsolete repo as failed for the continueAsNew chain, hiding the config error. + if (tokenInfos.length === 0) { + throw ApplicationFailure.create({ + message: 'No GitHub tokens configured (CROWD_GITHUB_PERSONAL_ACCESS_TOKENS is empty)', + type: 'TokenPoolEmpty', + nonRetryable: true, + }) + } + // 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) @@ -88,6 +99,10 @@ export async function triggerSecurityInsightsCheckForRepos( repoAttemptedTokens.set(repo.repoUrl, attemptedTokens) } while (attempts < MAX_TOKEN_ATTEMPTS) { + // Refresh wall-clock time before each token selection. A long-running executeChild can + // block the outer queue loop's refresh for >1h, leaving rate-limit cooldowns stuck when + // this is the only in-flight task (small pool or last repo). + currentTimeMs = await getCurrentTimeMs() const tokenInfo = getNextToken(tokenInfos, currentTimeMs, attemptedTokens) if (!tokenInfo) { // No untried, usable token remains for this repo. Fail only when either every token From 1daefa343b5cf24ae7dbd44b18f0a1e5c2ed2485 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 15:01:54 +0100 Subject: [PATCH 23/24] fix: skip first-attempt clock refresh to preserve sync token claim Signed-off-by: Joana Maia --- .../triggerSecurityInsightsCheckForRepos.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index 219c7264f7..c0987ddaee 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -99,10 +99,15 @@ export async function triggerSecurityInsightsCheckForRepos( repoAttemptedTokens.set(repo.repoUrl, attemptedTokens) } while (attempts < MAX_TOKEN_ATTEMPTS) { - // Refresh wall-clock time before each token selection. A long-running executeChild can - // block the outer queue loop's refresh for >1h, leaving rate-limit cooldowns stuck when - // this is the only in-flight task (small pool or last repo). - currentTimeMs = await getCurrentTimeMs() + // Refresh wall-clock time on retries only. A long-running executeChild can block the + // outer queue loop's refresh for >1h, leaving rate-limit cooldowns stuck; a per-retry + // refresh fixes that. The first attempt intentionally reuses the outer-loop's + // currentTimeMs so processRepo runs synchronously up to `tokenInfo.inUse = true` — + // otherwise the outer loop can oversubscribe MAX_PARALLEL_CHILDREN tasks against a + // smaller token pool before any inUse mark lands, causing immediate null-token requeues. + if (attempts > 0) { + currentTimeMs = await getCurrentTimeMs() + } const tokenInfo = getNextToken(tokenInfos, currentTimeMs, attemptedTokens) if (!tokenInfo) { // No untried, usable token remains for this repo. Fail only when either every token From 6a552dab03ed24996ef357d4c7bea00d678cd800 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 14 Jul 2026 15:18:25 +0100 Subject: [PATCH 24/24] refactor(security-worker): remove TokenRepoAccessError and simplify token handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Permission 403s don't need their own error type — the child workflow's retry policy (5 attempts) exhausts naturally and the repo defers to the next scheduled run. Introducing TokenRepoAccessError required per-repo attempted-token tracking (repoAttemptedTokens map, sawRateLimit flag, releaseCounter) that added ~60 lines of complexity for a marginal benefit. Core fixes from the AI-generated code are preserved: unwrapApplicationFailure (ChildWorkflowFailure traversal was silently broken), TokenAuthError for 401s, isInvalid token field, rateLimitedAt 1h expiry, hasFreeToken guard, and deferred flag to prevent continueAsNew spin on full rate-limit. Signed-off-by: Joana Maia --- .../src/activities/index.ts | 26 +---- .../src/types.ts | 2 +- .../triggerSecurityInsightsCheckForRepos.ts | 101 +++--------------- 3 files changed, 23 insertions(+), 106 deletions(-) diff --git a/services/apps/security_best_practices_worker/src/activities/index.ts b/services/apps/security_best_practices_worker/src/activities/index.ts index c9b51acd05..1b230c18a0 100644 --- a/services/apps/security_best_practices_worker/src/activities/index.ts +++ b/services/apps/security_best_practices_worker/src/activities/index.ts @@ -183,14 +183,7 @@ export async function saveOSPSBaselineInsightsToRedis( await redisCache.set(key, JSON.stringify(insights), 60 * 60 * 24) // 1 day } -// GitHub returns 403 for rate-limit exhaustion AND persistent permission/SAML failures, and -// 429 for primary rate-limit hits. Only 401 proves the token is globally invalid — SAML/repo -// permissions vary per org, so a permission 403 on one repo doesn't mean the token is unusable -// elsewhere. 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() - // directly (Temporal determinism rule). const failedAtMs = Date.now() if (output.includes('401 Unauthorized') || output.includes('401 Bad credentials')) { @@ -203,15 +196,14 @@ function classifyTokenError(output: string, source: string): void { }) } - // Word-boundary match so unrelated numbers (e.g. "processed 4291 records") don't get - // misclassified as HTTP 429/403. + // Word-boundary match so unrelated numbers don't get misclassified as HTTP 429/403. const has403 = /\b403\b/.test(output) const has429 = /\b429\b/.test(output) if (!has403 && !has429) return - // Rate-limit responses carry a distinctive body: "API rate limit exceeded" or - // "secondary rate limit". 429 is always a rate-limit signal. 403 without those markers - // is a permission problem (SAML enforcement, missing scopes, resource not accessible). + // 429 is always rate-limit. 403 with rate-limit body is rate-limit. 403 without is a + // permission problem (SAML, missing scopes) — log it but don't throw; the child workflow's + // retry policy will exhaust retries and the repo will be deferred to the next scheduled run. const isRateLimit = has429 || /rate limit|rate_limit|secondary rate/i.test(output) if (isRateLimit) { svc.log.warn(`Detected rate-limit in ${source} - token rate-limited!`) @@ -222,15 +214,7 @@ function classifyTokenError(output: string, source: string): void { details: [failedAtMs], }) } - // Permission 403: this token can't access THIS repo, but may work for others. Signal the - // workflow to try a different token without marking this one globally invalid. - svc.log.warn(`Detected 403 permission error in ${source} - token lacks access to this repo!`) - throw ApplicationFailure.create({ - message: 'GitHub token lacks required permissions for this repo', - type: 'TokenRepoAccessError', - nonRetryable: true, - details: [failedAtMs], - }) + svc.log.warn(`Detected 403 permission error in ${source} - token may lack access to this repo`) } function computeRunDuration(start: string | undefined, end: string | undefined): string { diff --git a/services/apps/security_best_practices_worker/src/types.ts b/services/apps/security_best_practices_worker/src/types.ts index 43277126e2..7945eb9fc1 100644 --- a/services/apps/security_best_practices_worker/src/types.ts +++ b/services/apps/security_best_practices_worker/src/types.ts @@ -57,5 +57,5 @@ export interface ITokenInfo { lastUsed: Date | string isRateLimited: boolean rateLimitedAt?: string // ISO timestamp; used to auto-reset after 1 hour - isInvalid?: boolean // 401 auth failure. Effectively permanent while the scheduler keeps running (`updateTokenInfos` rewrites the Redis key with a fresh 24h TTL on every batch, so the cache doesn't expire on its own). Recovery paths: rotate the PAT out of `CROWD_GITHUB_PERSONAL_ACCESS_TOKENS` (env is authoritative for pool membership) or let the cache expire during a >24h scheduler outage. + isInvalid?: boolean // 401 auth failure; recover by rotating the PAT out of CROWD_GITHUB_PERSONAL_ACCESS_TOKENS } diff --git a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts index c0987ddaee..2d16068856 100644 --- a/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts +++ b/services/apps/security_best_practices_worker/src/workflows/triggerSecurityInsightsCheckForRepos.ts @@ -28,11 +28,6 @@ export async function triggerSecurityInsightsCheckForRepos( const LIMIT_REPOS_TO_CHECK_PER_RUN = 100 const MAX_PARALLEL_CHILDREN = 5 - // Monotonic offset added to nowMs on each release so lastUsed is strictly increasing per - // release even when currentTimeMs hasn't refreshed. Kept function-scoped (not module-level) - // so Temporal workflow isolate reuse across executions doesn't leak state between runs. - let releaseCounter = 0 - const info = workflowInfo() const failedRepoUrls = args?.failedRepoUrls || [] @@ -73,55 +68,24 @@ export async function triggerSecurityInsightsCheckForRepos( const queue = [...repos] const activeTasks: Promise[] = [] - // Per-repo attempted-token state kept across in-run requeues. Only TokenRepoAccessError - // records into this set — Token403Error and TokenAuthError already mark the token - // isRateLimited/isInvalid, so getNextToken filters them out globally. Preserving the set - // across requeues prevents the same TokenRepoAccessError token from being immediately - // re-picked when other tokens are held by concurrent children. The map is function-scoped - // so continueAsNew starts each batch with a clean slate. - const repoAttemptedTokens = new Map>() - async function processRepo(repo: (typeof repos)[0]): Promise { let attempts = 0 - // Distinguishes attempt exhaustion caused by transient rate-limits from exhaustion caused - // by per-repo access failures. With MAX_TOKEN_ATTEMPTS == poolSize, a pool where every - // token 403s in this run exits the loop via `attempts >= MAX_TOKEN_ATTEMPTS` rather than - // via `!tokenInfo`, so requeue decisions have to be made here too — otherwise a rate-limit - // wave would falsely fail the repo for the rest of the continueAsNew chain. - let sawRateLimit = false - // Look up (or create) the persisted attempted-token set for this repo. Persistence - // across requeues is the fix for a concurrency race: a requeued repo with a fresh set - // could immediately re-pick the same token that just returned TokenRepoAccessError - // while other tokens are held by concurrent children. - let attemptedTokens = repoAttemptedTokens.get(repo.repoUrl) - if (!attemptedTokens) { - attemptedTokens = new Set() - repoAttemptedTokens.set(repo.repoUrl, attemptedTokens) - } while (attempts < MAX_TOKEN_ATTEMPTS) { - // Refresh wall-clock time on retries only. A long-running executeChild can block the - // outer queue loop's refresh for >1h, leaving rate-limit cooldowns stuck; a per-retry - // refresh fixes that. The first attempt intentionally reuses the outer-loop's - // currentTimeMs so processRepo runs synchronously up to `tokenInfo.inUse = true` — - // otherwise the outer loop can oversubscribe MAX_PARALLEL_CHILDREN tasks against a - // smaller token pool before any inUse mark lands, causing immediate null-token requeues. + // Refresh wall-clock time on retries only. First attempt intentionally reuses the + // outer-loop's currentTimeMs so processRepo runs synchronously up to + // `tokenInfo.inUse = true` — otherwise the outer loop can oversubscribe + // MAX_PARALLEL_CHILDREN tasks against a smaller token pool before any inUse mark lands. if (attempts > 0) { currentTimeMs = await getCurrentTimeMs() } - const tokenInfo = getNextToken(tokenInfos, currentTimeMs, attemptedTokens) + const tokenInfo = getNextToken(tokenInfos, currentTimeMs) if (!tokenInfo) { - // No untried, usable token remains for this repo. Fail only when either every token - // is permanently invalid, or every non-invalid token has been tried AND none of the - // attempts hit a rate-limit that could cool down. Otherwise requeue: some tokens are - // rate-limited (may recover this run) or in-use by concurrent children (may free up). + // No usable token — requeue if any may recover (rate-limited tokens expire after 1h + // or free up from concurrent children); fail if all are permanently invalid. const allPermanentlyInvalid = tokenInfos.every((t) => t.isInvalid) - const anyUnattemptedNonInvalid = tokenInfos.some( - (t) => !t.isInvalid && !attemptedTokens.has(t.token), - ) - if (allPermanentlyInvalid || (!anyUnattemptedNonInvalid && !sawRateLimit)) { - console.error(`No untried tokens remain for repo ${repo.repoUrl}, skipping`) + if (allPermanentlyInvalid) { + console.error(`No usable tokens for repo ${repo.repoUrl}, skipping`) failedRepoUrls.push(repo.repoUrl) - repoAttemptedTokens.delete(repo.repoUrl) } else { queue.unshift(repo) } @@ -140,7 +104,7 @@ export async function triggerSecurityInsightsCheckForRepos( initialInterval: 2000, backoffCoefficient: 2, maximumInterval: 30000, - nonRetryableErrorTypes: ['Token403Error', 'TokenAuthError', 'TokenRepoAccessError'], + nonRetryableErrorTypes: ['Token403Error', 'TokenAuthError'], }, args: [ { @@ -151,20 +115,15 @@ export async function triggerSecurityInsightsCheckForRepos( }, ], }) - repoAttemptedTokens.delete(repo.repoUrl) return // success } catch (error) { // executeChild rejects with ChildWorkflowFailure wrapping ActivityFailure wrapping - // ApplicationFailure — traverse the cause chain to find the root ApplicationFailure + // ApplicationFailure — traverse the cause chain to find the root ApplicationFailure. const appFailure = unwrapApplicationFailure(error) if (appFailure?.type === 'Token403Error') { tokenInfo.isRateLimited = true - // Activity captures the wall-clock time of the 403 in details[0]; fall back to - // the iteration's currentTimeMs (from the getCurrentTimeMs activity) so we still - // record something when details are missing. tokenInfo.rateLimitedAt = extractFailureTimestamp(appFailure) ?? new Date(currentTimeMs).toISOString() - sawRateLimit = true attempts++ continue // retry with a different token } else if (appFailure?.type === 'TokenAuthError') { @@ -174,41 +133,25 @@ export async function triggerSecurityInsightsCheckForRepos( tokenInfo.isInvalid = true attempts++ continue // retry with a different token - } else if (appFailure?.type === 'TokenRepoAccessError') { - // Token can't access this repo (SAML/perms) but may work elsewhere — don't taint - // the token's global state. Record it in attemptedTokens (persisted across - // requeues) so getNextToken won't hand this token back for this repo again. - console.warn(`Token lacks access to repo ${repo.repoUrl}, trying different token`) - attemptedTokens.add(token) - attempts++ - continue } else { console.error(`Failed to process repo ${repo.repoUrl}:`, error) - // we retried this error using the retry policy (because it's not a token non-retryable error) - // but it failed in all retries. - // to proceed with processing, we don't wanna try this repo again in this run failedRepoUrls.push(repo.repoUrl) - repoAttemptedTokens.delete(repo.repoUrl) break } } finally { tokenInfo.inUse = false - tokenInfo.lastUsed = new Date(currentTimeMs + releaseCounter++) + tokenInfo.lastUsed = new Date(currentTimeMs) } } if (attempts >= MAX_TOKEN_ATTEMPTS) { - // If any attempt hit a rate-limit and not every token is permanently invalid, the - // exhaustion is transient — requeue so the repo can be retried once cooldowns expire - // (via the outer loop's `refreshExpiredRateLimits`) or on a later scheduled run. const allPermanentlyInvalid = tokenInfos.every((t) => t.isInvalid) - if (sawRateLimit && !allPermanentlyInvalid) { - console.warn(`Exhausted token attempts (rate-limit) for repo ${repo.repoUrl}, requeuing`) + if (!allPermanentlyInvalid) { + console.warn(`Exhausted token attempts for repo ${repo.repoUrl}, requeuing`) queue.unshift(repo) } else { - console.error(`Exhausted token attempts for repo ${repo.repoUrl}, skipping`) + console.error(`All tokens invalid for repo ${repo.repoUrl}, skipping`) failedRepoUrls.push(repo.repoUrl) - repoAttemptedTokens.delete(repo.repoUrl) } } } @@ -300,20 +243,10 @@ function refreshExpiredRateLimits(tokenInfos: ITokenInfo[], nowMs: number): void } } -function getNextToken( - tokenInfos: ITokenInfo[], - nowMs: number, - excludeTokens?: Set, -): ITokenInfo | null { +function getNextToken(tokenInfos: ITokenInfo[], nowMs: number): ITokenInfo | null { refreshExpiredRateLimits(tokenInfos, nowMs) - const usableTokenInfos = tokenInfos.filter( - (t) => - !t.inUse && - !t.isRateLimited && - !t.isInvalid && - !(excludeTokens && excludeTokens.has(t.token)), - ) + const usableTokenInfos = tokenInfos.filter((t) => !t.inUse && !t.isRateLimited && !t.isInvalid) // sort usable tokens by last used date from oldest to newest const sortedTokenInfos = usableTokenInfos.sort((a, b) => {