Refactor oxen's Merkle tree operations: use MerkleStore

[malcolmgreaves]

Refactor
Change all reading & writing of Merkle tree nodes to use the new traits:

• Use MerkleReader::exists calls in commit_merkle_tree.rs
• Use MerkleReader::get_{node,children} in merkle_tree_node.rs
• Use MerkleWriter's write sessions in:
  • create_empty_commit & create_initial_commit (commits.rs)
  • update_metadata (entries.rs)
  • commit_dir_entires, write_commit_entires, r_create_dir_node (commit_writer.rs)
  • run_on_commit, rewrite_nodes (migration ..add_child_counts_to_nodes.rs)
  • write_tree, p_write_tree (tree.rs)

Access to a MerkleStore for these operations is provided via the LocalRepository.

LocalRepository
Adds method LocalRepository::merkle_store() -> Box<dyn MerkleStore + '_>. Provides the
repository's Merkle tree node storage implementation. The return type is dynamic: it
allows for selecting a compatible MerkleStore at runtime.

OxenError Updates
Converts 2 string errors into:

• MerkleNodeNotFound(HexHash): no such commit/dir/vnode in lookup
• DisallowedNodeWrite(std::any::type_name_of_val)

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Migrate Merkle node I/O from per-node MerkleNodeDB read/write to a session-based MerkleStore / MerkleWriteSession model across repository accessors, node loading, tree/commit writers, metadata traversal, and a migration, with explicit per-namespace finish() and long-lived write sessions.

Changes

Session-backed Merkle write migration

Layer / File(s)	Summary
Store accessor crates/lib/src/model/repository/local_repository.rs	Add LocalRepository::merkle_store() returning Box<dyn MerkleStore + '_> (currently FileBackend).
Existence / Load surface crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs, crates/lib/src/core/v_latest/index/commit_merkle_tree.rs	Replace MerkleNodeDB existence/get with repo.merkle_store().exists/get_node/get_children; preserve missing-node semantics and propagate store errors.
Core write API changes crates/lib/src/repositories/tree.rs, crates/lib/src/repositories/commits/commit_writer.rs, crates/lib/src/core/v_latest/commits.rs	Replace per-node MerkleNodeDB open/add/close with repo.merkle_store().begin() → session.create_node(...) → NodeWriteSession::add_child(...); add explicit ns.finish() and session.finish(); update private function signatures to accept &dyn MerkleWriteSession / &mut dyn NodeWriteSession.
Traversal & metadata update crates/lib/src/core/v_latest/entries.rs	Use a single long-lived MerkleWriteSession for traversal; change traversal helpers to accept &dyn MerkleWriteSession, create per-node namespaces, write children via add_children_to_session, finish namespaces, and update recursion to aggregate counts/sizes via session writes.
Migration wiring crates/lib/src/command/migrate/m20250111083535_add_child_counts_to_nodes.rs	Start write sessions on old/new repos in run_on_commit, create/finish commit namespace, pass both sessions into rewrite_nodes, and create/finish directory/vnode namespaces using respective sessions while forwarding sessions through recursion.
Backend docs crates/lib/src/core/db/merkle_node/file_backend.rs	Document that FileBackend::begin() returns a FileWriteSession and create_node() returns a FileNodeSession (internally using MerkleNodeDB::open_read_write).
Imports & signatures various files	Remove MerkleNodeDB imports; adjust function signatures to accept session trait refs; add TODOs and trace logging in some error branches.

Sequence Diagram(s)

sequenceDiagram
  participant Migrator as Migrator
  participant OldRepo as OldRepo
  participant NewRepo as NewRepo
  participant OldSession as OldSession
  participant NewSession as NewSession

  Migrator->>OldRepo: merkle_store().begin()
  Migrator->>NewRepo: merkle_store().begin()
  OldRepo->>OldSession: create_node(commit_node)
  OldSession->>OldSession: add_child(root_dir)
  OldSession->>OldSession: finish()
  Migrator->>OldSession: rewrite_nodes(dir...)
  Migrator->>NewSession: rewrite_nodes(...)
  note right of Migrator: rewrite_nodes creates per-node namespaces
  Migrator->>NewSession: create_node(vnode)
  NewSession->>NewSession: add_child(file/dir)
  NewSession->>NewSession: finish()
  OldSession->>OldSession: create_node(dir)
  OldSession->>OldSession: add_child(vnode)
  OldSession->>OldSession: finish()
  Migrator->>OldSession: finish()
  Migrator->>NewSession: finish()

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Oxen-AI/Oxen#634 — Overlaps on CommitMerkleTree node-loading and traversal changes.
• Oxen-AI/Oxen#630 — Related changes to commit/tree node loading and storage access patterns.
• Oxen-AI/Oxen#500 — Directly overlaps entries::update_metadata traversal/write refactor.

Suggested reviewers

• rpschoenburg
• CleanCut
• gschoeni

Poem

🐇 I hopped through sessions, namespaces in line,
Began and wrote nodes, one commit at a time.
Old tree and new tree, children all placed,
Finished each namespace, no traces left misplaced. 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 41.94% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main refactoring effort: migrating Merkle tree operations to use the MerkleStore trait.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The pull request description clearly outlines the refactoring of Merkle tree operations to use new traits, specifying which files and functions are modified and how they now use MerkleStore instead of direct MerkleNodeDB calls.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch mg/merkle_dyn_use_file_backend

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

crates/lib/src/core/v_latest/index/commit_merkle_tree.rs (1)

208-208: 🏗️ Heavy lift

Avoid rebuilding the Merkle store on every existence check.

These guards are on the tree-read hot path, and repo.merkle_store() allocates a new boxed backend each time. On large traversals this adds a lot of tiny allocations for no functional gain. Construct the store once per top-level read and thread a &dyn MerkleReader / &dyn MerkleStore through the helper chain instead.

Also applies to: 233-233, 259-259, 283-283, 315-315, 344-344, 376-376

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/lib/src/core/v_latest/index/commit_merkle_tree.rs` at line 208, The
code repeatedly calls repo.merkle_store() (e.g., around the existence checks
using exists(hash) in commit_merkle_tree.rs), causing per-check allocation;
instead, obtain the Merkle store once at the start of the top-level
read/traversal and pass a borrowed trait object through the helper chain (use
&dyn MerkleReader or &dyn MerkleStore). Modify the public/top-level
traversal/read entry function to accept an extra parameter like merkle: &dyn
MerkleReader (or construct it once there via let merkle = repo.merkle_store()?;
let merkle_ref: &dyn MerkleReader = &*merkle;) and update downstream helpers to
take that borrowed trait reference rather than calling repo.merkle_store()
themselves; replace all local repo.merkle_store().exists(...) usages with
merkle_ref.exists(...) (and similarly for other Merkle calls) so no boxed
backend is reallocated on the hot path.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/lib/src/command/migrate/m20250111083535_add_child_counts_to_nodes.rs`:
- Around line 197-200: The migrated directory is being created with the wrong
parent because old_session.create_node(&dir, node.parent_id) uses the original
node's parent; change the parent argument to the current node's identifier (the
node hash) so the rewritten directory is nested under the current node.
Specifically, in the block that builds DirNode (dir.get_opts(), DirNode::new,
dir_node_opts.num_entries) replace the second parameter to
old_session.create_node with the current node's id/hash (e.g., node.id or
node.hash depending on the struct field) instead of node.parent_id.

In `@crates/lib/src/repositories/commits/commit_writer.rs`:
- Around line 804-805: The code updates dir_hash_db (put/delete) while building
nodes using session: &dyn MerkleWriteSession and commit_ns: &mut dyn
NodeWriteSession, which can leave the cache pointing at uncommitted hashes if
later create_node/session.finish() fails; change the logic to buffer all
dir_hash_db mutations (in the block around the functions that perform node
creation and the code referenced in lines 822-834) and only apply (flush) those
buffered put/delete operations after session.finish() returns Ok(), or
alternatively perform the dir_hash_db updates inside the same transactional
write path so they are rolled back if session.finish() fails; ensure you
reference and guard around session.finish() and create_node/commit_ns usages so
no dir_hash_db mutation happens before successful finish().
- Around line 509-511: This branch creates the node namespace with
session.create_node(&node, None) which differs from
commit_dir_entries_with_parents and commit_dir_entries_new; change it to pass
the previous commit id used by those paths (the same parent-id/previous-commit
value they supply) instead of None so the created commit_ns uses the same
namespace semantics; locate the call in commit_dir_entries (around where
repo.merkle_store(), store.begin()?, and let mut commit_ns =
session.create_node(&node, None)? are defined) and replace the None with the
variable or expression representing the prior commit hash used by the other
implementations.

In `@crates/lib/src/repositories/tree.rs`:
- Around line 1068-1070: In write_tree (crates/lib/src/repositories/tree.rs) the
match arm handling an unexpected node currently panics; change it to return an
Err instead: replace the panic!("p_write_tree Unexpected node type: {node:?}")
with return Err(OxenError::DisallowedNodeWrite(node.clone())); so the
Result-returning API fails gracefully on malformed/unsupported node variants
(ensure node is cloned if the error type requires ownership).

---

Nitpick comments:
In `@crates/lib/src/core/v_latest/index/commit_merkle_tree.rs`:
- Line 208: The code repeatedly calls repo.merkle_store() (e.g., around the
existence checks using exists(hash) in commit_merkle_tree.rs), causing per-check
allocation; instead, obtain the Merkle store once at the start of the top-level
read/traversal and pass a borrowed trait object through the helper chain (use
&dyn MerkleReader or &dyn MerkleStore). Modify the public/top-level
traversal/read entry function to accept an extra parameter like merkle: &dyn
MerkleReader (or construct it once there via let merkle = repo.merkle_store()?;
let merkle_ref: &dyn MerkleReader = &*merkle;) and update downstream helpers to
take that borrowed trait reference rather than calling repo.merkle_store()
themselves; replace all local repo.merkle_store().exists(...) usages with
merkle_ref.exists(...) (and similarly for other Merkle calls) so no boxed
backend is reallocated on the hot path.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e2173166-7ac4-4dfe-9b8e-b9a7fad7e26a

📥 Commits

Reviewing files that changed from the base of the PR and between 9ed02e2 and 3af1140.

📒 Files selected for processing (8)

• crates/lib/src/command/migrate/m20250111083535_add_child_counts_to_nodes.rs
• crates/lib/src/core/v_latest/commits.rs
• crates/lib/src/core/v_latest/entries.rs
• crates/lib/src/core/v_latest/index/commit_merkle_tree.rs
• crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs
• crates/lib/src/model/repository/local_repository.rs
• crates/lib/src/repositories/commits/commit_writer.rs
• crates/lib/src/repositories/tree.rs

[coderabbitai]

🧹 Nitpick comments (1)

crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs (1)

42-47: 🏗️ Heavy lift

Reuse a borrowed store across the read pipeline.

These helpers are on the inner tree-loading path, and each call now rebuilds a boxed MerkleStore before reading a single node. On large traversals that adds O(nodes) extra allocation/dyn-dispatch overhead on top of the storage I/O. Thread one borrowed reader/store through the load pipeline instead of recreating it per node.

Also applies to: 80-88

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs` around lines 42 -
47, The helper from_hash_uncached (and the similar helpers at lines ~80-88)
currently calls repo.merkle_store() inside each node read, causing repeated
boxing/allocation; change their signatures to accept a borrowed store/reader
(e.g., &impl MerkleStore or &dyn MerkleStore) as an extra parameter and thread
that borrowed reference through the tree-loading call chain so you call
store.get_node(hash)? once per node without recreating the boxed MerkleStore;
update all callers in the load pipeline to pass the same borrowed store instead
of calling repo.merkle_store() repeatedly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs`:
- Around line 42-47: The helper from_hash_uncached (and the similar helpers at
lines ~80-88) currently calls repo.merkle_store() inside each node read, causing
repeated boxing/allocation; change their signatures to accept a borrowed
store/reader (e.g., &impl MerkleStore or &dyn MerkleStore) as an extra parameter
and thread that borrowed reference through the tree-loading call chain so you
call store.get_node(hash)? once per node without recreating the boxed
MerkleStore; update all callers in the load pipeline to pass the same borrowed
store instead of calling repo.merkle_store() repeatedly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7ff56166-f113-45ae-842c-662826d31d18

📥 Commits

Reviewing files that changed from the base of the PR and between 3af1140 and 291ef08.

📒 Files selected for processing (8)

• crates/lib/src/command/migrate/m20250111083535_add_child_counts_to_nodes.rs
• crates/lib/src/core/v_latest/commits.rs
• crates/lib/src/core/v_latest/entries.rs
• crates/lib/src/core/v_latest/index/commit_merkle_tree.rs
• crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs
• crates/lib/src/model/repository/local_repository.rs
• crates/lib/src/repositories/commits/commit_writer.rs
• crates/lib/src/repositories/tree.rs

