From 9a54a6c784f4baabdef10143375599e90c5cc914 Mon Sep 17 00:00:00 2001 From: Umberto Sgueglia Date: Fri, 16 Oct 2026 16:41:58 +0200 Subject: [PATCH] feat: retry transient evaluation errors within and across runs before recording them Refs linuxfoundation/lfx-cdp-insights-ops#215 Signed-off-by: Umberto Sgueglia --- .../projectEvaluation/evaluateProject.test.ts | 86 ++++++++++++++++++- .../v1/projectEvaluation/evaluateProject.ts | 77 ++++++++++++++++- ...0__project-catalog-evaluation-attempts.sql | 2 + .../src/activities/activities.ts | 42 +++++++-- .../src/activities/classifyAndRoute.test.ts | 1 + .../src/activities/evaluationRetry.test.ts | 19 ++++ .../src/activities/evaluationRetry.ts | 12 +++ .../activities/immediateOnboarding.test.ts | 1 + .../src/evaluator/evaluator.test.ts | 70 ++++++++++----- .../src/evaluator/evaluator.ts | 45 +++++++--- .../src/evaluator/retry.ts | 7 ++ .../src/workflows/evaluateProjects.ts | 35 ++++++-- services/libs/common/src/errors/http.ts | 9 ++ services/libs/common/src/index.ts | 2 + .../project-catalog/projectCatalog.test.ts | 21 +++++ .../src/project-catalog/projectCatalog.ts | 26 +++++- .../src/project-catalog/types.ts | 1 + 17 files changed, 406 insertions(+), 50 deletions(-) create mode 100644 backend/src/database/migrations/V1792160000__project-catalog-evaluation-attempts.sql create mode 100644 services/apps/projects_evaluation_worker/src/activities/evaluationRetry.test.ts create mode 100644 services/apps/projects_evaluation_worker/src/activities/evaluationRetry.ts create mode 100644 services/apps/projects_evaluation_worker/src/evaluator/retry.ts diff --git a/backend/src/api/public/v1/projectEvaluation/evaluateProject.test.ts b/backend/src/api/public/v1/projectEvaluation/evaluateProject.test.ts index 5afb0b76ea..d158f745d3 100644 --- a/backend/src/api/public/v1/projectEvaluation/evaluateProject.test.ts +++ b/backend/src/api/public/v1/projectEvaluation/evaluateProject.test.ts @@ -1,6 +1,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { IPublicRepoMetrics, IPublicRepoReadme } from '@crowd/common' +import { + GithubPublicClientError, + IPublicRepoMetrics, + IPublicRepoReadme, + ServiceUnavailableError, +} from '@crowd/common' import { IProjectEvaluationRequest } from '@crowd/data-access-layer/src/project-catalog/types' import { Logger } from '@crowd/logging' @@ -12,7 +17,8 @@ const getGithubToken = vi.fn() const parseLlmJson = vi.fn() const queryLlm = vi.fn() -vi.mock('@crowd/common', () => ({ +vi.mock('@crowd/common', async (importOriginal) => ({ + ...(await importOriginal()), fetchPublicRepoMetrics: (...args: unknown[]) => fetchPublicRepoMetrics(...args), fetchPublicRepoReadme: (...args: unknown[]) => fetchPublicRepoReadme(...args), getGithubToken: (...args: unknown[]) => getGithubToken(...args), @@ -176,6 +182,82 @@ describe('evaluateProject', () => { expect(result.evaluationReason).toBe('rate limited') }) + it('answers 503 instead of a verdict when GitHub or Bedrock fail transiently', async () => { + getGithubToken.mockReturnValue('token') + fetchPublicRepoMetrics.mockRejectedValue( + new GithubPublicClientError('TRANSIENT', 'Network error calling GitHub'), + ) + fetchPublicRepoReadme.mockResolvedValue(readme) + + await expect(evaluateProject(input, qx, bedrockCredentials, log)).rejects.toBeInstanceOf( + ServiceUnavailableError, + ) + + fetchPublicRepoMetrics.mockResolvedValue(metrics) + const throttled = Object.assign(new Error('Too many requests'), { + name: 'ThrottlingException', + $fault: 'client', + $retryable: { throttling: true }, + }) + queryLlm.mockRejectedValue(throttled) + + await expect(evaluateProject(input, qx, bedrockCredentials, log)).rejects.toMatchObject({ + status: 503, + message: 'Too many requests', + }) + }) + + it('keeps a retired model or a bad GitHub token as a verdict-level error', async () => { + getGithubToken.mockReturnValue('token') + fetchPublicRepoMetrics.mockResolvedValue(metrics) + fetchPublicRepoReadme.mockResolvedValue(readme) + queryLlm.mockRejectedValue( + Object.assign(new Error('This model version has reached the end of its life.'), { + name: 'ValidationException', + $fault: 'client', + }), + ) + + expect(await evaluateProject(input, qx, bedrockCredentials, log)).toMatchObject({ + evaluationResult: 'error', + evaluationReason: 'This model version has reached the end of its life.', + }) + + fetchPublicRepoMetrics.mockRejectedValue( + new GithubPublicClientError('AUTH', '401 Unauthorized'), + ) + expect(await evaluateProject(input, qx, bedrockCredentials, log)).toMatchObject({ + evaluationResult: 'error', + evaluationReason: '401 Unauthorized', + }) + }) + + it('records, without overriding, a not-on-GitHub verdict on a busy GitHub repository', async () => { + getGithubToken.mockReturnValue('token') + fetchPublicRepoMetrics.mockResolvedValue({ ...metrics, closedPullRequests: 1200 }) + fetchPublicRepoReadme.mockResolvedValue(readme) + queryLlm.mockResolvedValue({ + answer: '{"onboard": false, "non_onboard_reason": "project is not mainly run on GitHub"}', + model: 'test-model', + inputTokenCount: 10, + outputTokenCount: 5, + responseTimeSeconds: 1, + }) + parseLlmJson.mockReturnValue({ + onboard: false, + non_onboard_reason: 'project is not mainly run on GitHub', + }) + + const result = await evaluateProject(input, qx, bedrockCredentials, log) + + expect(result.outcome).toBe('skip') + expect(result.evaluationReason).toBe('project is not mainly run on GitHub') + expect(log.warn).toHaveBeenCalledWith( + expect.objectContaining({ repoUrl: input.repoUrl, closedPullRequests: 1200 }), + expect.stringContaining('the verdict is kept'), + ) + }) + it('returns an unsure/error result when the LLM query throws', async () => { getGithubToken.mockReturnValue('token') fetchPublicRepoMetrics.mockResolvedValue(metrics) diff --git a/backend/src/api/public/v1/projectEvaluation/evaluateProject.ts b/backend/src/api/public/v1/projectEvaluation/evaluateProject.ts index 1a339cc28f..2772202649 100644 --- a/backend/src/api/public/v1/projectEvaluation/evaluateProject.ts +++ b/backend/src/api/public/v1/projectEvaluation/evaluateProject.ts @@ -1,6 +1,8 @@ import { + GithubPublicClientError, IPublicRepoMetrics, IPublicRepoReadme, + ServiceUnavailableError, fetchPublicRepoMetrics, fetchPublicRepoReadme, getErrorMessage, @@ -83,6 +85,76 @@ function errorResult(err: unknown): IProjectEvaluationResponse { } } +const TRANSIENT_LLM_ERROR_NAMES = new Set([ + 'ThrottlingException', + 'ServiceUnavailableException', + 'ModelNotReadyException', + 'ModelTimeoutException', + 'InternalServerException', + 'TimeoutError', +]) + +const NETWORK_ERROR_CODES = new Set(['ECONNRESET', 'ECONNREFUSED', 'ETIMEDOUT', 'EAI_AGAIN']) + +interface IAwsLikeError { + code?: string + $fault?: string + $retryable?: unknown + $metadata?: { httpStatusCode?: number } +} + +export function isTransientEvaluationError(err: unknown): boolean { + if (err instanceof GithubPublicClientError) { + return err.kind === 'TRANSIENT' || err.kind === 'RATE_LIMIT' + } + if (!(err instanceof Error)) { + return false + } + const aws = err as Error & IAwsLikeError + const status = aws.$metadata?.httpStatusCode + return ( + TRANSIENT_LLM_ERROR_NAMES.has(aws.name) || + aws.$fault === 'server' || + aws.$retryable !== undefined || + (status !== undefined && (status === 429 || status >= 500)) || + (aws.code !== undefined && NETWORK_ERROR_CODES.has(aws.code)) + ) +} + +function failedResult(err: unknown): IProjectEvaluationResponse { + if (isTransientEvaluationError(err)) { + throw new ServiceUnavailableError(getErrorMessage(err)) + } + return errorResult(err) +} + +const NOT_ON_GITHUB_REASON: NonOnboardReason = 'project is not mainly run on GitHub' +const GITHUB_ACTIVITY_CONTRADICTION_CLOSED_PRS = 100 + +function recordNotOnGithubContradiction( + input: IProjectEvaluationRequest, + metrics: IPublicRepoMetrics, + decision: IProjectEvaluationDecision, + log: Logger, +): void { + if ( + decision.onboard || + decision.non_onboard_reason !== NOT_ON_GITHUB_REASON || + metrics.closedPullRequests < GITHUB_ACTIVITY_CONTRADICTION_CLOSED_PRS + ) { + return + } + log.warn( + { + repoUrl: input.repoUrl, + closedPullRequests: metrics.closedPullRequests, + closedIssues: metrics.closedIssues, + stars: metrics.stars, + }, + 'The evaluator says the project is not mainly run on GitHub although the repository has substantial GitHub activity; the verdict is kept.', + ) +} + const INSUFFICIENT_ACTIVITY_REASON = 'project has insufficient GitHub activity to evaluate' const MIN_CLOSED_ACTIVITY_FOR_EVALUATION = 3 const MIN_STARS_FOR_EVALUATION = 25 @@ -133,7 +205,7 @@ export async function evaluateProject( metrics = results[0] readme = results[1] } catch (err) { - return errorResult(err) + return failedResult(err) } if (hasInsufficientActivity(metrics)) { @@ -154,7 +226,7 @@ export async function evaluateProject( input.id, ) } catch (err) { - return errorResult(err) + return failedResult(err) } if (!response) { @@ -175,6 +247,7 @@ export async function evaluateProject( } const validatedDecision: IProjectEvaluationDecision = decision + recordNotOnGithubContradiction(input, metrics, validatedDecision, log) return { outcome: validatedDecision.onboard ? 'onboard' : 'skip', diff --git a/backend/src/database/migrations/V1792160000__project-catalog-evaluation-attempts.sql b/backend/src/database/migrations/V1792160000__project-catalog-evaluation-attempts.sql new file mode 100644 index 0000000000..cd8cc4cca1 --- /dev/null +++ b/backend/src/database/migrations/V1792160000__project-catalog-evaluation-attempts.sql @@ -0,0 +1,2 @@ +ALTER TABLE "projectCatalog" + ADD COLUMN "evaluationAttempts" SMALLINT NOT NULL DEFAULT 0; diff --git a/services/apps/projects_evaluation_worker/src/activities/activities.ts b/services/apps/projects_evaluation_worker/src/activities/activities.ts index 25a1d2e786..07b64f15ba 100644 --- a/services/apps/projects_evaluation_worker/src/activities/activities.ts +++ b/services/apps/projects_evaluation_worker/src/activities/activities.ts @@ -3,6 +3,7 @@ import { Context } from '@temporalio/activity' import { ONBOARDING_AUTO_APPLY, fetchPublicRepoMetrics, getGithubToken } from '@crowd/common' import { finalizeProjectCatalogEvaluation, + deferProjectCatalogEvaluation, findProjectCatalogById, findProjectCatalogPendingEvaluation, findProjectCatalogPendingOnboarding, @@ -25,7 +26,7 @@ import { isGithubDiscussionProvenance, isSlackBotProvenance, } from '@crowd/data-access-layer/src/project-catalog/types' -import { pgpQx } from '@crowd/data-access-layer/src/queryExecutor' +import { QueryExecutor, pgpQx } from '@crowd/data-access-layer/src/queryExecutor' import { getServiceLogger } from '@crowd/logging' import { BotStatus, @@ -59,6 +60,7 @@ import { WorkflowIdReusePolicy } from '@crowd/temporal' import { estimateLlmCostUsd } from '@crowd/types' import { evaluateProject } from '../evaluator/evaluator' +import { MAX_EVALUATION_RUNS } from '../evaluator/retry' import { svc } from '../main' import { IClassifyAndRouteResult, @@ -68,6 +70,7 @@ import { IPriorityConfig, } from '../types' import { classifyAndRouteProjects } from './classifyAndRoute' +import { canDeferEvaluation } from './evaluationRetry' import { buildImmediateOnboardingPlan } from './immediateOnboarding' import { buildRequestAlert, buildSkippedDiscussionAlert } from './skippedRequestAlert' @@ -375,20 +378,49 @@ export async function askRequester(project: IDbProjectCatalog): Promise { } } -export async function markEvaluationFailed(projectId: string, reason: string): Promise { +export type EvaluationFailureRecord = 'failed' | 'deferred' | 'moved' + +async function deferEvaluation( + qx: QueryExecutor, + projectId: string, + reason: string, +): Promise { + const current = await findProjectCatalogById(qx, projectId) + if (!current || !canDeferEvaluation(current)) { + return false + } + const deferred = await deferProjectCatalogEvaluation(qx, projectId, reason, MAX_EVALUATION_RUNS) + if (!deferred) { + return false + } + log.warn( + { id: projectId, reason, attempts: deferred.evaluationAttempts }, + 'Evaluation hit a transient error, the row waits for the next run.', + ) + return true +} + +export async function markEvaluationFailed( + projectId: string, + reason: string, + transient = false, +): Promise { const qx = pgpQx(svc.postgres.writer.connection()) + if (transient && (await deferEvaluation(qx, projectId, reason))) { + return 'deferred' + } const marked = await markProjectCatalogEvaluationFailed(qx, projectId, reason) if (marked > 0) { log.error({ id: projectId, reason }, 'Evaluation failed, marked as error.') - return true + return 'failed' } const current = await findProjectCatalogById(qx, projectId) if (current && isRecordedCheckError(current)) { log.info({ id: projectId }, 'Check error already recorded by an earlier attempt.') - return true + return 'failed' } log.info({ id: projectId }, 'Row left evaluate before the failure could be recorded.') - return false + return 'moved' } // The requester must act, so the Try again notice is always a new message; the status diff --git a/services/apps/projects_evaluation_worker/src/activities/classifyAndRoute.test.ts b/services/apps/projects_evaluation_worker/src/activities/classifyAndRoute.test.ts index 5dae358e99..f30a95abe1 100644 --- a/services/apps/projects_evaluation_worker/src/activities/classifyAndRoute.test.ts +++ b/services/apps/projects_evaluation_worker/src/activities/classifyAndRoute.test.ts @@ -21,6 +21,7 @@ function project(overrides: Partial = {}): IDbProjectCatalog lfCriticalityScore: null, evaluationResult: null, evaluationReason: null, + evaluationAttempts: 0, evaluatedAt: null, onboardedAt: null, onboardingError: null, diff --git a/services/apps/projects_evaluation_worker/src/activities/evaluationRetry.test.ts b/services/apps/projects_evaluation_worker/src/activities/evaluationRetry.test.ts new file mode 100644 index 0000000000..6524b8347e --- /dev/null +++ b/services/apps/projects_evaluation_worker/src/activities/evaluationRetry.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from 'vitest' + +import { canDeferEvaluation } from './evaluationRetry' + +describe('canDeferEvaluation', () => { + it('lets a scheduled row wait for the next run until the last allowed run', () => { + expect(canDeferEvaluation({ provenance: 'lf-criticality-score', evaluationAttempts: 0 })).toBe( + true, + ) + expect(canDeferEvaluation({ provenance: null, evaluationAttempts: 1 })).toBe(true) + expect(canDeferEvaluation({ provenance: 'lf-criticality-score', evaluationAttempts: 2 })).toBe( + false, + ) + }) + + it('never defers a bot request, which gets the Try again button instead', () => { + expect(canDeferEvaluation({ provenance: 'slack-bot', evaluationAttempts: 0 })).toBe(false) + }) +}) diff --git a/services/apps/projects_evaluation_worker/src/activities/evaluationRetry.ts b/services/apps/projects_evaluation_worker/src/activities/evaluationRetry.ts new file mode 100644 index 0000000000..791cca61bc --- /dev/null +++ b/services/apps/projects_evaluation_worker/src/activities/evaluationRetry.ts @@ -0,0 +1,12 @@ +import { + IDbProjectCatalog, + isSlackBotProvenance, +} from '@crowd/data-access-layer/src/project-catalog/types' + +import { MAX_EVALUATION_RUNS } from '../evaluator/retry' + +type RetryRow = Pick + +export function canDeferEvaluation(row: RetryRow): boolean { + return !isSlackBotProvenance(row.provenance) && row.evaluationAttempts + 1 < MAX_EVALUATION_RUNS +} diff --git a/services/apps/projects_evaluation_worker/src/activities/immediateOnboarding.test.ts b/services/apps/projects_evaluation_worker/src/activities/immediateOnboarding.test.ts index 27bdbf21fe..3e5b13d13f 100644 --- a/services/apps/projects_evaluation_worker/src/activities/immediateOnboarding.test.ts +++ b/services/apps/projects_evaluation_worker/src/activities/immediateOnboarding.test.ts @@ -17,6 +17,7 @@ function row(overrides: Partial): IDbProjectCatalog { lfCriticalityScore: null, evaluationResult: null, evaluationReason: null, + evaluationAttempts: 0, evaluatedAt: null, onboardedAt: null, onboardingError: null, diff --git a/services/apps/projects_evaluation_worker/src/evaluator/evaluator.test.ts b/services/apps/projects_evaluation_worker/src/evaluator/evaluator.test.ts index 7e367f1d99..5d034f545f 100644 --- a/services/apps/projects_evaluation_worker/src/evaluator/evaluator.test.ts +++ b/services/apps/projects_evaluation_worker/src/evaluator/evaluator.test.ts @@ -1,6 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { evaluateProject } from './evaluator' +import { TRANSIENT_EVALUATION_FAILURE } from './retry' import { IEvaluationInput } from './types' const input: IEvaluationInput = { @@ -65,16 +66,17 @@ describe('evaluateProject', () => { expect(result).toEqual(apiResult) }) - it('returns an unsure/error result when the fetch call rejects', async () => { + it('throws a retryable transient failure when the fetch call rejects', async () => { fetchMock.mockRejectedValue(new Error('network down')) - const result = await evaluateProject(input) - - expect(result.outcome).toBe('unsure') - expect(result.evaluationReason).toContain('network down') + await expect(evaluateProject(input)).rejects.toMatchObject({ + type: TRANSIENT_EVALUATION_FAILURE, + nonRetryable: false, + message: 'API request failed: network down', + }) }) - it('returns a rate-limited reason on HTTP 429 from the daily evaluation cap', async () => { + it('throws a transient failure without an immediate retry on the daily evaluation cap', async () => { fetchMock.mockResolvedValue({ ok: false, status: 429, @@ -86,13 +88,14 @@ describe('evaluateProject', () => { }), }) - const result = await evaluateProject(input) - - expect(result.outcome).toBe('unsure') - expect(result.evaluationReason).toBe('rate limited: daily evaluation cap reached') + await expect(evaluateProject(input)).rejects.toMatchObject({ + type: TRANSIENT_EVALUATION_FAILURE, + nonRetryable: true, + message: 'rate limited: daily evaluation cap reached', + }) }) - it('falls back to the generic reason on HTTP 429 from an unrelated rate limiter', async () => { + it('throws a retryable transient failure on HTTP 429 from an unrelated rate limiter', async () => { fetchMock.mockResolvedValue({ ok: false, status: 429, @@ -104,13 +107,14 @@ describe('evaluateProject', () => { }), }) - const result = await evaluateProject(input) - - expect(result.outcome).toBe('unsure') - expect(result.evaluationReason).toContain('API returned HTTP 429') + await expect(evaluateProject(input)).rejects.toMatchObject({ + type: TRANSIENT_EVALUATION_FAILURE, + nonRetryable: false, + message: 'API returned HTTP 429: Too many requests, please try again later', + }) }) - it('falls back to the generic reason on HTTP 429 with a non-JSON body', async () => { + it('throws a transient failure on HTTP 429 with a non-JSON body', async () => { fetchMock.mockResolvedValue({ ok: false, status: 429, @@ -122,23 +126,43 @@ describe('evaluateProject', () => { }), }) - const result = await evaluateProject(input) + await expect(evaluateProject(input)).rejects.toMatchObject({ + type: TRANSIENT_EVALUATION_FAILURE, + message: 'API returned HTTP 429: Too Many Requests', + }) + }) - expect(result.outcome).toBe('unsure') - expect(result.evaluationReason).toContain('API returned HTTP 429') + it('throws a transient failure on a server error, with the reason the API gave', async () => { + fetchMock.mockResolvedValue({ + ok: false, + status: 503, + statusText: 'Service Unavailable', + clone: () => ({ + json: async () => ({ + error: { code: 'SERVICE_UNAVAILABLE', message: 'Bedrock throttled' }, + }), + }), + }) + + await expect(evaluateProject(input)).rejects.toMatchObject({ + type: TRANSIENT_EVALUATION_FAILURE, + nonRetryable: false, + message: 'API returned HTTP 503: Bedrock throttled', + }) }) - it('returns an unsure/error result on a non-ok HTTP status', async () => { + it('returns an unsure/error result on a client error that a retry would not fix', async () => { fetchMock.mockResolvedValue({ ok: false, - status: 500, - statusText: 'Internal Server Error', + status: 400, + statusText: 'Bad Request', }) const result = await evaluateProject(input) expect(result.outcome).toBe('unsure') - expect(result.evaluationReason).toContain('500') + expect(result.evaluationResult).toBe('error') + expect(result.evaluationReason).toContain('400') }) it('returns an unsure/error result when the response body is not valid JSON', async () => { diff --git a/services/apps/projects_evaluation_worker/src/evaluator/evaluator.ts b/services/apps/projects_evaluation_worker/src/evaluator/evaluator.ts index f9ec47ab21..07f1c2297f 100644 --- a/services/apps/projects_evaluation_worker/src/evaluator/evaluator.ts +++ b/services/apps/projects_evaluation_worker/src/evaluator/evaluator.ts @@ -1,7 +1,21 @@ +import { ApplicationFailure } from '@temporalio/activity' + import { getErrorMessage } from '@crowd/common' +import { TRANSIENT_EVALUATION_FAILURE, isTransientHttpStatus } from './retry' import { IEvaluationInput, IEvaluationMetrics, IEvaluationResult } from './types' +export function transientEvaluationFailure( + message: string, + { retryNow = true }: { retryNow?: boolean } = {}, +): ApplicationFailure { + return ApplicationFailure.create({ + type: TRANSIENT_EVALUATION_FAILURE, + message, + nonRetryable: !retryNow, + }) +} + function isValidMetrics(metrics: unknown): metrics is IEvaluationMetrics | null { if (metrics === null) { return true @@ -28,6 +42,15 @@ function isValidResult(result: unknown): result is IEvaluationResult { ) } +async function responseErrorMessage(response: Response): Promise { + try { + const body = (await response.clone().json()) as { error?: { message?: string } } + return body?.error?.message ?? response.statusText + } catch { + return response.statusText + } +} + export async function evaluateProject(input: IEvaluationInput): Promise { const apiUrl = process.env.CROWD_API_SERVICE_URL const apiKey = process.env.CROWD_PROJECT_EVALUATION_STATIC_API_KEY @@ -53,12 +76,7 @@ export async function evaluateProject(input: IEvaluationInput): Promise= 500 +} diff --git a/services/apps/projects_evaluation_worker/src/workflows/evaluateProjects.ts b/services/apps/projects_evaluation_worker/src/workflows/evaluateProjects.ts index 3e20f2b466..f5ec36506e 100644 --- a/services/apps/projects_evaluation_worker/src/workflows/evaluateProjects.ts +++ b/services/apps/projects_evaluation_worker/src/workflows/evaluateProjects.ts @@ -1,4 +1,5 @@ import { + ApplicationFailure, CancellationScope, isCancellation, log, @@ -10,6 +11,7 @@ import { import type { IPipelineRunEvaluatorModelUsage } from '@crowd/data-access-layer/src/project-catalog-pipeline-runs/types' import type * as activities from '../activities' +import { TRANSIENT_EVALUATION_FAILURE } from '../evaluator/retry' import type { IClassifyAndRouteResult, IEvaluateProjectsInput, @@ -40,7 +42,7 @@ const classifyActivities = proxyActivities({ // Each AI evaluation call takes ~30-40s; give generous headroom per project. const evaluateActivities = proxyActivities({ startToCloseTimeout: '3 minutes', - retry: { maximumAttempts: 2 }, + retry: { maximumAttempts: 3, initialInterval: '30 seconds', backoffCoefficient: 2 }, }) const pipelineRunActivities = proxyActivities({ @@ -103,6 +105,17 @@ function rootErrorMessage(err: unknown): string { return current instanceof Error ? current.message : String(current) } +function isTransientEvaluationFailure(err: unknown): boolean { + let current = err + while (current instanceof Error) { + if (current instanceof ApplicationFailure && current.type === TRANSIENT_EVALUATION_FAILURE) { + return true + } + current = (current as { cause?: unknown }).cause + } + return false +} + async function postPendingQuestions(catalogIds?: string[]): Promise { const questions = await fetchActivities.fetchPendingQuestions(catalogIds) for (const project of questions) { @@ -154,6 +167,7 @@ export async function evaluateProjects(input: IEvaluateProjectsInput = {}): Prom let totalCandidates = 0 let succeeded = 0 let failed = 0 + let deferred = 0 let skipped = 0 let skippedPreCheck = 0 let classification: Pick | null = @@ -265,16 +279,27 @@ export async function evaluateProjects(input: IEvaluateProjectsInput = {}): Prom throw err } - failed++ const reason = rootErrorMessage(err) + const transient = isTransientEvaluationFailure(err) log.error( `Evaluation failed for project id=${project.id} repoUrl=${project.repoUrl}: ${reason}`, ) try { - if (await markActivities.markEvaluationFailed(project.id, reason)) { - await notifyEvaluationError(project, reason) + const recorded = await markActivities.markEvaluationFailed( + project.id, + reason, + transient, + ) + if (recorded === 'deferred') { + deferred++ + } else { + failed++ + if (recorded === 'failed') { + await notifyEvaluationError(project, reason) + } } } catch (markErr) { + failed++ // Don't let a failure to record the error state abort the rest of the batch. log.error(`Failed to mark project id=${project.id} as errored: ${String(markErr)}`) } @@ -282,7 +307,7 @@ export async function evaluateProjects(input: IEvaluateProjectsInput = {}): Prom } log.info( - `Batch evaluation complete. total=${remainingProjects.length} succeeded=${succeeded} failed=${failed} skipped=${skipped} skippedPreCheck=${skippedPreCheck}`, + `Batch evaluation complete. total=${remainingProjects.length} succeeded=${succeeded} failed=${failed} deferred=${deferred} skipped=${skipped} skippedPreCheck=${skippedPreCheck}`, ) // The evaluator can route rows to review too (new projects, undecided verdicts). if (patched('issue-803-review-after-evaluation')) { diff --git a/services/libs/common/src/errors/http.ts b/services/libs/common/src/errors/http.ts index dd29489af1..4acf794732 100644 --- a/services/libs/common/src/errors/http.ts +++ b/services/libs/common/src/errors/http.ts @@ -94,6 +94,15 @@ export class RateLimitError extends HttpError { } } +export class ServiceUnavailableError extends HttpError { + readonly code = 'SERVICE_UNAVAILABLE' + readonly status = 503 + + constructor(message = 'Service temporarily unavailable', context?: Record) { + super(message, context) + } +} + export class InternalError extends HttpError { readonly code = 'INTERNAL_ERROR' readonly status = 500 diff --git a/services/libs/common/src/index.ts b/services/libs/common/src/index.ts index f0db4a572f..cdfe9d07c9 100644 --- a/services/libs/common/src/index.ts +++ b/services/libs/common/src/index.ts @@ -14,6 +14,7 @@ import { HttpError, InsufficientScopeError, InternalError, + ServiceUnavailableError, NotFoundError, RateLimitError, UnauthorizedError, @@ -74,6 +75,7 @@ export { ConflictError, RateLimitError, InternalError, + ServiceUnavailableError, // Application errors ApplicationError, diff --git a/services/libs/data-access-layer/src/project-catalog/projectCatalog.test.ts b/services/libs/data-access-layer/src/project-catalog/projectCatalog.test.ts index ee77c7e139..e63b4e5d97 100644 --- a/services/libs/data-access-layer/src/project-catalog/projectCatalog.test.ts +++ b/services/libs/data-access-layer/src/project-catalog/projectCatalog.test.ts @@ -9,6 +9,7 @@ import { claimProjectCatalogForOnboarding, claimProjectCatalogForSlackEvaluation, finalizeProjectCatalogEvaluation, + deferProjectCatalogEvaluation, markProjectCatalogEvaluationFailed, requeueProjectCatalogEvaluation, findProjectCatalogById, @@ -756,6 +757,26 @@ describe('evaluation failures and retries', () => { expect(requeued?.evaluatedAt).toBeNull() }) + test('defers a transient failure to the next run until the last allowed run', async ({ qx }) => { + const inserted = await insertProjectCatalog(qx, catalogRow({ action: 'evaluate' })) + + const first = await deferProjectCatalogEvaluation(qx, inserted.id, 'API returned HTTP 503', 3) + expect(first).toMatchObject({ + action: 'evaluate', + evaluationAttempts: 1, + evaluationReason: 'API returned HTTP 503', + evaluatedAt: null, + }) + expect(await deferProjectCatalogEvaluation(qx, inserted.id, 'again', 3)).toMatchObject({ + evaluationAttempts: 2, + }) + expect(await deferProjectCatalogEvaluation(qx, inserted.id, 'and again', 3)).toBeNull() + + await markProjectCatalogEvaluationFailed(qx, inserted.id, 'and again') + const requeued = await requeueProjectCatalogEvaluation(qx, inserted.id) + expect(requeued?.evaluationAttempts).toBe(0) + }) + test('never touches rows outside evaluate or outside an evaluation error', async ({ qx }) => { const onboarded = await insertProjectCatalog(qx, catalogRow({ action: 'onboarded' })) const onboardingError = await insertProjectCatalog( diff --git a/services/libs/data-access-layer/src/project-catalog/projectCatalog.ts b/services/libs/data-access-layer/src/project-catalog/projectCatalog.ts index c95356093e..daddfee036 100644 --- a/services/libs/data-access-layer/src/project-catalog/projectCatalog.ts +++ b/services/libs/data-access-layer/src/project-catalog/projectCatalog.ts @@ -30,6 +30,7 @@ const PROJECT_CATALOG_COLUMNS = [ 'lfCriticalityScore', 'evaluationResult', 'evaluationReason', + 'evaluationAttempts', 'evaluatedAt', 'onboardedAt', 'onboardingError', @@ -702,6 +703,10 @@ export async function upsertProjectCatalogManualAction( WHEN EXCLUDED."action" IN ('auto', 'evaluate') THEN NULL ELSE "projectCatalog"."evaluatedAt" END, + "evaluationAttempts" = CASE + WHEN EXCLUDED."action" IN ('auto', 'evaluate') THEN 0 + ELSE "projectCatalog"."evaluationAttempts" + END, "classifiedAt" = CASE WHEN EXCLUDED."action" IN ('auto', 'evaluate') THEN NULL ELSE "projectCatalog"."classifiedAt" @@ -1028,7 +1033,7 @@ export async function requeueProjectCatalogEvaluation( ` UPDATE "projectCatalog" SET "action" = 'evaluate', "evaluationResult" = NULL, "evaluationReason" = NULL, - "evaluatedAt" = NULL, "updatedAt" = NOW() + "evaluationAttempts" = 0, "evaluatedAt" = NULL, "updatedAt" = NOW() WHERE id = $(id) AND "action" = 'error' AND "evaluationResult" = 'error' AND "onboardingError" IS NULL RETURNING ${prepareSelectColumns(PROJECT_CATALOG_COLUMNS)} @@ -1037,6 +1042,25 @@ export async function requeueProjectCatalogEvaluation( ) } +export async function deferProjectCatalogEvaluation( + qx: QueryExecutor, + id: string, + reason: string, + maxRuns: number, +): Promise { + return qx.selectOneOrNone( + ` + UPDATE "projectCatalog" + SET "evaluationAttempts" = "evaluationAttempts" + 1, "evaluationReason" = $(reason), + "updatedAt" = NOW() + WHERE id = $(id) AND "action" = 'evaluate' AND "evaluatedAt" IS NULL + AND "evaluationAttempts" + 1 < $(maxRuns) + RETURNING ${prepareSelectColumns(PROJECT_CATALOG_COLUMNS)} + `, + { id, reason, maxRuns }, + ) +} + export async function markProjectCatalogPreCheckSkipped( qx: QueryExecutor, id: string, diff --git a/services/libs/data-access-layer/src/project-catalog/types.ts b/services/libs/data-access-layer/src/project-catalog/types.ts index 3700e5e6b9..61791b0aab 100644 --- a/services/libs/data-access-layer/src/project-catalog/types.ts +++ b/services/libs/data-access-layer/src/project-catalog/types.ts @@ -148,6 +148,7 @@ export interface IDbProjectCatalog { lfCriticalityScore: number | null evaluationResult: string | null evaluationReason: string | null + evaluationAttempts: number evaluatedAt: string | null onboardedAt: string | null onboardingError: string | null