diff --git a/backend/.sqlx/query-bf4e451d2748c880ce5bd189037dda592a71717644c9c4206721de16cff59392.json b/backend/.sqlx/query-bf4e451d2748c880ce5bd189037dda592a71717644c9c4206721de16cff59392.json new file mode 100644 index 0000000000..28cf2579be --- /dev/null +++ b/backend/.sqlx/query-bf4e451d2748c880ce5bd189037dda592a71717644c9c4206721de16cff59392.json @@ -0,0 +1,30 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT\n (SELECT hash FROM script\n WHERE path = $1 AND workspace_id = $2 AND deleted = false\n AND lock IS NOT NULL AND lock_error_logs IS NULL\n ORDER BY created_at DESC LIMIT 1) AS hash,\n (SELECT lock IS NULL AND lock_error_logs IS NULL\n AND created_at > now() - make_interval(secs => $3) FROM script\n WHERE path = $1 AND workspace_id = $2 AND deleted = false\n ORDER BY created_at DESC LIMIT 1) AS pending_lock", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "hash", + "type_info": "Int8" + }, + { + "ordinal": 1, + "name": "pending_lock", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text", + "Text", + "Float8" + ] + }, + "nullable": [ + null, + null + ] + }, + "hash": "bf4e451d2748c880ce5bd189037dda592a71717644c9c4206721de16cff59392" +} diff --git a/backend/migrations/20260821084723_script_insert_notify_before_lock.down.sql b/backend/migrations/20260821084723_script_insert_notify_before_lock.down.sql new file mode 100644 index 0000000000..83b7bec12b --- /dev/null +++ b/backend/migrations/20260821084723_script_insert_notify_before_lock.down.sql @@ -0,0 +1,7 @@ +DROP TRIGGER IF EXISTS script_insert_trigger ON script; + +CREATE TRIGGER script_insert_trigger +AFTER INSERT ON script +FOR EACH ROW +WHEN (NEW.lock IS NOT NULL) +EXECUTE FUNCTION notify_runnable_version_change('script'); diff --git a/backend/migrations/20260821084723_script_insert_notify_before_lock.up.sql b/backend/migrations/20260821084723_script_insert_notify_before_lock.up.sql new file mode 100644 index 0000000000..938471448a --- /dev/null +++ b/backend/migrations/20260821084723_script_insert_notify_before_lock.up.sql @@ -0,0 +1,10 @@ +-- A version deployed without a lockfile gets one later from a dependency job, so gating the +-- insert notification on `lock IS NOT NULL` left the path-to-hash caches holding the previous +-- version until that UPDATE (or their TTL) came around. Notify on every insert instead: the +-- deploy itself is what makes the cached answer suspect. +DROP TRIGGER IF EXISTS script_insert_trigger ON script; + +CREATE TRIGGER script_insert_trigger +AFTER INSERT ON script +FOR EACH ROW +EXECUTE FUNCTION notify_runnable_version_change('script'); diff --git a/backend/src/main.rs b/backend/src/main.rs index 157454af2a..1819146cbe 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -1762,11 +1762,14 @@ async fn process_notify_event( let key = (workspace_id.to_string(), path.to_string()); match *source_type { "script" => { - windmill_common::DEPLOYED_SCRIPT_HASH_CACHE.remove(&key); - // Bundle-cache key resolution for imported scripts; evicted - // together with the content-side caches below so key and - // inlined content flip to the new version in the same window. - windmill_common::IMPORTED_SCRIPT_HASH_CACHE.remove(&key); + // Evicts DEPLOYED_SCRIPT_HASH_CACHE together with the bundle-cache + // key resolution for imported scripts (IMPORTED_SCRIPT_HASH_CACHE), + // so key and inlined content flip to the new version in the same + // window as the content-side cache below. + windmill_common::invalidate_latest_script_hash_caches( + workspace_id, + path, + ); // 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 diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index df82a7a928..79c6c99b9b 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -547,6 +547,7 @@ async fn create_snapshot_script( let mut uploaded = false; let mut handle_deployment_metadata = None; let mut moved_native_triggers = Vec::new(); + let mut deployed_path = None; while let Some(field) = multipart.next_field().await.unwrap() { let name = field.name().unwrap().to_string(); let data = field.bytes().await.unwrap(); @@ -554,6 +555,7 @@ async fn create_snapshot_script( let ns: NewScript = Some(serde_json::from_slice(&data).map_err(to_anyhow)?).unwrap(); let is_tar = ns.codebase.as_ref().is_some_and(|x| x.ends_with(".tar")); let use_esm = ns.codebase.as_ref().is_some_and(|x| x.contains(".esm")); + deployed_path = Some(ns.path.clone()); let (new_hash, ntx, hdm, moved) = create_script_internal( ns, w_id.clone(), @@ -606,6 +608,9 @@ async fn create_snapshot_script( } tx.unwrap().commit().await?; + if let Some(script_path) = deployed_path.as_deref() { + invalidate_script_path_caches(&w_id, script_path); + } reregister_moved_native_triggers(&db, &authed, &w_id, moved_native_triggers); if let Some(hdm) = handle_deployment_metadata { hdm.handle(&db).await?; @@ -741,6 +746,7 @@ async fn deploy_script( ) .await?; tx.commit().await?; + invalidate_script_path_caches(&w_id, &script_path); reregister_moved_native_triggers(&db, &authed_for_triggers, &w_id, moved_native_triggers); if let Some(hdm) = hdm { // Only a script that needed no lock generation is deployed and runnable by @@ -770,6 +776,14 @@ async fn deploy_script( Ok((StatusCode::CREATED, format!("{}", hash))) } +/// Drop the path -> hash entries a just-committed deploy made stale, so this process resolves the +/// path to the new version straight away instead of at the next `notify_event` poll. Must run +/// after the commit, or a concurrent resolution repopulates them from the pre-deploy state. +fn invalidate_script_path_caches(w_id: &str, script_path: &str) { + windmill_common::invalidate_latest_script_hash_caches(w_id, script_path); + RAW_SCRIPT_LATEST_HASH_CACHE.remove(&format!("{w_id}:{script_path}")); +} + /// What a script deploy still has to do once its transaction has committed. enum PostCommitDeploy { /// Everything, for a deploy with no dependency job to hand it to. diff --git a/backend/windmill-api/openapi-deref.json b/backend/windmill-api/openapi-deref.json index 3688bdfe94..5001e7d665 100644 --- a/backend/windmill-api/openapi-deref.json +++ b/backend/windmill-api/openapi-deref.json @@ -12771,7 +12771,7 @@ "description": "Creates a new script at a path that does not already hold one.\nSupplying `parent_hash` instead deploys a new version of the script that hash names, and a `path` differing from that version's moves the script there. `POST /w/{workspace}/scripts/update/{path}` does the same, naming the superseded version in its URL.\n", "operationId": "createScript", "x-mcp-tool": true, - "x-mcp-instructions": "Specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change it. A new version generates its lock async and can take up to a minute before a run by path uses it rather than the previous one.", + "x-mcp-instructions": "Specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change it. A new version generates its lock async, and only a version with a lock is runnable: until it lands, a run by path still executes the previous version. Poll getScriptByPath before running the new one and stop on either outcome: lock non-null means it is ready, lock_error_logs set means the lockfile failed and that version will never run, so report the error instead of polling on.", "x-mcp-tool-include-fields": [ "path", "content", @@ -12822,7 +12822,7 @@ "description": "Deploys a new version of the script at `path`, which must already hold one.\nThe body's `path` is the destination: the same path leaves the script where it\nis, a different one moves it there and archives the old path.\n", "operationId": "updateScript", "x-mcp-tool": true, - "x-mcp-instructions": "Deploys a new version of an existing script, preserving its history, so do NOT delete and recreate a script to change it. Send the whole script, not a patch: read the current one with getScriptByPath first, unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A new version generates its lock async and can take up to a minute before a run by path uses it rather than the previous one.", + "x-mcp-instructions": "Deploys a new version of an existing script, preserving its history, so do NOT delete and recreate a script to change it. Send the whole script, not a patch: read the current one with getScriptByPath first, unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A new version generates its lock async, and only a version with a lock is runnable: until it lands, a run by path still executes the previous version. Poll getScriptByPath before running the new one and stop on either outcome: lock non-null means it is ready, lock_error_logs set means the lockfile failed and that version will never run, so report the error instead of polling on.", "x-mcp-tool-include-fields": [ "path", "content", diff --git a/backend/windmill-api/openapi-deref.yaml b/backend/windmill-api/openapi-deref.yaml index fbeeb2579d..1b5ab9792c 100644 --- a/backend/windmill-api/openapi-deref.yaml +++ b/backend/windmill-api/openapi-deref.yaml @@ -13315,8 +13315,12 @@ paths: For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change - it. A new version generates its lock async and can take up to a minute - before a run by path uses it rather than the previous one. + it. A new version generates its lock async, and only a version with a + lock is runnable: until it lands, a run by path still executes the + previous version. Poll getScriptByPath before running the new one and + stop on either outcome: lock non-null means it is ready, lock_error_logs + set means the lockfile failed and that version will never run, so report + the error instead of polling on. x-mcp-tool-include-fields: - path - content @@ -13519,8 +13523,12 @@ paths: unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A - new version generates its lock async and can take up to a minute before - a run by path uses it rather than the previous one. + new version generates its lock async, and only a version with a lock is + runnable: until it lands, a run by path still executes the previous + version. Poll getScriptByPath before running the new one and stop on + either outcome: lock non-null means it is ready, lock_error_logs set + means the lockfile failed and that version will never run, so report the + error instead of polling on. x-mcp-tool-include-fields: - path - content diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 2656e336b1..2e08452880 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -9454,7 +9454,7 @@ paths: x-mcp-tool: true # The description above names `parent_hash`, which this tool does not expose. x-mcp-tool-description: Creates a script at a path that does not already hold one. - x-mcp-instructions: "Specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change it. A new version generates its lock async and can take up to a minute before a run by path uses it rather than the previous one." + x-mcp-instructions: "Specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change it. A new version generates its lock async, and only a version with a lock is runnable: until it lands, a run by path still executes the previous version. Poll getScriptByPath before running the new one and stop on either outcome: lock non-null means it is ready, lock_error_logs set means the lockfile failed and that version will never run, so report the error instead of polling on." x-mcp-tool-include-fields: - path - content @@ -9493,7 +9493,7 @@ paths: is, a different one moves it there and archives the old path. operationId: updateScript x-mcp-tool: true - x-mcp-instructions: "Deploys a new version of an existing script, preserving its history, so do NOT delete and recreate a script to change it. Send the whole script, not a patch: read the current one with getScriptByPath first, unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A new version generates its lock async and can take up to a minute before a run by path uses it rather than the previous one." + x-mcp-instructions: "Deploys a new version of an existing script, preserving its history, so do NOT delete and recreate a script to change it. Send the whole script, not a patch: read the current one with getScriptByPath first, unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A new version generates its lock async, and only a version with a lock is runnable: until it lands, a run by path still executes the previous version. Poll getScriptByPath before running the new one and stop on either outcome: lock non-null means it is ready, lock_error_logs set means the lockfile failed and that version will never run, so report the error instead of polling on." x-mcp-tool-include-fields: - path - content diff --git a/backend/windmill-api/src/mcp/auto_generated_endpoints.rs b/backend/windmill-api/src/mcp/auto_generated_endpoints.rs index 6b9d0f14fc..568ad854f9 100644 --- a/backend/windmill-api/src/mcp/auto_generated_endpoints.rs +++ b/backend/windmill-api/src/mcp/auto_generated_endpoints.rs @@ -679,7 +679,7 @@ pub fn all_tools() -> Vec { EndpointTool { name: Cow::Borrowed("createScript"), description: Cow::Borrowed("create script: Creates a script at a path that does not already hold one"), - instructions: Cow::Borrowed("Specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change it. A new version generates its lock async and can take up to a minute before a run by path uses it rather than the previous one."), + instructions: Cow::Borrowed("Specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change it. A new version generates its lock async, and only a version with a lock is runnable: until it lands, a run by path still executes the previous version. Poll getScriptByPath before running the new one and stop on either outcome: lock non-null means it is ready, lock_error_logs set means the lockfile failed and that version will never run, so report the error instead of polling on."), path: Cow::Borrowed("/w/{workspace}/scripts/create"), method: Cow::Borrowed("POST"), path_params_schema: None, @@ -730,7 +730,7 @@ pub fn all_tools() -> Vec { description: Cow::Borrowed("update script: Deploys a new version of the script at `path`, which must already hold one. The body's `path` is the destination: the same path leaves the script where it is, a different one moves it there and archives the old path"), - instructions: Cow::Borrowed("Deploys a new version of an existing script, preserving its history, so do NOT delete and recreate a script to change it. Send the whole script, not a patch: read the current one with getScriptByPath first, unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A new version generates its lock async and can take up to a minute before a run by path uses it rather than the previous one."), + instructions: Cow::Borrowed("Deploys a new version of an existing script, preserving its history, so do NOT delete and recreate a script to change it. Send the whole script, not a patch: read the current one with getScriptByPath first, unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A new version generates its lock async, and only a version with a lock is runnable: until it lands, a run by path still executes the previous version. Poll getScriptByPath before running the new one and stop on either outcome: lock non-null means it is ready, lock_error_logs set means the lockfile failed and that version will never run, so report the error instead of polling on."), path: Cow::Borrowed("/w/{workspace}/scripts/update/{path}"), method: Cow::Borrowed("POST"), path_params_schema: Some(serde_json::json!({ diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index 7129ae0e48..e7cb368d25 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -404,6 +404,19 @@ lazy_static::lazy_static! { const LATEST_VERSION_ID_CACHE_TTL: std::time::Duration = std::time::Duration::from_secs(60); +/// TTL for a path -> hash answer that a dependency job is about to invalidate by writing the +/// lockfile of a newer version. That job lands at an unpredictable moment and the eviction it +/// notifies only reaches this process on the next `notify_event` poll, so the entry must not +/// outlive it by more than a beat. +const LATEST_VERSION_ID_PENDING_LOCK_CACHE_TTL: std::time::Duration = + std::time::Duration::from_secs(2); + +/// How long a version without a lockfile is still believed to have a dependency job coming for +/// it. A job that is cancelled while queued, or whose worker dies before it can write +/// `lock_error_logs`, leaves that version pending for good; past this age the short TTL above +/// would be a permanent cost for a version that is never going to become runnable. +const PENDING_LOCK_MAX_AGE: std::time::Duration = std::time::Duration::from_secs(10 * 60); + /// Test hook: disables the process-global deployed-script hash/info caches so /// every resolution reads the current DB. Integration tests use `#[sqlx::test]` /// isolated DBs that share one workspace id and reuse script paths, so a cache @@ -1843,11 +1856,12 @@ pub fn get_latest_deployed_hash_for_path<'e>( } _ => { tracing::debug!("Fetching script hash for {script_path}"); - let hash = if let Some(db) = db { + let latest = if let Some(db) = db { let authed = db.authed; let mut conn = db.acquire().await?; - let hash = get_latest_script_hash(&mut *conn, script_path, w_id).await?; - if let Some(hash) = hash { + let latest = + get_latest_deployed_script_hash(&mut *conn, script_path, w_id).await?; + if let Some(hash) = latest.hash { HASH_PERMS_CACHE.insert( computed_hash.unwrap_or_else(|| PermsCache::compute_hash(authed)), ScriptHash(hash), @@ -1861,19 +1875,24 @@ pub fn get_latest_deployed_hash_for_path<'e>( return Err(Error::NotAuthorized(format!("You are not authorized to access this script: {script_path} (but it exists). Your permissions are: {:?}", authed))); } } - hash + latest } else { let mut conn = db2.acquire().await?; - get_latest_script_hash(&mut *conn, script_path, w_id).await? + get_latest_deployed_script_hash(&mut *conn, script_path, w_id).await? }; - let hash = utils::not_found_if_none(hash, "script", script_path)?; + let hash = utils::not_found_if_none(latest.hash, "script", script_path)?; if use_cache { + let ttl = if latest.pending_lock { + LATEST_VERSION_ID_PENDING_LOCK_CACHE_TTL + } else { + LATEST_VERSION_ID_CACHE_TTL + }; DEPLOYED_SCRIPT_HASH_CACHE.insert( cache_key, ExpiringLatestVersionId { id: hash, - expires_at: std::time::Instant::now() + LATEST_VERSION_ID_CACHE_TTL, + expires_at: std::time::Instant::now() + ttl, }, ); } @@ -1901,6 +1920,60 @@ pub async fn get_latest_script_hash<'e, E: sqlx::PgExecutor<'e>>( return Ok(hash); } +pub struct LatestDeployedScriptHash { + pub hash: Option, + /// The newest version of the path is still waiting on the dependency job that writes its + /// lockfile, so `hash` points at the version before it and will change the moment that job + /// lands, at a moment nothing notifies the caller of. + pub pending_lock: bool, +} + +/// Applies no authorization of its own, exactly like [`get_latest_script_hash`]: pass an +/// RLS-scoped executor, or check the caller's permissions on the hash it returns. +pub async fn get_latest_deployed_script_hash<'e, E: sqlx::PgExecutor<'e>>( + db: E, + script_path: &'e str, + w_id: &'e str, +) -> error::Result { + let row = sqlx::query!( + "SELECT + (SELECT hash FROM script + WHERE path = $1 AND workspace_id = $2 AND deleted = false + AND lock IS NOT NULL AND lock_error_logs IS NULL + ORDER BY created_at DESC LIMIT 1) AS hash, + (SELECT lock IS NULL AND lock_error_logs IS NULL + AND created_at > now() - make_interval(secs => $3) FROM script + WHERE path = $1 AND workspace_id = $2 AND deleted = false + ORDER BY created_at DESC LIMIT 1) AS pending_lock", + script_path, + w_id, + PENDING_LOCK_MAX_AGE.as_secs_f64() + ) + .fetch_one(db) + .await?; + Ok( + LatestDeployedScriptHash { + hash: row.hash, + pending_lock: row.pending_lock.unwrap_or(false), + }, + ) +} + +/// Drop this process's path -> runnable-hash entry for a script whose newest runnable version +/// just moved, so the process that deployed it (or that generated its lockfile) resolves the +/// path to it without waiting out the `notify_event` poll. Other replicas get there through +/// `notify_runnable_version_change`. +pub fn invalidate_deployed_script_hash_cache(w_id: &str, script_path: &str) { + DEPLOYED_SCRIPT_HASH_CACHE.remove(&(w_id.to_string(), script_path.to_string())); +} + +/// Same, for a new version row, which also moves the import-side answer (that one has no lock +/// predicate, so only a new row moves it). +pub fn invalidate_latest_script_hash_caches(w_id: &str, script_path: &str) { + invalidate_deployed_script_hash_cache(w_id, script_path); + IMPORTED_SCRIPT_HASH_CACHE.remove(&(w_id.to_string(), script_path.to_string())); +} + /// Latest non-archived hash for an imported `path`, for bundle cache keying. /// MUST select the same row as the bundler's content endpoint /// (`raw_script_by_path_internal`: `archived = false ORDER BY created_at DESC`, diff --git a/backend/windmill-common/tests/latest_deployed_script_hash.rs b/backend/windmill-common/tests/latest_deployed_script_hash.rs new file mode 100644 index 0000000000..a8bdc2b14f --- /dev/null +++ b/backend/windmill-common/tests/latest_deployed_script_hash.rs @@ -0,0 +1,86 @@ +use sqlx::{Pool, Postgres}; +use windmill_common::get_latest_deployed_script_hash; + +const WORKSPACE: &str = "test-workspace"; + +async fn deploy_version(db: &Pool, path: &str, hash: i64, lock: Option<&str>, age: f64) { + sqlx::query( + "INSERT INTO script (workspace_id, hash, path, summary, description, content, created_by, \ + language, kind, lock, created_at) \ + VALUES ($1, $2, $3, '', '', 'def main(): pass', 'test-user', 'python3', 'script', $4, \ + now() - make_interval(secs => $5))", + ) + .bind(WORKSPACE) + .bind(hash) + .bind(path) + .bind(lock) + .bind(age) + .execute(db) + .await + .expect("failed to deploy script version"); +} + +/// A version whose dependency job has not written its lockfile yet is not runnable, so the answer +/// stays on the version before it. Callers cache that answer, and only `pending_lock` tells them +/// the dependency job is about to change it under them. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn pending_lock_flags_an_answer_a_dependency_job_is_about_to_change(db: Pool) { + let path = "f/test/probe"; + deploy_version(&db, path, 1, Some(""), 2.0).await; + + let latest = get_latest_deployed_script_hash(&db, path, WORKSPACE) + .await + .unwrap(); + assert_eq!(latest.hash, Some(1)); + assert!(!latest.pending_lock); + + deploy_version(&db, path, 2, None, 1.0).await; + + let latest = get_latest_deployed_script_hash(&db, path, WORKSPACE) + .await + .unwrap(); + assert_eq!(latest.hash, Some(1), "an unlocked version is not runnable"); + assert!(latest.pending_lock, "but it is about to become the answer"); + + sqlx::query("UPDATE script SET lock = '' WHERE workspace_id = $1 AND hash = 2") + .bind(WORKSPACE) + .execute(&db) + .await + .expect("failed to write the lockfile"); + + let latest = get_latest_deployed_script_hash(&db, path, WORKSPACE) + .await + .unwrap(); + assert_eq!(latest.hash, Some(2)); + assert!(!latest.pending_lock); +} + +/// A version that will never become runnable must not keep the path on the short TTL: its +/// dependency job either reported a failure, or is old enough that it is not coming. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn pending_lock_gives_up_on_a_dependency_job_that_will_not_land(db: Pool) { + let failed = "f/test/failed"; + deploy_version(&db, failed, 1, Some(""), 2.0).await; + deploy_version(&db, failed, 2, None, 1.0).await; + sqlx::query("UPDATE script SET lock_error_logs = 'boom' WHERE workspace_id = $1 AND hash = 2") + .bind(WORKSPACE) + .execute(&db) + .await + .expect("failed to report the lock error"); + + let latest = get_latest_deployed_script_hash(&db, failed, WORKSPACE) + .await + .unwrap(); + assert_eq!(latest.hash, Some(1)); + assert!(!latest.pending_lock); + + let abandoned = "f/test/abandoned"; + deploy_version(&db, abandoned, 3, Some(""), 3600.0).await; + deploy_version(&db, abandoned, 4, None, 1800.0).await; + + let latest = get_latest_deployed_script_hash(&db, abandoned, WORKSPACE) + .await + .unwrap(); + assert_eq!(latest.hash, Some(3)); + assert!(!latest.pending_lock); +} diff --git a/backend/windmill-common/tests/notify_events.rs b/backend/windmill-common/tests/notify_events.rs index 50ada6f606..8161130875 100644 --- a/backend/windmill-common/tests/notify_events.rs +++ b/backend/windmill-common/tests/notify_events.rs @@ -492,6 +492,8 @@ async fn test_trigger_notify_runnable_version_change_script(db: Pool) let script_path = format!("f/test/script_{}", uuid::Uuid::new_v4()); let script_hash: i64 = rand::random::().abs(); + let before_insert_id = get_latest_event_id(&db).await.unwrap(); + sqlx::query( "INSERT INTO script (workspace_id, hash, path, summary, description, content, created_by, language, kind) VALUES ('test-workspace', $1, $2, 'test', 'test', 'def main(): pass', 'test-user', 'python3', 'script')", @@ -502,6 +504,20 @@ async fn test_trigger_notify_runnable_version_change_script(db: Pool) .await .expect("Failed to insert script"); + // The deploy has to notify even though the version is not runnable yet: it is what makes the + // path -> hash caches suspect, and waiting for the lock UPDATE leaves them serving the + // previous version for a poll interval past the moment the new one becomes runnable. + let insert_events = poll_notify_events(&db, before_insert_id) + .await + .expect("Should poll events"); + assert!( + insert_events + .iter() + .any(|e| e.channel == "notify_runnable_version_change" + && e.payload.starts_with("test-workspace:script:")), + "Should have notify_runnable_version_change event for a version deployed without a lock" + ); + let before_id = get_latest_event_id(&db).await.unwrap(); // Update the lock field (this should trigger the notification) diff --git a/backend/windmill-worker/src/worker_lockfiles.rs b/backend/windmill-worker/src/worker_lockfiles.rs index 6706c5c07b..5cfb9f08ce 100644 --- a/backend/windmill-worker/src/worker_lockfiles.rs +++ b/backend/windmill-worker/src/worker_lockfiles.rs @@ -489,6 +489,11 @@ pub async fn handle_dependency_job( // Since only worker that ran this Dependency Job has the cache // we do not need to think about invalidating cache for other workers. cache::script::invalidate(current_hash); + // The version only became runnable now, so this process still resolves the path to + // the one before it. Only the runnable-hash cache: the import-side caches ignore the + // lock, so evicting this process' half of that pair here would key a bundle by a + // hash whose content cache has not caught up. + windmill_common::invalidate_deployed_script_hash_cache(w_id, script_path); if let Err(e) = handle_deployment_metadata( &job.permissioned_as_email, diff --git a/frontend/src/lib/mcpEndpointTools.ts b/frontend/src/lib/mcpEndpointTools.ts index 5896e9e1ce..3e8ae4cba0 100644 --- a/frontend/src/lib/mcpEndpointTools.ts +++ b/frontend/src/lib/mcpEndpointTools.ts @@ -688,7 +688,7 @@ export const mcpEndpointTools: EndpointTool[] = [ { name: "createScript", description: "create script: Creates a script at a path that does not already hold one", - instructions: "Specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change it. A new version generates its lock async and can take up to a minute before a run by path uses it rather than the previous one.", + instructions: "Specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. A path that already holds a script is refused: use updateScript to deploy a new version of it, and do NOT delete and recreate a script to change it. A new version generates its lock async, and only a version with a lock is runnable: until it lands, a run by path still executes the previous version. Poll getScriptByPath before running the new one and stop on either outcome: lock non-null means it is ready, lock_error_logs set means the lockfile failed and that version will never run, so report the error instead of polling on.", path: "/w/{workspace}/scripts/create", method: "POST", pathParamsSchema: undefined, @@ -737,7 +737,7 @@ export const mcpEndpointTools: EndpointTool[] = [ { name: "updateScript", description: "update script: Deploys a new version of the script at `path`, which must already hold one.\nThe body's `path` is the destination: the same path leaves the script where it\nis, a different one moves it there and archives the old path", - instructions: "Deploys a new version of an existing script, preserving its history, so do NOT delete and recreate a script to change it. Send the whole script, not a patch: read the current one with getScriptByPath first, unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A new version generates its lock async and can take up to a minute before a run by path uses it rather than the previous one.", + instructions: "Deploys a new version of an existing script, preserving its history, so do NOT delete and recreate a script to change it. Send the whole script, not a patch: read the current one with getScriptByPath first, unless you wrote its content yourself. Set path__body only to move the script to a different path; omit it to leave the script where it is. A path that holds no script is refused: use createScript to create one. A new version generates its lock async, and only a version with a lock is runnable: until it lands, a run by path still executes the previous version. Poll getScriptByPath before running the new one and stop on either outcome: lock non-null means it is ready, lock_error_logs set means the lockfile failed and that version will never run, so report the error instead of polling on.", path: "/w/{workspace}/scripts/update/{path}", method: "POST", pathParamsSchema: {