✅ Files skipped from review due to trivial changes (1)

• crates/lib/src/core/v_latest/commits.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/lib/src/core/v_latest/entries.rs

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/lib/src/repositories/commits/commit_writer.rs`:
- Around line 933-940: The code in r_create_dir_node silently continues when
CommitMerkleTree::read_node(repo, node.hash(), false) returns None, dropping
that subtree; instead, treat a missing old directory as a fatal invariant breach
and return an error to abort the commit: replace the log+continue branch with
constructing and returning an appropriate error (e.g., Err(...) or a CommitError
variant) that includes the node.hash() and context so the caller fails the
commit rather than producing a partial tree, ensuring the failure propagates out
of r_create_dir_node (or use the ? operator if converting option to Result).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 59986dce-55e0-4d56-9249-9dd00c775dec

📥 Commits

Reviewing files that changed from the base of the PR and between 291ef08 and 844e985.

📒 Files selected for processing (8)

• crates/lib/src/command/migrate/m20250111083535_add_child_counts_to_nodes.rs
• crates/lib/src/core/v_latest/commits.rs
• crates/lib/src/core/v_latest/entries.rs
• crates/lib/src/core/v_latest/index/commit_merkle_tree.rs
• crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs
• crates/lib/src/model/repository/local_repository.rs
• crates/lib/src/repositories/commits/commit_writer.rs
• crates/lib/src/repositories/tree.rs

✅ Files skipped from review due to trivial changes (2)

• crates/lib/src/model/repository/local_repository.rs
• crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• crates/lib/src/core/v_latest/commits.rs
• crates/lib/src/core/v_latest/entries.rs
• crates/lib/src/core/v_latest/index/commit_merkle_tree.rs

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/lib/src/repositories/commits/commit_writer.rs`:
- Around line 814-834: The code calls commit_ns.add_child(&dir_node) (and
similarly parent_ns.add_child(&vnode_obj) and vnode_ns.add_child(&dir_node>)
before the child's session (created via session.create_node / r_create_dir_node)
is finished, risking parents pointing at partially-written children; move the
add_child(...) calls so they occur after the child's node session is finished
(i.e., after dir_ns.finish() / vnode_ns.finish()), ensuring you create the child
with session.create_node, run r_create_dir_node, call finish() on the child's
namespace (dir_ns or vnode_ns), then add_child(...) on the parent (commit_ns,
parent_ns, or vnode_ns) and only after that update str_val_db or total_written
as needed; apply the same change pattern to the other occurrence referenced (the
vnode/parent blocks).

In `@crates/lib/src/repositories/tree.rs`:
- Around line 1057-1063: The parent is adding child entries (ns.add_child)
before the recursive write (p_write_tree) completes, which can leave parent
referencing unfinished subtrees if the recursion errors; change the order in the
EMerkleTreeNode match arms handling VNode and Directory so you call
p_write_tree(session, child, vnode/dir_node) first and only call
ns.add_child(vnode/dir_node)? after that recursive call returns successfully;
ensure the same adjustment is applied for both the VNode and Directory branches
and preserve error propagation so NodeWriteSession.finish() is only reached for
fully-written children.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7b14c970-bbc1-4939-8aef-4b853374dada

📥 Commits

Reviewing files that changed from the base of the PR and between 844e985 and 460483e.

📒 Files selected for processing (8)

• crates/lib/src/command/migrate/m20250111083535_add_child_counts_to_nodes.rs
• crates/lib/src/core/v_latest/commits.rs
• crates/lib/src/core/v_latest/entries.rs
• crates/lib/src/core/v_latest/index/commit_merkle_tree.rs
• crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs
• crates/lib/src/model/repository/local_repository.rs
• crates/lib/src/repositories/commits/commit_writer.rs
• crates/lib/src/repositories/tree.rs

✅ Files skipped from review due to trivial changes (5)

• crates/lib/src/model/repository/local_repository.rs
• crates/lib/src/core/v_latest/commits.rs
• crates/lib/src/model/merkle_tree/node/merkle_tree_node.rs
• crates/lib/src/command/migrate/m20250111083535_add_child_counts_to_nodes.rs
• crates/lib/src/core/v_latest/entries.rs

[malcolmgreaves]

NOTE: Stacked PR! Must merge #512 before merging.