Skip to content

Add an in-memory storage backend - #31

Open
onslaughtq wants to merge 4 commits into
mainfrom
in-memory-storage
Open

Add an in-memory storage backend#31
onslaughtq wants to merge 4 commits into
mainfrom
in-memory-storage

Conversation

@onslaughtq

Copy link
Copy Markdown
Member

Adds newInMemoryKiokuDB/withInMemoryKiokuDB, which hold the same content-addressed representation as an on-disk database in an in-process map instead of files, so consumers (such as test suites) can build and query databases without any filesystem access.

All object reads/writes now flow through storage primitives that dispatch on the backend. The file backend keeps its existing streaming, mmap-based behavior; the trie index writer is generalized from a Handle to a byte sink to support both. The query test suite runs every case against both backends and property-checks that they give identical results.

onslaughtq and others added 2 commits July 15, 2026 15:01
Adds newInMemoryKiokuDB/withInMemoryKiokuDB, which hold the same
content-addressed representation as an on-disk database in an in-process
map instead of files, so consumers (such as test suites) can build and
query databases without any filesystem access.

All object reads/writes now flow through storage primitives that
dispatch on the backend. The file backend keeps its existing streaming,
mmap-based behavior; the trie index writer is generalized from a Handle
to a byte sink to support both. The query test suite runs every case
against both backends and property-checks that they give identical
results.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an in-memory KiokuDB storage backend alongside the existing file-backed implementation, routing reads/writes through backend-specific storage primitives so callers (notably tests) can use Kioku without filesystem access.

Changes:

  • Introduces Storage (FileStorage / MemoryStorage) and storage primitives (storageRead/write/list/remove, createBlob) to unify persistence behavior.
  • Generalizes trie index writing to target a byte sink rather than a Handle, enabling both file and memory backends.
  • Updates the query test suite to run against both backends and adds a property asserting backend result equivalence.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/QueryTest.hs Runs query tests against both file and in-memory backends; adds an equivalence property test.
src/Database/Kioku/Internal/TrieIndex.hs Generalizes index writing to stream to a ByteString sink instead of a Handle.
src/Database/Kioku/Internal/KiokuDB.hs Introduces Storage abstraction and backend-dispatching read/write/list/remove plus createBlob.
src/Database/Kioku/Internal/Buffer.hs Adds bufferFromByteString to support in-memory buffers.
src/Database/Kioku/Core.hs Exposes newInMemoryKiokuDB / withInMemoryKiokuDB and routes dataset/index creation through createBlob.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/QueryTest.hs
Comment on lines +42 to +46
backends :: [Backend]
backends =
[ Backend "file backend" (withKiokuDB defaultKiokuPath)
, Backend "memory backend" withInMemoryKiokuDB
]

@onslaughtq onslaughtq Aug 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the file backend tests now open each database in its own withSystemTempDirectory "kioku-test", nested outside withKiokuDB so the directory is only removed after the mmapped buffers are unmapped. temporary was already a library dependency, so it just needed adding to the test suite. Fixed in 5ce817b.


🤖 This reply was written by Claude Code.

Comment thread src/Database/Kioku/Internal/KiokuDB.hs Outdated
Comment on lines +103 to +106
createDirectoryIfMissing True (takeDirectory fullPath)
BS.writeFile fullPath bytes
MemoryStorage ref ->
modifyIORef' ref (M.insert path bytes)

@onslaughtq onslaughtq Aug 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — switched to atomicModifyIORef'. Fixed in 5ce817b.


🤖 This reply was written by Claude Code.

Comment thread src/Database/Kioku/Internal/KiokuDB.hs Outdated
Comment on lines +131 to +134
FileStorage root ->
removeFile (root </> path)
MemoryStorage ref ->
modifyIORef' ref (M.delete path)

@onslaughtq onslaughtq Aug 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — switched to atomicModifyIORef'. Fixed in 5ce817b.


🤖 This reply was written by Claude Code.

Comment on lines +161 to +166
let
lazyBytes = Builder.toLazyByteString builder
sha = hashBytes lazyBytes

modifyIORef' ref (M.insert (dataFilePath sha) (LBS.toStrict lazyBytes))
pure (sha, result)

@onslaughtq onslaughtq Aug 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — the shared map insert now uses atomicModifyIORef'. The builderRef in the same branch is left as modifyIORef' since it is a per-call local ref whose sink is driven sequentially by writeRows/writeIndex. Fixed in 5ce817b.


🤖 This reply was written by Claude Code.

Comment thread src/Database/Kioku/Internal/KiokuDB.hs Outdated
Comment on lines +117 to +125
let
prefix = addTrailingPathSeparator path
childName key = takeWhile (not . isPathSeparator) <$> stripPrefix prefix key
stripPrefix pre str =
if pre == take (length pre) str
then Just (drop (length pre) str)
else Nothing

pure $ nub $ mapMaybe childName $ M.keys contents

@onslaughtq onslaughtq Aug 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — deduped via Data.Set instead. The hand-rolled local stripPrefix went away at the same time in favor of Data.List.stripPrefix, which has identical semantics here. Fixed in 5ce817b.


🤖 This reply was written by Claude Code.

@qxjit qxjit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copilot feedback is worthwhile here, but I think this look good aside from those things.

