diff --git a/CLAUDE.md b/CLAUDE.md index d841c242c..a06488b46 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -130,7 +130,7 @@ The current inventory is auto-published to a stable tracking issue every Monday ## Key Constraints - **No file-based locks** — all locking through daemon's in-memory managers -- **Dev mode isolation** — `FBUILD_DEV_MODE=1` → port 8865, `~/.fbuild/dev/` +- **Dev mode isolation** — `FBUILD_DEV_MODE=1` → `~/.fbuild/dev/`. The daemon endpoint is no longer a fixed port: it's derived per (backend version + cache identity) in the IANA dynamic range 49152–65535 (`fbuild_paths::default_daemon_port` / `daemon_endpoint_key`), so different-version checkouts get isolated daemons and can't serve each other wrong-version builds (FastLED/fbuild#1009). Override with `FBUILD_DAEMON_PORT`. - **HTTP API compatibility** — same endpoints and JSON schemas as the Python daemon - **Windows USB-CDC** — 30 retries, aggressive buffer drain, DTR/RTS toggling after flash - **Emulator CLI convention** — prefer `fbuild test-emu` for CI; `fbuild deploy --to emu [--emulator ]` for interactive use; keep `--target` and `--qemu` only as compatibility aliases diff --git a/crates/CLAUDE.md b/crates/CLAUDE.md index 9afd8d3cc..93f7cb076 100644 --- a/crates/CLAUDE.md +++ b/crates/CLAUDE.md @@ -59,7 +59,7 @@ fbuild-test-support (test utilities) ────────────── - **fbuild-core** — `FbuildError`/`Result`, `BuildProfile`, `Platform`, `SizeInfo`, `DaemonState`. Also ships the `dump_usb_ids` example (`examples/dump_usb_ids.rs`) — **tier-1 source** for the nightly `online-data` USB-VID merge (FastLED/fbuild#720). Do not delete; the `usb-ids` workspace dep exists *only* for that example, so removing it drops the aggregator from 4 → 3 independent sources. - **fbuild-config** — `PlatformIOConfig` (INI parser with `extends` inheritance), `BoardConfig`, `McuSpec` -- **fbuild-paths** — Dev/prod path isolation (`~/.fbuild/{dev|prod}/`), port mapping (8765/8865), cache dirs +- **fbuild-paths** — Dev/prod path isolation (`~/.fbuild/{dev|prod}/`), version+identity-keyed daemon endpoint (`daemon_endpoint_key`/`default_daemon_port`, dynamic range 49152–65535; FastLED/fbuild#1009), cache dirs - **fbuild-packages** — URL-based package downloads, toolchain resolution, library manager, parallel pipeline - **fbuild-serial** — `SharedSerialManager` (centralized serial I/O), deploy preemption protocol, WebSocket messages, USB-CDC retry logic - **fbuild-build** — `BuildOrchestrator` trait, per-platform orchestrators (AVR, ESP32, ESP8266, RP2040, STM32, Teensy, WASM) diff --git a/crates/fbuild-cli/src/daemon_client.rs b/crates/fbuild-cli/src/daemon_client.rs index c6e186e6f..3503038b3 100644 --- a/crates/fbuild-cli/src/daemon_client.rs +++ b/crates/fbuild-cli/src/daemon_client.rs @@ -686,6 +686,38 @@ fn compute_daemon_binary_mtime() -> f64 { 0.0 } +/// Decide whether the CLI should restart the running daemon it just probed. +/// +/// FastLED/fbuild#1009 — arbitrate by version, not raw binary mtime: +/// - CLI **newer** than the daemon → restart (legitimate upgrade). +/// - CLI **older** than the daemon → **never** restart (an older CLI must not +/// evict a newer daemon just because its freshly-built binary has a newer +/// mtime — the exact bug #940/#1006 left open). +/// - **Same** version → restart only if the CLI's sibling daemon binary is +/// newer on disk than the running one (the dev rebuild-then-restart flow). +/// +/// If either version string doesn't parse as semver, fall back to the legacy +/// mtime heuristic. +fn should_restart_daemon( + cli_version: &str, + daemon_version: &str, + cli_mtime: f64, + daemon_mtime: f64, +) -> bool { + let newer_on_disk = cli_mtime > 0.0 && daemon_mtime > 0.0 && cli_mtime > daemon_mtime; + match ( + semver::Version::parse(cli_version), + semver::Version::parse(daemon_version), + ) { + (Ok(cli), Ok(daemon)) => match cli.cmp(&daemon) { + std::cmp::Ordering::Greater => true, // upgrade + std::cmp::Ordering::Less => false, // never evict a newer daemon + std::cmp::Ordering::Equal => newer_on_disk, // dev rebuild + }, + _ => newer_on_disk, // unparseable → legacy mtime behaviour + } +} + /// Ensure the daemon is running. Spawn it if not. /// If the daemon binary has been updated since the running daemon started, /// gracefully restart it (stale source detection, matching Python behavior). @@ -784,29 +816,35 @@ async fn ensure_direct_daemon_running() -> fbuild_core::Result<()> { // Check if already running if client.health().await { - // Check if daemon binary is stale (updated since daemon started) if let Some(health) = client.health_full().await { - if health.source_mtime > 0.0 { - let current_mtime = compute_daemon_binary_mtime(); - if current_mtime > 0.0 && current_mtime > health.source_mtime { - tracing::info!( - "daemon binary is stale (daemon={}, current={}), restarting...", - health.source_mtime, - current_mtime - ); - eprintln!("daemon binary updated, restarting..."); - let _ = client.shutdown().await; - // Wait for it to stop - for _ in 0..50 { - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - if !client.health().await { - break; - } + // FastLED/fbuild#1009: arbitrate by VERSION first, mtime second. + // Never let an older-version CLI evict a newer daemon (the old + // mtime-only check could, since a freshly-built older binary has a + // newer mtime). Restart only on an upgrade, or a same-version dev + // rebuild. + if should_restart_daemon( + env!("CARGO_PKG_VERSION"), + &health.version, + compute_daemon_binary_mtime(), + health.source_mtime, + ) { + tracing::info!( + "daemon needs restart (daemon v{} mtime={}, cli v{} mtime={})", + health.version, + health.source_mtime, + env!("CARGO_PKG_VERSION"), + compute_daemon_binary_mtime(), + ); + eprintln!("daemon binary updated, restarting..."); + let _ = client.shutdown().await; + // Wait for it to stop + for _ in 0..50 { + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + if !client.health().await { + break; } - // Fall through to spawn a fresh daemon below - } else { - return Ok(()); } + // Fall through to spawn a fresh daemon below } else { return Ok(()); } diff --git a/crates/fbuild-cli/src/daemon_client/tests.rs b/crates/fbuild-cli/src/daemon_client/tests.rs index aa1c576dd..03984c86c 100644 --- a/crates/fbuild-cli/src/daemon_client/tests.rs +++ b/crates/fbuild-cli/src/daemon_client/tests.rs @@ -2,7 +2,8 @@ //! parent file under the 1000-LOC gate (see ci.yml LOC Gate workflow). use super::{ - broker_refusal_is_fatal, daemon_cache_identity_error, DaemonAcquisition, DaemonInfoResponse, + broker_refusal_is_fatal, daemon_cache_identity_error, should_restart_daemon, DaemonAcquisition, + DaemonInfoResponse, }; use running_process::broker::client::RefusalKind::{VersionBlocked, VersionUnsupported}; @@ -97,3 +98,36 @@ fn daemon_cache_identity_rejects_wrong_schema() { let err = daemon_cache_identity_error(&info).expect("schema mismatch must fail closed"); assert!(err.contains("cache schema")); } + +// FastLED/fbuild#1009 — version-based daemon arbitration. + +#[test] +fn older_cli_never_evicts_newer_daemon_regardless_of_mtime() { + // The bug: a freshly-built OLDER binary (newer mtime) displacing a running + // NEWER daemon. Must not restart even though cli_mtime > daemon_mtime. + assert!(!should_restart_daemon("2.4.0", "2.5.0", 9999.0, 1.0)); + assert!(!should_restart_daemon("2.4.0", "2.4.1", 9999.0, 1.0)); +} + +#[test] +fn newer_cli_upgrades_the_daemon() { + // CLI strictly newer → restart regardless of mtime. + assert!(should_restart_daemon("2.5.0", "2.4.0", 1.0, 9999.0)); + assert!(should_restart_daemon("2.4.1", "2.4.0", 0.0, 0.0)); +} + +#[test] +fn same_version_restarts_only_on_newer_binary_mtime() { + // Dev rebuild of the same version: restart iff the on-disk binary is newer. + assert!(should_restart_daemon("2.4.0", "2.4.0", 200.0, 100.0)); + assert!(!should_restart_daemon("2.4.0", "2.4.0", 100.0, 200.0)); + assert!(!should_restart_daemon("2.4.0", "2.4.0", 100.0, 100.0)); + // No usable mtimes → don't churn. + assert!(!should_restart_daemon("2.4.0", "2.4.0", 0.0, 0.0)); +} + +#[test] +fn unparseable_versions_fall_back_to_mtime() { + assert!(should_restart_daemon("not-semver", "2.4.0", 200.0, 100.0)); + assert!(!should_restart_daemon("2.4.0", "garbage", 100.0, 200.0)); +} diff --git a/crates/fbuild-paths/src/lib.rs b/crates/fbuild-paths/src/lib.rs index 6c398843a..e880037a6 100644 --- a/crates/fbuild-paths/src/lib.rs +++ b/crates/fbuild-paths/src/lib.rs @@ -43,9 +43,63 @@ pub fn get_daemon_pid_file() -> PathBuf { get_daemon_dir().join("fbuild_daemon.pid") } +/// Short, stable hex key identifying this daemon *endpoint*: a hash of the +/// backend version + the cache identity (mode + trust + cache-root + schema). +/// +/// FastLED/fbuild#1009: the default endpoint used to be a fixed per-user port +/// (8765/8865) shared by every checkout, so a daemon of a *different version* +/// could silently serve another checkout's builds. Keying the endpoint on +/// version+identity means daemons of different versions land on distinct ports +/// and distinct port files — they can no longer serve each other. Two checkouts +/// of the SAME version sharing the SAME cache still (correctly) share a daemon, +/// which is not a wrong-version hazard. +/// +/// `fbuild-paths` is workspace-versioned, so `env!("CARGO_PKG_VERSION")` here is +/// identical to the value compiled into the CLI and the daemon — both sides +/// derive the same key without any handshake. +pub fn daemon_endpoint_key() -> String { + let identity = crate::running_process::DaemonCacheIdentity::discover(); + let material = format!("{}|{}", env!("CARGO_PKG_VERSION"), identity.label_value()); + endpoint_key_from_material(&material) +} + +/// FNV-1a (64-bit) of `material`, formatted as 16 lowercase hex chars. +/// Deterministic + dependency-free; pure (unit-tested). +fn endpoint_key_from_material(material: &str) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for byte in material.as_bytes() { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + format!("{hash:016x}") +} + +/// Deterministic default daemon port derived from [`daemon_endpoint_key`]. +/// +/// Lands in the IANA dynamic range (49152–65535) so it never collides with a +/// well-known service, and is stable for a given version+identity. Distinct +/// versions (and dev vs prod, since mode is part of the identity) get distinct +/// ports. `FBUILD_DAEMON_PORT` still overrides this (see [`get_daemon_port`]). +pub fn default_daemon_port() -> u16 { + port_from_endpoint_key(&daemon_endpoint_key()) +} + +/// Map a hex endpoint key into the IANA dynamic port window (49152–65535). +/// Pure + deterministic (unit-tested). +fn port_from_endpoint_key(key: &str) -> u16 { + const LOW: u32 = 49152; + const SPAN: u32 = 65536 - LOW; // 16384 + let n = u64::from_str_radix(key, 16).unwrap_or(0); + (LOW + (n % u64::from(SPAN)) as u32) as u16 +} + /// Daemon port file path (written by daemon so clients can discover the port). +/// +/// Keyed by [`daemon_endpoint_key`] (FastLED/fbuild#1009) so daemons of +/// different versions/identities write distinct files and never read each +/// other's port. pub fn get_daemon_port_file() -> PathBuf { - get_daemon_dir().join("daemon.port") + get_daemon_dir().join(format!("daemon-{}.port", daemon_endpoint_key())) } /// Daemon log file path. @@ -221,12 +275,18 @@ fn read_port_from_file(path: &Path) -> Option { /// /// Priority: /// 1. `FBUILD_DAEMON_PORT` environment variable (if set and valid 1–65535) -/// 2. Port file in current mode's daemon dir (if exists and valid) -/// 3. Port file in OTHER mode's daemon dir (cross-mode fallback — -/// handles dev daemon running but client not in dev mode, or vice versa) -/// 4. Mode-based default: 8865 (dev) or 8765 (prod) +/// 2. Port file for this endpoint (if it exists and is valid) +/// 3. [`default_daemon_port`] — a deterministic per-(version, cache-identity) +/// port +/// +/// FastLED/fbuild#1009: the endpoint is keyed by version+identity (via +/// [`daemon_endpoint_key`]) rather than a fixed per-user port, so a daemon of a +/// different version can no longer bind the same endpoint and serve another +/// checkout's builds. The old cross-mode fallback (dev CLI adopting a prod +/// daemon and vice-versa) was an *anti*-isolation bridge and is intentionally +/// dropped — dev and prod already have separate roots and now separate ports. pub fn get_daemon_port() -> u16 { - // Priority 1: env var + // Priority 1: env var override. if let Ok(port_str) = std::env::var("FBUILD_DAEMON_PORT") { if let Ok(port) = port_str.parse::() { if port > 0 { @@ -235,24 +295,13 @@ pub fn get_daemon_port() -> u16 { } } - // Priority 2: port file in current mode's daemon dir - let port_file = get_daemon_port_file(); - if let Some(port) = read_port_from_file(&port_file) { + // Priority 2: this endpoint's port file. + if let Some(port) = read_port_from_file(&get_daemon_port_file()) { return port; } - // Priority 3: cross-mode fallback — check the OTHER mode's port file - let other_port_file = get_other_fbuild_root().join("daemon").join("daemon.port"); - if let Some(port) = read_port_from_file(&other_port_file) { - return port; - } - - // Priority 4: mode-based default - if is_dev_mode() { - 8865 - } else { - 8765 - } + // Priority 3: deterministic per-endpoint default. + default_daemon_port() } /// Daemon URL. @@ -367,13 +416,75 @@ mod tests { #[test] fn dev_mode_port() { // Note: can't set env vars in parallel tests safely, and the - // function's own priority chain (env var > current-mode port file > - // other-mode port file > mode-default) legitimately returns any - // u16 > 0. Assert only the contract the function actually promises. + // function's own priority chain (env var > endpoint port file > + // per-endpoint default) legitimately returns any u16 > 0. Assert only + // the contract the function actually promises. let port = get_daemon_port(); assert!(port > 0); } + #[test] + fn endpoint_key_is_deterministic_16_hex() { + // FastLED/fbuild#1009: the key must be deterministic per (version, + // identity) so the CLI and daemon derive the same endpoint. Tested on + // the pure hasher so it's immune to parallel env-var mutation. + let a = endpoint_key_from_material("2.4.0|mode=prod;trust=local;schema=1;cache=/x"); + let b = endpoint_key_from_material("2.4.0|mode=prod;trust=local;schema=1;cache=/x"); + assert_eq!(a, b, "same material must hash identically"); + assert_eq!(a.len(), 16, "expected 16 hex chars, got {a:?}"); + assert!(a.chars().all(|c| c.is_ascii_hexdigit())); + // Different version → different key (this is the #1009 isolation). + let other_version = + endpoint_key_from_material("2.5.0|mode=prod;trust=local;schema=1;cache=/x"); + assert_ne!(a, other_version, "different version must key differently"); + // Live key is well-formed too. + let live = daemon_endpoint_key(); + assert_eq!(live.len(), 16); + assert!(live.chars().all(|c| c.is_ascii_hexdigit())); + } + + #[test] + fn default_daemon_port_is_in_dynamic_range() { + let p = default_daemon_port(); + assert!( + (49152..=65535).contains(&p), + "port {p} outside dynamic range" + ); + } + + #[test] + fn port_from_key_is_deterministic_and_ranged() { + // Same key → same port; keys differing (e.g. by version or checkout) + // map into the dynamic range and generally differ. + assert_eq!( + port_from_endpoint_key("0123456789abcdef"), + port_from_endpoint_key("0123456789abcdef") + ); + for key in ["0000000000000000", "ffffffffffffffff", "deadbeefcafef00d"] { + let p = port_from_endpoint_key(key); + assert!((49152..=65535).contains(&p), "key {key} → {p} out of range"); + } + // Two distinct version/identity keys should not collapse to one port + // for these representative values. + assert_ne!( + port_from_endpoint_key("1111111111111111"), + port_from_endpoint_key("2222222222222222") + ); + } + + #[test] + fn port_file_name_is_endpoint_keyed() { + // The port file must carry the endpoint key so different versions / + // identities never read each other's port. FastLED/fbuild#1009. + let file = get_daemon_port_file(); + let name = file.file_name().unwrap().to_string_lossy(); + assert!( + name.starts_with("daemon-") && name.ends_with(".port"), + "unexpected port file name: {name}" + ); + assert!(name.contains(&daemon_endpoint_key())); + } + #[test] fn other_fbuild_root_is_opposite_mode() { // get_other_fbuild_root should return the opposite mode's root