Merge source scan and conflict-check aren't pinned to one Delta snapshot — silent lost update under concurrent writes
## Summary
A duckrun incremental **merge** performs **two independent reads of the same Delta table** with no shared snapshot pin. Under heavy concurrent writes a commit can land between them, so the merge enforces against a different version than the one its source rows were computed from — **silently producing a wrong result, with no error raised**.
## The two unpinned reads
1. **Source scan (snapshot A).** The model's source rows come from DuckDB. For an ordinary incremental model the source derives from `{{ this }}`, which duckrun registers as `select * from delta_scan('<path>')` ([impl.py L191](https://github.com/djouallah/duckrun/blob/main/dbt/adapters/duckrun/impl.py#L191)).
2. **Merge target / conflict check (snapshot B).** [`merge_delta`](https://github.com/djouallah/duckrun/blob/main/dbt/adapters/duckrun/engine.py#L515) calls `_delta_table(path)` fresh and `dt.merge(...)`; delta-rs binds its target read and conflict check to **HEAD-at-merge-start = snapshot B**.
Spark pins one snapshot for the whole merge — the scan and the conflict check see the same Delta version. duckrun's scan is lazy, so *in practice* A and B line up — but that's a practical consequence, **not a guarantee**.
## Consequence — silent lost update
When the merge target read (B) is **newer** than the source scan (A) — i.e. a concurrent commit landed in the A→B window and nothing commits *after* the merge starts — OCC raises nothing, and the merge applies **stale snapshot-A source rows over the committed snapshot-B value**. A committed write is silently lost.
The opposite ordering (merge reads A, HEAD then advances to B before commit) is the **safe** case: delta-rs's OCC fires with `CommitFailedError`. The danger is purely the divergent direction.
## Reproduction
[`repro/scan_pin_lost_update.py`](https://github.com/djouallah/duckrun/blob/main/repro/scan_pin_lost_update.py) reproduces it deterministically:
1. Bootstrap target `id=1, value=10` (snapshot A).
2. Build the source from a real DuckDB `delta_scan` of the target at A, materialised to Arrow — passthrough `id=1, value=10` plus new `id=2, value=20`. Source pinned to **A**.
3. A legitimate concurrent commit lands `UPDATE id=1 SET value=999` → **snapshot B**.
4. Default upsert merge runs; its target reads B but its source is A.
5. **Result:** final `id=1` = `10` — the committed `999` is silently gone, no error.
```
committed concurrent value (snapshot B): 999
source value (snapshot A): 10
final value after merge: 10
BUG REPRODUCED: committed value 999 was silently lost, stale 10 won (no conflict error raised).
```
The monkeypatch in the repro is a **timing device only** — it forces the A→B window open deterministically rather than relying on losing a real, tiny race. It does not manufacture an impossible condition.
## Scope
- **Merge only.** Affects the key-based incremental path (`merge` / `insert` dedupe), where the source is derived from a snapshot of the target and the merge conflict-checks against HEAD.
- **`append` and `overwrite` are NOT affected** — they do no OCC / read-snapshot conflict check; they just write, so there is no A↔B snapshot to diverge.
- **Trigger: heavy concurrent writes only.** The A→B window is tiny, so a serial / single-writer pipeline never hits it.
## Fix
### Proven fix — but not usable in duckrun: `ATTACH (TYPE delta, VERSION ...)`
The clean fix is to pin both sides to one version. DuckDB's `ATTACH` supports it today, demonstrated in [How far Python alone can take you on Delta](https://datamonkeysite.com/2026/05/24/how-far-python-alone-can-take-you-on-delta/):
```python
vB = DeltaTable(target_path).version() # read HEAD once
```
```sql
ATTACH '<target_path>' AS tgt (TYPE delta, VERSION {vB}); -- pin the read to vB
```
…and pin the delta-rs merge to the same `version=vB`. The OCC check then compares against `vB` instead of HEAD, so a concurrent change between read and write **raises** instead of being silently lost. Read and write share one snapshot.
**Why duckrun can't use this.** duckrun deliberately rejected `ATTACH (TYPE delta)` as its read mechanism ([delta-scan-views-design.md](https://github.com/djouallah/duckrun/blob/main/delta-scan-views-design.md)): a single-table attach **becomes its own catalog** (`tgt`), referenced as `tgt`. It cannot live inside dbt's three-part `database.schema.identifier`, so `{{ this }}` / `ref()` / `is_incremental()` would not resolve. duckrun exposes each table as a `delta_scan` **view** named exactly `lake.mart.dim_duid` precisely to keep dbt's naming working. Swapping to ATTACH to get version pinning would break that core invariant.
### Fix that fits duckrun — version pinning on `delta_scan`
The view-based design needs `delta_scan` itself to accept a version. [duckdb/duckdb-delta#312](https://github.com/duckdb/duckdb-delta/pull/312) adds exactly that. Once released, duckrun can keep its `delta_scan` views and:
1. Read HEAD once: `version = DeltaTable(path).version()`.
2. Scan that exact version in DuckDB: `delta_scan(..., version=version)`.
3. Load the merge target at the same version: `DeltaTable(path, version=version)`.
So the source scan and the merge conflict check share one pinned snapshot — matching Spark — without giving up dbt's three-part naming.
0 条评论