Skip to content

ci: rust-cache was caching nothing, it needs workspaces - #6

Open
MattJackson wants to merge 4 commits into
mainfrom
ci/rust-cache-workspaces
Open

ci: rust-cache was caching nothing, it needs workspaces#6
MattJackson wants to merge 4 commits into
mainfrom
ci/rust-cache-workspaces

Conversation

@MattJackson

Copy link
Copy Markdown
Contributor

rust-cache was caching nothing here

Swatinem/rust-cache@v2 was used with no workspaces: input. The action looks for one Cargo workspace at $GITHUB_WORKSPACE, and nothing is checked out there: every actions/checkout in these jobs uses path:. The action logged "could not find Cargo.toml" and CARRIED ON WITHOUT FAILING, so the job rebuilt everything from scratch on every run while the log said the cache step succeeded. That is the worst shape a cache misconfiguration can take: it costs full build time on every run and reports success.

Every Cargo workspace the job actually builds is now named. Where the job also builds the sibling busbar checkout, that is listed too, and it matters more than the plugin's own: busbar-plugin-pack plus a full release build of busbar is by far the larger of the two.

Deliberately left alone

Checked every workflow in this repo that uses Swatinem/rust-cache, not just the ones changed. A job whose Cargo workspace genuinely is at the checkout root needs nothing and got nothing, rather than a workspaces: . that would be pure noise. headroom-hook/.github/workflows/docker.yml is one such case (its checkout has no path:), and headroom-hook/.github/workflows/docker-bundle.yml already carried a correct workspaces: busbarAI.

Part of a fleet sweep

Same fix applied across the first-party plugin repos, and to the reusable plugin-ci.yml in core (GetBusbar/busbar#58), which had the same defect and reaches every plugin repo at once.

Swatinem/rust-cache@v2 was used with no workspaces input. The action looks for
one Cargo workspace at the checkout root, and nothing is checked out there:
every checkout in these jobs uses path:. The action logged "could not find
Cargo.toml" and carried on WITHOUT failing, so the job rebuilt everything from
scratch on every run while the log said the cache step succeeded.

Every workspace the job actually builds is now named. Where the job also builds
the sibling busbar checkout, that is listed too: it is by far the larger of the
two builds and the one worth caching most.
@MattJackson

Copy link
Copy Markdown
Contributor Author

Verified on real runners, not assumed

Before, from run 31282480013 (unfixed config, ci/consumer-verification)

The cache step's own log, verbatim:

Error: The process '/home/runner/.cargo/bin/cargo' failed with exit code 101
  commandFailed: {
    command: 'cargo metadata --all-features --format-version 1 --no-deps',
    stderr: 'error: could not find `Cargo.toml` in `/home/runner/work/store-valkey/store-valkey` or any parent directory'
  }
Cache Configuration
  Workspaces:
      /home/runner/work/store-valkey/store-valkey
  Cache Paths:
      /home/runner/.cargo/bin
      /home/runner/.cargo/.crates.toml
      /home/runner/.cargo/.crates2.json
      /home/runner/.cargo/registry
      /home/runner/.cargo/git
      /home/runner/work/store-valkey/store-valkey/target

and then, on the very next line, Cache restored successfully. That is the whole defect in one screen. The single configured workspace is the checkout root, where there is no Cargo.toml because every checkout uses path:. The only target path it cached is a directory that does not exist. Neither real target directory was ever cached. The step reported success and the job was green.

After, from run 31283264799 (this branch)

with:
  workspaces: store-valkey-repo/store-valkey
             busbarAI
Cache Configuration
  Workspaces:
      /home/runner/work/store-valkey/store-valkey/store-valkey-repo/store-valkey
      /home/runner/work/store-valkey/store-valkey/busbarAI
  Cache Paths:
      ...
      /home/runner/work/store-valkey/store-valkey/store-valkey-repo/store-valkey/target
      /home/runner/work/store-valkey/store-valkey/busbarAI/target

The could not find Cargo.toml error is gone and both real target directories are now cached.

Warm run, 31283395287

Cache hit for: v0-rust-migrate-destructive-wipe-test-Linux-x64-e8b3ee54-9dc4cd49 followed by Cache restored successfully, with no error.

The timing number, stated honestly

Cold 26s, warm 22s on the cargo test step. That is a real but modest delta, and it would be misleading to present it as the headline. Two reasons it is small here, both worth stating rather than glossing:

  1. The "cold" run was not truly cold. The job-id cache key is unchanged by this PR, so that run already restored a populated ~/.cargo/registry saved by earlier runs of the BROKEN config. The registry was always being cached; only the target directories were not. So this delta measures the target-dir contribution alone.
  2. This particular job builds very little: one small crate and a --ignored single test. The jobs where the missing target cache actually costs real money are the ones that build busbar itself (release.yml, and core's reusable plugin-ci.yml), and those run on tags rather than on pushes, so they are not cheaply measurable from a PR.

The load-bearing evidence is therefore the structural before/after above: the workspaces now resolve, the error is gone, and the target directories are in the cache paths where previously a nonexistent directory was. The timing is consistent with that and is not, on this job, dramatic.

Note on this PR's red check

The ci / build, test, clippy, fmt, signoff job is red on PUBLIC-HYGIENE GATE, which is pre-existing on main and unrelated to this change. Its fix is open separately as #5. Confirmed by running the lint directly: main reports hits and FAILED, the #5 branch reports 0 hit(s), 0 allowed and passes. Merge #5 first and this goes green.

Two independent ways this job's only real assertion could vanish silently.

1. `cargo test <filter>` is a SUBSTRING match with NO floor. Rename the test --
   dropping just `the_` from `wipes_the_entire_namespace_destructively` is
   enough -- and the filter matches zero tests:

     running 0 tests
     test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
     exit status 0

   The job goes green, having stood up a dedicated valkey/valkey:8 service
   container to run nothing at all, and because the test is `#[ignore]`d no other
   job covers it either. The step now asserts `test result: ok. 1 passed`, so a
   rename fails loudly instead of quietly deleting the coverage.

   Deliberately NOT `--exact`: the `1 passed` floor is the load-bearing half and
   cannot break on a guess about the module path.

2. `plugin_path()` in the plugin's e2e tests was the only one of the ten sibling
   plugin repos without the `is_none() && CI -> panic!()` guard -- while
   `valkey_url()` seventeen lines below it in the same file has it. Demonstrated
   rather than assumed, with the cdylib absent:

     running 3 tests
     test admin_api_installs_the_valkey_plugin_and_writes_land_in_real_valkey ... ok
     test load_and_exercise_valkey_plugin_bad_config_fails_over_abi ... ok
     test load_and_exercise_valkey_plugin_persists_to_real_valkey_across_reopen ... ok
     test result: ok. 3 passed; ... finished in 0.00s

   Three tests "passed" in 0.00s without their subject existing. It now checks
   both the uplifted target dir and target/deps (a bare `cargo test` uplifts to
   neither for this crate), and refuses to skip under CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant