mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-05 08:02:18 +00:00
fix: record supplied script lock hashes so importers can skip relocking (#10915)
* fix: record supplied script lock hashes so importers can skip relocking
Creating a script with a caller-supplied lock — a CLI push, a git-sync deploy,
any create carrying a lockfile — stored the lock on `script` but never wrote the
matching `lock_hash(workspace_id, path, hash_script(lock))` row. Only
worker-generated locks did.
`try_skip_relock` treats a missing hash for an imported script as changed, so no
importer of such a script could ever satisfy the skip predicate: every deploy of
it relocked every importer, forever.
The create transaction now records the hash for any lock it accepts, including
the empty one a codebase or a language with no lock generation carries — the
worker writes `hash_script("")` there, and a path going from a real lock to an
empty one has to stop matching what its importers recorded. Only a lock left to
a dependency job is skipped, because that job writes it.
A workspace clone now carries `lock_hash` too, without which every
dependency-map snapshot the clone later recorded held NULL and nothing in it
could ever skip. `dependency_map.imported_lockfile_hash` is deliberately not
copied: it records what an importer resolved against when it was last locked,
the clone runs READ COMMITTED, and a relock landing in the source between the
scripts being cloned and that statement would attach a hash the cloned
importer's lock was never resolved against — a hash older than the cloned
scripts costs one relock, a newer one skips a relock that was needed.
Lock generation is untouched, as is everything a relock does once it runs. The
only behavior that moves is which relocks are skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138oct9a6SLEZvFyCQgHRBx
* fix: narrow to the create-path lock hash
Drop the workspace-clone copy of lock_hash. It sits outside the reported
bug, and its double join over `script` can emit a path twice where two
versions are live, which the unique key on (workspace_id, path) then
rejects, failing the whole fork.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138oct9a6SLEZvFyCQgHRBx
* fix: restore the workspace-clone lock hash copy, guarded against fanout
A path can hold two live versions, and both joins match on path alone, so
the select can emit it four times against a primary key that admits one.
Every such row carries the single hash the path has, so ON CONFLICT DO
NOTHING settles it rather than aborting the fork.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138oct9a6SLEZvFyCQgHRBx
* fix: hash a clone's own locks rather than copying the source's rows
A source row is only as current as the last write to it, and a supplied
lock deployed before this was recorded leaves one naming a lock the path
no longer holds. Copying that into a fork hands an importer a hash it
never resolved against; hashing what the clone holds cannot.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138oct9a6SLEZvFyCQgHRBx
* test: pin the lock hash written on a no-op push
Removing that write leaves the assertion with no row, which is the state
a script deployed before this shipped would stay in.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138oct9a6SLEZvFyCQgHRBx
* refactor: share one lock hash writer between the create and clone paths
Both wrote the same upsert with different SQL. The existing writers fold
theirs into the statement that writes the lock itself, which is what keeps
the two consistent; these two have nothing to fold it into, so they take a
shared one instead. The clone walks its pages by path rather than listing
them first, dropping a query with it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138oct9a6SLEZvFyCQgHRBx
* fix: stream a clone's locks rather than reading them in pages
script.lock is unbounded, so a page of them is bounded only by how many
it holds. Hashing each as it arrives keeps one in memory at a time and
lets the clone site collapse to a single call.
Also states on both writers that they check no access to the workspace
they write, which their callers are the ones to have established.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138oct9a6SLEZvFyCQgHRBx
* fix: make the lock hash writer safe to repeat and free when unchanged
A path given twice in one call would have Postgres reject the whole
statement, so the last hash for each wins. And recording a hash a path
already has cut a row version for nothing on every unchanged sync, which
is the mode the no-op push runs in.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0138oct9a6SLEZvFyCQgHRBx
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
419741e5d2
commit
17ba521c35
+23
@@ -0,0 +1,23 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT lockfile_hash FROM lock_hash WHERE workspace_id = $1 AND path = $2",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "lockfile_hash",
|
||||
"type_info": "Int8"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
false
|
||||
]
|
||||
},
|
||||
"hash": "1572b7348a05b7e357031f8d44b5bbee155569488352c10b334ce57d83ce1c0a"
|
||||
}
|
||||
+28
@@ -0,0 +1,28 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT DISTINCT ON (path) path, lock FROM script\n WHERE workspace_id = $1 AND NOT archived AND NOT deleted AND lock IS NOT NULL\n ORDER BY path, created_at DESC",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "path",
|
||||
"type_info": "Varchar"
|
||||
},
|
||||
{
|
||||
"ordinal": 1,
|
||||
"name": "lock",
|
||||
"type_info": "Text"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
false,
|
||||
true
|
||||
]
|
||||
},
|
||||
"hash": "8da59f1ace46dc9830cb0fc5a640df68f552e38d4b587839e0e41285a2d55455"
|
||||
}
|
||||
+23
@@ -0,0 +1,23 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT COUNT(*) FROM script WHERE workspace_id = $1 AND path = $2",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "count",
|
||||
"type_info": "Int8"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "abdf62ef0e4eeb8c3213d2e8837e7032f710fe20dd272b7840c9bfbdb92554db"
|
||||
}
|
||||
+16
@@ -0,0 +1,16 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "INSERT INTO lock_hash (workspace_id, path, lockfile_hash)\n SELECT $1, * FROM UNNEST($2::text[], $3::bigint[])\n ON CONFLICT (workspace_id, path) DO UPDATE SET lockfile_hash = EXCLUDED.lockfile_hash\n WHERE lock_hash.lockfile_hash IS DISTINCT FROM EXCLUDED.lockfile_hash",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Varchar",
|
||||
"TextArray",
|
||||
"Int8Array"
|
||||
]
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "cc507843e00c83a42cc4a463999656ce9a8b0499d6b9282a3ecfae3164b17c2a"
|
||||
}
|
||||
+15
@@ -0,0 +1,15 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "DELETE FROM lock_hash WHERE workspace_id = $1 AND path = $2",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "dd15827cbac128ec53cf03fa305e4cad34e540a2cc09c92e262491145a0de05a"
|
||||
}
|
||||
Generated
+1
@@ -15650,6 +15650,7 @@ name = "windmill-dep-map"
|
||||
version = "1.801.0"
|
||||
dependencies = [
|
||||
"chrono",
|
||||
"futures",
|
||||
"itertools 0.14.0",
|
||||
"lazy_static",
|
||||
"serde",
|
||||
|
||||
@@ -38,6 +38,85 @@ fn new_script(path: &str, summary: &str, content: &str) -> serde_json::Value {
|
||||
})
|
||||
}
|
||||
|
||||
/// A supplied lock queues no dependency job, so if the create does not record its hash nothing
|
||||
/// ever will, and every importer of this script relocks on each of its deploys forever after.
|
||||
#[sqlx::test(migrations = "../migrations", fixtures("base"))]
|
||||
async fn test_create_script_persists_supplied_lock_hash(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let port = server.addr.port();
|
||||
let path = "u/test-user/supplied_lock";
|
||||
let lock = r#"{"version":"4","remote":{}}"#;
|
||||
let mut script = new_script(
|
||||
path,
|
||||
"Supplied lock",
|
||||
"export async function main() { return 42; }",
|
||||
);
|
||||
script["lock"] = json!(lock);
|
||||
|
||||
let resp = authed(client().post(format!(
|
||||
"http://localhost:{port}/api/w/test-workspace/scripts/create"
|
||||
)))
|
||||
.json(&script)
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 201, "create: {}", resp.text().await?);
|
||||
|
||||
let stored_hash = sqlx::query_scalar!(
|
||||
"SELECT lockfile_hash FROM lock_hash WHERE workspace_id = $1 AND path = $2",
|
||||
"test-workspace",
|
||||
path,
|
||||
)
|
||||
.fetch_one(&db)
|
||||
.await?;
|
||||
assert_eq!(stored_hash, windmill_common::scripts::hash_script(lock));
|
||||
|
||||
// A script deployed before the create recorded hashes has no row, and pushing it unchanged
|
||||
// creates no version to hang one off. Without the write on that path it would keep its
|
||||
// importers relocking until someone edited it.
|
||||
sqlx::query!(
|
||||
"DELETE FROM lock_hash WHERE workspace_id = $1 AND path = $2",
|
||||
"test-workspace",
|
||||
path,
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
// The no-op comparison covers every field, so the push has to carry what the first deploy
|
||||
// filled in by itself; `auto_parent` both resolves the parent and keeps the hash distinct.
|
||||
script["auto_parent"] = json!(true);
|
||||
script["ws_error_handler_muted"] = json!(false);
|
||||
script["assets"] = json!([]);
|
||||
let resp = authed(client().post(format!(
|
||||
"http://localhost:{port}/api/w/test-workspace/scripts/create?skip_if_noop=true"
|
||||
)))
|
||||
.json(&script)
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 201, "no-op push: {}", resp.text().await?);
|
||||
|
||||
let versions: i64 = sqlx::query_scalar!(
|
||||
"SELECT COUNT(*) FROM script WHERE workspace_id = $1 AND path = $2",
|
||||
"test-workspace",
|
||||
path,
|
||||
)
|
||||
.fetch_one(&db)
|
||||
.await?
|
||||
.unwrap_or_default();
|
||||
assert_eq!(versions, 1, "no-op push must not create a version");
|
||||
|
||||
let repaired_hash = sqlx::query_scalar!(
|
||||
"SELECT lockfile_hash FROM lock_hash WHERE workspace_id = $1 AND path = $2",
|
||||
"test-workspace",
|
||||
path,
|
||||
)
|
||||
.fetch_one(&db)
|
||||
.await?;
|
||||
assert_eq!(repaired_hash, windmill_common::scripts::hash_script(lock));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[sqlx::test(migrations = "../migrations", fixtures("base"))]
|
||||
async fn test_script_endpoints(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
@@ -797,10 +876,12 @@ async fn test_update_script_reports_losing_to_a_concurrent_deploy(
|
||||
|
||||
// What a deploy leaves behind: the old head archived, a new one live at the path.
|
||||
// Copied through a temp table so this does not have to restate every column.
|
||||
sqlx::query("CREATE TEMP TABLE superseding ON COMMIT DROP AS SELECT * FROM script WHERE hash = $1")
|
||||
.bind(head)
|
||||
.execute(&mut *winner)
|
||||
.await?;
|
||||
sqlx::query(
|
||||
"CREATE TEMP TABLE superseding ON COMMIT DROP AS SELECT * FROM script WHERE hash = $1",
|
||||
)
|
||||
.bind(head)
|
||||
.execute(&mut *winner)
|
||||
.await?;
|
||||
sqlx::query("UPDATE superseding SET hash = $1, archived = false, parent_hashes = ARRAY[$2]")
|
||||
.bind(head + 1)
|
||||
.bind(head)
|
||||
@@ -818,7 +899,10 @@ async fn test_update_script_reports_losing_to_a_concurrent_deploy(
|
||||
let resp = tokio::time::timeout(std::time::Duration::from_secs(20), update).await??;
|
||||
let status = resp.status();
|
||||
let body = resp.text().await?;
|
||||
assert_eq!(status, 400, "losing the race should not read as success: {body}");
|
||||
assert_eq!(
|
||||
status, 400,
|
||||
"losing the race should not read as success: {body}"
|
||||
);
|
||||
assert!(
|
||||
body.contains("deployed to concurrently"),
|
||||
"the loser must say it was superseded, not that the script is missing: {body}"
|
||||
|
||||
@@ -39,7 +39,7 @@ use sqlx::{FromRow, Postgres, Transaction};
|
||||
use std::{collections::HashMap, sync::Arc};
|
||||
use windmill_audit::audit_oss::{audit_log, AuditAuthorable};
|
||||
use windmill_audit::ActionKind;
|
||||
use windmill_dep_map::process_relative_imports;
|
||||
use windmill_dep_map::{lock_hash::record_lock_hashes, process_relative_imports};
|
||||
use windmill_dep_map::scoped_dependency_map::ScopedDependencyMap;
|
||||
|
||||
use windmill_common::{
|
||||
@@ -1073,6 +1073,14 @@ fn modules_eq(
|
||||
}
|
||||
}
|
||||
|
||||
/// Recorded for the empty lock a codebase or a language with no lock generation carries as well as
|
||||
/// for a real one: the worker writes `hash_script("")` in the same situation, and a path going from
|
||||
/// a real lock to an empty one has to stop matching what its importers recorded, or they wrongly
|
||||
/// skip rather than merely relock too often.
|
||||
fn lock_hash_entry(path: &str, lock: &str) -> [(String, i64); 1] {
|
||||
[(path.to_string(), hash_script(lock))]
|
||||
}
|
||||
|
||||
async fn create_script_internal<'c>(
|
||||
mut ns: NewScript,
|
||||
w_id: String,
|
||||
@@ -1340,6 +1348,12 @@ async fn create_script_internal<'c>(
|
||||
parent_hash = %p_hash.0,
|
||||
"Skipping no-op script deploy (identical to parent)"
|
||||
);
|
||||
// The version is unchanged, but the row recording its lock's hash may never have
|
||||
// been written — nothing else writes it for a supplied lock, and a path only ever
|
||||
// pushed unchanged would otherwise keep its importers relocking forever.
|
||||
if let Some(lock) = ps.lock.as_deref() {
|
||||
record_lock_hashes(&mut tx, &w_id, &lock_hash_entry(&ns.path, lock)).await?;
|
||||
}
|
||||
return Ok((p_hash.clone(), tx, None, Vec::new()));
|
||||
}
|
||||
|
||||
@@ -1887,6 +1901,13 @@ async fn create_script_internal<'c>(
|
||||
.execute(&mut *tx)
|
||||
.await?;
|
||||
|
||||
// A lock that is not left to a dependency job queues none, so this is the only place its hash
|
||||
// can be recorded. `try_skip_relock` treats a missing hash for an imported script as changed,
|
||||
// so leaving the row out makes every importer of this path relock on every deploy of it.
|
||||
if let Some(lock) = lock.as_deref() {
|
||||
record_lock_hashes(&mut tx, &w_id, &lock_hash_entry(&ns.path, lock)).await?;
|
||||
}
|
||||
|
||||
// Update ci_test_reference table for test scripts
|
||||
// Delete by both new and old path to handle renames
|
||||
let old_path = parent_hashes_and_perms.as_ref().map(|x| x.p_path.as_str());
|
||||
|
||||
@@ -12,6 +12,7 @@ use windmill_api_auth::{
|
||||
};
|
||||
use windmill_api_users::users::WorkspaceInvite;
|
||||
use windmill_common::email_oss::send_email_if_possible;
|
||||
use windmill_dep_map::lock_hash::record_lock_hashes_for_workspace;
|
||||
use windmill_common::usernames::{get_instance_username_or_create_pending, VALID_USERNAME};
|
||||
use windmill_common::webhook::WebhookShared;
|
||||
use windmill_common::{BASE_URL, DB};
|
||||
@@ -7139,7 +7140,16 @@ async fn clone_workspace_runnable_dependencies(
|
||||
.execute(&mut **tx)
|
||||
.await?;
|
||||
|
||||
// Clone dependency_map to preserve import relationships
|
||||
// Recorded so the clone's own relocks have something to match; with no row they record NULL
|
||||
// and nothing in it ever skips. Hashed from the locks the clone holds rather than copied from
|
||||
// the source's rows, which are only as current as the last write to them: one left stale by a
|
||||
// supplied lock deployed before this was recorded names a lock the clone no longer has, and an
|
||||
// importer that resolved against the real one would then skip a relock it needed.
|
||||
record_lock_hashes_for_workspace(tx, target_workspace_id).await?;
|
||||
|
||||
// Deliberately without `imported_lockfile_hash`: it records what an importer resolved against
|
||||
// when it was last locked, which nothing here can establish for the version the clone got.
|
||||
// Left NULL, every importer relocks once and re-anchors both sides to what the clone holds.
|
||||
sqlx::query!(
|
||||
"INSERT INTO dependency_map (workspace_id, importer_path, importer_kind, imported_path, importer_node_id)
|
||||
SELECT $1, importer_path, importer_kind, imported_path, importer_node_id
|
||||
|
||||
@@ -26,4 +26,5 @@ tracing.workspace = true
|
||||
lazy_static.workspace = true
|
||||
chrono.workspace = true
|
||||
itertools.workspace = true
|
||||
futures.workspace = true
|
||||
uuid.workspace = true
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
pub mod ci_tests;
|
||||
#[cfg(feature = "private")]
|
||||
pub mod ci_tests_ee;
|
||||
pub mod lock_hash;
|
||||
pub mod scoped_dependency_map;
|
||||
pub mod trigger_dependents;
|
||||
pub mod workspace_dependencies;
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
use std::collections::HashMap;
|
||||
|
||||
use futures::TryStreamExt;
|
||||
use sqlx::{Postgres, Transaction};
|
||||
use windmill_common::error::Result;
|
||||
use windmill_common::scripts::hash_script;
|
||||
|
||||
/// Records what the lock now at each path hashes to, which is one half of the comparison a relock
|
||||
/// skip makes against what each importer resolved against.
|
||||
///
|
||||
/// Writes any path in `w_id` and checks nothing: callers are responsible for having established
|
||||
/// the caller's access to that workspace. A path repeated in `entries` keeps its last hash.
|
||||
///
|
||||
/// Callers that write the lock itself in the same statement fold the upsert into that statement
|
||||
/// instead; this is for the ones with nothing to fold it into.
|
||||
pub async fn record_lock_hashes(
|
||||
tx: &mut Transaction<'_, Postgres>,
|
||||
w_id: &str,
|
||||
entries: &[(String, i64)],
|
||||
) -> Result<()> {
|
||||
// Postgres rejects a whole statement that resolves a conflict on one key twice, so a path
|
||||
// given more than once keeps its last hash, as it would if the two were written in order.
|
||||
let mut deduped: HashMap<&str, i64> = HashMap::with_capacity(entries.len());
|
||||
for (path, hash) in entries {
|
||||
deduped.insert(path.as_str(), *hash);
|
||||
}
|
||||
if deduped.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
let (paths, hashes): (Vec<String>, Vec<i64>) = deduped
|
||||
.into_iter()
|
||||
.map(|(path, hash)| (path.to_string(), hash))
|
||||
.unzip();
|
||||
// Recording a hash a path already has would still cut a row version, and the no-op push this
|
||||
// is reached from is the mode a git-sync of an unchanged workspace runs in.
|
||||
sqlx::query!(
|
||||
"INSERT INTO lock_hash (workspace_id, path, lockfile_hash)
|
||||
SELECT $1, * FROM UNNEST($2::text[], $3::bigint[])
|
||||
ON CONFLICT (workspace_id, path) DO UPDATE SET lockfile_hash = EXCLUDED.lockfile_hash
|
||||
WHERE lock_hash.lockfile_hash IS DISTINCT FROM EXCLUDED.lockfile_hash",
|
||||
w_id,
|
||||
&paths[..],
|
||||
&hashes[..]
|
||||
)
|
||||
.execute(&mut **tx)
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Records the hash of every live lock in `w_id`, for a workspace whose scripts arrived without
|
||||
/// going through a deploy — a clone, which copies their locks verbatim and so would otherwise hold
|
||||
/// none of the hashes describing them.
|
||||
///
|
||||
/// Carries the same caller obligation as [`record_lock_hashes`].
|
||||
///
|
||||
/// `script.lock` is unbounded and a workspace holds one per script, so the rows are streamed and
|
||||
/// each lock is hashed and dropped before the next arrives; only the hashes accumulate.
|
||||
pub async fn record_lock_hashes_for_workspace(
|
||||
tx: &mut Transaction<'_, Postgres>,
|
||||
w_id: &str,
|
||||
) -> Result<()> {
|
||||
let mut entries: Vec<(String, i64)> = Vec::new();
|
||||
{
|
||||
let mut rows = sqlx::query!(
|
||||
"SELECT DISTINCT ON (path) path, lock FROM script
|
||||
WHERE workspace_id = $1 AND NOT archived AND NOT deleted AND lock IS NOT NULL
|
||||
ORDER BY path, created_at DESC",
|
||||
w_id
|
||||
)
|
||||
.fetch(&mut **tx);
|
||||
|
||||
while let Some(row) = rows.try_next().await? {
|
||||
if let Some(lock) = row.lock {
|
||||
entries.push((row.path, hash_script(&lock)));
|
||||
}
|
||||
}
|
||||
}
|
||||
record_lock_hashes(tx, w_id, &entries).await
|
||||
}
|
||||
Reference in New Issue
Block a user