diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8eb3779..3ed665f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -345,6 +345,64 @@ jobs: --no-fallback -o "$RUNNER_TEMP/user/main" Main env -u ZU_LIBRARY "$RUNNER_TEMP/user/main" + # A binding holds native memory, and the process that finds out later + # is the user's. The suite cannot see that: a test that closes nothing + # and asserts on a message passes, and the memory it left behind is + # somebody else's problem an hour into a run. + # + # So the allocator is asked instead. LeakSanitizer is loaded ahead of + # the JVM, a driver opens and closes every handle this client hands + # out, and the report is read for blocks whose stack names libzu. The + # JVM's own unfreed megabyte is not read, because a JVM does not free + # at exit on purpose and none of it is anything a user can act on. + # + # Both providers, because the two allocate down different paths: FFM + # arenas on one side, NewDirectByteBuffer and a shim on the other, and + # a leak in one of them is invisible from the other. + leaks: + strategy: + fail-fast: false + matrix: + provider: [ffm, jni] + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + + - uses: actions/checkout@v5 + with: + repository: tamnd/zu + path: engine + + # 25 for both rows. The FFM provider cannot be compiled by anything + # older, and the shim asks for JNI 1.8 in JNI_OnLoad, so the JNI + # row is the same code a caller on 17 runs. + - uses: actions/setup-java@v5 + with: + distribution: temurin + java-version: "25" + cache: maven + + - uses: Swatinem/rust-cache@v2 + with: + workspaces: engine + + # Not built with the sanitizer, deliberately. Interposing the + # allocator is enough to see a block nobody freed, and an + # instrumented engine would mean building the whole of Rust twice + # to answer a question about this repository. + - name: Build libzu + working-directory: engine + run: cargo build --release -p zu-capi + + - name: Where the library landed + run: echo "ZU_LIBRARY=$GITHUB_WORKSPACE/engine/target/release/libzu.so" >> "$GITHUB_ENV" + + - name: Build the JNI shim + if: matrix.provider == 'jni' + run: ./scripts/build-shim.sh + + - run: ./scripts/leaks.sh ${{ matrix.provider }} + # What Maven Central will run over the artifacts, run here instead so # that a release is not the first time anyone sees it. javadoc: diff --git a/README.md b/README.md index da1a05c..edad893 100644 --- a/README.md +++ b/README.md @@ -311,6 +311,8 @@ On 17 through 21, the second dependency at the top of this page is the only line One thing to know before your first run: from JDK 24, native access is granted by whoever starts the JVM rather than by the library being called. On a class path that means `--enable-native-access=ALL-UNNAMED` on the command line, or the same thing as an `Enable-Native-Access` line in the manifest of the jar that `java -jar` names. On a module path it names the provider instead, `--enable-native-access=dev.zudb.ffm` or `--enable-native-access=dev.zudb.jni` for whichever one is in play. The FFM provider checks `Module::isNativeAccessEnabled` before the first downcall, so a run without the grant is an exception naming the flag rather than a JVM warning on stderr three frames from any of our code. The JNI provider is not restricted this way on a class path, which is one more thing the 17 artifact is quieter about. +Every handle here holds memory the engine owns, and a `close` that is never reached is a leak nothing in Java can see: the pointer is still reachable from a live object, so no collector calls it garbage, and this client has no `Cleaner` behind it on purpose. The allocator is asked instead. A driver opens and closes every handle the client hands out, failures included, under LeakSanitizer, and the report is read for blocks whose stack names `libzu`. The JVM's own unfreed megabyte is not read, because a JVM does not free at exit by design and none of it is anything a caller can act on. It runs on both providers, since FFM arenas and `NewDirectByteBuffer` are different paths to the same memory. Before the clean run it does the run that is meant to leak, so a green report from a job that never loaded the sanitizer is caught rather than believed. + ## Errors Every failure is a `ZuException`, and the subclass is chosen from the GQLSTATUS class rather than from the message: `ZuSyntaxException` for 42, `ZuDataException` for 22, `ZuTransactionException` for 25 and 40, and so on down. The exception carries the whole diagnostic, so a caller reads fields instead of parsing prose: @@ -358,6 +360,14 @@ ZU_LIBRARY=/path/to/libzu.dylib java -jar zudb-bench/target/benchmarks.jar `ZU_LIBRARY` rather than `-Dzu.library` there, because JMH forks a JVM of its own and a fork inherits the environment rather than the system properties. +The leak run is a script rather than a test, because what reads the result is the allocator rather than an assertion: + +```sh +ZU_LIBRARY=/path/to/libzu.so scripts/leaks.sh ffm +``` + +Linux only, since LeakSanitizer does not exist on macOS, and it wants `gcc` for the runtime and `llvm-symbolizer` for the names. The second argument is how many times round, and it defaults to twenty five. On macOS what covers the same ground is the lifecycle half of the misuse suite, which counts open file descriptors either side of a few hundred failures and needs no allocator to agree with it. + A release is a tag. `v0.11.0` builds `0.11.0`, takes its libraries from the engine release of the same name, runs the suite against the very library it is about to publish, and puts one signed deployment of the whole reactor in the Central portal. The version is never committed: a pom that has to be bumped before a release is a pom that is wrong between the bump and the tag. Nothing is published without a human pressing the button, and dropping a deployment in the portal is the only way a mistake is undone, because a version that went out cannot be taken back. ## Beyond Java @@ -398,7 +408,7 @@ Inside this repository: | JMH benchmarks | `zudb-bench` | | The staged libraries, built by the release rather than by a clone | `zudb-native` | | Every published name and the shape it is published in | `api/surface.txt` | -| Building the shim, staging the libraries, installing on a clean machine | `scripts` | +| Building the shim, staging the libraries, installing on a clean machine, watching the allocator | `scripts` | `api/surface.txt` is generated, one line per exported type and per member a caller outside the module can name, in the spirit of the `api/go1.N.txt` files Go holds itself to. `SurfaceTest` regenerates it and compares, so a change to the API is a change to that file in the same commit, where it is the first thing in the diff rather than something a user finds after the release. Write it down with `mvn -pl zudb test -Dzu.surface.write=true` and review it like any other file: a name that arrived is a minor release, a name that went or changed shape is a major one or a mistake, and the gate says which of the three a diff is while it is still a diff. It reads the compiled classes of the API module and links nothing, so it answers on a clone with no library staged and no Rust installed. diff --git a/scripts/leaks.sh b/scripts/leaks.sh new file mode 100755 index 0000000..7c2893c --- /dev/null +++ b/scripts/leaks.sh @@ -0,0 +1,204 @@ +#!/usr/bin/env bash +# +# What the engine allocated through this binding, and did not get back. +# +# A JVM is not a program a leak checker was designed for. It allocates at +# startup, holds most of it for the life of the process, and frees almost +# none of it at exit, on purpose, because an exiting process has an operating +# system to give its pages back and a shutdown that walks them is time spent +# for nobody. Point LeakSanitizer at a JVM that does nothing at all and it +# reports about a megabyte in several thousand allocations, every one of them +# the JVM's own and none of them anything a user of this client can act on. +# +# So the question this script asks is narrower than "did anything leak". It +# is: of the blocks nobody freed, is any of them one the engine allocated. +# That is answerable, because a leak record carries the stack it was +# allocated from, and a block that came out of libzu has libzu in its stack. +# The filter below is that sentence, and it is the whole gate. +# +# Which makes the negative case load bearing. A report with no libzu in it +# looks exactly like a report from a run where the sanitizer was never +# loaded, the library was never called, or the driver exited early, and +# three of those four are green for the wrong reason. So the gate runs +# first: the same driver, told to drop a database, a connection, a +# statement, a result, an appender and a frame on the floor, which has to +# come back with libzu in the report. If it does not, this script has +# stopped measuring anything and says so. +# +# Linux only. LeakSanitizer is not available on macOS at all, and the +# darwin rows in CI get the misuse suite's descriptor counting instead, +# which catches a leaked handle without needing an allocator to agree. + +set -euo pipefail + +provider="${1:-ffm}" +rounds="${2:-25}" + +case "$provider" in + ffm|jni) ;; + *) echo "usage: $0 [ffm|jni] [rounds]"; exit 2 ;; +esac + +if [ "$(uname -s)" != "Linux" ]; then + echo "LeakSanitizer is a Linux tool, and this is $(uname -s)" + exit 1 +fi + +root="$(cd "$(dirname "$0")/.." && pwd)" +work="${TMPDIR:-/tmp}/zu-leaks-$provider" +rm -rf "$work" +mkdir -p "$work" + +step() { printf '\n=== %s\n' "$1"; } + +# The sanitizer runtime, which has to be first in the process so that its +# allocator is the one both sides call. A gcc install has one and knows +# where it is, and asking it is more durable than a path with a version in +# it. +step "the runtime this run is watched by" +asan="$(gcc -print-file-name=libasan.so)" +if [ "$asan" = "libasan.so" ] || [ ! -e "$asan" ]; then + echo "no libasan.so, which on Debian and Ubuntu is in libasan or gcc" + exit 1 +fi +echo "$asan" + +# Without a symbolizer every frame is a hexadecimal address, the filter +# below has no name to match, and the report comes back empty for a reason +# that has nothing to do with leaks. Same resolution zu-go uses. +symbolizer="$(command -v llvm-symbolizer || ls /usr/bin/llvm-symbolizer-* 2>/dev/null | head -1 || true)" +if [ -z "$symbolizer" ]; then + echo "no llvm-symbolizer, so every frame would be an address and nothing would match" + exit 1 +fi +echo "$symbolizer" + +step "the engine this run calls" +if [ -z "${ZU_LIBRARY:-}" ]; then + echo "set ZU_LIBRARY to a libzu.so, which is what CI does after it builds one" + exit 1 +fi +ls -l "$ZU_LIBRARY" + +step "building the driver and the $provider provider" +modules="zudb,zudb-tck,zudb-$provider" +mvn -B -ntp -pl "$modules" -am -DskipTests package + +classpath="$root/zudb/target/classes:$root/zudb-tck/target/classes:$root/zudb-$provider/target/classes" + +# handle_segv off and a user handler allowed, because the JVM installs +# signal handlers of its own and uses the faults they catch as ordinary +# control flow: null checks, safepoint polls, stack banging. A sanitizer +# that takes those first turns a working JVM into a crash on the first +# query. exitcode 0 because the report is what is being read here, not the +# status, and detect_odr_violation off because a JVM maps the same symbols +# from more than one place and means to. +export ASAN_OPTIONS="detect_leaks=1:handle_segv=0:allow_user_segv_handler=1:detect_odr_violation=0:abort_on_error=0:exitcode=0" +export ASAN_SYMBOLIZER_PATH="$symbolizer" +export LD_PRELOAD="$asan" + +# From JDK 24 native access belongs to whoever starts the JVM, and this is +# that. Every JDK since 17 accepts the flag, so asking whether it does is +# cheaper than deciding from a version number. +grant="" +if java --enable-native-access=ALL-UNNAMED -version >/dev/null 2>&1; then + grant="--enable-native-access=ALL-UNNAMED" +fi + +# One leak record, in the shape LSan writes them: a heading, the stack that +# allocated it, and a blank line. Prints the records that are ours, and what +# makes a record ours is frame #1 rather than any frame, because #0 is the +# sanitizer's own interceptor and #1 is whoever called malloc. +# +# The distinction earns its keep on the JNI row. Asking for a jmethodID +# allocates a JVM-side table entry which the JVM never frees, by design, and +# the stack for it runs through the shim because the shim is what asked. Any +# frame at all would call that ours and it is not: at #1 it is os::malloc in +# libjvm. A block the shim really did allocate has the shim at #1 and is +# caught, which is the point of naming the shim here at all. +# +# The symbol test beside the library names is for an engine built with debug +# info, where a frame reads zu_query at a Rust source line instead of naming +# the library it came out of. +ours() { + awk ' + /^(Direct|Indirect) leak of/ { inside = 1; count = 0; ours = 0 } + !inside { next } + { record[++count] = $0 } + /^[[:space:]]*#1 / && (/libzu\.(so|dylib)/ || /libzudb_jni\./ || /in zu_[a-z]/) { + ours = 1 + } + /^[[:space:]]*$/ { + if (ours) { for (i = 1; i <= count; i++) print record[i] } + inside = 0 + } + ' "$1" +} + +# The wider question, for the line below that says what was let through: how +# many records name one of ours anywhere in the stack rather than at the top +# of it. +mentions() { + awk ' + /^(Direct|Indirect) leak of/ { inside = 1; seen = 0 } + !inside { next } + /libzu\.(so|dylib)/ || /libzudb_jni\./ || /in zu_[a-z]/ { seen = 1 } + /^[[:space:]]*$/ { if (seen) total++; inside = 0 } + END { print total + 0 } + ' "$1" +} + +run() { + local name="$1" file="$2" + shift 2 + set +e + # shellcheck disable=SC2086 + env "$@" java ${grant:+$grant} -Xmx512m -Dzu.provider="$provider" \ + -cp "$classpath" dev.zudb.tck.Leaks "$rounds" \ + > "$work/$name.out" 2> "$file" + local status=$? + set -e + cat "$work/$name.out" + if [ $status -ne 0 ]; then + echo "the driver exited $status, and a driver that did not finish has not measured anything" + sed -n '1,80p' "$file" + exit 1 + fi +} + +step "the gate: one of everything dropped on the floor" +run gate "$work/gate.txt" ZU_LEAK_GATE=1 +ours "$work/gate.txt" > "$work/gate-ours.txt" +if [ ! -s "$work/gate-ours.txt" ]; then + echo "the gate leaked on purpose and the report has no libzu in it, so this" + echo "script is not measuring what it says it measures" + grep -c "leak of" "$work/gate.txt" || true + sed -n '1,40p' "$work/gate.txt" + exit 1 +fi +echo "the gate leaked and was caught, in $(grep -c "leak of" "$work/gate-ours.txt") records:" +sed -n '1,12p' "$work/gate-ours.txt" + +step "the run that is meant to be clean" +run clean "$work/clean.txt" +ours "$work/clean.txt" > "$work/clean-ours.txt" + +step "what the report says" +grep "^SUMMARY: AddressSanitizer" "$work/clean.txt" || echo "no summary, which means nothing leaked at all" +echo "records in total: $(grep -c "leak of" "$work/clean.txt" || true)" +echo "records allocated by us: $(grep -c "leak of" "$work/clean-ours.txt" || true)" +# Said out loud rather than dropped quietly, because a record that names one +# of our libraries somewhere below the top of its stack is a record this +# script decided not to fail on, and a decision nobody can see is a decision +# nobody can argue with. On the JNI row this number is the JVM's jmethodID +# table and is expected to be small and steady. +echo "records that only pass through us: $(($(mentions "$work/clean.txt") - $(grep -c "leak of" "$work/clean-ours.txt" || true)))" + +if [ -s "$work/clean-ours.txt" ]; then + echo + echo "the engine allocated these through the $provider provider and never got them back:" + cat "$work/clean-ours.txt" + exit 1 +fi + +step "nothing the engine allocated is still out, on the $provider provider" diff --git a/zudb-tck/src/main/java/dev/zudb/tck/Leaks.java b/zudb-tck/src/main/java/dev/zudb/tck/Leaks.java new file mode 100644 index 0000000..23aa3de --- /dev/null +++ b/zudb-tck/src/main/java/dev/zudb/tck/Leaks.java @@ -0,0 +1,253 @@ +package dev.zudb.tck; + +import dev.zudb.Appender; +import dev.zudb.Connection; +import dev.zudb.Database; +import dev.zudb.Frame; +import dev.zudb.Loader; +import dev.zudb.Result; +import dev.zudb.Statement; +import dev.zudb.Zu; +import dev.zudb.ZuException; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.LongBuffer; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.List; +import java.util.stream.Stream; + +/** + * Every handle this client hands out, opened and closed until an allocator has + * something to say about it. + * + *

