Rust bindings

[Choochmeque]

Description

Contributor Declaration

By opening this pull request, I affirm the following:

• All authors agree to the Contributor License Agreement.
• The code follows the project's coding standards.
• I have performed self-review and added comments where needed.
• I have added or updated tests to verify that my changes are effective and functional.
• I have run all existing tests and confirmed they pass.

🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-256

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.19%. Comparing base (62cc4bd) to head (dd64d41).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #256   +/-   ##
========================================
  Coverage    71.19%   71.19%           
========================================
  Files          376      376           
  Lines        23762    23762           
  Branches      2478     2478           
========================================
  Hits         16918    16918           
  Misses        6844     6844           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Ozaq]

Review from Claude Code:

---

Code Review: Rust FFI Bindings for FDB

Branch: rust-bindings -> develop
Commits: 3 (439cebe8, 5b89b8ba, 75dd35f7)
Scope: +7,292 lines across 31 files
Date: 2026-04-01
Reviewer: Claude Code

Methodology: Findings are based on static analysis of the source code. No
build or test execution was performed. Line numbers reference the code at commit
75dd35f7.

---

Executive Summary

This branch introduces a well-structured Rust FFI layer for FDB using CXX,
with proper -sys / safe-API crate separation, RAII handle management, and a
solid error-mapping strategy. The main concerns fall into three areas:

1. FFI safety boundaries — panic propagation through C++ frames (C1),
   unverified Send claims on iterators (H3), and potential callback
   use-after-free (H2).
2. Silent data loss — the axes() function drops non-standard axes (H1),
   and three struct fields always return empty data without signaling it (H5).
3. Documentation drift — the README example does not compile against the
   actual API (C2).

Most of these are straightforward to fix. The overall design is sound, and the
checklist at the end of this document is prioritized to help you plan the work.

---

What Went Well

Before diving into findings, it's worth highlighting what this branch does
right — there's a lot of solid work here:

• CXX as the bridge layer is the right call for this codebase. It provides
  compile-time type checking across the FFI boundary, which is significantly
  safer than raw bindgen for a C++ API of this complexity.
• The -sys / safe-API split follows Rust ecosystem conventions and makes
  the unsafe surface area easy to audit.
• The Mutex<HandleInner> pattern correctly addresses thread safety for
  the C++ handle, with the with_handle / with_handle_ref helpers keeping
  lock management clean and consistent.
• The error type hierarchy (error.rs) maps C++ eckit exception types to
  distinct Rust error variants via the trycatch prefix convention — a clever
  approach that gives callers useful pattern-matching granularity.
• Builder-pattern APIs for Key and Request are idiomatic and
  ergonomic.
• Test and bench coverage exists: integration tests, thread-safety tests,
  async tests, and Criterion benchmarks are all present. The gaps identified
  below are specific, not systemic.
• The vendored build system handles a complex multi-library CMake build
  with proper RPATH handling for portable binaries.

---

Severity Levels

Level	Definition	Merge impact
CRITICAL	Undefined behavior, or a user-facing contract that is completely broken	Blocks merge
HIGH	Data correctness, safety gaps, or build reliability issues	Should fix before or shortly after merge
MEDIUM	Robustness, API design, or test quality issues	Fix when convenient
LOW	Ergonomics, style, or minor correctness	Backlog

---

Findings Summary

Severity	Count
CRITICAL	2
HIGH	11
MEDIUM	16
LOW	15

---

CRITICAL

C1. Rust panics in FFI callbacks cause undefined behavior

Files	fdb-sys/src/lib.rs:638-668
Impact	Undefined behavior — memory corruption, segfaults, or silent data corruption

Details:

invoke_flush_callback (line 638) and invoke_archive_callback (line 642) are
declared in the extern "Rust" block (lines 615-631), meaning C++ calls into
Rust through them. If a user-supplied closure panics inside either function,
Rust will attempt to unwind through C++ stack frames. Unwinding across an FFI
boundary is undefined behavior per both the Rust Reference and the C++ standard.

The callback traits at lines 27 and 32 require only Send, not UnwindSafe,
so there is no compile-time protection against closures that could panic:

// fdb-sys/src/lib.rs:638
fn invoke_flush_callback(callback: &FlushCallbackBox) {
    callback.0.on_flush(); // If this panics -> UB
}

