windmill/docs/duckdb-isolation.md

# 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 patches add

`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. fixes `max_temp_directory_size` the same way, so the ceiling on what may be written
   there cannot be raised either;
3. routes the buffer manager's temporary files through a local file system that
   `disabled_filesystems` does not apply to.

The halves are what make it safe as a set. 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. And it is only
bounded because of (2): DuckDB accepts `SET max_temp_directory_size='100GB'` from any
statement, so a cap a script can lift is not a cap.
`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 neither move the temp directory nor raise the cap on it.

## Bounding the spill

`configure_duckdb_resource_limits` sets `max_temp_directory_size` from
`DUCKDB_MAX_TEMP_DIRECTORY_SIZE`, immediately before it takes the lock. Unset — the
self-hosted default — leaves DuckDB's own default of 90% of the temp volume's free space,
so nothing changes for an install that does not opt in. The cloud sets `6GiB` per worker
pod (`cloudee/cloud/cloud.yml` in `infra-aws`); those pods run a single worker each, so it
is effectively a per-pod cap and is not divided by `NUM_WORKERS`.

Those pods carry no `ephemeral-storage` limit, so what the cap has to stay clear of is the
node's eviction threshold rather than a per-pod one, and the budget is tighter than the
node's size suggests: a worker pod already holds 4.7-6.9 GiB of language-runtime caches and
job dirs before any spill, so a ~100 GB node running 5-6 of them sits at 45-60 GB used at
rest. Every pod on the fullest node spilling to its cap at the same instant would land near
the kubelet's default `nodefs.available<10%`. That is the tail, not the common case, and it
is still bounded where the previous behaviour was not — one query could take all the free
space by itself — but the cap is sized against a mostly-full node, not an empty one. Re-measure
before raising it, and re-measure if the runtime-cache baseline grows.

It is read from the environment rather than passed across the FFI: the cdylib is versioned
separately from the worker and agent workers ship it out of band, so a signature change
would mean an ABI bump and a coordinated redeploy for what is a config value.

The value is DuckDB's spelling, not Kubernetes': `KiB`/`MiB`/`GiB`/`TiB` for 1024-based
units, `KB`/`MB`/`GB`/`TB` for 1000-based. Kubernetes' `6Gi` is not a unit DuckDB knows,
and an unparseable value fails every DuckDB job on the worker rather than being ignored —
a cap that quietly disappeared would be worse. `DUCKDB_MEMORY_LIMIT` is spelled the same
way and DuckDB's parse error says only "memory", so the cap's `SET` is issued on its own
statement: batched together, a bad value in either would produce the same message and name
neither.

Enforcement is graceful. A query that needs more spill than the cap fails with
`Out of Memory Error: failed to offload data block of size … This limit was set by the
'max_temp_directory_size' setting.`, its temp files are removed, and the worker carries on.
Size it from measurement, not from the data: DuckDB compresses temp files, so the logical
spill a given cap permits is well above the number set.

That engine message goes on to suggest `PRAGMA max_temp_directory_size='10GiB'`, which the
lock refuses — advice that was actionable before the cap was frozen and is now a dead end
ending in a second, unrelated-looking permission error. `decode_ffi_error`
(`duckdb_executor.rs`) appends a correction whenever the message appears, on every worker
rather than only isolating ones, since the lock is taken on every connection. It matches
DuckDB's wording verbatim, which
`spilling_past_the_configured_cap_fails_the_query_and_cleans_up` asserts against real engine
output so an engine bump that rewords it fails the test instead of silently dropping the
correction.

The engine emits that same message for **its own default cap**, so which limit was hit
cannot be read off the message: with `DUCKDB_MAX_TEMP_DIRECTORY_SIZE` unset it means the
worker is low on disk, not that anyone configured a ceiling. `spill_cap_hint` takes the
answer from the worker's config instead and says whichever is true — naming the env var on a
worker that never set it would point the reader at a knob that does not exist.

## 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 the windmill commits; `duckdb.tar.gz` is left byte-identical to upstream's and
`crates/libduckdb-sys/windmill/0*.patch` are applied to the extracted sources at build
time, in order, so `git diff` against `duckdb/duckdb-rs` shows the whole delta as text.
`0001` adds the setting and the exemption, `0002` extends the lock to
`max_temp_directory_size`. `crates/libduckdb-sys/windmill/README.md` in the fork carries
the rebase recipe.

### The patches 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 `0001` 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 patches disappearing silently

An unpatched engine compiles, links and passes every existing test. Three things catch it:

- **The fork's build script** asserts each patch's marker string is present in the patched
  sources after applying it, and panics if one 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 also pins that the lock leaves
  neither `temp_directory` nor `max_temp_directory_size` settable, and that a configured
  connection stops at its cap. 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

Both patches are written against `duckdb/duckdb` `v1.5.5` in the shape upstream takes
changes. Besides the engine sources they carry `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. They are `git format-patch` files, so they apply 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/0*.patch
GEN=ninja make && ./build/release/test/unittest "test/sql/settings/lock_temp_directory*"
```

`0001` 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.

**`0002` is windmill-only and is not on #24694.** It is kept as a separate commit rather
than folded into `0001` for exactly that reason: `0001` stays byte-identical to what the
open PR carries, and `0002` applies to it verbatim as a second commit whenever a human
chooses to push it. That is the one sanctioned divergence; when it is pushed, say so here
and in the fork's `windmill/README.md`, and the "same change" rule below covers it too.

**[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.

`0001` 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.