diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f90466bed2..9934375030 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,17 +1,56 @@ repos: -- repo: local + - repo: local hooks: - - id: rust-linting - name: Rust linting - description: Run cargo fmt on files included in the commit. + # Format changed Rust files. Kept on nightly because the tree is + # nightly-formatted; stable fmt would reflow files needlessly. + - id: rust-fmt + name: cargo fmt + description: Format Rust sources included in the commit. entry: cargo +nightly fmt -- pass_filenames: true types: [file, rust] language: system - - id: rust-clippy - name: Rust clippy - description: Run cargo clippy on files included in the commit. - entry: cargo +nightly clippy --workspace --all-targets --all-features -- + + # Lint with the gate CI enforces (.github/workflows/ci.yml): -D warnings + # plus the three allows CI sets. --features all,dist-server so the + # feature-gated dist-client / dist-server code is actually linted - CI's + # clippy runs on default features and never covers it. + - id: rust-clippy + name: cargo clippy + description: Lint all targets with warnings denied. + entry: cargo clippy --locked --all-targets --features all,dist-server -- -D warnings -A unknown-lints -A clippy::type_complexity -A clippy::new-without-default pass_filenames: false types: [file, rust] language: system + + # Compile every target including the integration-test harness. This is the + # gate that catches a changed function signature or struct literal under + # tests/ that plain `cargo check` and `makepkg` never compile. + - id: rust-check + name: cargo check --all-targets + description: Type-check every target, including tests and benches. + entry: cargo check --all-targets --features all,dist-server + pass_filenames: false + types: [file, rust] + language: system + + # Run the CI test set on push (lib, bins, integration tests). On push, not + # per commit, so commit turnaround stays fast. + - id: rust-test + name: cargo test + description: Run the library, binary, and integration tests. + entry: cargo test --locked --features all,dist-server --lib --bins --tests + pass_filenames: false + types: [file, rust] + language: system + stages: [pre-push] + + # RustSec advisory scan on push (org security standard). + - id: cargo-audit + name: cargo audit + description: Scan dependencies for known advisories. + entry: cargo audit + pass_filenames: false + types: [file, rust] + language: system + stages: [pre-push] diff --git a/Cargo.lock b/Cargo.lock index 9263cd2c55..3be7bc16da 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2889,6 +2889,7 @@ version = "0.16.0" dependencies = [ "anyhow", "ar", + "arc-swap", "assert_cmd", "async-trait", "backon", diff --git a/Cargo.toml b/Cargo.toml index e35ea56ae3..6649dac77a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,6 +41,7 @@ strip = true [dependencies] anyhow = { version = "1.0", features = ["backtrace"] } ar = "0.9" +arc-swap = "1.7.1" async-trait = "0.1" backon = { version = "1", default-features = false, features = [ "std-blocking-sleep", diff --git a/src/bin/sccache-dist/build.rs b/src/bin/sccache-dist/build.rs index bfd7a980e5..1a2ff1e15a 100644 --- a/src/bin/sccache-dist/build.rs +++ b/src/bin/sccache-dist/build.rs @@ -26,7 +26,7 @@ use std::io; use std::iter; use std::path::{self, Path, PathBuf}; use std::process::{ChildStdin, Command, Output, Stdio}; -use std::sync::Mutex; +use std::sync::{Arc, Mutex}; use std::time::Instant; use version_compare::Version; @@ -104,6 +104,28 @@ pub struct OverlayBuilder { bubblewrap: PathBuf, dir: PathBuf, toolchain_dir_map: Mutex>, + // Per-toolchain preparation locks. The gzip+untar of an uncached toolchain + // runs under the matching entry lock here, NOT under toolchain_dir_map, so + // two requests for different toolchains prepare in parallel while two for + // the same uncached toolchain serialize (no half-written dir, no double + // unpack). + toolchain_prepare_locks: Mutex>>>, +} + +// Return the preparation lock for `tc`, creating it on first use. Held only +// long enough to look up or insert the entry, so distinct toolchains never +// contend here. Same key yields the same Arc; distinct keys yield distinct +// Arcs. +fn get_prepare_lock( + locks: &Mutex>>>, + tc: &Toolchain, +) -> Arc> { + locks + .lock() + .unwrap() + .entry(tc.clone()) + .or_insert_with(|| Arc::new(Mutex::new(()))) + .clone() } impl OverlayBuilder { @@ -151,6 +173,7 @@ impl OverlayBuilder { bubblewrap, dir, toolchain_dir_map: Mutex::new(HashMap::new()), + toolchain_prepare_locks: Mutex::new(HashMap::new()), }; ret.cleanup()?; fs::create_dir(&ret.dir).context("Failed to create base directory for builder")?; @@ -163,7 +186,7 @@ impl OverlayBuilder { fn cleanup(&self) -> Result<()> { if self.dir.exists() { - fs::remove_dir_all(&self.dir).context("Failed to clean up builder directory")? + fs::remove_dir_all(&self.dir).context("Failed to clean up builder directory")?; } Ok(()) } @@ -173,75 +196,174 @@ impl OverlayBuilder { tc: &Toolchain, tccache: &Mutex, ) -> Result { - let DeflatedToolchain { - path: toolchain_dir, - build_count: id, - ctime: _, - } = { - let mut toolchain_dir_map = self.toolchain_dir_map.lock().unwrap(); - // Create the toolchain dir (if necessary) while we have an exclusive lock - let toolchain_dir = self.dir.join("toolchains").join(&tc.archive_id); - if toolchain_dir_map.contains_key(tc) && toolchain_dir.exists() { - // TODO: use if let when sccache can use NLL - let entry = toolchain_dir_map - .get_mut(tc) - .expect("Key missing after checking"); - entry.build_count += 1; - entry.clone() - } else { - trace!("Creating toolchain directory for {}", tc.archive_id); - fs::create_dir(&toolchain_dir)?; + let toolchain_dir = self.dir.join("toolchains").join(&tc.archive_id); + // Fast path: an already-prepared toolchain just bumps its build + // counter. The global map lock is held only for this lookup. + if let Some(entry) = self.bump_prepared_toolchain(tc, &toolchain_dir) { + return self.make_overlay_spec(tc, entry); + } + + // Serialize preparation on the per-toolchain lock, NOT the global map + // lock, so requests for different toolchains prepare in parallel while + // requests for the same uncached toolchain wait here. + let prepare_lock = get_prepare_lock(&self.toolchain_prepare_locks, tc); + let _prepare_guard = prepare_lock.lock().unwrap(); + + // Another request may have prepared this toolchain while we waited on + // the prepare lock; re-check before unpacking. + if let Some(entry) = self.bump_prepared_toolchain(tc, &toolchain_dir) { + return self.make_overlay_spec(tc, entry); + } + + trace!("Creating toolchain directory for {}", tc.archive_id); + // We hold this toolchain's prepare lock and it is absent from the map, so + // any directory on disk is a stale leftover (an interrupted preparation, + // or an eviction that pruned the map entry before deleting the dir). Clear + // it so create_dir does not fail with AlreadyExists. + if toolchain_dir.exists() { + fs::remove_dir_all(&toolchain_dir) + .context("Failed to remove stale toolchain directory")?; + } + fs::create_dir(&toolchain_dir)?; + + { + // Take an owned handle to the cached archive under the lock, then + // release the tccache mutex before untarring. The archive reader used + // to borrow the cache guard, which serialized every toolchain's untar + // on this one mutex; an owned File lets different toolchains untar in + // parallel. + let toolchain_file = { let mut tccache = tccache.lock().unwrap(); - let toolchain_rdr = match tccache.get(tc) { - Ok(rdr) => rdr, + match tccache.get_file(tc) { + Ok(file) => file, Err(LruError::FileNotInCache) => { bail!("expected toolchain {}, but not available", tc.archive_id) } Err(e) => { return Err(Error::from(e).context("failed to get toolchain from cache")); } - }; + } + }; - tar::Archive::new(GzDecoder::new(toolchain_rdr)) - .unpack(&toolchain_dir) - .or_else(|e| { - warn!("Failed to unpack toolchain: {:?}", e); - fs::remove_dir_all(&toolchain_dir) - .context("Failed to remove unpacked toolchain")?; - tccache - .remove(tc) - .context("Failed to remove corrupt toolchain")?; - Err(Error::from(e)) - })?; - - let entry = DeflatedToolchain { - path: toolchain_dir, - build_count: 1, - ctime: Instant::now(), - }; + tar::Archive::new(GzDecoder::new(toolchain_file)) + .unpack(&toolchain_dir) + .or_else(|e| { + warn!("Failed to unpack toolchain: {:?}", e); + fs::remove_dir_all(&toolchain_dir) + .context("Failed to remove unpacked toolchain")?; + // Removing the corrupt archive needs the cache lock again; + // the untar handle is dropped by now, so re-acquire briefly. + tccache + .lock() + .unwrap() + .remove(tc) + .context("Failed to remove corrupt toolchain")?; + Err(Error::from(e)) + })?; + } + + let entry = DeflatedToolchain { + path: toolchain_dir, + build_count: 1, + ctime: Instant::now(), + }; - toolchain_dir_map.insert(tc.clone(), entry.clone()); - if toolchain_dir_map.len() > tccache.len() { - let dir_map = toolchain_dir_map.clone(); - let mut entries: Vec<_> = dir_map.iter().collect(); - // In the pathological case, creation time for unpacked - // toolchains could be the opposite of the least recently - // recently used, so we clear out half of the accumulated - // toolchains to prevent repeated sort/delete cycles. - entries.sort_by(|a, b| (a.1).ctime.cmp(&(b.1).ctime)); - entries.truncate(entries.len() / 2); - for (tc, _) in entries { - warn!("Removing old un-compressed toolchain: {:?}", tc); - assert!(toolchain_dir_map.remove(tc).is_some()); - fs::remove_dir_all(self.dir.join("toolchains").join(&tc.archive_id)) - .context("Failed to remove old toolchain directory")?; + // Insert the new entry and pick eviction victims under the global map + // lock, but run the remove_dir_all for each victim AFTER dropping it so + // filesystem teardown never serializes concurrent preparations. + let mut evictions: Vec = Vec::new(); + { + let mut toolchain_dir_map = self.toolchain_dir_map.lock().unwrap(); + toolchain_dir_map.insert(tc.clone(), entry.clone()); + if toolchain_dir_map.len() > tccache.lock().unwrap().len() { + let dir_map = toolchain_dir_map.clone(); + let mut entries: Vec<_> = dir_map.iter().collect(); + // In the pathological case, creation time for unpacked + // toolchains could be the opposite of the least recently + // recently used, so we clear out half of the accumulated + // toolchains to prevent repeated sort/delete cycles. + entries.sort_by_key(|a| (a.1).ctime); + entries.truncate(entries.len() / 2); + for (victim, _) in entries { + if toolchain_dir_map.remove(victim).is_some() { + evictions.push(victim.clone()); + } else { + warn!( + "toolchain {:?} already gone from dir map during eviction", + victim + ); } } - entry } - }; + } + for victim in evictions { + // Delete each victim under its own preparation lock so a concurrent + // request that legitimately re-prepares the same toolchain cannot + // create_dir+untar into the path we are deleting. Use try_lock, never + // a blocking acquire: we already hold this request's prepare lock, and + // blocking on a second prepare lock could deadlock against a racer that + // picked us as its own eviction victim. A held lock means a racer is + // actively preparing the victim, so skip its deletion. + let victim_lock = get_prepare_lock(&self.toolchain_prepare_locks, &victim); + let _victim_guard = match victim_lock.try_lock() { + Ok(guard) => guard, + Err(_) => { + trace!("Skipping eviction of {:?}: being prepared", victim); + continue; + } + }; + + // A racer may have finished re-preparing the victim while we waited on + // its lock. Re-verify it is still absent from the map before deleting, + // holding the map lock only for the check and never across the + // remove_dir_all below (keeps map and prepare locks disjoint). + { + let toolchain_dir_map = self.toolchain_dir_map.lock().unwrap(); + if toolchain_dir_map.contains_key(&victim) { + trace!("Skipping eviction of {:?}: re-prepared", victim); + continue; + } + } + warn!("Removing old un-compressed toolchain: {:?}", victim); + fs::remove_dir_all(self.dir.join("toolchains").join(&victim.archive_id)) + .context("Failed to remove old toolchain directory")?; + + // Prune the now-unused preparation lock entry so the map does not grow + // without bound as toolchains churn. + self.toolchain_prepare_locks.lock().unwrap().remove(&victim); + } + + self.make_overlay_spec(tc, entry) + } + + // Bump and return a clone of the prepared-toolchain entry when it is both + // recorded in the map and present on disk, else None. Holds the global map + // lock only for the lookup and counter bump. + fn bump_prepared_toolchain( + &self, + tc: &Toolchain, + toolchain_dir: &Path, + ) -> Option { + let mut toolchain_dir_map = self.toolchain_dir_map.lock().unwrap(); + if toolchain_dir_map.contains_key(tc) && toolchain_dir.exists() { + let entry = toolchain_dir_map + .get_mut(tc) + .expect("Key missing after checking"); + entry.build_count += 1; + Some(entry.clone()) + } else { + None + } + } + + fn make_overlay_spec(&self, tc: &Toolchain, entry: DeflatedToolchain) -> Result { + let DeflatedToolchain { + path: toolchain_dir, + build_count: id, + ctime: _, + } = entry; trace!("Creating build directory for {}-{}", tc.archive_id, id); let build_dir = self .dir @@ -306,7 +428,7 @@ impl OverlayBuilder { // This error is unfortunately not Send+Sync ) .mount() - .map_err(|e| anyhow!("Failed to mount overlay FS: {}", e.to_string()))?; + .map_err(|e| anyhow!("Failed to mount overlay FS: {}", e))?; trace!("copying in inputs"); // Note that we don't unpack directly into the upperdir since there overlayfs has some @@ -393,11 +515,11 @@ impl OverlayBuilder { Ok(file) => { let output = OutputData::try_from_reader(file) .context("Failed to read output file")?; - outputs.push((path, output)) + outputs.push((path, output)); } Err(e) => { if e.kind() == io::ErrorKind::NotFound { - debug!("Missing output path {:?}", path) + debug!("Missing output path {:?}", path); } else { return Err( Error::from(e).context("Failed to open output file") @@ -525,7 +647,7 @@ impl DockerBuilder { bail!("Malformed container listing - third field on row") } if image_name.starts_with("sccache-builder-") { - containers_to_rm.push(container_id) + containers_to_rm.push(container_id); } } if !containers_to_rm.is_empty() { @@ -555,7 +677,7 @@ impl DockerBuilder { bail!("Malformed image listing - third field on row") } if image_name.starts_with("sccache-builder-") { - images_to_rm.push(image_id) + images_to_rm.push(image_id); } } if !images_to_rm.is_empty() { @@ -563,7 +685,7 @@ impl DockerBuilder { .args(["rmi"]) .args(images_to_rm) .check_run() - .context("Failed to remove image")? + .context("Failed to remove image")?; } } @@ -647,7 +769,7 @@ impl DockerBuilder { .check_run() { // We do a final check anyway, so just continue - warn!("Failed to remove added path in a container: {}", e) + warn!("Failed to remove added path in a container: {}", e); } } @@ -683,7 +805,7 @@ impl DockerBuilder { // Good as new, add it back to the container list if let Some(entry) = self.container_lists.lock().unwrap().get_mut(tc) { debug!("Reclaimed container {}", cid); - entry.push(cid) + entry.push(cid); } else { warn!( "Was ready to reclaim container {} but toolchain went missing", @@ -835,9 +957,9 @@ impl DockerBuilder { if output.status.success() { let output = OutputData::try_from_reader(&*output.stdout) .expect("Failed to read compress output stdout"); - outputs.push((path, output)) + outputs.push((path, output)); } else { - debug!("Missing output path {:?}", path) + debug!("Missing output path {:?}", path); } } @@ -873,3 +995,79 @@ impl BuilderIncoming for DockerBuilder { Ok(res) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn tc(id: &str) -> Toolchain { + Toolchain { + archive_id: id.to_owned(), + } + } + + // Two requests for the same uncached toolchain must serialize on one + // preparation lock (so the gzip+untar happens once, with no half-written + // dir), while requests for different toolchains get independent locks and + // never block each other. Constructing an OverlayBuilder requires root, so + // drive the lock-acquisition invariant that prepare_overlay_dirs relies on + // directly. + #[test] + fn test_prepare_lock_shared_per_key_and_distinct_across_keys() { + let locks = Mutex::new(HashMap::new()); + + let a1 = get_prepare_lock(&locks, &tc("aaaa")); + let a2 = get_prepare_lock(&locks, &tc("aaaa")); + let b = get_prepare_lock(&locks, &tc("bbbb")); + + assert!( + Arc::ptr_eq(&a1, &a2), + "same toolchain key must share one preparation lock" + ); + assert!( + !Arc::ptr_eq(&a1, &b), + "different toolchain keys must get distinct preparation locks" + ); + + // The shared lock actually serializes: while a1 is held, a second + // acquire of the same Arc blocks, but a different toolchain's lock is + // free to take. + let guard = a1.lock().unwrap(); + assert!( + a2.try_lock().is_err(), + "second acquire of the shared preparation lock must block" + ); + assert!( + b.try_lock().is_ok(), + "a different toolchain's preparation lock must not block" + ); + drop(guard); + } + + // The eviction path in prepare_overlay_dirs deletes a victim only after a + // try_lock on the victim's preparation lock succeeds. Constructing an + // OverlayBuilder requires root, so drive the try_lock invariant the eviction + // loop relies on directly: while a victim is being prepared (its lock held), + // try_lock must fail so the victim is skipped; once released, try_lock must + // succeed so the stale directory can be removed. + #[test] + fn test_eviction_skips_victim_while_being_prepared() { + let locks = Mutex::new(HashMap::new()); + let victim = tc("cccc"); + + let prepare_lock = get_prepare_lock(&locks, &victim); + let held = prepare_lock.lock().unwrap(); + + let eviction_view = get_prepare_lock(&locks, &victim); + assert!( + eviction_view.try_lock().is_err(), + "a victim under active preparation must not be evicted" + ); + + drop(held); + assert!( + eviction_view.try_lock().is_ok(), + "once preparation finishes the victim is free to evict" + ); + } +} diff --git a/src/bin/sccache-dist/cmdline/mod.rs b/src/bin/sccache-dist/cmdline/mod.rs index fafe3902c1..82ccdbb5e4 100644 --- a/src/bin/sccache-dist/cmdline/mod.rs +++ b/src/bin/sccache-dist/cmdline/mod.rs @@ -19,6 +19,10 @@ mod parse; pub use parse::try_parse_from; +// The Scheduler/Server variants carry the full parsed daemon config; this enum +// is constructed once at startup and matched once, so the size difference is +// irrelevant and boxing the variants would only add indirection. +#[allow(clippy::large_enum_variant)] #[derive(Debug)] pub enum Command { Auth(AuthSubcommand), diff --git a/src/bin/sccache-dist/cmdline/parse.rs b/src/bin/sccache-dist/cmdline/parse.rs index 4ead7131a1..68e0532506 100644 --- a/src/bin/sccache-dist/cmdline/parse.rs +++ b/src/bin/sccache-dist/cmdline/parse.rs @@ -207,7 +207,7 @@ pub fn try_parse_from( matches .get_one::("secret-key") .expect("`secret-key` is required") - .to_string() + .clone() }; AuthSubcommand::JwtHS256ServerToken { @@ -282,7 +282,7 @@ mod tests { #[test] fn debug_assert() { - get_clap_command().debug_assert() + get_clap_command().debug_assert(); } #[test] diff --git a/src/bin/sccache-dist/main.rs b/src/bin/sccache-dist/main.rs index eb85d3125d..acdcba84ee 100644 --- a/src/bin/sccache-dist/main.rs +++ b/src/bin/sccache-dist/main.rs @@ -8,14 +8,15 @@ use sccache::config::{ INSECURE_DIST_CLIENT_TOKEN, scheduler as scheduler_config, server as server_config, }; use sccache::dist::{ - self, AllocJobResult, AssignJobResult, BuilderIncoming, CompileCommand, HeartbeatServerResult, - InputsReader, JobAlloc, JobAuthorizer, JobComplete, JobId, JobState, RunJobResult, - SchedulerIncoming, SchedulerOutgoing, SchedulerStatusResult, ServerId, ServerIncoming, - ServerNonce, ServerOutgoing, SubmitToolchainResult, TcCache, Toolchain, ToolchainReader, - UpdateJobStateResult, + self, AllocJobResult, AssignJobResult, BuilderIncoming, CompileCommand, DeregisterServerResult, + HeartbeatServerResult, InputsReader, JobAlloc, JobAuthorizer, JobComplete, JobId, JobState, + RunJobResult, SchedulerIncoming, SchedulerOutgoing, SchedulerStatusResult, ServerId, + ServerIncoming, ServerNonce, ServerOutgoing, ServerStatusResult, SubmitToolchainResult, + TcCache, Toolchain, ToolchainReader, UpdateJobStateResult, }; use sccache::util::BASE64_URL_SAFE_ENGINE; use sccache::util::daemonize; +use sccache::util::num_cpus; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, HashMap, HashSet, btree_map}; use std::env; @@ -239,6 +240,8 @@ fn run(command: Command) -> Result { scheduler_url, scheduler_auth, toolchain_cache_size, + num_cpus: advertised_num_cpus, + colocated_reserve_fraction, }) => { let builder: Box = match builder { #[cfg(not(target_os = "freebsd"))] @@ -298,11 +301,20 @@ fn run(command: Command) -> Result { let server = Server::new(builder, &cache_dir, toolchain_cache_size) .context("Failed to create sccache server instance")?; + let colocated = scheduler_url.to_url().host_str().map(str::to_owned) + == Some(public_addr.ip().to_string()); + let advertised_num_cpus = advertised_cores( + advertised_num_cpus, + num_cpus(), + colocated, + colocated_reserve_fraction, + ); let http_server = dist::http::Server::new( public_addr, bind_address, scheduler_url.to_url(), scheduler_auth, + advertised_num_cpus, server, ) .context("Failed to create sccache HTTP server instance")?; @@ -331,24 +343,91 @@ fn init_logging() { // Maximum number of jobs per core - only occurs for one core, usually less, see load_weight() const MAX_PER_CORE_LOAD: f64 = 2f64; const SERVER_REMEMBER_ERROR_TIMEOUT: Duration = Duration::from_secs(300); +// A server that just failed a job assignment is demoted so the scheduler +// prefers a healthy one. The demotion must not be absolute, though: a +// network-remote server takes transient assignment errors that a +// scheduler-colocated server never does, so a hard demotion strands the remote +// node's idle capacity while jobs pile onto a busy local one (measured: 1710 +// idle-server skips in one build, all on the remote node). Treat a recent error +// as this much extra load instead, so a recently-errored server still wins when +// it is more than this margin less loaded than the healthy alternative - a +// repeat failure only costs one wasted round-trip and a local fallback. +const RECENT_ERROR_LOAD_PENALTY: f64 = 0.125; const UNCLAIMED_PENDING_TIMEOUT: Duration = Duration::from_secs(300); const UNCLAIMED_READY_TIMEOUT: Duration = Duration::from_secs(60); +// A job that entered Started but never reported Complete leaks its +// jobs_assigned slot: the Ready->Started transition drops it from +// jobs_unclaimed, so the unclaimed reaper can never see it again, and +// prune_servers only reclaims a DEAD server's jobs. On a live server, such +// jobs accumulate until jobs_assigned hits admission_ceiling and load_weight +// stops routing to the node - the measured PC2-idle-at-37/37 lockout, its +// completion updates lost while it sat pinned. Reap a Started job the owning +// server has not completed within this timeout, mirroring prune_servers (dead +// servers) and the unclaimed reaper (stuck Ready/Pending). The value is far +// longer than any real remote single-TU compile (a heavy LLVM object is ~60s) +// yet short enough to reclaim a leaked slot briskly. Reaping only the +// scheduler's accounting never aborts the client's in-flight compile - that +// runs over a separate client<->server channel - so an over-eager reap at +// worst mildly oversubscribes, which admission_ceiling already tolerates. +const STARTED_COMPLETE_TIMEOUT: Duration = Duration::from_secs(180); +// Minimum time a job must have been in Started before the liveness lease may +// reap it for two consecutive absent heartbeats. Must exceed the 30s +// HEARTBEAT_INTERVAL (src/dist/http.rs) so a reapable job has been Started +// across at least one full beat period; otherwise a job that transitions to +// Started between two beats looks vacuously "absent from the prior beat" and +// could be reaped milliseconds after it starts. +const LIVENESS_GRACE: Duration = Duration::from_secs(45); +// A build server drops its per-job toolchain entry in handle_run_job. An +// assign whose run_job never arrives (client crash, scheduler restart) would +// otherwise leak the entry forever, so sweep entries older than this on each +// assign. Far longer than a legitimate assign->run_job gap. Must stay greater +// than UNCLAIMED_PENDING_TIMEOUT (300s) + UNCLAIMED_READY_TIMEOUT (60s) so the +// GC never evicts a still-in-flight job's toolchain entry. +const ABANDONED_TOOLCHAIN_TIMEOUT: Duration = Duration::from_secs(600); #[derive(Copy, Clone)] struct JobDetail { server_id: ServerId, state: JobState, + // When the job entered its current state; used to reap Started jobs whose + // Complete update was lost (see STARTED_COMPLETE_TIMEOUT). + since: Instant, } // To avoid deadlicking, make sure to do all locking at once (i.e. no further locking in a downward scope), // in alphabetical order pub struct Scheduler { + // Pure 0-based allocation counter. Logged as allocated= in the + // dist-accounting line, where the operator validates the identity + // allocated - started - reaped_unclaimed - live == 0, so it must start at + // 0. Job id uniqueness across restarts comes from job_id_base, not here. job_count: AtomicUsize, + // Restart-safety base for minted job ids. Seeded from wall-clock millis so + // a scheduler restart does not remint ids from 0 and collide with an id a + // surviving build server still holds (which would trip the assign-time + // overwrite path). Kept separate from job_count so the millis seed does not + // pollute the 0-based allocated= counter. + job_id_base: usize, + // Currently running jobs, can never be Complete jobs: Mutex>, servers: Mutex>, + + // Cumulative job-lifecycle counters for the between-nodes leak detector + // logged once per heartbeat (see the dist-accounting line in + // handle_heartbeat_server). Relaxed: diagnostic counters, not + // synchronization. Two leak modes are separable from the balance: + // allocated - started - reaped_unclaimed - (unclaimed jobs still live) + // drifting up => a feed/unclaimed leak (jobs allocated, never claimed). + // started - completed - reaped_started - (started jobs still live) + // drifting up => a lost-completion leak (the Leak #1 class). + jobs_started: AtomicUsize, + jobs_completed: AtomicUsize, + jobs_reaped_unclaimed: AtomicUsize, + jobs_reaped_started: AtomicUsize, + jobs_pruned: AtomicUsize, } struct ServerDetails { @@ -356,6 +435,11 @@ struct ServerDetails { // Jobs assigned that haven't seen a state change. Can only be pending // or ready. jobs_unclaimed: HashMap, + // Job ids this server reported running in its previous heartbeat. A Started + // job is reaped only when it is absent from both this set and the incoming + // heartbeat's active_jobs (two consecutive misses), so a single dropped or + // racing heartbeat never reaps a live compile. + last_active_jobs: HashSet, last_seen: Instant, last_error: Option, num_cpus: usize, @@ -367,8 +451,20 @@ impl Scheduler { pub fn new() -> Self { Scheduler { job_count: AtomicUsize::new(0), + // Seed from wall-clock millis so a scheduler restart does not remint + // job ids from 0 and collide with an id a surviving build server + // still holds (which would trip the assign-time overwrite path). + job_id_base: std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis() as usize) + .unwrap_or(0), jobs: Mutex::new(BTreeMap::new()), servers: Mutex::new(HashMap::new()), + jobs_started: AtomicUsize::new(0), + jobs_completed: AtomicUsize::new(0), + jobs_reaped_unclaimed: AtomicUsize::new(0), + jobs_reaped_started: AtomicUsize::new(0), + jobs_pruned: AtomicUsize::new(0), } } @@ -395,6 +491,8 @@ impl Scheduler { let server_details = servers .remove(&server_id) .expect("server went missing from map"); + self.jobs_pruned + .fetch_add(server_details.jobs_assigned.len(), Ordering::Relaxed); for job_id in server_details.jobs_assigned { warn!( "Non-terminated job {} was cleaned up in server pruning", @@ -419,20 +517,79 @@ impl Default for Scheduler { } } -fn load_weight(job_count: usize, core_count: usize) -> f64 { +fn admission_ceiling(core_count: usize) -> usize { // Oversubscribe cores just a little to make up for network and I/O latency. This formula is // not based on hard data but an extrapolation to high core counts of the conventional wisdom // that slightly more jobs than cores achieve the shortest compile time. Which is originally // about local compiles and this is over the network, so be slightly less conservative. - let cores_plus_slack = core_count + 1 + core_count / 8; + core_count + 1 + core_count / 8 +} + +fn load_weight(job_count: usize, core_count: usize) -> f64 { // Note >=, not >, because the question is "can we add another job"? - if job_count >= cores_plus_slack { + if job_count >= admission_ceiling(core_count) { MAX_PER_CORE_LOAD + 1f64 // no new jobs for now } else { job_count as f64 / core_count as f64 } } +/// Default fraction of its cores a colocated build server holds back from the +/// scheduler, reserving them for the local client's preprocessing and bitbake. +const DEFAULT_COLOCATED_RESERVE_FRACTION: f64 = 0.35; + +/// Resolve the core count a build server advertises to the scheduler. An +/// explicit `num_cpus` wins; otherwise a server colocated with the scheduler +/// reserves `reserve_fraction` of its hardware (default +/// [`DEFAULT_COLOCATED_RESERVE_FRACTION`]) so the load model routes +/// proportionally more to the pure-remote nodes, while a remote server +/// advertises all its cores. Computed from the node's own `hw`, so the same +/// binary and config are portable across cluster topologies. +fn advertised_cores( + explicit: Option, + hw: usize, + colocated: bool, + reserve_fraction: Option, +) -> usize { + if let Some(n) = explicit { + return n; + } + if colocated { + let fraction = reserve_fraction + .unwrap_or(DEFAULT_COLOCATED_RESERVE_FRACTION) + .clamp(0.0, 0.9); + std::cmp::max(1, (hw as f64 * (1.0 - fraction)).round() as usize) + } else { + hw + } +} + +#[cfg(test)] +mod advertised_cores_tests { + use super::advertised_cores; + + #[test] + fn reserves_the_default_fraction_when_colocated() { + // 32 cores, colocated, default 0.35 -> round(32 * 0.65) = 21. + assert_eq!(advertised_cores(None, 32, true, None), 21); + } + + #[test] + fn a_remote_server_advertises_all_cores() { + assert_eq!(advertised_cores(None, 32, false, None), 32); + } + + #[test] + fn an_explicit_count_overrides_the_fraction() { + assert_eq!(advertised_cores(Some(16), 32, true, None), 16); + } + + #[test] + fn a_custom_fraction_is_honoured() { + assert_eq!(advertised_cores(None, 32, true, Some(0.5)), 16); + } +} + impl SchedulerIncoming for Scheduler { fn handle_alloc_job( &self, @@ -443,13 +600,34 @@ impl SchedulerIncoming for Scheduler { // LOCKS let mut servers = self.servers.lock().unwrap(); + // R0 instrumentation: snapshot every candidate's load so the + // allocation decision logged below exposes the load distribution + // across nodes and any first-hashed-server tie bias. + let mut candidate_loads: Vec<(ServerId, f64, usize, usize)> = Vec::new(); + let res = { let mut best = None; let mut best_err = None; let mut best_load: f64 = MAX_PER_CORE_LOAD; + let mut best_free: usize = 0; + let mut best_err_load: f64 = MAX_PER_CORE_LOAD; let now = Instant::now(); for (&server_id, details) in servers.iter_mut() { + // A server silent past the heartbeat timeout is dead but + // is only removed by prune_servers (heartbeat / status + // paths), not here. Skip it so a job is never dispatched + // to a node that is gone - which would fail and fall back + // local, silently skewing dist measurements. + if now.duration_since(details.last_seen) > dist::http::HEARTBEAT_TIMEOUT { + continue; + } let load = load_weight(details.jobs_assigned.len(), details.num_cpus); + candidate_loads.push(( + server_id, + load, + details.jobs_assigned.len(), + details.num_cpus, + )); if let Some(last_error) = details.last_error { if load < MAX_PER_CORE_LOAD { @@ -471,6 +649,7 @@ impl SchedulerIncoming for Scheduler { now - last_error ); best_err = Some((server_id, details)); + best_err_load = load; } } _ => { @@ -480,23 +659,52 @@ impl SchedulerIncoming for Scheduler { now - last_error ); best_err = Some((server_id, details)); + best_err_load = load; } } } - } else if load < best_load { - best = Some((server_id, details)); - trace!("Selected {:?} as the server with the best load", server_id); - best_load = load; - if load == 0f64 { - break; + } else { + // Deterministic, remote-preferring tie-break, and no early + // break: scan every server so the logged candidate list is + // complete (a truncated list can never expose a misroute) and + // hash-map order never silently decides a both-idle tie. On + // equal load prefer the server with more free admission slots - + // with A1's advertised-cores override the colocated node + // advertises fewer, so the remote node wins the tie and a + // compile burst starts off the already-busy local node. + let free = admission_ceiling(details.num_cpus) + .saturating_sub(details.jobs_assigned.len()); + if load < best_load || (load == best_load && free > best_free) { + trace!("Selected {:?} as the server with the best load", server_id); + best = Some((server_id, details)); + best_load = load; + best_free = free; } } } - // Assign the job to our best choice - if let Some((server_id, server_details)) = best.or(best_err) { - let job_count = self.job_count.fetch_add(1, Ordering::SeqCst) as u64; - let job_id = JobId(job_count); + // Assign the job to our best choice. Prefer the least-loaded + // healthy server, but fall to a recently-errored one when it is + // more than RECENT_ERROR_LOAD_PENALTY less loaded - so a remote + // node briefly demoted by a transient error is not stranded idle + // while a busy healthy node absorbs the work. best_load / + // best_err_load stay MAX_PER_CORE_LOAD while their bucket is + // empty, so the comparison naturally keeps the populated one. + let choice = match (best, best_err) { + (Some(clean), Some(errored)) => { + if best_err_load + RECENT_ERROR_LOAD_PENALTY < best_load { + Some(errored) + } else { + Some(clean) + } + } + (Some(clean), None) => Some(clean), + (None, errored) => errored, + }; + if let Some((server_id, server_details)) = choice { + let job_id = JobId( + (self.job_id_base + self.job_count.fetch_add(1, Ordering::SeqCst)) as u64, + ); assert!(server_details.jobs_assigned.insert(job_id)); assert!( server_details @@ -509,6 +717,15 @@ impl SchedulerIncoming for Scheduler { "Job {} created and will be assigned to server {:?}", job_id, server_id ); + info!( + "dist-alloc: job {} -> {:?} (now {}/{} jobs, load {:.3}); candidates {:?}", + job_id, + server_id, + server_details.jobs_assigned.len(), + server_details.num_cpus, + load_weight(server_details.jobs_assigned.len(), server_details.num_cpus), + candidate_loads, + ); let auth = server_details .job_authorizer .generate_token(job_id) @@ -526,6 +743,7 @@ impl SchedulerIncoming for Scheduler { "Insufficient capacity across {} available servers", servers.len() ); + warn!("dist-alloc: {}; candidates {:?}", msg, candidate_loads); return Ok(AllocJobResult::Fail { msg }); } }; @@ -558,8 +776,15 @@ impl SchedulerIncoming for Scheduler { job_id, state ); assert!( - jobs.insert(job_id, JobDetail { server_id, state }) - .is_none() + jobs.insert( + job_id, + JobDetail { + server_id, + state, + since: Instant::now(), + } + ) + .is_none() ); } let job_alloc = JobAlloc { @@ -579,6 +804,7 @@ impl SchedulerIncoming for Scheduler { server_nonce: ServerNonce, num_cpus: usize, job_authorizer: Box, + active_jobs: Vec, ) -> Result { if num_cpus == 0 { bail!("Invalid number of CPUs (0) specified in heartbeat") @@ -625,6 +851,8 @@ impl SchedulerIncoming for Scheduler { "The following stale jobs will be de-allocated: {:?}", stale_jobs ); + self.jobs_reaped_unclaimed + .fetch_add(stale_jobs.len(), Ordering::Relaxed); for job_id in stale_jobs { if !details.jobs_assigned.remove(&job_id) { @@ -651,6 +879,105 @@ impl SchedulerIncoming for Scheduler { } } + // Primary Started-reap mechanism: the liveness lease. Every + // heartbeat carries the ids the server is actively running. + // Reap a Started job only once it is absent from BOTH the + // incoming active_jobs AND last_active_jobs (the prior beat) - + // two consecutive misses - so a genuinely-slow live compile, + // which the server reports every beat, is never reaped, and one + // dropped or racing heartbeat cannot reap a live job. The + // fixed-timeout sweep below stays only as a backstop for a + // server that stops heartbeating its running set entirely. + let active_set: HashSet = active_jobs.into_iter().collect(); + let mut liveness_reap = Vec::new(); + for &job_id in details.jobs_assigned.iter() { + if let Some(detail) = jobs.get(&job_id) { + if detail.state == JobState::Started + && !active_set.contains(&job_id) + && !details.last_active_jobs.contains(&job_id) + && now.duration_since(detail.since) > LIVENESS_GRACE + { + liveness_reap.push(job_id); + } + } + } + if !liveness_reap.is_empty() { + warn!( + "The following Started jobs were absent from two consecutive heartbeats and will be de-allocated: {:?}", + liveness_reap + ); + self.jobs_reaped_started + .fetch_add(liveness_reap.len(), Ordering::Relaxed); + for job_id in liveness_reap { + details.jobs_assigned.remove(&job_id); + jobs.remove(&job_id); + } + } + details.last_active_jobs = active_set; + + // Backstop for jobs stuck in Started (see STARTED_COMPLETE_TIMEOUT): + // the unclaimed sweep above cannot see them because Ready->Started + // drops them from jobs_unclaimed. Reap any this server has held in + // Started past the timeout so a lost Complete update cannot leak the + // slot forever and eventually lock the node out at its ceiling. + // Never reap a job the server just reported active (now stored in + // last_active_jobs): a genuinely-slow live compile must survive at + // any duration, so the backstop only fires once the server has + // stopped naming the job in its heartbeats. + let mut stale_started = Vec::new(); + for &job_id in details.jobs_assigned.iter() { + if let Some(detail) = jobs.get(&job_id) { + if detail.state == JobState::Started + && now.duration_since(detail.since) > STARTED_COMPLETE_TIMEOUT + { + if details.last_active_jobs.contains(&job_id) { + // Still reported active past the backstop timeout: + // not reaped (a genuinely-slow live compile must + // survive) but likely a wedged build the server + // names forever. Surface it so it is visible in + // logs rather than silently accumulating. + warn!( + "Started job {} on server {} still reported active past {:?}; possible wedged build", + job_id, + server_id.addr(), + STARTED_COMPLETE_TIMEOUT + ); + } else { + stale_started.push(job_id); + } + } + } + } + if !stale_started.is_empty() { + warn!( + "The following Started jobs had no Complete within {:?} and will be de-allocated: {:?}", + STARTED_COMPLETE_TIMEOUT, stale_started + ); + self.jobs_reaped_started + .fetch_add(stale_started.len(), Ordering::Relaxed); + for job_id in stale_started { + details.jobs_assigned.remove(&job_id); + jobs.remove(&job_id); + } + } + + // Between-nodes leak detector: one cumulative accounting line + // per heartbeat (cheap - ~per-30s per server, NOT per compile). + // A rising (allocated - started - reaped_unclaimed - unclaimed + // still live) points to a feed/unclaimed leak; a rising (started + // - completed - reaped_started - started still live) points to a + // lost-completion leak. `live` = current jobs-map size. + info!( + "dist-accounting: allocated={} started={} completed={} reaped_unclaimed={} reaped_started={} pruned={} live={}", + self.job_count.load(Ordering::Relaxed), + self.jobs_started.load(Ordering::Relaxed), + self.jobs_completed.load(Ordering::Relaxed), + self.jobs_reaped_unclaimed.load(Ordering::Relaxed), + self.jobs_reaped_started.load(Ordering::Relaxed), + self.jobs_pruned.load(Ordering::Relaxed), + jobs.len(), + ); + return Ok(HeartbeatServerResult { is_new: false }); } Some(ref mut details) if details.server_nonce != server_nonce => { @@ -674,6 +1001,7 @@ impl SchedulerIncoming for Scheduler { last_error: None, jobs_assigned: HashSet::new(), jobs_unclaimed: HashMap::new(), + last_active_jobs: HashSet::new(), num_cpus, server_nonce, job_authorizer, @@ -682,6 +1010,28 @@ impl SchedulerIncoming for Scheduler { Ok(HeartbeatServerResult { is_new: true }) } + fn handle_deregister_server(&self, server_id: ServerId) -> Result { + // LOCKS (jobs before servers, matching the alphabetical lock order + // used across the scheduler to avoid deadlock). + let mut jobs = self.jobs.lock().unwrap(); + let mut servers = self.servers.lock().unwrap(); + + if let Some(details) = servers.remove(&server_id) { + info!( + "Server {} deregistered on graceful shutdown", + server_id.addr() + ); + // Drop any jobs the departing server still held, as prune_servers + // does, so the scheduler does not track work no node will finish. + self.jobs_pruned + .fetch_add(details.jobs_assigned.len(), Ordering::Relaxed); + for job_id in details.jobs_assigned { + jobs.remove(&job_id); + } + } + Ok(DeregisterServerResult { success: true }) + } + fn handle_update_job_state( &self, job_id: JobId, @@ -706,31 +1056,64 @@ impl SchedulerIncoming for Scheduler { let mut server_details = servers.get_mut(&server_id); if let Some(ref mut details) = server_details { details.last_seen = now; - }; + } match (job_detail.state, job_state) { - (JobState::Pending, JobState::Ready) => entry.get_mut().state = job_state, + (JobState::Pending, JobState::Ready) => { + // Reset the unclaimed-reservation clock to Ready time rather + // than allocation time, so time spent uploading a toolchain + // (Pending) does not consume the claim budget and a job that + // is still uploading is not reaped as a no-show. + if let Some(details) = server_details { + if let Some(t) = details.jobs_unclaimed.get_mut(&job_id) { + *t = now; + } + } + let detail = entry.get_mut(); + detail.state = job_state; + detail.since = now; + } (JobState::Ready, JobState::Started) => { if let Some(details) = server_details { details.jobs_unclaimed.remove(&job_id); } else { - warn!("Job state updated, but server is not known to scheduler") + warn!("Job state updated, but server is not known to scheduler"); } - entry.get_mut().state = job_state + let detail = entry.get_mut(); + detail.state = job_state; + detail.since = now; + self.jobs_started.fetch_add(1, Ordering::Relaxed); } (JobState::Started, JobState::Complete) => { let (job_id, _) = entry.remove_entry(); if let Some(entry) = server_details { - assert!(entry.jobs_assigned.remove(&job_id)) + assert!(entry.jobs_assigned.remove(&job_id)); } else { bail!("Job was marked as finished, but server is not known to scheduler") } + self.jobs_completed.fetch_add(1, Ordering::Relaxed); } (from, to) => bail!("Invalid job state transition from {} to {}", from, to), } info!("Job {} updated state to {:?}", job_id, job_state); } else { - bail!("Unknown job") + // Idempotent late update: a Started/Complete update for a job the + // scheduler no longer tracks (already reaped, or reclaimed by a + // liveness sweep) is a logged no-op success, never an error. Erroring + // here fails the server's run_job (which propagates the terminal + // update), so the client would discard finished objects and recompile + // locally - the reap-then-complete hazard. + match job_state { + JobState::Started | JobState::Complete => { + info!( + "Late {:?} update for untracked job {} (already reaped); no-op success", + job_state, job_id + ); + } + // A Pending/Ready update for an untracked job is a real anomaly, + // not a benign late terminal update, so keep erroring on it. + _ => bail!("Unknown job"), + } } Ok(UpdateJobStateResult::Success) } @@ -742,10 +1125,30 @@ impl SchedulerIncoming for Scheduler { self.prune_servers(&mut servers, &mut jobs); + // R0 instrumentation: one-line status snapshot per poll so the + // in-progress cadence across nodes is recoverable from the log. + let status_snapshot: Vec<(String, usize, usize)> = servers + .iter() + .map(|(id, d)| (id.addr().to_string(), d.jobs_assigned.len(), d.num_cpus)) + .collect(); + info!( + "dist-status poll: {} in-progress jobs; servers {:?}", + jobs.len(), + status_snapshot + ); + Ok(SchedulerStatusResult { num_servers: servers.len(), num_cpus: servers.values().map(|v| v.num_cpus).sum(), in_progress: jobs.len(), + servers: servers + .iter() + .map(|(server_id, details)| ServerStatusResult { + id: server_id.addr().to_string(), + num_cpus: details.num_cpus, + in_progress: details.jobs_assigned.len(), + }) + .collect(), }) } } @@ -753,7 +1156,10 @@ impl SchedulerIncoming for Scheduler { pub struct Server { builder: Box, cache: Mutex, - job_toolchains: Mutex>, + job_toolchains: Mutex>, + // Jobs currently executing a build. Reported in every heartbeat so the + // scheduler's liveness lease never reaps a compile still in flight here. + running_jobs: Mutex>, } impl Server { @@ -768,20 +1174,45 @@ impl Server { builder, cache: Mutex::new(cache), job_toolchains: Mutex::new(HashMap::new()), + running_jobs: Mutex::new(HashSet::new()), }) } } +// Removes a job id from the running set no matter how handle_run_job returns +// (normal completion, build error, or panic), so a failed build can never +// leave a phantom entry that makes the scheduler believe the job is still live. +struct RunningJobGuard<'a> { + running_jobs: &'a Mutex>, + job_id: JobId, +} + +impl Drop for RunningJobGuard<'_> { + fn drop(&mut self) { + self.running_jobs.lock().unwrap().remove(&self.job_id); + } +} + impl ServerIncoming for Server { fn handle_assign_job(&self, job_id: JobId, tc: Toolchain) -> Result { let need_toolchain = !self.cache.lock().unwrap().contains_toolchain(&tc); - assert!( - self.job_toolchains - .lock() - .unwrap() - .insert(job_id, tc) - .is_none() - ); + { + let now = Instant::now(); + let mut toolchains = self.job_toolchains.lock().unwrap(); + // GC abandoned entries (assigns whose run_job never arrived) so the + // map cannot grow without bound. + toolchains + .retain(|_, (_, since)| now.duration_since(*since) < ABANDONED_TOOLCHAIN_TIMEOUT); + // Overwrite rather than assert: after a scheduler restart the job + // counter reseeds and a surviving server can still hold a stale entry + // for a reused id. Warn and overwrite instead of panicking the worker. + if toolchains.insert(job_id, (tc, now)).is_some() { + warn!( + "Overwrote a stale toolchain entry for reused job id {}", + job_id + ); + } + } let state = if need_toolchain { JobState::Pending } else { @@ -804,19 +1235,42 @@ impl ServerIncoming for Server { .context("Updating job state failed")?; // TODO: need to lock the toolchain until the container has started // TODO: can start prepping container - let tc = match self.job_toolchains.lock().unwrap().get(&job_id).cloned() { + let tc = match self + .job_toolchains + .lock() + .unwrap() + .get(&job_id) + .map(|(tc, _)| tc.clone()) + { Some(tc) => tc, None => return Ok(SubmitToolchainResult::JobNotFound), }; - let mut cache = self.cache.lock().unwrap(); - // TODO: this returns before reading all the data, is that valid? - if cache.contains_toolchain(&tc) { - return Ok(SubmitToolchainResult::Success); + // Fast path: skip the transfer entirely if the toolchain is already + // cached. Grab the cache directory while we hold the lock so the temp + // file lands on the same filesystem and the final insert is a rename. + let tc_cache_dir = { + let cache = self.cache.lock().unwrap(); + // TODO: this returns before reading all the data, is that valid? + if cache.contains_toolchain(&tc) { + return Ok(SubmitToolchainResult::Success); + } + cache.path().to_path_buf() + }; + // Stream the upload to a temp file WITHOUT holding the cache lock, so a + // slow transfer for one toolchain no longer serializes every other + // submit behind the global cache mutex. + let mut tmpfile = match tempfile::NamedTempFile::new_in(&tc_cache_dir) { + Ok(tmpfile) => tmpfile, + Err(_) => return Ok(SubmitToolchainResult::CannotCache), + }; + if io::copy(&mut { tc_rdr }, tmpfile.as_file_mut()).is_err() { + return Ok(SubmitToolchainResult::CannotCache); } + let temp_path = tmpfile.into_temp_path(); + // Re-acquire the lock ONLY to graft the finished file into the cache. + let mut cache = self.cache.lock().unwrap(); Ok(cache - .insert_with(&tc, |mut file| { - io::copy(&mut { tc_rdr }, &mut file).map(|_| ()) - }) + .insert_at(&tc, &temp_path) .map(|_| SubmitToolchainResult::Success) .unwrap_or(SubmitToolchainResult::CannotCache)) } @@ -828,10 +1282,25 @@ impl ServerIncoming for Server { outputs: Vec, inputs_rdr: InputsReader, ) -> Result { + // Register the job and arm the cleanup guard BEFORE telling the + // scheduler the job started, so the id is tracked before the scheduler + // acts on the Started update (shrinking the vacuous liveness-reap + // window). De-registers on every exit path (success, build error, + // panic), and the guard's Drop still cleans up if the update fails. + self.running_jobs.lock().unwrap().insert(job_id); + let _running_guard = RunningJobGuard { + running_jobs: &self.running_jobs, + job_id, + }; requester .do_update_job_state(job_id, JobState::Started) .context("Updating job state failed")?; - let tc = self.job_toolchains.lock().unwrap().remove(&job_id); + let tc = self + .job_toolchains + .lock() + .unwrap() + .remove(&job_id) + .map(|(tc, _)| tc); let res = match tc { None => Ok(RunJobResult::JobNotFound), Some(tc) => { @@ -847,9 +1316,632 @@ impl ServerIncoming for Server { } } }; - requester - .do_update_job_state(job_id, JobState::Complete) - .context("Updating job state failed")?; + if let Err(e) = requester.do_update_job_state(job_id, JobState::Complete) { + // Do not fail the compile because the scheduler rejected the terminal + // update (e.g. the job was reaped mid-compile): the objects are built + // and must reach the client rather than being discarded for a local + // recompile. + warn!( + "Completing job {} with the scheduler failed ({:#}); returning results anyway", + job_id, e + ); + } res } + fn active_jobs(&self) -> Vec { + self.running_jobs.lock().unwrap().iter().copied().collect() + } +} + +#[cfg(test)] +mod scheduler_tests { + use super::*; + use std::net::SocketAddr; + + struct NoopAuthorizer; + impl JobAuthorizer for NoopAuthorizer { + fn generate_token(&self, _job_id: JobId) -> Result { + Ok(String::new()) + } + fn verify_token(&self, _job_id: JobId, _token: &str) -> Result<()> { + Ok(()) + } + } + + struct PanicOnAssign; + impl SchedulerOutgoing for PanicOnAssign { + fn do_assign_job( + &self, + _server_id: ServerId, + _job_id: JobId, + _tc: Toolchain, + _auth: String, + ) -> Result { + panic!("a job must never be assigned to a stale server"); + } + } + + struct AssignOk; + impl SchedulerOutgoing for AssignOk { + fn do_assign_job( + &self, + _server_id: ServerId, + _job_id: JobId, + _tc: Toolchain, + _auth: String, + ) -> Result { + Ok(AssignJobResult { + state: JobState::Ready, + need_toolchain: false, + }) + } + } + + fn insert_server(scheduler: &Scheduler, addr: &str, last_seen: Instant) { + let server_id = ServerId::new(addr.parse::().unwrap()); + let mut servers = scheduler.servers.lock().unwrap(); + servers.insert( + server_id, + ServerDetails { + last_seen, + last_error: None, + jobs_assigned: HashSet::new(), + jobs_unclaimed: HashMap::new(), + last_active_jobs: HashSet::new(), + num_cpus: 32, + server_nonce: ServerNonce::new(), + job_authorizer: Box::new(NoopAuthorizer), + }, + ); + } + + fn a_toolchain() -> Toolchain { + Toolchain { + archive_id: "deadbeef".to_owned(), + } + } + + // A server silent past HEARTBEAT_TIMEOUT is dead but is only removed by + // prune_servers, which runs on the heartbeat and status paths - not on + // alloc_job. Until then it must never receive a job, or the scheduler + // dispatches to a node that is gone and the compile falls back local, + // silently skewing a dist measurement. + #[test] + fn test_alloc_job_skips_server_stale_beyond_heartbeat_timeout() { + let scheduler = Scheduler::new(); + let stale = Instant::now() - (dist::http::HEARTBEAT_TIMEOUT + Duration::from_secs(10)); + insert_server(&scheduler, "10.42.0.2:10501", stale); + + let res = scheduler + .handle_alloc_job(&PanicOnAssign, a_toolchain()) + .expect("handle_alloc_job should not error"); + + assert!( + matches!(res, AllocJobResult::Fail { .. }), + "a stale server must not be selected for assignment" + ); + } + + // Guard against over-filtering: a server seen within the timeout is alive + // and must remain a valid assignment target. + #[test] + fn test_alloc_job_uses_fresh_server() { + let scheduler = Scheduler::new(); + insert_server(&scheduler, "10.42.0.1:10501", Instant::now()); + + let res = scheduler + .handle_alloc_job(&AssignOk, a_toolchain()) + .expect("handle_alloc_job should not error"); + + assert!( + matches!(res, AllocJobResult::Success { .. }), + "a fresh server must be selected for assignment" + ); + } + + // A graceful deregister must drop the server at once, without waiting for + // the heartbeat timeout - even though its last_seen is current. + #[test] + fn test_deregister_server_removes_it_immediately() { + let scheduler = Scheduler::new(); + insert_server(&scheduler, "10.42.0.2:10501", Instant::now()); + + let server_id = ServerId::new("10.42.0.2:10501".parse::().unwrap()); + let res = scheduler + .handle_deregister_server(server_id) + .expect("handle_deregister_server should not error"); + assert!(res.success); + + let status = scheduler.handle_status().expect("status"); + assert_eq!( + status.num_servers, 0, + "a deregistered server must be gone immediately" + ); + } + + // Insert a live server carrying `num_jobs` assigned jobs and an optional + // recent error. Job ids start high so they never collide with the ids the + // scheduler mints from job_count (which starts at 0) during the alloc. + fn insert_busy_server( + scheduler: &Scheduler, + addr: &str, + num_jobs: usize, + last_error: Option, + ) { + let server_id = ServerId::new(addr.parse::().unwrap()); + let mut jobs_assigned = HashSet::new(); + for i in 0..num_jobs { + jobs_assigned.insert(JobId(1000 + i as u64)); + } + let mut servers = scheduler.servers.lock().unwrap(); + servers.insert( + server_id, + ServerDetails { + last_seen: Instant::now(), + last_error, + jobs_assigned, + jobs_unclaimed: HashMap::new(), + last_active_jobs: HashSet::new(), + num_cpus: 32, + server_nonce: ServerNonce::new(), + job_authorizer: Box::new(NoopAuthorizer), + }, + ); + } + + // A recently-errored but idle server must still win over a healthy but + // heavily-loaded one. A network-remote node takes transient assignment + // errors a colocated node never does; stranding its idle capacity while a + // busy node absorbs the work was the dominant misroute (measured 1710x, all + // on the remote node). Here the 12/32-vs-0 load gap clears the error + // penalty, so the idle errored node wins. + #[test] + fn test_alloc_job_prefers_idle_errored_server_over_loaded_healthy_server() { + let scheduler = Scheduler::new(); + insert_busy_server(&scheduler, "10.42.0.1:10501", 12, None); + insert_busy_server(&scheduler, "10.42.0.2:10501", 0, Some(Instant::now())); + + let res = scheduler + .handle_alloc_job(&AssignOk, a_toolchain()) + .expect("handle_alloc_job should not error"); + + match res { + AllocJobResult::Success { job_alloc, .. } => assert_eq!( + job_alloc.server_id.addr().to_string(), + "10.42.0.2:10501", + "the idle recently-errored server must win over the loaded healthy one" + ), + _ => panic!("expected a successful allocation"), + } + } + + // The error demotion is not abandoned: when the healthy server is only + // marginally busier (within the penalty), the recently-errored server stays + // demoted, so a flaky node is not chosen for a negligible capacity gain. + #[test] + fn test_alloc_job_keeps_healthy_server_when_error_penalty_outweighs_gap() { + let scheduler = Scheduler::new(); + insert_busy_server(&scheduler, "10.42.0.1:10501", 2, None); + insert_busy_server(&scheduler, "10.42.0.2:10501", 0, Some(Instant::now())); + + let res = scheduler + .handle_alloc_job(&AssignOk, a_toolchain()) + .expect("handle_alloc_job should not error"); + + match res { + AllocJobResult::Success { job_alloc, .. } => assert_eq!( + job_alloc.server_id.addr().to_string(), + "10.42.0.1:10501", + "a marginally-busier healthy server must be kept over a flaky one" + ), + _ => panic!("expected a successful allocation"), + } + } + + // Register `job_id` on `server_id` as an in-flight Started job whose state + // was entered `age` ago, exactly as it sits after a Ready->Started update: + // present in jobs_assigned and the jobs map, absent from jobs_unclaimed. + fn insert_started_job( + scheduler: &Scheduler, + server_id: ServerId, + job_id: JobId, + age: Duration, + ) { + let mut servers = scheduler.servers.lock().unwrap(); + let mut jobs_assigned = HashSet::new(); + jobs_assigned.insert(job_id); + servers.insert( + server_id, + ServerDetails { + last_seen: Instant::now(), + last_error: None, + jobs_assigned, + jobs_unclaimed: HashMap::new(), + last_active_jobs: HashSet::new(), + num_cpus: 32, + server_nonce: ServerNonce::new(), + job_authorizer: Box::new(NoopAuthorizer), + }, + ); + let mut jobs = scheduler.jobs.lock().unwrap(); + jobs.insert( + job_id, + JobDetail { + server_id, + state: JobState::Started, + since: Instant::now() - age, + }, + ); + } + + // A job stuck in Started (its Complete update lost) must be reaped on the + // owning server's next heartbeat, or its jobs_assigned slot leaks until the + // node hits admission_ceiling and load_weight locks it out - the measured + // PC2-idle-at-37/37 failure. prune_servers reclaims only dead servers and + // the unclaimed reaper sees only Ready/Pending, so without a Started + // backstop the slot never frees on a live server. Falsifier: dropping the + // Started sweep leaves the job assigned after the heartbeat. + #[test] + fn test_heartbeat_reaps_started_job_past_completion_timeout() { + let scheduler = Scheduler::new(); + let server_id = ServerId::new("10.42.0.2:10501".parse::().unwrap()); + let job_id = JobId(1000); + insert_started_job( + &scheduler, + server_id, + job_id, + STARTED_COMPLETE_TIMEOUT + Duration::from_secs(10), + ); + // Seed the prior-beat active set so the liveness lease grants this job + // its grace beat: with the job absent from active_jobs but present in + // last_active_jobs, the liveness path skips it, so the reap here can + // only come from the age-based backstop this test exercises. + scheduler + .servers + .lock() + .unwrap() + .get_mut(&server_id) + .unwrap() + .last_active_jobs + .insert(job_id); + let nonce = scheduler + .servers + .lock() + .unwrap() + .get(&server_id) + .unwrap() + .server_nonce + .clone(); + + scheduler + .handle_heartbeat_server(server_id, nonce, 32, Box::new(NoopAuthorizer), Vec::new()) + .expect("handle_heartbeat_server should not error"); + + let servers = scheduler.servers.lock().unwrap(); + let details = servers.get(&server_id).expect("server still registered"); + assert!( + !details.jobs_assigned.contains(&job_id), + "a Started job past the completion timeout must be de-allocated from jobs_assigned" + ); + let jobs = scheduler.jobs.lock().unwrap(); + assert!( + !jobs.contains_key(&job_id), + "the stale Started job must be removed from the jobs map" + ); + assert_eq!( + scheduler.jobs_reaped_started.load(Ordering::Relaxed), + 1, + "reaping a stale Started job must increment the leak-detector counter" + ); + } + + // Guard against over-reaping: a Started job within the timeout is a compile + // in flight, so it must stay assigned and keep counting toward the server's + // load. + #[test] + fn test_heartbeat_keeps_started_job_within_completion_timeout() { + let scheduler = Scheduler::new(); + let server_id = ServerId::new("10.42.0.2:10501".parse::().unwrap()); + let job_id = JobId(1000); + insert_started_job(&scheduler, server_id, job_id, Duration::from_secs(0)); + let nonce = scheduler + .servers + .lock() + .unwrap() + .get(&server_id) + .unwrap() + .server_nonce + .clone(); + + // The server reports the job still running, so both the liveness path + // (present in active_jobs) and the age-based backstop (within timeout) + // must leave it assigned. + scheduler + .handle_heartbeat_server(server_id, nonce, 32, Box::new(NoopAuthorizer), vec![job_id]) + .expect("handle_heartbeat_server should not error"); + + let servers = scheduler.servers.lock().unwrap(); + let details = servers.get(&server_id).expect("server still registered"); + assert!( + details.jobs_assigned.contains(&job_id), + "a Started job within the completion timeout must stay assigned" + ); + } + + // The liveness lease is the primary Started-reap mechanism: a Started job is + // reaped only after it is absent from two consecutive heartbeats of the + // owning server. A job the server keeps reporting active is never reaped (no + // matter how long the compile runs), and a single missed heartbeat is not + // enough - guarding both the slow-live-compile false reap the old fixed + // timeout caused and an over-eager one-beat reap. Job age is seeded past the + // LIVENESS_GRACE floor (45s) but below the STARTED_COMPLETE_TIMEOUT backstop + // (180s), so the backstop never fires and every reap decision here is the + // liveness path. + #[test] + fn test_liveness_lease_reap() { + let scheduler = Scheduler::new(); + let server_id = ServerId::new("10.42.0.2:10501".parse::().unwrap()); + let job_id = JobId(1000); + insert_started_job(&scheduler, server_id, job_id, Duration::from_secs(60)); + let nonce = scheduler + .servers + .lock() + .unwrap() + .get(&server_id) + .unwrap() + .server_nonce + .clone(); + + // Reported active across many heartbeats: never reaped. + for _ in 0..5 { + scheduler + .handle_heartbeat_server( + server_id, + nonce.clone(), + 32, + Box::new(NoopAuthorizer), + vec![job_id], + ) + .expect("handle_heartbeat_server should not error"); + let servers = scheduler.servers.lock().unwrap(); + assert!( + servers + .get(&server_id) + .unwrap() + .jobs_assigned + .contains(&job_id), + "a job reported active must never be reaped" + ); + } + + // First absent heartbeat: one miss alone must not reap (needs two). + scheduler + .handle_heartbeat_server( + server_id, + nonce.clone(), + 32, + Box::new(NoopAuthorizer), + Vec::new(), + ) + .expect("handle_heartbeat_server should not error"); + assert!( + scheduler + .servers + .lock() + .unwrap() + .get(&server_id) + .unwrap() + .jobs_assigned + .contains(&job_id), + "a single absent heartbeat must not reap - the lease needs two consecutive misses" + ); + + // Second consecutive absent heartbeat: now the job is reaped. + scheduler + .handle_heartbeat_server(server_id, nonce, 32, Box::new(NoopAuthorizer), Vec::new()) + .expect("handle_heartbeat_server should not error"); + { + let servers = scheduler.servers.lock().unwrap(); + let details = servers.get(&server_id).expect("server still registered"); + assert!( + !details.jobs_assigned.contains(&job_id), + "two consecutive absent heartbeats must reap the Started job" + ); + } + assert!( + !scheduler.jobs.lock().unwrap().contains_key(&job_id), + "the liveness-reaped job must be removed from the jobs map" + ); + assert_eq!( + scheduler.jobs_reaped_started.load(Ordering::Relaxed), + 1, + "the liveness reap must increment the Started-reap counter exactly once" + ); + } + + // The LIVENESS_GRACE floor protects a just-started job: a job that has been + // Started for less than the grace must NOT be reaped even after two absent + // heartbeats, because a job that transitioned to Started between two beats + // is vacuously "absent from the prior beat" and would otherwise be reaped + // milliseconds after it started. Falsifier: dropping the grace clause reaps + // this job on the second absent heartbeat. + #[test] + fn test_liveness_lease_does_not_reap_within_grace() { + let scheduler = Scheduler::new(); + let server_id = ServerId::new("10.42.0.2:10501".parse::().unwrap()); + let job_id = JobId(1000); + // Aged below LIVENESS_GRACE (45s): a fresh Started job. + insert_started_job(&scheduler, server_id, job_id, Duration::from_secs(10)); + let nonce = scheduler + .servers + .lock() + .unwrap() + .get(&server_id) + .unwrap() + .server_nonce + .clone(); + + // Two consecutive absent heartbeats: the two-miss condition is met, but + // the grace floor must still protect the just-started job. + for _ in 0..2 { + scheduler + .handle_heartbeat_server( + server_id, + nonce.clone(), + 32, + Box::new(NoopAuthorizer), + Vec::new(), + ) + .expect("handle_heartbeat_server should not error"); + } + + { + let servers = scheduler.servers.lock().unwrap(); + let details = servers.get(&server_id).expect("server still registered"); + assert!( + details.jobs_assigned.contains(&job_id), + "a job Started within LIVENESS_GRACE must not be reaped even after two absent heartbeats" + ); + } + assert!( + scheduler.jobs.lock().unwrap().contains_key(&job_id), + "a job Started within the grace must remain in the jobs map" + ); + assert_eq!( + scheduler.jobs_reaped_started.load(Ordering::Relaxed), + 0, + "no Started reap must occur within the grace floor" + ); + } + + // A late Complete (or Started) update for a job the scheduler no longer + // tracks must be a no-op success, not an error. If it errors, the build + // server's run_job (which propagates the terminal update) fails, and the + // client discards the finished objects and recompiles locally - the + // reap-then-complete hazard that a fixed-timeout reaper can trigger on a + // slow-but-successful remote compile. + #[test] + fn test_late_update_for_reaped_job_is_ok() { + let scheduler = Scheduler::new(); + let server_id = ServerId::new("10.42.0.2:10501".parse::().unwrap()); + + // No job was ever inserted, so job 1000 is "unknown" to the scheduler, + // exactly as it would be after a reap. + let res = scheduler.handle_update_job_state(JobId(1000), server_id, JobState::Complete); + + assert!( + res.is_ok(), + "a Complete update for an untracked (reaped) job must be a logged no-op success, not an error" + ); + } + + // The unclaimed-reservation clock must be reset when a job becomes claimable + // (Pending->Ready), so time spent uploading a toolchain does not eat the + // claim budget and cause a no-show reap of a job that is still arriving. + #[test] + fn test_pending_to_ready_resets_unclaimed_clock() { + let scheduler = Scheduler::new(); + let server_id = ServerId::new("10.42.0.2:10501".parse::().unwrap()); + let job_id = JobId(1000); + let old = Instant::now() - Duration::from_secs(120); + { + let mut servers = scheduler.servers.lock().unwrap(); + let mut jobs_unclaimed = HashMap::new(); + jobs_unclaimed.insert(job_id, old); + let mut jobs_assigned = HashSet::new(); + jobs_assigned.insert(job_id); + servers.insert( + server_id, + ServerDetails { + last_seen: Instant::now(), + last_error: None, + jobs_assigned, + jobs_unclaimed, + last_active_jobs: HashSet::new(), + num_cpus: 32, + server_nonce: ServerNonce::new(), + job_authorizer: Box::new(NoopAuthorizer), + }, + ); + let mut jobs = scheduler.jobs.lock().unwrap(); + jobs.insert( + job_id, + JobDetail { + server_id, + state: JobState::Pending, + since: old, + }, + ); + } + + scheduler + .handle_update_job_state(job_id, server_id, JobState::Ready) + .expect("Pending->Ready update should succeed"); + + let servers = scheduler.servers.lock().unwrap(); + let t = servers + .get(&server_id) + .unwrap() + .jobs_unclaimed + .get(&job_id) + .copied() + .expect("job must still be unclaimed after reaching Ready"); + assert!( + t.elapsed() < Duration::from_secs(60), + "the unclaimed clock must be reset to Ready time, not left at the ~120s-old allocation time" + ); + } + + // A scheduler restart must not remint job ids from 0: a new job N would then + // collide with an id a surviving build server still holds, tripping the + // assign-time overwrite (previously an assert-panic). + #[test] + fn test_job_id_base_seeded_and_counter_zero() { + let scheduler = Scheduler::new(); + assert!( + scheduler.job_id_base > 0, + "job_id_base must be seeded from a non-zero base so a restart does not reuse ids starting at 0" + ); + assert_eq!( + scheduler.job_count.load(Ordering::Relaxed), + 0, + "job_count is the 0-based allocated= counter and must start at 0" + ); + } + + // The upload transfer in handle_submit_toolchain streams to a temp file + // with the cache lock released; only the final graft reacquires the lock. + // Drive that graft path directly: a materialized temp file inserts under + // its toolchain key, and a file whose contents do not hash to the key is + // refused rather than served as a corrupt entry. + #[test] + fn test_submit_toolchain_no_lock_across_transfer() { + use sccache::util::Digest; + use std::io::Write; + + let cache_dir = tempfile::TempDir::new().unwrap(); + let mut cache = TcCache::new(cache_dir.path(), u64::MAX).unwrap(); + + // Materialize the upload exactly as the handler does after io::copy. + let mut tmpfile = tempfile::NamedTempFile::new_in(cache.path()).unwrap(); + tmpfile.write_all(b"toolchain-bytes").unwrap(); + tmpfile.flush().unwrap(); + let archive_id = Digest::reader_sync(std::fs::File::open(tmpfile.path()).unwrap()).unwrap(); + let tc = Toolchain { archive_id }; + let temp_path = tmpfile.into_temp_path(); + + assert!(!cache.contains_toolchain(&tc)); + cache.insert_at(&tc, &temp_path).unwrap(); + assert!(cache.contains_toolchain(&tc)); + + // A temp file that does not hash to the requested key is refused. + let mut bad = tempfile::NamedTempFile::new_in(cache.path()).unwrap(); + bad.write_all(b"not-the-right-bytes").unwrap(); + bad.flush().unwrap(); + let wrong_tc = Toolchain { + archive_id: "deadbeefcafe".to_owned(), + }; + let bad_path = bad.into_temp_path(); + assert!(cache.insert_at(&wrong_tc, &bad_path).is_err()); + } } diff --git a/src/compiler/c.rs b/src/compiler/c.rs index 7cf84b5059..f3a4c229d9 100644 --- a/src/compiler/c.rs +++ b/src/compiler/c.rs @@ -47,6 +47,34 @@ use crate::errors::*; use super::CacheControl; use super::preprocessor_cache::PreprocessorCacheEntry; +/// Live count of local preprocess (`cc1 -E`) invocations, gauged so the PC1 +/// preprocessing wall - the jobserver's cores saturated by preprocessing for +/// the whole cluster while remote nodes idle - is a measured number rather than +/// an inference. +#[cfg(feature = "dist-client")] +static PREPROC_INFLIGHT: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + +/// RAII counter for `PREPROC_INFLIGHT`: increments on `enter`, returning the +/// concurrency including this one, and decrements on drop so the count stays +/// correct across every exit of the preprocess future. +#[cfg(feature = "dist-client")] +struct PreprocInflightGuard; + +#[cfg(feature = "dist-client")] +impl PreprocInflightGuard { + fn enter() -> (Self, usize) { + let inflight = PREPROC_INFLIGHT.fetch_add(1, std::sync::atomic::Ordering::SeqCst) + 1; + (PreprocInflightGuard, inflight) + } +} + +#[cfg(feature = "dist-client")] +impl Drop for PreprocInflightGuard { + fn drop(&mut self) { + PREPROC_INFLIGHT.fetch_sub(1, std::sync::atomic::Ordering::SeqCst); + } +} + /// A generic implementation of the `Compiler` trait for C/C++ compilers. #[derive(Clone)] pub struct CCompiler @@ -137,6 +165,13 @@ struct CCompilation { is_locally_preprocessed: bool, #[cfg(feature = "dist-client")] preprocessed_input: Vec, + /// R0 instrumentation: wall-time spent running the local preprocessor + /// (`cc1 -E`) for this compile during hash-key generation, threaded into + /// the per-job dist log so the client-side preprocessing tax is timed + /// rather than inferred. `None` when no local preprocessing ran (input was + /// already preprocessed, or a preprocessor-cache hit skipped the run). + #[cfg(feature = "dist-client")] + preprocess_duration: Option, executable: PathBuf, compiler: I, cwd: PathBuf, @@ -307,6 +342,11 @@ impl Compiler for CCompiler { Box::new(CToolchainPackager { executable: self.executable.clone(), kind: self.compiler.kind(), + // `CCompiler` carries no compile environment; the standalone + // `--package-toolchain` path keeps resolving against the daemon's + // PATH. The dist-compile path threads the task env via + // `into_dist_packagers`. + env_vars: Vec::new(), }) } fn parse_arguments( @@ -459,6 +499,8 @@ where None }; + #[cfg(feature = "dist-client")] + let mut preprocess_elapsed: Option = None; let (preprocessor_output, include_files) = if needs_preprocessing { if let Some(preprocessor_key) = &preprocessor_key { if cache_control == CacheControl::Default { @@ -514,6 +556,8 @@ where #[cfg(feature = "dist-client")] preprocessed_input: PREPROCESSING_SKIPPED_COMPILE_POISON .to_vec(), + #[cfg(feature = "dist-client")] + preprocess_duration: None, executable: self.executable.clone(), compiler: self.compiler.to_owned(), cwd: cwd.clone(), @@ -529,6 +573,10 @@ where } } + #[cfg(feature = "dist-client")] + let preprocess_start = std::time::Instant::now(); + #[cfg(feature = "dist-client")] + let (preproc_guard, preproc_concurrency) = PreprocInflightGuard::enter(); let result = self .compiler .preprocess( @@ -542,6 +590,17 @@ where use_preprocessor_cache_mode, ) .await; + #[cfg(feature = "dist-client")] + { + let elapsed = preprocess_start.elapsed(); + preprocess_elapsed = Some(elapsed); + drop(preproc_guard); + info!( + "preprocess done in {}ms (concurrent {})", + elapsed.as_millis(), + preproc_concurrency + ); + } let out_pretty = self.parsed_args.output_pretty().into_owned(); let result = result.map_err(|e| { debug!("[{}]: preprocessor failed: {:?}", out_pretty, e); @@ -672,6 +731,8 @@ where is_locally_preprocessed: true, #[cfg(feature = "dist-client")] preprocessed_input: preprocessor_output, + #[cfg(feature = "dist-client")] + preprocess_duration: preprocess_elapsed, executable: self.executable.clone(), compiler: self.compiler.clone(), cwd, @@ -1217,6 +1278,7 @@ impl Compilation for CCompilation preprocessed_input, executable, compiler, + env_vars, .. } = *self; trace!("Dist inputs: {:?}", parsed_args.input); @@ -1232,6 +1294,7 @@ impl Compilation for CCompilation let toolchain_packager = Box::new(CToolchainPackager { executable, kind: compiler.kind(), + env_vars, }); let outputs_rewriter = Box::new(NoopOutputsRewriter); Ok((inputs_packager, toolchain_packager, outputs_rewriter)) @@ -1241,6 +1304,11 @@ impl Compilation for CCompilation self.is_locally_preprocessed } + #[cfg(feature = "dist-client")] + fn preprocess_duration(&self) -> Option { + self.preprocess_duration + } + fn outputs<'a>(&'a self) -> Box + 'a> { Box::new( self.parsed_args @@ -1283,10 +1351,14 @@ impl pkg::InputsPackager for CInputsPackager { format!("unable to transform input path {}", input_path.display()) })?; - let mut file_header = pkg::make_tar_header(&input_path, &dist_input_path)?; + let mut file_header = pkg::make_tar_header(&input_path)?; file_header.set_size(preprocessed_input.len() as u64); // The metadata is from non-preprocessed - file_header.set_cksum(); - builder.append(&file_header, preprocessed_input.as_slice())?; + pkg::append_tar_entry( + &mut builder, + &mut file_header, + &dist_input_path, + preprocessed_input.as_slice(), + )?; } for input_path in extra_hash_files.iter().chain(extra_dist_files.iter()) { @@ -1311,10 +1383,9 @@ impl pkg::InputsPackager for CInputsPackager { let mut output = vec![]; io::copy(&mut file, &mut output)?; - let mut file_header = pkg::make_tar_header(&input_path, &dist_input_path)?; + let mut file_header = pkg::make_tar_header(&input_path)?; file_header.set_size(output.len() as u64); - file_header.set_cksum(); - builder.append(&file_header, &*output)?; + pkg::append_tar_entry(&mut builder, &mut file_header, &dist_input_path, &*output)?; } // Finish archive @@ -1323,11 +1394,29 @@ impl pkg::InputsPackager for CInputsPackager { } } +/// Resolve a program path reported by `-print-prog-name`/`-print-file-name`. +/// Absolute paths are returned unchanged; a bare program name is resolved +/// against `path_env` (the compile task's PATH) when supplied, so an OE native +/// or cross gcc that reports a bare `as` packages the recipe's binutils rather +/// than the build host's. Falls back to the daemon's PATH when no compile PATH +/// is available. +#[cfg(feature = "dist-client")] +fn resolve_prog_in_path(raw: PathBuf, path_env: Option<&OsStr>) -> Option { + if raw.is_absolute() { + Some(raw) + } else if let Some(path) = path_env { + which::which_in(&raw, Some(path), std::env::current_dir().ok()?).ok() + } else { + which::which(raw).ok() + } +} + #[cfg(feature = "dist-client")] #[allow(unused)] struct CToolchainPackager { executable: PathBuf, kind: CCompilerKind, + env_vars: Vec<(OsString, OsString)>, } #[cfg(feature = "dist-client")] @@ -1344,11 +1433,21 @@ impl pkg::ToolchainPackager for CToolchainPackager { package_builder.add_common()?; package_builder.add_executable_and_deps(self.executable.clone())?; + // PATH from the compile task's environment. Resolving sub-tools against + // it (rather than the daemon's PATH) is what keeps an OE native or cross + // toolchain from packaging the build host's assembler. + let path_env = self + .env_vars + .iter() + .find(|(k, _)| k == OsStr::new("PATH")) + .map(|(_, v)| v.clone()); + // Helper to use -print-file-name and -print-prog-name to look up // files by path. let named_file = |kind: &str, name: &str| -> Option { let mut output = process::Command::new(&self.executable) .arg(format!("-print-{}-name={}", kind, name)) + .envs(self.env_vars.iter().cloned()) .output() .ok()?; debug!( @@ -1368,14 +1467,11 @@ impl pkg::ToolchainPackager for CToolchainPackager { output.stdout.pop(); } - // Create our PathBuf from the raw bytes. Assume that relative - // paths can be found via PATH. + // Create our PathBuf from the raw bytes. Relative paths are + // resolved against the compile task's PATH, falling back to the + // daemon's PATH when none was captured. let path: PathBuf = OsString::from_vec(output.stdout).into(); - if path.is_absolute() { - Some(path) - } else { - which::which(path).ok() - } + resolve_prog_in_path(path, path_env.as_deref()) }; // Helper to add a named file/program by to the package. @@ -1597,6 +1693,37 @@ mod test { use super::*; + #[test] + #[cfg(feature = "dist-client")] + fn test_resolve_prog_in_path_honors_supplied_path() { + use crate::test::utils::mk_bin; + let dir = tempfile::Builder::new() + .prefix("sccache_tc") + .tempdir() + .unwrap(); + let prog = mk_bin(dir.path(), "sccache-fake-as").unwrap(); + let path_env = std::env::join_paths([dir.path()]).unwrap(); + + // A bare program name resolves against the supplied compile PATH, not + // the daemon's PATH. This is the toolchain-packaging bug: an OE native + // or cross gcc reports a bare `as`, which must resolve to the recipe's + // binutils on the compile PATH, not the build host's /usr/bin/as. + let resolved = + resolve_prog_in_path(PathBuf::from("sccache-fake-as"), Some(path_env.as_os_str())); + assert_eq!( + resolved.map(|p| p.canonicalize().unwrap()), + Some(prog.clone()) + ); + + // Without the compile PATH, the daemon PATH cannot find this program. + let unresolved = resolve_prog_in_path(PathBuf::from("sccache-fake-as"), None); + assert_eq!(unresolved, None); + + // An absolute path passes through untouched. + let absolute = resolve_prog_in_path(prog.clone(), Some(path_env.as_os_str())); + assert_eq!(absolute, Some(prog)); + } + #[test] fn test_same_content() { let args = ovec!["a", "b", "c"]; diff --git a/src/compiler/compiler.rs b/src/compiler/compiler.rs index 78bb5a4332..b49fb36a2f 100644 --- a/src/compiler/compiler.rs +++ b/src/compiler/compiler.rs @@ -850,6 +850,34 @@ where .map(move |o| (cacheable, DistType::NoDist, o)) } +/// R0 instrumentation: number of compiles currently attempting distribution. +/// Read when a dist job starts so the log shows client-side supply pressure +/// (a low value while nodes sit idle points at job-supply starvation). +#[cfg(feature = "dist-client")] +static DIST_INFLIGHT: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + +/// RAII counter for `DIST_INFLIGHT`: increments on `enter` and decrements on +/// drop, so the count stays correct across every exit of the dist future - +/// success, `?`, `bail!`, or a dropped future - without a manual decrement in +/// each error arm and the local-fallback path. +#[cfg(feature = "dist-client")] +struct DistInflightGuard; + +#[cfg(feature = "dist-client")] +impl DistInflightGuard { + fn enter() -> (Self, usize) { + let inflight = DIST_INFLIGHT.fetch_add(1, std::sync::atomic::Ordering::SeqCst) + 1; + (DistInflightGuard, inflight) + } +} + +#[cfg(feature = "dist-client")] +impl Drop for DistInflightGuard { + fn drop(&mut self) { + DIST_INFLIGHT.fetch_sub(1, std::sync::atomic::Ordering::SeqCst); + } +} + #[cfg(feature = "dist-client")] async fn dist_or_local_compile( service: &server::SccacheService, @@ -877,7 +905,10 @@ where let dist_client = match dist_compile_cmd.clone().and(dist_client) { Some(dc) => dc, None => { - debug!("[{}]: Compiling locally", out_pretty); + info!( + "[{}]: Compiling locally (not eligible for distributed compilation)", + out_pretty + ); return compile_cmd .execute(service, &creator) .await @@ -885,13 +916,23 @@ where } }; - debug!("[{}]: Attempting distributed compilation", out_pretty); + info!("[{}]: Attempting distributed compilation", out_pretty); let out_pretty2 = out_pretty.clone(); let local_executable = compile_cmd.get_executable(); let local_executable2 = compile_cmd.get_executable(); let do_dist_compile = async move { + let (_inflight_guard, in_flight) = DistInflightGuard::enter(); + let dist_started = Instant::now(); + info!("[{}]: dist-job start (in_flight {})", out_pretty, in_flight); + // Local preprocessing (`cc1 -E`) already ran during hash-key generation, + // before this dist round-trip began; read its duration here so the tax + // that skews load onto the colocated node is timed in the per-job line. + let ms_preprocess = compilation + .preprocess_duration() + .map(|d| d.as_millis()) + .unwrap_or(0); let mut dist_compile_cmd = dist_compile_cmd.context("Could not create distributed compile command")?; debug!("[{}]: Creating distributed compile request", out_pretty); @@ -900,6 +941,15 @@ where .map(|output| path_transformer.as_dist_abs(&cwd.join(output.path))) .collect::>() .context("Failed to adapt an output path for distributed compile")?; + // The local paths every declared output must occupy once the compile + // finishes. Captured before `compilation` is moved into the packagers + // below so a remote compile that drops a declared output can be detected + // and salvaged (see the missing-output check after the run completes). + let expected_output_paths: Vec = compilation + .outputs() + .filter(|output| !output.optional) + .map(|output| cwd.join(output.path)) + .collect(); let (inputs_packager, toolchain_packager, outputs_rewriter) = compilation.into_dist_packagers(path_transformer)?; @@ -907,9 +957,11 @@ where "[{}]: Identifying dist toolchain for {:?}", out_pretty, local_executable ); + let t_put_toolchain = Instant::now(); let (dist_toolchain, maybe_dist_compile_executable) = dist_client .put_toolchain(local_executable, weak_toolchain_key, toolchain_packager) .await?; + let ms_put_toolchain = t_put_toolchain.elapsed().as_millis(); let mut tc_archive = None; if let Some((dist_compile_executable, archive_path)) = maybe_dist_compile_executable { dist_compile_cmd.executable = dist_compile_executable; @@ -917,7 +969,10 @@ where } debug!("[{}]: Requesting allocation", out_pretty); + let t_alloc = Instant::now(); let jares = dist_client.do_alloc_job(dist_toolchain.clone()).await?; + let ms_alloc = t_alloc.elapsed().as_millis(); + let mut ms_submit: u128 = 0; let job_alloc = match jares { dist::AllocJobResult::Success { job_alloc, @@ -928,11 +983,13 @@ where out_pretty, dist_toolchain.archive_id, job_alloc.job_id ); - match dist_client + let t_submit = Instant::now(); + let submit_res = dist_client .do_submit_toolchain(job_alloc.clone(), dist_toolchain) .await - .map_err(|e| e.context("Could not submit toolchain"))? - { + .map_err(|e| e.context("Could not submit toolchain"))?; + ms_submit = t_submit.elapsed().as_millis(); + match submit_res { dist::SubmitToolchainResult::Success => Ok(job_alloc), dist::SubmitToolchainResult::JobNotFound => { bail!("Job {} not found on server", job_alloc.job_id) @@ -954,6 +1011,7 @@ where let job_id = job_alloc.job_id; let server_id = job_alloc.server_id; debug!("[{}]: Running job", out_pretty); + let t_run = Instant::now(); let ((job_id, server_id), (jres, path_transformer)) = dist_client .do_run_job( job_alloc, @@ -970,7 +1028,7 @@ where ) })?; - let mut jc = match jres { + let jc = match jres { dist::RunJobResult::Complete(jc) => jc, dist::RunJobResult::JobNotFound => bail!("Job {} not found on server", job_id), }; @@ -1036,14 +1094,64 @@ where ); if jc.output.code != 0 { - // Add server info to help diagnose host-specific failures, e.g. due to flaky hardware. - // Failed builds are not cached so this tampering should not cause too much trouble. - let server_info = format!("sccache: Job failed on server {}:\n", server_id.addr()); - jc.output - .stderr - .splice(0..0, server_info.as_bytes().to_vec()); + // The one-line reason above is logged by the caller at warn; the + // remote compiler's own stdout/stderr is the only place the actual + // failure (a missing target std, an unshipped input) is visible, and + // the local fallback discards it. Emit it at debug so SCCACHE_LOG=debug + // surfaces the remote error without a rebuild, while a normal + // SCCACHE_LOG=info run stays quiet. + debug!( + "[{}]: distributed compile on {} exited {}; remote stderr:\n{}\nremote stdout:\n{}", + out_pretty, + server_id.addr(), + jc.output.code, + String::from_utf8_lossy(&jc.output.stderr), + String::from_utf8_lossy(&jc.output.stdout), + ); + // A non-zero remote result is frequently a distribution artifact + // rather than a genuine compiler error: e.g. an object that + // .incbin's a binary the inputs packager did not ship (the kernel's + // vdso/dtb/embedded-config wrappers), which the build-server cannot + // assemble. Fall back to a local recompile via the or_else below - it + // either succeeds (confirming a dist-only artifact) or reproduces the + // real error locally, so a remote failure never breaks a build that + // would compile fine locally. This only affects failing dist + // compiles; successful ones are returned unchanged above. + bail!( + "distributed compile on {} returned exit code {}; recompiling locally", + server_id.addr(), + jc.output.code + ); + } + + // The remote compile returned exit 0 but may have dropped a declared + // output: glibc's ldconfig.o/sprof.o omit their `.o.dt` dep file, which + // the build-server does not return. Zipping the outputs later would then + // fail fatally with no recourse. Treat a missing declared output as a + // distribution artifact and fall back to a local recompile via the + // or_else below, which reproduces the full output set. Compiles whose + // dist output set is complete are unaffected. + if let Some(missing) = expected_output_paths.iter().find(|p| !p.exists()) { + bail!( + "distributed compile on {} did not return expected output {}; recompiling locally", + server_id.addr(), + missing.display() + ); } + let ms_run_fetch = t_run.elapsed().as_millis(); + info!( + "[{}]: dist-job done on {} in {}ms (preprocess {}ms, put_tc {}ms, alloc {}ms, submit {}ms, run+fetch {}ms, in_flight {})", + out_pretty, + server_id.addr(), + dist_started.elapsed().as_millis(), + ms_preprocess, + ms_put_toolchain, + ms_alloc, + ms_submit, + ms_run_fetch, + in_flight, + ); Ok((DistType::Ok(server_id), jc.output.into())) }; @@ -1112,6 +1220,17 @@ where true } + /// R0 instrumentation: wall-time the client spent locally preprocessing + /// this compile's source (`cc1 -E`) during hash-key generation, when that + /// step ran. Threaded into the per-job dist log so the preprocessing tax is + /// timed rather than inferred. `None` when no local preprocessing ran + /// (input already preprocessed, or a preprocessor-cache hit skipped it), or + /// for compilers whose hash step does no local preprocessing. + #[cfg(feature = "dist-client")] + fn preprocess_duration(&self) -> Option { + None + } + /// Returns an iterator over the results of this compilation. /// /// Each item is a descriptive (and unique) name of the output paired with @@ -3209,6 +3328,7 @@ LLVM version: 6.0", test_dist::ErrorAllocJobClient::new(), test_dist::ErrorSubmitToolchainClient::new(), test_dist::ErrorRunJobClient::new(), + test_dist::IncompleteOutputsClient::new(), ]; // Write a dummy input file so the preprocessor cache mode can work std::fs::write(f.tempdir.path().join("foo.c"), "whatever").unwrap(); @@ -3677,4 +3797,112 @@ mod test_dist { None } } + + /// A dist client whose remote compile succeeds (exit 0) but whose returned + /// output set drops a declared output - the glibc `.o.dt` failure mode, + /// where the build-server runs the compile fine yet does not return every + /// declared output. The client must detect the missing output and fall back + /// to a local recompile rather than failing the build. + pub struct IncompleteOutputsClient { + has_started: AtomicBool, + tc: Toolchain, + output: ProcessOutput, + } + + impl IncompleteOutputsClient { + #[allow(clippy::new_ret_no_self)] + pub fn new() -> Arc { + Arc::new(Self { + has_started: AtomicBool::default(), + tc: Toolchain { + archive_id: "somearchiveid".to_owned(), + }, + output: ProcessOutput::fake_output(0, vec![], vec![]), + }) + } + } + + #[async_trait] + impl dist::Client for IncompleteOutputsClient { + async fn do_alloc_job(&self, tc: Toolchain) -> Result { + assert!( + !self + .has_started + .swap(true, std::sync::atomic::Ordering::AcqRel) + ); + assert_eq!(self.tc, tc); + + Ok(AllocJobResult::Success { + job_alloc: JobAlloc { + auth: "abcd".to_owned(), + job_id: JobId(0), + server_id: ServerId::new(([0, 0, 0, 0], 1).into()), + }, + need_toolchain: true, + }) + } + async fn do_get_status(&self) -> Result { + unreachable!("fn do_get_status is not used for this test. qed") + } + async fn do_submit_toolchain( + &self, + job_alloc: JobAlloc, + tc: Toolchain, + ) -> Result { + assert_eq!(job_alloc.job_id, JobId(0)); + assert_eq!(self.tc, tc); + + Ok(SubmitToolchainResult::Success) + } + async fn do_run_job( + &self, + job_alloc: JobAlloc, + command: CompileCommand, + outputs: Vec, + inputs_packager: Box, + ) -> Result<(RunJobResult, PathTransformer)> { + assert_eq!(job_alloc.job_id, JobId(0)); + assert_eq!(command.executable, "/overridden/compiler"); + + let mut inputs = vec![]; + let path_transformer = inputs_packager.write_inputs(&mut inputs).unwrap(); + // Drop one declared output to mimic a build-server that returned a + // successful compile with an incomplete output set. + let mut outputs = outputs; + outputs.pop(); + let outputs = outputs + .into_iter() + .map(|name| { + let data = format!("some data in {}", name); + let data = OutputData::try_from_reader(data.as_bytes()).unwrap(); + (name, data) + }) + .collect(); + let result = RunJobResult::Complete(JobComplete { + output: self.output.clone(), + outputs, + }); + Ok((result, path_transformer)) + } + async fn put_toolchain( + &self, + _: PathBuf, + _: String, + _: Box, + ) -> Result<(Toolchain, Option<(String, PathBuf)>)> { + Ok(( + self.tc.clone(), + Some(( + "/overridden/compiler".to_owned(), + PathBuf::from("somearchiveid"), + )), + )) + } + fn rewrite_includes_only(&self) -> bool { + false + } + fn get_custom_toolchain(&self, _exe: &Path) -> Option { + None + } + } } diff --git a/src/compiler/gcc.rs b/src/compiler/gcc.rs index 84ddbd3c96..af3d8ce35a 100644 --- a/src/compiler/gcc.rs +++ b/src/compiler/gcc.rs @@ -654,6 +654,15 @@ where // We can't cache compilation without an input. None => cannot_cache!("no input file"), }; + // Compiler feature-probes compile /dev/null (e.g. OpenSSL's + // `-Wa,--help ... -x assembler /dev/null`, which asks the assembler to + // print its help). They never produce a real object, so caching aborts + // when the missing output can't be zipped and swallows the probe's stdout, + // making the caller mis-detect the feature. Refuse to cache so sccache runs + // the compiler locally and the probe's output reaches the caller. + if Path::new(&input) == Path::new("/dev/null") { + cannot_cache!("/dev/null input"); + } let language = match language { None => { let mut lang = Language::from_file_name(Path::new(&input)); @@ -927,6 +936,37 @@ where run_input_output(cmd, None).await } +/// Autoconf and CMake configure-time feature probes must run locally, not +/// distributed. autoconf compiles `conftest.`; CMake's `try_compile()` +/// compiles generated sources inside a `CMakeScratch`/`CMakeTmp`/`TryCompile-*` +/// scratch directory. These probes are tiny, run by the hundreds during +/// `do_configure`, and are frequently *designed* to fail (detecting the absence +/// of a feature). Distributing them ships a network round-trip per probe and, +/// for the intentionally-failing ones, a dropped-output fallback - hundreds of +/// wasted dispatches contending with real compiles for build-server slots, for +/// no benefit: a probe's pass/fail result is identical whether it compiles +/// locally or remotely, and a sub-second local compile beats the round-trip. +/// distcc/icecc tolerate this only because their dispatch is cheaper. Detection +/// keys on the autoconf input name and the CMake scratch directory rather than +/// the generated probe source names (CheckSymbolExists.c, src.c, ...), which are +/// not stable across CMake versions. A false positive is harmless: the compile +/// merely runs locally (still cached), exactly as it would for any non-eligible +/// recipe. +#[cfg(feature = "dist-client")] +fn is_configure_probe(input: &Path, cwd: &Path) -> bool { + if input + .file_stem() + .and_then(|stem| stem.to_str()) + .is_some_and(|stem| stem == "conftest") + { + return true; + } + cwd.components().any(|component| { + let part = component.as_os_str().to_str().unwrap_or_default(); + part == "CMakeScratch" || part == "CMakeTmp" || part.starts_with("TryCompile-") + }) +} + #[allow(clippy::too_many_arguments)] pub fn generate_compile_commands( path_transformer: &mut dist::PathTransformer, @@ -1008,7 +1048,23 @@ where // output is parsed by tools like CMake and must reflect the local toolchain // 2. ClangCUDA cannot be dist-compiled because Clang has separate host and // device preprocessor outputs and cannot compile preprocessed CUDA files. - let dist_command = if has_verbose_flag || parsed_args.language == Language::Cuda { + // 3. Precompiled-header generation (the input is itself a header, e.g. + // `gcc -c asmjit.hpp -o asmjit.hpp.gch`) must run locally. A .gch built + // from preprocessed source on a remote builder captures a macro state + // (notably __has_builtin, evaluated differently under -fpreprocessed) + // that diverges from a native build and then breaks every consumer that + // -includes the header. distcc refuses PCH for the same reason. + // 4. Autoconf/CMake configure feature-probes (conftest.c, CMake try_compile + // sources under a CMakeScratch/CMakeTmp/TryCompile-* dir) run locally: + // they are tiny, run by the hundreds during do_configure, and are often + // designed to fail, so distributing them only wastes build-server slots + // on round-trips whose result is identical to a local compile. See + // is_configure_probe. + let dist_command = if has_verbose_flag + || parsed_args.language == Language::Cuda + || parsed_args.language.is_c_like_header() + || is_configure_probe(&parsed_args.input, cwd) + { None } else { (|| { @@ -1029,7 +1085,14 @@ where } arguments.extend(vec![ parsed_args.compilation_flag.clone().into_string().ok()?, - path_transformer.as_dist(&parsed_args.input)?, + // Match the path the inputs packager ships the preprocessed + // content at (CInputsPackager uses cwd.join(input) + simplify_path). + // parsed_args.input is relative for out-of-tree builds (e.g. OE's + // `../sources/foo.c`); passing it raw made the server look for the + // input at a path the package never placed it, failing with + // "No such file or directory". Absolutize+simplify so both agree. + path_transformer + .as_dist(&dist::pkg::simplify_path(&cwd.join(&parsed_args.input)).ok()?)?, "-o".into(), path_transformer.as_dist(out_file)?, ]); @@ -1445,6 +1508,29 @@ mod test { assert!(preprocessor_args.is_empty()); } + #[test] + fn test_parse_arguments_dev_null_input_not_cached() { + // OpenSSL's assembler feature-probe compiles /dev/null: + // gcc -Wa,--help -c -o null.o -x assembler /dev/null + // -Wa,--help makes the assembler print its help and exit without an + // object, so caching aborts when the missing output can't be zipped and + // swallows the probe's stdout - the caller then mis-detects + // --noexecstack support and emits exec-stack objects. Run it locally. + let args = stringvec![ + "-Wa,--help", + "-c", + "-o", + "null.o", + "-x", + "assembler", + "/dev/null" + ]; + assert_eq!( + CompilerArguments::CannotCache("/dev/null input", None), + parse_arguments_(args, false) + ); + } + #[test] fn test_parse_arguments_depfile_for_preprocessed_c_clang() { let args = stringvec!["-c", "foo.i", "-o", "foo.o", "-MD", "-MF", "foo.d"]; @@ -2486,6 +2572,66 @@ mod test { assert_eq!(0, creator.lock().unwrap().children.len()); } + #[test] + #[cfg(feature = "dist-client")] + fn test_compile_relative_input_dist_command_is_absolute() { + // Regression: the dist command must reference the same absolute, simplified + // input path the inputs packager ships the preprocessed content at + // (CInputsPackager uses cwd.join(input) + simplify_path). For out-of-tree + // builds the input is relative (e.g. OE's `../sources/foo.c`); passing it + // raw made the build-server look where the input was never placed, failing + // with "No such file or directory" (OE xz/glibc out-of-tree compiles). + let f = TestFixture::new(); + let parsed_args = ParsedArguments { + input: "../foo.c".into(), + double_dash_input: false, + language: Language::C, + compilation_flag: "-c".into(), + depfile: None, + outputs: vec![( + "obj", + ArtifactDescriptor { + path: "foo.o".into(), + optional: false, + }, + )] + .into_iter() + .collect(), + dependency_args: vec![], + preprocessor_args: vec![], + common_args: vec![], + arch_args: vec![], + unhashed_args: vec![], + extra_dist_files: vec![], + extra_hash_files: vec![], + msvc_show_includes: false, + profile_generate: false, + color_mode: ColorMode::Auto, + suppress_rewrite_includes_only: false, + too_hard_for_preprocessor_cache_mode: None, + }; + let mut path_transformer = dist::PathTransformer::new(); + let (_command, dist_command, _cacheable) = generate_compile_commands( + &mut path_transformer, + &f.bins[0], + &parsed_args, + f.tempdir.path(), + &[], + CCompilerKind::Gcc, + false, + language_to_gcc_arg, + ) + .unwrap(); + let dist_command = dist_command.expect("relative input must still produce a dist command"); + // No argument may carry an unresolved `..`: the input must be the simplified + // absolute path the packager ships, not the raw relative one. + assert!( + !dist_command.arguments.iter().any(|a| a.contains("..")), + "dist command still references a relative input: {:?}", + dist_command.arguments + ); + } + #[test] fn test_compile_simple_verbose_short() { let creator = new_creator(); @@ -2604,6 +2750,159 @@ mod test { assert_eq!(0, creator.lock().unwrap().children.len()); } + #[test] + fn test_compile_pch_generation_never_dist() { + // Compiling a header (precompiled-header generation, e.g. + // `gcc -c asmjit.hpp -o asmjit.hpp.gch`) must never be distributed: a + // .gch built from preprocessed source / a remote toolchain captures a + // macro state (notably __has_builtin) that differs from a local build + // and then poisons every consumer that -includes the header. distcc + // refuses PCH for the same reason; we build it locally. + let creator = new_creator(); + let f = TestFixture::new(); + let parsed_args = ParsedArguments { + input: "asmjit.hpp".into(), + double_dash_input: false, + language: Language::CxxHeader, + compilation_flag: "-c".into(), + depfile: None, + outputs: vec![( + "obj", + ArtifactDescriptor { + path: "asmjit.hpp.gch".into(), + optional: false, + }, + )] + .into_iter() + .collect(), + dependency_args: vec![], + preprocessor_args: vec![], + common_args: vec![], + arch_args: vec![], + unhashed_args: vec![], + extra_dist_files: vec![], + extra_hash_files: vec![], + msvc_show_includes: false, + profile_generate: false, + color_mode: ColorMode::Auto, + suppress_rewrite_includes_only: false, + too_hard_for_preprocessor_cache_mode: None, + }; + let runtime = single_threaded_runtime(); + let storage = MockStorage::new(None, false); + let storage: std::sync::Arc = std::sync::Arc::new(storage); + let service = server::SccacheService::mock_with_storage(storage, runtime.handle().clone()); + let compiler = &f.bins[0]; + // Compiler invocation. + next_command(&creator, Ok(MockChild::new(exit_status(0), "", ""))); + let mut path_transformer = dist::PathTransformer::new(); + let (command, dist_command, cacheable) = generate_compile_commands( + &mut path_transformer, + compiler, + &parsed_args, + f.tempdir.path(), + &[], + CCompilerKind::Gcc, + // rewrite_includes_only=true: dist is enabled, yet a header still + // must not produce a dist command. + true, + language_to_gcc_arg, + ) + .unwrap(); + assert!(dist_command.is_none()); + let _ = command.execute(&service, &creator).wait(); + assert_eq!(Cacheable::Yes, cacheable); + assert_eq!(0, creator.lock().unwrap().children.len()); + } + + #[test] + fn test_compile_conftest_never_dist() { + // Autoconf compiles `conftest.c` to probe for a feature; the probe is + // tiny and often designed to fail. It must run locally, never + // distributed, regardless of rewrite_includes_only (dist enabled). + let creator = new_creator(); + let f = TestFixture::new(); + let parsed_args = ParsedArguments { + input: "conftest.c".into(), + double_dash_input: false, + language: Language::C, + compilation_flag: "-c".into(), + depfile: None, + outputs: vec![( + "obj", + ArtifactDescriptor { + path: "conftest.o".into(), + optional: false, + }, + )] + .into_iter() + .collect(), + dependency_args: vec![], + preprocessor_args: vec![], + common_args: vec![], + arch_args: vec![], + unhashed_args: vec![], + extra_dist_files: vec![], + extra_hash_files: vec![], + msvc_show_includes: false, + profile_generate: false, + color_mode: ColorMode::Auto, + suppress_rewrite_includes_only: false, + too_hard_for_preprocessor_cache_mode: None, + }; + let runtime = single_threaded_runtime(); + let storage = MockStorage::new(None, false); + let storage: std::sync::Arc = std::sync::Arc::new(storage); + let service = server::SccacheService::mock_with_storage(storage, runtime.handle().clone()); + let compiler = &f.bins[0]; + next_command(&creator, Ok(MockChild::new(exit_status(0), "", ""))); + let mut path_transformer = dist::PathTransformer::new(); + let (command, dist_command, cacheable) = generate_compile_commands( + &mut path_transformer, + compiler, + &parsed_args, + f.tempdir.path(), + &[], + CCompilerKind::Gcc, + true, + language_to_gcc_arg, + ) + .unwrap(); + assert!(dist_command.is_none()); + let _ = command.execute(&service, &creator).wait(); + assert_eq!(Cacheable::Yes, cacheable); + assert_eq!(0, creator.lock().unwrap().children.len()); + } + + #[test] + #[cfg(feature = "dist-client")] + fn test_is_configure_probe() { + use std::path::Path; + // autoconf: conftest. in any directory. + assert!(is_configure_probe( + Path::new("conftest.c"), + Path::new("/build/foo/1.0/foo-1.0") + )); + assert!(is_configure_probe( + Path::new("conftest.cpp"), + Path::new("/build/foo/1.0/foo-1.0") + )); + // CMake try_compile: generated source inside a scratch directory. + assert!(is_configure_probe( + Path::new("CheckSymbolExists.c"), + Path::new("/build/foo/1.0/build/CMakeFiles/CMakeScratch/TryCompile-aB3xZ9") + )); + assert!(is_configure_probe( + Path::new("src.c"), + Path::new("/build/foo/1.0/build/CMakeTmp") + )); + // A real translation unit in a normal build directory must distribute. + assert!(!is_configure_probe( + Path::new("list.c"), + Path::new("/build/foo/1.0/foo-1.0/src") + )); + } + #[test] fn test_compile_double_dash_input() { let args = stringvec!["-c", "-o", "foo.o", "--", "foo.c"]; diff --git a/src/compiler/nvcc.rs b/src/compiler/nvcc.rs index e6f72a7bf0..8ec211cb79 100644 --- a/src/compiler/nvcc.rs +++ b/src/compiler/nvcc.rs @@ -1183,7 +1183,7 @@ fn remap_generated_filenames( // Don't use the count as the first character of the file name, because the file name // may be used as an identifier (via the __FILE__ macro) and identifiers with leading // digits are not valid in C/C++, i.e. `x_0.cudafe1.cpp` instead of `0.cudafe1.cpp`. - .join("x_".to_owned() + &count + extension) + .join("x_".to_owned() + count.as_str() + extension) .to_string_lossy() .to_string() }) diff --git a/src/compiler/rust.rs b/src/compiler/rust.rs index d6c2c71927..7d9493f9e3 100644 --- a/src/compiler/rust.rs +++ b/src/compiler/rust.rs @@ -534,9 +534,9 @@ where &self, arguments: &[OsString], cwd: &Path, - _env_vars: &[(OsString, OsString)], + env_vars: &[(OsString, OsString)], ) -> CompilerArguments + 'static>> { - match parse_arguments(arguments, cwd) { + match parse_arguments(arguments, cwd, env_vars) { CompilerArguments::Ok(args) => CompilerArguments::Ok(Box::new(RustHasher { executable: self.executable.clone(), // if rustup exists, this must already contain the true resolved compiler path host: self.host.clone(), @@ -983,6 +983,23 @@ impl FromArg for ArgTarget { )) } } +/// Resolve a bare `--target ` against `RUST_TARGET_PATH` to the custom +/// target-spec JSON it refers to, if one exists. +/// +/// rustc resolves a bare target name by searching each `RUST_TARGET_PATH` +/// directory for `.json` (built-in targets have no file and are left +/// alone). sccache-dist ships a target spec only when `--target` is a path, so +/// a name-form custom target is invisible to the build server and the remote +/// rustc aborts with "could not find specification for target". Resolving the +/// name to its JSON path here lets the existing path-form machinery hash and +/// ship the spec. `env_vars` is the compile's own environment, not the daemon's. +fn resolve_target_json_in_path(name: &str, env_vars: &[(OsString, OsString)]) -> Option { + let (_, rust_target_path) = env_vars.iter().find(|(k, _)| k == "RUST_TARGET_PATH")?; + std::env::split_paths(rust_target_path) + .map(|dir| dir.join(format!("{name}.json"))) + .find(|candidate| candidate.is_file()) +} + impl IntoArg for ArgTarget { fn into_arg_os_string(self) -> OsString { match self { @@ -1065,7 +1082,11 @@ counted_array!(static ARGS: [ArgInfo; _] = [ take_arg!("-o", PathBuf, CanBeSeparated, TooHardPath), ]); -fn parse_arguments(arguments: &[OsString], cwd: &Path) -> CompilerArguments { +fn parse_arguments( + arguments: &[OsString], + cwd: &Path, + env_vars: &[(OsString, OsString)], +) -> CompilerArguments { let mut args = vec![]; let mut emit: Option> = None; @@ -1174,7 +1195,15 @@ fn parse_arguments(arguments: &[OsString], cwd: &Path) -> CompilerArguments match target { ArgTarget::Path(json_path) => target_json = Some(json_path.to_owned()), ArgTarget::Unsure(_) => cannot_cache!("target unsure"), - ArgTarget::Name(_) => (), + ArgTarget::Name(name) => { + // Keep the name form (rustc records it as the target's + // identity, matching the prebuilt std in the sysroot); we + // ship the spec and repoint RUST_TARGET_PATH for the remote + // instead. Resolving here lets the spec be hashed and shipped. + if let Some(json_path) = resolve_target_json_in_path(name, env_vars) { + target_json = Some(json_path); + } + } }, None => { match arg { @@ -1622,7 +1651,13 @@ where dep_info.to_string_lossy().into_owned(), ArtifactDescriptor { path: p.clone(), - optional: false, + // The build server does not reliably return the rust + // dep-info (.d) file, and it only feeds cargo's rebuild + // tracking (a bitbake do_compile runs cargo from scratch and + // never consumes it). Mark it optional so a distributed + // compile that omits it is not discarded and recompiled + // locally - which stranded every rust compile on the client. + optional: true, }, ); Some(p) @@ -1663,6 +1698,9 @@ where .into_iter() .chain(abs_externs) .chain(abs_staticlibs) + // Ship the custom target-spec JSON (if any) so a remote rustc can + // load it; without it a distributed compile fails to find the target. + .chain(self.parsed_args.target_json.iter().map(|p| cwd.join(p))) .collect(); Ok(HashResult { @@ -1809,6 +1847,18 @@ impl Compilation for RustCompilation { "CARGO" | "CARGO_MANIFEST_DIR" => { *v = path_transformer.as_dist(Path::new(v))?; } + "RUST_TARGET_PATH" => { + // Repoint the target-spec search path at the shipped + // copies so the remote rustc resolves a name-form + // --target the same way the client did. The spec JSON is + // shipped as a compile input above. The server is always + // Linux, so the entries join with ':'. + let mut dist_dirs = Vec::new(); + for dir in std::env::split_paths(v) { + dist_dirs.push(path_transformer.as_dist(&dir)?); + } + *v = dist_dirs.join(":"); + } _ => (), } } @@ -1931,6 +1981,81 @@ fn can_trim_this(input_path: &Path) -> bool { && !ar_path.exists() } +/// Return a metadata-only archive for a trimmable rlib: the single-member `ar` +/// holding its `rust.metadata.bin`, or `None` when the rlib has no embedded +/// metadata member. +/// +/// A rustc rlib normally carries its metadata as a `rust.metadata.bin` ar +/// member, so a dependency only inspected for metadata can ship trimmed. But a +/// split-metadata sysroot (e.g. an OpenEmbedded target `rustlib//lib`, +/// whose std/core rlibs keep metadata in a sibling `.rmeta` and carry only +/// object members) has no such member. Callers must ship the whole rlib in +/// that case -- trimming it to nothing drops the crate and leaves a remote +/// compile unable to find `std`. +#[cfg(feature = "dist-client")] +fn trimmed_rlib_metadata(input_path: &Path) -> Result>> { + let file = fs::File::open(input_path)?; + let mut archive = ar::Archive::new(file); + while let Some(entry_result) = archive.next_entry() { + let mut entry = entry_result?; + if entry.header().identifier() != b"rust.metadata.bin" { + continue; + } + let mut metadata_ar = vec![]; + { + let mut ar_builder = ar::Builder::new(&mut metadata_ar); + let header = entry.header().clone(); + ar_builder.append(&header, &mut entry)?; + } + return Ok(Some(metadata_ar)); + } + Ok(None) +} + +#[test] +#[cfg(feature = "dist-client")] +fn test_trimmed_rlib_metadata() { + let tmp = tempfile::Builder::new() + .prefix("sccache_trim") + .tempdir() + .unwrap(); + + // A split-metadata rlib (object members only, no rust.metadata.bin), as an + // OE target sysroot ships: must not trim to nothing -> None (ship whole). + let split = tmp.path().join("libcore-split.rlib"); + { + let mut b = ar::Builder::new(fs::File::create(&split).unwrap()); + b.append(&ar::Header::new(b"foo.o".to_vec(), 3), &b"abc"[..]) + .unwrap(); + } + assert!( + trimmed_rlib_metadata(&split).unwrap().is_none(), + "an rlib without rust.metadata.bin must ship whole, not trim to nothing" + ); + + // A normal rlib with an embedded rust.metadata.bin -> trim to that member. + let embedded = tmp.path().join("libfoo-embedded.rlib"); + { + let mut b = ar::Builder::new(fs::File::create(&embedded).unwrap()); + b.append( + &ar::Header::new(b"rust.metadata.bin".to_vec(), 4), + &b"META"[..], + ) + .unwrap(); + b.append(&ar::Header::new(b"foo.o".to_vec(), 3), &b"abc"[..]) + .unwrap(); + } + let trimmed = trimmed_rlib_metadata(&embedded).unwrap(); + assert!( + trimmed.is_some(), + "an rlib with metadata should trim to Some" + ); + assert!( + trimmed.unwrap().windows(4).any(|w| w == b"META"), + "the trimmed archive must contain the metadata member" + ); +} + #[test] #[cfg(feature = "dist-client")] fn test_can_trim_this() { @@ -2009,6 +2134,24 @@ fn test_maybe_add_cargo_toml() { assert!(maybe_add_cargo_toml(&wgpu_lib, true).is_none()); } +/// Is `dir` a rustc target sysroot lib dir (`/lib/rustlib//lib`)? +/// +/// Such a dir holds the implicit sysroot crates (core, std, alloc, ...) that +/// rustc injects but that never appear as `--extern` deps, so the crate-dep +/// filter in `write_inputs` drops them. A cross-compile references the target's +/// prebuilt std here via `-L`; the remote rustc needs those rlibs, so libs from +/// this dir shape are shipped unconditionally. Anchored on the `rustlib/*/lib` +/// tail so an ordinary `-L dependency=target/*/deps` dir still gets filtered. +#[cfg(feature = "dist-client")] +fn is_rustlib_target_libdir(dir: &Path) -> bool { + dir.file_name().is_some_and(|n| n == "lib") + && dir + .parent() + .and_then(Path::parent) + .and_then(Path::file_name) + .is_some_and(|n| n == "rustlib") +} + #[cfg(feature = "dist-client")] impl pkg::InputsPackager for RustInputsPackager { #[allow(clippy::cognitive_complexity)] // TODO simplify this method. @@ -2090,6 +2233,9 @@ impl pkg::InputsPackager for RustInputsPackager { let mut tar_crate_libs = vec![]; for crate_link_path in crate_link_paths { let crate_link_path = pkg::simplify_path(&crate_link_path)?; + // Ship every rlib from a target sysroot lib dir; its implicit + // sysroot crates are absent from the cargo dep-name set below. + let ship_all = is_rustlib_target_libdir(&crate_link_path); let dir_entries = match fs::read_dir(crate_link_path) { Ok(iter) => iter, Err(e) if e.kind() == io::ErrorKind::NotFound => continue, @@ -2108,13 +2254,15 @@ impl pkg::InputsPackager for RustInputsPackager { Some(name) => { let mut rev_name_split = name.rsplitn(2, '-'); let _extra_filename_and_ext = rev_name_split.next(); - let libname = if let Some(libname) = rev_name_split.next() { + if let Some(libname) = rev_name_split.next() { + assert!(rev_name_split.next().is_none()); libname } else { - continue; - }; - assert!(rev_name_split.next().is_none()); - libname + // No `-` suffix (e.g. an OE target sysroot's + // libstd.rlib): fall back to the name with its + // extension stripped so std still ships. + name.rsplit_once('.').map(|(stem, _)| stem).unwrap_or(name) + } } None => continue, }; @@ -2131,8 +2279,9 @@ impl pkg::InputsPackager for RustInputsPackager { _ => continue, }; if let Some((_, ref dep_crate_names)) = rlib_dep_reader_and_names { - // We have a list of crate names we care about, see if this lib is a candidate - if !dep_crate_names.contains(crate_name) { + // We have a list of crate names we care about, see if this lib is a candidate. + // A target rustlib dir is exempt: its sysroot crates never appear as deps. + if !ship_all && !dep_crate_names.contains(crate_name) { continue; } } @@ -2172,30 +2321,26 @@ impl pkg::InputsPackager for RustInputsPackager { let mut builder = tar::Builder::new(wtr); for (input_path, dist_input_path) in all_tar_inputs.iter() { - let mut file_header = pkg::make_tar_header(input_path, dist_input_path)?; - let file = fs::File::open(input_path)?; - if can_trim_rlibs && can_trim_this(input_path) { - let mut archive = ar::Archive::new(file); - - while let Some(entry_result) = archive.next_entry() { - let mut entry = entry_result?; - if entry.header().identifier() != b"rust.metadata.bin" { - continue; - } - let mut metadata_ar = vec![]; - { - let mut ar_builder = ar::Builder::new(&mut metadata_ar); - let header = entry.header().clone(); - ar_builder.append(&header, &mut entry)?; - } - file_header.set_size(metadata_ar.len() as u64); - file_header.set_cksum(); - builder.append(&file_header, metadata_ar.as_slice())?; - break; - } + let mut file_header = pkg::make_tar_header(input_path)?; + let trimmed = if can_trim_rlibs && can_trim_this(input_path) { + trimmed_rlib_metadata(input_path)? + } else { + None + }; + if let Some(metadata_ar) = trimmed { + file_header.set_size(metadata_ar.len() as u64); + pkg::append_tar_entry( + &mut builder, + &mut file_header, + dist_input_path, + metadata_ar.as_slice(), + )?; } else { - file_header.set_cksum(); - builder.append(&file_header, file)?; + // Either not trimmable, or a split-metadata rlib with no + // embedded metadata member -- ship the whole file so the remote + // gets the real crate (e.g. an OE target sysroot's std/core). + let file = fs::File::open(input_path)?; + pkg::append_tar_entry(&mut builder, &mut file_header, dist_input_path, file)?; } } @@ -2262,7 +2407,7 @@ impl OutputsRewriter for RustOutputsRewriter { if let Some(dep_info) = self.dep_info { let extra_input_str = extra_inputs .iter() - .fold(String::new(), |s, p| s + " " + &p.to_string_lossy()); + .fold(String::new(), |s, p| s + " " + p.to_string_lossy().as_ref()); for dep_info_local_path in output_paths { trace!("Comparing with {}", dep_info_local_path.display()); if dep_info == *dep_info_local_path { @@ -2281,7 +2426,7 @@ impl OutputsRewriter for RustOutputsRewriter { local_path.display() ) })?; - error!( + trace!( "RE replacing {} with {} in {}", re_str, local_path_str, deps ); @@ -2298,13 +2443,52 @@ impl OutputsRewriter for RustOutputsRewriter { return Ok(()); } } - // We expected there to be dep info, but none of the outputs matched - bail!("No outputs matched dep info file {}", dep_info.display()); + // The compile succeeded on the build server, but none of the + // outputs it returned matched the expected dep-info (.d) path. Do + // NOT fail the whole distributed compile over the dep file: keep the + // object/rlib the server produced and skip the dep-info path + // rewrite, instead of discarding a good remote compile and forcing a + // local recompile - which stranded every distributed rust compile + // back on the client. The dep file only feeds rebuild tracking, and + // a bitbake do_compile runs cargo from scratch, so a missing or + // unremapped .d is harmless here. + warn!( + "distributed compile did not return the expected dep info file {}; \ + keeping the remote outputs and skipping the dep-info rewrite", + dep_info.display() + ); } Ok(()) } } +// A successful distributed compile whose returned outputs do not include the +// expected dep-info file must NOT be failed - the object/rlib is good, and +// discarding it forces a local recompile that strands every rust compile on the +// client (measured: 0 of 659 rust compiles distributed, all rebuilt on PC1). +// The rewriter keeps the remote outputs and returns Ok, only skipping the +// dep-info path rewrite. +#[test] +#[cfg(feature = "dist-client")] +fn test_rust_outputs_rewriter_tolerates_unmatched_dep_info() { + use crate::compiler::OutputsRewriter; + + let rewriter = Box::new(RustOutputsRewriter { + dep_info: Some(PathBuf::from("/build/deps/somecrate.d")), + }); + let pt = dist::PathTransformer::new(); + + // No output path matches the expected dep-info - the old code bailed here. + let res = rewriter.handle_outputs(&pt, &[PathBuf::from("/build/deps/somecrate.rlib")], &[]); + + assert!( + res.is_ok(), + "an otherwise-successful distributed compile must not be failed just \ + because its dep-info file was not among the returned outputs: {:?}", + res.err() + ); +} + #[test] #[cfg(all(feature = "dist-client", target_os = "windows"))] fn test_rust_outputs_rewriter() { @@ -2674,7 +2858,7 @@ mod test { fn _parse_arguments(arguments: &[String]) -> CompilerArguments { let arguments = arguments.iter().map(OsString::from).collect::>(); - parse_arguments(&arguments, ".".as_ref()) + parse_arguments(&arguments, ".".as_ref(), &[]) } macro_rules! parses { @@ -3567,7 +3751,7 @@ proc_macro false F: Fn(&Path) -> Result<()>, { let oargs = args.iter().map(OsString::from).collect::>(); - let parsed_args = match parse_arguments(&oargs, f.tempdir.path()) { + let parsed_args = match parse_arguments(&oargs, f.tempdir.path(), env_vars) { CompilerArguments::Ok(parsed_args) => parsed_args, o => panic!("Got unexpected parse result: {:?}", o), }; @@ -3984,4 +4168,109 @@ proc_macro false ))); assert_eq!(h.target_json, Some(PathBuf::from("/path/to/target.json"))); } + + #[test] + fn test_parse_target_name_resolved_via_rust_target_path() { + // A bare `--target NAME` naming a custom target spec found on + // RUST_TARGET_PATH is resolved to that JSON path, so the spec is hashed + // and shipped and a remote rustc can load it (OE passes the target by + // name and relies on RUST_TARGET_PATH, which the build server lacks). + let tmp = tempfile::Builder::new() + .prefix("rust-target-path") + .tempdir() + .unwrap(); + let spec = tmp.path().join("aarch64-avocado-linux-gnu.json"); + std::fs::write(&spec, b"{}").unwrap(); + let env_vars = vec![( + OsString::from("RUST_TARGET_PATH"), + OsString::from(tmp.path()), + )]; + + let args = [ + "--crate-name", + "foo", + "--crate-type", + "lib", + "./src/lib.rs", + "--emit=dep-info,link", + "--out-dir", + "/out", + "--target", + "aarch64-avocado-linux-gnu", + ] + .iter() + .map(OsString::from) + .collect::>(); + + let h = match parse_arguments(&args, ".".as_ref(), &env_vars) { + CompilerArguments::Ok(a) => a, + o => panic!("Got unexpected parse result: {:?}", o), + }; + + // The spec is recorded for hashing + shipping, but --target keeps its + // name form so rustc's target identity still matches the prebuilt std. + assert_eq!(h.target_json, Some(spec)); + assert!(h.arguments.contains(&Argument::WithValue( + "--target", + ArgData::Target(ArgTarget::Name("aarch64-avocado-linux-gnu".to_owned())), + ArgDisposition::Separated + ))); + } + + #[test] + fn test_parse_target_name_unresolved_stays_name() { + // RUST_TARGET_PATH set but no matching .json: the target stays a + // bare name and target_json is None. Guards builtins from being rewritten. + let tmp = tempfile::Builder::new() + .prefix("rust-target-path") + .tempdir() + .unwrap(); + let env_vars = vec![( + OsString::from("RUST_TARGET_PATH"), + OsString::from(tmp.path()), + )]; + + let args = [ + "--crate-name", + "foo", + "--crate-type", + "lib", + "./src/lib.rs", + "--emit=dep-info,link", + "--out-dir", + "/out", + "--target", + "x86_64-unknown-linux-gnu", + ] + .iter() + .map(OsString::from) + .collect::>(); + + let h = match parse_arguments(&args, ".".as_ref(), &env_vars) { + CompilerArguments::Ok(a) => a, + o => panic!("Got unexpected parse result: {:?}", o), + }; + + assert!(h.target_json.is_none()); + assert!(h.arguments.contains(&Argument::WithValue( + "--target", + ArgData::Target(ArgTarget::Name("x86_64-unknown-linux-gnu".to_owned())), + ArgDisposition::Separated + ))); + } + + #[cfg(feature = "dist-client")] + #[test] + fn test_is_rustlib_target_libdir() { + // A target sysroot lib dir (holds the prebuilt std/core rlibs). + assert!(is_rustlib_target_libdir(Path::new( + "/work/recipe-sysroot/usr/lib/rustlib/aarch64-avocado-linux-gnu/lib" + ))); + // An ordinary cargo dependency search path must stay filtered. + assert!(!is_rustlib_target_libdir(Path::new( + "/work/git/target/aarch64-avocado-linux-gnu/release/deps" + ))); + // `lib` leaf without a `rustlib` grandparent is not a rustlib dir. + assert!(!is_rustlib_target_libdir(Path::new("/usr/lib"))); + } } diff --git a/src/config.rs b/src/config.rs index f6c3660efc..5e8b5b4f59 100644 --- a/src/config.rs +++ b/src/config.rs @@ -119,6 +119,12 @@ fn default_disk_cache_size() -> u64 { fn default_toolchain_cache_size() -> u64 { TEN_GIGS } +fn default_client_deserialize_offload_threshold() -> u64 { + 1024 * 1024 +} +fn default_run_job_compression_level() -> u32 { + 1 +} struct StringOrU64Visitor; @@ -763,6 +769,9 @@ pub struct DistConfig { #[serde(deserialize_with = "deserialize_size_from_str")] pub toolchain_cache_size: u64, pub rewrite_includes_only: bool, + #[serde(deserialize_with = "deserialize_size_from_str")] + pub client_deserialize_offload_threshold: u64, + pub run_job_compression_level: u32, } impl Default for DistConfig { @@ -774,6 +783,8 @@ impl Default for DistConfig { toolchains: Default::default(), toolchain_cache_size: default_toolchain_cache_size(), rewrite_includes_only: false, + client_deserialize_offload_threshold: default_client_deserialize_offload_threshold(), + run_job_compression_level: default_run_job_compression_level(), } } } @@ -1599,6 +1610,22 @@ pub mod server { pub scheduler_auth: SchedulerAuth, #[serde(default = "default_toolchain_cache_size")] pub toolchain_cache_size: u64, + // Advertise fewer cores than the hardware so the scheduler de-weights + // this node (main.rs load_weight). On the colocated node the real cores + // are also spent preprocessing for the whole cluster and packaging + // toolchains, which the scheduler cannot see; capping the advertised + // count reserves them. None = use the detected hardware count. + #[serde(default)] + pub num_cpus: Option, + // When this build server shares a host with the scheduler (the colocated + // orchestrator node that also runs the client's preprocessing and + // bitbake), reserve this fraction of its cores instead of advertising + // them all - computed from the node's own hardware so the same config is + // portable across cluster topologies. Defaults to 0.35 when unset; + // ignored on a pure remote server. An explicit `num_cpus` above overrides + // this with an absolute count. + #[serde(default)] + pub colocated_reserve_fraction: Option, } pub fn from_path(conf_path: &Path) -> Result> { @@ -2570,6 +2597,8 @@ key_prefix = "cosprefix" toolchains: vec![], toolchain_cache_size: 5368709120, rewrite_includes_only: false, + client_deserialize_offload_threshold: 1024 * 1024, + run_job_compression_level: 1, }, server_startup_timeout_ms: Some(10000), basedirs: vec![], @@ -2636,8 +2665,10 @@ fn server_toml_parse() { token: "my server's token".to_owned() }, toolchain_cache_size: 10737418240, + num_cpus: None, + colocated_reserve_fraction: None, } - ) + ); } #[test] diff --git a/src/dist/cache.rs b/src/dist/cache.rs index 72e9c3d668..0cc26cbb63 100644 --- a/src/dist/cache.rs +++ b/src/dist/cache.rs @@ -489,6 +489,24 @@ impl TcCache { } } + /// Insert an already-materialized toolchain file into the cache by moving it + /// under `tc`'s key. The caller streams the upload to a temp file first, so + /// the transfer runs without holding the cache lock; this method takes the + /// lock only to graft the finished file in. + pub fn insert_at(&mut self, tc: &Toolchain, path: &Path) -> Result<()> { + self.inner + .insert_file(make_lru_key_path(&tc.archive_id), path)?; + let verified_archive_id = file_key(self.get(tc)?)?; + if verified_archive_id == tc.archive_id { + Ok(()) + } else { + // The grafted file hashes to the wrong key; drop it so a poisoned + // entry cannot satisfy a later lookup under the expected key. + let _ = self.inner.remove(make_lru_key_path(&tc.archive_id)); + Err(anyhow!("written file does not match expected hash key")) + } + } + pub fn get_file(&mut self, tc: &Toolchain) -> LruResult { self.inner.get_file(make_lru_key_path(&tc.archive_id)) } @@ -497,6 +515,10 @@ impl TcCache { self.inner.get(make_lru_key_path(&tc.archive_id)) } + pub fn path(&self) -> &Path { + self.inner.path() + } + pub fn len(&self) -> usize { self.inner.len() } diff --git a/src/dist/http.rs b/src/dist/http.rs index 743a5ca69e..dacc53ff97 100644 --- a/src/dist/http.rs +++ b/src/dist/http.rs @@ -66,12 +66,15 @@ mod common { } #[cfg(feature = "dist-client")] - pub async fn bincode_req_fut( + pub async fn bincode_req_fut( req: reqwest::RequestBuilder, + deserialize_offload_threshold: u64, ) -> Result { - // Work around tiny_http issue #151 by disabling HTTP pipeline with - // `Connection: close`. - let res = req.header(header::CONNECTION, "close").send().await?; + // A5: keep-alive is left enabled (no `Connection: close`), so a reused + // connection reaches tiny_http, which risks issue #151 (pipeline/body + // desync) surfacing as bincode decode errors under soak. Revert point: + // re-add `.header(header::CONNECTION, "close")` on the request below. + let res = req.send().await?; let status = res.status(); let bytes = res.bytes().await?; @@ -86,6 +89,11 @@ mod common { } else { anyhow::bail!(errmsg); } + } else if bytes.len() as u64 > deserialize_offload_threshold { + // Deserializing a multi-MB blob inline stalls this async worker from + // polling other in-flight requests; offload large payloads to a + // blocking thread so the reactor keeps making progress. + Ok(tokio::task::spawn_blocking(move || bincode::deserialize(&bytes)).await??) } else { Ok(bincode::deserialize(&bytes)?) } @@ -150,6 +158,7 @@ mod common { pub server_nonce: dist::ServerNonce, pub cert_digest: Vec, pub cert_pem: Vec, + pub active_jobs: Vec, } #[cfg(feature = "dist-server")] @@ -163,6 +172,7 @@ mod common { "cert_digest", &BASE64_URL_SAFE_ENGINE.encode(&self.cert_digest), ) + .field("active_jobs", &self.active_jobs) .finish() } } @@ -208,6 +218,11 @@ pub mod urls { .join("/api/v1/scheduler/status") .expect("failed to create alloc job url") } + pub fn scheduler_deregister_server(scheduler_url: &reqwest::Url) -> reqwest::Url { + scheduler_url + .join("/api/v1/scheduler/deregister_server") + .expect("failed to create deregister url") + } pub fn server_assign_job(server_id: ServerId, job_id: JobId) -> reqwest::Url { let url = format!( @@ -249,7 +264,7 @@ mod server { use std::net::SocketAddr; use std::result::Result as StdResult; use std::sync::atomic; - use std::sync::{LazyLock, Mutex}; + use std::sync::{Arc, LazyLock, Mutex}; use std::thread; use std::time::Duration; @@ -259,9 +274,10 @@ mod server { }; use super::urls; use crate::dist::{ - self, AllocJobResult, AssignJobResult, HeartbeatServerResult, InputsReader, JobAuthorizer, - JobId, JobState, RunJobResult, SchedulerStatusResult, ServerId, ServerNonce, - SubmitToolchainResult, Toolchain, ToolchainReader, UpdateJobStateResult, + self, AllocJobResult, AssignJobResult, DeregisterServerResult, HeartbeatServerResult, + InputsReader, JobAuthorizer, JobId, JobState, RunJobResult, SchedulerStatusResult, + ServerId, ServerNonce, SubmitToolchainResult, Toolchain, ToolchainReader, + UpdateJobStateResult, }; use crate::errors::*; @@ -272,9 +288,12 @@ mod server { pub fn bincode_req( req: reqwest::blocking::RequestBuilder, ) -> Result { - // Work around tiny_http issue #151 by disabling HTTP pipeline with - // `Connection: close`. - let mut res = req.header(reqwest::header::CONNECTION, "close").send()?; + // A5: keep-alive is left enabled (no `Connection: close`), so a reused + // connection reaches tiny_http, which risks issue #151 (pipeline/body + // desync) surfacing as bincode decode errors under soak. Revert point: + // re-add `.header(reqwest::header::CONNECTION, "close")` on the request + // below. + let mut res = req.send()?; let status = res.status(); let mut body = vec![]; res.copy_to(&mut body) @@ -745,9 +764,11 @@ mod server { } // Finish the client let new_client = client_builder - // Disable connection pool to avoid broken connection - // between runtime - .pool_max_idle_per_host(0) + // Keep a small idle pool so keep-alive connections are + // reused across coordination calls; expire them under the + // LAN idle limit to avoid reusing a half-closed socket. + .pool_max_idle_per_host(2) + .pool_idle_timeout(Duration::from_secs(90)) .build() .context("failed to create a HTTP client")?; // Use the updated certificates @@ -805,7 +826,7 @@ mod server { let heartbeat_server = try_or_400_log!(req_id, bincode_input(request)); trace!(target: "sccache_heartbeat", "Req {}: heartbeat_server: {:?}", req_id, heartbeat_server); - let HeartbeatServerHttpRequest { num_cpus, jwt_key, server_nonce, cert_digest, cert_pem } = heartbeat_server; + let HeartbeatServerHttpRequest { num_cpus, jwt_key, server_nonce, cert_digest, cert_pem, active_jobs } = heartbeat_server; try_or_500_log!(req_id, maybe_update_certs( &mut requester.client.lock().unwrap(), &mut server_certificates.lock().unwrap(), @@ -815,7 +836,8 @@ mod server { let res: HeartbeatServerResult = try_or_500_log!(req_id, handler.handle_heartbeat_server( server_id, server_nonce, num_cpus, - job_authorizer + job_authorizer, + active_jobs )); prepare_response(request, &res) }, @@ -833,6 +855,13 @@ mod server { let res: SchedulerStatusResult = try_or_500_log!(req_id, handler.handle_status()); prepare_response(request, &res) }, + (POST) (/api/v1/scheduler/deregister_server) => { + let server_id = check_server_auth_or_err!(request); + trace!("Req {}: deregister_server: {:?}", req_id, server_id); + + let res: DeregisterServerResult = try_or_500_log!(req_id, handler.handle_deregister_server(server_id)); + prepare_response(request, &res) + }, _ => { warn!("Unknown request {:?}", request); rouille::Response::empty_404() @@ -871,6 +900,24 @@ mod server { } } + static SHUTDOWN_REQUESTED: atomic::AtomicBool = atomic::AtomicBool::new(false); + + extern "C" fn handle_shutdown_signal(_sig: libc::c_int) { + // Async-signal-safe: only an atomic store. The deregister + exit run + // on a normal worker thread that polls this flag. + SHUTDOWN_REQUESTED.store(true, atomic::Ordering::SeqCst); + } + + fn install_shutdown_handler() { + // SAFETY: the handler performs only an async-signal-safe atomic store, + // which is sound to run from a signal handler. + let handler = handle_shutdown_signal as extern "C" fn(libc::c_int) as libc::sighandler_t; + unsafe { + libc::signal(libc::SIGTERM, handler); + libc::signal(libc::SIGINT, handler); + } + } + pub struct Server { bind_address: SocketAddr, scheduler_url: reqwest::Url, @@ -883,6 +930,11 @@ mod server { jwt_key: Vec, // Randomly generated nonce to allow the scheduler to detect server restarts server_nonce: ServerNonce, + // Core count advertised to the scheduler's load model. Defaults to the + // detected hardware count; a config override lets the colocated node + // advertise fewer so load_weight reserves cores for its preprocessing + // and toolchain-packaging tax. + advertised_num_cpus: usize, handler: S, } @@ -892,6 +944,7 @@ mod server { bind_address: Option, scheduler_url: reqwest::Url, scheduler_auth: String, + advertised_num_cpus: usize, handler: S, ) -> Result { let (cert_digest, cert_pem, privkey_pem) = @@ -910,6 +963,7 @@ mod server { privkey_pem, jwt_key, server_nonce, + advertised_num_cpus, handler, }) } @@ -924,15 +978,54 @@ mod server { privkey_pem, jwt_key, server_nonce, + advertised_num_cpus, handler, } = self; - let heartbeat_req = HeartbeatServerHttpRequest { - num_cpus: num_cpus(), - jwt_key: jwt_key.clone(), - server_nonce, - cert_digest, - cert_pem: cert_pem.clone(), - }; + // Shared so the heartbeat thread can snapshot the handler's + // running-jobs set each beat while the request router still owns it. + let handler = Arc::new(handler); + // Graceful shutdown: on SIGTERM/SIGINT, deregister from the + // scheduler before exiting so it drops us at once instead of + // waiting out the 90s heartbeat timeout. The timeout stays as the + // backstop for an ungraceful death (crash / power loss). + let deregister_url = urls::scheduler_deregister_server(&scheduler_url); + let deregister_auth = scheduler_auth.clone(); + install_shutdown_handler(); + thread::spawn(move || { + let client = reqwest::blocking::Client::builder() + // Keep a small idle pool with a sub-LAN-limit idle timeout + // so keep-alive connections are reused, not rebuilt. + .pool_max_idle_per_host(2) + .pool_idle_timeout(Duration::from_secs(90)) + .timeout(Duration::from_secs(5)) + .build() + .expect("deregister http client must build"); + loop { + if SHUTDOWN_REQUESTED.load(atomic::Ordering::SeqCst) { + info!("Shutdown signal received; deregistering from scheduler"); + let res: Result = bincode_req( + client + .post(deregister_url.clone()) + .bearer_auth(deregister_auth.clone()) + .bincode(&()) + .expect("failed to serialize deregister"), + ); + match res { + Ok(_) => info!("Deregistered from scheduler"), + Err(e) => error!("Failed to deregister from scheduler: {}", e), + } + std::process::exit(0); + } + thread::sleep(Duration::from_millis(200)); + } + }); + + // Values the heartbeat thread rebuilds its request from each beat. + // cert_digest and server_nonce are used only here, so move them in; + // jwt_key and cert_pem are still needed below, so clone for the thread. + let heartbeat_jwt_key = jwt_key.clone(); + let heartbeat_cert_pem = cert_pem.clone(); + let heartbeat_handler = Arc::clone(&handler); let job_authorizer = JWTJobAuthorizer::new(jwt_key); let heartbeat_url = urls::scheduler_heartbeat_server(&scheduler_url); let requester = ServerRequester { @@ -946,6 +1039,19 @@ mod server { let client = new_reqwest_blocking_client(); loop { trace!(target: "sccache_heartbeat", "Performing heartbeat"); + // Rebuild the request each beat so active_jobs is a fresh + // snapshot of what this server is running. The scheduler + // reaps a Started job only after two consecutive heartbeats + // omit it, so a stale snapshot could either strand a done + // job or reap a live one. + let heartbeat_req = HeartbeatServerHttpRequest { + num_cpus: advertised_num_cpus, + jwt_key: heartbeat_jwt_key.clone(), + server_nonce: server_nonce.clone(), + cert_digest: cert_digest.clone(), + cert_pem: heartbeat_cert_pem.clone(), + active_jobs: heartbeat_handler.active_jobs(), + }; match bincode_req( client .post(heartbeat_url.clone()) @@ -959,11 +1065,11 @@ mod server { if is_new { info!("Server connected to scheduler"); } - thread::sleep(HEARTBEAT_INTERVAL) + thread::sleep(HEARTBEAT_INTERVAL); } Err(e) => { error!(target: "sccache_heartbeat", "Failed to send heartbeat to server: {}", e); - thread::sleep(HEARTBEAT_ERROR_INTERVAL) + thread::sleep(HEARTBEAT_ERROR_INTERVAL); } } } @@ -1065,6 +1171,7 @@ mod client { SchedulerStatusResult, SubmitToolchainResult, Toolchain, }; + use arc_swap::ArcSwap; use async_trait::async_trait; use byteorder::{BigEndian, WriteBytesExt}; use flate2::Compression; @@ -1087,18 +1194,31 @@ mod client { const REQUEST_TIMEOUT_SECS: u64 = 1200; const CONNECT_TIMEOUT_SECS: u64 = 5; + // Map a config level to a zlib setting: 0 disables compression (for + // measurement on a fast LAN), 1-9 select the zlib level. Level 1 reproduces + // the historical `Compression::fast()` default. + fn run_job_compression(level: u32) -> Compression { + match level { + 0 => Compression::none(), + l => Compression::new(l.min(9)), + } + } + pub struct Client { auth_token: String, scheduler_url: reqwest::Url, // cert_digest -> cert_pem server_certs: Arc, Vec>>>, - client: Arc>, + client: Arc>, pool: tokio::runtime::Handle, tc_cache: Arc, rewrite_includes_only: bool, + deserialize_offload_threshold: u64, + run_job_compression_level: u32, } impl Client { + #[allow(clippy::too_many_arguments)] pub fn new( pool: &tokio::runtime::Handle, scheduler_url: reqwest::Url, @@ -1107,15 +1227,19 @@ mod client { toolchain_configs: &[config::DistToolchainConfig], auth_token: String, rewrite_includes_only: bool, + deserialize_offload_threshold: u64, + run_job_compression_level: u32, ) -> Result { let timeout = Duration::new(REQUEST_TIMEOUT_SECS, 0); let connect_timeout = Duration::new(CONNECT_TIMEOUT_SECS, 0); let client = reqwest::ClientBuilder::new() .timeout(timeout) .connect_timeout(connect_timeout) - // Disable connection pool to avoid broken connection - // between runtime - .pool_max_idle_per_host(0) + // Keep a small idle pool so keep-alive connections are reused + // across coordination calls; expire them under the LAN idle + // limit to avoid reusing a half-closed socket. + .pool_max_idle_per_host(2) + .pool_idle_timeout(Duration::from_secs(90)) .build() .context("failed to create an async HTTP client")?; let client_toolchains = @@ -1125,15 +1249,17 @@ mod client { auth_token, scheduler_url, server_certs: Default::default(), - client: Arc::new(Mutex::new(client)), + client: Arc::new(ArcSwap::from_pointee(client)), pool: pool.clone(), tc_cache: Arc::new(client_toolchains), rewrite_includes_only, + deserialize_offload_threshold, + run_job_compression_level, }) } fn update_certs( - client: &mut reqwest::Client, + client: &ArcSwap, certs: &mut HashMap, Vec>, cert_digest: Vec, cert_pem: Vec, @@ -1153,12 +1279,15 @@ mod client { let timeout = Duration::new(REQUEST_TIMEOUT_SECS, 0); let new_client_async = client_async_builder .timeout(timeout) - // Disable keep-alive - .pool_max_idle_per_host(0) + // Keep a small idle pool so keep-alive connections are reused; + // expire them under the LAN idle limit. + .pool_max_idle_per_host(2) + .pool_idle_timeout(Duration::from_secs(90)) .build() .context("failed to create an async HTTP client")?; - // Use the updated certificates - *client = new_client_async; + // Hot-swap the client without holding a lock across the rebuild, so + // in-flight requests keep serving off the old client until the swap. + client.store(Arc::new(new_client_async)); certs.insert(cert_digest, cert_pem); Ok(()) } @@ -1169,13 +1298,13 @@ mod client { async fn do_alloc_job(&self, tc: Toolchain) -> Result { let scheduler_url = self.scheduler_url.clone(); let url = urls::scheduler_alloc_job(&scheduler_url); - let mut req = self.client.lock().unwrap().post(url); + let mut req = self.client.load().post(url); req = req.bearer_auth(self.auth_token.clone()).bincode(&tc)?; let client = self.client.clone(); let server_certs = self.server_certs.clone(); - match bincode_req_fut(req).await? { + match bincode_req_fut(req, self.deserialize_offload_threshold).await? { AllocJobHttpResponse::Success { job_alloc, need_toolchain, @@ -1194,10 +1323,11 @@ mod client { server_id.addr() ); let url = urls::scheduler_server_certificate(&scheduler_url, server_id); - let req = client.lock().unwrap().get(url); - let res: ServerCertificateHttpResponse = bincode_req_fut(req) - .await - .context("GET to scheduler server_certificate failed")?; + let req = client.load().get(url); + let res: ServerCertificateHttpResponse = + bincode_req_fut(req, self.deserialize_offload_threshold) + .await + .context("GET to scheduler server_certificate failed")?; // TODO: Move to asynchronous reqwest client only. // This function internally builds a blocking reqwest client; @@ -1210,7 +1340,7 @@ mod client { .pool .spawn_blocking(move || { Self::update_certs( - &mut client.lock().unwrap(), + &client, &mut server_certs.lock().unwrap(), res.cert_digest, res.cert_pem, @@ -1229,8 +1359,8 @@ mod client { async fn do_get_status(&self) -> Result { let scheduler_url = self.scheduler_url.clone(); let url = urls::scheduler_status(&scheduler_url); - let req = self.client.lock().unwrap().get(url); - bincode_req_fut(req).await + let req = self.client.load().get(url); + bincode_req_fut(req, self.deserialize_offload_threshold).await } async fn do_submit_toolchain( @@ -1241,12 +1371,22 @@ mod client { match self.tc_cache.get_toolchain(&tc) { Ok(Some(toolchain_file)) => { let url = urls::server_submit_toolchain(job_alloc.server_id, job_alloc.job_id); - let req = self.client.lock().unwrap().post(url); + let req = self.client.load().post(url); let toolchain_file = tokio::fs::File::from_std(toolchain_file.into()); let toolchain_file_stream = tokio_util::io::ReaderStream::new(toolchain_file); let body = Body::wrap_stream(toolchain_file_stream); - let req = req.bearer_auth(job_alloc.auth).body(body); - bincode_req_fut(req).await + // This path streams the toolchain as an HTTP chunked body (no + // Content-Length). tiny_http's chunked reader has no drop-drain, + // and the server's submit handler has early returns that skip + // reading the body (cache fast-path, JobNotFound), so keep-alive + // here can desync a pooled connection on the server's unread + // early-returns. Submits are rare and huge, so reuse is worth + // ~nothing; force a close. + let req = req + .bearer_auth(job_alloc.auth) + .header(reqwest::header::CONNECTION, "close") + .body(body); + bincode_req_fut(req, self.deserialize_offload_threshold).await } Ok(None) => Err(anyhow!("couldn't find toolchain locally")), Err(e) => Err(e), @@ -1261,6 +1401,7 @@ mod client { inputs_packager: Box, ) -> Result<(RunJobResult, PathTransformer)> { let url = urls::server_run_job(job_alloc.server_id, job_alloc.job_id); + let compression = run_job_compression(self.run_job_compression_level); let (body, path_transformer) = self .pool @@ -1276,7 +1417,7 @@ mod client { .expect("Infallible write of bincode body to vec failed"); let path_transformer; { - let mut compressor = ZlibWriteEncoder::new(&mut body, Compression::fast()); + let mut compressor = ZlibWriteEncoder::new(&mut body, compression); path_transformer = inputs_packager .write_inputs(&mut compressor) .context("Could not write inputs for compilation")?; @@ -1292,9 +1433,9 @@ mod client { Ok((body, path_transformer)) }) .await??; - let mut req = self.client.lock().unwrap().post(url); + let mut req = self.client.load().post(url); req = req.bearer_auth(job_alloc.auth.clone()).bytes(body); - bincode_req_fut(req) + bincode_req_fut(req, self.deserialize_offload_threshold) .map_ok(|res| (res, path_transformer)) .await } diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 6bc1024aa8..59466394c4 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -564,6 +564,14 @@ pub struct HeartbeatServerResult { pub is_new: bool, } +// DeregisterServer + +#[derive(Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct DeregisterServerResult { + pub success: bool, +} + // RunJob #[derive(Clone, Serialize, Deserialize)] @@ -587,6 +595,18 @@ pub struct SchedulerStatusResult { pub num_servers: usize, pub num_cpus: usize, pub in_progress: usize, + /// Per-server breakdown so a client can see how capacity and load are + /// distributed, not just the cluster aggregate. + pub servers: Vec, +} + +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct ServerStatusResult { + /// The server's socket address, as the scheduler knows it. + pub id: String, + pub num_cpus: usize, + pub in_progress: usize, } // SubmitToolchain @@ -672,8 +692,14 @@ pub trait SchedulerIncoming: Send + Sync { server_nonce: ServerNonce, num_cpus: usize, job_authorizer: Box, + active_jobs: Vec, ) -> ExtResult; // From Server + fn handle_deregister_server( + &self, + server_id: ServerId, + ) -> ExtResult; + // From Server fn handle_update_job_state( &self, job_id: JobId, @@ -704,6 +730,10 @@ pub trait ServerIncoming: Send + Sync { outputs: Vec, inputs_rdr: InputsReader<'_>, ) -> ExtResult; + // Snapshot of the job ids this server is currently running, reported in + // every heartbeat so the scheduler can reap only jobs a server no longer + // reports (liveness lease), never a genuinely-slow live compile. + fn active_jobs(&self) -> Vec; } #[cfg(feature = "dist-server")] diff --git a/src/dist/pkg.rs b/src/dist/pkg.rs index 8bc1744a4f..c52bdc5673 100644 --- a/src/dist/pkg.rs +++ b/src/dist/pkg.rs @@ -98,6 +98,33 @@ mod toolchain_imp { } pub fn add_executable_and_deps(&mut self, executable: PathBuf) -> Result<()> { + // A relocated program interpreter (e.g. a Yocto/OE uninative + // toolchain) resolves its libc/libm against its own sysroot, not the + // host paths ldd reports. Bundle the interpreter's directory so those + // libraries exist where the loader looks inside the build sandbox. + if let Some(libdir) = read_elf_interpreter(&executable) + .and_then(|interp| relocated_interpreter_libdir(&interp)) + { + self.add_dir_contents(&libdir).with_context(|| { + format!( + "Failed to bundle relocated interpreter libdir {}", + libdir.display() + ) + })?; + // Split-sysroot toolchains (e.g. the OE buildtools SDK) keep the + // loader plus libc/libm in /lib but the compiler's own + // dependency libraries (libmpc, libbfd, libsframe, ...) in + // /usr/lib. The relocated loader searches both, but ldd + // (run against the host) resolves those deps to host paths or + // reports them "not found", so they never land where the sandbox + // loader looks. Bundle the sysroot usr/lib shared libraries so + // every NEEDED lib resolves inside the build sandbox. + if let Some(usr_libdir) = sysroot_usr_libdir(&libdir) { + self.add_shared_libraries(&usr_libdir).with_context(|| { + format!("Failed to bundle sysroot usr/lib {}", usr_libdir.display()) + })?; + } + } let mut remaining = vec![executable]; while let Some(obj_path) = remaining.pop() { assert!(obj_path.is_absolute()); @@ -163,8 +190,15 @@ mod toolchain_imp { if file_type.is_dir() { continue; } else if file_type.is_symlink() { - let metadata = fs::metadata(entry.path())?; - if !metadata.file_type().is_file() { + // A dangling symlink (e.g. a sysroot-relative link that + // resolves outside the packaged tree, like an OE + // recipe-sysroot-native `usr/bin/sg`) makes metadata fail. + // Skip it rather than aborting the whole best-effort + // package, matching add_shared_libraries below. + if !fs::metadata(entry.path()) + .map(|m| m.is_file()) + .unwrap_or(false) + { continue; } } else if !file_type.is_file() { @@ -178,6 +212,43 @@ mod toolchain_imp { Ok(()) } + /// Add the shared libraries directly in `dir_path` to the package. + /// + /// Unlike `add_dir_contents` this is shallow and filtered to files + /// whose name looks like an ELF shared object (`*.so`, `*.so.N...`), so + /// bundling a large sysroot `usr/lib` pulls in only the runtime + /// libraries the relocated loader resolves -- not headers, static + /// archives, or subdirectories. Missing directories are a no-op. + pub fn add_shared_libraries(&mut self, dir_path: &Path) -> Result<()> { + if !dir_path.is_dir() { + return Ok(()); + } + for entry in fs::read_dir(dir_path)? { + let entry = entry?; + let name = entry.file_name(); + let name = name.to_string_lossy(); + if !(name.ends_with(".so") || name.contains(".so.")) { + continue; + } + let file_type = entry.file_type()?; + if file_type.is_symlink() { + // A symlink pointing at a regular file; add_file resolves it + // and tarify_path records the link. + if !fs::metadata(entry.path()) + .map(|m| m.is_file()) + .unwrap_or(false) + { + continue; + } + } else if !file_type.is_file() { + continue; + } + trace!("shared lib add_file {}", entry.path().display()); + self.add_file(entry.path())?; + } + Ok(()) + } + pub fn into_compressed_tar(self, writer: W) -> Result<()> { use gzp::{ deflate::Gzip, @@ -350,6 +421,106 @@ mod toolchain_imp { libs } + /// Read a binary's ELF program interpreter (PT_INTERP), if it has one. + /// + /// Returns `None` for static binaries, non-ELF or non-64-bit files, and + /// anything that fails to parse -- callers treat that as "nothing special + /// to bundle", preserving the default ldd-only behaviour. + fn read_elf_interpreter(executable: &Path) -> Option { + use object::Endianness; + use object::read::elf::{ElfFile64, ProgramHeader}; + + let data = fs::read(executable).ok()?; + let elf = ElfFile64::::parse(data.as_slice()).ok()?; + let endian = elf.endian(); + for header in elf.elf_program_headers() { + if let Ok(Some(interp)) = header.interpreter(endian, elf.data()) { + return str::from_utf8(interp).ok().map(PathBuf::from); + } + } + None + } + + /// If `interp` is a relocated program interpreter -- one living outside the + /// standard host loader directories -- return the directory that should be + /// bundled alongside the toolchain. + /// + /// Yocto/OpenEmbedded "uninative" cross toolchains ship their own glibc and + /// loader, and the loader's built-in search path points at its own sysroot + /// lib dir rather than the host's. `ldd` resolves a binary's NEEDED + /// libraries against the *host* loader, so for these binaries it reports + /// host paths (e.g. /usr/lib/libm.so.6) that do not exist where the + /// relocated loader actually searches at runtime. Bundling the + /// interpreter's own directory -- which holds the matching libc/libm and + /// the loader itself -- makes the toolchain resolvable inside the sandbox. + fn relocated_interpreter_libdir(interp: &Path) -> Option { + const STANDARD_LOADER_PREFIXES: &[&str] = &["/lib/", "/lib64/", "/usr/lib/", "/usr/lib64/"]; + let interp_str = interp.to_str()?; + if STANDARD_LOADER_PREFIXES + .iter() + .any(|prefix| interp_str.starts_with(prefix)) + { + return None; + } + interp.parent().map(Path::to_path_buf) + } + + /// Given a relocated interpreter's lib dir (`/lib`), return the + /// sibling `/usr/lib` when it exists. + /// + /// Split-sysroot toolchains (the OE buildtools SDK) keep the compiler's + /// dependency libraries there rather than beside the loader; uninative + /// sysroots have no `usr/lib`, so this returns `None` and leaves that path + /// untouched. + fn sysroot_usr_libdir(libdir: &Path) -> Option { + let usr_lib = libdir.parent()?.join("usr").join("lib"); + usr_lib.is_dir().then_some(usr_lib) + } + + #[test] + fn test_sysroot_usr_libdir() { + let tmp = tempfile::tempdir().unwrap(); + let libdir = tmp.path().join("sysroot").join("lib"); + std::fs::create_dir_all(&libdir).unwrap(); + + // A uninative sysroot has no usr/lib sibling: leave that path untouched. + assert_eq!(sysroot_usr_libdir(&libdir), None); + + // A split sysroot (OE buildtools SDK) has usr/lib: bundle it. + let usr_lib = tmp.path().join("sysroot").join("usr").join("lib"); + std::fs::create_dir_all(&usr_lib).unwrap(); + assert_eq!(sysroot_usr_libdir(&libdir), Some(usr_lib)); + } + + #[test] + fn test_add_dir_contents_skips_dangling_symlink() { + use std::os::unix::fs::symlink; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + // A real file that must be packaged. + let real = root.join("real_file"); + std::fs::write(&real, b"contents").unwrap(); + + // A dangling symlink whose target does not exist -- the shape of an + // OE recipe-sysroot-native `usr/bin/sg` that points outside the + // packaged tree. Before the fix, fs::metadata on this aborted the + // whole toolchain package. + let dangling = root.join("dangling_link"); + symlink("does/not/exist", &dangling).unwrap(); + + let mut builder = ToolchainPackageBuilder::new(); + builder.add_dir_contents(root).unwrap(); + + let packaged: Vec = builder.file_set.values().cloned().collect(); + assert!(packaged.contains(&real), "real file must still be packaged"); + assert!( + !packaged.contains(&dangling), + "dangling symlink must be skipped, not packaged" + ); + } + #[test] fn test_ldd_parse() { let ubuntu_ls_output = "\tlinux-vdso.so.1 => (0x00007fffcfffe000) @@ -409,12 +580,41 @@ mod toolchain_imp { ] ); } + + #[test] + fn test_relocated_interpreter_libdir() { + // Standard host loaders are left to ldd's host resolution. + for standard in [ + "/lib64/ld-linux-x86-64.so.2", + "/usr/lib64/ld-linux-x86-64.so.2", + "/usr/lib/ld-linux-aarch64.so.1", + "/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2", + ] { + assert_eq!( + relocated_interpreter_libdir(Path::new(standard)), + None, + "{standard} is a standard host loader and must not trigger bundling" + ); + } + + // A relocated loader (e.g. a Yocto/OE uninative toolchain) resolves its + // libc/libm against its own sysroot lib dir, which ldd does not report. + // Return that directory so it gets bundled into the toolchain package. + assert_eq!( + relocated_interpreter_libdir(Path::new( + "/home/u/build/tmp/sysroots-uninative/x86_64-linux/lib/ld-linux-x86-64.so.2" + )), + Some(PathBuf::from( + "/home/u/build/tmp/sysroots-uninative/x86_64-linux/lib" + )) + ); + } } -pub fn make_tar_header(src: &Path, dest: &str) -> io::Result { +pub fn make_tar_header(src: &Path) -> io::Result { let metadata_res = fs::metadata(src); - let mut file_header = tar::Header::new_ustar(); + let mut file_header = tar::Header::new_gnu(); // TODO: test this works if let Ok(metadata) = metadata_res { // TODO: if the source file is a symlink, I think this does bad things @@ -430,24 +630,38 @@ pub fn make_tar_header(src: &Path, dest: &str) -> io::Result { file_header.set_mtime(0); file_header .set_device_major(0) - .expect("expected a ustar header"); + .expect("expected a gnu header"); file_header .set_device_minor(0) - .expect("expected a ustar header"); + .expect("expected a gnu header"); file_header.set_entry_type(tar::EntryType::file()); } - // tar-rs imposes that `set_path` takes a relative path + Ok(file_header) +} + +/// Append a file entry to `builder` under the dist path `dest`. +/// +/// `dest` is an absolute dist path; tar-rs requires a relative path, so the +/// leading `/` is stripped. Routing through `Builder::append_data` (rather than +/// `Builder::append` with a pre-set path) makes tar-rs emit a GNU `@LongLink` +/// long-name entry when `dest` exceeds the ustar `name`/`prefix` limits. Deep +/// OE build paths (e.g. gcc-runtime's libstdc++-v3 tree) overrun those limits, +/// and a plain ustar header fails with "provided value is too long when setting +/// path". `Header::set_size` must already be correct on `header`. +pub fn append_tar_entry( + builder: &mut tar::Builder, + header: &mut tar::Header, + dest: &str, + data: R, +) -> io::Result<()> { + // tar-rs imposes that the entry path is relative. assert!(dest.starts_with('/')); let dest = dest.trim_start_matches('/'); assert!(!dest.starts_with('/')); - // `set_path` converts its argument to a Path and back to bytes on Windows, so this is - // a bit of an inefficient round-trip. Windows path separators will also be normalised - // to be like Unix, and the path is (now) relative so there should be no funny results - // due to Windows - // TODO: should really use a `set_path_str` or similar - file_header.set_path(dest)?; - Ok(file_header) + // `append_data` sets the path (emitting a GNU long-name entry if needed) and + // recomputes the checksum before writing the header and `data`. + builder.append_data(header, dest, data) } /// Simplify a path to one without any relative components, erroring if it looks @@ -504,3 +718,49 @@ impl SimplifyPath<'_> { Ok(final_path) } } + +#[cfg(test)] +mod pkg_tests { + use super::*; + use std::io::Read; + + #[test] + fn test_append_tar_entry_roundtrips_long_path() { + // A final path component longer than the ustar name field (100 bytes), + // which ustar cannot split across name/prefix, mirroring the deep + // gcc-runtime libstdc++-v3 build tree that triggers the failure. + let dest = format!( + "/build/gcc-runtime/13.4.0/libstdc++-v3/src/c++98/{}.o", + "b".repeat(120) + ); + let relative = dest.trim_start_matches('/'); + assert!(relative.len() > 100); + + // The old ustar-header path could not encode this: set_path errors with + // "provided value is too long", which is the bug this change fixes. + let mut ustar = tar::Header::new_ustar(); + assert!(ustar.set_path(relative).is_err()); + + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("input.o"); + let contents = b"long-path-payload"; + fs::write(&src, &contents[..]).unwrap(); + + let mut buf = Vec::new(); + { + let mut builder = tar::Builder::new(&mut buf); + let mut header = make_tar_header(&src).unwrap(); + header.set_size(contents.len() as u64); + append_tar_entry(&mut builder, &mut header, &dest, &contents[..]).unwrap(); + builder.into_inner().unwrap(); + } + + let mut archive = tar::Archive::new(buf.as_slice()); + let mut entry = archive.entries().unwrap().next().unwrap().unwrap(); + let entry_path = entry.path().unwrap().to_string_lossy().into_owned(); + assert_eq!(entry_path, relative); + let mut read_back = Vec::new(); + entry.read_to_end(&mut read_back).unwrap(); + assert_eq!(read_back, contents); + } +} diff --git a/src/dist/test.rs b/src/dist/test.rs index 8b13789179..3c158c0c76 100644 --- a/src/dist/test.rs +++ b/src/dist/test.rs @@ -1 +1,44 @@ +use super::*; +/// The JSON shape emitted by `sccache --dist-status` is a contract consumed by +/// the bakar cluster-info preflight (`_parse_cluster_status`): a top-level +/// `servers` array of `{id, num_cpus, in_progress}` objects alongside the +/// aggregate counts. +#[test] +fn scheduler_status_serializes_per_server_breakdown() { + let status = SchedulerStatusResult { + num_servers: 2, + num_cpus: 64, + in_progress: 3, + servers: vec![ + ServerStatusResult { + id: "10.42.0.1:10501".to_string(), + num_cpus: 32, + in_progress: 2, + }, + ServerStatusResult { + id: "10.42.0.2:10501".to_string(), + num_cpus: 32, + in_progress: 1, + }, + ], + }; + + let json = serde_json::to_value(&status).unwrap(); + assert_eq!(json["num_servers"], 2); + assert_eq!(json["num_cpus"], 64); + assert_eq!(json["in_progress"], 3); + let servers = json["servers"].as_array().unwrap(); + assert_eq!(servers.len(), 2); + assert_eq!(servers[0]["id"], "10.42.0.1:10501"); + assert_eq!(servers[0]["num_cpus"], 32); + assert_eq!(servers[0]["in_progress"], 2); + assert_eq!(servers[1]["id"], "10.42.0.2:10501"); + assert_eq!(servers[1]["in_progress"], 1); + + // The aggregate in_progress is independent of the per-server sum: a job + // can be queued at the scheduler before assignment to any server. + let round: SchedulerStatusResult = serde_json::from_value(json).unwrap(); + assert_eq!(round.servers.len(), 2); + assert_eq!(round.servers[0].id, "10.42.0.1:10501"); +} diff --git a/src/lib.rs b/src/lib.rs index d5aa6a6032..f6084803e9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -62,8 +62,6 @@ pub const VERSION: &str = env!("CARGO_PKG_VERSION"); pub const LOGGING_ENV: &str = "SCCACHE_LOG"; pub fn main() { - init_logging(); - let command = match cmdline::try_parse() { Ok(cmd) => cmd, Err(e) => match e.downcast::() { @@ -79,6 +77,10 @@ pub fn main() { }, }; + // Logging is initialized after parsing so the compile-wrapper case can keep + // sccache's own log records off the wrapped compiler's stderr. + init_logging(&command); + std::process::exit(match commands::run_command(command) { Ok(s) => s, Err(e) => { @@ -91,7 +93,7 @@ pub fn main() { }); } -fn init_logging() { +fn init_logging(command: &cmdline::Command) { if env::var(LOGGING_ENV).is_ok() { let mut builder = env_logger::Builder::from_env(LOGGING_ENV); @@ -100,6 +102,35 @@ fn init_logging() { builder.format_timestamp_millis(); } + // When sccache runs as a compiler wrapper (`sccache ...`) it + // shares stderr with the wrapped compiler. Build tools treat any + // unexpected stderr on a feature probe as a compiler failure: libtool's + // `-fPIC` check sets `pic_flag=""` on non-empty stderr, producing non-PIC + // objects that then fail to link into a shared library. sccache's own log + // records must never reach that stderr. Send them to SCCACHE_ERROR_LOG + // when set; otherwise keep stderr only for an interactive terminal (so + // `SCCACHE_LOG=debug sccache gcc ...` at a prompt still shows logs) and + // discard them when stderr is captured by a build. + if matches!(command, cmdline::Command::Compile { .. }) { + use std::io::IsTerminal; + let target: Option> = + match env::var("SCCACHE_ERROR_LOG") { + Ok(path) if !path.is_empty() => Some( + std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(&path) + .map(|f| Box::new(f) as Box) + .unwrap_or_else(|_| Box::new(std::io::sink())), + ), + _ if !std::io::stderr().is_terminal() => Some(Box::new(std::io::sink())), + _ => None, + }; + if let Some(target) = target { + builder.target(env_logger::Target::Pipe(target)); + } + } + match builder.try_init() { Ok(_) => (), Err(e) => panic!("Failed to initialize logging: {:?}", e), diff --git a/src/server.rs b/src/server.rs index 143ea6375c..0454c02493 100644 --- a/src/server.rs +++ b/src/server.rs @@ -133,13 +133,20 @@ fn notify_server_startup(name: Option<&OsString>, status: ServerStartup) -> Resu } #[cfg(unix)] -fn get_signal(status: ExitStatus) -> i32 { +fn get_signal(status: ExitStatus) -> Option { use std::os::unix::prelude::*; - status.signal().expect("must have signal") + // None when the process produced neither an exit code nor a terminating + // signal - e.g. an ExitStatus synthesized from a distributed-compile result, + // or an abnormal wait status. Previously this was `.expect("must have + // signal")`, which panicked the compile task (surfacing as a misleading + // "Failed to bind socket") and could be tripped repeatedly under load. + status.signal() } #[cfg(windows)] -fn get_signal(_status: ExitStatus) -> i32 { - panic!("no signals on windows") +fn get_signal(_status: ExitStatus) -> Option { + // On Windows ExitStatus::code() is always Some, so the signal branch is + // never reached; return None rather than panicking. + None } pub struct DistClientContainer { @@ -160,6 +167,8 @@ pub struct DistClientConfig { toolchain_cache_size: u64, toolchains: Vec, rewrite_includes_only: bool, + deserialize_offload_threshold: u64, + run_job_compression_level: u32, } #[cfg(feature = "dist-client")] @@ -216,6 +225,8 @@ impl DistClientContainer { toolchain_cache_size: config.dist.toolchain_cache_size, toolchains: config.dist.toolchains.clone(), rewrite_includes_only: config.dist.rewrite_includes_only, + deserialize_offload_threshold: config.dist.client_deserialize_offload_threshold, + run_job_compression_level: config.dist.run_job_compression_level, }; let state = Self::create_state(config); let state = pool.block_on(state); @@ -381,6 +392,8 @@ impl DistClientContainer { &config.toolchains, auth_token, config.rewrite_includes_only, + config.deserialize_offload_threshold, + config.run_job_compression_level, ); let dist_client = try_or_retry_later!(dist_client.context("failure during dist client creation")); @@ -1059,6 +1072,8 @@ where toolchain_cache_size: 0, toolchains: vec![], rewrite_includes_only: false, + deserialize_offload_threshold: 1024 * 1024, + run_job_compression_level: 1, }), dist_client, ))), @@ -1573,7 +1588,7 @@ where match status.code() { Some(code) => res.retcode = Some(code), - None => res.signal = Some(get_signal(status)), + None => res.signal = get_signal(status), } res.stdout = stdout; @@ -1589,7 +1604,7 @@ where match output.status.code() { Some(code) => res.retcode = Some(code), - None => res.signal = Some(get_signal(output.status)), + None => res.signal = get_signal(output.status), } res.stdout = output.stdout; res.stderr = output.stderr; @@ -2444,6 +2459,26 @@ fn waits_until_zero() { mod tests { use super::*; + #[cfg(unix)] + #[test] + fn test_get_signal_handles_signal_and_abnormal_status() { + use std::os::unix::process::ExitStatusExt; + + // Terminated by SIGKILL: a real terminating signal is reported. + let killed = ExitStatus::from_raw(9); + assert_eq!(killed.code(), None); + assert_eq!(get_signal(killed), Some(9)); + + // A wait status that is neither a normal exit (code) nor a terminating + // signal - here WIFSTOPPED (low byte 0x7f). The same neither-code-nor- + // signal shape can arise from an ExitStatus synthesized for a + // distributed compile. get_signal must return None, not panic + // "must have signal" (which used to crash the in-flight compile task). + let abnormal = ExitStatus::from_raw(0x7f); + assert_eq!(abnormal.code(), None); + assert_eq!(get_signal(abnormal), None); + } + struct StringWriter { buffer: String, } diff --git a/tests/dist.rs b/tests/dist.rs index f3f52aca34..391b76cf31 100644 --- a/tests/dist.rs +++ b/tests/dist.rs @@ -237,6 +237,9 @@ impl ServerIncoming for FailingServer { .context("Updating job state failed")?; bail!("internal build failure") } + fn active_jobs(&self) -> Vec { + Vec::new() + } } #[test] diff --git a/tests/harness/mod.rs b/tests/harness/mod.rs index d04def7075..873676085e 100644 --- a/tests/harness/mod.rs +++ b/tests/harness/mod.rs @@ -230,6 +230,8 @@ fn sccache_server_cfg( token: DIST_SERVER_TOKEN.to_owned(), }, toolchain_cache_size: TC_CACHE_SIZE, + num_cpus: None, + colocated_reserve_fraction: None, } } @@ -366,7 +368,8 @@ impl DistSystem { SchedulerStatusResult { num_servers: 0, num_cpus: _, - in_progress: 0 + in_progress: 0, + servers: _ } ) { Ok(()) @@ -459,6 +462,7 @@ impl DistSystem { Some(SocketAddr::from(([0, 0, 0, 0], server_addr.port()))), self.scheduler_url().to_url(), token, + sccache::util::num_cpus(), handler, ) .unwrap(); @@ -472,8 +476,11 @@ impl DistSystem { env::set_var("SCCACHE_LOG", "sccache=trace"); } env_logger::try_init().unwrap(); - server.start().unwrap(); - unreachable!(); + // start() returns Result, so it can only resolve to an + // error; surface it as a panic so the arm diverges cleanly without + // an unreachable expression for clippy to flag. + let err = server.start().unwrap_err(); + panic!("dist build server exited: {err}"); } }; @@ -498,7 +505,7 @@ impl DistSystem { panic!("restart not yet implemented for pids") } } - self.wait_server_ready(handle) + self.wait_server_ready(handle); } pub fn count_toolchains_on_server(&mut self, handle: &ServerHandle) -> usize { match handle { @@ -540,7 +547,8 @@ impl DistSystem { SchedulerStatusResult { num_servers: 1, num_cpus: _, - in_progress: 0 + in_progress: 0, + servers: _ } ) { Ok(()) @@ -641,7 +649,7 @@ impl Drop for DistSystem { nix::sys::wait::waitpid(pid, Some(WaitPidFlag::WNOHANG)).map(|ws| { if ws != WaitStatus::StillAlive { killagain = false; - exits.push(ws) + exits.push(ws); } }) ); @@ -696,7 +704,7 @@ impl Drop for DistSystem { ); } for exit in exits { - println!("EXIT: {:?}", exit) + println!("EXIT: {:?}", exit); } if did_err && !thread::panicking() { @@ -740,7 +748,7 @@ fn wait_for_http(url: HTTPUrl, interval: Duration, max_wait: Duration) { }, interval, max_wait, - ) + ); } fn wait_for Result<(), String>>(f: F, interval: Duration, max_wait: Duration) {