CXX protects C++-to-Rust calls via trycatch, but does NOT insert
catch_unwind on extern "Rust" functions. The Rust-to-C++ unwind path is
completely unprotected. Since callbacks are public API, any consumer of the
crate can trigger this.

Recommendation:

Wrap both callback invocations in std::panic::catch_unwind:

fn invoke_flush_callback(callback: &FlushCallbackBox) {
    if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        callback.0.on_flush();
    }))
    .is_err()
    {
        eprintln!("fdb: panic in flush callback (suppressed at FFI boundary)");
    }
}

Consider also adding UnwindSafe to the trait bounds, or documenting that
panicking in a callback is unsupported.

---

C2. README code example does not compile against the actual API

File	rust/crates/fdb/README.md:9-25
Impact	The README example will not compile, blocking new users on first contact

Details:

The README example does not correspond to the actual public API. Every type
name, constructor, and method call is wrong:

README shows	Actual API (from lib.rs re-exports)
use fdb::{FDB, Key, WriteRequest}	use fdb::{Fdb, Key, Request}
FDB::open()?	Fdb::new()?
key.set("class", "od")	Key::new().with("class", "od")
WriteRequest::new(&key)	No such type exists
fdb.archive(&request, &data)?	fdb.archive(&key, &data)?

The crate-level doc in lib.rs (lines 7-23) already has a correct example
that could serve as a starting point.

Recommendation:

Replace the README example with working code. A corrected version:

use fdb::{Fdb, Key, Request};

let fdb = Fdb::new()?;

let key = Key::new()
    .with("class", "od")
    .with("stream", "oper")
    .with("type", "fc");
fdb.archive(&key, &data)?;
fdb.flush()?;

let request = Request::new().with("class", "od");
let reader = fdb.retrieve(&request)?;

Ideally, make it a compiled example (examples/basic.rs) and reference it from
the README so CI catches drift.

---

HIGH

H1. axes() hardcodes axis names, silently drops dynamic axes

File	fdb_bridge.cpp:800-813
Impact	axes() returns incomplete results with no error or warning

Details:

The axes() function iterates over a hardcoded list of 11 axis names:

// fdb_bridge.cpp:800-801
static const std::vector<std::string> common_axes = {
    "class", "expver", "stream", "type", "levtype", "date",
    "time",  "step",   "param",  "levelist", "number"};

Any axis not in this list — domain, origin, frequency, direction,
method, or any user-defined axis — is silently omitted from the result.

Meanwhile, axes_iterator() (which drives AxesIteratorHandle::next() at
lines 674-698) correctly iterates the full axis map via
current_.axes().map(). The two APIs return different results for the same
request, and there is no warning about this.

Recommendation:

Replace the hardcoded list with iteration over the actual axis map, matching
the approach in AxesIteratorHandle::next(). If the list was intentional for
performance, rename the function to common_axes() and document that it
returns a subset.

---

H2. Callback registration may be unsafe under concurrent access

Files	handle.rs:490-507, fdb_bridge.cpp:915-967
Impact	Potential use-after-free if callbacks are registered while C++ is invoking a previous one

Details:

Fdb implements Send + Sync, so on_flush and on_archive can be called
from any thread. On the Rust side (handle.rs:490-507), these methods acquire
the Mutex via with_handle, call the C++ registration function, then release
the lock.

On the C++ side (fdb_bridge.cpp:915-922), register_flush_callback captures
the Rust Box<FlushCallbackBox> in a shared_ptr, wraps it in a lambda, and
passes it to handle.inner().registerFlushCallback(). When a new callback is
registered, C++ replaces the stored std::function, destroying the old lambda
and its captured shared_ptr.

The concern: if C++ is currently executing the old callback (e.g., during a
flush triggered on another thread), the old lambda's captures are destroyed
while still in use. The Rust Mutex only serializes registration from the
Rust side; it does not prevent C++ from concurrently invoking an old callback.

Whether this is a real issue depends on fdb5::FDB's internal
synchronization guarantees, which should be verified.

Recommendation (pick one):

1. Document the C++ guarantee: If registerFlushCallback in fdb5::FDB
   guarantees the old callback is never invoked after registration returns,
   add a comment documenting this. No code change needed.
2. Use Arc: Store callbacks in Arc so that both the old C++ lambda and
   the new registration hold a reference. The old callback stays alive until
   C++ finishes invoking it.
