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
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
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
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
| # timeout so a hung run cannot hold that queue for hours. Bot-authored PRs and | ||
| # other events get their own group from the run ID and are never held back. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ 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.
🔧 wrench — A per-author queue that never cancels superseded runs backs the required checks up for hours, and it can reach the 100-pending cap.
I replayed the 391 Run Tests PR runs from Oct 18 to 23 under this group. Each run's duration is its uncontended critical path (the longest job), so the replay measures only the delay this policy adds and ignores the runner-pool limit:
- The run for an author's latest push waits p50 594 min / max 1850 min on jwrosewell's stacked PRs, where a restack fires 18 to 29 runs within two minutes, and p50 104 / max 550 min for prk-Jr, whose release-candidate jobs take one to two hours.
- 124 of the 391 runs would start after a newer push to the same PR already existed, so most of the queue is stale work.
- The cap is reachable. The replay peaks at 99 pending, and 5
Run Testsruns (79CodeQL Advancedruns) for an author's latest push get cancelled, because "When the queue is full, any additional jobs or workflow runs are canceled." Those PRs get nocargo testresult for their head without a manual re-run, and the re-run joins the back of a full queue. - main requires branches to be up to date, so every merge adds "Update branch" runs to these queues.
The description says cancelling superseded runs needs a separate privileged workflow because a workflow has only one concurrency group. Jobs have their own jobs.<job_id>.concurrency, which accepts queue: max and can read the matrix and strategy contexts. So you can cancel per PR at the workflow level and queue per author at the job level. In the same replay (approximated per run), runner time drops from 138 h to 93 h, no runs are cancelled at the cap, the latest-push wait falls to p50 284 / max 852 min for jwrosewell and p50 0 / max 58 min for prk-Jr, and peak concurrent jobs stay at 56, the same as this PR.
Proposed shape (apply manually: it touches every job in the five workflows):
# One run per PR: a newer push cancels the superseded run.
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
cancel-in-progress: true
jobs:
test-rust:
# One test-rust job per author at a time; the rest wait in order.
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 }}
queue: maxMatrix jobs (test-cli, CodeQL analyze) add ${{ matrix.os }} or ${{ matrix.language }} to the job group so their legs don't queue behind each other. Please confirm in your throwaway repo that cancelling a superseded run also drops its jobs still waiting in a job-level queue; I couldn't check that live. If you keep the workflow-level design, the shared comment at lines 11-15 should say that runs past 100 pending are cancelled, since that limit is reachable.
| runs-on: ubuntu-latest | ||
| timeout-minutes: 25 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔧 wrench — Nothing enforces the shared concurrency block or the job timeouts, and open PRs already break both.
Trial merges of the open workflow-touching PRs with this head:
- Gate PRs on non-allowlisted URL hosts in changed lines #1254 adds
lint-domainsand Set up pinned Node in Rust CI jobs and drop unused tsjs deps #1257 addscheck-ci-node-pin, both with notimeout-minutes. Gate PRs on non-allowlisted URL hosts in changed lines #1254 merges cleanly. - Enforce absolute deadlines for auction response collection #1433 adds
worker-auction-deadlines.yml, Adapt Trusted Server to the EdgeZero AWS demo RC #1424 addsnative-container.yml, and the Add the permission model with the Privacy Taxonomy vocabulary #1345 to Move module browser scripts and Tinybird into their crates #1423 stack addsinspector.yml. All are PR-triggered with no per-author group, and the stack'sbuild-inspector-wasmandcheck-core-platform-namesjobs have no timeout either.
Following #1259's precedent of a guard step in the required cargo fmt job, this step fails when any PR-triggered workflow's concurrency differs from test.yml's, or when a job lacks timeout-minutes. Jobs that call a reusable workflow are skipped, since they can't set it. It uses the runner image's yq (4.54.1 on ubuntu-latest), so it needs no setup.
Verified on 15 fixtures, locally and under bash 5.2.15 with yq 4.54.1 in Docker. It passes on this head. It fails on trial merges with #1254, #1257, #1433, #1424 and #1345, on main, on one edited group, on every group removed, on a dropped queue, on on: pull_request and on: [push, pull_request] without the block, and on a matrix job missing its timeout. It passes for push-only workflows and for reusable-workflow callers. actionlint and shellcheck report nothing new. If you adopt the job-level design from the other comment, the check moves to the workflow block plus each job's queue.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| # A hung job, or a PR-triggered workflow outside the per-author queue, | |
| # brings back the runner pile-up that the shared concurrency block in | |
| # test.yml prevents. Every PR-triggered workflow must carry that exact | |
| # block, and every job must set timeout-minutes. | |
| - name: Require the per-author CI queue and job timeouts | |
| shell: bash | |
| run: | | |
| expected=$(yq -o=json -I=0 '.concurrency' .github/workflows/test.yml) | |
| case "$expected" in | |
| *pull_request.user.login*) ;; | |
| *) echo "::error file=.github/workflows/test.yml::test.yml must define the per-author concurrency group."; exit 1 ;; | |
| esac | |
| status=0 | |
| for f in .github/workflows/*.yml .github/workflows/*.yaml; do | |
| [ -f "$f" ] || continue | |
| on_pr=$(yq '.on | ((tag == "!!map" and has("pull_request")) or (tag == "!!str" and . == "pull_request") or (tag == "!!seq" and ([.[] | select(. == "pull_request")] | length > 0)))' "$f") | |
| [ "$on_pr" = true ] || continue | |
| if [ "$(yq -o=json -I=0 '.concurrency' "$f")" != "$expected" ]; then | |
| echo "::error file=$f::PR-triggered workflows must use the concurrency block from test.yml." | |
| status=1 | |
| fi | |
| for job in $(yq '.jobs | to_entries[] | select(.value.uses == null and .value["timeout-minutes"] == null) | .key' "$f"); do | |
| echo "::error file=$f::Job '$job' must set timeout-minutes." | |
| status=1 | |
| done | |
| done | |
| exit "$status" |
| # Each pull request author runs one PR at a time per workflow, so one person | ||
| # opening many PRs cannot fill the runner queue ahead of everyone else. Their | ||
| # other runs wait in order instead of being cancelled, and every job has a | ||
| # timeout so a hung run cannot hold that queue for hours. Bot-authored PRs and |
There was a problem hiding this comment.
🤔 thinking — Bot-authored PRs get a run-ID group, so a Dependabot batch runs fully in parallel. Dependabot opens PRs here in bursts (#1021 to #1024 within 30 seconds, and three ecosystems update weekly), and no one is waiting on those runs, so they are the cheapest ones to queue. Keying bots on their login like everyone else (one dependabot[bot] queue) fits the goal of this PR. #1425 asks for the exemption explicitly, so it would help to say why somewhere. Low stakes this week: 8 of 391 Run Tests PR runs were Dependabot's.
| test-axum: | ||
| name: cargo test (axum native) | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 |
There was a problem hiding this comment.
📝 note — Heads-up for the module stack: ten of jwrosewell's PRs (#1347 to #1399, and #1423) get a new test.yml conflict from this PR, because they turn test-axum and test-cloudflare into ubuntu/windows matrices. Once that's resolved, this 30-minute limit applies to the windows-latest axum leg, which ran p50 19.8, p95 21.5 and max 84.2 min across 188 runs. The 3x rule used here would put it near 65. A matrix-aware value keeps both legs covered:
timeout-minutes: ${{ matrix.os == 'windows-latest' && 65 || 30 }}The windows cloudflare leg (max 10.0 min) fits in 15.
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)