feat: retry transient evaluation errors within and across runs before recording them (lfx-cdp-insights-ops#215)

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<typeof import('@crowd/common')>()),
   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)

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',

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;

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<void> {
   }
 }
 
-export async function markEvaluationFailed(projectId: string, reason: string): Promise<boolean> {
+export type EvaluationFailureRecord = 'failed' | 'deferred' | 'moved'
+
+async function deferEvaluation(
+  qx: QueryExecutor,
+  projectId: string,
+  reason: string,
+): Promise<boolean> {
+  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<EvaluationFailureRecord> {
   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

services/apps/projects_evaluation_worker/src/activities/classifyAndRoute.test.ts

@@ -21,6 +21,7 @@ function project(overrides: Partial<IDbProjectCatalog> = {}): IDbProjectCatalog
     lfCriticalityScore: null,
     evaluationResult: null,
     evaluationReason: null,
+    evaluationAttempts: 0,
     evaluatedAt: null,
     onboardedAt: null,
     onboardingError: null,

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)
+  })
+})

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<IDbProjectCatalog, 'provenance' | 'evaluationAttempts'>
+
+export function canDeferEvaluation(row: RetryRow): boolean {
+  return !isSlackBotProvenance(row.provenance) && row.evaluationAttempts + 1 < MAX_EVALUATION_RUNS
+}

services/apps/projects_evaluation_worker/src/activities/immediateOnboarding.test.ts

@@ -17,6 +17,7 @@ function row(overrides: Partial<IDbProjectCatalog>): IDbProjectCatalog {
     lfCriticalityScore: null,
     evaluationResult: null,
     evaluationReason: null,
+    evaluationAttempts: 0,
     evaluatedAt: null,
     onboardedAt: null,
     onboardingError: null,