[improvement](workflow) Keep only latest code review per PR - #67119
[improvement](workflow) Keep only latest code review per PR#67119hello-stephen wants to merge 2 commits into
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: Cancel an older Code Review Runner job when a newer review is requested for the same PR. Keep concurrency at job scope so unrelated skipped issue comments do not cancel active reviews, and require a fresh review request instead of rerunning an older workflow attempt. ### Release note None ### Check List (For Author) - Test: Manual test - Parsed the workflow with Ruby/Psych - Ran actionlint and confirmed only the four pre-existing shellcheck findings remain - Verified the PR-keyed concurrency and first-attempt policy assertions - Behavior changed: Yes. Only the latest review request for a PR keeps running. - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: N/A Related PR: apache#67119 Problem Summary: Extend per-PR latest-run concurrency from Code Review Runner to License Check, FE Code Style Checker, Code Formatter, and Gitleaks PR Check. PR jobs share a stable PR-number group, while non-PR License Check runs use a unique run id and remain independent. ### Release note None ### Check List (For Author) - Test: Manual test - Parsed all five workflows with Ruby/Psych - Ran actionlint and confirmed only existing baseline findings remain - Verified all five PR-number concurrency groups and cancel-in-progress policies - Behavior changed: Yes. New PR runs cancel older jobs from these workflows for the same PR. - Does this need documentation: No
|
/review |
|
skip buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes. I reviewed all five workflow changes, every declared trigger/guard, the Code Review Runner finalization tail, the status-sync workflow, recent rerun/dispatch behavior, and a concrete canceled runner execution. Two review rounds converged with no new findings; all candidates were adjudicated, and the six accepted issues are covered by five inline threads.
Critical checkpoint conclusions:
- Goal and proof: Canonical, normally ordered first-attempt jobs do coalesce, but the change does not guarantee that the newest logical request or current-head validation survives. YAML parsing and the PR's green checks prove basic syntax/execution only.
- Scope: The diff is small and focused, but Code Review Runner cancellation necessarily reaches unchanged failure, status, telemetry, session, and credential finalizers; those interactions are part of the feature's correctness.
- Concurrency: GitHub Actions scheduling is the concurrency source; PR reviews/comments/statuses and shared OSS auth are the mutable state. There are no Doris process threads or locks here. Historical reruns, reverse first-attempt admission, raw group keys, and mixed status ownership leave six correctness/lifecycle gaps.
- Lifecycle and error handling: Expected supersession is treated as a genuine failure, and optional finalizers can consume the forced-cancellation window before required auth persistence. Input/head/base mismatches otherwise fail loudly.
- Parallel paths and conditions: All four validation workflows, issue comments, manual dispatch, reusable-call scope, skipped events, and License master pushes were checked. Unique prefixes and run-ID fallbacks safely isolate unrelated paths; the accepted issues cover the remaining exceptions.
- Tests and results: All five YAML files parse, and current checks are green. There is no end-to-end coverage for overlapping same-PR runs, historical attempts, cancellation cleanup, noncanonical manual input, mixed status ownership, or reversed admission.
- Compatibility, persistence, and data: No Doris runtime configuration, FE/BE propagation, storage/protocol, transaction/EditLog, or data-write behavior changes. GitHub status/comment/review state and OSS auth persistence are the relevant external state and are covered inline.
- Performance and observability: Supersession can save runner time, but optional cleanup may hold cancellation for nearly five minutes, and the permanent false failure comment is misleading observability. Existing run IDs/logs are otherwise sufficient.
No additional user-provided review focus was supplied. Review status: complete and converged after Round 2.
| runs-on: ubuntu-latest | ||
| concurrency: | ||
| group: fe-code-style-${{ github.event.pull_request.number || github.run_id }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
[P1] Keep historical reruns out of the current-PR group
These groups are keyed only by PR number, so rerunning an older workflow attempt after head B's check starts lets the old attempt join the same group and cancel (or replace while pending) B's validation. GitHub reruns retain the original GITHUB_SHA/GITHUB_REF, so the replacement still checks head A and cannot restore validation for B. The same issue is present in the new Clang Formatter, Gitleaks, and License Check blocks. Please isolate github.run_attempt > 1 with a run-unique group such as github.run_id; changing only cancel-in-progress does not protect a pending current-head job because the default queue replaces an existing pending member.
| runs-on: ubuntu-latest | ||
| concurrency: | ||
| group: code-review-${{ inputs.pr_number || github.event.issue.number || github.run_id }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
[P1] Make expected supersession cancellation-safe
Once a running review has resolved its inputs, a newer request cancels the review step, but the existing always() path treats steps.review.outcome == 'cancelled' as a genuine error. Canceled Doris run 31655483365 concretely posted "Review step was cancelled" to PR 66473 and reset the status to pending. The same run then spent the rest of GitHub's five-minute cancellation window in Litefuse and was killed before session or refreshed-auth sync began; commit 296b8f2bc0d explicitly requires that auth write-back after unrelated review failure because Codex may already have refreshed the credential. Please exclude cancellation from genuine-failure reporting and give refreshed auth a strictly bounded early persistence path (or cooperative/external handoff) before optional telemetry/session work.
| jobs: | ||
| code-review: | ||
| runs-on: ubuntu-latest | ||
| concurrency: |
There was a problem hiding this comment.
[P1] Preserve status ownership across manual supersession
This shared group includes issue-comment runs, which always manage the code-review status, and manual dispatches where manage_status can be set to false. A false-status dispatch can cancel /review after it posts pending, complete its own review, and skip both terminal status writes; code-review-sync-result.yml has no review-completion trigger to repair the stranded status. Please require every replacement admitted to this group to take over status management, or isolate non-status-managing runs until there is an explicit ownership handoff.
| code-review: | ||
| runs-on: ubuntu-latest | ||
| concurrency: | ||
| group: code-review-${{ inputs.pr_number || github.event.issue.number || github.run_id }} |
There was a problem hiding this comment.
[P2] Canonicalize the PR number before admission
inputs.pr_number is a raw string here, and GitHub accepts equivalent forms such as 67119, 067119, and 00067119 for the same pull request. Those supported dispatch/call inputs create different group names, so they can run in parallel with each other or with /review instead of coalescing per PR. Please strictly canonicalize and validate the numeric identifier in a prerequisite resolver, then key this job from its canonical output.
| # the latest review for the same PR. | ||
| if: >- | ||
| inputs.pr_number != '' || | ||
| github.run_attempt == 1 && |
There was a problem hiding this comment.
[P1] Fence by logical request order, not concurrency admission order
run_attempt == 1 excludes historical reruns, but two fresh requests still both pass. GitHub documents that concurrency is ordered by when a job starts waiting for the group, not workflow dispatch/event time, and ordering is not guaranteed. If older request A reaches this job after newer B, A becomes the latest entrant and cancels B, preserving A's older focus or explicit head/base inputs. Please add a monotonic latest-request ownership check/fence rather than treating scheduler admission order as request freshness.
1 participant
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
Several high-frequency PR workflows can keep obsolete jobs queued or running after a newer event arrives for the same PR. This wastes GitHub-hosted runner capacity and increases queue latency.
This PR applies per-PR latest-run concurrency to all five measured workflows:
Code Review RunnerLicense CheckFE Code Style CheckerCode FormatterGitleaks PR CheckEach eligible PR job uses the PR number as its concurrency key and enables
cancel-in-progress. The concurrency is job-scoped, so skipped jobs and unrelated events cannot cancel an active PR check.License Checkpush runs use the unique run id and remain independent.Code Review Runneradditionally requires a first workflow attempt. A failed or canceled review is retried with a fresh/review, preventing a manual rerun of an older workflow from replacing a newer logical review request.Measured effect
The sample covers the last three complete workdays in CST: 2026-08-20, 2026-08-21, and 2026-08-24.
Methodology:
/reviewcomment actor and timestamp;pull_requestsarray;Most later same-PR runs were sequential checks after the preceding job had completed and are intentionally not counted as waste. The 51 actual overlaps would have reclaimed approximately 276 runner minutes (4.60 hours).
One overlapping old Code Review workflow was manually rerun and consumed another 37.8 minutes. Requiring a fresh
/reviewremoves that stale-rerun path, raising the modeled total to 313.8 minutes (5.23 hours) for this sample.The long Code Review jobs account for most of the direct runner-time saving. The four shorter checks mainly avoid queued obsolete jobs during bursts, which reduces queue pressure even though their direct minute saving is smaller.
Release note
None
Check List (For Author)
Test
actionlint; findings are limited to the same four Code Review shellcheck findings and the same CheckStyleactions/checkout@v3warning present onorigin/master.cancel-in-progresspolicies.Behavior changed:
/reviewrequest.Does this need documentation?
Check List (For Reviewer who merge this PR)