fix: eliminate Owner poisoning in own_result by requiring value recovery on error

[QuantumExplorer]

Summary

• The Owner::own_result method previously consumed the inner value and left the Owner in a poisoned state (inner = None) when the closure returned Err
• While analysis confirmed this was not exploitable in the current codebase (all error paths drop the Walker without accessing the poisoned Owner), the API was inherently fragile
• Changed the closure signature to FnOnce(T) -> Result<T, (T, E)>, requiring closures to return the value alongside the error so the Owner can restore it on failure
• Updated TreeNode::attach and TreeNode::put_value to return Self in error tuples

Test plan

• owner::tests::test_own_result_success_preserves_value — verifies value preserved on success
• owner::tests::test_own_result_error_restores_value — verifies value restored on error
• owner::tests::test_own_result_error_restores_modified_value — verifies modified value returned in error is restored
• All existing merk tests pass (no breaking changes to external API)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved error handling in tree operations to return state alongside errors, enabling recovery from failures and preventing poisoning scenarios
  • Tree write operations now preserve node state when errors occur, facilitating rollback and retry capabilities

[coderabbitai]

Warning

Rate limit exceeded

@QuantumExplorer has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 59 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 38a2fadd-3d4c-4a47-8fa2-21b99e607ab4

📥 Commits

Reviewing files that changed from the base of the PR and between c1f6bfa and 0989fe0.

📒 Files selected for processing (1)

• merk/src/owner.rs

📝 Walkthrough

Walkthrough

Changes implement an error-recovery pattern across the Owner and Tree APIs, where failed operations now return owned values alongside errors instead of discarding them. The own_result method signature and related Tree attachment and put_value methods are updated to return tuples of (Self, Error) on failure, enabling callers to recover state rather than face poisoned values.

Changes

Cohort / File(s)	Summary
Owner Error Handling merk/src/owner.rs	Updated own_result to require closures returning Result<T, (T, E)>, restoring the value from the error tuple instead of leaving the owner poisoned on failure.
Tree Attachment & Put Methods merk/src/tree/mod.rs	attach now returns Result<Self, (Self, Error)> and put_value family methods return CostContext<Result<Self, (Self, Error)>>, propagating the node alongside errors to prevent state loss in traversal and insertion operations.
Clippy Linting Suppressions merk/src/tree/walk/mod.rs	Added #[allow(clippy::result_large_err)] attributes to walker methods (walk, walk_expect, attach, put_value variants) to suppress lint warnings about large error result types.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 Hops through code with gentle care,
No more poison in the air!
Errors bring their values back,
Recovery's now on the right track!
State's preserved, no loss in sight,
Owner trees will sleep tonight! 🌙

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically describes the main change: eliminating Owner poisoning by requiring value recovery on error in own_result, which is the primary focus of this PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/owner-poisoning-safety

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

❌ Patch coverage is 91.80328% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.71%. Comparing base (6285dde) to head (0989fe0).
⚠️ Report is 11 commits behind head on develop.

Files with missing lines	Patch %	Lines
merk/src/tree/mod.rs	88.37%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #621      +/-   ##
===========================================
+ Coverage    90.70%   90.71%   +0.01%     
===========================================
  Files          182      182              
  Lines        50734    50880     +146     
===========================================
+ Hits         46016    46157     +141     
- Misses        4718     4723       +5     

Components	Coverage Δ
grovedb-core	88.86% <ø> (+0.06%)	⬆️
merk	92.03% <91.80%> (-0.03%)	⬇️
storage	86.37% <ø> (ø)
commitment-tree	96.41% <ø> (ø)
mmr	96.72% <ø> (ø)
bulk-append-tree	90.85% <ø> (ø)
element	97.55% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

merk/src/tree/mod.rs (1)

893-910: ⚠️ Potential issue | 🟠 Major

Return a hash-consistent node on these new recovery paths.

