feat: keep duckdb spilling behind the local-filesystem fence (#10607)

* fix: explain duckdb failures caused by job isolation

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: apply the isolation policy to the schema-sync pre-pass

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore: bump ee ref for the out-of-memory hint wording

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: bump the bundled DuckDB engine to 1.5.5

The 1.5.5 duckdb crate no longer hands back a 96-bit `rust_decimal`, so a
DECIMAL wider than that renders instead of panicking inside an `extern "C"`
frame — which, being unable to unwind, aborted the whole worker process and
left the job running as a zombie. `SELECT
'1234567890123456789012345678.9012345678'::DECIMAL(38, 10)` was enough.

Adapting to the crate's API: `Value` is now `#[non_exhaustive]` and gained
`UHugeInt` and `Geometry`, and `rust_decimal` became an optional feature that
the `decimal`/`numeric` argument path still needs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: address review findings on the duckdb bump

Run the FFI crate's own tests in CI: it is excluded from the workspace, so the
`cargo test --all` in backend-test never reached them and the new guard against
the worker-aborting DECIMAL would not have run. build_dev.sh now honors a
caller-pinned CARGO_TARGET_DIR so the test build reuses that compile instead of
building the bundled engine a second time.

Also pin UHUGEINT rendering, and correct the rust_decimal rationale —
`Decimal::new` is public without the feature, so the reason is that the feature
reproduces the exact binding the crate used to derive, not that nothing else can.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: address review nits on the duckdb bump

Name the unsupported DuckDB type rather than dumping the value, which may be
arbitrarily large or hold data that does not belong in an error message, and
say which column it came from.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore: pin the ee ref to the narrowed duckdb extension allowlist

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat: keep duckdb spilling behind the local-filesystem fence

* chore: repin the duckdb fork after adding the reset-test exclusion

* docs: stop claiming the duckdb patch has been filed upstream

* docs: point the backend duckdb bullet at the fork's rationale

* fix: place lock_temp_directory so no existing struct member moves

* fix: skip the extension-load guard when the repo is unreachable

* refactor: trim the fork comments and fail the extension guard in CI

* chore: repin the duckdb fork onto upstream duckdb-rs main

* fix: keep the engine patch applying on a CRLF checkout

* docs: link the upstream issue tracking the underlying problem

* chore: repin the duckdb fork onto the patch as filed upstream

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore: update ee-repo-ref to 88568d11162ffa11723e7955e613224bab4f0568

This commit updates the EE repository reference after PR #720 was merged in windmill-ee-private.

Previous ee-repo-ref: 22f075c1164d9dd5a3ba92d682905aabd071d273

New ee-repo-ref: 88568d11162ffa11723e7955e613224bab4f0568

Automated by sync-ee-ref workflow.

* chore: repin the duckdb fork onto the cmake/fmt build fix

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test: pin the immutability half of lock_temp_directory

The spill test proves the exemption works; nothing proved the lock that makes
it sound. A rebase could drop the refusals and leave every other tripwire green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: windmill-internal-app[bot] <windmill-internal-app[bot]@users.noreply.github.com>

Dockerfile

@@ -50,7 +50,10 @@ RUN apt-get update && apt-get install -y clang=1:19.0* libclang-dev=1:19.0* cmak
 
 COPY ./backend/windmill-duckdb-ffi-internal .
 
+# The `duckdb` crate comes from a git dependency (a fork carrying an engine patch),
+# which cargo checks out under $CARGO_HOME/git rather than the registry cache.
 RUN --mount=type=cache,target=/usr/local/cargo/registry \
+    --mount=type=cache,target=/usr/local/cargo/git \
     --mount=type=cache,target=$SCCACHE_DIR,sharing=locked \
     cargo build --release -p windmill_duckdb_ffi_internal
 

backend/CLAUDE.md

@@ -15,6 +15,8 @@
   worktrees (keyed by the crate's `Cargo.lock`), so a fresh worktree reuses it and
   the build is near-instant — you don't pay the full compile per worktree. Editing
   the FFI crate's own source falls back to an isolated per-worktree `./target`.
+  The engine is a **patched fork** of duckdb-rs, not the crates.io crate — read
+  `docs/duckdb-isolation.md` before bumping it or touching the isolation transform.
 - **Running data pipelines (DuckLake) from source**: see the section below — a plain build
   advertises the `duckdb` tag but cannot execute DuckDB scripts and has no working S3 proxy.
 

backend/ee-repo-ref.txt

@@ -1 +1 @@
-f8d523195e40fd1d740595dcab6ce5cdc1bdbf09
+88568d11162ffa11723e7955e613224bab4f0568

backend/windmill-duckdb-ffi-internal/Cargo.toml

@@ -10,7 +10,14 @@ chrono = "0.4.41"
 # used to derive itself, so a `decimal`/`numeric` job argument still binds to the
 # identical DECIMAL. Building one by hand would mean reimplementing that
 # f64 → (mantissa, scale) inference.
-duckdb = { version = "1.10505.0", features = ["bundled", "rust_decimal"] }
+#
+# Not crates.io: a fork of duckdb-rs 1.10505.0 carrying one patch to the bundled engine —
+# see docs/duckdb-isolation.md for why, and for the exit path back to the crates.io crate.
+# Pinned by rev, not branch, so `cargo update` cannot move the engine under us.
+duckdb = { git = "https://github.com/windmill-labs/duckdb-rs", rev = "475383b71cde3d009ca6eaa0cd733faa52bc7393", features = [
+    "bundled",
+    "rust_decimal",
+] }
 regex = "1"
 rust_decimal = "1.37.2"
 serde = { version = "1.0", features = ["derive"] }

backend/windmill-duckdb-ffi-internal/build_dev.sh

@@ -42,4 +42,11 @@ fi
 
 CARGO_NET_GIT_FETCH_WITH_CLI=true cargo build --release -p windmill_duckdb_ffi_internal
 mkdir -p ../target/debug/
-cp "$CARGO_TARGET_DIR/release/"libwindmill_duckdb_ffi_internal.* ../target/debug/
+# Install through a rename rather than writing over the destination: a running worker
+# has the cdylib mapped, and rewriting those pages under it kills the process with
+# SIGBUS. A rename swaps the directory entry and leaves the old inode alone.
+for artifact in "$CARGO_TARGET_DIR/release/"libwindmill_duckdb_ffi_internal.*; do
+  dest="../target/debug/$(basename "$artifact")"
+  cp "$artifact" "$dest.tmp"
+  mv -f "$dest.tmp" "$dest"
+done

backend/windmill-duckdb-ffi-internal/src/lib.rs

@@ -1277,3 +1277,113 @@ fn string_to_duckdb_time(s: &str) -> Result<duckdb::types::Value, String> {
         time.num_seconds_from_midnight() as i64,
     ))
 }
+
+// The bundled engine is not stock (docs/duckdb-isolation.md). These two guard the ways an
+// engine bump can drop the patch without anything else noticing.
+#[cfg(test)]
+mod patched_engine_tests {
+    /// Too large to be answered within the `memory_limit` `spill_attempt` sets, so the engine
+    /// has to reach the temp directory to finish it.
+    const SPILLING_QUERY: &str = "CREATE TABLE t AS SELECT * FROM range(1000000)";
+
+    fn spill_attempt(temp_dir: &std::path::Path, lock: bool) -> Result<(), String> {
+        let conn = duckdb::Connection::open_in_memory().unwrap();
+        conn.execute_batch(&format!(
+            "SET threads=2; SET memory_limit='2MB'; SET temp_directory='{}';",
+            temp_dir.display()
+        ))
+        .unwrap();
+        if lock {
+            conn.execute_batch("SET lock_temp_directory=true;")
+                .map_err(|e| format!("lock_temp_directory rejected: {e}"))?;
+        }
+        conn.execute_batch("SET disabled_filesystems='LocalFileSystem';")
+            .unwrap();
+        conn.execute_batch(SPILLING_QUERY)
+            .map_err(|e| e.to_string())
+    }
+
+    #[test]
+    fn locking_the_temp_directory_keeps_spilling_available() {
+        let temp_dir = std::env::temp_dir().join(format!(
+            "windmill_duckdb_spill_{}_{:?}",
+            std::process::id(),
+            std::thread::current().id()
+        ));
+        std::fs::create_dir_all(&temp_dir).unwrap();
+
+        // Without the lock the fence reaches the buffer manager too, which is both the
+        // stock engine's behaviour and the proof that this query really does spill —
+        // otherwise the assertion below would pass on an unpatched engine.
+        let unlocked = spill_attempt(&temp_dir, false).expect_err("query did not need to spill");
+        assert!(
+            unlocked.contains("LocalFileSystem has been disabled"),
+            "expected the fence to stop the spill, got: {unlocked}"
+        );
+
+        let locked = spill_attempt(&temp_dir, true);
+        std::fs::remove_dir_all(&temp_dir).ok();
+        locked.expect("a locked temp directory must stay usable behind disabled_filesystems");
+    }
+
+    /// The exemption above is only sound while the directory cannot be moved: a script that
+    /// repointed it would get a write primitive through the very fence it is exempt from. A
+    /// rebase can drop that half of the patch while leaving the spill test green, so pin it.
+    #[test]
+    fn locking_the_temp_directory_makes_it_immutable() {
+        let conn = duckdb::Connection::open_in_memory().unwrap();
+        conn.execute_batch("SET temp_directory='/tmp/windmill_duckdb_lock_probe';")
+            .unwrap();
+        conn.execute_batch("SET lock_temp_directory=true;")
+            .expect("the patched engine must accept lock_temp_directory");
+
+        for stmt in [
+            "SET temp_directory='/tmp/windmill_duckdb_elsewhere';",
+            "RESET temp_directory;",
+            "SET lock_temp_directory=false;",
+            "RESET lock_temp_directory;",
+        ] {
+            let err = conn.execute_batch(stmt).expect_err(&format!(
+                "`{stmt}` must be refused once the directory is locked"
+            ));
+            assert!(
+                err.to_string().contains("locked"),
+                "expected a lock refusal for `{stmt}`, got: {err}"
+            );
+        }
+    }
+
+    // The engine loads the *prebuilt* extensions from extensions.duckdb.org, which are
+    // compiled against the struct layouts of the release it claims to be. A patch that
+    // moves a member those extensions reach does not fail — the extension reads the
+    // neighbouring field and LOAD blocks forever — so this is a timeout, not an assertion.
+    #[test]
+    fn prebuilt_extensions_still_load() {
+        let conn = duckdb::Connection::open_in_memory().unwrap();
+        // Fetching the extension needs egress, which loading it back — the part this guards —
+        // does not. Skipping keeps a developer offline from seeing a failure they can't act on,
+        // but never in CI, where a skip and a pass would be indistinguishable.
+        if let Err(e) = conn.execute_batch("INSTALL httpfs;") {
+            assert!(
+                std::env::var_os("CI").is_none(),
+                "could not install httpfs, so the extension-load guard did not run: {e}"
+            );
+            eprintln!("skipping: INSTALL httpfs failed ({e})");
+            return;
+        }
+        let (tx, rx) = std::sync::mpsc::channel();
+        std::thread::spawn(move || {
+            tx.send(
+                conn.execute_batch("LOAD httpfs;")
+                    .map_err(|e| e.to_string()),
+            )
+        });
+        match rx.recv_timeout(std::time::Duration::from_secs(120)) {
+            Ok(r) => r.expect("could not load httpfs"),
+            Err(_) => panic!(
+                "LOAD httpfs did not return: the patched engine's struct layout no longer matches \
+                 the release the prebuilt extensions were built against"
+            ),
+        }
+    }
+}

backend/windmill-worker/src/duckdb_executor.rs

@@ -1516,6 +1516,10 @@ pub async fn do_duckdb(
                 probe_blocks,
             )
             .await?;
+            // The probe opens its own DuckDB connection, so it needs the same
+            // connection policy as the main query below.
+            #[cfg(feature = "private")]
+            let probe_blocks = crate::duckdb_isolation_ee::apply_duckdb_isolation(probe_blocks)?;
             let alter_ddl = compute_sync_alter_ddl(
                 probe_blocks,
                 job_args.clone(),
@@ -1844,6 +1848,8 @@ pub async fn do_duckdb(
     match result {
         Ok(result) => Ok(result),
         Err(e) => {
+            #[cfg(feature = "private")]
+            let e = crate::duckdb_isolation_ee::explain_error(e);
             // Passwords might appear in the error message — and, for the
             // structured data-test failure, in sampled row data read from an
             // attached database — so every outgoing error is sanitized here.

docker/RHEL8/Dockerfile

@@ -86,6 +86,7 @@ RUN --mount=type=cache,target=/usr/local/cargo/registry \
     CARGO_NET_GIT_FETCH_WITH_CLI=true cargo build --release --features "$features"
 
 RUN --mount=type=cache,target=/usr/local/cargo/registry \
+    --mount=type=cache,target=/usr/local/cargo/git \
     cd windmill-duckdb-ffi-internal && \
     CARGO_NET_GIT_FETCH_WITH_CLI=true cargo build --release
 

docker/RHEL9/Dockerfile

@@ -86,6 +86,7 @@ RUN --mount=type=cache,target=/usr/local/cargo/registry \
     CARGO_NET_GIT_FETCH_WITH_CLI=true cargo build --release --features "$features"
 
 RUN --mount=type=cache,target=/usr/local/cargo/registry \
+    --mount=type=cache,target=/usr/local/cargo/git \
     cd windmill-duckdb-ffi-internal && \
     CARGO_NET_GIT_FETCH_WITH_CLI=true cargo build --release
 

docs/duckdb-isolation.md

@@ -0,0 +1,155 @@
+# DuckDB isolation and the patched engine
+
+DuckDB runs **in-process** in the worker, through the FFI cdylib in
+`backend/windmill-duckdb-ffi-internal`. It is therefore the one language that the
+nsjail/unshare isolation cannot confine: there is no child process to jail. On a worker
+that isolates jobs, `windmill-worker/src/duckdb_isolation_ee.rs` mirrors that policy at
+the connection level instead, rewriting the block list handed to the engine so it starts
+with three irreversible settings:
+
+```sql
+SET lock_temp_directory=true;
+SET disabled_filesystems='LocalFileSystem';
+SET allow_community_extensions=false;
+```
+
+`disabled_filesystems` is the fence. `lock_temp_directory` is what keeps out-of-core
+execution working behind it, and it is **not a stock DuckDB setting** — see below.
+
+## Why the engine had to change
+
+DuckDB spills through the same file system a query would use to read `/etc/hostname`, so
+`disabled_filesystems='LocalFileSystem'` also stops the buffer manager. Every query whose
+working set outgrows `memory_limit` then fails instead of going out-of-core. No
+combination of stock settings gives "spill, but no other local file access":
+
+- `allowed_directories` / `allowed_paths` enforce nothing while `enable_external_access`
+  is true — `DBConfig::CanAccessFile` returns early.
+- Turning `enable_external_access` off does preserve spilling (startup adds the temp
+  directory to `allowed_directories`), but the postgres and mysql extensions refuse to
+  `ATTACH` on that flag alone, with no allowlist entry able to re-permit it. That would
+  take DuckLake catalogs and datatables down with it.
+- A remote `temp_directory` is impossible: DuckDB cannot open an HTTP file for both
+  reading and writing, and the attempt aborts the process.
+
+The engine is also not arranged the way the layering suggests. `FileSystem::GetLocal(db)`
+looks like an escape hatch — it is what secret storage and extension installation use —
+but `LocalDatabaseFileSystem::GetFileSystem()` re-applies the `disabled_filesystems`
+check by hand, so it is not one. (That is also why extension autoload broke behind the
+fence in DuckDB 1.5.3, which `EXTENSION_ALLOWLIST` and `PRELOAD_EXTENSIONS` exist to work
+around.) The buffer manager, meanwhile, reaches its temp files through
+`FileSystem::GetFileSystem(db)`, an opener-wrapped virtual file system, so both the path
+allowlist and the disable apply to it.
+
+## What the patch adds
+
+`lock_temp_directory` (BOOLEAN, default false, global, irreversible). Setting it:
+
+1. fixes `temp_directory` in place — it can no longer be modified, and the setting itself
+   cannot be unset or reset;
+2. routes the buffer manager's temporary files through a local file system that
+   `disabled_filesystems` does not apply to.
+
+The two halves are what make it safe as a pair. The exemption is only sound because the
+directory can no longer be redirected: without (1), a script could point `temp_directory`
+at an arbitrary path and get a blind write primitive through the exemption.
+`allowed_directories` still applies either way.
+
+The isolation transform therefore emits `SET lock_temp_directory=true` **before**
+`disabled_filesystems`, while the local file system is still reachable, and after the FFI
+has already pointed `temp_directory` at the job dir
+(`configure_duckdb_resource_limits`). Everything a script authored runs after all three
+settings, so a script can never move the temp directory.
+
+## Where the patched engine comes from
+
+`backend/windmill-duckdb-ffi-internal` depends on
+[`windmill-labs/duckdb-rs`](https://github.com/windmill-labs/duckdb-rs), branch
+`windmill/duckdb-1.5.5`, pinned by rev. That fork is duckdb-rs 1.10505.0 (DuckDB 1.5.5)
+plus one commit; `duckdb.tar.gz` is left byte-identical to upstream's and the patch is
+applied to the extracted sources at build time, so `git diff` against `duckdb/duckdb-rs`
+shows the whole delta as text. `crates/libduckdb-sys/windmill/README.md` in the fork
+carries the rebase recipe.
+
+### The patch may not move a struct member
+
+The engine loads the **prebuilt** extensions from `extensions.duckdb.org` — httpfs, azure,
+postgres, ducklake, parquet, and the rest of `EXTENSION_ALLOWLIST` — which are compiled
+against the struct layouts of the release the engine claims to be. A patch that shifts a
+member those extensions reach breaks them, and not loudly: the extension reads the
+neighbouring field and `LOAD httpfs` **blocks forever** rather than failing, so every
+DuckDB job hangs until its timeout.
+
+The first revision of this patch declared its new `DatabaseInstance` member next to
+`local_db_file_system`, which pushed `create_api_v1` — the extension C-API entry point —
+8 bytes along, and every extension load hung. It is declared last for that reason, and
+`lock_temp_directory` is declared at the end of `DBConfigOptions`' run of bools so it takes
+a byte of padding the struct already had.
+
+The invariant is **no pre-existing member changes offset**, which is strictly stronger than
+`sizeof` holding — the first attempt at that field went in next to `temporary_directory`,
+kept `sizeof(DBConfigOptions)` at 920 by consuming interior padding, and still slid four
+bools along by a byte each. A rebase must diff the full record layouts
+(`clang++ -Xclang -fdump-record-layouts`, recipe in the fork's README), not the sizes.
+`prebuilt_extensions_still_load` in the FFI crate is the end-to-end guard — a timeout, not
+an assertion, because the failure mode is a hang — but it covers extension *entry*, not
+every field an extension reads, so it does not substitute for the layout diff.
+
+Alternatives considered and rejected: vendoring a patched `libduckdb-sys` in this repo
+would put a 6 MB binary tarball into windmill's history on every engine bump and would
+need the FFI builder stage in `Dockerfile` to copy more than
+`backend/windmill-duckdb-ffi-internal`; linking a prebuilt engine through
+`DUCKDB_LIB_DIR` would drop the `bundled` build, so every developer and every CI job
+would need a patched `libduckdb` on the box before `build_dev.sh` could work.
+
+## What stops the patch disappearing silently
+
+An unpatched engine compiles, links and passes every existing test. Three things catch it:
+
+- **The fork's build script** asserts a marker string is present in the patched sources
+  after applying the patch, and panics if the patch no longer applies.
+- **`patched_engine_tests` in the FFI crate** runs a query that must spill, first
+  without the lock (asserting the fence stops it, which is what proves the query really
+  spills) and then with it (asserting it succeeds). It fails outright on a stock engine,
+  where the `SET` is an unrecognized parameter. CI runs it via the FFI crate's own
+  `cargo test` step in `backend-test.yml`.
+- **The transform itself fails closed**: on a stock engine `SET lock_temp_directory=true`
+  errors, so every DuckDB job on an isolating worker fails immediately rather than
+  quietly losing the ability to spill.
+
+## Upstream
+
+The patch is written against `duckdb/duckdb` `v1.5.5` in the shape upstream takes changes.
+Besides the engine sources it carries `src/common/settings.json` (the settings generator's
+input, regenerated with `scripts/generate_settings.py` and formatted with clang-format
+11.0.1), two sqllogictests under `test/sql/settings/`, and one line in
+`test/api/test_reset.cpp` — that test requires every setting to survive a `RESET`, so an
+irreversible one has to join the exclusion list `disabled_filesystems` and
+`lock_configuration` are already on. It is a `git format-patch` file, so it applies with
+`git am`:
+
+```bash
+git clone https://github.com/duckdb/duckdb.git && cd duckdb && git checkout v1.5.5
+git am < .../crates/libduckdb-sys/windmill/0001-lock-temp-directory.patch
+GEN=ninja make && ./build/release/test/unittest "test/sql/settings/lock_temp_directory*"
+```
+
+It is filed as **[duckdb/duckdb#24694](https://github.com/duckdb/duckdb/pull/24694)**,
+against `main` rather than `v1.5.5`. Filing was a human decision on purpose: their
+`CONTRIBUTING.md` asks contributors not to submit LLM-generated pull requests, to discuss
+the change on GitHub first, and to run CI on a fork before opening one.
+
+**[duckdb/duckdb#24695](https://github.com/duckdb/duckdb/issues/24695)** states the
+underlying problem without prescribing a fix, and is where a different design would land:
+`lock_temp_directory` is one shape, and an enforceable `allowed_directories` would solve it
+equally well. Watch the issue rather than the PR for whether this fork can be retired.
+
+The vendored copy and #24694 are the same change and must stay that way. They differ only
+where the base version forces it: the `DUCKDB_SETTING_ALIAS` indices, `vfs` versus
+`db.config.file_system` in `FileSystem::GetLocal`, and the sqllogictest directory
+placeholder. Nothing else should diverge. Note that the build applies neither the `test/`
+hunks nor `settings.json` (`NOT_IN_TARBALL` in `build_windmill_patch.rs`), so drift in
+those is invisible here and only surfaces when #24694 is rebased.
+
+Until it lands in a released DuckDB the fork is the delivery mechanism, and the exit path
+is to delete `crates/libduckdb-sys/windmill/` and go back to the crates.io crate.