From f63f4677548049a607fb82c1676dadacabcabd16 Mon Sep 17 00:00:00 2001 From: Tam Nguyen Duc <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 04:33:05 +0700 Subject: [PATCH 1/9] the lifecycle half, and the segfault writing it found DX3 asks for a misuse suite and a lifecycle suite in every client. The misuse half has been here since DX2; this is the other one, and the two defects it turned up. A parameter that contains itself was a segfault. The conversion walked the value recursively, so a dict holding itself ran the C stack out and took the interpreter with it, which arrives as a signal rather than as a RecursionError because the recursion is on this side of the boundary. It is refused at depth 64 now, where a real value never reaches and a cycle always does, as a ValueError, which is what the standard library raises for a circular reference. A row appended through a connection that was closed under the appender was accepted and buffered. The flush was where it was noticed, so the same call was refused or not depending on whether the batch happened to fill, which is a rule nobody can hold in their head. The connection is checked where the appender is, at the call that made the mistake. The lifecycle tests are three: a thousand connections opened and closed, a thousand opened and dropped for the callers who never write close, and one closed with an appender and a transaction still open on it. Each counts the process descriptors from outside rather than trusting the client to report on itself. --- src/appender.rs | 18 +++++ src/value.rs | 27 ++++++- tests/test_misuse.py | 163 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 201 insertions(+), 7 deletions(-) diff --git a/src/appender.rs b/src/appender.rs index 3b9c41f..cbdfd70 100644 --- a/src/appender.rs +++ b/src/appender.rs @@ -396,11 +396,29 @@ impl Appender { /// The buffers, for a call that adds to them, which a closed /// appender has no business doing. + /// + /// The connection is checked as well as the appender, because a row + /// appended through a closed connection has nowhere to go and the + /// buffer is the only thing that would take it. Left to the flush, + /// the same call would be refused or not depending on whether the + /// batch happened to fill, which is a rule nobody can hold in their + /// head. It is refused here instead, at the call that made the + /// mistake, whatever the buffer is holding. fn writable(&self, py: Python<'_>) -> PyResult> { let state = self.locked(py)?; if !state.open { return Err(Snag::Finished.raise(py)); } + if self + .conn + .bind(py) + .borrow() + .inner + .lock() + .is_ok_and(|c| c.is_none()) + { + return Err(Snag::Closed.raise(py)); + } Ok(state) } } diff --git a/src/value.rs b/src/value.rs index fbaaedf..9d5f6f8 100644 --- a/src/value.rs +++ b/src/value.rs @@ -478,6 +478,27 @@ fn zone_of(py: Python<'_>, offset: i16) -> PyResult> { /// number against its own spelling would answer nothing and say /// nothing. pub fn from_py(value: &Bound<'_, PyAny>) -> PyResult { + nested(value, 0) +} + +/// How deep a parameter may nest before this stops reading it. +/// +/// A list of lists of records is a value somebody meant to send, and a +/// value that contains itself is a call that would otherwise walk until +/// the stack ran out and take the interpreter with it, which it does +/// here rather than in Python and so arrives as a segfault instead of a +/// `RecursionError`. There is no depth between the two that anybody +/// writes on purpose, so the limit is set where a real value never +/// reaches and a cycle always does. +const DEEP: usize = 64; + +fn nested(value: &Bound<'_, PyAny>, depth: usize) -> PyResult { + if depth > DEEP { + return Err(pyo3::exceptions::PyValueError::new_err(format!( + "a parameter nests deeper than {DEEP}, which is what a value that contains itself \ + looks like" + ))); + } if value.is_none() { return Ok(Value::Null); } @@ -531,7 +552,7 @@ pub fn from_py(value: &Bound<'_, PyAny>) -> PyResult { return Ok(Value::List( items .iter() - .map(|item| from_py(&item)) + .map(|item| nested(&item, depth + 1)) .collect::>()?, )); } @@ -539,14 +560,14 @@ pub fn from_py(value: &Bound<'_, PyAny>) -> PyResult { return Ok(Value::List( items .iter() - .map(|item| from_py(&item)) + .map(|item| nested(&item, depth + 1)) .collect::>()?, )); } if let Ok(fields) = value.cast::() { let mut out = Vec::with_capacity(fields.len()); for (name, item) in fields.iter() { - out.push((name.extract::()?, from_py(&item)?)); + out.push((name.extract::()?, nested(&item, depth + 1)?)); } return Ok(Value::record(out)); } diff --git a/tests/test_misuse.py b/tests/test_misuse.py index d8ada2e..faaf14a 100644 --- a/tests/test_misuse.py +++ b/tests/test_misuse.py @@ -1,6 +1,6 @@ """Deliberately wrong programs, and what each of them is told. -DX2 asks for a misuse suite in both clients: no crash, no leak, and a +DX3 asks for a misuse suite in every client: no crash, no leak, and a clear error for every program that is wrong on purpose. Clear is the hard word of the three, so it is spelled out here as three things a message has to do. It names the thing the caller named, being the file @@ -19,10 +19,18 @@ caller who mistyped one to file a bug would be the wrong answer twice. No crash is the suite running at all. No leak is checked from outside -the call that would cause one, twice: every case is followed by a read -on the connection it was aimed at, and the failing connects are +the call that would cause one, three ways: every case is followed by a +read on the connection it was aimed at, the failing connects are repeated five hundred times, which is past the descriptor limit a -process starts with. +process starts with, and the descriptors themselves are counted where +the operating system will say. + +The lifecycle tests are the second half DX3 asks for. A misuse suite +watches what a wrong program is told; a lifecycle suite watches what a +right program leaves behind, which is the failure nobody sees until the +loop has run for a week. Opened and closed, opened and dropped, closed +with something still open on it: each of the three is counted rather +than described. The last two tests are the half of a misuse suite that is usually missing. The programs that look wrong and are not, each of which is a @@ -134,6 +142,12 @@ def read_only(where: Path) -> zudb.Connection: TypeError, ("a parameter cannot be a object", "zu holds"), ), + Misuse( + "passes a parameter that contains itself", + lambda conn, tmp: conn.execute("RETURN $x AS x", {"x": knot()}), + ValueError, + ("nests deeper than 64", "contains itself"), + ), Misuse( "passes the parameters as a list", lambda conn, tmp: conn.execute("RETURN $x AS x", [1, 2]), @@ -247,6 +261,14 @@ def read_only(where: Path) -> zudb.Connection: ) +def knot() -> dict[str, object]: + """A dict that holds itself, which is the value a conversion written + the obvious way walks until the stack runs out.""" + tied: dict[str, object] = {} + tied["self"] = tied + return tied + + def closed_appender(conn: zudb.Connection) -> zudb.Appender: """An appender that has been closed, which is the state a `with` block leaves one in.""" @@ -332,6 +354,131 @@ def test_five_hundred_failed_connections_leave_nothing_open(tmp_path: Path) -> N assert alive == [] +def descriptors() -> int | None: + """How many files this process has open, or `None` where the + operating system will not say. + + Linux and macOS both keep the answer in a directory, at different + paths, and Windows keeps it nowhere a process can read. A count is + not a leak detector on its own, which is why what the tests below + compare is the count before a thousand cycles against the count + after. + """ + for where in ("/proc/self/fd", "/dev/fd"): + if Path(where).is_dir(): + return len(list(Path(where).iterdir())) + return None + + +def test_a_thousand_connections_opened_and_closed_leave_nothing_behind( + tmp_path: Path, +) -> None: + """The loop a job runs, a thousand times. + + A descriptor kept per connection is a program that works in a + notebook and dies overnight, and it is invisible until the count is + taken from outside. Eight paths rather than a thousand, because what + is being counted is connections and not files, and reopening the + same database is the shape a job actually has. + """ + before = descriptors() + + for step in range(1000): + conn = zudb.connect(tmp_path / f"cycle{step % 8}.zu1") + conn.execute("INSERT (p:person {uid: 1, name: 'ada'})") + conn.close() + + gc.collect() + if before is not None: + assert descriptors() == before + + # And nothing is holding one open behind the collector's back. + alive = [obj for obj in gc.get_objects() if isinstance(obj, zudb.Connection)] + assert alive == [] + + +def test_a_thousand_connections_dropped_rather_than_closed_leave_nothing_behind( + tmp_path: Path, +) -> None: + """The same loop written by somebody who never calls `close`. + + Which is most people most of the time, so the collector taking a + connection has to release the file the way `close` does. A client + where only the explicit path is clean is a client that leaks for + every caller who used a `for` loop and trusted the language. + """ + before = descriptors() + + for step in range(1000): + conn = zudb.connect(tmp_path / f"dropped{step % 8}.zu1") + del conn + + gc.collect() + if before is not None: + assert descriptors() == before + + +def test_a_connection_closed_with_things_open_on_it_closes_them_too( + tmp_path: Path, +) -> None: + """Close is the one call a caller makes in a `finally`, so it has to + work from every state rather than from the tidy one. + + An appender with rows in it and a transaction that never ended are + the two things that can be open when it arrives. Neither may hold + the file after, and neither may fail the close: a cleanup path that + raises is a cleanup path that hides the error it was cleaning up + after. + """ + before = descriptors() + + conn = zudb.connect(tmp_path / "open.zu1") + conn.execute("INSERT (p:person {uid: 1, name: 'ada'})") + rows = conn.appender("person") + rows.append_row([2, "grace"]) + txn = conn.transaction() + txn.__enter__() + assert conn.in_transaction + + conn.close() + assert conn.closed + + # Both say so rather than reaching through a connection that is + # gone, which is the difference between a message and a segfault. + # Caught with `except` rather than with `pytest.raises`, because the + # traceback that one keeps holds the appender alive past the `del` + # below and its warning would then land in whatever test ran next. + try: + rows.append_row([3, "kay"]) + except zudb.ProgrammingError: + pass + else: + pytest.fail("appending through a closed connection returned normally") + + try: + txn.commit() + except zudb.ProgrammingError: + pass + else: + pytest.fail("committing through a closed connection returned normally") + + # The rows that were buffered went nowhere, which is worth the + # warning it gets: the close was the caller's, and what it threw + # away was the caller's too. + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + del rows, txn + gc.collect() + assert [warning.category for warning in caught] == [ResourceWarning] + + if before is not None: + assert descriptors() == before + + # And the database is a database, opened again by somebody else. + with zudb.connect(tmp_path / "open.zu1") as conn: + assert conn.execute(READ).fetchall() == [(1,)] + + def test_a_statement_that_failed_wrote_nothing_and_left_the_connection_alone( social: zudb.Connection, ) -> None: @@ -363,6 +510,14 @@ def test_the_programs_that_look_like_misuse_and_are_not( # every statement wants. assert len(social.execute(READ, {"unread": 1}).fetchall()) == 3 + # A value nested deeply is not a value nested endlessly. The limit + # that catches a cycle sits where nothing anybody wrote reaches, so + # a list forty deep goes through and comes back. + deep: object = 1 + for _ in range(40): + deep = [deep] + assert social.execute("RETURN $x AS x", {"x": deep}).fetchall()[0][0] is not None + # A label nothing carries matches nothing. A pattern with no answer # is the ordinary answer to a question about a graph, and the other # reading gives a query that fails on the day the last row of a From 36347f7840c0c51eb6efb7cd882935a29e756eb2 Mon Sep 17 00:00:00 2001 From: Tam Nguyen Duc <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 07:56:27 +0700 Subject: [PATCH 2/9] the two jobs that watch the memory, and the marker that lets them A sanitizer job and a leak job over the same suite, because they see different things. ASan instruments the source it compiles and catches what this extension does to its own memory; valgrind instruments the instructions and catches both sides of the boundary, every prebuilt thing either of them links, and a read of memory nobody wrote, which ASan does not look for at all. Both deselect the timing marker, which the tests that assert on a wall clock now carry. A ratio between two things a sanitizer slowed down by different factors is not a measurement of anything. The leak job installs pandas rather than everything, because polars starts a thread pool on import and holds thread-local state for the life of the process, and none of it belongs here. tools/valgrind.supp names the interpreter as the allocator rather than listing symptoms, so it cannot grow a line every bad week, and it says out loud what that leaves out. --- .github/workflows/ci.yml | 99 ++++++++++++++++++++++++++++++++++++++++ pyproject.toml | 8 ++++ tests/test_aio.py | 2 + tests/test_appender.py | 1 + tests/test_interrupt.py | 4 ++ tests/test_register.py | 1 + tools/valgrind.supp | 49 ++++++++++++++++++++ 7 files changed, 164 insertions(+) create mode 100644 tools/valgrind.supp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3e96859..86f982d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,6 +62,105 @@ jobs: - run: pip install pytest griffe ipython - run: pytest + # The same suite again, over an extension built with AddressSanitizer. + # + # There is no `unsafe` in this crate, which is the reason to run this + # rather than the reason not to: what a binding gets wrong is not + # arithmetic on a raw pointer but a buffer read after the object that + # owned it was collected, or an engine allocation freed on one side of + # the boundary and touched from the other. Neither is an `unsafe` + # block here and both are a use-after-free. + # + # ASan's runtime has to be loaded before anything it instruments, and + # the extension is opened by `import` long after python has started, + # so it is preloaded rather than linked: `-Zexternal-clangrt` tells + # rustc not to bundle its own copy and LD_PRELOAD supplies clang's. + # `--target` is what keeps the flags off the build scripts, which are + # host programs with no runtime preloaded and would not link. + # Leak detection is off, because LSan reports the interpreter's own + # hundred one-time allocations and cannot be told to stop; the job + # below is where leaks are counted, with a tool that can. + sanitizer: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-python@v6 + with: + python-version: "3.14" + - uses: Swatinem/rust-cache@v2 + # The instrumented build is nightly, because `-Zsanitizer` is, and + # it is the only thing here that is: what ships is built by the + # pinned compiler in every other job. + - run: rustup toolchain install nightly --profile minimal + - run: sudo apt-get update && sudo apt-get install -y libclang-rt-18-dev + - run: pip install maturin pytest ipython + - name: The extension, instrumented + env: + RUSTUP_TOOLCHAIN: nightly + RUSTFLAGS: -Zsanitizer=address -Zexternal-clangrt + CC: clang + CXX: clang++ + run: | + maturin build --target x86_64-unknown-linux-gnu --out dist + pip install --force-reinstall --no-deps dist/*.whl + - name: The suite, watched + run: | + runtime=$(clang -print-file-name=libclang_rt.asan-$(uname -m).so) + test -f "$runtime" + LD_PRELOAD="$runtime" ASAN_OPTIONS=detect_leaks=0:abort_on_error=1 \ + pytest -m "not timing" + + # The third tool over the same suite, and the one that watches what + # the sanitizer cannot. ASan instruments the source it compiles, so it + # sees nothing the interpreter does with the memory it hands the + # extension; Valgrind instruments the instructions that run, so both + # sides of the boundary are watched and so is every prebuilt thing + # either of them links. It also reports a read of memory nobody wrote, + # which ASan does not look for at all. + # + # PYTHONMALLOC=malloc is what makes this readable. CPython pools small + # objects behind its own allocator by default, so every allocation + # this extension makes through the interpreter would arrive as one + # 256KB arena and nothing inside it could be attributed to anybody. + # + # tools/valgrind.supp is one rule and says what it leaves out. The + # gate is validated the only way a gate can be: a deliberate leak + # through ctypes fails it. + leaks: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-python@v6 + with: + python-version: "3.14" + - uses: Swatinem/rust-cache@v2 + - run: sudo apt-get update && sudo apt-get install -y valgrind + # An ordinary build. Valgrind wants the instructions the extension + # actually ships, and an instrumented one would be a different + # program with a different allocator underneath it. + # Every optional dependency except polars, which starts a thread + # pool the moment it is imported and holds a block of thread-local + # state per worker for the life of the process. Valgrind reports + # all of it and none of it belongs to this client, and the one + # test that wants polars skips itself when it is missing. Pandas + # and pyarrow stay because the README examples this suite runs go + # through them. + - run: pip install ".[pandas]" pytest ipython + - name: A leak the job is meant to catch, caught + run: | + set +e + PYTHONMALLOC=malloc valgrind --error-exitcode=1 --leak-check=full \ + --errors-for-leak-kinds=definite,possible \ + --suppressions=tools/valgrind.supp -q \ + python -c 'import ctypes; ctypes.CDLL("libc.so.6").malloc(4096)' + test $? -eq 1 || { echo "the leak gate did not fire on a leak"; exit 1; } + - name: The suite, counted + run: | + PYTHONMALLOC=malloc valgrind --error-exitcode=1 --leak-check=full \ + --errors-for-leak-kinds=definite,possible \ + --suppressions=tools/valgrind.supp -q \ + python -m pytest -m "not timing" + # The shared corpus, which is the same 945 cases the engine runs # against itself and the eight other clients run against theirs. It is # a job of its own because it needs a second checkout, and it runs on diff --git a/pyproject.toml b/pyproject.toml index b561cc7..84c3deb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -74,6 +74,14 @@ addopts = "-q" # holds `conformance`, which is a development tool rather than something # the wheel ships. pythonpath = ["tools", "."] +# Tests that assert on a wall clock: that two connections overlap, that +# an interrupt is felt inside a budget, that appending beats inserting +# by a margin. They are the tests that say this client is fast, and +# they are meaningless under a sanitizer, which makes every instruction +# slower by a factor nobody controls and the two sides of a ratio +# slower by different ones. The jobs that run under one deselect them +# by this marker and the ordinary jobs run them all. +markers = ["timing: asserts on elapsed time, so a sanitizer run has to skip it"] [tool.ruff] target-version = "py311" diff --git a/tests/test_aio.py b/tests/test_aio.py index aab5395..3711565 100644 --- a/tests/test_aio.py +++ b/tests/test_aio.py @@ -125,6 +125,7 @@ async def test_the_loop_runs_while_a_statement_does(tmp_path: Path) -> None: @run +@pytest.mark.timing async def test_two_connections_run_at_the_same_time(tmp_path: Path) -> None: """A thread each, and the engine puts the GIL down for the work, so two statements together cost about what one costs rather than two. @@ -174,6 +175,7 @@ async def test_cancelling_the_task_stops_the_statement(tmp_path: Path) -> None: @run +@pytest.mark.timing async def test_a_cancelled_statement_leaves_the_connection_free(tmp_path: Path) -> None: """The reason the cancellation waits for the statement it stopped. diff --git a/tests/test_appender.py b/tests/test_appender.py index 6f3ed24..87b5176 100644 --- a/tests/test_appender.py +++ b/tests/test_appender.py @@ -432,6 +432,7 @@ def run() -> None: COMPARED = 200 +@pytest.mark.timing def test_appending_beats_inserting_by_the_margin_that_makes_it_worth_having( tmp_path: Path, ) -> None: diff --git a/tests/test_interrupt.py b/tests/test_interrupt.py index 5c4cce8..f671324 100644 --- a/tests/test_interrupt.py +++ b/tests/test_interrupt.py @@ -48,6 +48,7 @@ def wait() -> None: return thread +@pytest.mark.timing def test_a_press_raises_keyboard_interrupt_within_the_budget(throng: zudb.Connection) -> None: pressed: list[float] = [] @@ -76,6 +77,7 @@ def test_the_connection_is_the_same_afterwards(throng: zudb.Connection) -> None: assert throng.execute("MATCH (p:person) RETURN count(p) AS n").fetchone() == (12_000,) +@pytest.mark.timing def test_interrupt_stops_a_statement_from_another_thread(throng: zudb.Connection) -> None: asked: list[float] = [] @@ -137,6 +139,7 @@ def test_interrupt_on_a_closed_connection_says_so(social: zudb.Connection) -> No social.interrupt() +@pytest.mark.timing def test_a_connection_answers_while_it_is_busy(throng: zudb.Connection) -> None: """Asking a connection how it is going does not queue behind the statement it is going through.""" @@ -165,6 +168,7 @@ def test_rows_read_holds_what_the_last_statement_cost(social: zudb.Connection) - assert social.rows_read >= 3 +@pytest.mark.timing def test_a_statement_that_finishes_is_not_slowed_by_being_watched(social: zudb.Connection) -> None: """The thread a statement runs on is kept rather than made, so a small statement on the main thread costs about what it costs on any diff --git a/tests/test_register.py b/tests/test_register.py index 06ecc2c..fc4ce5e 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -319,6 +319,7 @@ def test_a_closed_connection_registers_nothing(empty: zudb.Connection) -> None: empty.register("people", {"a": [1]}) +@pytest.mark.timing def test_registering_costs_the_same_whatever_the_frame_holds(empty: zudb.Connection) -> None: """Nothing is copied, so nothing about the call is per row. diff --git a/tools/valgrind.supp b/tools/valgrind.supp new file mode 100644 index 0000000..1ae6886 --- /dev/null +++ b/tools/valgrind.supp @@ -0,0 +1,49 @@ +# What the leak job does not count, and why. +# +# CPython leaks on purpose. Interned strings, the import machinery's +# tables and a few hundred one-time allocations are never freed, because +# an interpreter that is about to exit has nothing to gain by walking +# them, and the run below reports about eighty of them on a suite that +# does nothing at all. None of that is this client's memory and none of +# it is a bug anybody here can fix. +# +# So there is one rule here, written out once per allocator because +# valgrind matches a function name and C has three of them. It is narrow +# on purpose. A leak is ignored only when the frame that allocated it is +# inside the interpreter binary itself. Memory allocated by this +# extension has _zudb in that frame instead and is reported, which is +# what the job is for. A suppression file of the usual shape, a list +# that grows a line +# every time somebody wants the build green, would suppress the thing +# being looked for on the first bad week; this one cannot, because it +# names the allocator rather than the symptom. +# +# The gap it leaves, said out loud: a Python object this extension leaks +# was allocated by the interpreter on its behalf, so it lands under this +# rule. That leak is the one the suite catches from the other side, +# where it counts live zudb.Connection objects after a collection and +# counts the process descriptors before and after a thousand cycles. + +{ + the interpreter allocating for itself, malloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:malloc + obj:*/python3* +} + +{ + the interpreter allocating for itself, realloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:realloc + obj:*/python3* +} + +{ + the interpreter allocating for itself, calloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:calloc + obj:*/python3* +} From 96bce8e2ceb75d6e9db0b77452c59821d7f7f2b2 Mon Sep 17 00:00:00 2001 From: Tam Nguyen Duc <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 08:18:43 +0700 Subject: [PATCH 3/9] what the public surface was, against what it is Griffe reads the package the way a reader does, out of the .py files and the .pyi that declares the compiled half, and it names what moved, changed shape or went away. Nothing is built in this job on purpose: the declared API is what the stub says, so the stub is what gets compared, and a stub that has drifted from the extension is what the typing tests in the suite are for. A break is allowed and has to be paid for with a version bump, which is the rule cargo semver-checks applies to the engine and the rule api-extractor's committed report applies to the TypeScript client. The change is fine; the change happening quietly is not. The gate is validated the way the leak gate beside it is: renaming a class the DB-API layer inherits from has to fail it, and the tree goes back before the real check reads it. --- .github/workflows/ci.yml | 58 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 86f982d..dbe2330 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,6 +38,64 @@ jobs: env: PYO3_NO_PYTHON: 1 + # What the public surface was, against what it is. Griffe reads the + # package the way a reader does, out of the .py files and the .pyi + # that declares the compiled half, and it names what moved, changed + # shape or went away. Nothing is built here on purpose: the declared + # API is what the stub says, so the stub is what gets compared, and a + # stub that has drifted from the extension is what the typing tests in + # the suite are for. + # + # A break is allowed. It has to be paid for with a version bump, which + # is the rule cargo semver-checks applies to the engine and the rule + # api-extractor's committed report applies to the TypeScript client: + # the change is fine, the change happening quietly is not. So this + # compares the version too and steps aside once it has moved, after + # printing what moved with it. + api: + runs-on: ubuntu-latest + steps: + # Griffe builds a worktree at the baseline, so it wants the ref + # and not just the one commit under test. + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + - uses: actions/setup-python@v6 + with: + python-version: "3.14" + - run: pip install griffe==2.2.0 + # The gate is validated the only way a gate can be: a break has to + # fail it. Renaming a class the DB-API layer inherits from is the + # cheapest break that is unambiguously one, and the tree is put + # back before anything else looks at it. + - name: A break the gate is meant to catch, caught + env: + BASE_REF: ${{ github.base_ref }} + run: | + base="${BASE_REF:+origin/$BASE_REF}" + base="${base:-HEAD^}" + sed -i 's/^class DataError(/class DataErrorRenamed(/' python/zudb/errors.py + set +e + griffe check zudb -s python --against "$base" + rc=$? + set -e + git checkout -- python/zudb/errors.py + test $rc -ne 0 || { echo "the api gate did not fire on a removed name"; exit 1; } + - name: The surface, against what it was + env: + BASE_REF: ${{ github.base_ref }} + run: | + base="${BASE_REF:+origin/$BASE_REF}" + base="${base:-HEAD^}" + was=$(git show "$base:pyproject.toml" | sed -n 's/^version = "\(.*\)"/\1/p' | head -1) + now=$(sed -n 's/^version = "\(.*\)"/\1/p' pyproject.toml | head -1) + if [ "$was" != "$now" ]; then + echo "version moved from $was to $now, so a break here is one that was declared" + griffe check zudb -s python --against "$base" || true + exit 0 + fi + griffe check zudb -s python --against "$base" + test: strategy: fail-fast: false From 9ba7596e47830ae580770012ebaf53e1d82f4562 Mon Sep 17 00:00:00 2001 From: Tam Nguyen Duc <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 08:21:39 +0700 Subject: [PATCH 4/9] count definite leaks only, and give the sanitizer its pandas Two things the first run of these jobs found. The sanitizer job had no pandas, so the two whole programs the README publishes that call to_pandas failed inside it and the job was watching a smaller suite than it claimed to. They are installed by name rather than through the extra that names them, because the extra would build this extension from source to read its metadata and this job builds its own a step later. The leak job counted possible losses as well as definite ones, and over the whole suite that is 420 possible and zero definite, of which 417 are pyarrow registering compute kernels into a static table it never tears down. A possible loss is a block whose only surviving pointer is into its middle, which is what a registry of C++ objects looks like from outside and what almost nothing this extension allocates looks like. The alternative was a suppression naming pyarrow, and naming a dependency is how a suppression file starts growing. This is the same pair of flags the TypeScript client's job carries. --- .github/workflows/ci.yml | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dbe2330..710861d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -151,7 +151,14 @@ jobs: # pinned compiler in every other job. - run: rustup toolchain install nightly --profile minimal - run: sudo apt-get update && sudo apt-get install -y libclang-rt-18-dev - - run: pip install maturin pytest ipython + # pandas and pyarrow by name rather than through the extra that + # names them, because installing the extra would build this + # extension from source to get at its metadata and this job builds + # its own a step later. They are here at all because two of the + # whole programs the README publishes call to_pandas, and a job + # that skipped them would be watching a smaller suite than the + # one it claims to watch. + - run: pip install maturin pytest ipython pandas pyarrow - name: The extension, instrumented env: RUSTUP_TOOLCHAIN: nightly @@ -184,6 +191,20 @@ jobs: # tools/valgrind.supp is one rule and says what it leaves out. The # gate is validated the only way a gate can be: a deliberate leak # through ctypes fails it. + # + # Definite leaks and not possible ones, which is the same pair of + # flags the TypeScript client's job carries and for a sharper reason + # here. Over the whole suite this counts 420 possible losses and zero + # definite ones, and 417 of the 420 are pyarrow registering compute + # kernels into a static table it never tears down. A possible loss is + # a block whose only surviving pointer is into its middle, which is + # what a registry of C++ objects looks like from the outside and what + # almost nothing this extension allocates looks like. Counting them + # would mean either a red job or a suppression naming pyarrow, and + # naming a dependency in a suppression file is how a suppression file + # starts growing. What it costs is a Rust leak that happens to leave + # an interior pointer behind, and the suite counts live connections + # and process descriptors from the other side for that. leaks: runs-on: ubuntu-latest steps: @@ -208,14 +229,14 @@ jobs: run: | set +e PYTHONMALLOC=malloc valgrind --error-exitcode=1 --leak-check=full \ - --errors-for-leak-kinds=definite,possible \ + --show-leak-kinds=definite --errors-for-leak-kinds=definite \ --suppressions=tools/valgrind.supp -q \ python -c 'import ctypes; ctypes.CDLL("libc.so.6").malloc(4096)' test $? -eq 1 || { echo "the leak gate did not fire on a leak"; exit 1; } - name: The suite, counted run: | PYTHONMALLOC=malloc valgrind --error-exitcode=1 --leak-check=full \ - --errors-for-leak-kinds=definite,possible \ + --show-leak-kinds=definite --errors-for-leak-kinds=definite \ --suppressions=tools/valgrind.supp -q \ python -m pytest -m "not timing" From 7b17f45d92ef1d9294ae78591f19ccbe708479ec Mon Sep 17 00:00:00 2001 From: tamnd <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 08:53:12 +0700 Subject: [PATCH 5/9] Count leaks under valgrind, not the speed of valgrind The leak job was red on two counts, and neither of them was a leak. Three tests assert wall clock. An empty transaction under 50 us, ten thousand turns of the main thread while a result becomes Arrow, twenty turns of the event loop while a statement runs. Under valgrind every instruction is interpreted and the machine is between twenty and fifty times slower, so all three fail and say so in microseconds, which reads like a regression and is not one. They carry the timing marker now, which is what that marker is for and what the job's own -m "not timing" was already asking for. The other count is the suppression file, which was letting through twenty two records that belong to nobody here. Seventeen are readline building its keymaps and terminfo strings at import and never freeing them, which pytest pays for because pytest imports readline. One is glibc keeping the old loader scope array after a dlopen grows it, which it does on purpose so a thread walking the list does not have it freed underneath. One is pyarrow caching a tzinfo. The rest are the interpreter allocating for itself, and those should already have been covered: the rule said obj:*/python3*, and the interpreter on the runner is a shared build whose code lives in libpython3.14.so, which that pattern does not name. Fixing that turned up the more interesting half. A * in a valgrind object pattern matches slashes, so */python3* also matched every extension under lib/python3.14/site-packages, this one included. The rule meant to name the interpreter was covering the code the job exists to watch. It now names the two files an interpreter can be, the shared library and the static binary, and nothing else. The three library rules are narrow and each says what leaks, how much, and why the library is right to do it. The gate step is unchanged and still fires on its deliberate leak, which is what says the file did not grow into a way of making the build green. --- tests/test_aio.py | 1 + tests/test_arrow.py | 1 + tests/test_transactions.py | 1 + tools/valgrind.supp | 110 +++++++++++++++++++++++++++++++------ 4 files changed, 97 insertions(+), 16 deletions(-) diff --git a/tests/test_aio.py b/tests/test_aio.py index 3711565..5b59693 100644 --- a/tests/test_aio.py +++ b/tests/test_aio.py @@ -106,6 +106,7 @@ async def test_what_comes_back_is_read_without_awaiting(tmp_path: Path) -> None: @run +@pytest.mark.timing async def test_the_loop_runs_while_a_statement_does(tmp_path: Path) -> None: """The claim the module is for. diff --git a/tests/test_arrow.py b/tests/test_arrow.py index 48510ad..ac8b496 100644 --- a/tests/test_arrow.py +++ b/tests/test_arrow.py @@ -243,6 +243,7 @@ def test_polars_reads_the_same_rows(loaded: zudb.Connection) -> None: assert frame["name"].to_list() == ["ada", "grace", "kay"] +@pytest.mark.timing def test_python_keeps_running_while_a_result_becomes_arrow(tmp_path: Path) -> None: rows = 400_000 zudb.load( diff --git a/tests/test_transactions.py b/tests/test_transactions.py index b046c1a..444e85c 100644 --- a/tests/test_transactions.py +++ b/tests/test_transactions.py @@ -205,6 +205,7 @@ def test_a_closed_connection_has_no_transactions(social: zudb.Connection) -> Non assert social.in_transaction is None +@pytest.mark.timing def test_the_wrapper_costs_nothing_worth_measuring(social: zudb.Connection) -> None: """Two statements and a Python object, which is what it should be. diff --git a/tools/valgrind.supp b/tools/valgrind.supp index 1ae6886..117f30e 100644 --- a/tools/valgrind.supp +++ b/tools/valgrind.supp @@ -7,43 +7,121 @@ # does nothing at all. None of that is this client's memory and none of # it is a bug anybody here can fix. # -# So there is one rule here, written out once per allocator because -# valgrind matches a function name and C has three of them. It is narrow -# on purpose. A leak is ignored only when the frame that allocated it is -# inside the interpreter binary itself. Memory allocated by this -# extension has _zudb in that frame instead and is reported, which is -# what the job is for. A suppression file of the usual shape, a list -# that grows a line -# every time somebody wants the build green, would suppress the thing -# being looked for on the first bad week; this one cannot, because it -# names the allocator rather than the symptom. +# So the first rule is the only general one, written out once per +# allocator because valgrind matches a function name and C has three of +# them, and once per shape of interpreter because a CPython built +# --enable-shared puts its own code in libpython3.x.so and a CPython +# built without it puts the same code in bin/python3.x. A leak is +# ignored only when the frame that allocated it is inside one of those +# two files. Memory allocated by this extension has _zudb in that frame +# instead and is reported, which is what the job is for. A suppression +# file of the usual shape, a list that grows a line every time somebody +# wants the build green, would suppress the thing being looked for on +# the first bad week; this one cannot, because it names the allocator +# rather than the symptom. +# +# The object patterns are spelled out to the file rather than left as +# */python3*, which is what they used to say. A * in a valgrind object +# pattern matches slashes as well as anything else, so */python3* also +# matched every extension under lib/python3.14/site-packages, this one +# included: a rule meant to name the interpreter was quietly covering +# the code it exists to watch. # # The gap it leaves, said out loud: a Python object this extension leaks # was allocated by the interpreter on its behalf, so it lands under this # rule. That leak is the one the suite catches from the other side, # where it counts live zudb.Connection objects after a collection and # counts the process descriptors before and after a thousand cycles. +# +# The three rules after it are not general. Each one names a library +# that leaks a fixed amount once, at import or at load, and each is here +# because the leak has a name and a reason rather than because it was in +# the way. { - the interpreter allocating for itself, malloc + the interpreter allocating for itself, shared, malloc Memcheck:Leak match-leak-kinds: definite,possible fun:malloc - obj:*/python3* + obj:*/libpython3* } { - the interpreter allocating for itself, realloc + the interpreter allocating for itself, shared, realloc Memcheck:Leak match-leak-kinds: definite,possible fun:realloc - obj:*/python3* + obj:*/libpython3* } { - the interpreter allocating for itself, calloc + the interpreter allocating for itself, shared, calloc Memcheck:Leak match-leak-kinds: definite,possible fun:calloc - obj:*/python3* + obj:*/libpython3* +} + +{ + the interpreter allocating for itself, static, malloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:malloc + obj:*/bin/python3* +} + +{ + the interpreter allocating for itself, static, realloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:realloc + obj:*/bin/python3* +} + +{ + the interpreter allocating for itself, static, calloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:calloc + obj:*/bin/python3* +} + +# readline builds its keymaps, its terminfo strings and its history +# buffer when the module is initialised and frees none of them, because +# a line editor is live for as long as the process is and there is +# nothing after it to hand them back to. Seventeen records, about a +# hundred and fifty kilobytes, every one of them under PyInit_readline. +# pytest imports readline through its own console handling, so a suite +# that never reads a line still pays for it. +{ + readline's tables, built at import and never freed + Memcheck:Leak + match-leak-kinds: definite,possible + ... + fun:PyInit_readline +} + +# glibc grows the loader's namespace scope array on a dlopen and keeps +# the old one, since a thread walking the list must not have it freed +# underneath it. Every extension import goes through this, so the number +# of records is the number of extensions and the fix is in glibc. +{ + the dynamic loader's scope array, kept on purpose + Memcheck:Leak + match-leak-kinds: definite,possible + ... + fun:_dl_open +} + +# pyarrow builds a tzinfo per timezone name it is asked about and caches +# it for the life of the process. This is the definite half of what +# pyarrow already accounts for four hundred and seventeen possible +# records of at import, and it is the reason the job counts definite +# losses rather than all of them. +{ + pyarrow's timezone cache + Memcheck:Leak + match-leak-kinds: definite,possible + ... + obj:*/libarrow_python.so* } From 6bfd832b78c1cc1717cbae774d809e2948eeec24 Mon Sep 17 00:00:00 2001 From: tamnd <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 08:53:46 +0700 Subject: [PATCH 6/9] Press Ctrl-C on Windows without killing the process Both Windows rows have been red for a while, and the log said almost nothing: five lines of dots, then an exit code, no failure line and no summary. That shape is not a test failing. It is the interpreter not being there to write the summary. os.kill(os.getpid(), signal.SIGINT) is what this file used to press the key, and on Windows os.kill is not a signal at all. CPython special cases exactly two values there, CTRL_C_EVENT and CTRL_BREAK_EVENT, and everything else falls through to OpenProcess and TerminateProcess. So the four tests that press the key were terminating the process they were running in, one of them at a time, and pytest died in the middle of the first one. The two console events are no better. A console event goes to every process attached to the console, which on a build machine is the build. What is left is the thing CPython itself calls once a real press has arrived: PyErr_SetInterrupt, spelled _thread.interrupt_main. It trips the same flag the console handler trips, so everything downstream of it is the same code on both platforms, which is all of the code this repository wrote. A tripped SIGINT, a statement deep in the executor, and the next PyErr_CheckSignals turning it into a KeyboardInterrupt on the main thread inside the fifty millisecond budget. The hop that is no longer covered on Windows is the one from the operating system into the C runtime, and nothing running inside this process can cover it without taking the process with it, which is what was happening. The helper says so where a reader will find it. --- tests/test_interrupt.py | 50 ++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 8 deletions(-) diff --git a/tests/test_interrupt.py b/tests/test_interrupt.py index f671324..a8e0c19 100644 --- a/tests/test_interrupt.py +++ b/tests/test_interrupt.py @@ -14,8 +14,10 @@ from __future__ import annotations +import _thread import os import signal +import sys import threading import time @@ -35,6 +37,40 @@ SETTLED = 0.2 +def press() -> None: + """`Ctrl-C`, from inside the process that is going to feel it. + + Everywhere but Windows that is the signal a terminal sends, sent the + way a terminal sends it: to this process, for the main thread to + answer. + + Windows has no way to send one process a SIGINT. `os.kill` there is + `TerminateProcess` for every signal except the two console events, + so `os.kill(os.getpid(), SIGINT)` does not deliver a press, it kills + the process, which is what it had been doing to this suite on every + Windows run: five lines of dots and an exit code, no failure and no + summary, because there was no interpreter left to write one. The two + console events are no better, since a console event goes to every + process attached to the console and on a build machine that is the + build. + + So Windows presses the key the way CPython's own console handler + does when a real press arrives: `PyErr_SetInterrupt`, which + `_thread.interrupt_main` is the spelling of. What that leaves + uncovered is one hop, from the operating system into the C runtime, + and nothing running inside this process can cover that hop without + taking the process with it. Everything after the hop is the same on + both platforms and is all of the code this repository wrote: a + tripped SIGINT, a statement deep in the executor, and the next + `PyErr_CheckSignals` turning one into a `KeyboardInterrupt` on the + main thread inside the budget. + """ + if sys.platform == "win32": + _thread.interrupt_main() + else: + os.kill(os.getpid(), signal.SIGINT) + + def after(delay: float, do) -> threading.Thread: """Runs `do` on a thread of its own, `delay` from now, and answers the thread so a test can join it.""" @@ -52,13 +88,11 @@ def wait() -> None: def test_a_press_raises_keyboard_interrupt_within_the_budget(throng: zudb.Connection) -> None: pressed: list[float] = [] - def press() -> None: + def timed_press() -> None: pressed.append(time.perf_counter()) - # The signal a terminal sends, sent the way a terminal sends it: - # to the process, for the main thread to answer. - os.kill(os.getpid(), signal.SIGINT) + press() - presser = after(SETTLED, press) + presser = after(SETTLED, timed_press) with pytest.raises(KeyboardInterrupt): throng.execute(WORK) felt = time.perf_counter() @@ -67,7 +101,7 @@ def press() -> None: def test_the_connection_is_the_same_afterwards(throng: zudb.Connection) -> None: - presser = after(SETTLED, lambda: os.kill(os.getpid(), signal.SIGINT)) + presser = after(SETTLED, press) with pytest.raises(KeyboardInterrupt): throng.execute(WORK) presser.join(timeout=30) @@ -125,7 +159,7 @@ def test_an_ask_with_nothing_running_does_not_end_the_next_statement( def test_a_press_with_nothing_running_is_pythons_to_deliver(social: zudb.Connection) -> None: with pytest.raises(KeyboardInterrupt): - os.kill(os.getpid(), signal.SIGINT) + press() # Python raises the press at the next thing this thread does, # which is this call, and the statement it stopped is none. for _ in range(1000): @@ -202,7 +236,7 @@ def test_a_press_during_a_statement_that_finishes_first_is_still_raised( """A press is never swallowed. The statement was over before it arrived, so Python raises it at the next thing this thread does.""" with pytest.raises(KeyboardInterrupt): - os.kill(os.getpid(), signal.SIGINT) + press() social.execute("MATCH (p:person) RETURN p.name AS n") for _ in range(1000): pass From 3cc73968bfd6e6cb5a82232ee0420c970ea77832 Mon Sep 17 00:00:00 2001 From: tamnd <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:13:56 +0700 Subject: [PATCH 7/9] Let a Windows path through the magic with its separators on With the suite no longer terminating itself on Windows, the rows came back with one failure on them, and it is a real one that has been there the whole time behind the crash. %gql --read-only C:\data\social.zu1 opens nothing. The magic lexes its line with shlex.split, which is POSIX by default, and POSIX means a backslash is an escape. So that path arrives at connect as C:datasocial.zu1 and the person reads that the system cannot find a file they are looking at in the directory listing in front of them. The line still has to be lexed rather than split on whitespace, because a notebook on any platform can be pointed at a path with a space in it and quoting is how somebody says so. What changes is the escape character, which is now the platform's: none on Windows, backslash everywhere else, so a Unix user goes on writing two\ words and a Windows user goes on writing what every other program on their machine accepts. The comment character goes off for the same reason shlex.split turns it off, which is that # is a legal character in a file name. Four tests. A quoted path with a space in it, which runs everywhere and is the only reason any of this is lexed at all. The Windows separators, which runs on Windows. The Unix escape, which runs everywhere else and is there so this cannot be fixed by taking something away. And a hash in a name. --- python/zudb/magic.py | 46 ++++++++++++++++++++++++++++++++++---------- tests/test_magic.py | 39 +++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 10 deletions(-) diff --git a/python/zudb/magic.py b/python/zudb/magic.py index 2d65074..a75cf9d 100644 --- a/python/zudb/magic.py +++ b/python/zudb/magic.py @@ -39,6 +39,7 @@ from __future__ import annotations +import os import shlex from pathlib import Path from typing import Any @@ -51,6 +52,31 @@ __all__ = ["ZuMagics", "load_ipython_extension"] +def words(line: str) -> list[str]: + r"""A magic's line, split the way the shell of this machine splits. + + `shlex.split` is the obvious thing to call and it is wrong on + Windows, where a backslash is a path separator rather than an + escape. `%gql C:\data\social.zu1` comes back out of it as + `C:datasocial.zu1`, and the message a person then reads is that + the system cannot find a file they can see in the directory + listing in front of them. + + So the escape character is the platform's. Quoting is not: a path + with a space in it is quoted the same way everywhere, because that + is what a person typing into a notebook cell will have done. The + comment character is turned off for the same reason `shlex.split` + turns it off, which is that `#` is a legal character in a file name + and a line magic is not a script. + """ + lexer = shlex.shlex(line, posix=True) + lexer.whitespace_split = True + lexer.commenters = "" + if os.name == "nt": + lexer.escape = "" + return list(lexer) + + @magics_class class ZuMagics(Magics): """The two magics, and the connection `%gql` opened if it opened one.""" @@ -71,11 +97,11 @@ def gql(self, line: str) -> Connection | None: The connection comes back so that a cell can keep it, and it is closed at the end of the session like any other. """ - words = shlex.split(line) - read_only = "--read-only" in words - closing = "--close" in words - paths = [word for word in words if not word.startswith("-")] - unknown = [word for word in words if word.startswith("-")] + given = words(line) + read_only = "--read-only" in given + closing = "--close" in given + paths = [word for word in given if not word.startswith("-")] + unknown = [word for word in given if word.startswith("-")] for word in unknown: if word not in ("--read-only", "--close"): raise UsageError(f"%gql does not take {word}") @@ -120,16 +146,16 @@ def gql_cell(self, line: str, cell: str) -> Any: def options(self, line: str) -> dict[str, str]: """`--conn`, `--params` and `--out`, each naming a variable.""" - words = shlex.split(line) + given = words(line) taken: dict[str, str] = {} - while words: - word = words.pop(0) + while given: + word = given.pop(0) name = word.removeprefix("--") if name == word or name not in ("conn", "params", "out"): raise UsageError(f"%%gql takes --conn, --params and --out, and not {word}") - if not words: + if not given: raise UsageError(f"--{name} names a variable, and this named none") - taken[name] = words.pop(0) + taken[name] = given.pop(0) return taken def namespace(self) -> dict[str, Any]: diff --git a/tests/test_magic.py b/tests/test_magic.py index 671d2d7..80a24c6 100644 --- a/tests/test_magic.py +++ b/tests/test_magic.py @@ -14,6 +14,7 @@ from __future__ import annotations +import os from pathlib import Path from typing import Any @@ -24,6 +25,7 @@ from IPython.core.error import UsageError # noqa: E402 from IPython.core.interactiveshell import InteractiveShell # noqa: E402 +from zudb import magic # noqa: E402 @pytest.fixture @@ -196,3 +198,40 @@ def test_a_statement_that_is_wrong_raises_what_it_would_have(shell: Any, tmp_pat shell.run_line_magic("gql", str(tmp_path / "wrong.zu1")) with pytest.raises(zudb.SyntaxError): gql(shell, "MATCH (p:person RETURN p") + + +def test_a_path_with_a_space_in_it_is_quoted_and_opens(shell: Any, tmp_path: Path) -> None: + """Which is the reason the line is lexed rather than split on + whitespace, and the only reason.""" + room = tmp_path / "two words" + room.mkdir() + path = room / "spaced.zu1" + conn = shell.run_line_magic("gql", f'"{path}"') + assert conn.closed is False + assert conn.execute("RETURN 1 AS n").fetchone() == (1,) + + +@pytest.mark.skipif(os.name != "nt", reason="a backslash is only a separator on Windows") +def test_a_windows_path_keeps_its_separators() -> None: + r"""The failure this is here for: `shlex.split` treats a backslash + as an escape, so `C:\data\social.zu1` came out of it as + `C:datasocial.zu1` and the magic then said the system could not + find a file the person could see in front of them.""" + assert magic.words(r"--read-only C:\data\social.zu1") == [ + "--read-only", + r"C:\data\social.zu1", + ] + + +@pytest.mark.skipif(os.name == "nt", reason="a backslash is an escape everywhere else") +def test_a_backslash_is_still_an_escape_where_a_shell_says_it_is() -> None: + """Nothing is taken away from the platforms that were right: a + person on a Unix quotes a space with a backslash and expects that + to go on working.""" + assert magic.words(r"/tmp/two\ words/spaced.zu1") == ["/tmp/two words/spaced.zu1"] + + +def test_a_hash_in_a_name_is_a_name_and_not_a_comment() -> None: + """A line magic is not a script, and `#` is a legal character in a + file name on every platform this runs on.""" + assert magic.words("a#b.zu1 --read-only") == ["a#b.zu1", "--read-only"] From be002bab6427b1625523f58cd8fad44f44807ea9 Mon Sep 17 00:00:00 2001 From: Tam Nguyen Duc <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 10:12:14 +0700 Subject: [PATCH 8/9] Leave the symbols on the build the leak job reads Fixing the object patterns uncovered what they had been hiding: one definite loss of eighty bytes inside this extension, on every run, allocated while the module was still being executed. It is pyo3's. A class with members gets its tp_members as a Vec whose pointer is handed to CPython, and CPython holds that array for as long as the type exists, so pyo3 leaks it rather than free memory the interpreter is still reading. One class here has members and the rest have getters, which is why it is one record and not eight. It is in this object file only because pyo3 is compiled into it. Naming that in the suppression file takes a symbol, and the wheel a release ships is stripped, which is why the report from CI read ??? on every frame. So the leaks job now builds with the symbol table left on. The instructions are the same optimised ones, because a build valgrind reads has to be the build that runs; what changes is that a report from this job says where, which is the difference between a leak checker and a number. --- .github/workflows/ci.yml | 14 +++++++++++--- tools/valgrind.supp | 27 +++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 710861d..5da48c7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -214,9 +214,14 @@ jobs: python-version: "3.14" - uses: Swatinem/rust-cache@v2 - run: sudo apt-get update && sudo apt-get install -y valgrind - # An ordinary build. Valgrind wants the instructions the extension - # actually ships, and an instrumented one would be a different - # program with a different allocator underneath it. + # The release build, with its symbols left on. Valgrind wants the + # instructions the extension actually ships, so this is the same + # optimised build and not an instrumented one, which would be a + # different program with a different allocator underneath it. What + # changes is only that the symbol table survives: a leak report + # whose every frame reads ??? says a number and nothing a person + # can act on, and the suppression file cannot name a function it + # has no name for. # Every optional dependency except polars, which starts a thread # pool the moment it is imported and holds a block of thread-local # state per worker for the life of the process. Valgrind reports @@ -225,6 +230,9 @@ jobs: # and pyarrow stay because the README examples this suite runs go # through them. - run: pip install ".[pandas]" pytest ipython + env: + CARGO_PROFILE_RELEASE_STRIP: "false" + CARGO_PROFILE_RELEASE_DEBUG: "1" - name: A leak the job is meant to catch, caught run: | set +e diff --git a/tools/valgrind.supp b/tools/valgrind.supp index 117f30e..18a68a8 100644 --- a/tools/valgrind.supp +++ b/tools/valgrind.supp @@ -33,10 +33,17 @@ # where it counts live zudb.Connection objects after a collection and # counts the process descriptors before and after a thousand cycles. # -# The three rules after it are not general. Each one names a library -# that leaks a fixed amount once, at import or at load, and each is here +# The four rules after it are not general. Each one names a library that +# leaks a fixed amount once, at import or at load, and each is here # because the leak has a name and a reason rather than because it was in # the way. +# +# Naming a function at all takes symbols, and the wheel a release ships +# is stripped. The leaks job builds one that is not, which is the only +# reason the rules below can say what they mean: a leak report whose +# every frame reads ??? is a report nobody can act on, and a job that +# produces one is a job that gets ignored on the week it finds +# something. { the interpreter allocating for itself, shared, malloc @@ -101,6 +108,22 @@ fun:PyInit_readline } +# pyo3 builds each class's member table as a Vec and hands CPython the +# pointer, which is what tp_members is: an array the type holds for as +# long as the type exists, and a type created at import exists until the +# process ends. The Vec is leaked on purpose because freeing it would +# leave the interpreter reading memory that had been handed back. One +# record of eighty bytes, from the one class that has members rather +# than getters, and it is in this extension's object file only because +# pyo3 is compiled into it. +{ + pyo3 handing a type its member table + Memcheck:Leak + match-leak-kinds: definite,possible + ... + fun:*create_type_object* +} + # glibc grows the loader's namespace scope array on a dlopen and keeps # the old one, since a thread walking the list must not have it freed # underneath it. Every extension import goes through this, so the number From bc630a02e466a79d93dcba0ee63a29dd05cc7729 Mon Sep 17 00:00:00 2001 From: Tam Nguyen Duc <1218621+tamnd@users.noreply.github.com> Date: Wed, 19 Aug 2026 11:21:41 +0700 Subject: [PATCH 9/9] Strip the wheel the leak job reads with the tool that strips it The job asked cargo to keep the symbols and got a stripped extension anyway, because two things strip this wheel and only one of them is cargo. `strip = true` in pyproject.toml is maturin's, applied to the artifact after the build rather than through the profile, so a profile that says to keep the symbols is a profile maturin then undoes. The first full run said so: 555 passed in thirty minutes, then one definite loss of eighty bytes with ??? on every frame. That record is pyo3 handing a type its member table, which the suppression file has named since it was written and could not match, because a rule that names a function cannot match a frame that has no name. MATURIN_STRIP is the setting that reaches the tool doing the stripping. The cargo pair stays: it is what puts the debug info there in the first place, and maturin only decides whether it survives. And a step that says so before valgrind runs. The failure this had was an hour of work followed by a report nobody could act on, and the check that would have caught it in a second is a look at the file for the section valgrind reads a frame's name out of. --- .github/workflows/ci.yml | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5da48c7..9612575 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -222,6 +222,17 @@ jobs: # whose every frame reads ??? says a number and nothing a person # can act on, and the suppression file cannot name a function it # has no name for. + # + # Three settings and not one, because two things strip this wheel + # and only one of them is cargo. `strip = true` in pyproject.toml + # is maturin's own, applied after the build and to the artifact + # rather than through the profile, so a cargo profile that says to + # keep the symbols is a cargo profile maturin then throws away. + # That is what happened the first time this job ran the suite: one + # eighty byte record out of pyo3's type constructor, named in the + # suppression file, reported with ??? on every frame and matched + # by nothing. + # # Every optional dependency except polars, which starts a thread # pool the moment it is imported and holds a block of thread-local # state per worker for the life of the process. Valgrind reports @@ -231,8 +242,20 @@ jobs: # through them. - run: pip install ".[pandas]" pytest ipython env: + MATURIN_STRIP: "false" CARGO_PROFILE_RELEASE_STRIP: "false" CARGO_PROFILE_RELEASE_DEBUG: "1" + # Said here rather than trusted, because the whole job rests on it + # and the way it fails is an hour of valgrind followed by a report + # nobody can read. The section is what valgrind reads a frame's + # name out of, and a stripped object has neither it nor a symbol + # table to fall back on. + - name: The symbols the report is read with + run: | + set -eu + so=$(python -c 'import zudb, pathlib; print(pathlib.Path(zudb.__file__).parent / "_zudb.abi3.so")') + readelf -S "$so" | grep -q debug_info \ + || { echo "$so carries no debug info, so every frame of the report would read ???"; exit 1; } - name: A leak the job is meant to catch, caught run: | set +e