This is a program rather than a test, because the thing that reads the + * result is not an assertion, it is the leak checker the process was started + * under. {@code scripts/leaks.sh} runs this with the sanitizer's allocator + * interposed and then reads the report for blocks the engine allocated and + * nobody gave back. A run of this on its own, printing its last line and + * exiting zero, has said nothing at all. + * + *

What it is written against is the shape a leak has in a binding, which is + * not the shape it has in a library. A handle here is memory the engine owns + * and a Java object holds, and the way it goes missing is a close that was + * never reached: a failure that returned before it, a value the engine refused + * halfway through a row, a statement that threw where a result was being + * built. So the failures run beside the successes below, because the happy + * path is the one already covered by two hundred other tests. + * + *

There is no {@code Cleaner} in this client and nothing waits for a + * collector, which is a decision the API documents rather than an oversight: a + * handle that is dropped stays open until the process ends. That is what makes + * this apparatus the only thing that can see the mistake, and it is what makes + * the gate worth having. Run with {@code ZU_LEAK_GATE=1} and this drops one of + * everything on the floor on purpose, so that a clean report from a run which + * never interposed the allocator cannot pass for a clean report from one that + * did. + */ +public final class Leaks { + + /** How many times round, when nobody says. Enough to be seen, quick enough to run. */ + private static final int ROUNDS = 25; + + private Leaks() {} + + public static void main(String[] args) throws IOException { + int rounds = args.length > 0 ? Integer.parseInt(args[0]) : ROUNDS; + Path dir = Files.createTempDirectory("zu-leaks"); + try { + System.out.println("zu " + Zu.version() + " through the " + Zu.provider() + " provider"); + if (gate()) { + drop(dir.resolve("gate.zu1")); + System.out.println("one of everything dropped on the floor, which the report should say"); + return; + } + for (int round = 0; round < rounds; round++) { + whereItLies(); + onDisk(dir.resolve("round" + round + ".zu1")); + whatFails(dir.resolve("failing" + round + ".zu1")); + } + System.out.println(rounds + " rounds, every handle closed"); + } finally { + remove(dir); + } + } + + /** + * A database with no file under it and a frame the engine reads where it + * lies, which is the shortest life any handle here has. + */ + private static void whereItLies() { + try (Database db = Database.memory(); + Connection conn = db.connect(); + Frame frame = Frame.of("Person", 3)) { + frame.column("id", longs(1, 2, 3)); + conn.register(frame); + try (Result r = conn.query("MATCH (p:Person) RETURN p.id AS id ORDER BY id")) { + // A row at a time, a column at a time and a chunk at a time, because + // the three take different paths out of the engine and only one of + // them is a copy. + long total = 0; + for (var row : r) { + total += row.getLong(0); + } + LongBuffer values = r.longs(0); + ByteBuffer valid = r.valid(0); + for (int i = 0; i < values.remaining(); i++) { + total += valid.get(i) != 0 ? values.get(i) : 0; + } + r.chunks().forEach(chunk -> chunk.longs(0)); + if (total != 12) { + throw new AssertionError("the engine answered " + total + " rather than 12"); + } + } + conn.unregister("Person"); + } + } + + /** A database on a disk, and the handles that only exist for one. */ + private static void onDisk(Path path) { + people(path); + try (Database db = Database.open(path); + Connection conn = db.connect()) { + try (Statement stmt = conn.prepare("MATCH (p:Person) WHERE p.id = $id RETURN p.name AS n")) { + for (long id = 1; id <= 3; id++) { + try (Result r = stmt.bind("id", id).execute()) { + r.rows(); + } + } + } + try (Appender rows = conn.appender("Person")) { + rows.append(4L).append("hedy").endRow(); + rows.append(5L).append("katherine").endRow(); + rows.finish(); + } + // An appender closed without being finished, and one told to throw + // away what it has, because the three endings free different things. + try (Appender rows = conn.appender("Person")) { + rows.append(6L).append("mary").endRow(); + } + try (Appender rows = conn.appender("Person")) { + rows.append(7L).append("dorothy").endRow(); + rows.discard(); + } + conn.transaction( + () -> { + try (Result r = conn.query("MATCH (p:Person) RETURN p.id AS id")) { + r.rows(); + } + }); + try (Connection second = conn.duplicate(); + Result r = second.query("MATCH (p:Person) RETURN p.name AS name")) { + r.rows(); + } + } + } + + /** + * The same handles, on the paths that never reach the end. + * + *

This is where a leak lives. A statement that fails has allocated on the + * way to failing, a value the engine refuses has half a row behind it, and a + * caller who catches all of that never reaches the line that would have + * freed anything. + */ + private static void whatFails(Path path) { + people(path); + try (Database db = Database.open(path); + Connection conn = db.connect()) { + raises(() -> conn.execute("MATCH (p:Person) RETRUN p.id")); + raises(() -> conn.execute("RETURN nobody")); + raises(() -> conn.prepare("MATCH (p:Person RETURN p")); + try (Result r = conn.query("MATCH (p:Person) RETURN p.id AS id, p.name AS name")) { + raises(() -> r.longs(1)); + raises(() -> r.row(99)); + raises(() -> r.row(0).getLong("nope")); + } + // A fresh appender for each of these, because an appender that has + // already refused a value is not the state the next one is testing. + try (Appender rows = conn.appender("Person")) { + raises(() -> rows.append("four").append("hedy").endRow()); + } + try (Appender rows = conn.appender("Person")) { + raises(() -> rows.row(4L, List.of("hedy"))); + } + try (Appender rows = conn.appender("Person")) { + raises(() -> rows.append(4L).endRow().flush()); + } + raises(() -> Database.open(path.resolveSibling("never-was-a-database.zu1")).close()); + } + } + + /** + * One of everything, opened and never closed. + * + *

Nothing below is a mistake this client would make. It is the mistake a + * user makes, written down once, so that the apparatus which is meant to + * catch it can be seen catching it. + */ + private static void drop(Path path) { + people(path); + Database db = Database.open(path); + Connection conn = db.connect(); + Statement stmt = conn.prepare("MATCH (p:Person) RETURN p.id AS id"); + Result result = stmt.execute(); + Appender rows = conn.appender("Person"); + rows.append(9L).append("gate").endRow(); + Frame frame = Frame.of("Held", 1); + frame.column("id", longs(1)); + conn.register(frame); + // Every one of those is still open, and this client has no cleaner, so + // none of it is coming back. The condition is here so that a compiler + // cannot decide any of the above was work nobody wanted. + if (result.rows() < 0 || db.isClosed() || stmt.isClosed() || rows.isFinished() + || frame.isClosed()) { + throw new AssertionError("unreachable, and here to keep the handles above alive"); + } + } + + /** Three people, in a database that did not exist a moment ago. */ + private static void people(Path path) { + try (Loader loader = Loader.create(path)) { + loader.table("Person", "Knows", 3); + loader.column("id", 1L, 2L, 3L); + loader.column("name", "ada", "grace", "lynn"); + loader.finish(); + } + } + + /** Runs a call that is here because it fails, and insists that it failed. */ + private static void raises(Runnable wrong) { + try { + wrong.run(); + } catch (ZuException expected) { + return; + } + throw new AssertionError("a call that is here because it fails did not fail"); + } + + /** A direct buffer of longs, which is the only kind a frame reads in place. */ + private static LongBuffer longs(long... values) { + LongBuffer buffer = + ByteBuffer.allocateDirect(values.length * 8).order(ByteOrder.nativeOrder()).asLongBuffer(); + buffer.put(values).flip(); + return buffer; + } + + /** Whether this is the run that is meant to leak. */ + private static boolean gate() { + String asked = System.getenv("ZU_LEAK_GATE"); + return asked != null && !asked.isBlank() && !asked.equals("0"); + } + + /** The temporary directory, and everything under it. */ + private static void remove(Path dir) throws IOException { + try (Stream walk = Files.walk(dir)) { + for (Path each : walk.sorted(Comparator.reverseOrder()).toList()) { + Files.deleteIfExists(each); + } + } + } +}