These methods now expose Self in the error tuple, but the error branch runs after self.inner.kv / feature_type have been mutated and before any update_hashes* call. That means callers can recover a node whose value changed while its Merkle hashes still describe the old bytes. Either stage the KV mutation until all fallible work succeeds, or recompute the matching hashes before returning Err((self, e)). A small tuple-aware cost helper/macro would also make these four paths much harder to drift apart.

💡 Minimal consistency fix

-            if let Err(e) = self.just_in_time_tree_node_value_update(
+            if let Err(e) = self.just_in_time_tree_node_value_update(
                 old_specialized_cost,
                 get_temp_new_value_with_old_flags,
                 update_tree_value_based_on_costs,
                 section_removal_bytes,
             ) {
+                self.inner.kv = self.inner.kv.update_hashes().unwrap_add_cost(&mut cost);
                 return Err((self, e)).wrap_with_cost(cost);
             }

For the value-hash variants, use update_hashes_using_reference_value_hash(value_hash) instead.

As per coding guidelines, Use cost_return_on_error! macro for early returns with cost accumulation in Rust source files.

Also applies to: 947-966, 1003-1020, 1062-1082

🤖 Prompt for AI Agents

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

In `@merk/src/tree/mod.rs` around lines 893 - 910, The error branch returns
Err((Self, Error)) after mutating self.inner.kv via
put_value_no_update_of_hashes and setting self.inner.kv.feature_type, which can
leave the node with an updated value but stale Merkle hashes; to fix, either
stage the KV mutation until just_in_time_tree_node_value_update and other
fallible calls succeed or, if you must mutate early, recompute the matching
hashes (call the appropriate update_hashes_* helper — for value-hash variants
use update_hashes_using_reference_value_hash(value_hash)) before returning
Err((self, e)); also replace the manual Err((self, e)).wrap_with_cost(cost)
pattern with the cost_return_on_error! macro to ensure consistent cost
accumulation and reduce drift across the similar paths in
just_in_time_tree_node_value_update and the surrounding methods (apply this same
change to the other affected branches around lines 947–966, 1003–1020, and
1062–1082).

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@merk/src/owner.rs`:
- Around line 69-71: Update the doc comments for Owner methods described around
the comments (references: the Owner type and the methods whose docs at the
blocks currently saying "never left in a poisoned state") to say the API is
error-safe (returns Ok or Err((T,E)) and restores the inner value on Err) but
not unwind/panic-safe — a panic inside the user closure can still observe Owner
in a poisoned state; also adjust any panic messages that claim "never poisoned"
to avoid overstating guarantees. Modify the text at both comment locations (the
current blocks around lines ~69–71 and ~127–130) to explicitly state the limit:
restoration is guaranteed for returned Err/Ok paths but not across
panics/unwinding.

---

Outside diff comments:
In `@merk/src/tree/mod.rs`:
- Around line 893-910: The error branch returns Err((Self, Error)) after
mutating self.inner.kv via put_value_no_update_of_hashes and setting
self.inner.kv.feature_type, which can leave the node with an updated value but
stale Merkle hashes; to fix, either stage the KV mutation until
just_in_time_tree_node_value_update and other fallible calls succeed or, if you
must mutate early, recompute the matching hashes (call the appropriate
update_hashes_* helper — for value-hash variants use
update_hashes_using_reference_value_hash(value_hash)) before returning
Err((self, e)); also replace the manual Err((self, e)).wrap_with_cost(cost)
pattern with the cost_return_on_error! macro to ensure consistent cost
accumulation and reduce drift across the similar paths in
just_in_time_tree_node_value_update and the surrounding methods (apply this same
change to the other affected branches around lines 947–966, 1003–1020, and
1062–1082).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 38b29f68-b3f7-4544-8f66-fd8123a2ffc7

📥 Commits

Reviewing files that changed from the base of the PR and between 6285dde and c1f6bfa.

📒 Files selected for processing (3)

• merk/src/owner.rs
• merk/src/tree/mod.rs
• merk/src/tree/walk/mod.rs

[QuantumExplorer]

Closing: analysis confirmed this is not exploitable in the current codebase. All error paths drop the Walker without accessing the poisoned Owner. Will add documentation explaining why this is a non-issue instead.