From f595787409a3fcda9278bbcf2cfcc80092f16460 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Thu, 4 Jun 2026 12:09:06 +0200 Subject: [PATCH] fix: invalidate relative-import cache when imported script changes (#9443) Co-authored-by: Claude Opus 4.8 (1M context) --- ...8e72605971a3f417cb2b032c4d43652aca056.json | 29 ++++ backend/Cargo.lock | 2 + backend/Cargo.toml | 1 + backend/src/main.rs | 6 + backend/windmill-api-scripts/Cargo.toml | 2 + backend/windmill-api-scripts/src/scripts.rs | 137 +++++++++++++++--- backend/windmill-api/Cargo.toml | 2 +- 7 files changed, 157 insertions(+), 22 deletions(-) create mode 100644 backend/.sqlx/query-e88e1009f5359e205a523a32e4e8e72605971a3f417cb2b032c4d43652aca056.json diff --git a/backend/.sqlx/query-e88e1009f5359e205a523a32e4e8e72605971a3f417cb2b032c4d43652aca056.json b/backend/.sqlx/query-e88e1009f5359e205a523a32e4e8e72605971a3f417cb2b032c4d43652aca056.json new file mode 100644 index 0000000000..3db3b0c977 --- /dev/null +++ b/backend/.sqlx/query-e88e1009f5359e205a523a32e4e8e72605971a3f417cb2b032c4d43652aca056.json @@ -0,0 +1,29 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT hash, content FROM script WHERE path = $1 AND workspace_id = $2 AND archived = false ORDER BY created_at DESC LIMIT 1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "hash", + "type_info": "Int8" + }, + { + "ordinal": 1, + "name": "content", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + false + ] + }, + "hash": "e88e1009f5359e205a523a32e4e8e72605971a3f417cb2b032c4d43652aca056" +} diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 221e36646c..2d9a60ee9a 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -13833,6 +13833,7 @@ dependencies = [ "windmill-api-agent-workers", "windmill-api-auth", "windmill-api-client", + "windmill-api-scripts", "windmill-api-settings", "windmill-autoscaling", "windmill-common", @@ -14379,6 +14380,7 @@ dependencies = [ "hyper 1.10.1", "itertools 0.14.0", "lazy_static", + "prometheus", "quick_cache", "reqwest 0.13.1", "serde", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 456cb15b07..c7af01b37f 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -251,6 +251,7 @@ windmill-object-store.workspace = true windmill-git-sync.workspace = true windmill-api = { workspace = true, default-features = false } windmill-api-agent-workers = { workspace = true, optional = true } +windmill-api-scripts.workspace = true windmill-api-settings.workspace = true windmill-worker.workspace = true windmill-indexer = { workspace = true, optional = true } diff --git a/backend/src/main.rs b/backend/src/main.rs index 29bea050ed..e5daf3201b 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -1664,6 +1664,12 @@ async fn process_notify_event( match *source_type { "script" => { windmill_common::DEPLOYED_SCRIPT_HASH_CACHE.remove(&key); + // Evict the relative-import latest-hash cache so a redeployed + // imported script flips the content cache to its new version + // across all replicas within a poll interval (see #6769). Keyed + // by the bare path, matching this event's payload. + windmill_api_scripts::scripts::RAW_SCRIPT_LATEST_HASH_CACHE + .remove(&format!("{workspace_id}:{path}")); if *kind == "preprocessor" { match sqlx::query_scalar::<_, i64>( "SELECT fv.id diff --git a/backend/windmill-api-scripts/Cargo.toml b/backend/windmill-api-scripts/Cargo.toml index 200a2aef64..5ea3088837 100644 --- a/backend/windmill-api-scripts/Cargo.toml +++ b/backend/windmill-api-scripts/Cargo.toml @@ -13,6 +13,7 @@ default = [] enterprise = ["windmill-common/enterprise"] private = ["windmill-common/private", "windmill-dep-map/private"] python = ["dep:windmill-parser-py"] +prometheus = ["dep:prometheus", "windmill-common/prometheus"] [dependencies] windmill-common = { workspace = true, default-features = false } windmill-object-store.workspace = true @@ -38,4 +39,5 @@ tracing.workspace = true chrono.workspace = true lazy_static.workspace = true tokio.workspace = true +prometheus = { workspace = true, optional = true } windmill-parser-py = { workspace = true, optional = true } diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index c783f18d97..c91a45ddfa 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -2099,14 +2099,64 @@ async fn raw_script_by_path_unpinned( lazy_static::lazy_static! { static ref DEBUG_RAW_SCRIPT_ENDPOINTS: bool = std::env::var("DEBUG_RAW_SCRIPT_ENDPOINTS").is_ok(); + + /// Fallback freshness window (seconds) for [`RAW_SCRIPT_LATEST_HASH_CACHE`]. + /// Primary invalidation is event-driven: deploying a script writes a + /// `notify_runnable_version_change` row, and the server's polling-events handler + /// evicts the entry across all replicas (see `main.rs`). This TTL only bounds + /// staleness if that event is missed. Defaults to 60s (matches + /// `DEPLOYED_SCRIPT_HASH_CACHE`). Override with `RAW_SCRIPT_CACHE_TTL_SECONDS`. + static ref RAW_SCRIPT_CACHE_TTL_S: i64 = std::env::var("RAW_SCRIPT_CACHE_TTL_SECONDS") + .ok() + .and_then(|s| s.parse::().ok()) + .filter(|s| *s >= 0) + .unwrap_or(60); } lazy_static::lazy_static! { + // Imported-script content, keyed by + // `{ws}:{path}:{importer_cache_key}[:unpinned]:{latest_hash}`. Including the + // imported script's own latest hash makes each entry immutable, so no + // per-entry TTL is needed; staleness is bounded by RAW_SCRIPT_LATEST_HASH_CACHE. pub static ref RAW_SCRIPT_CACHE: Cache = Cache::new(1000); + // `{ws}:{path}` (bare path) -> (latest non-archived hash, unix_ts cached). + // Resolving the imported script's own hash and keying content by it is what + // fixes relative-import staleness for deployed scripts, whose importer hash + // never moves (see #6769). Evicted on deploy by the `notify_runnable_version_change` + // handler in main.rs (cross-replica, within a poll interval); RAW_SCRIPT_CACHE_TTL_S + // is a fallback bound. + pub static ref RAW_SCRIPT_LATEST_HASH_CACHE: Cache = Cache::new(1000); pub static ref CACHE_FOLDERS_PATH: Cache = Cache::new(1000); } +/// Records a [`RAW_SCRIPT_CACHE`] lookup outcome (`hit` / `expired` / `miss`) to +/// the `raw_script_cache_total` counter when the prometheus feature is enabled. +#[cfg(feature = "prometheus")] +fn record_raw_script_cache(result: &str) { + if let Some(c) = RAW_SCRIPT_CACHE_METRIC.as_ref() { + c.with_label_values(&[result]).inc(); + } +} + +#[cfg(not(feature = "prometheus"))] +fn record_raw_script_cache(_result: &str) {} + +#[cfg(feature = "prometheus")] +lazy_static::lazy_static! { + /// Raw relative-import cache lookups, labeled by `result` (hit/expired/miss). + static ref RAW_SCRIPT_CACHE_METRIC: Option = + if windmill_common::METRICS_ENABLED.load(std::sync::atomic::Ordering::Relaxed) { + Some(prometheus::register_int_counter_vec!( + "raw_script_cache_total", + "Raw script relative-import cache lookups by result (hit/expired/miss)", + &["result"] + ).unwrap()) + } else { + None + }; +} + async fn raw_script_by_path_internal( path: StripPath, user_db: UserDB, @@ -2128,23 +2178,10 @@ async fn raw_script_by_path_internal( } } - let cache_path = query - .cache_key - .map(|x| format!("{w_id}:{path}:{x}{}", if unpin { ":unpinned" } else { "" })); - if let Some(cache_path) = cache_path.clone() { - let cached_content = RAW_SCRIPT_CACHE.get(&cache_path); - if let Some(cached_content) = cached_content { - if *DEBUG_RAW_SCRIPT_ENDPOINTS { - tracing::warn!("Raw script by path request: {} (cached)", path); - } - return Ok(cached_content); - } - } - - if *DEBUG_RAW_SCRIPT_ENDPOINTS { - tracing::warn!("Raw script by path request: {} (not cached)", path); - } - + // Validate + strip the language extension up front so cache keys use the bare + // script path. This matches the `notify_runnable_version_change` event payload + // (which carries the bare path), so a deploy can evict RAW_SCRIPT_LATEST_HASH_CACHE + // by key from the polling-events handler in the server binary. if !path.ends_with(".py") && !path.ends_with(".ts") && !path.ends_with(".go") @@ -2163,6 +2200,52 @@ async fn raw_script_by_path_internal( .trim_end_matches(".go") .trim_end_matches(".sh"); + // Content cache is keyed by the IMPORTED script's own latest hash, not by the + // importer's runnable hash (`query.cache_key`). The importer hash never moves + // when only an imported script's content changes (relock is in-place — see + // #6769), so keying solely on it served stale content indefinitely. The + // importer + unpin dimensions are kept to preserve per-runnable authorization + // scoping (a content-cache hit skips the authed RLS query, so an entry must + // stay scoped to the runnable that fetched it); the imported latest hash is + // appended for content correctness. + let cache_path_base = query + .cache_key + .as_ref() + .map(|x| format!("{w_id}:{path}:{x}{}", if unpin { ":unpinned" } else { "" })); + + // Resolve the imported script's latest hash from RAW_SCRIPT_LATEST_HASH_CACHE + // (keyed by the bare path so the deploy event can evict it). A fresh entry + // serves from the immutable content cache with no DB hit; a stale/absent entry + // falls through to the query below, which refreshes both caches. + let hash_cache_key = format!("{w_id}:{path}"); + let (fresh_hash, had_stale_hash) = match RAW_SCRIPT_LATEST_HASH_CACHE.get(&hash_cache_key) { + Some((hash, cached_at)) + if chrono::Utc::now().timestamp() - cached_at <= *RAW_SCRIPT_CACHE_TTL_S => + { + (Some(hash), false) + } + Some(_) => (None, true), + None => (None, false), + }; + + if let (Some(base), Some(latest_hash)) = (cache_path_base.as_ref(), fresh_hash) { + let content_key = format!("{base}:{latest_hash}"); + if let Some(cached_content) = RAW_SCRIPT_CACHE.get(&content_key) { + if *DEBUG_RAW_SCRIPT_ENDPOINTS { + tracing::warn!("Raw script by path request: {path} (cached, key={content_key})"); + } + record_raw_script_cache("hit"); + return Ok(cached_content); + } + } + if cache_path_base.is_some() { + record_raw_script_cache(if had_stale_hash { "expired" } else { "miss" }); + } + + if *DEBUG_RAW_SCRIPT_ENDPOINTS { + tracing::warn!("Raw script by path request: {} (not cached)", path); + } + // folder cache is only useful for python given it needs to recuse over all intermediate folders to find the package. // When a script exists in a folder, we can cache the fact that the folder exists to avoid extra db calls. let mut split_path = path.split("/").collect::>(); @@ -2195,8 +2278,10 @@ async fn raw_script_by_path_internal( let mut tx = user_db.begin(&authed).await?; - let content_o = sqlx::query_scalar!( - "SELECT content FROM script WHERE path = $1 AND workspace_id = $2 AND archived = false ORDER BY created_at DESC LIMIT 1", + // Fetch the latest non-archived row's hash AND content in one query: the hash + // keys the (immutable) content cache and refreshes RAW_SCRIPT_LATEST_HASH_CACHE. + let row_o = sqlx::query!( + "SELECT hash, content FROM script WHERE path = $1 AND workspace_id = $2 AND archived = false ORDER BY created_at DESC LIMIT 1", path, w_id ) @@ -2204,6 +2289,10 @@ async fn raw_script_by_path_internal( .warn_after_seconds(5) .await?; tx.commit().await?; + let (db_hash, content_o) = match row_o { + Some(r) => (Some(r.hash), Some(r.content)), + None => (None, None), + }; if *DEBUG_RAW_SCRIPT_ENDPOINTS { tracing::warn!( "Raw script by path request: {} (content: {:?})", @@ -2267,8 +2356,14 @@ async fn raw_script_by_path_internal( } } - if let Some(cache_path) = cache_path { - RAW_SCRIPT_CACHE.insert(cache_path, content.clone()); + // content_o was Some, so db_hash is Some too (same row). Refresh the latest-hash + // cache and store the content under the hash-qualified key. + if let Some(db_hash) = db_hash { + RAW_SCRIPT_LATEST_HASH_CACHE + .insert(hash_cache_key, (db_hash, chrono::Utc::now().timestamp())); + if let Some(base) = cache_path_base { + RAW_SCRIPT_CACHE.insert(format!("{base}:{db_hash}"), content.clone()); + } } if *DEBUG_RAW_SCRIPT_ENDPOINTS { tracing::warn!("Raw script by path request: {} (content response)", path); diff --git a/backend/windmill-api/Cargo.toml b/backend/windmill-api/Cargo.toml index fbe6fb23c8..94e72e7c4a 100644 --- a/backend/windmill-api/Cargo.toml +++ b/backend/windmill-api/Cargo.toml @@ -19,7 +19,7 @@ enterprise_saml = ["dep:samael", "dep:libxml"] benchmark = [] embedding = ["windmill-api-embeddings/embedding"] parquet = ["dep:datafusion", "windmill-common/parquet", "windmill-object-store/parquet", "windmill-worker?/parquet", "windmill-api-users/parquet", "windmill-api-settings/parquet", "windmill-api-workspaces/parquet", "dep:aws-sigv4", "dep:aws-sdk-config"] -prometheus = ["windmill-common/prometheus", "windmill-queue/prometheus", "dep:prometheus", "windmill-worker?/prometheus"] +prometheus = ["windmill-common/prometheus", "windmill-queue/prometheus", "dep:prometheus", "windmill-worker?/prometheus", "windmill-api-scripts/prometheus"] openidconnect = ["dep:openidconnect", "windmill-common/openidconnect", "windmill-store/openidconnect"] tantivy = ["dep:windmill-indexer"] kafka = ["dep:windmill-trigger-kafka", "windmill-store/kafka"]