fix(ci): fix stale artifact caching in prebuild step (#1838)

# What does this PR do?

`inner_build_artifact` had a simple `if artifact_path.exists() { return }` check that skipped cargo build entirely when the binary file was on disk, even if source code had changed. This caused bin_test tests to run against stale binaries after code edits.

**Changes**
`rebuild_artifacts` (used by the prebuild step): Always invokes cargo build, letting cargo's own dependency tracking handle staleness. 
`fetch_built_artifacts` (used by tests): Only looks up the prebuilt artifact path. If it doesn't exist, the test fails with a message telling the developer to add the artifact to `all_prebuild_artifacts()`. No silent fallback building that would hide missing prebuild entries and slow down CI.

# Motivation

Noticed bin_tests running against stale binaries here: https://gitlab.ddbuild.io/DataDog/apm-reliability/libddprof-build/-/jobs/1559430494

[[PROF-14184]
](https://datadoghq.atlassian.net/jira/software/c/projects/PROF/boards/11?selectedIssue=PROF-14184)
# Additional Notes

Anything else we should know when reviewing?

# How to test the change?

Run `cargo nextest run` locally with clean artifacts directory, then making changes to observe rebuild of stale artifacts, then again with no changes to observe cargo handling of non-stale files

[in progress]: doing the same combination of runs as the above local verification but in CI


Co-authored-by: gyuheon.oh <gyuheon.oh@datadoghq.com>

Author: gyuheon0h

bin_tests/src/bin/prebuild.rs

@@ -9,7 +9,7 @@
 //!
 //! Run with: cargo run -p bin_tests --bin prebuild
 
-use bin_tests::{artifacts, build_artifacts};
+use bin_tests::{artifacts, rebuild_artifacts};
 
 fn main() -> anyhow::Result<()> {
     println!("Pre-building bin_tests artifacts...");
@@ -20,7 +20,7 @@ fn main() -> anyhow::Result<()> {
 
     // Build all artifacts
     let start = std::time::Instant::now();
-    build_artifacts(&artifact_refs)?;
+    rebuild_artifacts(&artifact_refs)?;
     let elapsed = start.elapsed();
 
     println!(

bin_tests/src/lib.rs

@@ -79,12 +79,10 @@ pub fn compute_artifact_path(c: &ArtifactsBuild) -> anyhow::Result<PathBuf> {
 /// This crate implements an abstraction over compilation with cargo with the purpose
 /// of testing full binaries or dynamic libraries, instead of just rust static libraries.
 ///
-/// The main entrypoint is `fn build_artifacts` which takes a list of artifacts to build,
-/// either executable crates, cdylib, or extra binaries, invokes cargo and return the path
-/// of the built artifact.
-///
-/// Builds are cached between invocations so that multiple tests can use the same artifact
-/// without doing expensive work twice.
+/// Artifacts are built upfront by the prebuild step via [`rebuild_artifacts`], which
+/// invokes `cargo build` and lets cargo handle staleness. Tests then use
+/// [`fetch_built_artifacts`] to look up the prebuilt paths, failing if any are missing
+/// (indicating the prebuild script needs to be updated).
 ///
 /// It is assumed that functions in this crate are invoked in the context of a cargo #[test]
 /// item, or a `cargo run` command to be able to locate artifacts built by cargo from the position
@@ -115,15 +113,9 @@ pub struct ArtifactsBuild {
     pub panic_abort: Option<bool>,
 }
 
-fn inner_build_artifact(c: &ArtifactsBuild) -> anyhow::Result<PathBuf> {
-    // Compute the expected artifact path first
+fn cargo_build_artifact(c: &ArtifactsBuild) -> anyhow::Result<PathBuf> {
     let artifact_path = compute_artifact_path(c)?;
 
-    // If the artifact already exists, skip building
-    if artifact_path.exists() {
-        return Ok(artifact_path);
-    }
-
     let mut build_cmd = process::Command::new(env!("CARGO"));
     build_cmd.arg("build");
 
@@ -167,29 +159,49 @@ fn inner_build_artifact(c: &ArtifactsBuild) -> anyhow::Result<PathBuf> {
     Ok(artifact_path)
 }
 
-/// Caches and returns the path of the artifacts built by cargo
-/// This function should only be called from cargo tests
-pub fn build_artifacts<'b>(
+/// Returns the paths of prebuilt artifacts, failing if any are missing.
+/// Tests should use this: if an artifact doesn't exist, it means the prebuild
+/// script needs to be updated rather than silently building inline (which would
+/// slow down CI).
+pub fn fetch_built_artifacts<'b>(
+    crates: &[&'b ArtifactsBuild],
+) -> anyhow::Result<HashMap<&'b ArtifactsBuild, PathBuf>> {
+    let mut res = HashMap::new();
+    for &c in crates {
+        let artifact_path = compute_artifact_path(c)?;
+        anyhow::ensure!(
+            artifact_path.exists(),
+            "Prebuilt artifact not found at {path}. \
+             Add this artifact to `artifacts::all_prebuild_artifacts()` \
+             and re-run the prebuild step.",
+            path = artifact_path.display(),
+        );
+        res.insert(c, artifact_path);
+    }
+    Ok(res)
+}
+
+/// Invokes `cargo build` for each artifact, letting cargo's own dependency
+/// tracking decide whether recompilation is needed.
+pub fn rebuild_artifacts<'b>(
     crates: &[&'b ArtifactsBuild],
 ) -> anyhow::Result<HashMap<&'b ArtifactsBuild, PathBuf>> {
     static ARTIFACTS: OnceCell<Mutex<HashMap<ArtifactsBuild, PathBuf>>> = OnceCell::new();
 
     let mut res = HashMap::new();
-
     let artifacts = ARTIFACTS.get_or_init(|| Mutex::new(HashMap::new()));
     for &c in crates {
         let mut artifacts = artifacts.lock().unwrap();
         let artifacts = artifacts.deref_mut();
 
-        if artifacts.contains_key(c) {
-            res.insert(c, artifacts.get(c).unwrap().clone());
+        if let Some(p) = artifacts.get(c) {
+            res.insert(c, p.clone());
         } else {
-            let p = inner_build_artifact(c)?;
+            let p = cargo_build_artifact(c)?;
             res.insert(c, p.clone());
             artifacts.insert(c.clone(), p);
         }
     }
-
     Ok(res)
 }
 

bin_tests/src/test_runner.rs

@@ -7,7 +7,7 @@
 
 use crate::{
     artifacts::StandardArtifacts,
-    build_artifacts,
+    fetch_built_artifacts,
     test_types::{CrashType, TestMode},
     validation::{read_and_parse_crash_payload, validate_std_outputs, PayloadValidator},
     ArtifactsBuild, BuildProfile,
@@ -170,7 +170,7 @@ where
 /// # Example
 /// ```no_run
 /// use bin_tests::test_runner::run_custom_crash_test;
-/// use bin_tests::{build_artifacts, ArtifactType, ArtifactsBuild, BuildProfile};
+/// use bin_tests::{fetch_built_artifacts, ArtifactType, ArtifactsBuild, BuildProfile};
 ///
 /// # fn main() -> anyhow::Result<()> {
 /// let receiver = ArtifactsBuild {
@@ -189,7 +189,7 @@ where
 ///     ..Default::default()
 /// };
 ///
-/// let artifacts_map = build_artifacts(&[&receiver, &crashing_app])?;
+/// let artifacts_map = fetch_built_artifacts(&[&receiver, &crashing_app])?;
 ///
 /// run_custom_crash_test(
 ///     &artifacts_map[&crashing_app],
@@ -247,7 +247,7 @@ where
 /// (preload allocation detector). It just runs the binary and waits.
 pub fn run_crash_no_op(config: &CrashTestConfig) -> Result<()> {
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice())?;
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice())?;
     let fixtures = TestFixtures::new()?;
 
     let mut cmd = process::Command::new(&artifacts_map[&artifacts.crashtracker_bin]);

bin_tests/tests/crashtracker_bin_test.rs

@@ -14,7 +14,7 @@ use anyhow::Context;
 use bin_tests::test_runner::run_crash_no_op;
 use bin_tests::{
     artifacts::{self, StandardArtifacts},
-    build_artifacts,
+    fetch_built_artifacts,
     test_runner::{run_crash_test_with_artifacts, CrashTestConfig, ValidatorFn},
     test_types::{CrashType, TestMode},
     validation::PayloadValidator,
@@ -70,7 +70,7 @@ fn run_standard_crash_test_refactored(
 ) {
     let config = CrashTestConfig::new(profile, mode, crash_type);
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let crash_type_str = crash_type.as_str();
     let validator: ValidatorFn = Box::new(move |payload, fixtures| {
@@ -107,7 +107,7 @@ fn test_crash_tracking_bin_errno_preservation() {
         CrashType::NullDeref,
     );
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|_payload, fixtures| {
         let status_path = fixtures.output_dir.join(ERRNO_STATUS_FILENAME);
@@ -132,7 +132,7 @@ fn test_crash_tracking_bin_unhandled_exception() {
         CrashType::UnhandledException,
     );
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|payload, fixtures| {
         PayloadValidator::new(payload)
@@ -168,7 +168,7 @@ fn test_crash_tracking_bin_runtime_callback_frame() {
         CrashType::NullDeref,
     );
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|payload, fixtures| {
         PayloadValidator::new(payload).validate_counters()?;
@@ -198,7 +198,7 @@ fn test_crash_tracking_thread_name() {
         CrashType::NullDeref,
     );
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|payload, _fixtures| {
         let error = &payload["error"];
@@ -231,7 +231,7 @@ fn test_crash_tracking_bin_runtime_callback_string() {
         CrashType::NullDeref,
     );
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|payload, fixtures| {
         PayloadValidator::new(payload).validate_counters()?;
@@ -260,7 +260,7 @@ fn test_crash_tracking_bin_no_runtime_callback() {
         CrashType::NullDeref,
     );
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|payload, fixtures| {
         PayloadValidator::new(payload).validate_counters()?;
@@ -338,7 +338,7 @@ fn test_crash_tracking_bin_runtime_callback_frame_invalid_utf8() {
         CrashType::NullDeref,
     );
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|payload, fixtures| {
         PayloadValidator::new(payload).validate_counters()?;
@@ -380,7 +380,7 @@ fn test_crash_tracking_errors_intake_upload() {
     .with_env("DD_CRASHTRACKING_ERRORS_INTAKE_ENABLED", "true");
 
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|_payload, fixtures| {
         let errors_intake_path = fixtures.crash_profile_path.with_extension("errors");
@@ -413,7 +413,7 @@ fn test_crash_tracking_errors_intake_crash_ping() {
     .with_env("DD_CRASHTRACKING_ERRORS_INTAKE_ENABLED", "true");
 
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|_payload, fixtures| {
         let errors_intake_path = fixtures.crash_profile_path.with_extension("errors");
@@ -444,7 +444,7 @@ fn test_crash_tracking_errors_intake_crash_info_parity() {
     .with_env("DD_CRASHTRACKING_ERRORS_INTAKE_ENABLED", "true");
 
     let artifacts = StandardArtifacts::new(config.profile);
-    let artifacts_map = build_artifacts(&artifacts.as_slice()).unwrap();
+    let artifacts_map = fetch_built_artifacts(&artifacts.as_slice()).unwrap();
 
     let validator: ValidatorFn = Box::new(|crash_info_payload, fixtures| {
         let errors_intake_path = fixtures.crash_profile_path.with_extension("errors");
@@ -638,7 +638,7 @@ fn test_crash_tracking_app(crash_type: &str) {
     let crashtracker_receiver = artifacts::crashtracker_receiver(BuildProfile::Release);
     let crashing_app = artifacts::crashing_app(BuildProfile::Debug, true);
 
-    let artifacts_map = build_artifacts(&[&crashtracker_receiver, &crashing_app]).unwrap();
+    let artifacts_map = fetch_built_artifacts(&[&crashtracker_receiver, &crashing_app]).unwrap();
 
     // Create validator based on crash type
     let crash_type_owned = crash_type.to_owned();
@@ -712,7 +712,8 @@ fn test_panic_hook_mode(mode: &str, expected_category: &str, expected_panic_mess
     let crashtracker_receiver = artifacts::crashtracker_receiver(BuildProfile::Release);
     let crashtracker_bin_test = artifacts::crashtracker_bin_test(BuildProfile::Debug, true);
 
-    let artifacts_map = build_artifacts(&[&crashtracker_receiver, &crashtracker_bin_test]).unwrap();
+    let artifacts_map =
+        fetch_built_artifacts(&[&crashtracker_receiver, &crashtracker_bin_test]).unwrap();
 
     let expected_category = expected_category.to_owned();
     let expected_panic_message = expected_panic_message.map(|s| s.to_owned());
@@ -785,7 +786,7 @@ fn test_crash_tracking_callstack() {
     // compile in debug so we avoid inlining and can check the callchain
     let crashing_app = artifacts::crashing_app(BuildProfile::Debug, false);
 
-    let artifacts_map = build_artifacts(&[&crashtracker_receiver, &crashing_app]).unwrap();
+    let artifacts_map = fetch_built_artifacts(&[&crashtracker_receiver, &crashing_app]).unwrap();
 
     // Note: in Release, we do not have the crate and module name prepended to the function name
     // Here we compile the crashing app in Debug.
@@ -1532,7 +1533,7 @@ fn test_receiver_emits_debug_logs_on_receiver_issue() -> anyhow::Result<()> {
     use std::time::Duration;
 
     let receiver = artifacts::crashtracker_receiver(BuildProfile::Debug);
-    let artifacts = build_artifacts(&[&receiver])?;
+    let artifacts = fetch_built_artifacts(&[&receiver])?;
     let fixtures = bin_tests::test_runner::TestFixtures::new()?;
 
     let missing_file = fixtures.output_dir.join("missing_additional_file.txt");
@@ -1808,7 +1809,7 @@ struct TestFixtures<'a> {
 }
 
 fn setup_test_fixtures<'a>(crates: &[&'a ArtifactsBuild]) -> TestFixtures<'a> {
-    let artifacts = build_artifacts(crates).unwrap();
+    let artifacts = fetch_built_artifacts(crates).unwrap();
 
     let tmpdir = tempfile::TempDir::new().unwrap();
     let dirpath = tmpdir.path();

bin_tests/tests/test_the_tests.rs

@@ -5,7 +5,7 @@
 
 use std::{fs, process};
 
-use bin_tests::{build_artifacts, ArtifactType, ArtifactsBuild, BuildProfile};
+use bin_tests::{fetch_built_artifacts, ArtifactType, ArtifactsBuild, BuildProfile};
 
 #[test]
 #[cfg_attr(miri, ignore)]
@@ -38,7 +38,7 @@ fn test_the_tests_inner(profile: BuildProfile) {
         },
         &test_the_tests,
     ];
-    let artifacts = build_artifacts(crates).unwrap();
+    let artifacts = fetch_built_artifacts(crates).unwrap();
 
     for c in crates {
         assert!(fs::metadata(&artifacts[c]).unwrap().file_type().is_file());