From 7e2f1afffb4982099870e7ce32592cf3a447e513 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 22 Jul 2026 22:42:35 +0200 Subject: [PATCH] fix(python,windows): cross-platform cross-process wheel-install lock (#10264) * fix(python,windows): cross-platform cross-process wheel-install lock The advisory lock that serializes concurrent uv installs into a shared Python wheel-cache dir was gated `#[cfg(unix)]` and used `nix::fcntl::flock` directly, so on Windows there was no cross-process serialization at all. Multiple agents running as services on one Windows host share a single per-user cache dir (`.../Temp/windmill/cache/python_/`); when several jobs install the same package at once their uv processes clobber each other's atomic renames, surfacing as "no .dist-info directory", "RECORD ... cannot find the file specified", and "failed to rename ... os error 2" install failures. Replace the unix-only flock with `fs4`'s cross-platform advisory lock (flock on unix, LockFileEx on windows). Unix behavior is unchanged (same flock syscall, whole-file, released on handle close/process death); Windows now gets a real per-package cross-process lock so co-located agents serialize their installs instead of corrupting the shared cache. Fixes WIN-2225 Co-Authored-By: Claude Opus 4.8 (1M context) * chore(worker): drop now-unused nix `fs` feature The `fs` feature was only pulled in for `nix::fcntl::flock`, which the previous commit replaced with `fs4`. Remaining nix usages need only `user` (plus the workspace-inherited `process`/`signal`). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- backend/Cargo.lock | 1 + backend/Cargo.toml | 1 + backend/windmill-worker/Cargo.toml | 6 ++- .../windmill-worker/src/python_executor.rs | 51 ++++++++++--------- 4 files changed, 34 insertions(+), 25 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index fad8a28afa..f1966eb6e8 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -16378,6 +16378,7 @@ dependencies = [ "dotenv", "eventsource-stream", "flume", + "fs4", "futures", "gcp_auth", "git-version", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index aeecb11d60..683be6972d 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -588,6 +588,7 @@ rust_decimal = { version = "^1", features = ["db-postgres", "serde-float"]} jsonwebtoken = "8.3.0" pem = "3.0.1" nix = { version = "0.27.1", features = ["process", "signal"] } +fs4 = "0.13" tinyvector = { git = "https://github.com/windmill-labs/tinyvector", rev = "20823b94c20f2b9093f318badd24026cf54dcc85" } hf-hub = "0.4.3" tokenizers = "0.14.1" diff --git a/backend/windmill-worker/Cargo.toml b/backend/windmill-worker/Cargo.toml index bdc6ec49d6..99647ba81f 100644 --- a/backend/windmill-worker/Cargo.toml +++ b/backend/windmill-worker/Cargo.toml @@ -115,9 +115,11 @@ hmac.workspace = true pem = { workspace = true, optional = true } rsa = { workspace = true, optional = true } urlencoding.workspace = true -# `fs` adds flock(2) for the cross-process Python install lock (shared cache mounts); # `user` adds geteuid(2) to verify ownership of the ansible socket-dir root -nix = { workspace = true, features = ["fs", "user"] } +nix = { workspace = true, features = ["user"] } +# Cross-platform advisory file lock (flock on unix, LockFileEx on windows) for the +# cross-process Python install lock into shared wheel-cache dirs. +fs4 = { workspace = true } bytes.workspace = true reqwest.workspace = true reqwest-middleware.workspace = true diff --git a/backend/windmill-worker/src/python_executor.rs b/backend/windmill-worker/src/python_executor.rs index a3ae036894..13e3749fbd 100644 --- a/backend/windmill-worker/src/python_executor.rs +++ b/backend/windmill-worker/src/python_executor.rs @@ -2832,11 +2832,12 @@ pub async fn handle_python_reqs( }; // Cross-process advisory lock. Best-effort: if the filesystem doesn't - // support flock we log and proceed — verify_wheel_record + job retry - // still guard correctness, just without the dedup. - #[cfg(unix)] + // support locking we log and proceed — verify_wheel_record + job retry + // still guard correctness, just without the dedup. Cross-platform + // (flock on unix, LockFileEx on windows) so agents sharing a wheel-cache + // dir on a Windows host serialize just as they do on unix. let _venv_file_lock: Option = { - use std::os::unix::io::AsRawFd; + use fs4::fs_std::FileExt; let lock_path = format!("{venv_p}.lock"); if let Some(parent) = std::path::Path::new(&lock_path).parent() { let _ = std::fs::create_dir_all(parent); @@ -2844,17 +2845,17 @@ pub async fn handle_python_reqs( match std::fs::OpenOptions::new().create(true).write(true).open(&lock_path) { Ok(f) => { // Bounded wait: a holder that crashes releases the lock (the - // kernel drops it on fd close), but a live-but-stuck holder + // OS drops it on handle close), but a live-but-stuck holder // (e.g. uv wedged on a hung mount) would otherwise block us // forever. After the cap, proceed degraded rather than hang — // verify_wheel_record + retry still guard correctness. const MAX_WAIT: std::time::Duration = std::time::Duration::from_secs(300); let waited_since = std::time::Instant::now(); loop { - match nix::fcntl::flock(f.as_raw_fd(), nix::fcntl::FlockArg::LockExclusiveNonblock) { - Ok(()) => break Some(f), - // EWOULDBLOCK == EAGAIN on Linux: another holder has the lock. - Err(nix::errno::Errno::EWOULDBLOCK) => { + match f.try_lock_exclusive() { + Ok(true) => break Some(f), + // Another holder has the lock. + Ok(false) => { if waited_since.elapsed() >= MAX_WAIT { tracing::warn!( workspace_id = %w_id, @@ -2876,7 +2877,7 @@ pub async fn handle_python_reqs( Err(e) => { tracing::warn!( workspace_id = %w_id, - "could not flock {lock_path}, proceeding without cross-process install lock: {e}" + "could not lock {lock_path}, proceeding without cross-process install lock: {e}" ); break Some(f); } @@ -3782,14 +3783,14 @@ mod tests { ); } - #[cfg(unix)] #[tokio::test] async fn test_venv_file_lock_excludes_across_descriptions() { - // The cross-process layer: flock on a sibling `.lock` excludes a second - // independent open file description (i.e. another worker process) while - // held, and frees it on close. Mirrors the loop in handle_python_reqs. - use nix::fcntl::{flock, FlockArg}; - use std::os::unix::io::AsRawFd; + // The cross-process layer: an advisory lock on a sibling `.lock` excludes a + // second independent open file handle (i.e. another worker process) while + // held, and frees it on close. Mirrors the loop in handle_python_reqs and + // must hold on every platform (flock on unix, LockFileEx on windows) — a + // Windows host running several agents against one wheel cache relies on it. + use fs4::fs_std::FileExt; let dir = std::env::temp_dir().join("wm_venv_lock_test"); std::fs::create_dir_all(&dir).unwrap(); @@ -3800,24 +3801,28 @@ mod tests { .write(true) .open(&lock_path) .unwrap(); - flock(f1.as_raw_fd(), FlockArg::LockExclusiveNonblock).unwrap(); + assert!( + f1.try_lock_exclusive().unwrap(), + "first holder must acquire the lock" + ); - // A second descriptor (stand-in for another process) cannot take it. + // A second handle (stand-in for another process) cannot take it. let f2 = std::fs::OpenOptions::new() .create(true) .write(true) .open(&lock_path) .unwrap(); - assert_eq!( - flock(f2.as_raw_fd(), FlockArg::LockExclusiveNonblock), - Err(nix::errno::Errno::EWOULDBLOCK), + assert!( + !f2.try_lock_exclusive().unwrap(), "a second holder must be blocked while the lock is held" ); // Releasing the first lets the second acquire it. drop(f1); - flock(f2.as_raw_fd(), FlockArg::LockExclusiveNonblock) - .expect("lock must be acquirable once the holder releases it"); + assert!( + f2.try_lock_exclusive().unwrap(), + "lock must be acquirable once the holder releases it" + ); drop(f2); let _ = std::fs::remove_file(&lock_path);