Make the shared MemoryStorage map mutations atomic. modifyIORef' is a
non-atomic read-modify-write, so concurrent writers on a KiokuDB shared
across threads could lose updates. This also brings Storage in line with
the BufferMap field of the same record, which is already MVar-guarded.

Dedupe storageList through a Set rather than the O(n^2) nub, and drop
the hand-rolled stripPrefix in favor of the Data.List one, which has the
same semantics here.

Give each file backend test database its own temporary directory instead
of pointing them all at defaultKiokuPath. The tests were accumulating
blobs in the developer's (bind-mounted) working directory on every run
and could clobber a real local Kioku database.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Database/Kioku/Internal/Buffer.hs:33

  • Comment grammar: “does not using” / “do not depend end” reads like a typo and is a bit hard to parse.
-- This function does not using a length header to determine where
-- to stop reading, so it can only be used with Memorizable instances
-- that do not depend end of the ByteString for parsing

src/Database/Kioku/Internal/KiokuDB.hs:154

  • createBlob (file backend) does not guarantee the temp Handle is closed and the temp file is removed if writer (or hashing/rename) throws. That can leak file descriptors and leave behind tmp artifacts (and can break cleanup on platforms that lock open files). Wrap the temp file lifecycle in bracketOnError/finally so resources are always released.
    FileStorage root -> do
      (tmpFile, h) <- openTempFile (root </> tmpPath) name
      result <- writer (BS.hPutStr h)
      hClose h

      sha <- hashBytes <$> LBS.readFile tmpFile
      renameFile tmpFile (root </> dataFilePath sha)
      pure (sha, result)

Guard the file branch of createBlob with onException. Nothing cleans up
tmpPath -- gcKiokuDB only walks the data directory -- so a writer that
threw left both an open handle and an orphaned temp file behind. This
matters more than it used to: openTempFile now runs before the writer, so
the handle is held across the whole index sort (writeIndex sorts before
invoking its callback), where previously createIndex opened the temp file
inside that callback, after the sort had finished.

Raise a doesNotExist IOError rather than a KiokuException for a missing
object in the memory backend, matching what the file backend has always
thrown. A caller that handled a not-yet-created dataset or index by
testing isDoesNotExistError would otherwise pass against an in-memory
database and catch nothing in production. KiokuException stays for
corrupt index and schema content, which is a Kioku-level error rather
than an I/O one.

Document the remaining intentional divergence in storageList, where a
path naming nothing yields an empty list in memory but throws on disk,
and note on newInMemoryKiokuDB that its buffers outlive closeKiokuDB
while mmapped ones do not.

Also make the writeRows row counter strict and correct the equivalence
test's comment, which claimed more than the test checks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@onslaughtq

Copy link
Copy Markdown
Member Author

Ran a deeper review pass over the whole branch after the Copilot fixes; 14cd263 addresses what it turned up.

createBlob file branch was not exception-safe. There is no cleanup between openTempFile and hClose/renameFile, so a throwing writer orphaned both the handle and the temp file — and nothing ever cleans tmp/, since gcKiokuDB walks only data/. This PR widened the window: createIndex used to call openTempFile inside the flushIndex callback, i.e. after buildSortedOffsetArray finished, whereas createBlob opens it before invoking the writer, so the handle is now held across the entire sort (including every keyFunc call). Wrapped in onException with hClose + removePathForcibly. Confirmed with a throwaway probe: before the fix a failed createDataSet leaves tmp/probe1-0 behind, after it tmp/ is empty.

Missing objects threw different exception types per backend. storageRead raised KiokuException under MemoryStorage but let BS.readFile throw an isDoesNotExistError IOException under FileStorage. Since the point of the memory backend is to stand in for a real database, a caller handling a not-yet-created dataset/index via isDoesNotExistError would pass in memory and catch nothing in production — the probe confirmed the KiokuException escapes an IOException handler outright. The memory branch now raises the same doesNotExistErrorType IOError; KiokuException stays for corrupt index/schema content.

Also in that commit: writeRows' row counter is strict (it was building an N-deep thunk chain), and the equivalence test's comment no longer claims to prevent representation drift — it only compares query results, each produced wholly within its own backend.

Consciously left alone: storageList returns [] for a path that names nothing under MemoryStorage but throws under FileStorage. Nothing reaches it today (openKiokuDB creates the top-level directories, writeObjFile the per-namespace ones), so it is a latent trap rather than a live bug; it is now documented on the function instead of changed. Asserting cross-backend SHA equality would pin the "same content-addressed representation" claim properly, but it needs Database.Kioku.Internal.KiokuDB in exposed-modules, which did not seem worth publishing internals for.


🤖 This comment was written by Claude Code.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Database/Kioku/Internal/KiokuDB.hs:170

  • In createBlob (file backend), the cleanup action calls hClose h unconditionally. If an exception occurs after the explicit hClose h in the normal path (e.g., during hashing or renameFile), onException will run cleanup and hClose will likely throw “handle is closed”, masking the original exception and potentially preventing temp file cleanup.
        cleanup = do
          hClose h
          removePathForcibly tmpFile

@onslaughtq
onslaughtq requested a review from qxjit August 13, 2026 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants