ci: reuse Linux native libraries across workflow runs

[sunchao]

Which issue does this PR close?

Part of #5830. Rebased onto main after #5973 merged, incorporating its main-only cache writes to reduce eviction pressure from PR and merge-queue runs. Complements #5841, which shares native builds within one workflow run.

Rationale for this change

#5973 stopped large caches from being written by PR and merge-queue runs, allowing main's compiled native cache to survive. The normal comparison is now a warm incremental build. In two post-#5973 runs, the #5991 native job took 3m51s and a nightly Linux native job took 4m13s. Restoring the target cache took 43–47 seconds and Cargo took 1m57s–2m22s. Both restored a fallback entry and rebuilt workspace crates; these are warm-build baselines, not measurements of this PR's exact library-hit path.

This PR removes the remaining restore/build work when the native inputs already have a published library. JVM tests need libcomet.so, so a matching run can download that file instead of the roughly 1.4 GiB compiler cache and skip Cargo. Smaller entries also consume less of the shared cache budget. The older 23-minute cold-build example is no longer the expected saving after #5973; the actual reduction in job time and PR turnaround still needs measurement after main publishes the library cache.

For example, a PR that changes a Scala planner rule, then updates a Scala test after review, needs the same native engine for both runs. With matching inputs and environment, each run can restore the finished library and proceed with the changed JVM code. Rust or protobuf edits still require compilation. Queue time, setup and downstream tests remain part of overall PR turnaround.

What changes are included in this PR?

The Linux, Spark SQL, Iceberg, and manual writer workflows share a build-or-restore step for the finished native library. Before invoking Cargo, it computes a fingerprint of the native sources, protobufs, dependencies, build configuration, and observed toolchain environment. For PR, queue, scheduled and manual runs, an exact cache match restores libcomet.so and skips native compilation. A miss restores available intermediate Cargo files and runs cargo build --locked --profile ci. The --locked flag deliberately fails if a manifest change requires updating native/Cargo.lock: contributors must include that lockfile update in the PR. Restoring intermediate files alone always leaves Cargo responsible for checking and rebuilding them.

Main produces the reusable libraries, and PRs consume them. Only pushes to main save these native-library and Cargo caches. Main skips Cargo when both the finished-library and incremental cache entries match exactly. When either lacks an exact match, main builds to replenish the missing entry; this maintains the intermediate files needed by PRs that change native code without recompiling inputs whose two entries are already available. Changes to shared native inputs trigger main's cache-building jobs. The fingerprint and main's routing share the same input lists and glob matcher, keeping the producer aligned with the cache key.

This makes the reuse boundary explicit. A Scala-only change can reuse main's library when the native inputs and environment match. A change to library Rust code or protobuf inputs needs a new build. If that Rust-changing PR later receives a Scala-only update, it still needs a native build: its unmerged Rust changes remain part of the fingerprint, and PR runs do not publish their own cache entries. The fingerprint describes the current checkout, including changes from earlier commits in the PR.

The fingerprint follows what the native build actually consumes. It includes the observed Rust, system-package, and JDK versions, plus the build environment: Cargo/Rust settings, C/C++ compiler and flag overrides (including target-specific variants), and the HDFS library overrides used by the default dependencies. The shared build and setup actions remain in the key; the four caller workflows do not. For example, moving a Spark suite between shards no longer discards a reusable native library. The compiler and JDK selected by those callers are observed directly instead.

This contract is scoped to the existing official Linux builder. An environment variable that points to an arbitrary external tool or file does not fingerprint that file's contents; introducing such inputs requires updating the contract. Documentation, generated outputs, and benchmarks are excluded from the library key. Disabled contrib crates contribute their manifests, which Cargo still resolves, but their Rust sources and standalone lockfiles do not invalidate this default-feature build. Rust checks and tests continue to run with a separate debug cache that includes benchmarks; downstream JVM tests use the restored or rebuilt library through the existing artifact flow.

The incremental fallback retains the build-environment identity too. Default HDFS support compiles C against JNI headers and links libjvm; Cargo does not fully track external compiler/header changes. Keeping that boundary can miss after unrelated package updates, but avoids carrying old native objects into a library published under a new environment key. Both CI and debug incremental caches contain only native/target, including compiled dependencies. Cargo re-fetches registry and Git dependency sources as needed, keeping those downloads out of the shared cache budget. In the #5991 run above, re-fetching sources preserved the compiled dependencies and only the six Comet workspace crates rebuilt. This trades dependency download time and network availability for smaller caches. JVM workers that only consume the compiled library no longer restore an unused Cargo cache.

How this composes with #5841

This PR reuses a library across workflow runs. #5841 shares one producer among the selected Linux, Spark SQL and Iceberg consumers within a run. Together, that producer restores or builds once and distributes the library to the selected consumers. Ordinary unlabeled PRs already have one covered native producer; #5841's duplicate-build savings mainly apply to merge-queue, nightly and manual runs. Either implementation can land first, but the planned integration is to land this PR, then rebase #5841 and use this composite action in its shared producer while retaining the main-only cache-write policy.

Cache migration and rollout

The new Linux-cargo-ci-v3- and Linux-cargo-debug-v3- keys deliberately do not restore the old entries, whose keys lack the environment boundary. The first main build will therefore be cold. The merge-time plan is to inventory and explicitly delete the superseded legacy Cargo cache entries on refs/heads/main by their cache IDs, then let the first main push populate the new target-only CI/debug entries and the finished-library entry. Restrict cleanup to those inventoried legacy native-cache entries, preserving new namespaces and Maven/dataset caches. This avoids retaining both large generations indefinitely; PRs still using the old keys will lose their warm cache and should rebase to consume the new caches. Cache cleanup is a merge-time operation, not part of PR CI.

Measure all three compressed entry sizes and verify their retention after that first main push. Then confirm that a matching PR restores the library, skips Cargo and passes downstream tests. Record native-job and end-to-end PR timings against the warm baseline above, and track package/JDK changes behind misses. A library hit and an exact target hit describe the same build inputs, but their entries can be retained or evicted independently; the compact cache's benefit depends on both transfer/build savings and actual availability.

Precedents for this approach

ClickHouse uses a closely related approach: its CI combines build inputs and container/configuration digests and reuses artifacts from an earlier matching run. Microsoft's vcpkg binary cache similarly identifies reusable native packages from their build inputs, compilers, dependencies, and configuration. Bazel remote caching applies the same principle to individual build steps. This PR applies it to Comet's finished native library, with a fingerprint maintained alongside the existing Cargo workflows.

How are these changes tested?

Focused fingerprint and action-flow regression tests cover the cache contract. They check that native, dependency, toolchain, and configuration changes invalidate the appropriate keys; unrelated changes and disabled contrib sources preserve reuse; CI and debug caches remain separate; container checkout ownership is handled; and representative library inputs select main's cache-building job through the shared input patterns. This includes contrib-manifest and nested-path regressions, build-environment overrides, and stability across caller-workflow edits. The action-flow tests also cover library and incremental hits/misses, main rebuilding a missing entry, and main skipping Cargo when both entries match exactly. Validation includes CI configuration checks, actionlint, Markdown formatting, and whitespace checks.

A disposable Cargo build with one registry and one Git dependency also verified the target-only tradeoff: after deleting only its dependency-source directories, Cargo re-fetched the sources, reused both compiled dependencies and the application, and produced the same result. This complements the hosted Comet evidence above; it does not promise a fixed download time.

All selected hosted checks passed at bd4ee8a14. The target-only update at 80296cb7b passes the local checks above; hosted validation for this head and actual cross-run library reuse remain pending.

[andygrove]

Thanks for this. Collapsing four copy-pasted cargo cache blocks into one composite action is a clear win on its own, and the security posture is right: only push-to-main saves and pull requests only consume, so a fork PR cannot plant a libcomet.so that another PR then executes. That is the property that matters most in a scheme like this and it is handled correctly. The lookup-only flag on main so the producer does not download a library it will not run is a nice touch, and fixing the ~/.cargo versus /usr/local/cargo CARGO_HOME mismatch is a real latent bug fix. The test file using throwaway git repositories rather than mocks is also good to see.

A few things I would like to work through before this lands.

Sequencing against #5973

I think this needs #5973 in front of it, and I would rather not merge it first.

I queried the cache API while reading this. The repository reports 9.06 GB in use, and refs/heads/main holds nothing at all. Everything live is on refs/pull/5420/merge, refs/pull/5615/merge, refs/pull/5565/merge and one gh-readonly-queue/main/pr-5934-* branch, and there is no Linux-cargo-ci-* or Linux-cargo-debug-* entry left anywhere. That independently reproduces what you documented on #5973 from 2026-09-15.

The concern is that this PR's whole premise is that main publishes the library and pull requests restore it, but a run can only restore from its own ref plus the default branch. With main holding nothing and 2.1 to 2.3 GB Maven entries still being written from PR refs, I would expect the new library entry to be evicted before any pull request gets to read it. This PR is well behaved on its own writes, so it cannot fix that from here. It also adds two fresh namespaces, Linux-cargo-ci-v3- and Linux-native-ci-v2-, which consume budget while delivering nothing until the eviction pressure is gone.

Would you be up for landing #5973 first and then rebasing this onto it? That would also give you a real hit rate to put in the description instead of the current 28m45s cold build.

The incremental restore prefix gets weaker than what it replaces

In cache_keys, the prefix is Linux-cargo-{profile}-v3-{digest([environment, dependencies])}-, and environment carries the full dpkg-query -W package list along with rustc -vV, java_release, java_home and cargo_home.

setup-builder runs apt-get update && apt-get install -y protobuf-compiler clang against amd64/rust, which is an unpinned rolling tag. So package versions can drift between main's producer run and a pull request run hours later with no repository change at all. When that happens we miss the binary key and the incremental prefix together and get a fully cold build. Today the fallback is just the Cargo.lock and Cargo.toml hash, so it would still restore.

Could the prefix stay coarse and keep packages in the binary key only? That keeps the exact-match safety where it matters without giving up the incremental fallback.

contrib/*/native/** in the binary key

native/Cargo.toml carries exclude = ["../contrib"] and pulls the contrib crates in only as optional path dependencies behind their features, and the CI build is cargo build --locked --profile ci with no contrib feature enabled.

Does a contrib/*/native/** change actually affect libcomet.so? If it cannot, having it in INPUT_PATTERNS means a Delta-only change invalidates the shared library key for every consumer.

The JDK entries reverse a documented invariant

This drops # Note: Java version intentionally excluded - Rust target is JDK-independent from the debug key, and environment_inputs now feeds java_home and java_release into both keys.

Was that comment wrong? If the JNI headers and libjvm genuinely are build inputs it would help to say so where the old note used to be, since this directly contradicts it. If the target really is JDK independent, leaving the JDK out would avoid invalidating everything on a toolcache patch bump. Worth noting java_home is a path carrying the exact version, so it moves on patch bumps too. Every caller passes java: 17 today, so nothing is fragmented across jobs right now, but that is the part I would not want to rely on silently.

Two glob dialects for one set of paths

native-cache-key.py matches with fnmatch.fnmatchcase, where * crosses / and ** carries no special meaning. compute-changes.py matches with glob_to_regex, where * becomes [^/]* and ** is recursive. The same path strings now appear in three places: INPUT_PATTERNS, the new inline list inside compute(), and the FILTERS loop.

Concretely, contrib/a/b/native/x.rs enters the binary key under the first dialect but does not warm main under the second. test_every_binary_key_input_has_a_main_cache_warmer only walks files present in the tree today plus three hardcoded names, so that kind of drift would not be caught.

Could the helper import the matcher from compute-changes.py so there is one dialect and ideally one list?

--locked is a behaviour change worth calling out

The build step goes from cargo build --profile ci to cargo build --locked --profile ci. A pull request that edits Cargo.toml without refreshing Cargo.lock now fails the build rather than updating the lock. That seems like the right call for something we are going to cache and reuse, but it is not mentioned in the description and it will surprise someone. Worth confirming it is deliberate and noting it there.

[sunchao]

Thanks, @andygrove, for the review. I pushed e3b9e8542 and updated the description. Going through the six points:

1. Sequencing: agreed. The description now explicitly depends on ci: write large actions/cache entries only on push to main #5973 landing first and calls for rebasing onto it before merging. ci: write large actions/cache entries only on push to main #5973 is still open, so that rebase and verification of main's cache retention remain pending. The description continues to distinguish the sampled cold-build cost from savings that still need measurement.

2. Incremental fallback: I kept the package/JDK compatibility boundary for the compiled target cache after checking the native dependencies. There is a concrete correctness issue with relying on Cargo alone here: in a small offline build using Comet's exact locked cc 1.4.5, changing an external C header from a value of 1 to 2 left the ordinary rebuild returning 1; cleaning the target produced 2. Replacing a compiler at the same path similarly left its old output cached until cleaning. The default hdfs-sys dependency compiles C against JDK headers, and those external inputs are not fully tracked by its build scripts. A coarse target restore could therefore publish old native objects under the new library fingerprint. I documented why the environment stays in the fallback prefix and extended the existing test to cover that boundary. Unrelated package updates can still cause misses; narrowing that identity needs evidence about the actual native toolchain inputs, or a pinned builder.

3. Contrib inputs: narrowed to contrib/*/native/Cargo.toml. Changes to disabled contrib Rust sources and their standalone lockfiles now preserve the key and no longer select the shared Linux cache warmer. The manifests stay included because Cargo resolves optional dependencies when validating the native workspace lockfile, even when those features are disabled.

4. JDK identity: the old JDK-independent comment was wrong for the default HDFS build. It uses JNI headers and links libjvm; core/build.rs already documents a stale cached JDK-path failure. I added that explanation beside the debug fingerprint and retained the JDK identity.

5. Glob dialects: the key helper now imports the existing matcher and shared native-input lists from compute-changes.py. Main's warmer uses that same library-input list. The matcher module itself is included in the fingerprint, and the existing tests cover nested contrib paths as well as the supported one-level manifests.

6. --locked: deliberate, and now explicit in both the description and workflow documentation. A manifest edit that requires a new native/Cargo.lock fails CI until the lockfile update is included.

The six focused tests, CI configuration checks, actionlint, Markdown formatting, and whitespace checks pass. The expanded contrib/matcher regressions failed before these changes and pass afterward. Hosted CI for this new head is pending; actual cross-run library reuse still needs verification after main populates the cache.

[andygrove]

Thanks for the revision. I re-read the whole thing at e3b9e8542 and checked out the branch to verify the answers rather than just reading the diff. Four of the six are clean as far as I can tell:

The glob dialects really are unified now. contrib/a/b/native/Cargo.toml gets the same answer from the fingerprint and from main's routing, and the tests pin both directions. The contrib narrowing is right, and the rationale holds up: native/Cargo.toml has exclude = ["../contrib"], native/core/Cargo.toml has default = ["hdfs-opendal"] with no contrib feature, and both comet-contrib-delta and comet-contrib-lance do appear in native/Cargo.lock, so --locked genuinely depends on those manifests. The JDK point is settled: hdfs-opendal is a default feature, it pulls hdfs-sys, and core/build.rs reads JAVA_HOME, so the old comment was simply wrong. And --locked is now called out in both places.

On the incremental prefix I was wrong and you were right. The stale C object argument is the correct one and I should have tested cc's header tracking before asserting the old fallback was strictly better. The paragraph you added explaining it lands exactly where I wanted it.

I also want to flag something structural that I did not say clearly enough the first time, because it is the lens for most of what follows. An exact binary key hit is the first cache in Comet's CI that skips compilation outright. Every cargo cache we have had until now was an incremental aid where Cargo still re-validated everything, so an incomplete key cost time and nothing else. Here an incomplete key produces a wrong library that the JVM suites then test against. That raises the bar on key completeness a long way, and it is worth the two of us being paranoid about it.

Five things from this pass.

The four caller workflows in the library key are the biggest source of churn

NATIVE_BUILD_INPUTS hashes pr_build_linux.yml, spark_sql_test_reusable.yml, iceberg_spark_test_reusable.yml and spark_sql_writer_tests.yml into the library key. I counted commits touching each library key input on main over the last 90 days:

input	commits
.github/workflows/pr_build_linux.yml	57
native/Cargo.lock	40
dev/ci/compute-changes.py	17
.github/workflows/spark_sql_test_reusable.yml	9
.github/workflows/iceberg_spark_test_reusable.yml	7
.github/workflows/spark_sql_writer_tests.yml	4
.github/actions/setup-builder/action.yaml	0

pr_build_linux.yml is the highest churn input in the entire key, ahead of Cargo.lock, at roughly one edit every 1.6 days. Each of those is a cold native build for the PR that makes the edit.

After this PR I do not think those files carry anything the key needs. The build recipe moved into .github/actions/build-native-ci, which is already in the key. The only native relevant content left in the callers is the container image, RUST_VERSION, the JDK version and RUSTFLAGS, and environment_inputs already observes all four directly through dpkg-query, rustc -vV, $JAVA_HOME/release and the step env. The observational probe is the stronger guard anyway, because it also catches a base image change that no file in the repository records.

spark_sql_writer_tests.yml is the clearest case. It is workflow_dispatch only, so a manual workflow that never runs on a PR currently invalidates the shared library for everybody.

Could NATIVE_BUILD_INPUTS keep .github/actions/setup-builder/** and NATIVE_CACHE_RECIPES and drop the four workflow paths?

environment_inputs allowlists three variables

It reads JAVA_HOME, CARGO_HOME and RUSTFLAGS. Everything else that reaches the compiler passes through unrecorded: CC, CXX, CFLAGS, PROTOC, RUSTC_WRAPPER, CARGO_BUILD_*, CARGO_PROFILE_CI_*.

I checked all four callers and none of them sets anything beyond RUST_VERSION, RUST_BACKTRACE and RUSTFLAGS, so there is no bug in this revision. What bothers me is that nothing in the new tests or in check-ci-config.py would notice a fourth variable appearing, and per the point above this is the one place where being wrong produces a stale library rather than a slow build.

Would a prefix sweep be safer than an allowlist? Something like

"env": {k: v for k, v in sorted(env.items())
        if k.startswith(("CARGO_", "RUST"))
        or k in {"CC", "CXX", "CFLAGS", "CXXFLAGS", "LDFLAGS", "AR", "PROTOC", "JAVA_HOME"}},

It picks up RUSTFLAGS, CARGO_HOME and JAVA_HOME for free, stays deterministic on the fixed builder, and means a future env: addition invalidates the key by default rather than by someone remembering to update this function. It matters more if you take the previous point, since dropping the caller workflows from the file list leaves this probe as the only guard.

RUSTFLAGS is written out twice in the composite

Lines 29 and 58 of .github/actions/build-native-ci/action.yaml carry the same literal, once as the env the key is computed under and once as the env the build runs under. Those two have to agree or the key describes a build that did not happen, and nothing fails if they drift.

Composite actions do not take a runs: level env:, but a leading step would do it:

- name: Pin native build flags
  shell: bash
  run: echo 'RUSTFLAGS=-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd' >> "$GITHUB_ENV"

and then both step level env: blocks can go. That also makes the flags visible to the sweep above, if you take it.

The test file is routed to the merge queue suites

The _native_consumer loop appends dev/ci/test-native-cache-key.py alongside the recipes. I diffed the routing before and after: on merge_group, editing only that file now selects spark_4_1, spark_4_1_hive and iceberg_1_11, which are the three heaviest suites left on that tier after #5963.

The test cannot affect the library, the routing or the recipe, and Preflight already runs it on every event through the new "Check native cache keys" step, so those three suites are not checking anything. The comment above NATIVE_CACHE_RECIPES says tests "are routed separately below", but separately turns out to be the same nine jobs.

Dropping it from the loop leaves it covered by Preflight plus the existing dev/ci/** route into build_linux. I would keep dev/ci/compute-changes.py in the loop, since that one really is in the fingerprint.

The CARGO_HOME fix makes the entry bigger, not the same size

Worth saying explicitly in the description. ~/.cargo/registry does not exist in the amd64/rust container, so today's Linux-cargo-ci-* entry only ever held native/target. Pointed at /usr/local/cargo it will now also carry the registry for a 675 crate lockfile and the iceberg-rust git checkout.

I re-queried the cache API while reading this revision. The repository is now at 17.34 GB across 22 entries, up from the 9.06 GB I reported yesterday, refs/heads/main still holds nothing, and there is still no Linux-cargo-ci-* or Linux-cargo-debug-* entry anywhere. Everything live is on a PR merge ref or the merge queue, dominated by Linux-java-maven-* entries at 0.9 to 1.9 GB each. That is not an objection to the design, it just confirms the sequencing you already agreed to, and it means the new namespaces will be landing into a tighter budget than the description assumes.

Could the first main push after this lands report the measured size of both new entries?

Two smaller things

native-cache-key.py reaches for runpy.run_path and test-native-cache-key.py reaches for importlib.util.spec_from_file_location, for the same "this filename has a hyphen" problem, and then line 119 of the test goes back to runpy. Worth picking one so the next person copying either file gets a consistent answer.

digest and command have docstrings about as long as their bodies. The longer ones further down earn their keep, particularly the note on why the environment stays in the fallback prefix.

One question about shape

Since #5973 is still open and this is waiting on it, is there a case for landing the composite on its own first? The extraction, the CARGO_HOME fix and deleting the read-write Linux-cargo-registry-* cache from the linux-test matrix all stand alone, and that last one only ever ran in a job with skip-native-build: true that never calls cargo, so it is pure budget back at a moment when we are short of budget. The composite could carry the existing hashFiles key at first and take the fingerprint in a follow-up once main is actually retaining entries.

Happy to be told the double churn on the composite's key is not worth it.

---

For what it is worth, on the things I could check locally the fingerprint looks complete. The six new tests pass, check-ci-config.py and the Iceberg shard tests still pass, and on the real tree 297 of the 399 tracked files under native/ land in the CI key with the other 102 being exactly the 6 markdown files and the 96 under benches/. There is no include_str! or include_bytes! anywhere in native/, so excluding markdown is safe, and there is no [patch] section or path dependency outside native/ and contrib/. I walked the six step conditions in the composite by hand, including the case where steps.cargo-cache is skipped and cache-hit evaluates to the empty string, and did not find a hole.

[sunchao]

Thanks, Andy. Addressed this pass in 347fb07 and updated the description and workflow documentation.

Caller workflows and environment. The four caller files are out of the fingerprint. The shared setup/build actions remain included, and the observed environment now captures Cargo/Rust controls, compiler/linker/protobuf overrides, target-qualified compiler variables, and the HDFS controls used by our existing dependencies. For example, a shard edit preserves the key, while CARGO_PROFILE_CI_OPT_LEVEL, CC_x86_64_unknown_linux_gnu, or HDFS_LIB_DIR changes invalidate it. The documentation keeps the scope explicit: this describes our official builder; recording a path to an arbitrary external tool or library does not identify its contents.

One small clarification on the earlier behavior: a caller edit invalidated the finished-library key, but it preserved the incremental restore prefix when dependencies and environment were unchanged, so compilation was required without necessarily being cold.

Flags and routing. RUSTFLAGS is now defined once through GITHUB_ENV, before fingerprinting and compilation. The test file is removed from the explicit consumer loop. It still runs in Preflight and retains existing Linux routing, but test-only edits no longer select the extra Spark/Iceberg suites on the merge queue or nightly tier. The actual cache recipes remain in that loop.

Cache size and sequencing. The description now explicitly says that fixing CARGO_HOME grows the incremental entry by adding registry/Git contents, separately from the new finished-library entry. The first-main validation calls for reporting both compressed cache sizes from the save logs or cache API, then observing an exact library hit and passing downstream tests. Those measurements are still pending main publication. #5973 remains an explicit prerequisite, followed by rebasing this PR before merge. I would keep this PR together for now: the extraction and reuse behavior share one recipe, and splitting it would add another transition without removing the cache-retention prerequisite.

Smaller cleanup. Both files now use the same importlib loading idiom, the test reuses the already-loaded matcher, and the two short helper docstrings are removed. The longer contract explanations remain.

Validation: the existing six cache-key tests pass with expanded caller/environment/routing coverage; all 15 Iceberg shard tests, CI configuration and suite checks, benchmark-runner checks, actionlint, Markdown formatting, and whitespace checks pass. Independent review found no further issues. Hosted CI for this new commit is pending; the previous head's selected checks all passed.

[comphead]

Thanks @sunchao that makes sense to me as a direction, I came across the same yesterday and then realized you already have a PR.

Let me spin up an automated review process

[viirya]

Reviewed at bd4ee8a14bfd42fe7a35e6219985f667936c3bc9, including the previous review discussions and author responses.

The earlier concerns appear addressed: the fingerprint and routing now share their input definitions and matcher, disabled contrib sources no longer invalidate the library, caller workflow edits are excluded while the build environment is observed, and RUSTFLAGS has a single definition. Keeping the JDK/package boundary in the incremental restore prefix is justified by the native dependencies’ external inputs. The prerequisite #5973 has also landed and is included in this branch.

I did not find a blocking correctness issue in the current implementation. Only an exact library hit skips Cargo, main still builds after a lookup-only hit, and the existing artifact paths and downstream JVM packaging remain consistent. The independent Rust checks and tests continue to run.

I reran the six cache-key tests, CI configuration checks, and fifteen Iceberg shard tests successfully. The hosted checks for this head also passed, although the native job exercised a cache miss. Actual cross-run library reuse, retention, and turnaround improvements therefore remain to be verified as documented.

Two non-blocking suggestions below concern coverage of the action’s control flow and including the debug cache in the size measurements.

[comphead]

Review: reuse Linux native libraries across workflow runs

Solid design and the rationale holds up where I checked it. No correctness blocker found.

Verified while reviewing

• All four callers run the native build in the same amd64/rust container at JDK 17 (pr_build_linux.yml:364, spark_sql_test_reusable.yml:83 with every ci.yml call passing java: 17, iceberg_spark_test_reusable.yml:78, spark_sql_writer_tests.yml resolving to 17), so one key really is reachable from all of them.
• No native build input escapes the fingerprint. native/core/build.rs and native/proto/build.rs read nothing outside native/, there is no include_str!/include_bytes! reaching out, all seven .proto files live under native/proto/src/proto/, and the only path dependencies outside the workspace are contrib/{delta,lance}/native, both covered by contrib/*/native/Cargo.toml. The JAVA_HOME handling in core/build.rs confirms the JDK-in-key rationale.
• native/target/ci/libcomet.so is the only build output any consumer needs, so restoring just that file is sufficient for the artifact upload and for the writer workflow's stage-to-release step.
• The importlib load writes dev/ci/__pycache__, which is gitignored, and check-working-tree-clean.sh runs only in lint, so the helper cannot dirty a checked tree.
• Ran the new suite locally: 6 tests pass in 0.8s. check-ci-config.py passes with the dev/ci part of the diff applied.

