docs(zset-ffi): the ABI's reasoning, a CFFI smoke test and test evidence (improvise-bw4p.1)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze

context/code/zset-ffi.md

@@ -0,0 +1,90 @@
+# `zset-ffi`: the reasoning
+
+`crates/zset-ffi` is a C ABI over `zset` for CFFI callers (the first is Common Lisp). It is a
+separate crate so `zset` stays std-only, serde-free and wasm32-clean (ADR 009 clause 1). It is
+native only: it is **not** in `nix/ci.nix`'s `wasmCrates`, and the workspace-wide native
+build (`--workspace`) already compiles it, so nothing in the nix files changes. The contract
+is `crates/zset-ffi/include/zset_ffi.h`; this file records the decisions behind it. The
+evidence is in `context/test-evidence/zset-ffi.md`.
+
+## Build
+
+`cargo build -p zset-ffi --release` writes `target/release/libzset_ffi.{so,dylib}` (the
+`cdylib`), `libzset_ffi.a` (the `staticlib`) and the rlib. The `rlib` is what the Rust tests
+link, so they call the `extern "C"` functions as plain functions.
+
+## Decisions
+
+1. **Monomorphic.** Keys are byte strings `(ptr, len)`, weights are `int64_t`. The generic
+   `ZSet<K>` is instantiated once, at `Vec<u8>`. A caller encodes whatever it keys by.
+2. **Opaque handles, explicit create and free.** `ZsZSet`, `ZsCircuit` and `ZsRebuild` are
+   `Box::into_raw` pointers; `zs_X_new(&out)` makes one and `zs_X_free` releases it. Freeing
+   null is a no-op. A double free or a use after free is undefined, as in C. Circuit streams,
+   inputs and sinks are `uint32_t` ids local to their circuit, not handles: they own nothing.
+3. **Error codes, never unwinding.** Every function but the frees and two accessors returns
+   `int32_t`: 0, or a `ZS_ERR_*`. Every entry point body runs in `catch_unwind` (`guard`).
+   A `zset` weight-overflow panic is recognised by its message prefix and becomes
+   `ZS_ERR_OVERFLOW`; any other panic is `ZS_ERR_PANIC`. Results come back through
+   out-pointers written only on success. `zs_last_error()` is a thread-local, NUL-terminated
+   message, never null, valid until the next failure on that thread.
+   The Rust panic hook still prints the overflow message to stderr before the code returns,
+   because replacing a process-wide hook from a library is not this library's call. A host that
+   wants silence installs its own hook, or avoids the overflow.
+4. **Failure leaves the Z-set unchanged.** `add`, `sub`, `neg`, `scale` and `decode_add`
+   compute a new value and then assign it, so an overflow half way through cannot leave a
+   half-applied update (`zs_zset_insert` is one addition, so it has no half).
+5. **A failed step poisons the circuit.** A panic in `step` can leave operator state half
+   updated, and the circuit has no rollback (ADR 009 clause 11 is the host's). So the first
+   failure in a step, or in a rebuild step, marks the circuit poisoned, and every later call
+   that uses it returns `ZS_ERR_POISONED`. A caller drops it and builds another. Ids that do
+   not exist, and malformed arguments, are refused before any state changes and do not poison.
+6. **Buffers.** The caller owns every buffer it passes and may reuse it once the call
+   returns; the library copies. The library hands out no memory except handles. Output of
+   variable size uses a two-call protocol: `zs_zset_encode(z, buf, cap, &needed)` always sets
+   `needed`, and writes only if `cap >= needed` (else `ZS_ERR_BUFFER_TOO_SMALL`).
+7. **Wire shape.** A Z-set encodes as entries ascending by key, each `[u32 LE key length][key]
+   [i64 LE weight]`. A keyed element is `pair(k, v) = [u32 LE length of k][k][v]`. These are
+   this crate's own, not clause 9's wire form (that lives in `improvise-protocol`).
+8. **What the circuit offers.** Input, output sinks, `delay`, `integrate`, `differentiate`,
+   `distinct`, `plus`, `filter` and `flat_map` (C callbacks), `index_by` (a callback emits the
+   one group key), `join` over two pair streams, and `aggregate` with count, sum, min and max
+   over `int64` values. `map` is `flat_map` that emits once. `aggregate`'s fold is fixed to
+   those four because a C callback cannot return an `Option` of a group result without a
+   second encoding; richer folds belong to a caller that wants one. The `Trace` interface and
+   the `_in` constructors are Rust-side seams and are not exposed.
+9. **Callbacks** are `extern "C"` function pointers with a `void *ctx`; the library never
+   dereferences `ctx`. A callback emits images by calling the `emit` it is given, which copies
+   the bytes during the call. A callback must not call back into the library and must not
+   unwind (a Rust panic across an `extern "C"` boundary aborts; a CL callback should not
+   non-locally exit through the library). `index_by`'s callback must emit exactly one key and a
+   keyed stream must carry valid pairs; otherwise the step fails with `ZS_ERR_BAD_VALUE`
+   (poisoning). The engine records that fault rather than panicking.
+10. **Not thread-safe.** A handle is used from one thread at a time (the circuit is `Rc`
+    inside); different handles are independent. The error message is per thread.
+11. **The header is hand-written** (cbindgen is not a dependency of this repo). `tests/header.rs`
+    keeps it honest: the set of function names and each function's parameter count must match
+    the exported `extern "C"` functions in `src/lib.rs` (including the `unary_op!` expansions),
+    and every `#define` code must equal the Rust enum. Parameter *types* are checked by review
+    and by the ABI tests, which call each function with the header's types, not by the scanner.
+12. **Versioning.** `zs_abi_version()` and `ZS_ABI_VERSION` are 1. A change that breaks a
+    signature or a code bumps it.
+
+## Calling it from Common Lisp
+
+`crates/zset-ffi/tests/smoke.lisp` is a runnable CFFI program (`defcfun` for each call, a
+foreign string for keys, `with-foreign-object` for out-parameters, the overflow code checked
+and the message read back with `zs-last-error`). It loads the built library and passes. It is
+**not a CI gate**: `sbcl` and `cffi` come from the user's profile and Quicklisp here, and are
+not in the nix dev shell. The command to run it is in the file's header. Sketch of the
+definitions it uses:
+
+```lisp
+(defcfun "zs_zset_new" :int32 (out :pointer))
+(defcfun "zs_zset_insert" :int32 (z :pointer) (key :pointer) (len :size) (weight :int64))
+(defcfun "zs_zset_weight" :int32 (z :pointer) (key :pointer) (len :size) (out :pointer))
+(defcfun "zs_last_error" :string)
+(defcfun "zs_zset_free" :void (z :pointer))
+```
+
+A callback (`filter`, `flat_map`) is a `defcallback` passed with `(callback name)`; its
+`emit` argument is a foreign function pointer to call with `foreign-funcall-pointer`.

context/test-evidence/zset-ffi.md

@@ -0,0 +1,60 @@
+# Test evidence: `zset-ffi`
+
+What each test guards and where a rival failed. Keyed by test name; the reasoning is in
+`context/code/zset-ffi.md`. The tests are `crates/zset-ffi/tests/{abi,header,dylib}.rs`.
+
+**Injected in the source**, one at a time, each reverted and the suite rerun (a build that
+printed `Running` was confirmed each time). The failing tests, by name:
+- `guard` without `catch_unwind`: the overflow tests abort the test binary (`SIGABRT`, a panic
+  across `extern "C"`), which is the failure this crate exists to prevent;
+- `zs_zset_add` that adds in place: `overflow_is_an_error_code_and_leaves_the_set_unchanged`;
+- every panic classed `ZS_ERR_PANIC`: `overflow_is_an_error_code_and_leaves_the_set_unchanged`,
+  `a_weight_overflow_in_a_step_is_a_code_and_poisons_the_circuit`,
+  `a_loaded_library_adds_queries_frees_and_reports_overflow`;
+- a failed step that does not poison: `a_weight_overflow_in_a_step_is_a_code_and_poisons_the_circuit`;
+- a recorded fault that does not poison: `a_malformed_pair_and_an_unknown_id_are_codes`;
+- `rebuild_step` that ignores the deadline: `a_rebuild_resumes_across_calls_and_matches_one_step`;
+- `Min` that reads non-positive weights: `min_and_max_read_only_values_of_positive_weight`
+  (the first version of the suite did not catch this; the test was added for it);
+- a header declaration with a parameter dropped:
+  `the_header_declares_exactly_the_exported_functions_with_their_arity`;
+- a header code renumbered: `the_header_constants_are_the_rust_constants`;
+- `zs_zset_encode` that reports the wrong size: eight ABI tests, among them
+  `a_small_buffer_reports_the_size_it_needs`.
+
+### overflow_is_an_error_code_and_leaves_the_set_unchanged
+
+`a` holds `k = MAX` and `j = 1`; adding a set that overflows `k` must return the overflow code
+and leave **both** keys as they were. The key `j` sorts before `k`, so a partial in-place add
+changes `j` first; a fixture where the overflowing key came first would pass that rival.
+
+### a_weight_overflow_in_a_step_is_a_code_and_poisons_the_circuit
+
+A weight reaches `MAX` in an `integrate`, then one more overflows inside `step`. The step
+returns the overflow code, the next step and a new operator return the poisoned code.
+
+### a_rebuild_resumes_across_calls_and_matches_one_step
+
+Nine keys, chunks of two, a deadline callback that is always expired: the job needs five
+calls' worth of chunks (the callback is counted: five, one per chunk), and the sum of what
+the sink yields equals one uninterrupted step on a second circuit. The loop is bounded at 20.
+
+### min_and_max_read_only_values_of_positive_weight
+
+Weights of mixed sign in one group, where the smallest value has a negative weight, so
+`> 0` and `!= 0` give different minima.
+
+### a_loaded_library_adds_queries_frees_and_reports_overflow
+
+The built `libzset_ffi.{so,dylib}` is opened with `libloading` and driven through symbol
+lookups: the contract a CFFI caller meets, including the exported names, not the rlib.
+
+### the_header_declares_exactly_the_exported_functions_with_their_arity
+
+Parses `src/lib.rs` and the header (comments stripped) and compares names and parameter
+counts; asserts the scanner saw more than 30 functions, so an empty parse cannot pass.
+
+### the_header_constants_are_the_rust_constants
+
+Every `ZS_ERR_*`, `ZS_ABI_VERSION`, `ZS_OK` and the four `ZS_AGG_*` equal the Rust values; the
+last code's number equals the number of codes, so a new code without a header line fails.

crates/zset-ffi/src/engine.rs

@@ -1,6 +1,5 @@
-//! The byte-keyed circuit behind the C ABI, in plain Rust: every fallible operation
-//! returns a `Result`, and a panic from `zset` is caught and classified here.
-//! The reasoning is in `context/code/zset-ffi.md`.
+//! The byte-keyed circuit behind the C ABI, as plain `Result`-returning Rust; panics from
+//! `zset` are caught and classified here. See `context/code/zset-ffi.md`.
 
 use std::any::Any;
 use std::cell::Cell;

crates/zset-ffi/src/lib.rs

@@ -1,6 +1,5 @@
-//! The C ABI over `zset`: byte-string keys, `i64` weights, opaque handles, an `i32` code
-//! from every function. Ownership, lifetimes and the error model: `context/code/zset-ffi.md`
-//! and `include/zset_ffi.h`.
+//! The C ABI over `zset`: byte keys, `i64` weights, opaque handles, `i32` codes.
+//! Ownership and errors: `context/code/zset-ffi.md` and `include/zset_ffi.h`.
 
 pub mod engine;
 
@@ -64,6 +63,13 @@ fn bytes<'a>(p: *const u8, n: usize) -> Result<&'a [u8], Fail> {
     }
 }
 
