From 8018b4e3f32687681f15ce7cdacea0669c4a5005 Mon Sep 17 00:00:00 2001 From: zackees Date: Thu, 16 Jul 2026 06:14:51 -0700 Subject: [PATCH] fix(cache): revalidate staged sources before deploy Fixes #1075 --- crates/fbuild-daemon/src/context.rs | 8 ++-- crates/fbuild-daemon/src/watch_set_cache.rs | 45 ++++++++++++--------- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/crates/fbuild-daemon/src/context.rs b/crates/fbuild-daemon/src/context.rs index 2dff7664..f05a96d4 100644 --- a/crates/fbuild-daemon/src/context.rs +++ b/crates/fbuild-daemon/src/context.rs @@ -90,13 +90,13 @@ pub const SELF_EVICTION_TIMEOUT: Duration = SELF_EVICTION_TIMEOUT_DEFAULT; /// read from the `FBUILD_WATCH_SET_CACHE_SECS` env var at daemon /// startup (#122 follow-up). /// -/// - Unset / unparseable → 2 s (the cache's own default). +/// - Unset / unparseable → 0 s (the safe cache default). /// - Positive integer → that many seconds. /// - `0` → `Duration::ZERO`, i.e. every entry is stale the instant /// it's stored. Lets an operator bypass the cache at runtime — /// useful for A/B-ing a suspected regression without a rebuild. pub fn watch_set_cache_window_from_env() -> Duration { - const DEFAULT_SECS: u64 = 2; + const DEFAULT_SECS: u64 = 0; let secs = std::env::var("FBUILD_WATCH_SET_CACHE_SECS") .ok() .and_then(|s| s.trim().parse::().ok()) @@ -697,7 +697,7 @@ mod tests { // Unset → default. unsafe { std::env::remove_var("FBUILD_WATCH_SET_CACHE_SECS") }; - assert_eq!(watch_set_cache_window_from_env(), Duration::from_secs(2)); + assert_eq!(watch_set_cache_window_from_env(), Duration::ZERO); // Zero → zero duration (disables the cache by making every // entry stale on store). @@ -710,7 +710,7 @@ mod tests { // Garbage → default (graceful degradation, not a panic). unsafe { std::env::set_var("FBUILD_WATCH_SET_CACHE_SECS", "not-a-number") }; - assert_eq!(watch_set_cache_window_from_env(), Duration::from_secs(2)); + assert_eq!(watch_set_cache_window_from_env(), Duration::ZERO); // Whitespace around a valid value → parsed after trim. unsafe { std::env::set_var("FBUILD_WATCH_SET_CACHE_SECS", " 9 ") }; diff --git a/crates/fbuild-daemon/src/watch_set_cache.rs b/crates/fbuild-daemon/src/watch_set_cache.rs index 73d0ebb4..7bc5dd87 100644 --- a/crates/fbuild-daemon/src/watch_set_cache.rs +++ b/crates/fbuild-daemon/src/watch_set_cache.rs @@ -2,9 +2,9 @@ //! //! Implements [`fbuild_build::build_fingerprint::WatchSetStampCache`] //! over a `DashMap` keyed by a stable hash of the watch set's root -//! paths. Entries are invalidated by a freshness window so a long- -//! running daemon doesn't serve a stale "no changes" answer to a -//! warm-rebuild call that comes minutes after the last one. +//! paths. The production default forces a fresh walk for every build: +//! staging can happen immediately before deployment, and returning a +//! cached "no changes" answer would flash stale firmware. //! //! # Why //! @@ -14,14 +14,10 @@ //! that walk is the dominant cost on warm rebuilds — see //! `docs/PERF_WARM_BUILD.md`. //! -//! Within the same daemon lifetime a back-to-back `fbuild build` / -//! `fbuild deploy` round-trip can reuse the previous walk's result if -//! it's only a few seconds old: any source change a user just made -//! arrived through the file system, which already advanced the watch -//! root's mtime — but our heuristic deliberately doesn't try to be -//! that precise. A short freshness window (default 2 s, see -//! [`DEFAULT_FRESHNESS`]) is enough for the warm-loop case while -//! keeping the worst-case "ignored a real change" window human-noticeable. +//! Operators can explicitly configure a positive freshness window for +//! controlled performance experiments. It is unsafe for normal deploys: +//! an immediate source edit or AutoResearch staging step can otherwise reuse +//! the preceding fingerprint. //! //! # Cycle / staleness model //! @@ -59,12 +55,11 @@ pub struct WatchSetCacheStats { pub puts: u64, } -/// Default freshness window for cache entries. Short enough that a -/// user editing a file and immediately re-building still triggers -/// the real walk (modulo edit speed), long enough to cover the -/// back-to-back deploy / re-deploy interaction the sub-1 s budget -/// targets. Override per-instance via [`DaemonWatchSetCache::with_max_age`]. -pub const DEFAULT_FRESHNESS: Duration = Duration::from_secs(2); +/// Default freshness window for cache entries. Zero forces a source walk on +/// every deploy so an immediate staging step cannot reuse a stale firmware +/// fingerprint. Override per-instance via [`DaemonWatchSetCache::with_max_age`] +/// only for controlled performance experiments. +pub const DEFAULT_FRESHNESS: Duration = Duration::ZERO; /// In-memory cache. Cheap to clone via `Arc` because the only /// state is a `DashMap`. Counter fields are `AtomicU64` so the @@ -211,7 +206,7 @@ mod tests { /// window. Two distinct watch sets must not collide. #[test] fn put_then_get_returns_same_hash() { - let cache = DaemonWatchSetCache::new(); + let cache = DaemonWatchSetCache::with_max_age(Duration::from_secs(1)); let ws_a = vec![watch("/a")]; let ws_b = vec![watch("/b")]; cache.put(&ws_a, "AAA".to_string()); @@ -220,11 +215,23 @@ mod tests { assert_eq!(cache.get(&ws_b).as_deref(), Some("BBB")); } + #[test] + fn default_cache_does_not_reuse_a_fresh_hash_after_staging() { + let cache = DaemonWatchSetCache::new(); + let watches = vec![watch("/project/src/sketch")]; + cache.put(&watches, "before-staging".to_string()); + + assert!( + cache.get(&watches).is_none(), + "the default must force a source walk so an immediate staged change cannot deploy stale firmware" + ); + } + /// Same set of paths in different order hashes to the same key — /// orchestrator can hand us watches without sorting. #[test] fn key_is_order_insensitive() { - let cache = DaemonWatchSetCache::new(); + let cache = DaemonWatchSetCache::with_max_age(Duration::from_secs(1)); let ws_ab = vec![watch("/a"), watch("/b")]; let ws_ba = vec![watch("/b"), watch("/a")]; cache.put(&ws_ab, "X".to_string());