Production-readiness review: 24 defects fixed, incl. two heap-corruption paths; release v0.16.0 - #115
Merged
Merged
Conversation
…ffold DuckDB hands a loadable extension a struct of function pointers whose first 357 slots have been frozen since v1.2.0 and whose remainder — the "unstable" region — gains entries *in the middle* between releases (duckdb_appender_clear at slot 410 in v1.5.0, duckdb_geometry_type_get_crs at slot 493 in v1.5.2). All 105 C API functions behind the duckdb-1-5 / duckdb-1-5-3 features live in that region, yet the default C_STRUCT/v1.2.0 metadata makes DuckDB accept the binary on any release >= v1.2.0 and hand over the whole struct. Verified end to end against DuckDB 1.5.5: an extension built on 1.5.0's headers and stamped C_STRUCT/v1.2.0 aborts the process with "double free or corruption". With the new guard the same binary fails LOAD with a diagnostic naming both layouts and the remedy. - add `abi` module: stable/unstable split, verified release -> slot-count table, `check()`, `AbiPolicy` (Strict by default) - run the check in init_extension / init_extension_v2 and both entry-point macros; add *_with_policy variants and 3-argument macro arms - wrap the user's registration closure in catch_unwind; reject an api_version containing an interior NUL before it reaches libduckdb-sys - scripts/check-abi-table.py re-derives the table from upstream headers Also repairs several defects the above investigation surfaced: - ChunkWriter assumed a 2048-row capacity; DuckDB can be built with a different STANDARD_VECTOR_SIZE, so it now reads duckdb_vector_size() - DuckStringView::from_bytes was safe yet dereferenced an embedded pointer; split into unsafe from_raw and safe inline_from_bytes - TypeId::try_from_duckdb_type and fallible LogicalType constructors replace panics that would abort a DuckDB process from inside a callback - the scaffold emitted DUCKDB_PLATFORM_VERSION (ignored by extension-ci-tools) with USE_UNSTABLE_C_API=1, producing a binary stamped v0.0.1 that DuckDB refuses; it also set EXT_NAME instead of EXTENSION_NAME, defined none of the make targets it documents, pinned quack-rs 0.13, generated panic = "abort" (which makes the crate's own catch_unwind inert), referenced a nonexistent GitHub action, and failed its own generated clippy gate - CI gains abi-table, abi-guard and scaffold-e2e jobs, and extension-load now stamps a real metadata footer and asserts query results Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Three stable-API capability gaps and one incorrect claim about testability. `query`: extensions routinely need to talk SQL to the database loading them, but every handle in duckdb_query / duckdb_prepare / duckdb_bind_* / duckdb_fetch_chunk has a destroy that must run exactly once, including on the error paths. QueryResult, OwnedDataChunk, PreparedStatement and OwnedConnection are RAII wrappers for those; Connection gains query, execute, prepare and open_connection. OwnedConnection covers what the borrowed registration connection cannot: a duckdb_connection holds its own reference to the database instance, so one opened during load stays usable afterwards. `datetime`: DATE/TIME/TIMESTAMP travel through vectors as raw integers, and decoding them meant reimplementing the proleptic Gregorian calendar. DuckDB exposes the conversions in the stable API; this wraps them, along with HUGEINT/UHUGEINT/DECIMAL <-> f64 and the exact infinity sentinels (-infinity is -i32::MAX, not i32::MIN — treating i32::MIN as infinity would drop real rows). VectorWriter now caches its validity bitmap (2 FFI calls per vector instead of 4096 for an all-NULL one) and gains set_null_range; both reader and writer gain the physical layouts that were missing: u128, DECIMAL at all four widths, TIMETZ, and the TIMESTAMP_S/MS/NS variants. The crate documented that VectorReader/VectorWriter/register_* "cannot be called in cargo test". They can: InMemoryDb::open populates the dispatch table for the whole process. tests/ffi_roundtrip.rs registers real scalar functions and round-trips every vector type through SQL — integer extremes, HUGEINT and UHUGEINT extremes, NaN, the 12-byte string inline/pointer boundary, non-UTF-8 blobs, every temporal type cross-checked against DuckDB's own rendering, DECIMAL at each physical width, NULL in and out, multi-chunk scans, and a panicking callback surfacing as a SQL error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…he load
AbiPolicy::Warn reported through access.set_error, but DuckDB's loader throws
whenever an extension called set_error regardless of what the entry point
returned:
if (load_state.has_error) {
load_state.error_data.Throw("An error was thrown during ...");
}
so Warn was indistinguishable from Strict — it failed the load it was meant to
allow. The C extension API has no non-fatal diagnostic channel, so the warning
now goes to stderr. Verified against DuckDB 1.5.5: Warn loads and answers
queries, Strict still refuses.
Documentation:
- LESSONS.md gains P10 for the stable/unstable split, with the verified
per-release slot counts and the reproduction. Corrects an existing claim that
DuckDB 1.5.x has 573 struct fields (it has 545 or 546 depending on the patch
release) and narrows the P9 risk-table row that assumed field order is stable
across releases — it is not.
- P2 gains the caveat that `-dv` means the C API version only for C_STRUCT; for
C_STRUCT_UNSTABLE it is the exact DuckDB release.
- New book chapters: ABI Compatibility, Running SQL from an Extension, and
Dates/Times/Timestamps.
- README module table and pitfall table updated.
- The abi module documents the over-read in libduckdb-sys's dispatch-table
init when loading into an older DuckDB — upstream's to fix, but worth
stating rather than leaving as a surprise.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Only scalar and table-scan callbacks had catch_unwind wrappers. The other six kinds — table bind, table init, aggregate update/combine/finalize/destroy, cast and replacement scan — were unguarded, so a panic in any of them aborted the DuckDB process. The aggregate ones are the worst case: they run on DuckDB's worker threads, so the abort arrives from a thread the user never sees. Adds table_bind_callback!, table_init_callback!, aggregate_update_callback!, aggregate_combine_callback!, aggregate_finalize_callback!, aggregate_destroy_callback!, cast_callback! and replacement_scan_callback!, each routing the panic message to that kind's own set_error. cast_callback! also returns false so TRY_CAST yields NULL. The aggregate destructor has no error channel in the C API, so its panic is caught and dropped — leaking beats aborting during query teardown. Verified against a real DuckDB: a panicking aggregate update and a panicking cast both surface as SQL errors carrying the panic payload, and the connection stays usable afterwards. The suite also now covers a typed table function streaming rows, its bind error path, its panic path, and an aggregate computing across chunks and GROUP BY. Also: - the two pre-existing macros now share callback::panic_message and callback::message_to_c_string; the latter replaces an interior NUL instead of dropping the diagnostic entirely, which `if let Ok(c_msg) = CString::new(msg)` silently did - TypedTableFunctionBuilder reported every panic as one fixed string; it now includes the payload - deprecates FfiBindData::get_from_bind, which always returned None and always will — DuckDB exposes no duckdb_bind_get_bind_data, and being safe and Option-returning it sent `if let Some(..)` down the wrong branch silently - re-exports the scalar and aggregate callback type aliases at their module roots, matching what `table` already did Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…accessors
duckdb_list_vector_reserve takes a *total* capacity and reallocates the child
vector when it grows, so a VectorWriter obtained beforehand is left dangling.
That makes the natural "reserve as you go, keep one writer" loop a
use-after-free — the book's manual example is safe only because it reserves
exactly once, which requires knowing the element total up front.
ListBuilder re-fetches the child writer after every reserve, tracks the running
offset, writes each parent {offset, length} entry, and grows geometrically so
building a list is not quadratic. push_map_row does the same for MAP. It also
refuses capacities above DConstants::MAX_VECTOR_SIZE, above which
ListVector::Reserve throws a C++ exception that DuckDB's own C API wrapper does
not catch — that exception unwinding into Rust would be UB.
Value gained the accessors it was missing: a table function declared with a
TIMESTAMP or LIST parameter handed the bind callback a duckdb_value that could
only be read via as_str() and reparsed. Adds the temporal, DECIMAL, UUID,
UHUGEINT, LIST, STRUCT and MAP extractors plus the matching constructors.
Two behaviours verified and then documented rather than assumed:
- Value::display_string renders a SQL *literal*, so a VARCHAR comes back quoted
and a DATE carries a ::DATE cast. Surprising in a diagnostic string.
- Value::as_str truncates at an interior NUL, because duckdb_get_varchar returns
a NUL-terminated char*. DuckDB stores the full bytes; only the read path is
limited.
And one bug caught by its own test: as_uuid assembled duckdb_get_uuid's halves
directly as i128, which overflows once the upper half's high bit is set — half
of all UUIDs, panicking in a debug build. Now assembled as u128 and
reinterpreted, with a round-trip test across i128::MIN and i128::MAX.
Covered by tests building 2000 lists and 1500 maps of varying length through
real SQL, checking every row's contents and the flattened element total.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Two problems that both bite when DuckDB ships a new version. MSRV. DuckDB's reusable _extension_distribution.yml — the workflow the community-extensions repository builds every extension with — pins dtolnay/rust-toolchain@... # 1.86.0 for the WebAssembly job. quack-rs required 1.87.0, so Cargo refused outright, and no quack-rs extension could be built for wasm_mvp / wasm_eh / wasm_threads despite the crate advertising wasm32-unknown-emscripten support. The whole 1.87 requirement was five const fn accessors calling Vec::len, none reachable in a const context. Dropping const costs nothing; 1.86.0 now verified for the library, dev-dependencies and the example. scripts/check-msrv-vs-duckdb-ci.py plus a CI job re-derive DuckDB's pinned toolchains and fail if the MSRV creeps back above them. ABI guard forward compatibility. When DuckDB releases, the community repository rebuilds each extension from unchanged source against the new headers. That binary is correct — but AbiPolicy::Strict could only see an engine version its layout table predates, and refused it. So the guard would have broken the very path DuckDB provides for surviving a release. QUACK_RS_TARGET_DUCKDB_VERSION now declares the release the bindings were built against; check() matches on that instead of the table, needing no quack-rs release. The generated Makefile exports it automatically for unstable-ABI builds (and deliberately not for stable ones, where TARGET_DUCKDB_VERSION means the C API version, not a release). A declaration that contradicts the compiled slot count — Cargo resolved a different libduckdb-sys than intended — is reported as DeclaredVersionMismatch. AbiPolicy::AllowUnknownEngine is the fallback for builds that cannot set it. The decision is now a pure `decide()` function, so every branch is unit-tested including engine versions that do not exist yet. Verified end to end against DuckDB 1.5.5: a wrong declaration is caught with a diagnostic naming both slot counts, and a correct one loads and calls into the unstable region. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…ic type file_system: duckdb_file_handle_read/write return "the number of bytes *actually* read/written", so a single call can be short — routine over httpfs. Adds read_exact/read_to_end/write_all, which loop. size() and tell() move from i64 to Result<u64, ErrorData>: the C API signals failure with a negative return, and the old signature made `handle.size().max(0) as usize` the obvious thing to write. That exact line was in this crate's own module docs and book chapter; both fixed. Value::type_id wraps duckdb_get_value_type (stable prefix, slot 137, unchanged since v1.2.0). Value had forty as_* accessors and no way to ask what a value actually holds, so reading a VARCHAR with as_i64 returned garbage rather than an error. Debug: 58 public types had no impl. That is Rust API guideline C-DEBUG, and it is not cosmetic — Result::unwrap, Result::expect_err, assert_eq! and #[derive(Debug)] on any downstream struct holding a quack-rs type all fail to compile without it. It surfaced here as a test that could not call .expect_err() on a Result<FileHandle, _>. LogicalType and Value decode their handles (type id, alias, DECIMAL width/scale, DuckDB's rendering) rather than printing an address, and avoid every panicking path — a Debug impl that panics while formatting a panic message aborts the process, so an unknown type id prints its number instead of going through TypeId::from_duckdb_type. Builders print set/unset per callback. WarningCollector uses try_lock so printing can neither block nor deadlock against a caller inside emit. missing_debug_implementations is enabled crate-wide; CI's -D warnings makes it an error. Live coverage: a VFS round-trip over a real file (short reads, read_exact past EOF, EOF semantics, zero-length ops), and Debug decoding checked against a running DuckDB rather than asserted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…onvention Two findings from auditing every duckdb-1-5-gated module against the actual stable/unstable slot split. 1. appender and table_description were gated on duckdb-1-5 but are almost entirely in the frozen stable prefix: duckdb_appender_* occupies slots 281-291 and 330-356, table_description 292-297, all unchanged since v1.2.0. Gating them forced extensions onto the version-pinned unstable ABI for functionality that has been portable across four minor releases. Both modules are now unconditional; only the genuinely-1.5 methods stay gated (appender error_data/clear/append_default_to_chunk, table description column_count/column_type). The 24 row-at-a-time appender functions were never wrapped at all, so the only way to insert a row was to build a whole DataChunk. Adds the full set plus end_row, column_count/column_type, add_column/clear_columns, and a row(|row| ...) helper that calls end_row for you. Three details verified against DuckDB's own appender-c.cpp rather than assumed: append_str uses duckdb_append_varchar_length so interior NULs survive; that function narrows its length to uint32_t with an unchecked cast in release builds, so longer strings are refused; and duckdb_append_value dereferences its argument with no null check, so a null Value handle is refused. New appender::AppendError is ErrorData with duckdb-1-5 and ExtensionError without, so enabling the feature upgrades the error type in place without changing any method's shape. Existing duckdb-1-5 code is unaffected. table_description gains with_catalog and column_has_default -- the latter being the only way to know whether append_default will succeed. 2. The UUID accessors disagreed about which 128 bits they meant, and the docs on both claimed they matched. A UUID column is physically a HUGEINT, but DuckDB flips the top bit so signed ordering matches string ordering (BaseUUID::FromUHugeint subtracts 2^63 from the upper half). read_uuid returned the raw storage, Value::as_uuid returned the textual bits, so handing one to the other silently changed the UUID's first hex digit -- 0x9111... instead of 0x1111... Confirmed against a running DuckDB before changing anything. read_uuid/write_uuid (VectorReader, VectorWriter, StructReader, StructWriter, both mocks) now apply the flip and speak in u128 textual bits, matching Value::uuid/as_uuid and every Rust Uuid type. Value::uuid and as_uuid move i128 -> u128 for the same reason. The type change is deliberate: it makes every affected call site a compile error instead of a silent behaviour change. read_i128/write_i128 still give raw storage, and vector::uuid_from_storage / uuid_to_storage convert. Also fixes three broken relative links in the book that predate this work. Verified: 747 tests + 154 doc tests pass; the appender suite passes with duckdb-1-5 both on and off, against a live DuckDB on the pure stable ABI; clippy clean on every feature combination; MSRV 1.86.0 holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
validate::platform was stale in both directions. It rejected linux_amd64_musl and linux_arm64_musl -- real, currently-built targets, so an extension that legitimately cannot support musl had no way to declare it. And it accepted linux_amd64_gcc4, which DuckDB retired: DuckDBPlatform() in duckdb/common/platform.hpp now raises a compile error for the legacy CXX ABI instead of emitting a _gcc4 suffix, and it is absent from the distribution matrix, so excluding it was a silent no-op. The list now comes from config/distribution_matrix.json in duckdb/extension-ci-tools -- the file the community-extensions build reads -- with scripts/check-platform-table.py and a CI job failing on divergence. Adds DUCKDB_OPT_IN_PLATFORMS and is_opt_in_platform, since three of the twelve are built only on request and excluding those is also a no-op. validate_spdx_license rejected valid licenses with "is not a recognized SPDX identifier". COMMON_SPDX_LICENSES is a 42-entry shortlist of a 733-entry registry, so that claim was false for roughly 690 real identifiers. The message now says what is actually true and points at the registry. All 42 entries were checked against spdx/license-list-data: real, none deprecated. scripts/check-spdx-list.py and a CI job keep it that way and flag any new non-OSI-approved entry. Also: sorted the SPDX list (it was not) with a test to keep it sorted, and fixed the module doc's "extension.licence" -- real description.yml files and quack-rs's own parser both use "license". Empirically checked 43 accepted community extensions' description.yml files: all use MIT or Apache-2.0, both already accepted, so no published extension was affected by the SPDX message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…sions Tested parse_description_yml against every description.yml I could fetch from duckdb/community-extensions: 36 of 43 published, accepted extensions were reported invalid. The validator exists to tell an author their submission is good before they open a PR, and it was telling almost everyone it was bad. Four independent causes: 1. requires_toolchains was required. It is not -- 14 of 43 set it, and the community-extensions docs do not list it as required. This alone rejected half the corpus. 2. YAML quotes were not stripped. parse_kv returned quoted values with their quotes and left stripping to each caller; only excluded_platforms did it. 12 of 43 write version: '2025120401', so every version check ran against the quoted string. parse_kv now unquotes with a balanced-quote check rather than trim_matches, which would also eat ""doubled"" and a trailing a". 3. validate_extension_version imposed a format DuckDB does not. It took only semver or a git hash; 11 of 43 use a date-based build id. DuckDB's community-extension documentation specifies no version format -- it says the descriptor carries "the version of the extension" and points at existing extensions as examples. The check is now what would actually break something: empty, over 64 chars, or characters outside [A-Za-z0-9._+-]. classify_extension_version is untouched; DuckDB's three-tier stability scheme is documented and strict, and that is where it belongs. 4. windows_amd64_rtools was rejected. It is the R-tools Windows build that DuckDBPlatform() emits under DUCKDB_PLATFORM_RTOOLS, and 14 of 43 extensions exclude it. DUCKDB_PLATFORMS now accepts it and the four distribution-matrix group names; the new DUCKDB_CI_PLATFORMS keeps the matrix-derived list the guard script checks. Empty segments from a trailing ';' -- five real files have one -- are skipped. All 43 now parse, every name matching its directory. Separately, the scan was flat, so a version: or license: line inside docs.extended_description -- free-form prose in 42 of 43 files -- silently overwrote the real metadata. Demonstrated with a license: FAKE-LICENSE line in a documentation block failing an otherwise valid file; the same mechanism could have passed an invalid one. The parser is now section-aware and understands block scalars, capturing key: | and key: > bodies as the field's value rather than scanning them for mappings. Finding that exposed a third problem: three doc examples used \ line continuations, which eat the next line's leading whitespace, so the YAML they displayed as indented was parsed as fully unindented. They passed only because the parser ignored indentation. Rewritten as real multi-line literals. examples/parse_descriptions.rs runs the parser over a directory of real files so this can be re-checked against the live corpus. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…exist The module described itself as bridging into DuckDB's secrets system. It cannot: the extension C API has zero secret functions -- not one duckdb_secret_* among the 546 slots of duckdb_ext_api_v1 in DuckDB 1.5.5. An extension has no way to ask DuckDB for a credential through the C API. The only available route is the duckdb_secrets() table function, and DuckDB redacts sensitive fields there. Verified against a running 1.5.5: a secret created with SECRET 'super-secret-value' comes back as secret=redacted. The docs now say this, and say what SecretsManager actually is -- a trait over the extension's own credential source, pre-fitted with redacting Debug, zeroize-on-drop and no PartialEq -- rather than implying a route to DuckDB's store. The zeroize claim is narrowed to what is true: the buffers a SecretEntry owns, not one the caller still holds. Adds secrets::list_duckdb_secrets, which reads the metadata DuckDB does expose: name, type, provider, persistence, storage, scope prefixes and the redacted secret_string. Enough to choose a scope, warn that a required secret is missing, or pick a provider. It returns DuckDbSecretInfo, not SecretEntry, so nothing suggests it carries credentials. The live test asserts both halves -- the metadata arrives, the credential provably does not. Also: testing::InMemoryDb had no Debug impl (only visible once the new crate-wide lint ran with bundled-test on), and two zeroize tests read through a pointer derived before a &mut borrow, which is UB under Stacked Borrows even though it works today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
The scaffold wrote `ref: main`. DuckDB's community-extension documentation
is explicit -- "Provide the hash of the latest commit on the branch
targeting stable as `ref`" -- because the repository builds exactly that
revision and signs the result, so a branch makes the build unreproducible.
Of the 43 published extensions I fetched, 41 pin a full 40-character hash
and two pin a tag; none uses a branch.
ScaffoldConfig gains git_ref, defaulting to REF_PLACEHOLDER
("REPLACE_WITH_COMMIT_HASH") -- deliberately not a valid revision, so it
cannot be submitted by accident the way `main` silently could. The
generated file explains why, and carries a commented-out ref_next.
DescriptionYml silently dropped repo.ref_next, which is documented: while a
new DuckDB release is being prepared the community repository tests against
both the latest stable release and main, and ref_next names the revision
compatible with main. Now parsed into git_ref_next.
The generated description.yml also had no docs: section. All 43 published
extensions have one -- it is what renders on the community-extensions
documentation site -- so the scaffold now emits hello_world and
extended_description stubs.
Verified: the generated file still parses with quack-rs's own validator,
and scaffold_generated_code_compiles still passes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
validate_function_name required lowercase, and it gates try_new -- so
ScalarFunctionBuilder::try_new("myFunc") returned Err and such a function
could not be registered through quack-rs at all. DuckDB itself ships
formatReadableSize and formatReadableDecimalSize, and registering a
camelCase name through the C API succeeds: verified against DuckDB 1.5.5,
where it is then callable as formatReadableThing, formatreadablething and
FORMATREADABLETHING, since DuckDB identifiers are case-insensitive.
The rule was justified as avoiding "catalog issues"; that test disproves
it. Letters of either case are now accepted, and everything that would
genuinely break still is not: names needing quotes in SQL, names starting
with a digit, names over 256 chars, names with an interior NUL. snake_case
is documented as the convention it is instead of enforced as a rule that
blocks a legal name. AggregateFunctionBuilder, TableFunctionBuilder and
SqlMacro share the validator and get the same relaxation.
Found by running the validators over what DuckDB actually has: every name
in duckdb_functions() (746) and duckdb_extensions(). That check is now a
regression test asserting everything identifier-shaped is accepted and
every operator is rejected. Extension names came back clean -- 0 rejected.
Also corrects stale claims this exposed in validate's module docs and the
README about function-name and version rules, and exports the new
DUCKDB_CI_PLATFORMS / DUCKDB_PLATFORM_GROUPS constants.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…uard validate_release_profile required panic = "abort" and rejected "unwind". That is backwards for a quack-rs extension. quack-rs wraps every extern "C" entry point -- the extension entry point and every scalar, table, aggregate, cast and copy callback macro -- in catch_unwind, so a panic in an extension's code becomes a DuckDB error instead of a crash. catch_unwind catches nothing under panic = "abort": the runtime aborts before unwinding starts. Demonstrated rather than assumed: rustc -O panic_probe.rs -> caught, survived, exit 0 rustc -O -C panic=abort ... -> Aborted, exit 134 So the validator was telling extension authors to configure the one setting that turns a recoverable SQL error into a SIGABRT that kills the user's whole DuckDB session. The crate already disagreed with itself: the scaffold has generated panic = "unwind" since the panic-safety work in this release, with a comment explaining why. The validator now requires "unwind" and rejects "abort" with that explanation, ReleaseProfileCheck::panic_abort becomes panic_unwind, and a new test asserts the scaffold and the validator agree so they cannot drift apart again. The original justification -- "panics across FFI boundaries are undefined behavior" -- is also out of date: Rust defines an unwind escaping extern "C" as an abort, and quack-rs catches panics before the boundary anyway. quack-rs's own [profile.release] said panic = "abort" too. Cargo ignores a dependency's profile so it changed nothing downstream, but it contradicted the crate's own advice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
CopyGlobalInitInfo::get_file_path called duckdb_free on the pointer from
duckdb_copy_function_global_init_get_file_path. That function returns
info_ref.file_path.c_str() -- the interior pointer of a C++ std::string
DuckDB still owns and destroys itself -- so every COPY ... TO through a
quack-rs copy function handed the allocator a pointer it never issued. The
first live test of the path aborted with "corrupted size vs. prev_size in
fastbins".
Audited all twelve remaining duckdb_free call sites against DuckDB's own
src/main/capi implementations; the rest are correct. The signature alone
does not decide it: char * returns are owned, const char * returns are
usually borrowed, but duckdb_parameter_name is declared const char * and
returns strdup(...), so it is owned and must be freed. Recorded as
LESSONS.md P11 with the full table and the rule.
The bug survived because copy_function and replacement_scan had 19 unit
tests between them and not one registered anything against a running
DuckDB. Both now have live coverage:
- COPY ... TO over 5000 rows, threading bind data and global state
through all four lifecycle phases, asserting the sink saw every row and
that both destructors ran exactly once -- a leak or a double free is
invisible without counting.
- A replacement scan rewriting SELECT * FROM '10.myfmt' into a table
function call, the decline path (an identifier the callback ignores
must still reach DuckDB's own error handling), and a panicking scan
surfacing as a SQL error.
Adds copy_bind_callback!, copy_global_init_callback!, copy_sink_callback!
and copy_finalize_callback!. Every other callback kind had a panic-safe
macro; the four copy phases did not, so a panic in one had nothing to catch
it. Each routes the message through that phase's own
duckdb_copy_function_*_set_error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…docs Six modules had unit tests but never registered anything against a running DuckDB: scalar bind/init/local state, Expression::fold, catalog lookup, config options, selection vectors and the instance cache. That is the same gap that hid the copy-function heap corruption, so all six now have end-to-end coverage. Two more documentation defects fell out of it. ClientContext::catalog documented an empty name as "the default catalog". DuckDB rejects it outright -- duckdb_client_context_get_catalog opens with `if (!context || !name || strlen(name) == 0) return nullptr;` -- so an empty string is the one value guaranteed to fail. The in-memory database's catalog is named `memory`. The docs now say that, plus the other None case quack-rs never mentioned: DuckDB checks transaction.HasActiveTransaction(), so lookup works inside a callback but not on an idle auto-commit connection. Both confirmed by the new test. ClientContext::config_option aborts the process when asked for a setting that does not exist, on a DuckDB built with debug assertions: duckdb_client_context_get_config_option calls TryGetCurrentSetting(...).GetScope() before checking the lookup succeeded, and GetScope() asserts scope != SettingScope::INVALID. Release builds compile the assertion out and the function's own default: arm returns NULL as documented -- so it never reproduces for users and always reproduces in a test suite linking a debug DuckDB. That is DuckDB's defect, but it makes the obvious "does this setting exist?" probe unsafe, so it is now documented with the source lines, recorded as LESSONS.md P12, and the abort-free alternative given (SELECT ... FROM duckdb_settings()). The scalar test also covers Expression::fold constant-folding a bind argument, and asserts both the bind data and the per-thread local state are freed -- a leak or a double free there is invisible without counting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Splitting DUCKDB_PLATFORMS into a matrix-derived DUCKDB_CI_PLATFORMS and a superset of accepted names left check-platform-table.py comparing the superset against the matrix, so it reported a mismatch that was not one. It would have failed CI. It now checks each list against the thing that actually defines it: DUCKDB_CI_PLATFORMS against the matrix's duckdb_arch values, DUCKDB_OPT_IN_PLATFORMS against the entries marked opt_in, DUCKDB_PLATFORM_GROUPS against the matrix's top-level keys, and that DUCKDB_PLATFORMS accepts every CI platform. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
The trait covered scalar, scalar set, aggregate, aggregate set, table, SQL macro, cast and copy functions but not config options, so an extension that registers one could only have part of its registration closure exercised through MockRegistrar. Adds the trait method, the Connection implementation, and config_option_names / has_config_option on the mock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Three of the generated workflow's four actions were SHA-pinned; dtolnay/rust-toolchain@stable was not, justified by a comment saying its SHA "changes with each Rust release". That is not how the action works: it reads the toolchain from rust-toolchain.toml or its `toolchain:` input at run time, so pinning the action's SHA does not pin the Rust version. quack-rs's own CI SHA-pins the same action and still gets current stable. A branch is a moving target its owner can repoint at any time, and a workflow step runs arbitrary code in the user's CI. All four now use the same SHAs quack-rs itself uses, and a test asserts every `uses:` in the generated workflow carries a 40-character hex ref. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
The crate claimed every unsafe block carries a // SAFETY: comment. clippy::undocumented_unsafe_blocks reports 180 in the library, so it was not true. Most sit inside an unsafe fn and forward that function's own documented contract -- unsafe_op_in_unsafe_fn is denied crate-wide, so those blocks are required syntax rather than fresh assertions -- but around forty were in *safe* functions, where the crate rather than the caller asserts the invariant, and those had nothing at all. The claim is now the convention actually worth following, stated in both lib.rs and the README, and the convention is met: every unsafe block in a safe function has a SAFETY comment. The generated scaffold gets one too, so the convention is visible in the code users start from. Auditing them also caught three comments that described the wrong thing -- two duckdb_free calls and a duckdb_destroy_value annotated as if they were uses of the enclosing handle. They now say which allocation they own and why, cross-referencing LESSONS.md P11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
The section had grown to 22 headings with "### Fixed" appearing nine times and "### Security" twice, which made it hard to navigate on upgrade -- the one moment it actually gets read. Content is unchanged; the qualifiers move from duplicated ### headings to #### sub-headings under the five standard categories, and the maintainer note's cross-reference is updated to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
The test rewrote the generated Cargo.toml's quack-rs dependency to a path dep so `cargo check` would exercise "the local (possibly-modified) crate". It did that with a `.replace()` of the literal `version = "0.13"`, which stopped matching the moment the crate moved past 0.13. The replace then silently did nothing, and for several releases the test compiled the scaffold against the last *published* quack-rs instead of the working copy -- including every scaffold-template change made in this branch. The bump to 0.16.0 surfaced it: 0.16 is not on crates.io, so resolution failed outright. That is the same failure the maintainer would hit on every future bump, in the window between bumping and publishing. It now rewrites whichever line declares the dependency, and asserts the rewrite happened, so it cannot silently no-op again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
The PR checklist requires a new FFI pitfall to land in both LESSONS.md and book/src/reference/pitfalls.md. P10 (the unstable ABI tail) was added to LESSONS.md last cycle and never mirrored; P11 (freeing a borrowed const char *) and P12 (the config-option assertion) were added this cycle. All three are now in the book with the "your action" framing the other entries use, and the summary table is extended. P11 in particular belongs in front of extension authors, not just quack-rs maintainers: anyone calling the C API directly can free a pointer DuckDB still owns, and the const char * signature is only a hint -- duckdb_parameter_name is declared const and still returns strdup. examples/parse_descriptions.rs was also missing its SPDX header. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
CI found two defects that every combination I ran locally compiled through. MAX_LIST_CHILD_CAPACITY was typed usize. DuckDB's DConstants::MAX_VECTOR_SIZE is 1ULL << 37ULL -- an idx_t, not a pointer-sized value -- so as a usize const it is a const-eval overflow wherever pointers are 32 bits. That is every wasm32 target, three of which DuckDB's own extension CI builds, so the crate simply would not compile there. Now typed u64 to match idx_t, with a separate usize-clamped constant for the capacity arithmetic: on a 32-bit target DuckDB's ceiling exceeds any allocation usize can describe, so usize::MAX is the real limit. Three unit tests called duckdb-1-5-gated methods without a cfg gate, which breaks --features bundled-test on its own -- the combination that builds the live-DuckDB tests but not the 1.5 wrappers. Value::display_string and TableDescription::column_count / column_type are the gated methods. Adds scripts/check-matrix.sh, which runs the feature and target combinations CI runs in one command, including the two that caught these: bundled-test without duckdb-1-5, and cargo check --lib for wasm32-unknown-emscripten. Verified: every combination passes locally, including the exact wasm32 commands the CI job runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…stic Three CI jobs load an extension through the DuckDB CLI, and all three failed with assertions that could not be diagnosed from their logs. They ran `./duckdb -no-stdin < probe.sql`, which produced no output and still exited 0 -- so `set -o pipefail` did not trip and every assertion downstream failed as "wrong answer" when the SQL had simply never run. They now pass the statements with `-c`, which cannot silently no-op, and the install step proves `-c` executes SQL with a smoke test before three later steps depend on it. Each job also prints `duckdb --version` (these pull `releases/latest`, so the engine changes underneath them) and dumps DuckDB's actual output when an assertion fails. The abi-guard job additionally asserted something that is only sometimes true. It built a demo pinned to DuckDB 1.5.0's 545-slot layout and required the guard to refuse the load -- but when the runner's DuckDB is also 1.5.0 the layouts match, the guard correctly stays silent, and the job failed claiming the guard was broken. It now forces the contradiction through QUACK_RS_TARGET_DUCKDB_VERSION=v1.2.0 (408 slots) against the 545-slot bindings, which the guard settles without consulting the engine at all, so it fires on every runner. It also now asserts the refused extension did not register its function -- a refusal that still registers would be worse than no guard. src/abi.rs gains declared_version_mismatch_matches_the_ci_regression_test, which pins the exact message the job greps for and proves it fires for any engine version, so the job and the code cannot drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Owner
Author
CI status — what I've fixed, and one that isn't mineCI found four real defects that every feature combination I ran locally compiled through. Fixed and pushed:
Two CI jobs were asserting things they couldn't diagnose
|
CI sets RUSTFLAGS: -D warnings globally, so two unnecessary `unsafe` blocks in tests/ffi_roundtrip.rs -- warnings locally -- failed every bundled-test job. scripts/check-matrix.sh now exports the same RUSTFLAGS/RUSTDOCFLAGS as CI, so a plain `cargo check` there can no longer pass on code CI rejects. Adding `bundled-test` to the script's clippy runs then surfaced lints no CI job covers, because no clippy job enables that feature: a doc_markdown in src/testing/in_memory_db.rs and a cast_possible_wrap in src/query.rs, both fixed. The remaining 30-odd are pedantic style lints in the integration test file -- too_many_lines, deliberate casts at type edges -- which that file now allows with a documented reason. src/ keeps the full pedantic bar. More seriously: declared_version_mismatch_matches_the_ci_regression_test, added in the previous commit, landed in abi.rs's `live_tests` module rather than `tests`. That module is cfg'd on _duckdb-testing, so the test was not compiled when I ran it under default features and my "it passes" check proved nothing. Compiled under bundled-test it failed immediately, and for a real reason: decide's signature is (compiled_slots, declared, engine) and I had the last two swapped. The test now sits in `tests`, runs under every feature set, and asserts the right thing -- declared v1.2.0 (408 slots) against 545-slot bindings yields DeclaredVersionMismatch for every engine argument, which is what makes the abi-guard job deterministic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
CI failed check-abi-table.py with "KNOWN_LAYOUTS is out of date", printing a table that dropped v1.4.4. The script passed locally on the same commit. The cause: fetch() swallowed every exception -- 404, timeout, DNS, 5xx -- and returned None, which the caller printed as "not published (skipped)" and dropped from the derivation. One transient failure on v1.4.4 narrowed the derived range from v1.4.0-v1.4.4 to v1.4.0-v1.4.3, and the script then reported src/abi.rs as stale and told the maintainer to replace it with the narrower table. Following that advice would have shrunk the layout table and made the runtime guard refuse DuckDB versions it should accept -- the exact failure the table exists to prevent. fetch() now returns "ok", "missing" (a definite 404, routine for a version that does not exist yet) or "error", retries transient failures, and a tag that could not be downloaded suspends the staleness comparison via exit 2 rather than failing it. Verified by pointing the proxy at a closed port: exit 2, no drift claimed. The other three guards each fetch a single file, so a failure already meant no data rather than partial data. Separately, all four guard jobs ran the scripts bare, so exit 2 -- their documented "could not check" code -- failed the job. Every guard was one network flake away from a red build. They now surface exit 2 as a warning and fail only on exit 1. Also fixes the DuckDB CLI jobs' real error, which the diagnostics added in bf531a8 finally revealed: "Cannot change allow_unsigned_extensions setting while database is running". That setting has to be the -unsigned startup flag, not a SET statement, so all three jobs now pass -unsigned and the smoke test exercises the same invocation the probes use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Owner
Author
CI is green — 67 of 69 checks passThis closes out follow-up #1 in the description ("watch the first CI run"), and supersedes its "CI has never run on this branch" note. The macOS and Windows legs — the unverified surface that note was flagging — both pass, on both the The only red is The first CI run earned its keepFollow-up #1 asked someone to confirm the four network-dependent guard jobs exit 2 (warn, not fail) when GitHub is unreachable. They did not, and finding out why turned up the worse half of the bug:
Applying that advice would have shrunk the ABI layout table and made the runtime guard refuse DuckDB versions it should accept — the exact failure mode the table exists to prevent. A guard that flakes into recommending you weaken it is worse than no guard. A definite 404 is now distinguished from every other failure, transient errors retry, and a tag that cannot be downloaded suspends the staleness comparison (exit 2) instead of failing it. Verified against a closed port: exit 2, no drift claimed. The other three guards each fetch a single file, so an empty fetch already meant no data rather than partial data. And the premise of follow-up #1 was itself wrong: all four guard jobs ran the scripts bare, so exit 2 failed the job regardless of what the scripts intended. Every guard was one network flake from a red build. They now warn on exit 2 and fail only on exit 1. The other thing the run caughtThe three DuckDB CLI jobs failed on Remaining follow-upsUnchanged from the description: Generated by Claude Code |
…olicing Security (OSV / GHSA) was the only red check. It fails on RUSTSEC-2026-0235 in rkyv 0.7.46, which no build of this crate can reach: osv-scanner reads Cargo.lock, and Cargo pins optional dependencies there whether or not their feature is enabled, so the job flags a crate that is never compiled. The chain is quack-rs -> duckdb (optional dependency, enabled by bundled-test) -> rust_decimal 1.40.0 -> rkyv (optional feature of rust_decimal, not enabled). rust_decimal itself IS built under bundled-test; rkyv is not. `cargo tree --all-features --target all -e normal,build` shows exactly one rust_decimal node and zero rkyv nodes. The graph-aware cargo-deny job passes on this same lockfile. It is not fixable here either: rust_decimal 1.40 constrains rkyv to ^0.7 and the fix is 0.8.17. The advisory is red on main too. Suppressed via a new osv-scanner.toml carrying the reachability argument in full. The suppression does not depend on that comment remaining true: the osv-scan job re-derives it before every scan and fails if an rkyv node ever enters the tree, so enabling the feature later cannot silently mask a real vulnerability. Two details the check gets right deliberately. It tests the forward tree, because `cargo tree -i` exits 0 with "nothing to print" for a package that is in the lockfile but has no active edge -- it cannot distinguish "not built" from "built" by exit code, and an earlier draft of this guard was wrong for exactly that reason. And it anchors on rust_decimal being present, so a failed or truncated tree reports as unverified rather than as clean; the regex is bounded so rkyv_derive and ryu do not match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…aph node The reachability guard added in a2319af failed CI with "rkyv is now in the build graph", and the line it matched was: Downloaded rkyv v0.7.46 That is cargo's download progress on stderr, not a tree node. The step captured the tree with 2>&1, folding stderr in, and cargo downloads every lockfile entry -- including ones it never compiles -- so the progress line looks exactly like a node to the regex. It passed locally only because the crate was already cached there and no Downloaded line was emitted; a cold runner produces one every time. Capture stdout alone and keep stderr aside for diagnostics. Verified by replaying the exact failure: with stderr folded in the guard fires, with stdout only it passes, the rust_decimal anchor is still found, and a real `└── rkyv v0.7.46` node still trips it. Also switched the pipelines to here-strings, which removes the "printf: write error: Broken pipe" noise from `grep -q` exiting before printf finished writing. Note the scan itself has still never run: the guard failed ahead of it both times, so whether the osv-scanner.toml suppression is honoured is still unproven. That is what this push tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Owner
Author
All 69 checks green68 success, 1 skipped ( What went in
One correction to what I told you earlier: I said The suppression polices itselfA suppression justified by reachability is only as good as that reachability staying true, so the job re-derives it before every scan and fails if an The scan output now carries the reasoning too, so the justification is visible to anyone reading the log rather than buried in a config file: Worth knowing: the guard's first version was wrong in a way that matters. It captured Remaining follow-upsUnchanged, none blocking: Generated by Claude Code |
The suppression config added alongside the OSV job is CI-only configuration, like deny.toml, mutants.toml and codecov.yml -- all of which the exclude list already keeps out of the package. It was shipping to crates.io, where it is inert and just adds noise to what consumers download. Verified with `cargo package --list`: the file is no longer listed, and CHANGELOG.md, README.md and LICENSE still are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
…ally do The table said a removed or changed public API means MAJOR, 0.x.y -> 1.0.0. No release has ever done that. 0.16.0 and 0.8.0 both carry breaking changes and both took a minor bump, and the table's own "MINOR" row described 0.2.x -> 0.3.0 as the backward-compatible bump, which inverts Cargo's rule: for a 0.MINOR.PATCH crate Cargo treats the leftmost non-zero component as the major, so 0.17.0 IS the breaking bump and 0.16.1 is the compatible one. A maintainer following the old table literally would have cut 1.0.0 for a UUID signedness fix. The table now records what the history shows, with a real release cited for each row: no public API change -> PATCH (0.12.1, 0.7.1, 0.5.1) new public API -> MINOR (0.13.0, 0.15.0) changed/removed API -> MINOR (0.16.0, 0.8.0) declaring API stable -> MAJOR (not yet used) Since breaking and additive changes now share the minor position, the version number alone no longer separates them, so the policy points at the bold **Breaking:** CHANGELOG prefix as the way to tell -- the convention 0.16.0 and 0.8.0 already use. Also states that 1.0.0 is a deliberate stability commitment rather than something a single incompatible change triggers, and notes the post-1.0 rules. The MSRV note is deliberately hedged: there is no precedent to match, because 0.13.0 corrected an understated rust-version and 0.16.0 lowered it. Neither was a raise on consumers, so that paragraph is marked as guidance for the first real one rather than as established practice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
The section was dated the day the version bump was written, and the branch has been open since. Refreshed in CHANGELOG.md and mirrored in book/src/reference/changelog.md, which RELEASING.md requires to stay in sync. Each file keeps its own separator -- a hyphen in CHANGELOG.md, an em dash in the book. Nothing enforces this date: the release workflow's validate job only greps `^## \[$VERSION\]`. It needs another refresh if the tag is cut on a later day. The same string appears in src/value.rs, src/datetime.rs and tests/ffi_roundtrip.rs, but as date-literal test data -- one asserts 2026-08-18 is 20685 days after the epoch -- so those are deliberately left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
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.
Summary
A production-readiness review of quack-rs that turned into 24 fixed defects, two of
which corrupted the heap. The method mattered more than any single find: every
documented claim was checked against DuckDB's own C++ sources, a live DuckDB 1.5.5, or
the real community-extensions corpus — rather than against the header comments. Four of
those checks are now CI jobs so the answers stay current. Ships as v0.16.0.
Start here, reviewers:
src/copy_function/info.rs—get_file_pathcalledduckdb_freeoninfo_ref.file_path.c_str(), the interior pointer of a live C++std::stringDuckDB still owns. Every
COPY … TOthrough a quack-rs copy function handed theallocator a pointer it never issued. The first live test of the path aborted with
corrupted size vs. prev_size in fastbins. All twelve otherduckdb_freesites werethen audited against DuckDB's implementations; the rest are correct.
src/abi.rs— new module. DuckDB'sduckdb_ext_api_v1has a frozen 357-slotprefix and an unstable tail into which DuckDB inserts entries between releases,
shifting every later slot. quack-rs called 105 functions past that boundary while
declaring
v1.2.0compatibility, so a mismatched DuckDB loaded it happily and thencorrupted its own heap. Now verified at load time.
src/validate/release_profile.rs— the validator requiredpanic = "abort",the one setting that makes every
catch_unwindguard inert. Proven with a two-lineprobe:
exit 0under unwind,exit 134(SIGABRT) under abort.Type of change
Breaking changes, and why each one is deliberate
read_uuid/write_uuid/Value::uuid/as_uuid:i128→u1280x9111…silently became0x1111…. A same-signature fix would have changed behaviour invisibly.ReleaseProfileCheck::panic_abort→panic_unwindChunkWriter::new,DataChunk::into_chunk_writerno longerconst fnduckdb_vector_size()instead of assuming 2048.ScaffoldConfiggains three fieldsDefault;..Default::default()construction is unaffected.init_extensionnow runs the ABI check by defaultAbiPolicy::Trustrestores the old behaviour.DuckStringView::from_bytesdeprecatedWhat was found
Severity is by what happens when it fires, not by how likely it is. Each was reproduced
before being fixed and has a regression test.
COPY … TOfreed a pointer DuckDB owns → heap corruption every timev0.0.1stamp)validate_release_profiledemanded the setting that disables every panic guardread_uuidandValue::as_uuidmeant different 128 bits; docs said they matcheddescription.ymlvalidator rejected 36 of 43 published extensionstable_descriptiongated behind the unstable ABI they don't needDuckStringView::from_byteswas a safe fn dereferencing arbitrary pointersChunkWriterhardcoded 2048 rows; overruns a smallerSTANDARD_VECTOR_SIZEbuildpanic = "abort", silently disabling the guardscatch_unwindvalidate_function_namemade names DuckDB itself ships unregisterableDebug(C-DEBUG; breaksexpect_err,assert_eq!)linux_amd64_gcc4validate_spdx_licensetold users valid licenses did not existref: main; DuckDB documentsrefas a commit hashsecretsclaimed a bridge to DuckDB's secrets; the C API has zero secret functionsClientContext::catalogdocumented an empty name as "the default"; DuckDB rejects itFileHandle::size/tellreturnedi64, inviting.max(0) as usizeon errorFfiBindData::get_from_bindalways returnedNone; sentif let Somethe wrong waydocs:overwrote realdescription.ymlmetadataPlus
LESSONS.mdP11/P12 and the generated CI workflow's one unpinned action.How they were found
Valuederef, the empty-catalog rejection, theGetScope()assertiondescription.ymlfiles), #15 (746 real function names), #17/#18 (the real distribution matrix and SPDX registry)Checklist
Code quality
cargo test --all-targetspasses — 775 tests + 160 doc tests, 0 failingcargo clippy --all-targets -- -D warningspasses — default,duckdb-1-5,duckdb-1-5-3cargo fmtapplied (no diff)cargo doc --no-depsbuilds without warnings (RUSTDOCFLAGS=-D warnings)unsafeblocks have a// SAFETY:comment — see note belowTesting
cargo mutantsshows zero surviving mutants — not run;cargo-mutantsis notinstalled in this environment. Worth running before merge.
Documentation
CHANGELOG.mdupdated — now released as[0.16.0], regrouped under Keep aChangelog headings (it had reached 22 headings with
### Fixedappearing nine times)book/src/) updated — appender and table-metadata chapters moved out of the1.5-only section, platform/profile/ref guidance corrected, three broken links fixed
LESSONS.mdandbook/src/reference/pitfalls.md— P11 andP12 added; P10 was in
LESSONS.mdfrom last cycle and never mirrored, now isSafety (for changes touching
unsafe)callback macros, plus the four copy-function phases that had none
#[deny(unsafe_op_in_unsafe_fn)]remains satisfiedVerification
Every row is a command that ran on this branch, not an inference from reading source.
duckdb-1-5off — pure stable ABI-D warnings, three feature comboscargo fmt --checkcargo docwithRUSTDOCFLAGS=-D warningscargo publish --dry-rundistribution_matrix.jsonextension-ci-toolspinsdescription.ymlfilesLOAD→ queryStrict/Warn/ no guardCI has never run on this branch. Everything above was verified locally on Linux; the
macOS and Windows matrix legs are unproven. Seven CI jobs are new (
abi-table,abi-guard,scaffold-e2e,platform-table,spdx-list,msrv-vs-duckdb-ci, and astrengthened
extension-load), bringing the workflow to 26 jobs.Picking this up in a future session
Concrete, ranked follow-ups. Nothing here blocks merge.
surface. The four network-dependent guard jobs exit
2(warn, not fail) when GitHubis unreachable — confirm that behaves as intended in CI.
cargo mutantson the changed files; the repo's checklist asks for it and itcould not be run here.
clippy::undocumented_unsafe_blocksliterally, or enable it with anallowwhere the contract genuinely lives on the enclosingunsafe fn. ~140 sites.appender.rs(688) along a lifecycle/row seam;logical_type.rs(1165) andmock_vector.rs(1196) are the older offenders. Then the500-line box can actually be ticked.
wasmas anexcluded_platformsvalue is unresolved. One published extension usesthe bare group name. It is accepted because the four group names are the top-level keys
of
distribution_matrix.json, but whetherextbuildhonours them could not beverified — the Go source needs the GitHub API. Worth confirming.
SSPL-1.0is inCOMMON_SPDX_LICENSESand is not OSI-approved. Real SPDXidentifier, source-available rather than open source. A policy call for the maintainer;
scripts/check-spdx-list.pyflags any new non-OSI entry.duckdb_client_context_get_config_optioncalls
GetScope()before checking the lookup succeeded, so a missing setting aborts adebug build. Its own
default:arm shows the not-found case is meant to be tolerated.The review write-up, with the full reasoning behind each finding, is at
https://claude.ai/code/artifact/2dd13746-2e6c-4f30-b37e-47f40cf68cb2
🤖 Generated with Claude Code
https://claude.ai/code/session_01SQNm9jYKVzHoqhKNMkvvYq
Generated by Claude Code