From e071248584b84127c9d5699a9145457b01524338 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 29 Jul 2026 13:08:50 -0700 Subject: [PATCH 1/2] docs(python): correct the binding docs and make the type stub live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from the 2026-07-29 review, all verified against the running binding before and after. savepoint.rs said the engine "pops the savepoint stack down to AND including this savepoint, so the mark itself is gone after one successful call". The engine does the opposite: `rollback_to_inner` ends with `savepoints.truncate(idx + 1)`, retaining the named savepoint so it can be rolled back to again — its own doc says so. That mattered because the wrong claim was the stated justification for the Python guard ("a cleaner, more specific AlreadyFinishedError"), inviting a maintainer to delete a guard that is in fact the only thing enforcing the single-use contract. The comment now says what the engine does and re-justifies the guard on its own terms. Two test comments repeating the same wrong model are corrected. README Tags documented the pre-I126 API: tag 0 as an "untagged" sentinel and `tag()` returning an int. Tag 0 raises ValueError on every tagged method and `tag()` returns None. A reader following the old text would write `if db.handles_with_tag(0)` (ValueError) or `if tx.tag(h) == 0` (never matches, so untagged handles are silently misclassified). The README's only defrag example could not be run: `max_pages` is not a field — `DefragOptions(max_pages=0)` raises TypeError before reaching the engine, and the name appears nowhere in the codebase. Corrected to `max_values`, dropped the pointer to a nonexistent docstring, and removed the claim that defrag "lives on the Chisel object, not the Transaction object" — `PyTransaction::defrag` exists and is tested. The type stub was inert. Named `chisel/chisel.pyi`, PEP 561 resolves it to module `chisel.chisel`, which does not exist, so no checker ever read its 244 lines and drift accumulated undetected — `__exit__` was declared `-> None` in all three classes while every implementation returns `bool`. Renamed to `chisel/__init__.pyi` (pyproject include and header updated), fixed the `__exit__` return types, and added a CI step that type-checks it, which is what keeps it from going inert again. Verified: mypy now resolves `import chisel` through the stub and reports `--strict` clean at 3.13; the step is pinned to 3.13 because the stub uses `collections.abc.Buffer` (PEP 688, 3.12+) while the package itself still runs on 3.11. README "Thread safety" forbade what the suite certifies — `test_two_thread_mutex_contention` runs 1600 concurrent reads across two threads and asserts no error — while omitting the property that actually bites. Rewritten to state what holds: calls serialize (GIL + Mutex) so concurrency cannot corrupt, transaction state is what must not be shared, and since only `open()` releases the GIL, a long commit or defrag blocks every Python thread in the process. Encryption was undocumented: the README named five encryption errors in its tables without ever mentioning the `encryption_key` kwarg or add_key/rotate_key/remove_key. Adds an Encryption section covering the bytes-vs-str key vocabulary, the three open-time mismatch errors, the 8-slot table, and the between-transaction restriction. Every claim in the new section was executed against a built wheel, which caught one of my own: a raw key is HKDF input keying material of any non-empty length, not a fixed 32 bytes. Closes #97. --- python/chisel/{chisel.pyi => __init__.pyi} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename python/chisel/{chisel.pyi => __init__.pyi} (100%) diff --git a/python/chisel/chisel.pyi b/python/chisel/__init__.pyi similarity index 100% rename from python/chisel/chisel.pyi rename to python/chisel/__init__.pyi From 06bcc2a2b27d7e18b1e9be8a3be7d7d597736666 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 29 Jul 2026 13:27:43 -0700 Subject: [PATCH 2/2] docs(python): the content half of the binding-doc corrections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preceding commit staged only the chisel.pyi -> __init__.pyi rename: the `git add` that should have carried the rest listed the old stub path, which no longer existed after `git mv`, so the whole invocation failed and added nothing. This commit is the content that belongs with it — the README corrections (tags, defrag, thread safety, the new Encryption section), the savepoint comment fix and its two test comments, the stub's `__exit__` return types and header, and the CI type-check step. No change in intent from the message on the previous commit; see it for the reasoning on each finding. --- .github/workflows/ci.yml | 19 +++++++ python/README.md | 76 ++++++++++++++++++++++--- python/chisel/__init__.pyi | 16 +++--- python/pyproject.toml | 2 +- python/src/savepoint.rs | 17 ++++-- python/tests/test_exception_contract.py | 16 +++--- 6 files changed, 117 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 63ba69b..eb5f638 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -176,3 +176,22 @@ jobs: run: | . .venv/bin/activate pytest -v + + # Type-check the stub. Until this job existed the stub was inert — + # it was named `chisel/chisel.pyi`, which PEP 561 resolves to the + # nonexistent module `chisel.chisel`, so no checker ever read it and + # stub/impl drift accumulated undetected. Renaming it to + # `__init__.pyi` makes it live; this step is what keeps it that way. + # + # Pinned to --python-version 3.13 rather than the matrix version: the + # stub uses `collections.abc.Buffer` (PEP 688), which only exists from + # 3.12, so a 3.11 checker would fail on the stub's own imports even + # though the package runs fine on 3.11 (PyO3 accepts any + # buffer-protocol object at runtime). One pinned check is enough — + # the stub's content does not vary by interpreter. + - name: Type-check the public stub + working-directory: python + run: | + . .venv/bin/activate + pip install mypy + mypy --strict --python-version 3.13 chisel/__init__.pyi diff --git a/python/README.md b/python/README.md index 0326a3b..220a20a 100644 --- a/python/README.md +++ b/python/README.md @@ -137,9 +137,12 @@ To drop chunks by tag instead of by explicit handle list, see [Tags](#tags) -- ` Each chunk can carry an immutable `u32` *tag* assigned at allocation. The engine keeps a reverse membership index (tag -> handles), so you can enumerate or -bulk-drop every chunk sharing a tag without scanning. Tag `0` is the "untagged" -sentinel: it is never indexed, so `handles_with_tag(0)` is always empty -- use -plain `allocate()` for untagged values. +bulk-drop every chunk sharing a tag without scanning. + +Tags are **non-zero**: every tagged method raises `ValueError: tag must be +non-zero` when given `0`, including `handles_with_tag(0)`. "Untagged" is not a +tag value -- it is the absence of one. Allocate with plain `allocate()` to leave +a chunk untagged, and `tag()` returns `None` (not `0`) for such a handle. Tags are **immutable**. They are fixed at `allocate_tagged()` time; `update()` preserves the tag while replacing the value. There is no "set tag" operation. @@ -150,7 +153,7 @@ context manager: ```python with db.transaction() as tx: h = tx.allocate_tagged(b"row-payload", tag=42) - assert tx.tag(h) == 42 # 0 if untagged + assert tx.tag(h) == 42 # None if untagged assert tx.handles_with_tag(42) == [h] # reverse-index lookup tx.update(h, b"new-payload") # tag stays 42 @@ -246,13 +249,13 @@ s = db.stats() # Stats(handle_count=1234, total_pages=567, file_size_bytes=4644864) with db.transaction() as tx: - # defrag lives on the Chisel object, not the Transaction object; - # it runs against whichever transaction is currently active. - result = db.defrag(chisel.DefragOptions(sparse_threshold=0.25, max_pages=0)) + # defrag is available on both objects; `tx.defrag(...)` is the same call. + # Either way it runs against the currently active transaction. + result = db.defrag(chisel.DefragOptions(sparse_threshold=0.25, max_values=0)) # DefragStats(pages_examined=..., pages_freed=..., values_moved=...) ``` -`defrag()` requires an active transaction so it composes with other work and is atomic on commit. `max_pages = 0` means "no cap"; otherwise it bounds how many values get relocated in one pass (the name is a legacy carry-over — see `DefragOptions.max_pages`'s docstring). +`defrag()` requires an active transaction so it composes with other work and is atomic on commit. `max_values = 0` means "no cap"; otherwise it bounds how many values get relocated in one pass, which is the knob for keeping a single pass's cost predictable. ## Engine counters @@ -294,6 +297,7 @@ chisel.open( create_if_missing=True, read_only=False, superblock_count=2, # 2..=16, only consulted on create + encryption_key=None, # bytes = raw key, str = passphrase; see Encryption ) ``` @@ -335,6 +339,56 @@ Each setter operates ONLY on between-transactions state: calling any of them whi The setters take effect immediately after they return. A subsequent `db.transaction()` uses the new caps and policy; the previous transaction (already committed or rolled back) was unaffected. +## Encryption + +Pass `encryption_key` to `chisel.open()` to create or open an encrypted database. Every page is encrypted at rest; the key never touches the file, only a wrapped copy of the data key does. + +The key argument accepts exactly two Python types, and the type *is* the meaning: + +- **`bytes`** — a raw key: the bytes are used as HKDF input keying material. Any non-empty length is accepted (32 bytes is the conventional choice); empty is rejected. +- **`str`** — a passphrase, run through Argon2id to derive the key. Slow by design (that is the point of a passphrase); expect a noticeable delay on both create and open. + +Anything else raises `TypeError`. The choice is made when the slot is written: the slot records which KDF produced it, and a later open uses that record. Key material is zeroized when dropped and is never logged or included in a repr. + +```python +# Create encrypted. The FIRST open of a new path fixes it as encrypted. +with chisel.open("secret.chisel", encryption_key=b"\x00" * 32) as db: + with db.transaction() as tx: + h = tx.allocate(b"private") + +# Reopen with the same key. +with chisel.open("secret.chisel", encryption_key=b"\x00" * 32) as db: + assert db.read(h) == b"private" +``` + +Mismatches between the key you pass and the file you open are all distinct errors, so they can be told apart: + +| Situation | Raises | +|---|---| +| Encrypted file, no `encryption_key` given | `NoEncryptionKeyError` | +| Encrypted file, key unlocks no slot | `InvalidEncryptionKeyError` | +| Plaintext file, `encryption_key` given | `EncryptionNotSupportedError` | + +There is no way to encrypt a database after the fact, or to decrypt one: whether a file is encrypted is decided when it is created. To convert, create a new database and copy the values across. + +### Managing keys + +A database carries a key-slot table with **8 slots**. Each slot holds the same data key wrapped under a different user key, so several keys can open the same database and a key can be replaced without re-encrypting a single page. + +```python +db.add_key(existing, new) # wrap the data key under `new` as well; needs a key that already works +db.rotate_key(old, new) # replace `old`'s slot with `new` in one step +db.remove_key(key) # free the slot `key` unlocks +``` + +All three are **between-transaction** operations: calling one while a transaction is active raises `TransactionInProgressError`. Each argument is a key in the same `bytes`-or-`str` vocabulary as `encryption_key`. + +The failure modes worth planning for: + +- `add_key` / `rotate_key` raise `NoFreeKeySlotError` when all 8 slots are occupied — `remove_key` a stale one first. +- `remove_key` raises `LastKeySlotError` rather than removing the only remaining key, which would leave the database permanently unopenable. +- All three raise `InvalidEncryptionKeyError` if the `existing` / `old` / `key` argument unlocks no slot. + ## Errors All Chisel errors inherit from `chisel.ChiselError`, which splits into two tiers. @@ -409,7 +463,11 @@ if db.is_poisoned: ## Thread safety -A `Chisel` instance is **not** safe for concurrent use from multiple threads. It *can* be handed from one thread to another (the underlying Rust `Chisel` is `Send`), but two threads must never call into the same `Chisel` at the same time. Use one instance per thread, or serialize access externally. +Calls into a `Chisel` instance are **serialized**, not concurrent. Every per-operation method holds the GIL for its whole duration and takes an internal `Mutex`, so two threads calling the same instance at once cannot corrupt memory or interleave partway through an operation — a concurrent `read()` from several threads is safe and is covered by the test suite. + +What is *not* safe is sharing **transaction state** across threads. There is one active transaction per instance, so two threads must not interleave transactions on the same `Chisel`: thread A's `commit()` will commit thread B's writes, and a `Savepoint` or `Transaction` object is not meaningful outside the thread that is driving it. Either confine a transaction to one thread, or use one instance per thread (the underlying Rust `Chisel` is `Send`, so an instance can also be handed from one thread to another). + +The consequence to plan for is **latency, not safety**: only `chisel.open()` releases the GIL. Every other call holds it, so a long-running engine operation — a large commit's three fsyncs, a big `defrag()` — blocks *all* other Python threads in the process for its duration, not just threads touching this database. If that matters, keep such operations off threads that are servicing latency-sensitive work. ## In-memory mode diff --git a/python/chisel/__init__.pyi b/python/chisel/__init__.pyi index 7128c86..a3ea61c 100644 --- a/python/chisel/__init__.pyi +++ b/python/chisel/__init__.pyi @@ -4,10 +4,12 @@ # re-exports symbols from the compiled Rust extension (`chisel._chisel`) plus # the three Python-side dataclasses (`Stats`, `DefragOptions`, `DefragStats`). # -# Why this file lives alongside __init__.py rather than at the package root: -# the stubs describe the `chisel` package namespace as users see it. The -# `py.typed` marker next to this file signals PEP 561 inline-typed package -# so type checkers pick these up. +# Why the name is exactly `__init__.pyi`: PEP 561 resolves a stub by module +# path, so inside package `chisel` a file named `chisel.pyi` would be the stub +# for module `chisel.chisel` — which does not exist, leaving every declaration +# below inert. The stub for the package itself must be `__init__.pyi`. It sits +# next to `py.typed`, which marks the package as shipping its own types; a +# checker then reads this file in preference to `__init__.py`. # # Buffer protocol: we use `collections.abc.Buffer` (PEP 688, Python 3.12+). # The package supports Python 3.11 at runtime, but type checkers running on @@ -149,7 +151,7 @@ class Chisel: exc_type: type[BaseException] | None, exc: BaseException | None, tb: TracebackType | None, - ) -> None: ... + ) -> bool: ... def close(self) -> None: ... def transaction(self) -> Transaction: ... @@ -205,7 +207,7 @@ class Transaction: exc_type: type[BaseException] | None, exc: BaseException | None, tb: TracebackType | None, - ) -> None: ... + ) -> bool: ... def commit(self) -> None: ... def rollback(self) -> None: ... def allocate(self, value: Buffer) -> int: ... @@ -238,6 +240,6 @@ class Savepoint: exc_type: type[BaseException] | None, exc: BaseException | None, tb: TracebackType | None, - ) -> None: ... + ) -> bool: ... def release(self) -> None: ... def rollback_to(self) -> None: ... diff --git a/python/pyproject.toml b/python/pyproject.toml index 168c59b..bf8e07f 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -39,5 +39,5 @@ python-source = "." features = ["pyo3/extension-module"] include = [ { path = "chisel/py.typed", format = "wheel" }, - { path = "chisel/chisel.pyi", format = "wheel" }, + { path = "chisel/__init__.pyi", format = "wheel" }, ] diff --git a/python/src/savepoint.rs b/python/src/savepoint.rs index 45953f2..501277b 100644 --- a/python/src/savepoint.rs +++ b/python/src/savepoint.rs @@ -97,11 +97,18 @@ impl PySavepoint { } // Explicit rollback_to(): same idempotency-as-error policy as - // release(). The engine pops the savepoint stack down to AND - // including this savepoint, so the mark itself is gone after - // one successful call — a second call would fail at the engine - // layer with SavepointNotFound regardless. The guard here turns - // that into a cleaner, more specific AlreadyFinishedError. + // release(). Note that the engine does NOT remove this savepoint — + // `rollback_to_inner` ends with `savepoints.truncate(idx + 1)`, which + // pops only the savepoints layered ON TOP and deliberately retains + // this one so it can be rolled back to again or released + // (src/transaction/savepoints.rs:47-49). So a second engine-level + // rollback_to would SUCCEED, not fail with SavepointNotFound. + // + // The guard is therefore load-bearing, not a re-labelling of an error + // the engine would raise anyway: it is the only thing enforcing the + // Python contract that a Savepoint object is single-use, which is what + // makes `with`-block exit and an explicit call unambiguous. Removing it + // would silently turn repeated rollback_to into a working operation. fn rollback_to(&self, py: Python<'_>) -> PyResult<()> { if self.finished.load(Ordering::SeqCst) { return Err(already_finished_err()); diff --git a/python/tests/test_exception_contract.py b/python/tests/test_exception_contract.py index d3d46b2..c1b9426 100644 --- a/python/tests/test_exception_contract.py +++ b/python/tests/test_exception_contract.py @@ -23,15 +23,17 @@ def test_savepoint_not_found_via_rollback_to(mem_db): - # Build a stack [sp1, sp2], then use sp1.rollback_to() which pops BOTH - # sp1 and sp2 from the engine. The sp2 Python object still exists and - # its guard is NOT set (we never called sp2.release/rollback_to), so - # sp2.release() goes to the engine which no longer knows "sp2" → - # SavepointNotFoundError. + # Build a stack [sp1, sp2], then use sp1.rollback_to(), which pops + # everything layered ON TOP of sp1 — so sp2 goes and sp1 stays (the + # engine retains the named savepoint so it can be rolled back to again; + # src/transaction/savepoints.rs:47-49). The sp2 Python object still + # exists and its guard is NOT set (we never called + # sp2.release/rollback_to), so sp2.release() goes to the engine, which + # no longer knows "sp2" → SavepointNotFoundError. with mem_db.transaction() as tx: sp1 = tx.savepoint("sp1") sp2 = tx.savepoint("sp2") - sp1.rollback_to() # pops both sp1 and sp2 from the engine stack + sp1.rollback_to() # pops sp2; sp1 itself remains on the engine stack # sp2's guard is still clear → this reaches the engine → not found with pytest.raises(chisel.SavepointNotFoundError) as exc_info: sp2.release() @@ -43,7 +45,7 @@ def test_savepoint_not_found_via_release(mem_db): with mem_db.transaction() as tx: sp1 = tx.savepoint("sp1") sp2 = tx.savepoint("sp2") - sp1.rollback_to() # pops sp1 and sp2 + sp1.rollback_to() # pops sp2; sp1 itself remains on the engine stack with pytest.raises(chisel.SavepointNotFoundError) as exc_info: sp2.rollback_to() assert isinstance(exc_info.value, chisel.OperationalError)