Findings (inline)

Major - main rebuilds on a double cache hit, which is the common main push. The library key takes the whole dpkg database, which shortens entry life for packages that cannot affect libcomet.so.

Minor - source_inputs discards the blob OID that git ls-files --stage already gives it and then re-hashes every file, in two passes, with a whole-changeset predicate called per file. Two of the three NATIVE_CACHE_RECIPES routes added to build_linux are already covered by its dev/ci/**. compute() hardcodes the push tier outside POLICY. binary-key="" is dead output. Two tests assert one property many times. The README section carries a one-off verification checklist.

I did not find anything to move to SQL-file tests: this change has no expression or operator surface.

[comphead]

This swaps a step-level env: for a job-scoped one. The value is right: all four callers declare a workflow-level RUSTFLAGS: "-Clink-arg=-fuse-ld=bfd", and GITHUB_ENV does win over it, because the runner evaluates workflow and job env once into the same dictionary that GITHUB_ENV later writes to (JobExtension.InitializeJob populating Global.EnvironmentVariables, then StepsRunner seeding each step's env context from it).

What changes is scope. -Ctarget-cpu=x86-64-v3 now applies to every later step in the caller's job, where the old step-level env: applied to the one cargo build. Nothing downstream invokes Cargo today, so this is not a bug, but it is an unflagged widening. Dropping this step and putting env: RUSTFLAGS: ... on the fingerprint and build steps restores the old scope and removes a step.

[sunchao]

The scope observation is correct. I kept the single definition because Andy explicitly requested removing the duplicated fingerprint/build literals in the previous review. These are native-producer jobs, and their remaining steps upload or package the existing library without invoking Cargo. There is no affected downstream build today. Reintroducing the two copies would reverse that agreed simplification, so I have left this unchanged and am keeping the thread open for discussion.

[comphead]

Three things collapse here.

1. git ls-files --stage already emits the content hash. metadata.split() is [mode, oid, stage], and this keeps [0] and throws away [1], then re-reads and SHA-256s every file. Keying on mode + oid removes all the file I/O and the FileNotFoundError that an index entry with no worktree file would raise. The docstring already scopes this to a clean checkout, and untracked generated files never appear in ls-files, so hashing the working tree buys nothing the index does not already give.

2. dependencies on L68 is a second pass over the dict just built. Both maps fit in the one loop.

3. CHANGES.matches() is a whole-changeset predicate being invoked once per file, so it rebuilds the compiled include/exclude lists on every call. Sharing the semantics with compute-changes.py is the right instinct, but the reusable unit is the matcher, not the any-file wrapper. A compile_matcher(patterns) there that returns a predicate, with matches() calling it too, gives the same guarantee without the per-file rebuild.

For calibration, I measured this on the current tree: 309 files and 5.2 MB, 0.15s. So this is about single-traversal clarity, not runtime.

[sunchao]

The index-OID approach is reasonable under the clean-checkout contract. I have kept this part unchanged in the focused revision: hashing working-tree bytes directly describes the files Cargo sees, and the measured 0.15s does not justify changing that behavior or adding a matcher API here. The dependency comprehension is also small and readable. I did simplify the separate routing test so it no longer scans and hashes the repository inventory. Leaving this thread open since the helper refactor is deferred.

[comphead]

Major. The library key takes the entire dpkg database. amd64/rust is an unpinned latest tag that the runner re-pulls per job, and setup-builder runs apt-get update before installing, so any archive refresh discards a finished library over packages that cannot affect libcomet.so: tzdata, ca-certificates, git, imagemagick and the rest of the buildpack-deps base.

The stated rationale is the C/JNI boundary in hdfs-sys and core/build.rs, and that needs only the toolchain: clang*, gcc*, binutils, libc6-dev, libstdc++*, protobuf-compiler. Restricting dpkg-query -W to those keeps the invariant you actually depend on and materially extends how long an entry stays usable, which is the thing this PR is buying. Worth folding into the hit-rate measurement you already planned rather than deferring it, since it decides whether the reuse pays off at all.

Same question, smaller, for java_release on L93: the full release file rotates on every Zulu 17 patch, while the JNI headers it stands in for essentially never change.

[sunchao]

Unrelated installed-package changes can indeed cause conservative misses. I retained the current boundary because the suggested package list does not cover the full compiler dependency set. In the official Rust image I inspected, GCC's cc1 also links libisl23, libmpc3, libmpfr6, libgmp10, zlib1g and libzstd1. GCC constrains these with minimum versions, so they can change while the proposed whitelisted package versions remain unchanged. Narrowing this safely needs the complete set of relevant toolchain dependencies. Also, apt-get update alone changes repository indexes, not the installed versions this helper hashes.

I retained the JDK identity too: default HDFS builds against its JNI headers and links libjvm. Removing java_release alone would still leave patch-specific JAVA_HOME and PATH values in the key. The PR's rollout plan now explicitly calls for tracking package/JDK changes behind misses, alongside sizes, retention and hit behavior. Leaving this open rather than claiming the proposed narrowing is implemented.

[comphead]

binary-key="" is written to $GITHUB_OUTPUT for the debug profile and no caller reads it. Omitting the entry when profile != "ci" makes a caller that passes the wrong profile fail on a missing output instead of silently keying a cache on an empty string.

[sunchao]

Omitting the unused output is possible cleanup, but it would not add the proposed failure behavior: GitHub evaluates a nonexistent context property to an empty string, so steps.key.outputs.binary-key behaves the same either way. See the context-property documentation. I kept the uniform output mapping without adding a profile-specific branch; the action explicitly requests the CI profile. Leaving this unchanged.

[andygrove]

Thanks for the revision. The rebase onto #5973 is in and all 38 checks are green, so the sequencing prerequisite is satisfied. I re-queried the cache API and it confirms #5973 worked: 8 entries, 8.58 GB, everything on refs/heads/main except two TPC dataset entries on a PR merge ref. That is a completely different picture from the 17.34 GB with nothing on main that I reported on Monday.

Before the concerns, the things I checked rather than took on trust, since a couple of them were my own earlier worries. The RUSTFLAGS pin genuinely works: JobExtension.cs applies workflow- and job-level env: once into Global.EnvironmentVariables at job init, the GITHUB_ENV file command mutates that same dictionary, and StepsRunner rebuilds each step's env from it, so the composite's write beats the callers' workflow-level value and the fingerprint step reads exactly what the build gets. check_cache_save_scope does scan .github/actions/*/action.yaml, so #5973's guard covers both new save steps. All four callers have byte-identical env: blocks and every one passes java: 17, so nothing fragments the key across workflows today. And there are no submodules, no include_str! or include_bytes! under native/, no git-SHA stamping, and the three tracked symlinks fall outside the matched patterns. The helper resolves 307 files for the CI key and 405 for debug, the extra 98 being exactly the benches. Six tests and check-ci-config.py pass locally.

