ci: one job for the source gates, and caching that is not a net loss - #1857
Merged
Conversation
Thirteen jobs fired on every push. Seven of them were source-walking xtask gates, and each one spent ~26s compiling the xtask binary and ~13s compiling its test binary in order to do a few seconds of file walking. They also shared a single cache key, so they raced and six of every seven saves were discarded. `cargo xtask check-all-source-gates` runs all eight gates in one process and reports every failure before exiting, so one job still surfaces every breakage at once. The per-gate subcommands stay for narrowing a failure down locally. One `cargo test -p xtask` replaces eight filtered subsets of the same 178-test binary. License headers and the ship-change gate tests — bash and git only — share a second runner. Thirteen checks become six. The cache was a net loss, not a win. Repo usage stood at 11.76 GB against GitHub's 10 GB cap, so every save evicted a live entry, and restoring the 3.72 GB `cargo-test` entry took 66s to serve 53s of tests. Two causes: caching bare `target/`, which keeps workspace artifacts that are invalid on the next commit, and ref-scoped saves — three near-identical 1.97 GB wheel entries existed at once, one for main and one for each open PR, none readable by the others. Swatinem/rust-cache keeps dependency artifacts only, and `save-if` on main means PRs restore and never mint their own. No workflow cancelled a superseded run, so three pushes to a PR ran three full suites and nobody read the first two. Every PR-triggered workflow now has a concurrency group. The two expensive jobs skip prose-only changes; the source gates deliberately do not, since they scan `docs/` and Markdown. `cargo ci` runs what CI runs, so a green local run predicts a green PR. That needed the license check to leave inline workflow YAML — inline bash is by construction something nobody can run before pushing — so it is now `scripts/check-license-headers.sh`, shared by both. It discovers files with `git ls-files` rather than `find`: a `find` matches CI's clean checkout but reports 15k false positives on a developer machine, where venvs, uv caches and build output sit in the tree. Incremental compilation is off. Its artifacts had reached 419 GB of an 892 GB `target/` — cargo keeps a separate incremental tree per crate per profile and never collects across them, so on a workspace this wide the disk churn costs more than the rebuild it saves. Delete `incremental = false` to get it back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR consolidates source and local CI gates through ChangesCI gate consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant Cargo
participant xtask
participant SourceGates
participant LicenseCheck
Developer->>Cargo: cargo run-local-ci-gates
Cargo->>xtask: run-local-ci-gates
xtask->>SourceGates: run all source gates
SourceGates-->>xtask: aggregated gate results
xtask->>LicenseCheck: run license-header check
LicenseCheck-->>xtask: validation result
xtask-->>Developer: aggregate CI result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/source-gates.yml:
- Line 54: Update the actions/checkout@v4 step in the source-gates workflow to
set persist-credentials to false, matching the configuration used in
repo-gates.yml while preserving the existing checkout behavior.
In `@scripts/check-license-headers.sh`:
- Around line 66-72: Update the Rust invocation of report_files_missing_header
to scan the workspace-wide `**/*.rs` pathspec, including `xtask/**/*.rs`, while
retaining exactly the three existing `vendor/tatolab-vulkanalia*` exclusions and
the current Rust failure handling.
- Around line 39-51: Update the header validation loop in the license-check
script to validate both required header lines, including the exact
SPDX-License-Identifier: BUSL-1.1 line, rather than checking only
expected_header. Ensure Rust files without either required line are added to
missing_files while preserving the existing shebang/header positioning behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c6f4b5c-7d46-4eaa-9819-20ee423fdbba
📒 Files selected for processing (18)
.cargo/config.toml.github/workflows/check-boundaries.yml.github/workflows/check-device-wait-idle.yml.github/workflows/check-no-escalate-in-lifecycle.yml.github/workflows/check-no-in-process-placement.yml.github/workflows/check-no-inventory-submit.yml.github/workflows/check-no-unbounded-cstr-from-ptr.yml.github/workflows/check-ship-change-removed-gate.yml.github/workflows/license-check.yml.github/workflows/lint-logging.yml.github/workflows/pr-title.yml.github/workflows/python-wheel.yml.github/workflows/repo-gates.yml.github/workflows/schemas.yml.github/workflows/source-gates.yml.github/workflows/test.ymlscripts/check-license-headers.shxtask/src/main.rs
💤 Files with no reviewable changes (9)
- .github/workflows/license-check.yml
- .github/workflows/check-no-unbounded-cstr-from-ptr.yml
- .github/workflows/check-no-escalate-in-lifecycle.yml
- .github/workflows/check-ship-change-removed-gate.yml
- .github/workflows/check-device-wait-idle.yml
- .github/workflows/check-no-in-process-placement.yml
- .github/workflows/check-boundaries.yml
- .github/workflows/lint-logging.yml
- .github/workflows/check-no-inventory-submit.yml
Comment on lines
+66
to
+72
| report_files_missing_header \ | ||
| "// Copyright (c) 2025 Jonathan Fontanez" Rust \ | ||
| 'runtime/*.rs' 'sdk/*.rs' 'adapters/*.rs' 'vendor/*.rs' 'examples/*.rs' \ | ||
| ':(exclude)vendor/tatolab-vulkanalia/*' \ | ||
| ':(exclude)vendor/tatolab-vulkanalia-sys/*' \ | ||
| ':(exclude)vendor/tatolab-vulkanalia-vma/*' || | ||
| failed_language_checks+=("Rust") |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scan all Rust source paths.
The Rust pathspec list excludes xtask/**/*.rs. A new Rust file in xtask/ can merge without the required header. Use a workspace-wide Rust pathspec, then retain the three exact vendored-tree exclusions.
Proposed fix
report_files_missing_header \
"// Copyright (c) 2025 Jonathan Fontanez" Rust \
- 'runtime/*.rs' 'sdk/*.rs' 'adapters/*.rs' 'vendor/*.rs' 'examples/*.rs' \
+ ':(glob)**/*.rs' \
':(exclude)vendor/tatolab-vulkanalia/*' \As per coding guidelines: **/*.rs requires the BUSL copyright and SPDX header for every new Rust file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-license-headers.sh` around lines 66 - 72, Update the Rust
invocation of report_files_missing_header to scan the workspace-wide `**/*.rs`
pathspec, including `xtask/**/*.rs`, while retaining exactly the three existing
`vendor/tatolab-vulkanalia*` exclusions and the current Rust failure handling.
Source: Coding guidelines
…ease Two corrections after measuring what the previous commit only asserted. Disabling incremental compilation was wrong. The 419 GB in `target/debug/incremental` was real, but it was months of accumulation across crates, profiles and feature permutations — not a per-build cost. Measured on a one-line edit to streamlib-engine followed by `cargo check -p streamlib-engine`: 3.0s incremental, 8s without, with one crate's incremental state settling at ~255 MB. Trading a 2.7x slower edit loop for disk that `rm -rf target/debug/incremental` reclaims in seconds is a bad deal. CI keeps CARGO_INCREMENTAL=0 in the workflows, where it is unambiguously right: a runner never rebuilds after an edit. The gates now run in release locally. They syn-parse ~7k files, so the profile dominates — 11s release against 24s debug. That is the difference between a pre-commit check that gets run and one that gets skipped. CI deliberately does not mirror it: its job already compiles xtask in debug for `cargo test -p xtask`, and a second profile there would cost more compile time than the 13s it saves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.cargo/config.toml:
- Around line 8-11: Update the ci and gates aliases in .cargo/config.toml to
pass --locked to their cargo commands, matching the locked dependency behavior
used by the CI workflow while preserving the existing xtask subcommands.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Four review findings, all verified against the tree before applying. The licence gate only ever checked the copyright line, so a file carrying it without `SPDX-License-Identifier: BUSL-1.1` passed — and SPDX is the half a licence scanner actually reads. Both lines are now checked, as whole lines. No existing file fails either check, so this is pure hardening. Its Rust pathspec named runtime/ sdk/ adapters/ vendor/ examples/, which silently exempted `xtask/` and `tools/`. Widening it to every tracked `.rs`, minus the three vendored vulkanalia dirs, immediately found `xtask/src/check_no_inventory_submit.rs` carrying a `2026` copyright line — CLAUDE.md and .claude/rules/licensing.md both mandate `2025` verbatim, with no year-varying provision, so the file is corrected to match the rule. The PR-triggered workflows compile and run PR-authored code — build scripts, proc macros, test code — while `actions/checkout` leaves the job token in `.git/config` where any of it can read. All five checkout steps across source-gates, test and python-wheel now set `persist-credentials: false`; repo-gates already did. No workspace member has a git dependency, so nothing here needed credentials (the only git deps in the repo are public ones in `packages/clap`, which these jobs never build). `cargo ci` and `cargo gates` omitted `--locked` while the workflows use it, so they could resolve a different dependency graph or quietly rewrite Cargo.lock and go green on a build the PR would not reproduce — defeating the one promise those aliases exist to make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 13, 2026
tato123
added a commit
that referenced
this pull request
Aug 13, 2026
* refactor(engine)!: unique names are minted, never read off a wall clock Sixteen sites reached for `SystemTime::now()` to mint a unique name — iceoryx2 service names in `/dev/shm`, pipeline-cache temp files, test socket paths, a runtime-name seed. None of them wanted a timestamp; they wanted a string no concurrent process and no earlier run could collide with. Reading a wall clock for that is both wrong in kind and weaker in practice: two mints in the same nanosecond produced the same name. `mint_machine_global_unique_name_suffix()` is the one primitive for the job — pid (concurrent processes) + `MediaClock::now()` (runs on one boot, so a recycled pid cannot resurrect a stale `/dev/shm` entry) + a sequence (two mints in one nanosecond). It carries no separator a path or a file name would reject, so callers compose it into their own naming convention rather than the mint guessing at one. Not the mint: - `streamlib-surface-client`'s test sockets take a `TempDir` each. The kernel answers uniqueness and the socket is unlinked on drop rather than littering /tmp. That crate reaches no engine code, and a second local mint would be a parallel abstraction. - The api-server's runtime-name seed takes the OS CSPRNG it already links for bearer tokens, falling back to the pid with a warning rather than failing a runtime over a display name. Wall clock ^ pid was the weaker entropy source. - `apple/time.rs::system_time_to_ns` is deleted. Nothing imports `apple::time`, and a wall-clock-to-nanoseconds converter is the exact shape the one-monotonic-clock ADR retires. Behavior is unchanged everywhere; only the source of the unique part moves. Refs #1728 * feat(ci): check-clock-usage enforces the wall-clock allowlist The plan permits wall clock on exactly four observability surfaces and requires monotonic everywhere else. Prose alone does not hold that line: `SystemTime::now()` is the reflexive spelling for "what time is it", and a wall-clock value that reaches the data plane is indistinguishable from a media timestamp until someone subtracts one from the other. `cargo xtask check-clock-usage` scans `runtime/ sdk/ adapters/ xtask/` for wall-clock reads in Rust and Python and fails on any outside the permitted files. It is an entry in `ALL_SOURCE_WALKING_GATES`, so the existing `source-gates` job runs it — #1857 consolidated the per-gate workflows, and this ticket's "plus its workflow" predates that. What keeps the list from growing by accident: - No per-line pragma and no opt-out attribute. The file allowlist is the only way past, and every entry names one of four `ObservabilitySurface` variants — so a fifth surface means adding a variant, which is not something anyone does while reaching for a convenient timestamp. - Every entry must still read a wall clock. A surface that moves leaves a licence behind on a path some later change would inherit; the gate fails instead. - Every scan root and every language arm must read files. A gate whose tree moved out from under it reports clean, which is the failure mode that matters most. Discovery is `git ls-files`, not a filesystem walk: `sdk/streamlib-python-wheel` carries `.venv-pyright` with ~6000 third-party sources that are not ours to gate. Prose is blanked before scanning, so the wheel's own `clock.py` — whose module docstring names `time.time` / `datetime.now` / `time.time_ns` precisely to warn readers off them — is not a violation. Also corrects `RuntimeLogEvent::host_ts`, whose doc read "Host monotonic receipt timestamp, nanoseconds since UNIX epoch" — self-contradictory, and exactly the confusion this gate exists to prevent. Closes #1728 * docs(plan): record what the check-clock-usage guard actually landed as Two claims in the one-monotonic-clock change file were open or falsified, both derivable from the tree now: the guard has no workflow of its own (#1857 consolidated them) and no `Date.now` arm (no JS/TS source exists), and the unique-name uses the recon left "final at implementation" all converted rather than any joining the allowlist. Also records where the epoch-parity test landed — as the host + wheel pair the bullet itself anticipated, not one cross-process test. Records, not decisions: no DECIDED or OPEN entry moves. Refs #1728 * fix(ci): the clock gate refuses prose it cannot delimit Review found the one hole that mattered: `blank_out_python_prose` tracked triple-quoted spans across lines and threw the state away at EOF, so a span that never closed blanked every line after it and the file scanned clean. `files_scanned` still incremented, so neither read-source check noticed. That is this gate's only failure mode — it ships with no per-line pragma, so silent blindness is the whole risk surface — and the sibling `check-no-in-process-placement` already guards the identical hazard for `~~` spans. `blank_out_prose` now returns `Result`, and the Python arm refuses an unterminated span naming the file. Two tests cover it: the blanker's own error, and a scan that fails rather than reporting a file with a live `time.time_ns()` as clean. Also from review: - `SCAN_EXEMPT_FILES` replaces the single hardcoded gate-source path, so the next file that legitimately spells a banned pattern in source text is a reviewed list entry rather than a redesign. - The allowlist-liveness check no longer builds a violation list and a String per hit to answer a boolean. - Tests take the workspace root from `CARGO_MANIFEST_DIR` like the three sibling gates, instead of spawning `cargo locate-project` twice. - The api-server name fallback drew both indices from a u64 whose high half is zero when seeded from a pid, pinning every degraded runtime to the same noun. - Dropped three comments that narrated the change rather than explaining the code, per `.claude/rules/comments.md`. The adapter-cuda test takes a `TempDir` rather than reaching into `streamlib_engine::*` for the mint — `check-boundaries` rejects a consumer-tree test importing engine internals directly, and a socket path wants the kernel's uniqueness answer anyway. Refs #1728 * fix(engine): the mint rides the uuid the engine already links Review falsified the search that justified this module: `streamlib-engine` has carried `uuid` with `v4` since before this branch, and `pubsub/integration_tests.rs` already minted an iceoryx2 service name with `Uuid::new_v4()`. Adding a second answer to "give me a unique service name" is the parallel abstraction the engine doctrine exists to prevent, and the search that should have caught it was mine. `mint_machine_global_unique_name_suffix()` is now `<pid>-<uuid>`. A v4 UUID's 122 random bits make every mint distinct across processes, runs and reboots on their own, so the monotonic read and the sequence counter are both gone. That also retires a hazard the composite carried: the monotonic component resets on reboot while pids recycle, so a crash-left `/tmp` temp file plus a post-reboot pid collision at a similar offset was a real, if unlikely, collision the old doc claimed was impossible. The pid prefix stays, now honestly documented as diagnostic only — it is what names the process that left a stale entry. The one pre-existing service-name mint in `pubsub/integration_tests.rs` moves onto the helper, so the tree has one idiom for this concern rather than two. The other `Uuid::new_v4()` sites mint runtime ids and handle ids — a different concern, left alone. Review also showed the old `successive_mints_never_repeat` locked nothing: it passed with the counter replaced by a constant and with the clock replaced by a constant, because any one component makes 10k in-process mints distinct. Its message asserted a premise the evidence disproved. The new `the_uuid_component_is_what_differs_between_two_mints` strips the fixed pid prefix and compares what is left, so it fails if the random component is ever dropped. Refs #1728 * fix(ci): the clock gate sees the syscall a session would actually copy Review planted four wall-clock spellings the gate walked past. The load-bearing one: `libc::clock_gettime(libc::CLOCK_REALTIME, ..)`. The engine's own canonical clock read is `libc::clock_gettime(libc::CLOCK_MONOTONIC, ..)` in `core/media_clock.rs` — the file this gate's failure message points readers at — so the likeliest way to introduce a wall-clock read here is to copy that line and flip one token, which the gate could not see. `CLOCK_REALTIME` has zero hits in the tree, so banning it in both language arms cannot false-positive. Python has the mirror hole via `time.clock_gettime_ns(time.CLOCK_REALTIME)`, and the existing `accepts_the_monotonic_spellings` test enshrined that call without ever reading its clock-id argument. `UNIX_EPOCH.elapsed` joins them. The other two evasions — a call split at the `::`, and a wall clock renamed at the import — are the accepted floor of a substring scan, now stated in the module doc next to the trailing-comment limit rather than left for the next reader to discover. `packages/test-fixtures` joins the scan roots. It is engine-side test infrastructure compiled into the engine's own runs, not a consumer, and its 9 processors stamp data. It is clean today so the addition is free. The scan-root doc now names why `packages/escalate` and `packages/core` stay out (schemas only, no source to read) instead of implying the whole of `packages/` is downstream. Also: the api-server's degraded naming path took both indices from disjoint halves of a seed that is a bare pid on that path, and a pid is under 2^16 on any host with the kernel-default `pid_max` — so `>> 32`, and then `>> 16`, both pinned every degraded runtime to one noun. The seed is diffused through a hasher before indexing, as it was before this branch touched it. Corrects two facts in the change-file record: 16 sites span 15 files, not 13, and the cuda adapter's round-trip test took a `TempDir` too. Refs #1728 * fix(engine): test sockets take a TempDir, not a long unique name CodeRabbit caught a regression this branch introduced. `sun_path` is 108 bytes, and moving these paths onto `<pid>-<uuid>` cost 18 of them: headroom under a default `/tmp` fell from 31 bytes to 14, so a `TMPDIR` longer than 15 bytes now fails a bind that used to fit in 46. CI is unaffected — Actions leaves `TMPDIR` unset — but the failure it would produce is an opaque bind error, not a message naming the cause. Both remaining socket sites take a `TempDir`, which is what the surface-client and cuda-adapter tests already did: the path is ~35 bytes with 72 to spare, the socket is unlinked on drop, and uniqueness is the kernel's answer rather than a name we compose. That leaves one idiom for a test socket path across all four sites, and the mint keeps the two namespaces that have no length limit — iceoryx2 service names and pipeline-cache file names. Also from that review: - The mint's docs claimed a collision was impossible. 122 random bits make one vanishingly improbable, which is a different and true statement. - The version test asserted only that the suffix parses, so a timestamp-based UUID would have satisfied it — and a timestamp-based UUID is a wall-clock read, the one thing this ticket exists to keep out of the tree. It now asserts v4. The `uuid` dependency enabling only `v4` is the guard that fires first, via the compiler; this is the one that survives another feature being switched on. - `--help` for `check-clock-usage` listed four scan roots, not five. Refs #1728
3 tasks
tato123
added a commit
that referenced
this pull request
Aug 17, 2026
* ci(ops): fmt and clippy are enforced again, and docs/logging.md stops claiming they already were PR #1857 consolidated 13 workflow jobs into 6 and retired `lint-logging.yml`. Since then **no workflow has run `cargo fmt` or `cargo clippy`** — while `docs/logging.md` went on stating that "`cargo clippy --workspace` fails on any violation" and that a PR is merge-blocked by a workflow that no longer exists. Two `eprintln!` calls sat behind that claim, in the OpenGL and Skia adapter test helpers. **Placement follows cost.** `cargo fmt --all --check` compiles nothing, so it rides `source-gates.yml`, the job that already has a toolchain and no engine build dependencies. `cargo clippy` has to compile the workspace, so it rides `test.yml`'s Linux job, which already carries those dependencies and a warm cache. No new workflow — the consolidation stands. **Default targets only, deliberately.** `--all-targets` would deny `println!` in tests, which `xtask lint-logging` exempts on purpose by skipping `tests` directories. Scoping clippy the same way keeps the two layers saying the same thing. **The two helpers get an allow, not a rewrite.** Both are `[[bin]]` fixtures a harness spawns and reads stderr from, with no tracing subscriber installed — `tracing::error!` there would go nowhere, so stderr is the mechanism rather than a lapse. That is also why the layers disagreed: a `[[bin]]` whose `path` points into `tests/` is a default target to clippy and an exempt path to the AST walk. The doc now says so. Both gates were checked to actually fail: a stray edit trips rustfmt, and removing an allow trips clippy. The rustfmt gate caught its own author's formatting on the first run. `cargo xtask run-local-ci-gates` covers all three, so this stays runnable before pushing rather than only in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(ops): the skia allow narrows to the two functions that write, and the doc stops overclaiming Both from CodeRabbit on #1888, both fair. The skia helper had a file-level `#![allow]` where the OpenGL one had a function-scoped `#[allow]` — inconsistent, and the blanket would have let a `println!` added anywhere else in that file through unchecked. Now scoped to `die` and `crash_mid_skia_write`. Verified it still bites: a stray `println!` in `run()` fails clippy with the narrowed allow in place. And `run_local_ci_gates` cannot run layer 3 — the runtime fd2 capture is a property of the running host, not a gate — so "runs all three" was wrong in the same doc I was correcting for overclaiming. Fixed rather than left ironic. Not taken: "afterwards" → "afterward". No locale is configured (there is no `.coderabbit.yaml`), and the `-wards` forms are already used across `ARCHITECTURE.md`, `docs/logging.md` and the change archive, so the change would introduce the inconsistency it claims to remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(ops): clippy excludes the skia adapter, which no CI job has ever been able to build The new clippy step went red on its first CI run, and not on a lint: `skia-bindings` 404s on its prebuilt download and then needs FreeType headers to build from source. The cause is that this step is the first `--workspace` anything in the repo — every other cargo invocation in every workflow is `-p`-scoped, so no runner has ever built that crate. Adding FreeType would make CI build Skia from source for the first time, which is minutes of build and a new failure surface, to lint one adapter. That is its own decision, not a side effect of turning clippy on. The local aggregator takes the same exclusion. A local gate that lints more than CI is a gate whose result nobody can act on. Consequence, stated rather than buried: `streamlib-adapter-skia` is linted by nothing. That was already true before this PR — it is now visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Owner-directed in session rather than ticket-derived — the ask was "CI takes forever, can we unify it, prune it, and cache properly". Plumbing only: no change to what is verified, only to how many runners verify it and how they cache.
What was actually slow
Measured, not guessed. A representative
check-boundariesrun:cargo xtask check-boundariescargo test -p xtask check_boundariesSeven workflows did exactly that, and all seven shared one cache key, so they raced and six of every seven saves were discarded.
There were never too many tests.
streamlib-engineholds 1396#[test]; CI runs two module filters. Of 65 integration test targets tree-wide, CI runs one. Pruning tests would have been the wrong lever — what was bloated was jobs.Changes
Job consolidation — 13 checks to 6.
check-all-source-gatesruns all eight gates in one process, reporting every failure before exiting, so a single job still surfaces every breakage at once. Onecargo test -p xtaskreplaces eight filtered subsets of the same 178-test binary. License headers and the ship-change gate tests (bash + git only) share a second runner.Caching was a net loss. Repo usage was 11.76 GB against GitHub's 10 GB cap, so every save evicted a live entry; restoring the 3.72 GB
cargo-testentry took 66s to serve 53s of tests. Two causes — caching baretarget/, and ref-scoped saves (three near-identical 1.97 GB wheel entries coexisted: main, PR #1853, PR #1854, none readable by the others). NowSwatinem/rust-cachewithsave-ifon main. The seven orphaned entries are purged.Nothing cancelled superseded runs. Every PR-triggered workflow now has a concurrency group. The two expensive jobs skip prose-only changes; the source gates deliberately do not, since they scan
docs/and Markdown.cargo cimirrors CI, pluscargo gatesfor the fast no-build subset. This required the license check to leave inline workflow YAML — inline bash is by construction something nobody can run before pushing — so it is nowscripts/check-license-headers.sh, shared by both. It discovers files withgit ls-filesrather thanfind: afindmatches CI's clean checkout but produced 15,722 false positives locally, where venvs and uv caches sit in the tree.Second commit corrects the first
The first commit disabled incremental compilation after finding 419 GB in
target/debug/incremental. Measuring the thing that actually matters says that was wrong:cargo check -p streamlib-engineThe 419 GB was months of accumulation across crates, profiles and feature permutations, not a per-build cost —
rm -rf target/debug/incrementalreclaims it in seconds. Trading a 2.7x slower edit loop for that is a bad deal, so incremental stays on locally. CI keepsCARGO_INCREMENTAL=0, where it is unambiguously right.The same commit runs the gates in release locally: they syn-parse ~7k files, so the profile dominates — 11s release against 24s debug. CI deliberately does not mirror it, since its job already compiles xtask in debug for the tests and a second profile would cost more than the 13s it saves.
Verification
cargo test -p xtask— 178 passed, in one runcargo gates— all 8 gates pass, 11.5scargo ci— full local mirror, exit 0.rsand.pyprobes both caught, clean tree greenshellcheckclean; all 8 workflow files parseCaveat on the wall-clock claim: that run was fully cold — caches had just been purged, and PRs deliberately save nothing. Test came in at 161s vs a 150s baseline and Python Wheel at 209s vs 165s. That should invert once this merges and main warms the new caches, but the warm case is projected, not measured.
Notes, not blockers
cargo-nextest(no config exists yet) is the natural vehicle, since per-test process isolation addresses that flake class directly. Worth its own ticket.sdk/streamlib-python-wheel/.venv-pyrightlooked unignored while fixing the license walk, butuv venvwrites a.gitignorecontaining*inside each venv it creates, so it self-ignores. Non-issue.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Maintenance