Repository navigation
Conversation
One author opening or pushing many pull requests could fill the runner queue and delay everyone else. Each PR-triggered workflow now uses a concurrency group keyed on the workflow and the actor, so an author has one run in progress per workflow while the rest wait in order (queue: max) rather than being cancelled. Pushes to main, scheduled runs and manual dispatches fall back to the run ID and are never held back. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Group pull request runs by the PR author instead of the triggering actor, so a maintainer pushing to or updating contributors' branches does not pile those runs into the maintainer's queue. Bot-authored PRs fall back to the run ID so a Dependabot batch is not serialized. Give every job in the queued workflows a timeout of roughly three times its longest recent run, so a hung job cannot hold an author's queue for GitHub's six-hour default. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Reviewed all five changed workflows at 3d448f910c2c0445b979441452b29d401074d42d, including job dependencies, matrices, shared setup, triggers, permissions and concurrency policies. No blocking defects found; the implementation matches issue #1425. Author rather than actor grouping correctly handles maintainer updates, and all 18 job definitions have explicit timeouts.
Non-blocking observations
-
🤔 Workflow limits do not guarantee runner fairness — The concurrency block in
.github/workflows/test.yml:16–19admits one workflow run per author, but that run can launch eight jobs. Format and CodeQL can launch another eight simultaneously, alongside integration jobs. This substantially reduces burst multiplication, but still permits significant runner occupancy by one author. Measure contention after rollout before adding further serialization. -
📝 Clarify queue capacity and ordering — The repeated comments in the five workflows say queued runs are preserved and run in order. GitHub permits 100 pending runs per group, then cancels additional arrivals. FIFO follows when runs begin waiting on the concurrency group, rather than workflow dispatch order. Consider documenting those qualifications in the shared wording and PR description. See GitHub concurrency documentation. Apply manually: this clarification spans five workflow files.
-
🏕 Retain the policy verification script — The PR describes useful expression checks with
act, but the verification script is not included in the change. Keeping it in the repository would help catch drift across the five duplicated policies. Cover author versus actor, bots, non-PR events, group separation and timeout presence. Apply manually: retaining the script introduces a new file.
Suggested follow-up
Retain this author-level cap, then separately consider cancelling superseded runs within the same PR. That would reduce obsolete work while preserving the cap across different PRs. Any privileged cancellation workflow should manipulate Actions metadata without checking out or executing contributor code. Stale-run queueing is explicitly acknowledged in this PR and is not a blocker.
Validation
Local YAML/policy assertions passed for all five identical concurrency policies and all 18 job definitions; git diff --check passed. Local actionlint reported its outdated schema for queue and shell warnings on unchanged lines. Current GitHub documentation confirms queue: max is supported. Rust/JS suites were not rerun locally for this workflow-only change, and the reported live multi-PR experiment was not independently reproduced.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (actions): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- cargo test (axum native): PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- Analyze (rust): PASS
- prepare integration artifacts: PASS
- format-typescript: PASS (required)
- cargo test: PASS (required)
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- PBS Terraform and Compose checks: PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Approved. Queuing PR CI runs per author and workflow, with bounded job timeouts and independent groups for bots and non-PR runs, is a focused improvement to shared runner fairness.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This adds a per-PR-author concurrency group with queue: max to the five PR-triggered workflows, and a timeout-minutes on every job. The timeouts hold up against the last 400 runs of each workflow and against the cold runs after the Rust 1.98.1 upgrade. Two things block: without superseded-run cancellation, the per-author queue delays required checks by hours and can hit the 100-pending cap, and nothing keeps the new invariants in place as open PRs land.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- Per-author queue without superseded-run cancellation — see inline at
.github/workflows/test.yml:17 - Enforce the shared block and job timeouts — see inline at
.github/workflows/format.yml:58
Non-blocking
🤔 thinking
- Bot exemption covers the runs nobody waits on — see inline at
.github/workflows/test.yml:14
📝 note
- Windows axum leg from the module stack — see inline at
.github/workflows/test.yml:81
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (python): PASS
- docs-proposal scripts: PASS
- Analyze (javascript-typescript): PASS (both runs)
- Analyze (rust): PASS
- vitest: PASS
- cargo fmt: PASS (required)
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- format-typescript: PASS (required)
- prepare integration artifacts: PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- PBS Terraform and Compose checks: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- Analyze (actions): PASS
A workflow-level group per pull request now cancels a run when a newer push arrives, so stale runs no longer hold an author's queue or count toward GitHub's 100-pending cap. The per-author queue moves to each job's own concurrency block with queue: max, keyed on the matrix for matrix jobs so their legs do not queue behind each other. Bot-authored PRs and non-PR events still key on the run ID. The shared comment now states the FIFO ordering and the 100-pending limit. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
The required cargo fmt job now fails when a PR-triggered workflow lacks the per-PR cancellation block from test.yml, or when a job lacks timeout-minutes or a per-author queue keyed on pull_request.user.login with queue: max. Matrix jobs must also include the matrix in their group. Jobs that call a reusable workflow are skipped. The step uses the runner image's yq. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 2 at 5068110d2. Both round-1 blockers are fixed: each workflow now cancels a PR's superseded run, each job queues per author, and the guard enforces both. A job-level replay of the 372 Run Tests PR runs from Oct 21 to 30, using uncontended job durations, shows the effect. For the stacked-PR author, the latest push's cargo test wait drops from p50 547 / max 1199 min under the round-1 design to p50 170 / max 393 min. prk-Jr's max falls from 266 to 8 min, job-hours fall from 275 to 239, and nothing is cancelled at the 100-pending cap. What blocks now is mechanical: the required cargo fmt check fails, the description still describes round 1, and the live check is open.
2 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining findings describe the fix in prose because the change touches lines outside the diff or the PR description.
Blocking
🔧 wrench
- The required
cargo fmtcheck fails — see Cross-cutting below - The PR description still describes round 1 — see Cross-cutting below
❓ question
- Live check of cancelling a run whose jobs wait in a job-level queue — see inline at
.github/workflows/test.yml:20
Non-blocking
♻️ refactor
- The guard accepts three copy-paste mistakes — see inline at
.github/workflows/format.yml:124
🤔 thinking
- The bot exemption is now the largest load source — see inline at
.github/workflows/test.yml:28
⛏ nitpick
- 104-column comment line in all five workflows — see inline at
.github/workflows/test.yml:17
Cross-cutting / body-level findings
-
🔧 The required
cargo fmtcheck fails. The guard works as designed: the latest main merge brought incheck-ci-node-pin(from #1257,format.yml) andlint-domains(from #1254,test.yml) without the new blocks (job log). Those lines are outside the diff, so there's no suggestion. Add these after each job'sruns-on:, then merge main again (2 behind, and main requires up-to-date branches):check-ci-node-pin: name: CI Node pin guard runs-on: ubuntu-latest timeout-minutes: 5 concurrency: group: ${{ github.workflow }}-check-ci-node-pin-${{ github.event_name == 'pull_request' && github.event.pull_request.user.type != 'Bot' && github.event.pull_request.user.login || github.run_id }} queue: max
lint-domains: ... name: ts dev lint domains (changed lines) if: github.event_name == 'pull_request' runs-on: ubuntu-latest timeout-minutes: 10 concurrency: group: ${{ github.workflow }}-lint-domains-${{ github.event_name == 'pull_request' && github.event.pull_request.user.type != 'Bot' && github.event.pull_request.user.login || github.run_id }} queue: max
The timeouts follow the description's rule: the slowest recent runs were 0.3 min (52 runs) and 2.4 min (54 runs). Verified on a local merge with current main plus these blocks and both suggestions: the guard passes locally and in Docker, the Node pin checker reports 26 jobs OK and its tests pass 30/30, and the setup-rust guard passes.
-
🔧 The PR description still describes round 1. The summary and YAML snippet show
cancel-in-progress: falseand a workflow-level per-author group. "Known trade-offs" says cancelling superseded runs is left for a follow-up. The Changes table has no row for the guard or the per-job blocks, and the test plan's live run andactchecks cover the old design. Please rewrite it for the current design and add the live-check result. -
📝 Merge order. After this lands, the guard fails
cargo fmton rebase for #1107 (trace-runtime-tests; approved and behind main), #1433 (worker-auction-deadlines.yml), #1435 (release.yml), #1345 (inspector.yml) and draft #1460 (publisher-registration). Ten more already conflict with this PR in.github: the #1347 to #1423 stack, #1424, #1449, #917, #1002 and #1053. That's the guard doing its job, but those authors will want a heads-up. For #1435, the guard forces test.yml's block onto a workflow that also runs on tag pushes and has its ownbinary-attestations-${{ github.ref }}group. Tag pushes key on the run ID, so releases won't queue, but it's worth confirming that's intended. -
🏕 AGENTS.md. The CI Gates list names the standalone guards (10, 12 and 13). One line saying every PR-triggered workflow needs test.yml's workflow-level block and every job needs a timeout and a per-author queue would tell agents before the guard fails their run.
CI Status
- cargo fmt: FAIL (required)
- cargo test: PASS (required)
- format-typescript: PASS (required)
- format-docs: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (both runs)
- Analyze (python): PASS
- Analyze (rust): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (axum native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- HTML EOF runtime (Fastly): PASS
- ts dev lint domains (changed lines): PASS
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- docs-proposal scripts: PASS
- vitest: PASS
- PBS Terraform and Compose checks: PASS
| # PRs and non-PR events key on the run ID and never wait on a queue. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
❓ question — In the earlier thread you planned to confirm in your throwaway repo that cancelling a superseded run also drops its jobs still waiting in a job-level queue, and to report back before merge. This PR can't show it yet: the three rework commits went up in one push, so its own history has no cancelled run. Could you post the result here? A run link is enough.
| while read -r job; do | ||
| [ -n "$job" ] || continue | ||
| echo "::error file=$f::Job '$job' must join a per-author queue: concurrency.group keyed on pull_request.user.login and queue: max." | ||
| status=1 | ||
| done < <(yq '.jobs | to_entries[] | select(.value.uses == null and (.value.concurrency.queue != "max" or (.value.concurrency.group // "" | test("pull_request\.user\.login") | not))) | .key' "$f") |
There was a problem hiding this comment.
♻️ refactor — The per-author check only looks for pull_request.user.login and queue: max, so it accepts three copy-paste mistakes:
- A group without the
|| github.run_idfallback. On main pushes the login is empty, so every main push would share one queue per job, which the comment at the top of test.yml says never happens. - A group copied from another job, for example
test-cloudflarereusingtest-axum's. Two jobs of the same run then wait on each other. - A group without the
${{ github.workflow }}-prefix. Group names are repo-wide, so it collides with a same-named job in another workflow.
This version builds the expected shape from test-rust's group at runtime (a ${{ … }} literal inside run: would be expanded by Actions) and requires each job's group to be exactly <workflow>-<job id>[-<matrix>]-<author or run ID>. The matrix check below stays as it is.
Verified on 15 fixtures, locally and under bash 5.2.15 with yq 4.54.1 in Docker (the ubuntu-latest versions). It fails the three cases above, which the current check passes, and matches the current check on the other 12: an actor key, a string-form job concurrency, the default queue, a missing cancel, a missing timeout, a matrix job without the matrix, a new string-trigger workflow and a broken file all fail; push-only workflows, reusable-workflow callers and the fixed head pass. No false positives on the open PRs that pass the current check (#1453, #1343, #868), and actionlint and shellcheck report nothing new.
| while read -r job; do | |
| [ -n "$job" ] || continue | |
| echo "::error file=$f::Job '$job' must join a per-author queue: concurrency.group keyed on pull_request.user.login and queue: max." | |
| status=1 | |
| done < <(yq '.jobs | to_entries[] | select(.value.uses == null and (.value.concurrency.queue != "max" or (.value.concurrency.group // "" | test("pull_request\.user\.login") | not))) | .key' "$f") | |
| # Each job's group must match test-rust's in test.yml with its own job | |
| # id (plus the matrix, for matrix jobs), so a copied block cannot drop | |
| # the run-ID fallback or share another job's queue. | |
| shape=$(yq '.jobs["test-rust"].concurrency.group' "$reference") | |
| workflow_part=${shape%%-test-rust-*} | |
| author_part=${shape#*-test-rust-} | |
| while IFS=$'\t' read -r job queue group; do | |
| [ -n "$job" ] || continue | |
| middle=${group#"$workflow_part-$job-"} | |
| if [ "$queue" != max ] || [ "$workflow_part-$job-${middle%"$author_part"}$author_part" != "$group" ]; then | |
| echo "::error file=$f::Job '$job' must set queue: max and a concurrency group shaped like test-rust's in test.yml, with its own job id." | |
| status=1 | |
| fi | |
| done < <(yq '.jobs | to_entries[] | select(.value.uses == null) | [.key, .value.concurrency.queue // "", .value.concurrency.group // ""] | @tsv' "$f") |
| runs-on: ubuntu-latest | ||
| timeout-minutes: 45 | ||
| concurrency: | ||
| group: ${{ github.workflow }}-test-rust-${{ github.event_name == 'pull_request' && github.event.pull_request.user.type != 'Bot' && github.event.pull_request.user.login || github.run_id }} |
There was a problem hiding this comment.
🤔 thinking — Following up on the bot thread with this week's data. On Oct 29 Dependabot opened 13 PRs (#1466 to #1478) within minutes, and 58 bot-authored jobs ran at once at 16:04Z across the PR workflows. Under this design one human author tops out at 24 concurrent jobs (one per job name across the five workflows), and all human authors together peaked at 59 since Oct 19. So the exempt Dependabot batch is now the largest single source of runner load. Keying bots on their login like everyone else (one dependabot[bot] queue per job) would cap it, and the per-PR cancellation would still drop their superseded rebase runs. Your call; non-blocking.
| # fill the runner pool ahead of everyone else. Queued jobs start in the order they began waiting; GitHub | ||
| # keeps up to 100 pending per queue and cancels arrivals past that. Every job | ||
| # has a timeout so a hung job cannot hold its queue for hours. Bot-authored | ||
| # PRs and non-PR events key on the run ID and never wait on a queue. |
There was a problem hiding this comment.
⛏ nitpick — Line 14 is 104 columns. This rewraps the rest of the paragraph to 77 or less with the same words. The same lines need the same change in codeql.yml:25-28, format.yml:14-17, integration-tests.yml:16-19 and pbs-example.yml:23-26.
| # fill the runner pool ahead of everyone else. Queued jobs start in the order they began waiting; GitHub | |
| # keeps up to 100 pending per queue and cancels arrivals past that. Every job | |
| # has a timeout so a hung job cannot hold its queue for hours. Bot-authored | |
| # PRs and non-PR events key on the run ID and never wait on a queue. | |
| # fill the runner pool ahead of everyone else. Queued jobs start in the order | |
| # they began waiting; GitHub keeps up to 100 pending per queue and cancels | |
| # arrivals past that. Every job has a timeout so a hung job cannot hold its | |
| # queue for hours. Bot-authored PRs and non-PR events key on the run ID and | |
| # never wait on a queue. |
|
@dhruv8sh please resolve comments after addressing feedback |
Summary
github.event.pull_request.user.login), so an author has at most one run in progress per workflow and their extra runs wait aspendingwithout holding a runner. Keying on the author rather thangithub.actormeans a maintainer pushing to or clicking "Update branch" on contributors' PRs does not pile those runs into the maintainer's queue.queue: maxkeeps every waiting run and starts them in order. The default single-pending queue would silently cancel an author's older waiting run whenever they open another PR. Every job now setstimeout-minutes, so a hung job cannot hold an author's queue for GitHub's 6-hour default.main, scheduled runs, manual dispatches and bot-authored PRs (for example Dependabot) fall back togithub.run_id, so they get their own group and are never held behind anyone's queue.Changes
.github/workflows/test.ymlqueue: max; addtimeout-minutesto the six jobs that lacked one.github/workflows/format.ymltimeout-minutesto all five jobs.github/workflows/codeql.ymltimeout-minutestoanalyze.github/workflows/integration-tests.yml.github/workflows/pbs-example.ymldeploy-docs.ymlanddocs-proposal.ymlare untouched: they only run onmainand already define their own concurrency.New timeouts are about 3x the longest of each job's last five
mainruns, with a 10-minute floor (5 for the symlink guard):test-axumtest-cloudflaretest-spintest-paritytest-cli(macOS leg)test-typescriptcheck-claude-md-symlinkformat-rustformat-typescriptformat-docsdocs-proposal-scriptsanalyze(rust leg)Known trade-offs: several pushes to the same PR now run one after another rather than in parallel, and a re-run joins the back of the author's queue. Cancelling a PR's superseded runs needs a separate privileged workflow, since a workflow can only have one concurrency group; that is left for a follow-up.
Closes
Closes #1425
Spec
Spec: Not a feature (CI change)
Test plan
cargo test-fastly && cargo test-axum(andcargo test-cloudflare)cargo clippy-fastly && cargo clippy-axum(andcargo clippy-cloudflare)cargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveconcurrencyblock withqueue: maxin a throwaway private repo. One author opened three PRs back to back, then pushed tomainwhile they were queued. The PR runs ran one at a time, in creation order, none cancelled (pr-2 started 4s after pr-1 finished, pr-3 4s after pr-2). Themainpush started 4s after it was created while two PR runs were still pending.act: evaluated each workflow's actualgroup:expression. Two PRs by one author map to one group, different authors to separate groups, a maintainer pushing to another author's PR to that author's group, and Dependabot PRs,push,scheduleandworkflow_dispatchto run-ID groups. The same script asserts every job setstimeout-minutes. Against the previous revision it fails on the maintainer, Dependabot and timeout checks, as expected.queueas an unknown key; their schemas predate it. GitHub accepts it, anddocs-proposal.ymlalready usesqueue: maxonmain.Checklist
unwrap()in production code — useexpect("should ...")(no Rust changes)tracingmacros (notprintln!) (no Rust changes)actchecks above)