Repository navigation
feat: retry transient evaluation errors within and across runs before recording them (lfx-cdp-insights-ops#215) - #5008
Conversation
… recording them Refs linuxfoundation/lfx-cdp-insights-ops#215 Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
PR SummaryMedium Risk Overview The evaluation worker treats API 429/5xx and fetch failures as Also adds a warn-only log when the LLM says “not mainly run on GitHub” but the repo has 100+ closed PRs (verdict unchanged). Requeue/manual re-eval resets Reviewed by Cursor Bugbot for commit 9a54a6c. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
🟡 Changes recommended
Retry bookkeeping, timeout handling, execution budgets, and premature API alerts undermine the intended bounded-retry behavior.
4 open findings
What changed in this PR
Adds bounded retries for transient project-evaluation failures, reducing premature catalog errors while preserving terminal verdicts.
Changes:
- Returns HTTP 503 for transient failures and retries evaluations within runs.
- Tracks bounded cross-run retries, with manual resets and bot-specific handling.
- Adds retry tests and logs contradictory GitHub-activity verdicts without overriding them.
| File | Description |
|---|---|
| services/libs/data-access-layer/src/project-catalog/types.ts | Adds evaluation attempt count. |
| services/libs/data-access-layer/src/project-catalog/projectCatalog.ts | Adds deferral updates and counter resets. |
| services/libs/data-access-layer/src/project-catalog/projectCatalog.test.ts | Tests deferral limits and resets. |
| services/libs/common/src/index.ts | Exports service-unavailable error. |
| services/libs/common/src/errors/http.ts | Defines HTTP 503 error. |
| services/apps/projects_evaluation_worker/src/workflows/evaluateProjects.ts | Adds retries and deferred-result handling. |
| services/apps/projects_evaluation_worker/src/evaluator/retry.ts | Defines retry constants and status classification. |
| services/apps/projects_evaluation_worker/src/evaluator/evaluator.ts | Throws typed transient failures. |
| services/apps/projects_evaluation_worker/src/evaluator/evaluator.test.ts | Tests transient and terminal responses. |
| services/apps/projects_evaluation_worker/src/activities/immediateOnboarding.test.ts | Updates catalog fixture. |
| services/apps/projects_evaluation_worker/src/activities/evaluationRetry.ts | Determines cross-run retry eligibility. |
| services/apps/projects_evaluation_worker/src/activities/evaluationRetry.test.ts | Tests retry limits and bot exclusion. |
| services/apps/projects_evaluation_worker/src/activities/classifyAndRoute.test.ts | Updates catalog fixture. |
| services/apps/projects_evaluation_worker/src/activities/activities.ts | Defers eligible evaluation failures. |
| backend/src/database/migrations/V1792160000__project-catalog-evaluation-attempts.sql | Adds persisted attempt counter. |
| backend/src/api/public/v1/projectEvaluation/evaluateProject.ts | Classifies transient errors and logs contradictions. |
| backend/src/api/public/v1/projectEvaluation/evaluateProject.test.ts | Tests classification and contradiction logging. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (isTransientEvaluationError(err)) { | ||
| throw new ServiceUnavailableError(getErrorMessage(err)) |
| const evaluateActivities = proxyActivities<typeof activities>({ | ||
| startToCloseTimeout: '3 minutes', | ||
| retry: { maximumAttempts: 2 }, | ||
| retry: { maximumAttempts: 3, initialInterval: '30 seconds', backoffCoefficient: 2 }, |
| if (current instanceof ApplicationFailure && current.type === TRANSIENT_EVALUATION_FAILURE) { | ||
| return true | ||
| } |
| SET "evaluationAttempts" = "evaluationAttempts" + 1, "evaluationReason" = $(reason), | ||
| "updatedAt" = NOW() | ||
| WHERE id = $(id) AND "action" = 'evaluate' AND "evaluatedAt" IS NULL | ||
| AND "evaluationAttempts" + 1 < $(maxRuns) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9a54a6c. Configure here.
| const qx = pgpQx(svc.postgres.writer.connection()) | ||
| if (transient && (await deferEvaluation(qx, projectId, reason))) { | ||
| return 'deferred' | ||
| } |
There was a problem hiding this comment.
Defer increment is not idempotent
Medium Severity
deferProjectCatalogEvaluation always does evaluationAttempts + 1, and markEvaluationFailed still uses the mark activity retry policy (maximumAttempts: 3). If Temporal retries after the increment commits (worker crash, lost completion, overlapping runs), one transient failure can count as two. On the second failure that is especially bad: a retry sees evaluationAttempts = 2, skips defer, and finalizes the row as error with an alert, so the third run never happens. The old mark-as-error write was idempotent; this counter is not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9a54a6c. Configure here.
Summary
Transient evaluator failures (GitHub or Bedrock not answering, throttling, network, the daily cap) no longer finalize a catalog row on the first attempt. They are retried within the run and, for scheduled rows, across runs with a bound; only then does the row become
errorwith the existing alert. Verdict-level errors (missing config, bad GitHub token, retired model, malformed LLM answer) stay terminal as today.evaluateProjectclassifies the failure:GithubPublicClientErrorof kindTRANSIENTorRATE_LIMIT, BedrockThrottlingException/ServiceUnavailableException/ModelNotReadyException/ModelTimeoutException/InternalServerException, AWS$fault: 'server'or$retryable, HTTP 429/5xx metadata, andECONNRESET-style network codes throw the newServiceUnavailableError(503,@crowd/common). Everything else keeps theevaluationResult: 'error'response.TransientEvaluationFailureon a rejected fetch, HTTP 429 or 5xx (with the reason from the API body); the evaluate activity now retries 3 times with a 30s/60s backoff. The daily cap (Daily evaluation limit reached) throws it as non-retryable so no attempt is wasted in the same run.markEvaluationFailed(id, reason, transient): a discovery or API row that has not reachedMAX_EVALUATION_RUNS(3) stays inevaluatewithevaluationAttempts + 1and the last reason, and the next run picks it up; the run log counts it asdeferred, no alert. After the third failed run the row becomeserrorwith the alert, as today. Bot rows are never deferred: they geterrorand the Try again button at once, as today. Try again and the manual API reset the counter.Migration
V1792160000__project-catalog-evaluation-attempts.sqladdsevaluationAttempts SMALLINT NOT NULL DEFAULT 0toprojectCatalog(no index).Closes linuxfoundation/lfx-cdp-insights-ops#215
Test plan
pnpm tsc-check,pnpm lint --deny-warningsevaluateProject(503 on transient GitHub/Bedrock errors, terminal errors unchanged, contradiction log), worker evaluator client (typed failure per status, 400 stays a verdict error),canDeferEvaluation, DALdeferProjectCatalogEvaluation(CI)apiandprojects-evaluation-workerwith noevaluateProjectsrun in flightevaluatewithevaluationAttempts = 1anddeferred=Nin the run log, no alert; a bot request under the same conditions still getserror/unsurewithevaluationResult = 'error'can be requeued with the SQL from the issue thread and are evaluated by the next run🤖 Generated with Claude Code