Files
hugocasaandClaude Opus 5 17ba521c35 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>
2026-09-02 19:20:49 +02:00

214 lines
6.4 KiB
Rust

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;
use std::collections::HashMap;
use std::path::{Component, Path, PathBuf};
use serde_json::value::RawValue;
use sqlx::types::Json;
use uuid::Uuid;
use windmill_common::error;
use windmill_common::scripts::ScriptLang;
use windmill_common::utils::WarnAfterExt;
use windmill_common::workspace_dependencies::{
WorkspaceDependencies, WorkspaceDependenciesPrefetched,
};
use windmill_parser_ts::parse_expr_for_imports;
fn try_normalize(path: &Path) -> Option<PathBuf> {
let mut ret = PathBuf::new();
for component in path.components() {
match component {
Component::Prefix(..) | Component::RootDir => return None,
Component::CurDir => {}
Component::ParentDir => {
if !ret.pop() {
return None;
}
}
Component::Normal(c) => {
ret.push(c);
}
}
}
Some(ret)
}
fn parse_ts_relative_imports(
raw_code: &str,
script_path: &str,
) -> windmill_common::error::Result<Vec<String>> {
let mut relative_imports = vec![];
let r = parse_expr_for_imports(raw_code, true)?;
for import in r {
let import = import.trim_end_matches(".ts");
if import.starts_with("/") {
relative_imports.push(import.trim_start_matches("/").to_string());
} else if import.starts_with(".") {
let normalized = try_normalize(std::path::Path::new(&format!(
"{}/../{}",
script_path, import
)));
if let Some(normalized) = normalized {
let normalized = normalized.to_str().unwrap().to_string();
relative_imports.push(normalized);
} else {
tracing::error!("error canonicalizing path: {script_path} with import {import}");
}
}
}
Ok(relative_imports)
}
pub fn extract_relative_imports(
raw_code: &str,
script_path: &str,
language: &Option<ScriptLang>,
) -> Option<Vec<String>> {
match language {
#[cfg(feature = "python")]
Some(ScriptLang::Python3) => {
windmill_parser_py_imports::parse_relative_imports(&raw_code, script_path).ok()
}
Some(ScriptLang::Bun) | Some(ScriptLang::Bunnative) | Some(ScriptLang::Deno) => {
parse_ts_relative_imports(&raw_code, script_path).ok()
}
_ => None,
}
}
pub fn extract_referenced_paths(
raw_code: &str,
script_path: &str,
language: Option<ScriptLang>,
) -> Option<Vec<String>> {
let mut referenced_paths = vec![];
if let Some(wk_deps_refs) = language
.and_then(|l| {
windmill_common::scripts::extract_workspace_dependencies_annotated_refs(
&l,
raw_code,
script_path,
)
})
.map(|r| r.external)
{
let l = language.expect("should be some");
for wk_deps_ref in wk_deps_refs {
if let Some(path) = WorkspaceDependencies::to_path(&Some(wk_deps_ref), l).ok() {
referenced_paths.push(path);
};
}
} else if let (Some(l), true /* Only if it is not blacklisted */) = (
language,
WorkspaceDependenciesPrefetched::is_external_references_permitted(script_path),
) {
// we assume all runnables without annotated dependencies reference default dependencies file.
WorkspaceDependencies::to_path(&None, l)
.ok()
.inspect(|p| referenced_paths.push(p.to_owned()));
}
if let Some(relative_imports) = extract_relative_imports(raw_code, script_path, &language) {
referenced_paths.extend(relative_imports);
}
if referenced_paths.is_empty() {
None
} else {
Some(referenced_paths)
}
}
pub async fn process_relative_imports(
db: &sqlx::Pool<sqlx::Postgres>,
_job_id: Option<Uuid>,
args: Option<&Json<HashMap<String, Box<RawValue>>>>,
w_id: &str,
script_path: &str,
parent_path: Option<String>,
deployment_message: Option<String>,
code: &str,
script_lang: &Option<ScriptLang>,
permissioned_as_email: &str,
created_by: &str,
permissioned_as: &str,
) -> error::Result<()> {
use scoped_dependency_map::ScopedDependencyMap;
use trigger_dependents::trigger_dependents_to_recompute_dependencies;
// TODO: Should be moved into handle_dependency_job body to be more consistent with how flows and apps are handled
{
let mut tx = db.begin().await?;
let mut dependency_map = ScopedDependencyMap::fetch_maybe_rearranged(
&w_id,
script_path,
"script",
&parent_path,
db,
)
.await?;
tx = dependency_map
.patch(
extract_referenced_paths(&code, script_path, *script_lang),
// Ideally should be None, but due to current implementation will use empty string to represent None.
"".into(),
tx,
)
.await?;
dependency_map.dissolve(tx).await.commit().await?;
}
{
let mut already_visited = args
.map(|x| {
x.get("already_visited")
.map(|v| serde_json::from_str::<Vec<String>>(v.get()).ok())
.flatten()
})
.flatten()
.unwrap_or_default();
let importers = ScopedDependencyMap::get_dependents(script_path, w_id, db).await?;
already_visited.push(script_path.to_string());
match tokio::time::timeout(
core::time::Duration::from_secs(60),
Box::pin(trigger_dependents_to_recompute_dependencies(
w_id,
importers,
deployment_message,
parent_path,
permissioned_as_email,
created_by,
permissioned_as,
db,
already_visited,
)),
)
.warn_after_seconds(10)
.await
{
Ok(Err(e)) => {
tracing::error!(%e, "error triggering dependents to recompute dependencies")
}
Err(e) => {
tracing::error!(%e, "triggering dependents to recompute dependencies has timed out")
}
_ => {}
}
}
Ok(())
}