ci: cut the Windows build's redundant pass, Defender scanning, and PDB cost - #349
Conversation
…B cost Measured on run 30237145340: Windows 6m10s against Ubuntu's 1m56s, split as rust-cache restore 54s / 12s, cargo build 1m33s / 16s, cargo test 3m16s / 1m18s. Inside the Windows test step, ~91s is spent purely executing the 835-test suite. Three things, each aimed at one of those numbers: The separate cargo build step is gone. cargo test compiles and links the same libs and bins -- it builds every bin target so integration tests can spawn them, which is what crates/flare-git-shim/tests/shim_test.rs relies on -- so building first was a second full pass over the workspace for nothing. Non-test targets stay covered by clippy --all-targets. Windows runners get Defender exclusions for the workspace, the cargo home and TEMP. Real-time protection is on in that image and scans every object file, PDB and executable rustc writes, plus every temp file the tests create -- the likeliest reason the same suite takes 91s there. Best effort: a runner that refuses the exclusion builds slowly rather than failing. CARGO_PROFILE_DEV_DEBUG=line-tables-only workflow-wide. Nothing in CI attaches a debugger, MSVC spends real link time writing PDBs, and those artifacts are what the cache archive restores every run -- which is the 54s against 12s. Panic backtraces stay readable. Workflow-level so build and clippy produce matching artifacts instead of halving the hit rate. Also de-flakes agentflare-artifacts::update_existing_artifact, which failed this way on a windows-latest runner today (1785126773 against 1785126774) and forced a rerun. It asserted created_at == updated_at, true only while both publishes land in the same second -- and passing for that reason rather than because the invariant held: the same assertion still passed with created_at deliberately rewritten on update. It now backdates the stored value first and checks the update carried it forward, which does fail under that mutation.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CI workflow adjusts Rust debug settings, Windows build preparation, and test compilation behavior. The artifact update test backdates persisted creation metadata and verifies creation-time preservation and update-time ordering. ChangesCI workflow updates
Artifact timestamp test
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@crates/agentflare-artifacts/src/lib.rs`:
- Around line 87-90: Update the backdating logic around meta.replace so only the
root artifact-level created_at field is modified, leaving history[0].created_at
and other nested fields unchanged. Parse and update the root JSON property
directly, then serialize it while preserving the existing created_before value.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71a8ccfc-fcb8-4b8c-9d3d-742119b51888
📒 Files selected for processing (2)
.github/workflows/ci.ymlcrates/agentflare-artifacts/src/lib.rs
…tory The artifact's created_at and its first history entry's start out identical, so replacing the timestamp textually moved both -- leaving the fixture describing a version created an hour before the artifact it belongs to. Edit the root JSON field instead.
|
Fixed in b57e916 — you're right, and the fixture was describing a version created an hour before the artifact it belongs to. Switched to editing the root field as JSON: - let created_before = store.get(&id).unwrap().created_at - 3600;
- let meta = std::fs::read_to_string(&meta_path).unwrap();
- let backdated = meta.replace(
- &format!("\"created_at\": {}", created_before + 3600),
- &format!("\"created_at\": {created_before}"),
- );
- assert_ne!(meta, backdated, "created_at not found in {meta_path:?}");
- std::fs::write(&meta_path, backdated).unwrap();
+ let mut meta: serde_json::Value =
+ serde_json::from_str(&std::fs::read_to_string(&meta_path).unwrap()).unwrap();
+ let created_before = meta["created_at"]
+ .as_u64()
+ .unwrap_or_else(|| panic!("no created_at in {meta_path:?}"))
+ - 3600;
+ meta["created_at"] = created_before.into();
+ std::fs::write(&meta_path, serde_json::to_string_pretty(&meta).unwrap()).unwrap();Still passes, and still fails under the On the timing evidence, being straight about it: this run does not measure the change. Windows step-by-step, baseline run 30237145340 against run 30244129771 on this branch:
The 54s → 7s restore is not a faster restore, it is a miss: And it will stay that way on this branch: So what this run actually establishes is that the workspace still builds and every test passes with the Local gate on this branch is now green end to end ( |
The Windows leg of
buildtakes ~3x the Ubuntu one. Per-step timings from run 30237145340:cargo build --workspacecargo test --workspaceInside the Windows test step: ~60s compiling test harnesses, then ~91s purely executing the 835-test
agentflaresuite (04:37:58 → 04:39:29 in the log). Three changes, each aimed at one of those numbers.1. Drop the separate
cargo build --workspacestepcargo test --workspacecompiles and links the same libs and bins — it builds every bin target so integration tests can spawn them, which is exactly whatcrates/flare-git-shim/tests/shim_test.rsrelies on viaenv!("CARGO_BIN_EXE_git"). Building first was a second full pass over the workspace for nothing.Nothing is lost in coverage:
clippy --locked --workspace --all-targetsalready type-checks the non-test targets, and the bins are still linked by the test step. Saves 1m33s on Windows, 16s on Ubuntu, deterministically.2. Defender exclusions on Windows runners
The
windows-latestimage has real-time protection on. It scans every object file, PDB and executable rustc writes — and every temp file the tests create, of which this suite creates a great many (tempdirs, SQLite files, spawned binaries). That is the likeliest reason the same tests take 91s there.Best-effort by construction: a runner that refuses the exclusion should build slowly, not fail the job, so the
Add-MpPreferencecall is wrapped intry/catch.3.
CARGO_PROFILE_DEV_DEBUG=line-tables-onlyFull debuginfo costs twice: MSVC spends real link time writing PDBs, and those artifacts are what the rust-cache archive has to restore on every run — which is the 54s against 12s in the table. Nothing in CI attaches a debugger, and
line-tables-onlykeeps panic backtraces readable, which is the only debug information a failing job actually uses.Set at workflow level rather than per job so
buildandclippyproduce matching artifacts; divergent profiles between them would halve the cache hit rate. Expect one round of cache misses until master rebuilds its cache under the new setting.Also: de-flake
agentflare-artifacts::update_existing_artifactThis failed on a
windows-latestrunner today (left: 1785126773, right: 1785126774) and cost a full rerun of #348. It asserted:Two problems, not one. It fails whenever the two publishes straddle a second boundary — and it was passing for the wrong reason: I confirmed the same assertion still passes with
created_atdeliberately rewritten on update, because within one second the two values coincide either way. So the invariant it documents was never actually tested.Simply comparing against the previously stored
created_atinherits the same blind spot, so the test now backdates the stored value first, which makes preserved-vs-restamped observable regardless of clock:Verified both ways: passes as-is, and fails when
store.rs'screated_at: prev.as_ref().map(|m| m.created_at).unwrap_or(now)is mutated tocreated_at: now.Verification
cargo fmt --all --checkpasses and the artifacts test was run and mutation-checked locally. The full localcargo test --workspacecould not be completed — this machine's disk hit 0 bytes free partway through (it surfaced as spuriousonly metadata stub found for rlib dependency coreerrors, which are disk-exhaustion in disguise). CI is the verification for this one, which is fitting given what the PR changes; the workflow YAML was parsed and the resultingbuildstep list checked before pushing.Worth watching on this run: whether
build (windows-latest)drops meaningfully below 6m, and whether the first post-merge master run repopulates the cache cleanly under the new debug setting.Summary by CodeRabbit
Bug Fixes
Build & Testing