Cap what a single fork pull request can spend - #122
Merged
Conversation
This workflow is reachable from a fork pull request and had neither a concurrency group nor a job timeout, so it inherited GitHub's defaults for both: unlimited concurrent runs, and **six hours** per job. On paid Blacksmith runners that is a real number rather than a theoretical one — 15 job runs at the six-hour ceiling is 90 compute-hours for a single push, and without a concurrency group a contributor pushing five times held five complete sets at once rather than superseding the previous one. The repo now requires approval for every external contributor's fork PR, so nothing external starts without a deliberate click. This is the second half: the approval gate decides *who* runs, and these two settings decide *how much* a run that has been approved can cost. It bounds our own pushes too, which is where most of the waste actually was. Sized against measurement rather than a guess. Over the last six runs the slowest job here is 4m25s; 20 minutes leaves generous headroom while turning the worst case from 90 compute-hours into about 5. A job that hits 20 minutes is not a slow job, it is a stuck one.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Neither failure was caused by the timeout change in this branch — both failed well inside the new 20-minute cap, and both fail on any cdx-core commit made today. `main` last ran in June and has not run since, so the drift accumulated where nothing was looking. **Lint.** `clippy::manual_assert_eq` did not exist when this last passed; under `-D warnings` a new lint is a new failure. One site, `cdx-core/src/document/tests.rs:227`, and the line directly above it already used `assert_eq!`, so the fix is what the surrounding code was doing anyway. It also buys a real failure message: `assert!(len() == 1)` reports only that a bool was false, while `assert_eq!` prints both sides. **Security Audit.** Two advisories published since June, both against *transitive* crates — neither appears in any `Cargo.toml`, so this is a lockfile-only change with no manifest touched and no semver decision taken: - RUSTSEC-2026-0204, `crossbeam-epoch` 0.9.18 -> 0.9.20 (solution: >= 0.9.20) - RUSTSEC-2026-0185, `quinn-proto` 0.11.13 -> 0.11.17 (solution: >= 0.11.15) `cargo update` also adds `rand` 0.10.2 and `rand_pcg` 0.10.2 as new transitive entries of the newer `quinn-proto`; the existing `rand` 0.9.2 pin is untouched. Verified locally on rustc 1.97.1, running CI's own commands rather than approximations of them: `cargo audit --ignore RUSTSEC-2023-0071 --ignore RUSTSEC-2026-0037` exits 0, with only the five pre-existing *allowed* warnings (unsound and yanked) remaining, and `cargo clippy --workspace --all-targets --all-features -- -D warnings` is clean. The five allowed warnings are deliberately left alone: they were passing before, they are warnings rather than vulnerabilities, and folding them into a drift fix would hide a judgement call inside a mechanical change.
…warns The previous commit fixed what `cargo audit` was failing on and stopped there, because that is the command I checked. The job runs **two** tools and they do not agree: `cargo audit` treats an `informational = "unsound"` advisory as a warning, while `cargo deny` treats it as `error[unsound]`. So step 7 went green and step 9 stayed red on an advisory that had been sitting in audit's own output the whole time, counted among its "5 allowed warnings". RUSTSEC-2026-0190, `anyhow` — unsoundness in `Error::downcast_mut()`. The advisory gives `patched = [">= 1.0.103"]`; this moves 1.0.101 -> 1.0.104. `cdx-cli/Cargo.toml` already declares `anyhow = "1.0"`, so the caret range admits it and this stays lockfile-only, like the previous commit. The two configurations have drifted and this does not fix that. `ci.yml` passes `--ignore RUSTSEC-2023-0071 --ignore RUSTSEC-2026-0037` to audit, while `deny.toml` ignores only `RUSTSEC-2023-0071` and additionally sets `yanked = "warn"`. RUSTSEC-2026-0037 is ignored by one tool and unknown to the other, so the pair can disagree again on the next advisory. Left alone here: reconciling them is a policy decision about which tool is the source of truth, not a drift fix. Verified locally on rustc 1.97.1 with **both** of CI's commands this time, which is the check that was missing before: `cargo audit --ignore RUSTSEC-2023-0071 --ignore RUSTSEC-2026-0037` exits 0; `cargo deny check` reports "advisories ok, bans ok, licenses ok, sources ok"; and `cargo clippy --workspace --all-targets --all-features -- -D warnings` is clean.
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.
Cap what a single fork pull request can spend
This workflow is reachable from a fork pull request and had neither a
concurrency group nor a job timeout, so it inherited GitHub's defaults for
both: unlimited concurrent runs, and six hours per job. On paid
Blacksmith runners that is a real number rather than a theoretical one —
15 job runs at the six-hour ceiling is 90 compute-hours for a single
push, and without a concurrency group a contributor pushing five times held
five complete sets at once rather than superseding the previous one.
The repo now requires approval for every external contributor's fork PR, so
nothing external starts without a deliberate click. This is the second half:
the approval gate decides who runs, and these two settings decide how
much a run that has been approved can cost. It bounds our own pushes too,
which is where most of the waste actually was.
Sized against measurement rather than a guess. Over the last six runs the
slowest job here is 4m25s; 20 minutes leaves generous headroom while turning
the worst case from 90 compute-hours into about 5. A job that hits 20
minutes is not a slow job, it is a stuck one.
Change
concurrencygroup keyed on workflow + ref, withcancel-in-progress: truetimeout-minutes: 20on all 11 job definitionsWorst case per push
Test plan
maintimeout-minutes