Codebase audit fixes - #21
Merged
Merged
Conversation
…bugs - :memory databases give each reader thread its own engine handle over the shared CoreMemory, since a handle carries per-handle mutable state. - Values read from one database are refused when written into another (IllegalArgumentException pointing at materialize). Engine handles are registered in the new xitdb.util.db-registry namespace so a value read through any of a database's handles is accepted when written back. snapshot-memory-db materializes before copying. - Keypath writes store the key for new hash-map entries and refuse hash-set member cursors, as sorted sets already did. - XITDB wrappers nested in plain collections are unwrapped before writing, so sorted maps/sets keep their type and are structurally shared. - Cleanups: remove dead set-write-cursor and the write-value!/slot-for-value! aliases; reuse map-write-cursor-storing-key! from map-assoc-value!. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…handles Values read from a database carried the engine handle of the thread that read them, so sharing one across threads raced on that handle's digest and file position. Each database now has a single shared reader handle: - The engine's read path only touches a handle's Core and its immutable header. The in-memory Core already keeps a per-thread position; file databases get a Core that delegates to a read-only file handle per thread (thread-local-file-core) and closes every one of them on close, so closing from one thread no longer leaks the other threads' descriptors. - Key hashing (db-key-hash) uses per-thread MessageDigests of the handle's algorithm instead of the handle's shared digest. - materialize copies map keys as well as values, so a map with collection keys can be written into another database (and snapshot-memory-db works). The compaction digest test is rewritten since Clojure-side hashing no longer goes through the engine digest it instrumented. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n file descriptors
Collaborator
|
I updated xitdb-java with a bunch of thread safety fixes. I also was able to remove some of the changes in this branch by moving the functionality into the java library:
The last commit is optional, but it replaces the global db registry with a "db context" that accomplishes the same thing without global state. It allows the writeable database to find its readable equivalent without a global registry. Just thought it would be a good addition so I threw it in. |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Audits done by Fable 5.1 and GPT 6 Astra.
Fixes a set of correctness and thread-safety bugs found in a codebase audit, with regression tests for each.
Thread safety
Values read from a database can now be used from any thread. Reads go through one shared read-only handle instead of a per-thread Database, with a per-thread file handle behind a single Core for file-backed databases. Memory databases previously shared the writer's Core across threads unsafely.
Key hashing uses per-thread MessageDigest instances instead of the handle's shared digest, which raced when a read value was passed between threads.
Terminated reader threads no longer retain file descriptors. Per-thread file handles are tracked weakly and all remaining handles are closed when the database is closed. Touching a closed database from a new thread throws IllegalStateException.
Write correctness
A failed key lookup (e.g. an unsupported key type) could leave partial state in the shared digest and corrupt the next write. The digest is now reset even when hashing throws.
Writing a value read from one database into a different database is now refused with IllegalArgumentException. Slots are offsets into the source database's storage, so this silently wrote garbage before. A small registry ties each database's reader and writer handles together so round-tripping within the same database still uses structural sharing. materialize the value to copy it across databases (documented in the README).
Keypath writes to an absent hash-map key stored only the hash, leaving a keyless entry. write-cursor-for-key now stores the key as well.
Keypath writes into hash-set members now throw, matching sorted sets. Set members are immutable keys, so a write through a member cursor filed the value under the wrong hash.
materialize now materializes map keys too, not just values, for hash maps and sorted maps.
v->slot! unwraps XITDB wrapper types before the generic collection branches, so nested wrapped values are written by reference instead of being deep-copied (and sorted maps/sets no longer degrade to hash ones inside collections).
compact gives the compacted copy its own digest instead of sharing the source's.
Other
snapshot-memory-db materializes before writing into the memory database.
unwrap uses an interface check instead of satisfies? on the per-element write path.
Function behaviour is explained in docstrings instead of inline comments.
Tests added: close/file-descriptor lifecycle, cross-database writes, hashing after failed lookups, multi-threaded read sharing, keypath cursor writes, nested wrapped values, and snapshot materialization.