3. Require &mut self: Change on_flush/on_archive to take &mut self,
   preventing concurrent calls.

---

H3. unsafe impl Send on all iterators lacks safety justification

File	iterator.rs (9 instances: lines 55, 136, 190, 234, 279, 322, 369, 421, 467)
Impact	If any C++ iterator holds thread-local state, moving it to another thread causes UB

Details:

Every iterator type in iterator.rs has:

// SAFETY: The underlying C++ iterator is accessed through &mut self only.
#[allow(clippy::non_send_fields_in_send_ty)]
unsafe impl Send for ListIterator {}

This safety comment addresses only the Rust side: since &mut self prevents
concurrent access, Rust won't create data races. But Send makes a stronger
claim — the object is safe to use from a different OS thread than the one
that created it. C++ iterators may hold thread-local allocator handles, TLS
caches, or thread-affine file descriptors.

Without an audit of the underlying fdb5::ListIterator, fdb5::DumpIterator,
etc., this is an unverified safety claim. Rust users will move iterators across
threads (.collect() in a spawned task, channel sends, async runtimes), so if
the C++ iterators have any thread-local dependency, this causes UB that is
invisible at compile time.

Recommendation:

Audit the C++ iterator implementations for thread-local dependencies. Document
the audit result in each unsafe impl block. If thread-locality cannot be
ruled out, remove Send and optionally provide an
unsafe fn into_send(self) escape hatch.

---

H4. Iterators created under lock but used without it

File	handle.rs:143-147 (and all iterator-returning methods)
Impact	If C++ iterators hold back-pointers to the FDB handle, concurrent mutations corrupt iterator state

Details:

All iterator-returning methods follow this pattern:

// handle.rs:143-147
pub fn list(&self, request: &Request, depth: i32, deduplicate: bool) -> Result<ListIterator> {
    let it = self.with_handle(|h| fdb_sys::list(h, ...))?;
    // Mutex lock is dropped here
    Ok(ListIterator::new(it))
    // Iterator now lives independently of the Mutex
}

The iterator is created while the Mutex is held, then returned to the caller
who uses it without any lock. Whether this is safe depends on C++ iterator
semantics. If iterators are snapshot-based (fully independent after creation),
this is fine. If they are cursor-based (reading from the handle's internal
state), this is a data race.

Since users will naturally iterate while performing other operations, this needs
to be clarified.

Recommendation:

Determine whether C++ iterators are snapshot-based or cursor-based:

• If snapshot-based: add a comment in handle.rs confirming this.
• If cursor-based: iterators must hold a MutexGuard or the API must prevent
  concurrent handle mutation while iterators are alive.

---

H5. Three struct fields always return empty data

File	fdb_bridge.cpp (lines 438, 557, 644)
Impact	Callers receive populated structs with empty fields — silent data gap

Details:

Three iterator next() implementations return structs with fields that are
always empty, despite having non-optional types in the struct definition:

C++ function	Struct field	Returns
MoveIteratorHandle::next() (line 644)	destination: String	Always ""
StatusIteratorHandle::next() (line 438)	status: Vec<KeyValue>	Always empty vec
StatsIteratorHandle::next() (line 557)	location: String	Always ""

For example, in MoveIteratorHandle::next():

// fdb_bridge.cpp:643-644
data.source = rust::String(ss.str());    // populated from ostringstream
data.destination = rust::String("");      // hardcoded empty

The MoveElement struct at fdb-sys/src/lib.rs:172-178 declares destination
as String, not Option<String>. A caller has no way to distinguish "empty
destination" from "destination not yet implemented." The same pattern applies to
StatusElement.status and StatsElement.location.

Recommendation (for each field, pick one):

1. Populate it from the C++ objects (preferred if the data is accessible).
2. Change the type to Option<String> / Option<Vec<KeyValue>> and return
   None to signal "not available."
3. Remove the field and document the limitation.

The current approach — returning a populated type with empty data — is the most
problematic option because the type signature promises data that isn't actually
available.

---

H6. build.rs relies on fragile OUT_DIR ancestor traversal

File	fdb-sys/build.rs:545-548
Impact	Build fails or copies libraries to wrong location

Details:

// fdb-sys/build.rs:545-548
let target_dir = Path::new(&out_dir)
    .ancestors()
    .nth(3)  // assumes OUT_DIR = target/<profile>/build/<crate>-<hash>/out
    .expect("Could not determine target directory");

This hardcodes the assumption that OUT_DIR is exactly 3 levels deep inside
the target directory. This breaks when CARGO_TARGET_DIR is set to a custom
path, workspace builds produce different nesting depths, or future Cargo
versions change the OUT_DIR layout. The expect() produces a panic with no
diagnostic information.

Recommendation:

Use the CARGO_TARGET_DIR environment variable if set, or search ancestors for
a directory containing build/. At minimum, improve the error message:

.unwrap_or_else(|| panic!(
    "Could not determine target directory from OUT_DIR={out_dir}. \
     Set CARGO_TARGET_DIR explicitly."
));

---

H7. System build doesn't handle lib64 paths

File	fdb-sys/build.rs:257, 259, 586-587
Impact	Linking fails on RHEL, SUSE, Fedora, and other distros that use lib64

Details:

The system build hardcodes /lib for eckit and metkit link search paths:

// fdb-sys/build.rs:257, 259
println!("cargo:rustc-link-search=native={eckit_root}/lib");
println!("cargo:rustc-link-search=native={metkit_root}/lib");

The fdb5 path is correct (it comes from cmake_find_package, which checks for
lib64 at lines 68-72). But eckit and metkit paths are constructed by appending
/lib, skipping the lib64 check.

The vendored build has the same issue at lines 586-587, where eckit and metkit
lib directories are hardcoded to /lib, while fdb5's own lib directory
correctly checks for lib64 (lines 580-584).

Recommendation:

Apply the same lib64 detection logic used by cmake_find_package:

let eckit_lib = if Path::new(&eckit_root).join("lib64").exists() {
    format!("{eckit_root}/lib64")
} else {
    format!("{eckit_root}/lib")
};

---

H8. Thread safety tests reference non-existent feature flag

File	fdb_thread_safety.rs:8, 13, 16
Impact	Documented test instructions don't work

Details:

The file header says:

// line 8:  With the `thread-safe` feature:
// line 13: Run with: `cargo test --test fdb_thread_safety --features thread-safe`

Cargo.toml for fdb defines only vendored and system features (lines
15-18). There is no thread-safe feature — Fdb unconditionally implements
Send + Sync (handle.rs:511-512). Running the documented command produces:

error: Package `fdb` does not have the feature `thread-safe`

Recommendation:

Remove the feature flag references from the header. Correct the run
instructions to:

cargo test --test fdb_thread_safety --features vendored -- --ignored

---

H9. Five public API methods have zero test coverage

File	handle.rs, test directory
Impact	No regression safety net for core functionality

Details:

A grep of the test directory for these method names returns zero matches:

Method	handle.rs line	Description
archive_raw()	276	Archive raw GRIB data (key extracted from message)
read_uri()	175	Read data from a single URI location
read_uris()	194	Read data from multiple URI locations
read_from_list()	213	Read data from a list iterator (most efficient path)
move_data()	438	Move data to a new location

archive_raw is the primary ingestion path for GRIB data. read_from_list is
documented as the "most efficient" read path.

Recommendation:

Add at least one happy-path integration test per method. For archive_raw,
verify that a GRIB message can be archived and retrieved without an explicit
key. For read_from_list, verify that data archived via archive() can be
listed and read back through this path.

---

H10. Commit messages are too sparse for a 7,000-line introduction

Commits	439cebe8, 5b89b8ba, 75dd35f7
Impact	Git history provides no searchable context for design decisions

Details:

• 439cebe8 — "Initial commit" introduces 28 files and ~7,000 lines with no
  explanation of architecture, design decisions, or known limitations.
• 5b89b8ba — "C++ code formatting" is a cosmetic change that could be
  squashed into the initial commit to keep the history clean.
• 75dd35f7 — "Add GitHub Actions workflow for Rust project with checks and
  tests (example, because it should be reusable-action)" embeds a TODO
  directly in the commit message.

Recommendation:

Before merge:

1. Squash 5b89b8ba into 439cebe8.
2. Rewrite the initial commit message to cover: architecture (CXX bridge
   approach vs bindgen), design decisions (Mutex strategy, iterator model),
   known limitations, and link to any design doc or RFC.
3. Move the CI TODO to a GitHub issue and clean the commit message.

---

H11. No CHANGELOG

Location	Workspace root
Impact	Downstream consumers have no summary of scope or limitations

Recommendation:

Add a CHANGELOG.md covering initial release scope, supported operations,
known limitations, and supported platforms.

---

MEDIUM

M1. DataReaderHandle::tell()/size() callable on closed handle

File	fdb_bridge.cpp:294-306
Impact	May return stale or garbage values after close()

tell() (line 294) and size() (line 301) check impl_ for null but do not
check is_open_. After close() is called (lines 273-278), is_open_ is
false but impl_ is still valid. Calling impl_->position() or
impl_->size() on a closed DataHandle may return stale cached values.

By contrast, read() (lines 280-285) and seek() (lines 287-292) correctly
check both impl_ and is_open_.

Recommendation: Add is_open_ check to both methods, consistent with
read() and seek().

---

M2. trycatch allocates on the error path

File	fdb_bridge.h:21-55
Impact	Exception handling could itself throw under memory pressure

Every catch clause in the global trycatch handler performs std::string
concatenation:

catch (const eckit::SeriousBug& e) {
    fail((std::string("ECKIT_SERIOUS_BUG: ") + e.what()).c_str());
}

std::string concatenation allocates heap memory. Under memory pressure, this
allocation could itself throw bad_alloc. That exception would be caught by
the generic catch (const std::exception& e) or catch (...) handler, and
the original exception type would be lost.

Recommendation: Use std::array<char, N> with snprintf, or static
string prefixes where possible.

---

M3. Vendored build discards write errors

File	fdb-sys/build.rs:365
Impact	CMakeLists.txt patch silently fails

let _ = fs::write(&cmakelists, patched);

If the write fails (permissions, disk full), the build continues with the
unpatched CMakeLists.txt. The patch removes a tests subdirectory that is
"buggy when ENABLE_TESTS=OFF" (per the comment at line 361), so a failed patch
leads to a confusing build failure later.

Recommendation: Use .expect("Failed to patch CMakeLists.txt").

---

M4. Seek implementation has signed overflow potential

File	datareader.rs:92, 100
Impact	Wrapping arithmetic produces incorrect seek positions for large offsets

let size = self.size().cast_signed();   // u64 -> i64, wraps for > i64::MAX
let current = self.tell().cast_signed(); // same issue

cast_signed() performs a wrapping cast. For values exceeding i64::MAX, the
result wraps to a negative number, producing an incorrect seek position. The
Seek trait contract requires returning an io::Error for invalid positions.

Recommendation: Use i64::try_from(self.size()) and return
io::Error::new(ErrorKind::InvalidInput, "file size exceeds i64::MAX") on
failure.

---

M5. config_* methods cannot distinguish missing keys from default values

Files	handle.rs:466-487, fdb_bridge.cpp:216-246
Impact	Callers cannot tell "key doesn't exist" from "value is empty/zero/false"

The config_string, config_int, and config_bool methods return bare values
(String, i64, bool) rather than Option<T>. The C++ side handles missing
keys by returning defaults ("", 0, false) at fdb_bridge.cpp:216-246.
This means:

1. Callers cannot distinguish "key exists with value 0" from "key doesn't
   exist" without a separate config_has() call.
2. The CXX bridge declarations at lib.rs:246-252 don't use Result<>, so
   if the C++ methods throw for any unexpected reason, the process aborts
   rather than returning an error.

Recommendation: Change return types to Option<String>, Option<i64>,
Option<bool>, returning None for missing keys. This eliminates the need
for the separate config_has method.

---

M6. Request::from_str claims infallibility but silently drops data

File	request.rs:92-121
Impact	Malformed input silently loses data

type Err = Infallible;  // line 93

The FromStr implementation declares that parsing can never fail. But at line
114, parts that don't contain = are silently skipped:

if let Some((k, v)) = part.split_once('=') {
    // ... add to request ...
}
// else: silently dropped

So "class=od,INVALID,step=0" parses as "class=od,step=0" with no error.

Recommendation: Either define a ParseError type and return it for
malformed parts, or document the lossy behavior explicitly in the doc comment.

---

M7. read_uris and control take &[String] instead of &[impl AsRef<str>]

File	handle.rs:194, 420
Impact	Forces unnecessary allocations from callers holding &str

Both methods require &[String], so callers with Vec<&str> or literal slices
must allocate owned Strings. The conversion to Vec<String> already happens
internally (lines 195, 422), so accepting &[impl AsRef<str>] would avoid
forcing the caller to do the same.

---

M8. Error type discards C++ source location

File	error.rs:4-49
Impact	C++ error context is lost when translating to Rust errors

The error conversion at error.rs:56-79 parses the prefix added by trycatch
to discriminate error types, but only preserves the message string. eckit
exceptions carry source location information (file, line, function) that is
discarded.

Recommendation: Add an optional source_location: Option<String> field to
error variants. This makes debugging C++ failures from Rust significantly
easier, especially for AssertionFailed and SeriousBug errors.

---

M9. Massive boilerplate in iterator.rs

File	iterator.rs (477 lines, 9 near-identical implementations)
Impact	Maintenance burden; inconsistency risk across iterator types

All 9 iterators follow the exact same pattern: struct with a
UniquePtr<*IteratorHandle>, Iterator impl calling hasNext() then
next(), and unsafe impl Send. The only difference is the element conversion.

Recommendation: Extract a define_iterator! macro:

define_iterator!(ListIterator, ListIteratorHandle, ListElement, |data| {
    ListElement::from_cxx(data)
});

This reduces 477 lines to ~80, and ensures any safety fix (e.g., adding
catch_unwind, changing Send bounds) is applied consistently.

---

M10. No Sync bound on callback traits

File	fdb-sys/src/lib.rs:27, 32
Impact	Callbacks may be invoked from multiple C++ threads without synchronization

pub trait FlushCallback: Send { ... }
pub trait ArchiveCallback: Send { ... }

The traits require Send but not Sync. If fdb5::FDB invokes the callback
from multiple threads (e.g., concurrent flush calls), the callback receives
concurrent &self references without Sync guarantees.

Recommendation: Add Sync to the trait bounds, or document that C++
guarantees single-threaded callback invocation.

---

M11. CI workflow is .yml.example — not active

File	.github/workflows/rust.yml.example
Impact	No CI is running for this branch

The workflow file has an .example extension, so GitHub Actions ignores it.
Even if renamed, the test job runs cargo test --features vendored which
only executes unit tests — all integration tests are #[ignore].

Recommendation: Rename to rust.yml. Add a separate job that runs
--ignored tests in an environment with FDB available.

---

M12. Benchmark fixture path resolves to wrong directory

File	fdb_bench.rs:21-34
Impact	Benchmarks that need fixtures silently skip

fn project_root() -> PathBuf {
    PathBuf::from(manifest_dir)
        .parent()   // -> rust/crates/
        .parent()   // -> rust/
        .to_path_buf()
}

Then at line 34: root.join("tests/fixtures") resolves to
rust/tests/fixtures, which doesn't exist. The actual fixtures are at
rust/crates/fdb/tests/fixtures.

Recommendation: Use CARGO_MANIFEST_DIR directly:

let fixtures_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures");

---

M13. Tests that assert nothing

File	fdb_integration.rs
Impact	Tests pass regardless of whether the operation produced correct results

Three integration tests collect results and print them but make no assertions:

Test	What it should assert
test_fdb_dump (line 329)	Non-empty results; items contain expected content
test_fdb_status (line 370)	Non-empty results; location is non-empty
test_fdb_enabled_identifiers (line 1324)	Default-enabled operations return true

Recommendation: Add at minimum:

assert!(!dump_items.is_empty(), "dump should return at least one item");
assert!(retrieve_enabled, "retrieve should be enabled by default");

---

M14. test_fdb_control_lock_unlock swallows all errors

File	fdb_integration.rs:1264-1288
Impact	Test passes even if every operation fails

All three control operations use if let Ok(iter) = ..., silently ignoring
errors. If all three fail, the test produces no output and passes.

Recommendation: Use expect() so failures surface:

let iter = fdb.control(&request, ControlAction::None, &identifiers)
    .expect("control None should succeed");

---

M15. No concurrent write tests in thread safety suite

File	fdb_thread_safety.rs
Impact	Thread safety of the write path is unverified

The thread safety tests exercise only read-only operations (id(), name(),
dirty(), stats(), list(), axes()). There are no tests for concurrent
archive() + flush(), which is the primary concern for multi-threaded use.
The file even documents the flush caveat (lines 201-207) but doesn't test it.

Recommendation: Add a test with multiple threads archiving concurrently,
followed by concurrent flushes. Verify data integrity by listing and checking
that all archived items are present.

---

M16. findings.md is untracked in repo root

This file appears in git status as untracked. It should not be part of the
merge.

Recommendation: Add to .gitignore or delete before merge.

---

LOW

Low-severity items are summarized in table form below. These are not blocking
and can be addressed at any time.

#	Finding	File(s)	Recommendation
L1	fdb_init not enforced at fdb-sys level	fdb_bridge.cpp:29-39	The fdb crate calls initialize() in Fdb::new() (handle.rs:23-25), but fdb-sys consumers can call new_fdb() without fdb_init(). Add a Once guard inside new_fdb().
L2	DataReaderHandle default move ctor leaves is_open_ inconsistent	fdb_bridge.h:171	After move, the moved-from object has is_open_ = true but impl_ = nullptr. Not a real bug (destructor checks both), but set is_open_ = false for clarity.
L3	Archive callback silently swallows location future failures	fdb_bridge.cpp:948-958	The catch (...) at line 956 silently drops errors from location_future.get(). Log the failure.
L4	const_cast in fdb_init on argv	fdb_bridge.cpp:35-36	Use a char* array instead of const char*[] to avoid the cast.
L5	open()/close() silently no-op when already open/closed	fdb_bridge.cpp:266-278	Consider returning a status or logging.
L6	Key and Request share similar builder patterns	key.rs, request.rs	Key holds single values per entry; Request holds multi-values. Add From<Key> for Request for convenient conversion.
L7	No size_hint() on iterators	iterator.rs	Override if C++ exposes an element count to improve collect() allocation.
L8	list() takes magic i32 depth	handle.rs:143	Replace with an enum (Depth::Database, Depth::Index, Depth::Full).
L9	unsafe_wipe_all parameter name misleading in Rust	handle.rs:355-372	unsafe is a keyword with specific meaning in Rust. Rename to wipe_all.
L10	No LICENSE file in rust/ tree	workspace root	The workspace declares license = "Apache-2.0" but has no LICENSE file. Add one.
L11	SSH git URLs for workspace dependencies	rust/Cargo.toml:20-26	Document SSH access requirements in the README, or provide HTTPS fallback.
L12	Cargo.lock is gitignored	.gitignore	This workspace has benchmarks and examples that benefit from reproducibility. Consider checking it in.
L13	Redundant unsafe impl Send/Sync for Fdb	handle.rs:511-512	Fdb contains Mutex<HandleInner> where HandleInner: Send. parking_lot::Mutex<T: Send> auto-derives Send + Sync, so these explicit impls are redundant and can be removed. (The unsafe impl Send for HandleInner is still needed.)
L14	No crate-level doc on safety model	fdb/src/lib.rs:1-23	Add a "Safety" section explaining thread safety guarantees, error handling strategy, and FFI boundary rules.
L15	build.rs cross-compilation: detects host OS, not target	build.rs:262-265, 493-496	#[cfg(target_os = "...")] in a build script matches the build host. Use env::var("CARGO_CFG_TARGET_OS") instead.

---

Merge Checklist

Blocks merge

Item	Finding	Effort
Add catch_unwind in FFI callbacks	C1	S
Rewrite README example with actual API	C2	S
Populate or remove empty struct fields	H5	S
Squash formatting commit, rewrite messages	H10	S

Fix before merge (high risk if deferred)

Item	Finding	Effort
Fix axes() hardcoded list	H1	S
Audit callback registration safety	H2	M — requires C++ code review
Audit and document C++ iterator thread-locality	H3	M — requires C++ code review
Document iterator independence from handle	H4	S — documentation only
Fix OUT_DIR traversal in build.rs	H6	S
Handle lib64 in system + vendored build	H7	S
Add tests for 5 untested public methods	H9	M

Fix soon after merge (quality / maintainability)

Item	Finding	Effort
Fix thread-safety test instructions	H8	S
Add CHANGELOG	H11	S
Make config_* methods return Option<T>	M5	S
Extract iterator macro	M9	M
Activate CI (rename .yml.example)	M11	S
Fix benchmark fixture paths	M12	S
Add assertions to tests that lack them	M13-M14	S

Backlog

All LOW items and remaining MEDIUM items (M1-M4, M6-M8, M10, M15-M16).