Repository navigation
feat(uniffi,bindings): initialise MLS with the built-in file stores - #470
Merged
Merged
Conversation
It turns out a stopped ProtocolManager with BLE or the peer stream enabled is never freed. The core holds the registered transport callback, the callback holds its transport manager, and the manager holds the core. That cycle runs through Rust, so Python's collector can't see it, let alone break it. On the keyring path that was a quiet leak. On the file-store path it's an outage: the core holds each store's directory lock, so every later manager over the same directories is refused with InvalidState, stop() or not, gc.collect() or not. The tests only ever used an internet-only config, which is the one case where the cycle doesn't form. Once the engine has stopped cleanly, register callbacks that reference nothing. start() registers the real ones again, so stop() then start() on one manager keeps working, and dropping a stopped manager now closes its stores. The `as` name of an `async with` still outlives the block; the README says to drop it. While at it, take a copy of store_key at construction. Holding the caller's bytearray meant a buffer reused before start() created a fresh store sealed under whatever it held by then, after which the real key was refused forever. And refuse an all-zero key, an empty store_key_env and a blank root variable in the constructor, where the class says configuration errors surface, instead of at start().
…re docs A late initialize_mls_with_file_stores() call was refused with a message telling the caller to call initialize_mls before start(). Right rule, wrong method. The shared body now takes the entry point's name and puts it in the refusal. The docs also promised more than the code does. The key is not scrubbed on "this side": UniFFI lifts bytes by copying, the buffer it copied from is freed as is, and a Python bytes can't be scrubbed at all. And "a refusal changes nothing" is false for a state root that can't be created, because the MLS store has already made its directory and key check by then. A retry works, but say so rather than pretend. InvalidState also covers a call after start(), which the error table left out. The UDL change is a comment, so the generated bindings don't move; a regeneration diffs clean.
The last round made stop() release the BLE and peer-stream callbacks, so dropping a stopped manager closes its stores. It turns out there were three more ways to keep that manager alive, and each one leaves the store locks held, so the next manager over the same directories gets InvalidState. First, stop() awaited the cancelled process task and swallowed its CancelledError. That exception's traceback holds stop()'s frame, and so `self`. The task step that raised it keeps it until the caller next yields. On Python 3.13+ the telemetry hand-off can complete without suspending, so stop() finished inside that very step and `del` freed nothing. It hit about one run in twenty on 3.13 and 3.14 and never on 3.12, which is the version CI runs. Wait on the task instead of awaiting it, so nothing is raised into the frame at all. Second, the event callback was never released. The core holds it, it holds the application's handler, and the usual handler is a bound method of whatever owns the manager. That is the same cycle through Rust as before, only on every Python, every time. Swap in an inert event callback along with the transport ones. What's left is a plain Python cycle, so the collector frees it; the README says to call gc.collect() for that case. Third, a start() that failed after the stores opened left `_running` false, so stop() returned at once and the callbacks stayed registered for good. Release them on that path too. While at it, trim a root read from the environment. A service manager's environment file happily leaves a trailing newline, and the key text was already trimmed for the same reason. An explicit argument is still taken as given. Each fix has a test that fails without it, and the traceback test forces the non-suspending hand-off so it fails on 3.12 as well.
The package declares 3.10 through 3.13, but CI only ever ran 3.12. A manager pinned by a caught CancelledError showed up only on 3.13+ (an executor hand-off that completes without suspending), and CI stayed green right through it. Run pytest a second time on 3.13 against the library the job has already built. It costs an install and a test run, not a rebuild.
…p() finish A stopped Python manager could still hold the file-store locks in two more ways, and every later manager over the same directories got InvalidState until the process died. It turns out that releasing three of the five callback slots is not releasing the callbacks. The Nostr and Reticulum stubs tell an app that drives those transports to register its own callback, and that callback needs the core to drain its queue. Holding *only* the core is enough: the core is never freed, and neither are the stores. The second one is dumber. stop() clears the running flag before it tears anything down, so a stop() cancelled part-way by a shutdown deadline, or one whose engine stop raised, cannot be called again. The retry sees "not running", returns, and the callbacks stay registered for good. With BLE or the peer stream on, that is the process lifetime. Swap all five slots, each in its own try so one refusal does not leave the rest held; every setter accepts a callback for a disabled transport. And track a pending teardown separately from running, so a retry finishes the job. A second engine stop is accepted, so the retry is safe. This is the fifth way lock release has failed by hanging off Python freeing the core. An explicit close over the FFI is the real fix and wants its own PR.
The last round made a cancelled stop() retryable at the manager. It turns out that only helps when the cancel lands *between* transports, and a shutdown deadline lands where the waiting is: inside one. Every transport's stop() moves to STOPPING first and then returns at once for anything that is not RUNNING or STARTING. Cancel it part-way and it is wedged. The retry walks past it in a few milliseconds, calls the teardown clean, and releases nothing that transport still holds. With a peer-stream listener that is the open server, which holds the manager, which holds the core, which holds the store locks. Every later manager over those directories gets InvalidState. Let each transport's stop() resume from STOPPING. The steps after the guard already check what they close, so running them again is safe. That makes overlapping stop() calls worse, not better: a second one would enter the same transport stop and interleave over the same sockets and discovery handle. Even before this, it stopped the engine and dropped the callbacks while the first was still closing a transport. So serialise stop() behind a lock. A second caller waits for the first and then finds nothing left to do. While at it, the BLE peripheral awaited its cancelled monitor task and caught CancelledError, which also ate a cancel of stop() itself. A caller's deadline was simply ignored. Wait on the task instead. The explicit close over the FFI is still the real fix. Tying a directory lock to Python freeing an object has now failed six ways.
The last commit let all four transports resume a stop() that was cancelled part-way, and stopped the BLE peripheral from eating a cancel of its own stop(). It then tested exactly one of those five changes: the peer stream. It turns out you could put the old guard back in the BLE central, the BLE peripheral or the internet transport, or put the swallowed cancel back in the peripheral, and the suite stayed green. Four mutations, four survivors. A fix that nothing fails without is a fix that lasts until the next person tidies it up. The excuse for the peripheral was that it needs bless. It does not. The existing peripheral tests already run over a mocked server, and so do the central's and the relay's. Add a test per transport that cancels stop() inside the one thing it waits on (the scanner, the GATT server, the close handshake), checks it is left in STOPPING, and retries to a finished teardown. Add one that cancels the peripheral's stop() while it waits on a monitor that is slow to wind down, and expects the cancel to come out. Each mutation now fails the test written for it, on 3.12 and 3.14. None of this touches a real adapter, which is still not run.
Moving a deployment from the keyring onto the file stores keeps its state root more often than not. Both modes read the same OFFLINE_PROTOCOL_STATE_ROOT, so doing nothing is enough to keep it. It turns out that is quietly destructive. Nothing carries the old identity over, so the MLS store mints a new one and the device gets a new address. The new identity's record key cannot unseal the old records, and the first restore deletes the parked messages it could not read. Switching back gives you the old address and an empty queue. So refuse it before either store opens: if the MLS account directory does not exist yet and the state account directory holds anything but a lock file, the call fails with InvalidConfiguration and names the two ways out (a fresh state root, or the mls_root that wrote the state). Nothing is created on the way. The same check catches a lost mls_root next to a surviving state root, which is the same data loss by another route. While at it, refuse one directory for both roots. That put the identity inside the directory the installation removes, which is exactly what the two-root split exists to prevent.
start() registers every callback with the core before it opens the file stores. A failed engine start already released them, but a refusal from the stores themselves (a wrong key, a held directory, an unset key variable) just raised. stop() does nothing for a manager that never ran, so nothing ever released them. With BLE on, every refused attempt leaked a core. With an event handler bound to the object that owns the manager, it leaked the manager too. No lock stayed held, so later managers still opened, but a supervisor retrying a refused start grew by about 42 KiB an attempt for as long as the refusal lasted. Release and re-raise there too, the same as the engine start. While at it, document that a child made with a bare os.fork() while the stores are open inherits their locks and holds them until it exits. That is how flock works, and there is no fix on this side.
The FFI's kept-state guard needs to know whether an MLS account directory holds an identity. The only way to answer that today is to look at the directory from outside, which means either guessing from its existence or hardcoding the store's private file prefixes in another crate. The first is wrong and the second is fragile. It turns out existence is exactly the wrong question. An open that is refused after the account directory was made leaves that directory behind with a lock file and a key check, and no identity at all. Add a read-only probe to both stores. It creates, locks and changes nothing. The sealed store counts only its record directories. The state store counts anything but its lock file, which is all an open that wrote nothing leaves behind. A directory that cannot be read is an error, never "empty". A caller deciding whether it may start a new identity over a directory must not take a permission error for a fresh install.
…e guard The kept-state guard refuses a state root full of records when there is no identity beside it, because the new identity would delete every record it cannot unseal. It decided "no identity" by asking whether the MLS account directory exists. That is not the same question. An attempt refused at the state root has already opened the MLS store, which leaves its directory with a lock file and a key check and nothing else. The operator fixes the state root, retries, the directory now exists, and the guard waves the new identity straight through onto the old state. The parked messages are gone. We documented that retry as safe. The same function read an unreadable state directory as empty, and existence checks read an unreadable MLS root as "no identity", which sends the operator after the wrong fix. Ask the MLS store whether it holds records, and refuse a directory that cannot be read instead of guessing what is in it. While at it, the roots check now refuses one root inside the other, not only the same directory, and resolves both roots before comparing: the longest existing prefix is canonicalized and the rest resolved by component, so `two/../one` and a symlink are caught even before anything exists. Removing a state root must never remove the identity.
A file store held its directory lock until the last reference to it dropped. That sounds reasonable right up to the point where the holder is a garbage-collected runtime, a callback cycle, or an engine that keeps an Arc somewhere you did not look. Then the directory stays locked until further notice, and the next store over it is refused. So give both stores a close(). It releases the lock at once, whoever still holds the store, and every later operation is an error. A *failed operation*, never a lost record: the engine settles a lost record as gone for good and regenerates a lost record key, and a closed store must not talk it into either. The lock now lives inside the store's own mutex, so a write in flight finishes before the directory goes and none lands after. The probes got honest too. holds_records() counted a type directory as a record, which a first write that failed leaves behind with nothing in it. The state side counted anything that was not a lock file, including what the *other* store leaves in a directory the two were wrongly made to share. Both now look for a record file, and a directory that lists but cannot be searched is an error, not "empty". New: sealed_state(). The engine deletes a sealed record that does not open, which is right for one damaged record and a disaster for a store sealed under another identity's key. This answers, read-only, whether the pair belongs together: Empty, Opens, Foreign or KeyDamaged. The ceiling is per category, because damage in one category says nothing about the rest. The record key is read by the same function the restore uses, so the two cannot come to disagree about which failures are permanent.
A host object kept the store directories locked for as long as it lived, and it lived for as long as *anything* referred to it: a name the application kept, a callback cycle, an exception whose traceback still named it. When an object is freed is the runtime's decision. Tying a directory lock to that decision was never going to end well. Add close_file_stores(). It releases both directories while the instance still exists, after stop(). The instance cannot run again afterwards: start(), enable_telemetry() and both initialize_mls entry points are refused, because the engine still holds the closed stores and would fail every write. Telemetry is stopped first, under the lifecycle lock, since the uploader keeps its queue in the state store and its final flush has to land before that store closes. A poisoned engine lock counts as stopped. stop() cannot take it again, and "free the object" is exactly the answer this call exists to avoid. The entry point also learned two things the kept-state guard could not see. An identity can be in place and still not be the one that wrote the state, which is what an operator has after following the guard's own advice and later putting the old state root back. That is now refused once both stores are open, with no record changed. A damaged record key is *not* refused: nothing opens that state again, and the engine's regeneration is what settles the messages lost with it. Roots are compared a second time once they exist. A volume that folds case makes one directory of two spellings, and no comparison of names can know that in advance. A pair the engine refuses is released under the engine lock the refusal was given under, by closing it, not by hoping the last Arc goes away. All three bindings regenerated: one added method, nothing else.
The generated bindings keep every callback in one handle map behind a plain threading.Lock, taken on every callback lookup and every removal. The collector runs between any two bytecodes, including the ones inside that critical section. When it finalizes a core object there, the core's drop releases its callbacks, each release calls back into the map, and the thread asks for a lock it already holds. It waits for itself. Forever. This could not happen while stopped managers were never freed at all, so it arrived together with the fix for that leak. All it takes is a core object in a reference cycle (an event handler that is a method of the manager's owner will do) and a collection at the wrong bytecode. It showed up once in a few thousand runs, as a test process sitting at zero CPU. Give the map a re-entrant lock when the package is imported. Re-entry is removal only, and each critical section is a single dictionary operation, so an inner removal cannot leave the outer one looking at a half-changed map. The generated file cannot carry the change itself: it is regenerated from the UDL and the drift gate compares it byte for byte. The test frees a core while holding the map's lock, in a process of its own, so a regression fails one test instead of hanging the suite.
CI went red on a test that reopens the stores after a stop() that was cancelled and retried. Not a flake. A caller retrying after a cancelled stop() is still inside the task step that threw the CancelledError, and that step holds the exception, whose traceback holds the manager, until the task next suspends. On Python 3.13 and later an executor hand-off whose worker already finished completes in place. So about one run in fifty nothing in the retry suspended, the manager outlived `del`, and the next one was refused. stop() now takes one loop turn, always. After the process loop is cancelled and not before, or that loop gets one more tick and delivers one more message after stop() was called. That fixes the symptom. The disease is that releasing the stores depended on the interpreter freeing an object. So add close(): it stops, asks the core to release the stores, and the directories are free when it returns, whatever still refers to the manager. Leaving `async with` closes a manager that uses the file stores. So does an entry that failed after the stores opened, since `__aexit__` never runs for it and the inline form leaves no name to close. Every other manager keeps the exit it always had. The release is shielded and marks the manager closed from the hand-off itself: a caller's cancel does not stop the core, so the manager must end up closed whether or not anyone is still waiting. A loop whose executor was shut down runs the blocking steps in place, because a teardown that cannot finish is worse than one that blocks. Without the file stores, close() reports an engine that would not stop by itself, since the core has no stores to refuse.
The guide, the bridge contracts, the Python README and the changelog described a release that waited for the instance to be dropped, and refusals that "change nothing". The first is no longer how it works. The second was never quite true: a refusal after the MLS store opened leaves its directory, its lock file and its key check. Say what is actually guaranteed, which is that no refusal changes or deletes a *record*, and name what each refusal leaves behind. While at it, stop claiming that the engine deletes every sealed record of a foreign store. It deletes the ones its restore reads, and the data-layer records survive.
The file-store guard decided whether a protocol-state root was another identity's by asking its sealed records whether they open under the MLS store's record key. That works right up until the engine regenerates a damaged record key, which it does on purpose. It turns out the launch that regenerates the key cannot delete everything sealed under the lost one. Documents are read when they are opened, never at launch, and every restore walk stops at its delete allowance. So the next launch finds sealed records that open under no key the store holds, calls them foreign, and refuses the same identity over its own roots. On every launch, forever. Stop inferring ownership and record it. The first open of a pair writes one random pairing id into both stores, and later opens compare the ids. Nothing the engine does to its record key touches them. The record scan stays, but only for a state root with no id or an MLS store without a readable one. A new id goes to the state store first. The other order lets a crash between the two writes leave the stores on different ids, which reads as someone else's state for good. Neither pairing file is a record, so the probes that decide whether a new identity may start still see an empty pair. The guard itself now lives here as FileStorePair::open, so a Rust host gets the same refusals instead of reassembling them by hand. While at it, holds_entry propagates a failed entry-type read instead of calling it "no record". The whole point of that probe is not to mistake an unreadable directory for an empty one.
The FFI carried its own copy of the file-store guard: the root overlap checks, the state-without-identity refusal, and the foreign state verdict. That copy is exactly what just moved into the core as FileStorePair, so keeping it here would mean two guards drifting apart. Please don't. open_file_stores now validates what only the FFI owns (strings, bytes, the account namespace) and hands the rest to the pair. The new refusals map onto existing error variants, so the append-only enum is untouched. The FFI tests gain the relaunch that failed in review: a document sealed under a record key that is then damaged, one launch that regenerates the key, and the launch after, which used to be refused as another identity's state.
On Python 3.10 a manager whose stop() caught an error (telemetry would not disable, the engine stop raised) outlived `del`, and with the file stores its directories stayed locked until the collector got around to running. The reason is the logged exception's traceback. It holds the teardown's frames, and through them the manager, and the executor future that carried it keeps the whole thing in a reference cycle. 3.11 and later free it at once. 3.10 waits for a collection. Drop the traceback once it is logged. Nothing reads it afterwards. Nobody noticed because CI never ran the oldest Python the package declares. It does now.
The guides said the file-store guard asks the sealed records whether a state root belongs to this identity. It does not anymore, and that reading was wrong after a regenerated record key anyway. Describe the pairing ids, when the records still decide, and that a damaged record key stays harmless on later launches only while the MLS root keeps its pairing file. Also say what the ids cannot see: another writer sharing the state root. Two identities over one state root already delete each other's sealed records, so keep a file-store state root to the file stores. The Rust example now opens the stores as a pair.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A binding can now open the built-in file stores from #468 instead of supplying its own storage providers. This is what a headless host without a platform keyring needs.
initialize_mls_with_file_stores(mls_root, state_root, store_key). It opensSealedFileMlsStorageatmls_root, sealed under the 32-bytestore_key, andFileProtocolStateStorageatstate_root, in the account directory derived from the instance's ownapp_idandprofile. It keepsinitialize_mls's lifecycle: beforestart(), idempotent, and a repeated call opens nothing.close_file_stores(). It releases both directory locks at once, while the instance still exists. When a host object is freed is the host runtime's decision, and until then every later instance over the same directories was refused. It is valid afterstop(). The instance cannot run again afterwards. It disables telemetry first, so the uploader's final flush reaches its queue.FileStorePair::openin the core. It opens both stores together and refuses the pairs the engine would lose data over. The FFI entry point calls it, and Rust hosts get the same refusals.ProtocolManager(config, store_key=...)orstore_key_env=selects the file stores.close()stops and releases. Leavingasync withcloses a file-store manager and stops any other. On this pathstart()raises instead of starting without the identity.What the pair refuses
No refusal changes or deletes a record. No new FFI error variant: the append-only enum (C2) is untouched.
InvalidArgumentInvalidConfigurationstart()has run, orclose_file_stores()hasInvalidStateOwnership is recorded, not inferred. The first open of a pair writes one random pairing id into both stores, the state store first, so a crash between the writes never leaves the two on different ids. A later open compares them. When either store holds no readable id, the pair is judged by whether its sealed records open under the MLS store's record key. The ids say which MLS root a state root was paired with, not who wrote to it last, so a state root shared with another writer is not detected. Scanning the records alone refused the same identity over its own roots on every launch after a damaged record key was regenerated, because that launch cannot delete everything sealed under the lost key.
A damaged record key is not refused, on the launch that finds it or on any launch after while the MLS root keeps its pairing file: the engine regenerates it and settles what it lost as failed.
Also fixed
stop()swaps in inert callbacks for all five slots. An interrupted or overlappingstop()can be finished.ProtocolManager.__del__no longer raises on a manager whose constructor raised.Validation
cargo fmt --check,cargo clippy --workspace -D warnings, rustdoc under-D warnings, andcargo test --workspace --liball pass locally.Breaking changes
None. New UDL methods, new keyword-only Python parameters, and new
FileStoreErrorvariants on a#[non_exhaustive]enum.Notes for reviewers
secure_storage.pyis untouched. It overlaps fix(bindings): warn when Python SecureStorage gets keyring's failing or null backend #469.stop().