fix: fall back to string-only when the VARIANT write fails - #118
Merged
Conversation
2026-08-12 incident: enabling dual-write on prod-us events-nrt
crash-looped all 16 consumers. One tenant's payload carried a JSON
integer above INT64_MAX; try_cast(... AS VARIANT) accepts it (it is a
valid UINT64 variant), but DuckDB's shredded Parquet write then
overflows converting UINT64 -> INT64 and fails the whole INSERT. Offsets
never advanced, so the same batch failed forever.
Retry the failed flush once with the VARIANT projection dropped: the
records land string-only with a NULL companion, matching how
ensure_variant_columns already degrades a wrong-typed companion. If the
retry also fails the error propagates — the projection wasn't the cause.
Values are never rewritten; regex-normalizing big ints in the JSON text
corrupts any string containing a long digit run.
Adds variant_write_fallback_total + errors_total{type="variant_write"}.
Tests use a new ducklake_conn fixture — a REAL local DuckLake catalog
writing Parquet. The existing in-memory `conn` fixture cannot reproduce
this class of bug at all (no Parquet, no shredding), which is exactly
why the pre-incident test suite passed. Shredding needs a few hundred
rows to trip data inlining.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review found the batch-level retry was the wrong layer. It absorbed retryable failures (commit contention permanently NULLed companions and blinded the contention alert), dropped every source's companion rather than the offender's, skipped the reset_caches() the outer retry drives, leaked an abandoned Parquet file per flush, and could not help at all when a small write is INLINED into the catalog — verified: that path succeeds, then detonates later in ducklake_flush_inlined_data, where no retry can reach it. Prevent instead: _variant_projection nulls the companion per row for JSON carrying a 19+ digit integer in value position. Anchoring on the delimiters keeps long digit runs inside string values (session ids) from false-positiving — verified against a real catalog. Only offending rows lose VARIANT; neighbours, other sources, and every string column are untouched, and nothing unshreddable ever reaches the column, inlined or not. The retry survives as a narrow backstop for value shapes the pattern misses: only the out-of-range conversion signature is absorbed, counters increment after the string-only write actually succeeds (they inflated 3x per flush during outages), and the log says a nonzero counter means a guard bug, not routine degradation. Tests: promote the local-DuckLake fixture to tests/integration/conftest.py with its pytest.skip guard (the copy here would hard-error CI offline), add an inlining-enabled variant, and cover per-row precision, the inlined path, second-source isolation, contention NOT being absorbed, and the backstop still firing. The old re-raise test failed both INSERTs so it passed against unfixed main; it now fails only the projecting INSERT. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…JSON Re-review of the reworked guard found it would crash-loop prod again, for a new reason, and was wrong in both directions on top of that. Verified each claim against a real DuckLake catalog: - regexp_matches() only binds VARCHAR. arrow_converter types each key from its first non-null sample, so an all-numeric batch for the configured source yields int64 — BinderException, backstop doesn't match it, partition wedges. main handled this fine. - The pattern required a leading :/,/[ delimiter, so a bare top-level number never matched. Under default data inlining that commits poison into catalog state and breaks ducklake_flush_inlined_data forever, where no write-time retry can reach it. - "19+ digits" is not the hazard. Measured: only (INT64_MAX, UINT64_MAX] fails to shred. Nanosecond timestamps and snowflake ids — 19 digits, ubiquitous — shred fine but were having their whole row's companion nulled, silently, with no metric. Replace the SQL regex with a two-stage Arrow pass: a vectorized prefilter shaped to the window's decimal form (19 digits leading 9, or 20 leading 1 — so ns timestamps don't trip it), then orjson round-trip on just the flagged rows, rewriting only ints in the dangerous window as strings. Precision comes from the parser, never from a pattern. Result lands in a hidden per-source column read only inside the VARIANT cast, so the source column still writes byte for byte, and the SQL projection goes back to main's simple form — no type fragility. Degradation is now "one value is typed as a string" rather than "the row loses its whole companion", and it is visible: variant_values_coerced_total. ~10ms per 8k-row flush when nothing matches, ~22ms when something does. Tests pin the boundary in both directions (ns timestamps, snowflakes, INT64_MAX, >UINT64_MAX, negatives all keep numeric typing), cover the bare-number/inlined path and non-string sources, and assert the fallback counter fires. The old tests encoded a false premise — that a 30-digit INT128 value explodes — which would have blocked any correct narrowing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EDsCODE
approved these changes
Aug 13, 2026
jghoman
added a commit
that referenced
this pull request
Aug 13, 2026
…drift-gated (#119) The 1.5.2 extension channel is frozen at ducklake build 415a9ebd, which leaks per-connection native memory proportional to catalog metadata churn (A/B-verified in viaduck: ~1MiB per snapshot-advancing bind). The 1.5.5 channel serves d8a1881e, which is flat — millpond's long-lived writer connections against a busy shared catalog have the same exposure. pyducklake (ours) rides along as a deliberate lockstep canary: it hard-pins duckdb, so a one-sided bump of either package fails uv lock instead of silently splitting the two DuckLake stacks. Published yesterday, so it gets a fixed-date exclude-newer-package carve-out from the 7-day cooldown. Because the 1.5.5 channel is LIVE (the frozen channel was accidental drift protection), the Dockerfile now asserts the ducklake build SHA in the extension-install layer — an unexpected ducklake build cannot produce an image at all. Verified both directions: correct SHA builds, wrong SHA fails the layer. The release workflow runs in parallel with CI, so the test canary alone could not stop a drifted build from reaching the fleet via the mutable tag. Behavior changes under d8a1881e, all verified against real catalogs: - The (INT64_MAX, UINT64_MAX] shred-rejection window and its write-time error signature are UNCHANGED; the #118 guard and backstop hold. The inlined-path deferred failure now reports INT128 (was UINT64); the width-agnostic matcher still catches it. - JSON numbers outside [INT64_MIN, UINT64_MAX] in a VARIANT become lossy DOUBLE (were digit-preserving VARCHAR). Deliberate policy: follow native DuckLake semantics; the string column stays authoritative (now asserted). Rows written across the version boundary mix both representations in the companion permanently. - The ducklake attach no longer lists its metadata db as a separate duckdb_databases() row (test-fixture-only impact). - No catalog metadata migration: old and new builds read/write the same catalogs interchangeably, so mixed fleets and co-readers (viaduck, duckgres) are unaffected. New tests/integration/test_ducklake_engine_contract.py pins the engine behaviors the variant guard assumes — the containment invariant (engine-rejected values must be guard-rewritten, probed through the real sanitize_variant_sources chain with the guard bypassed at the engine), its converse (shreddable probes round-trip untouched), boundary values, nested/array/sci-notation shapes, error-signature recognition, and the inlined commit-then-detonate path — so the next bump that shifts any of this fails at the PR instead of on the fleet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause of the 2026-08-12 incident
Enabling VARIANT dual-write on prod-us
events-nrtcrash-looped all 16 consumers within minutes (PostHog/charts#14189, reverted in PostHog/charts#14232).One tenant's payload contained a JSON integer above
INT64_MAX(9223372036854775999).try_cast(try_cast(properties AS JSON) AS VARIANT)accepts it — it's a legitimate UINT64 variant, and sub-field access returns it fine. But DuckDB shreds VARIANT into typed Parquet columns on write, and there the value overflows converting UINT64 → INT64:The INSERT fails, the flush fails, offsets never advance, and the same batch fails forever.
try_castcannot guard this: the cast isn't what breaks, the physical write is.Fix
Retry the failed flush once with the VARIANT projection dropped. Records land string-only with a NULL companion — the same degradation
ensure_variant_columnsalready applies for a wrong-typed companion — instead of wedging the partition. If the string-only retry also fails, the error propagates unchanged, since the projection wasn't the cause.New signals:
millpond_variant_write_fallback_total(per flush) anderrors_total{type="variant_write"}.Rejected alternative: normalizing huge integers to strings in the JSON text before the cast. A regex over raw JSON corrupts any string value containing a 19+ digit run (
{"session": "12345678901234567890"}→ invalid JSON), and the string column is authoritative anyway. Values are never rewritten.Why the tests didn't catch it
The existing
connfixture is plain in-memory DuckDB — it never writes Parquet, so it cannot exercise shredding at all. This PR adds aducklake_connfixture backed by a real local DuckLake catalog with a Parquet data path, which reproduces the production failure exactly. Shredding only trips above DuckLake's data-inlining threshold, so these tests use a few hundred rows rather than one.Three new tests: the poison batch lands string-only (all rows present, companion NULL); a later clean batch still dual-writes (the fallback is per-flush, not sticky); an unrelated INSERT failure still raises.
Upstream
try_castsucceeding while the write overflows looks like a DuckDB bug worth reporting separately — this fix is the operational guard either way.Test plan
InvalidInputException)🤖 Generated with Claude Code