+fn write_out(buf: *mut u8, data: &[u8]) {
+    if !data.is_empty() {
+        // SAFETY: the ABI contract says `buf` has room for `data`; the caller checked `cap`.
+        unsafe { std::ptr::copy_nonoverlapping(data.as_ptr(), buf, data.len()) };
+    }
+}
+
 fn put<T>(out: *mut T, v: T) -> Result<(), Fail> {
     *as_mut(out)? = v;
     Ok(())
@@ -193,10 +199,7 @@ pub extern "C" fn zs_zset_encode(
                 "the buffer is smaller than *needed",
             ));
         }
-        if !enc.is_empty() {
-            // SAFETY: the contract says `buf` has `cap >= enc.len()` writable bytes.
-            unsafe { std::ptr::copy_nonoverlapping(enc.as_ptr(), buf, enc.len()) };
-        }
+        write_out(buf, &enc);
         Ok(())
     })
 }

crates/zset-ffi/tests/abi.rs

@@ -262,7 +262,7 @@ extern "C" fn upper(_: *mut c_void, k: *const u8, n: usize, emit: engine::Emit,
 }
 
 extern "C" fn even_len(_: *mut c_void, _: *const u8, n: usize) -> i32 {
-    i32::from(n % 2 == 0)
+    i32::from(n.is_multiple_of(2))
 }
 
 extern "C" fn first_byte(
@@ -460,3 +460,40 @@ fn a_rebuild_without_a_deadline_runs_to_completion() {
     zs_rebuild_free(job);
     zs_zset_free(snapshot);
 }
+
+#[test]
+fn min_and_max_read_only_values_of_positive_weight() {
+    let c = C::new();
+    let i = c.input();
+    let (mut lo, mut hi) = (0, 0);
+    assert_eq!(zs_circuit_aggregate(c.0, i, 2, &mut lo), OK);
+    assert_eq!(zs_circuit_aggregate(c.0, i, 3, &mut hi), OK);
+    let (slo, shi) = (c.sink(lo), c.sink(hi));
+    c.push(
+        i,
+        &[
+            (&pair(b"g", &le(5)), 1),
+            (&pair(b"g", &le(1)), -1),
+            (&pair(b"g", &le(9)), -1),
+        ],
+    );
+    assert_eq!(zs_circuit_step(c.0), OK);
+    assert_eq!(c.take(slo), one(&[(&pair(b"g", &le(5)), 1)]));
+    assert_eq!(c.take(shi), one(&[(&pair(b"g", &le(5)), 1)]));
+}
+
+#[test]
+fn an_index_by_callback_that_emits_nothing_is_a_bad_value() {
+    extern "C" fn nothing(_: *mut c_void, _: *const u8, _: usize, _: engine::Emit, _: *mut c_void) {
+    }
+    let c = C::new();
+    let i = c.input();
+    let mut o = 0;
+    assert_eq!(
+        zs_circuit_index_by(c.0, i, Some(nothing), null_mut(), &mut o),
+        OK
+    );
+    c.push(i, &[(b"x", 1)]);
+    assert_eq!(zs_circuit_step(c.0), Code::BadValue as i32);
+    assert!(last_error().contains("exactly one key"), "{}", last_error());
+}

crates/zset-ffi/tests/smoke.lisp

@@ -0,0 +1,42 @@
+;;; A CFFI smoke test: load libzset_ffi, add weights, query, hit the overflow code, free.
+;;; Run: sbcl --non-interactive --load ~/quicklisp/setup.lisp \
+;;;        --eval '(defvar cl-user::*zset-lib* "target/release/libzset_ffi.dylib")' \
+;;;        --load crates/zset-ffi/tests/smoke.lisp
+;;; Not a CI gate: sbcl and cffi are not in the dev shell. See context/code/zset-ffi.md.
+(ql:quickload :cffi :silent t)
+(defpackage :zset-smoke (:use :cl :cffi))
+(in-package :zset-smoke)
+
+(load-foreign-library cl-user::*zset-lib*)
+
+(defcfun "zs_abi_version" :uint32)
+(defcfun "zs_last_error" :string)
+(defcfun "zs_zset_new" :int32 (out :pointer))
+(defcfun "zs_zset_free" :void (z :pointer))
+(defcfun "zs_zset_insert" :int32 (z :pointer) (key :pointer) (len :size) (weight :int64))
+(defcfun "zs_zset_weight" :int32 (z :pointer) (key :pointer) (len :size) (out :pointer))
+(defcfun "zs_zset_len" :int32 (z :pointer) (out :pointer))
+
+(defconstant +overflow+ 2)
+
+(defun check (what got want)
+  (unless (equal got want) (error "~a: got ~s, wanted ~s" what got want))
+  (format t "ok ~a~%" what))
+
+(defun key-call (fn z key &rest more)
+  "Calls FN with Z, a foreign copy of the string KEY's UTF-8 bytes, its length, MORE."
+  (with-foreign-string ((ptr len) key :null-terminated-p nil)
+    (apply fn z ptr len more)))
+
+(with-foreign-object (out :pointer)
+  (check "new" (zs-zset-new out) 0)
+  (let ((z (mem-ref out :pointer)))
+    (check "insert" (key-call #'zs-zset-insert z "k" 40) 0)
+    (check "insert again" (key-call #'zs-zset-insert z "k" 2) 0)
+    (with-foreign-object (w :int64)
+      (check "weight" (progn (key-call #'zs-zset-weight z "k" w) (mem-ref w :int64)) 42)
+      (check "overflow code" (key-call #'zs-zset-insert z "k" (1- (expt 2 63))) +overflow+)
+      (check "error text" (and (search "overflow" (zs-last-error)) t) t)
+      (check "unchanged" (progn (key-call #'zs-zset-weight z "k" w) (mem-ref w :int64)) 42))
+    (zs-zset-free z)))
+(format t "zset smoke: all ok~%")