fix(review): rate-limit the ci_stuck_review_repeat_suppressed log (#4998) - #5037
Merged
Conversation
) The log line that announces a repeat suppression fired as its own console.error on EVERY suppressed evaluation, not once per incident -- 649 events over 4 days. The underlying suppression (capping the finalize/review spend to once per head SHA) was correct; only the observability side never got the same treatment. Rate-limit the log to once per (repo, pr, headSha) per day via the existing self-host transient-cache helpers; the defer itself is untouched and still runs on every evaluation.
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 #5037 +/- ##
=======================================
Coverage 94.36% 94.36%
=======================================
Files 459 459
Lines 39285 39291 +6
Branches 14329 14331 +2
=======================================
+ Hits 37072 37078 +6
Misses 1558 1558
Partials 655 655
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
ci_stuck_review_repeat_suppressed— a log line that exists specifically to announce "I am suppressing a repeat notification" — fired as its ownconsole.error(Sentry-forwarded) on every single suppression: 649 events over 4 days, escalating.#orb-ci-stuck-repeatguard (prReadyForReview,src/queue/processors.ts) correctly caps the expensive part (re-running a full paid AI review) to once per head SHA, but the log announcing that cap re-fires on every later evaluation of the same still-stuck PR — a webhook re-trigger or a sweep pass, at roughly a 5-10 minute cadence based on the raw event timestamps.(repo, pr, headSha)per day, via the sameSELFHOST_TRANSIENT_CACHE-backed helper pattern already used byciPendingDeferStuck/ciReReviewCoalescedin this file (getTransientKey/putTransientKey). The underlying defer/suppression behavior is untouched — it still runs on every evaluation; only the Sentry-visible log is coalesced. A new head SHA (a fresh commit) is a fresh key, so a genuinely new incident is never silently swallowed by an old PR's coalesce window.Investigation: what's actually wrong with the stuck PR(s) (issue requirement #2)
Pulled the raw event list behind this Sentry issue rather than trusting its title/culprit summary, since
forwardStructuredLogToSentrygroups everyci_stuck_review_repeat_suppressedoccurrence into one Sentry issue regardless of repo/PR/headSha (grouped by the fixed message template) — the issue's displayed "(JSONbored/awesome-claude#4816) headSha=..." is just the latest contributing event, not a single PR stuck for 4 days as the issue title implied. Confirmed: PR awesome-claude#4816 itself was only open ~77 minutes end-to-end (created 04:44 UTC, merged 06:01 UTC the same day) — not stuck at all.The real 649-event history spans several distinct PRs across
gittensoryandawesome-claude(at least #4343, #4344, #4346, #4349, #4371 in gittensory and #4739, #4745, #4816 in awesome-claude, from a 25-event sample), each re-evaluated repeatedly at a ~5-10 minute cadence while stuck — the exact "steady few-minute cadence" burn pattern the original#orb-ci-stuck-repeatguard's own code comment describes, just now visible as noisy logging instead of wasted AI spend.Checked the most-repeated one, gittensory#4349, directly: it carries a non-required
Contributor trustcheck (a third-party Action,superagent.sh) that resolved to conclusionACTION_REQUIREDin 3 seconds — butmain's actual branch-protection required-status-checks list is only["Superagent Security Scan", "validate"], so that check is not itself the blocker. All sampled PRs eventually merged cleanly once a later commit landed or a required check finally completed, so this is not a permanent hang — it's multiple PRs sitting through a slow-to-settle CI run (or a required check needing a manual fork-Action approval) for anywhere from tens of minutes to a few hours each, during which the sweep/webhook cadence re-evaluates and re-logs. No code change beyond this PR's rate-limit is proposed for that underlying CI-settling latency; it's a candidate follow-up (documented in the parent observability epic) rather than a fix bundled here.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #4998).Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocallynpm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
test:coverage(full unsharded): not run end-to-end — ran scopedvitest --coveragefortest/unit/queue.test.ts(159 tests) and confirmed via lcov that every changed line and both branches of the new coalescing guard are covered.actionlint/test:workers/build:mcp/test:mcp-pack/ui:openapi:check/ui:lint/ui:typecheck/ui:build/npm audit: not run — this change touches onlysrc/queue/processors.ts(an existing transient-cache-backed guard, no new API/schema/binding/dependency surface) and its tests; no workflow, MCP, UI, or dependency-manifest surface changed.Safety
UI Evidencesection. (N/A.)Notes
Part of a batch of 13 bug fixes filed from a Sentry-issue triage this session (#4994–#5006). This is #5 by priority.