fix(review): stop the medium-effort retry storm burning tokens on zero output - #3747
Merged
Conversation
…o output Two compounding bugs caused reviews to burn tokens for zero usable output, repeatedly, without bound: 1. EFFORT_TIMEOUT_MS gave "medium" (the default reasoning effort) the same 120s CLI timeout as "low", while high/xhigh/max correctly scale up. A real medium-effort review on a non-trivial diff can exceed 120s and gets SIGKILLed mid-generation -- the tokens already spent are wasted, and no review is produced. Gives medium its own 180s tier instead. 2. surfaceRepairPriorityPullNumbers flags any PR missing a completed gate check for its current head SHA as outage repair, which deliberately bypasses the sweep's normal staleness throttle to retry fast -- but it has no memory of prior attempts. A PR whose repair keeps failing for the SAME head SHA (e.g. every attempt times out) got re-swept and re-billed on every ~2-minute sweep tick, forever. Caps it at 3 attempts per head SHA (via the existing audit_events ledger, no new table), after which the PR falls back to ordinary staleness-gated candidacy and a single audit event records the exhaustion instead of retrying silently forever. Confirmed live in production logs: 13 subscription_cli_timeout failures across 8 PRs in one 47-minute window, with the same PR re-dispatched by two separate repair jobs 6 minutes apart, each burning its own 3x120s retry loop.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3747 +/- ##
=======================================
Coverage 93.29% 93.30%
=======================================
Files 317 317
Lines 32347 32363 +16
Branches 11871 11873 +2
=======================================
+ Hits 30179 30195 +16
Misses 1537 1537
Partials 631 631
🚀 New features to boost your workflow:
|
This was referenced Jul 6, 2026
JSONbored
added a commit
that referenced
this pull request
Jul 6, 2026
…t to Sentry Lower REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA from 3 to 2 (#3747 follow-up) -- one fewer wasted attempt per stuck SHA before the sweep falls back to ordinary staleness-gated cadence. Also add a matching structured console.error at the moment a SHA exhausts its repair budget, so it flows through the existing forwardStructuredLogToSentry pipeline instead of being visible only via a direct audit_events query (level:"error" is deliberate -- the line's own existence IS the anomaly signal, same convention as selfhost_ai_provider_ failed / selfhost_ai_providers_exhausted).
JSONbored
added a commit
that referenced
this pull request
Jul 6, 2026
…t to Sentry Lower REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA from 3 to 2 (#3747 follow-up) -- one fewer wasted attempt per stuck SHA before the sweep falls back to ordinary staleness-gated cadence. Also add a matching structured console.error at the moment a SHA exhausts its repair budget, so it flows through the existing forwardStructuredLogToSentry pipeline instead of being visible only via a direct audit_events query (level:"error" is deliberate -- the line's own existence IS the anomaly signal, same convention as selfhost_ai_provider_ failed / selfhost_ai_providers_exhausted).
JSONbored
added a commit
that referenced
this pull request
Jul 6, 2026
…t to Sentry (#3760) Lower REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA from 3 to 2 (#3747 follow-up) -- one fewer wasted attempt per stuck SHA before the sweep falls back to ordinary staleness-gated cadence. Also add a matching structured console.error at the moment a SHA exhausts its repair budget, so it flows through the existing forwardStructuredLogToSentry pipeline instead of being visible only via a direct audit_events query (level:"error" is deliberate -- the line's own existence IS the anomaly signal, same convention as selfhost_ai_provider_ failed / selfhost_ai_providers_exhausted).
JSONbored
added a commit
that referenced
this pull request
Jul 6, 2026
prReadyForReview finalizes (runs a full paid AI review) once CI has been pending past its staleness cap, so a PR is never silently deferred forever. But it had no memory of having already done so: a CI context that will never settle (an orphaned required context, a fork check with no webhook to ever fire) hits this exact branch again on every later evaluation and re-spends another review for a disposition already established. Confirmed live: 3 PRs whose CI never settled were each re-reviewed 200-300+ times over more than 20 hours this way -- independent of the sweep's own outage-repair attempt cap (#3747), since ordinary (non-priority) sweep candidacy and live webhook re-evaluations both still reach this function. Caps it at one finalize per head SHA via a SHA-scoped audit event (reusing the existing ledger, no new table): once already finalized for the exact current head SHA, defer again instead of paying for another review. A new commit changes the head SHA, resetting the guard so a still-stuck PR finalizes fresh.
JSONbored
added a commit
that referenced
this pull request
Jul 6, 2026
) * fix(review): stop re-reviewing a PR whose CI is permanently stuck prReadyForReview finalizes (runs a full paid AI review) once CI has been pending past its staleness cap, so a PR is never silently deferred forever. But it had no memory of having already done so: a CI context that will never settle (an orphaned required context, a fork check with no webhook to ever fire) hits this exact branch again on every later evaluation and re-spends another review for a disposition already established. Confirmed live: 3 PRs whose CI never settled were each re-reviewed 200-300+ times over more than 20 hours this way -- independent of the sweep's own outage-repair attempt cap (#3747), since ordinary (non-priority) sweep candidacy and live webhook re-evaluations both still reach this function. Caps it at one finalize per head SHA via a SHA-scoped audit event (reusing the existing ledger, no new table): once already finalized for the exact current head SHA, defer again instead of paying for another review. A new commit changes the head SHA, resetting the guard so a still-stuck PR finalizes fresh. * fix(review): surface repeated CI-stuck review suppression to Sentry The DB-only audit_events write from the previous commit has no operator visibility until someone queries the ledger directly, the way this bug was originally found. Add a matching structured console.error at the exact moment the guard suppresses a repeat review, so it flows through the existing forwardStructuredLogToSentry pipeline (level:"error" is deliberate -- the line's own existence IS the anomaly signal, same convention as selfhost_ai_provider_failed / selfhost_ai_providers_exhausted). * test(review): cover the fail-open audit writes in the CI-stuck finalize guard Both new recordAuditEvent calls in the #orb-ci-stuck-repeat guard (review_finalized_ci_stuck_guard on first finalize, review_deferred_ci_pending on a guarded repeat) swallow their own write failures via .catch(() => undefined), but neither failure path had a test -- codecov flagged both lines as uncovered patch. Add two regression tests forcing the audit_events insert to throw in each case and asserting the surrounding finalize/defer decision still completes correctly despite the swallowed write failure.
This was referenced Jul 7, 2026
JSONbored
added a commit
that referenced
this pull request
Jul 7, 2026
The existing review-burst anomaly only counts SUCCESSFUL publish events, so a repeat-failure retry storm (every attempt inconclusive, never reaching a publish) is invisible to it -- the exact incident c707394 (#3747) fixed. Every AI review call already records an `inconclusive` boolean in ai_usage_events' metadata_json regardless of whether it published, so add a companion query keyed on that and wire it into the same anomaly detector, alongside the existing publish-only signal (kept as-is; it still catches a different, valid incident shape: excessive successful re-reviews of an unchanged PR). Closes #3926
8 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 7, 2026
…#3991) The existing review-burst anomaly only counts SUCCESSFUL publish events, so a repeat-failure retry storm (every attempt inconclusive, never reaching a publish) is invisible to it -- the exact incident c707394 (#3747) fixed. Every AI review call already records an `inconclusive` boolean in ai_usage_events' metadata_json regardless of whether it published, so add a companion query keyed on that and wire it into the same anomaly detector, alongside the existing publish-only signal (kept as-is; it still catches a different, valid incident shape: excessive successful re-reviews of an unchanged PR). Closes #3926
3 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 7, 2026
…+ panel (#4043) runOpsAlerts (src/review/ops-wire.ts) already scanned gittensory's own outcome data hourly and emitted a structured ops_anomaly log on a review burst or review failure burst (the #3747 incident shape: a stuck-CI finalize loop or retry storm). Nothing surfaced this on /metrics, so an operator had to grep Workers Logs instead of alerting. Adds gittensory_ops_anomaly_total (counter, labeled by kind + repo, repo redacted on the cloud worker same as gittensory_gate_decisions_total), a GittensoryOpsAnomalyDetected Prometheus alert, and a Grafana panel pair on the self-host observability stack.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EFFORT_TIMEOUT_MSgivesmedium(the default reasoning effort) the same 120s CLI timeout aslow, so a real medium-effort review on a non-trivial diff can get SIGKILLed mid-generation. That PR's head SHA never gets a completed gate check, whichsurfaceRepairPriorityPullNumbers(queue/processors.ts) reads as an outage and deliberately bypasses the sweep's staleness throttle to repair fast -- but it has no memory of prior attempts, so a PR whose repair keeps failing the same way gets re-swept and re-billed on every ~2-minute sweep tick, forever.mediumits own 180s timeout tier instead of sharinglow's; (2) cap the outage-repair bypass at 3 attempts per exact head SHA (via the existingaudit_eventsledger, no new table/migration), after which the PR falls back to ordinary staleness-gated candidacy and one exhausted-audit-event is recorded instead of retrying silently forever.subscription_cli_timeoutfailures across 8 PRs in one 47-minute window, with the same PR (feat(enrichment): empty-catch / error-swallow analyzer #3532) re-dispatched by two separate repair jobs 6 minutes apart, each burning its own 3x120s retry loop.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlint-- not run (no workflow files touched)npm run typecheck-- cleannpm run test:coverage(full/unsharded) -- not run locally; ran the two affected files directly instead (test/unit/selfhost-ai.test.ts126/126 passed,test/unit/queue.test.ts586/586 passed incl. 2 new regression tests) plus a scoped--coveragepass confirming the new branches are exercised (the two structurally-unreachable defensive guards arev8 ignored with an inline rationale, matching existing house convention for the same pattern elsewhere in this file). Trusting CI for the full unsharded gate given this is a live-incident fix.npm run test:workers/npm run build:mcp/npm run test:mcp-pack/npm run ui:openapi:check/npm run ui:lint/npm run ui:typecheck/npm run ui:build/npm audit --audit-level=moderate-- not run locally (no wrangler/MCP/UI/dependency surface touched by this diff); left to CI.queue.test.tscover (a) the attempt cap kicking in and demoting a stuck SHA back to ordinary cadence with exactly one exhausted-audit-event, and (b) an under-cap dispatch recording an attempt, plus no duplicate exhausted-event across repeated ticks once already flagged.Safety
Notes
CLAUDE_AI_TIMEOUT_MS=240000env override and a temporaryCRON_INTERVAL_MS=900000sweep-cadence brake are already live on the VPS as stop-gap mitigations while this PR lands; both should be reverted once this ships (the code fix makes them unnecessary, and the cadence brake in particular trades off sweep/auto-merge responsiveness that should be restored).