Two things I would like to work through, and they are connected.

The 23-minute figure is pre-#5973

I measured the warm case. PR #5991 started at 22:05Z on the 16th and this PR at 22:12Z, seven minutes apart against the same cache state. #5991 changed three native Rust files and its Build Native Library took 4m39s. This PR's took 29m44s, because -v3- is a fresh namespace with nothing to restore. On the nightly against main the next morning, PR Build (Linux) / Build Native Library was 4.2 minutes and the three Iceberg builders 7.6 to 7.8.

So the step this PR removes now costs about 4.2 minutes, not 23. Worth noting too that binary-key and source-key are both functions of the same (environment, sources) pair, so the binary cache hits in exactly the cases the cargo cache would already have exact-hit. What it really buys is skipping a 1.4 GB download and a no-op Cargo revalidation in exchange for ~30 MB, plus a small entry surviving eviction better than a 2 GB one. That is a genuine 2 to 3 minutes per native-building job and I think it is still worth having, but could the description be re-based on the current numbers? Right now it leads with a saving its own prerequisite already delivered, and the CI-status paragraph still says hosted CI for the rebased head is pending.

Could the registry and git paths come out rather than be repointed?

Linux-cargo-debug-* is 4131 MB today and Linux-cargo-ci-* is 1439 MB, and both hold native/target alone, since ~/.cargo does not exist in amd64/rust. Those are the same entries producing the 4.2-minute builds above, which I think is the evidence that the registry is not needed for the target cache to work: cargo re-fetches it from crates.io in well under a minute, and registry deps are fingerprinted by package id rather than source mtime, so re-extracting them does not force a recompile. Pointing at /usr/local/cargo adds the registry and the iceberg-rust checkout to both entries, on the budget #5973 just recovered. Deleting the two dead path lines fixes the same wart at no cost.

That also softens the transition. Linux-cargo-ci- to -v3- and Linux-cargo-debug- to -v3- orphan 5.5 GB that stays resident for its 7-day window while the new entries are written alongside, and we both know what happens to main's entries when this repo goes over. Without the registry the new entries are the same size as the old, so the peak is around 11 GB instead of 15.

Either way, could we agree what happens to the superseded entries at merge? Deleting them through the cache API keeps the budget flat but sends every unrebased PR cold; leaving them doubles the cargo footprint for a week. I lean towards deleting, given how the eviction went last time, but it is worth being deliberate about rather than discovering it.

One last question on shape

#5841 is still open and shares one native build across workflows within a run, which would remove the duplicate Iceberg and Spark SQL native builds. That is where most of the remaining per-run saving lives now that the warm build is 4.2 minutes. How do you see these two composing, and does the order they land in matter?

---

For the record, two things from my earlier passes that I am dropping. Main's warmer runs on nearly every push, since build_linux matches spark/** and common/** and not just the native inputs, so an environment drift that invalidates the whole key self-heals within hours rather than waiting for a native change. And the workflow-level RUSTFLAGS in the four callers is now shadowed from the composite onwards, which is harmless given the precedence result above.

[sunchao]

Following up on your latest review.

Thanks, Andy. I addressed the two connected concerns in 80296cb7b and revised the description around the post-#5973 baseline.

Both CI and debug caches now contain only native/target; I removed the registry/git paths and the unused cargo-home output. The effective Cargo home still participates in the environment fingerprint. I checked the newer #5991 native job: it restored the target archive, fetched registry/git sources, and rebuilt only six Comet workspace crates. A separate disposable Cargo build with registry and Git dependencies also kept all compiled dependencies fresh after their source directories were deleted and fetched again.

The description now uses that job's 3m51s and the nightly Linux job's 4m13s, including 43–47s restoring the target cache and 1m57s–2m22s in Cargo. Both were fallback restores with workspace recompilation, so I explicitly distinguish them from the exact-library-hit path. The historical 23-minute cold build is no longer presented as the expected saving. Actual library-hit and end-to-end savings remain to be measured after main publishes an entry.

For migration, I agree with deliberate deletion: the description calls for inventorying and deleting the superseded legacy Cargo cache IDs on refs/heads/main at merge time, preserving the new namespaces and unrelated caches. The first main build will populate the new caches from cold. This avoids keeping both large generations resident, with the explicit tradeoff that PRs using old keys lose their warm cache and need to rebase. No live caches were deleted as part of this update.

For #5841, this PR reuses a library across runs; #5841 shares the producer within a run. Together, one producer restores or builds and then distributes the library. There is no functional ordering requirement, but the planned integration is to land this PR first, then rebase #5841 and use this composite action in its shared producer, preserving main-only writes. Ordinary unlabeled PRs already have one covered producer, so #5841 mainly removes duplicates in queue/nightly/manual runs.

The six fingerprint tests, action-flow tests, CI configuration checks, actionlint, Markdown formatting and whitespace checks pass locally. Hosted CI for this new head is pending; the description keeps that separate from the earlier green head and the still-unmeasured library-hit path.