fix(scheduler): lengthen scan-pr-queue's own heartbeat, don't drop it - #1704
Merged
Merged
Conversation
org-queue-sweep explicitly excludes ContextualWisdomLab/.github from its target list, so scan-pr-queue's own cron is the sole periodic fallback for this repository's PR queue, and for any required check (Security Scan, SAST Semgrep) with no workflow_run listener anywhere in this file. Deleting it would leave this repository strictly worse off than every sibling repo, which still gets the hourly org-sweep. Apply the same lever #1630 already used for org-queue-sweep: lengthen the cron from */30 (every 30 min) to hourly, offset to "30 * * * *" so it doesn't collide with org-queue-sweep's "0 * * * *" tick. Document why the entry exists and why it cannot simply be removed, the way the adjoining org-queue-sweep cron already documents its own rationale. Add a cadence-contract test asserting the new schedule, and refresh a stale docstring in test_required_workflow_queue_contract.py that referred to "the separate 30-minute scan". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+92
to
95
| - cron: "30 * * * *" | ||
| # Hourly org-wide sweep cadence for the org-queue-sweep job below. Target | ||
| # repositories only receive scheduler runs on PR events, review/security | ||
| # workflow completion, and protected-branch pushes; a PR whose approval or |
Contributor
There was a problem hiding this comment.
📝 Info: Schedule routing remains mutually exclusive
The 30 * * * * event runs only the repository scan; 0 * * * * remains reserved for the organization sweep. Their concurrency keys also differ.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+47
to
+53
| workflow = WORKFLOW.read_text(encoding="utf-8") | ||
| assert '- cron: "30 * * * *"' in workflow | ||
| assert '*/30 * * * *' not in workflow | ||
| schedule_block = workflow.split(" schedule:", 1)[1].split( | ||
| " repository_dispatch:", 1 | ||
| )[0] | ||
| assert schedule_block.count('- cron:') == 2 |
Contributor
There was a problem hiding this comment.
📝 Info: Text test matches narrow contract
This test does not prove job routing, but existing contract tests pin both schedule guards. It detects heartbeat removal, cadence reversal, and cron-count drift.
Was this helpful? React with 👍 or 👎 to provide feedback.
This was referenced Sep 2, 2026
seonghobae
added a commit
that referenced
this pull request
Sep 2, 2026
…l-failure fix The org's own automated PR review/fix loop had already reconciled this branch with main once (commit c946c7b), but that reconciliation silently regressed the actual headRefOid fix back to the pre-fix buggy pattern (str(pr.get("headRefOid") or "").lower()) in stale_pr_run_ids() and active_review_run_refs(), and lost the entire live-revalidation safety net (_direct_pr_run_still_superseded, _review_run_still_superseded, _cancel_revalidated_review_run_refs). Its own attempt to push a corrected commit then failed closed (correctly) when its configured push credential was unavailable, leaving the branch stuck in the regressed state. This commit resolves a fresh merge of current main (with #1707/#1702/#1704/ #1711/#1712 all applied) directly against a37a428 -- this branch's last verified-good commit (100% coverage, 2614 tests, real regression tests reproducing the naruon PR #1528 incident) -- rather than building on top of the already-regressed c946c7b. Combines both fixes at every cancellation call site (cancel_stale_pr_runs, cancel_stale_opencode_runs, _cancel_revalidated_review_run_refs): PR #1669's live revalidation immediately before each destructive cancel call (closing the TOCTOU gap a snapshot-only headRefOid check can't), and #1712's check of force_cancel_workflow_runs's actual per-run failure result (so a run proven stale but whose cancel API call GitHub itself rejected is never reported as cancelled). #1712's simpler force_cancel_workflow_run_refs wrapper is removed as dead code now that its call sites all use the more thorough per-caller revalidation; its own tests were adapted to target the functions that actually carry its safety guarantee forward, not deleted. Verified: PYTHONPATH=. coverage run -m pytest tests -- 2621 passed, 1 skipped, 21 subtests; coverage report -- 100% on scripts/ci; interrogate -- 100% docstrings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
pushed a commit
that referenced
this pull request
Sep 2, 2026
… main exact-head-path-policy failed on this branch's own copy of scripts/ci/test_strix_quick_gate.sh for two unrelated stale assertions, neither touching this PR's actual Strix evidence-hardening diff: 1. The LLM_TIMEOUT assertion (line 302) still expected the round-6 'export LLM_TIMEOUT=300' value this PR itself introduced on 2026-09-01 to match #1601's contemporary state. Main later reverted strix.yml back to 'export LLM_TIMEOUT=0' via #1658 ("remove the 300s LLM_TIMEOUT cap") without ever having carried the 300 assertion on main's own copy of this file, so a same-line 3-way merge always kept this branch's now-stale text with no conflict to surface it. Restored the assertion to match main's (and strix.yml's) current, unchanged content. 2. The scheduler-heartbeat cron assertion (line 1562) still expected the pre-#1704 'cron: "*/30 * * * *"' quarter-hourly schedule. #1704 ("lengthen scan-pr-queue's own heartbeat, don't drop it") lengthened pr-review-merge-scheduler.yml's repository-local scan to hourly ('cron: "30 * * * *"') for the same Actions-capacity reason as #1630, and added/updated the matching pytest contract (tests/test_actions_queue_saturation_scheduler_cadence.py, tests/test_required_workflow_queue_contract.py) but missed this repo's separate, duplicate shell-harness assertion of the same contract. Confirmed this exact failure reproduces identically on fresh main (same stale assertion, same actual hourly cron) -- it predates and is unrelated to this PR's diff. Updated the assertion to match #1704's now-current cron and added the mirroring assert_file_not_contains for the retired quarter-hourly string, same pattern #1704 already established in its own pytest contract. Verified on the merged head (origin/main merged in via the preceding merge commit, mergeable_state was "behind" only, no conflicts): - bash scripts/ci/test_strix_quick_gate.sh (full harness): PASS, 0 failures (previously 2: the LLM_TIMEOUT and cron assertions above). - PYTHONPATH=. python3.12 -m coverage run -m pytest tests -q: 2644 passed, 1 skipped, 21 subtests. - coverage report --show-missing: 100% on scripts/ci. - interrogate: 100% (RESULT: PASSED, minimum: 100.0%, actual: 100.0%). - python -m compileall on the five exact-head-path-policy test files, bash -n scripts/ci/strix_quick_gate.sh, git diff --exit-code: all clean after this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
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
1 participant
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
Per the org owner's request to review
scan-pr-queue's schedule frequency, and this session's investigation:org-queue-sweepexplicitly filtersContextualWisdomLab/.githubout of its target list (select(.full_name != "ContextualWisdomLab/.github")), soscan-pr-queue's own*/30 * * * *cron is the sole periodic fallback for this repository's own PR queue — including PRs to the governance workflows themselves.Security ScanandSAST Semgrep(both required central workflows perscripts/ci/audit_central_required_workflows.py) have noworkflow_runlistener anywhere in this file, so a PR where either lands last as the final required check has no event-driven re-wake in any repository — only a schedule ever notices it.docs/doctoring/actions-queue-saturation-hourly-sweep.md(#1630, 2026-09-01) already applied the "lengthen, don't remove" lever toorg-queue-sweep's own cadence for the same Actions-capacity reason, and explicitly keptscan-pr-queue's 30-minute cron untouched at the time.Verdict: keep, but lengthen — same lever, same rationale, one day later. This PR lengthens
scan-pr-queue's cron from*/30 * * * *to hourly ("30 * * * *", offset fromorg-queue-sweep's"0 * * * *"so the two heartbeats don't collide), and adds a comment next to the entry explaining why it exists and why it can't simply be dropped — mirroring how the adjoiningorg-queue-sweepcron already documents itself.test_scan_pr_queue_heartbeat_is_hourly_and_offset_not_removedintests/test_actions_queue_saturation_scheduler_cadence.py.tests/test_required_workflow_queue_contract.py.*/30value (checked every test file referencingpr-review-merge-scheduler.yml).Note on a concurrent sibling PR
This is a sibling, independent fix to a separate concurrent PR that adds
timeout-minutesto the samescan-pr-queuejob — a different, non-overlapping part of the file. A routine rebase may be needed against whichever merges first. Per standing backlog item 31, this PR's own required checks are themselves subject to the very Actions-capacity incident it helps fix, so — same chicken-and-egg situation as that sibling PR — a bypass-merge may be warranted here too. I am not merging this myself; that decision belongs to the org owner/maintainers.Test plan
actionlint .github/workflows/pr-review-merge-scheduler.yml— cleanpytest tests/test_actions_queue_saturation_scheduler_cadence.py tests/test_required_workflow_queue_contract.py— 76 passedpyteston every other test file referencingpr-review-merge-scheduler.yml(test_central_required_workflow_ruleset_audit.py,test_close_empty_pr_queue_pressure.py,test_merge_scheduler_runner_image_contract.py,test_opencode_agent_contract.py,test_opencode_workflow_shell_syntax.py,test_organization_commercial_readiness_loop_policy.py,test_queue_cancellation_scheduler_contract.py) — 102 passed🤖 Generated with Claude Code