feat(selfhost): add a per-analyzer circuit breaker to review-enrichment - #2624
Conversation
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-02 21:30:23 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Closes #2541. review-enrichment's analyzer pipeline is correctly fail-safe with respect to the main review verdict, but individual analyzers with a third-party HTTP dependency (registry lookups, GitHub API calls, endoflife.date, etc) had no memory of recent failures -- every incoming enrichment request re-attempted a currently-unhealthy dependency from a cold state, even seconds after an identical call just timed out or errored. Adds a generic, per-analyzer, in-process circuit breaker (module-level Map, no persistence layer, matching src/selfhost/ai.ts's per-provider AI circuit breaker for the same class of problem): after 3 consecutive THROWN failures (a timeout counts -- it's a rejection from runWithTimeout), an analyzer is skipped entirely at planning time for a 5-minute cooldown, falling through the SAME plan.skipped mechanism every other skip reason already uses. A non-throwing degraded/partial result does NOT count as a failure -- the dependency responded, just not completely -- so it resets the streak rather than risk tripping the breaker on a benign internal cap unrelated to third-party health. Applies uniformly to every analyzer, including a future one with the same shape, not special-cased to one specific name. Also evaluated (per the issue's own ask) whether cost classes running strictly sequentially could be parallelized. Verified directly against the real orchestration in brief.ts (the deliberate for-loop over COST_ORDER, each class's bounded worker pool fully draining before the next starts) -- not implemented: cost-class ordering is intentional prioritization (cheap, certain signals collected before expensive, uncertain ones; a shrinking time budget correctly starves later classes first, never the reverse), and running every class concurrently would sum every class's concurrency limit at once, spiking third-party call volume exactly when third-party health is the concern this issue is about. Documented in place (brief.ts) rather than implemented, per the issue's own "or file as a documented follow-up" option.
43a21cf to
4b27b5f
Compare
…eaker The breaker had no half-open state: once the cooldown expired, a burst of concurrent requests (review-enrichment serves one per in-flight PR review) would all see the circuit as closed and all retry the same still-unhealthy dependency at once, defeating the point of the cooldown. isAnalyzerCircuitOpen now claims a single probe slot as a side effect the first time it observes an expired cooldown; every other concurrent caller sees the circuit as still open until that one probe resolves. A claimed probe that never actually reaches the analyzer call (budget/timeout capped in brief.ts first) is released via releaseAnalyzerCircuitProbe rather than staying stuck forever with no outcome ever recorded.
…streak threshold isAnalyzerCircuitOpen claimed the half-open probe slot for ANY existing circuit state, since cooldownUntilMs <= nowMs is also true at the initial cooldownUntilMs === 0 (never tripped). A circuit with only 1-2 recorded failures would spuriously skip a concurrent second caller as circuit_open, even though the breaker never actually opened. Gate the whole half-open branch on cooldownUntilMs !== 0, the same signal recordAnalyzerCircuitFailure already uses to mean "reached the threshold."
…r, after every other skip reason isAnalyzerCircuitOpen was checked FIRST, so it could claim the half-open probe for an analyzer that's about to be skipped for a totally unrelated reason (missing head SHA, no dependency manifest, no added lines, etc.). Since a plan.skipped item never reaches runAnalyzer -- the only place a claimed probe is released -- that claim would leak forever, permanently blocking every later request from probing the analyzer again even once healthy. Move the circuit check to the end, after every other skip condition has cleared, so the probe is only ever claimed when the analyzer would otherwise actually run.
Summary
Closes #2541.
review-enrichment's analyzer pipeline is correctly fail-safe with respect to the main review verdict — an enrichment timeout degrades review quality but never blocks the review. However, individual analyzers that depend on a third-party HTTP API (registry lookups, GitHub API calls, endoflife.date, etc.) had no memory of recent failures: every incoming enrichment request re-attempted a currently-unhealthy dependency from a cold state, even seconds after an identical call just timed out or errored.
What changed
review-enrichment/src/analyzer-circuit-breaker.ts: a generic, per-analyzer, in-process circuit breaker (module-levelMap, no persistence layer — review-enrichment is a single long-lived process). After 3 consecutive thrown failures (a timeout counts — it's a rejection fromrunWithTimeout), an analyzer trips its breaker and enters a 5-minute cooldown. Mirrors the shape ofsrc/selfhost/ai.ts's per-provider AI circuit breaker built for the same class of problem in a related roadmap issue (fix(selfhost): validate AI reviewer-provider configuration and add a failure circuit breaker #2540).scheduler.ts:skipReasonForAnalyzernow checksisAnalyzerCircuitOpenfirst, before any other check — a circuit-open analyzer is skipped at planning time (via the exact sameplan.skipped/skipReasonmechanism every other skip reason already uses) regardless of whether it was explicitly requested viareq.analyzers, since an explicit request can't fix a dependency that's actually down.brief.ts:runAnalyzerrecords a circuit-breaker success afterfindings[name] = result(covering BOTH a clean "ok" run and a non-throwing degraded/partial one — the dependency responded, just not completely, so this is not the failure mode the breaker guards against) and records a failure at the top of thecatchblock (a genuine thrown error or timeout).for (const cost of COST_ORDER) { await runWithConcurrency(...) }) — cost classes DO run strictly sequentially, each class's bounded worker pool fully draining before the next starts. This is deliberate prioritization (cheap/certain signals collected before expensive/uncertain ones; a shrinking time budget correctly starves later classes first, never the reverse), not an oversight. Parallelizing it would sum every class's concurrency limit at once (8+3+2+1+1 = 15 simultaneous external calls on the "deep" profile instead of at most 8), spiking third-party call volume exactly when third-party health is the concern this issue is about — not a low-risk change. Documented in place inbrief.tsrather than implemented, per the issue's own "or file as a documented follow-up" option.Correctness notes
scheduler.tscircuit-open check and removing thebrief.tsfailure-recording call each caused the corresponding tests to fail as expected (a 4th call reaching the "broken" analyzer instead of being skipped), then reverted.tscbuild, its ownnpm testchain —build && validate:sourcemaps && metadata:check && test:node) outsidesrc/**, so the main app's Codecov patch-coverage gate does not apply here; validated via the full localnpm run rees:test(the exact command CI runs) instead.Tests
review-enrichment/test/analyzer-circuit-breaker.test.ts(new): stays closed below the failure-streak threshold; opens and skips after 3 consecutive failures with zero further calls to the broken dependency; a timeout counts the same as a thrown error; a non-throwing partial result does NOT count as a failure; a success resets the streak so it doesn't bleed into a later, separate failure run; a circuit-expired analyzer is tried again rather than staying open forever; an explicitly-requested analyzer is still skipped while its circuit is open.Validation
npm run rees:test(from repo root — the exact command CI's review-enrichment job runs: install, build, validate-sourcemaps, metadata:check, then the fullnode --testsuite)npm run db:migrations:check(no-op — no schema changes; this PR is entirely scoped toreview-enrichment/)git diff --checkscheduler.test.ts's own proven-stable valueScope
site/,CNAME,**/lovable/**, orCHANGELOG.mdSafety