ci: fix rust caches and cut test wall time#17
Conversation
…xtest Every rust job recompiled all ~490 crates on every run despite "full match: true" cache restores, because rust-cache pointed at the workspace member (apps/backend) while cargo writes artifacts to the repo-root target/. The saved 117MB caches held only the registry. - rust-cache: workspaces ". -> target" everywhere (ci + release) - split lint/test cache keys: caches are immutable and lint always won the save race, freezing clippy check-artifacts into the key the test lanes restored; tests could never save codegen artifacts - cache-on-failure: true so red runs still seed the dep cache - CARGO_PROFILE_DEV_DEBUG=0 in CI: ~30-40% less compile+link, smaller caches; only cost is line numbers in CI backtraces - cargo-nextest in test lanes: one parallel pool across the 4 test binaries (cargo test ran them sequentially - 5.5 min of runtime on the Windows lane alone); pg/mysql keep the 4-thread cap nextest previously broke the suite: it is process-per-test, so the once-per-process leftover-database sweep ran concurrently in every test process and dropped databases of still-running tests. The sweep is now age-gated - the UUIDv7 in each cms_test_* name embeds its creation time, and only databases older than 5 minutes are reclaimed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCI and release workflows were updated to reconfigure rust-cache paths/keys, disable dev/test debug info, and switch Rust test execution from ChangesCI/Release Workflow Updates
Test Database Sweep Gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as ci.yml
participant Cache as rust-cache
participant Nextest as cargo-nextest
Workflow->>Cache: restore cache (. -> target, linux-test)
Workflow->>Nextest: install cargo-nextest
Workflow->>Nextest: run nextest run --no-default-features --locked
Nextest-->>Workflow: test results
Related Issues: None specified. Related PRs: None specified. Suggested labels: ci, testing Suggested reviewers: None specified. Poem A rabbit hops through workflows deep, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/backend/tests/common/test_db.rs (1)
200-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
is_stalehas no unit tests.This function encodes a somewhat fragile string-slicing/parsing contract (prefix strip + fixed-width hex slice) that's easy to get subtly wrong (see hyphen concern above). A couple of unit tests asserting behavior for a fresh vs. old vs. malformed name would catch regressions cheaply.
🤖 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 `@apps/backend/tests/common/test_db.rs` around lines 200 - 205, Add unit tests for is_stale covering the parsing contract around DB_PREFIX stripping and the fixed-width UUID hex slice. Include cases for a fresh database name, an old/stale name, and malformed inputs that fail prefix/length parsing so the early return path is exercised. Place the tests near is_stale and use the same DB_PREFIX-based naming pattern to validate behavior stays correct if the string slicing logic changes..github/workflows/ci.yml (1)
203-205: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the cargo-nextest binary version.
The action is SHA-pinned, but
tool: cargo-nexteststill lets the installed nextest version drift, which can change CLI behavior or scheduling defaults between CI runs. Pin the validated cargo-nextest version in both jobs.Also applies to: 247-249
🤖 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 @.github/workflows/ci.yml around lines 203 - 205, The CI jobs using taiki-e/install-action for cargo-nextest are only pinning the action SHA, not the installed binary version, so nextest can still drift between runs. Update the install configuration in both affected jobs to pin the validated cargo-nextest version explicitly in the install-action inputs, keeping the version consistent wherever cargo-nextest is used.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 203-205: The CI jobs using taiki-e/install-action for
cargo-nextest are only pinning the action SHA, not the installed binary version,
so nextest can still drift between runs. Update the install configuration in
both affected jobs to pin the validated cargo-nextest version explicitly in the
install-action inputs, keeping the version consistent wherever cargo-nextest is
used.
In `@apps/backend/tests/common/test_db.rs`:
- Around line 200-205: Add unit tests for is_stale covering the parsing contract
around DB_PREFIX stripping and the fixed-width UUID hex slice. Include cases for
a fresh database name, an old/stale name, and malformed inputs that fail
prefix/length parsing so the early return path is exercised. Place the tests
near is_stale and use the same DB_PREFIX-based naming pattern to validate
behavior stays correct if the string slicing logic changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4241d973-0d20-46f8-91d2-e51fac42c244
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/release.ymlapps/backend/tests/common/test_db.rs
cargo-deny was already red on main: RUSTSEC-2026-0194/0195 (quick-xml < 0.41 XML-parsing DoS). No upgrade path exists - even object_store 0.14 pins quick-xml ^0.40.1 - and the parser only sees XML responses from the operator-configured S3 endpoint, not user input. Ignored in deny.toml with justification; revisit when object_store allows >= 0.41. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Problem
Every rust CI job recompiled all ~490 crates on every run (~15-19 min wall) even though rust-cache logged
full match: true. Two verified causes:Cargo.lock+target/live there), but every rust-cache step usedworkspaces: apps/backend— caching atarget/that never exists. Saved caches were 117MB of registry only. Warm-run logs showed 484 (Linux) / 493 (Windows)Compilinglines right after a full-match restore.lintand the 3 db test lanes sharedshared-key: linux. GH caches are immutable; lint (2.5 min) always saved first, freezing clippy check-artifacts into the key. Test lanes restored a useless cache and could never save their own.Fix
workspaces: ". -> target"on every rust-cache step (ci + release — release builds were cold on every tag too)linux-lintvslinux-test(the 3 db lanes build identical binaries, DB is chosen at runtime, so sharing among them is safe)cache-on-failure: trueso failed runs still seed the dep cacheCARGO_PROFILE_DEV_DEBUG=0(CI env only): ~30-40% less compile+link, smaller cachescargo testran them sequentially — 5.5 min of pure runtime on the Windows lane). pg/mysql keep the 4-concurrent-test cap.Why nextest is safe now
nextest failed before because it is process-per-test: the once-per-process leftover-DB sweep in
tests/common/test_db.rsran concurrently in every test process and droppedcms_test_*databases of still-running tests. The sweep is now age-gated using the UUIDv7 timestamp embedded in each database name — only databases older than 5 minutes (i.e. leaks from aborted runs) are reclaimed. Bonus: process-per-test also removes the known env-var race between paths/config unit tests.Expected
Warm runs: Linux test lanes ~10 min → ~3-5 min; Windows ~14-18 min → ~5-7 min. Will verify with a follow-up push once this run has seeded the caches.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores