Skip to content

fix: return error instead of silently discarding writes when storage batch is None (M3)#604

Merged
QuantumExplorer merged 1 commit into
developfrom
fix/M3-silent-storage-data-loss
Mar 8, 2026
Merged

fix: return error instead of silently discarding writes when storage batch is None (M3)#604
QuantumExplorer merged 1 commit into
developfrom
fix/M3-silent-storage-data-loss

Conversation

@QuantumExplorer
Copy link
Copy Markdown
Member

Summary

  • All 9 write operations in PrefixedRocksDbTransactionContext (put, put_aux, put_root, put_meta, delete, delete_aux, delete_root, delete_meta, commit_batch) now return Error::StorageError when self.batch is None, instead of silently returning Ok(())
  • Fixed 4 merk tests that were unknowingly relying on the silent-discard behavior (writes were silently lost) — they now provide a StorageBatch
  • Read operations continue to work correctly with None batch

Audit finding M3: All write operations silently succeeded and discarded data when no batch was provided. No legitimate code path writes through a batchless context — all batchless call sites only perform reads. The silent success made it impossible for callers to detect data loss.

Test plan

  • test_write_operations_error_when_batch_is_none — verifies all 9 write operations return errors
  • test_read_operations_succeed_when_batch_is_none — verifies reads still work with None batch
  • 4 merk tests fixed to use proper StorageBatch
  • All 2000+ workspace tests pass
  • Clippy clean

🤖 Generated with Claude Code

… None

All write operations (put, put_aux, put_root, put_meta, delete,
delete_aux, delete_root, delete_meta, commit_batch) on
PrefixedRocksDbTransactionContext previously returned Ok(()) silently
when self.batch was None, discarding the write without any indication
to the caller. This could lead to silent data loss if a transactional
context without a batch was mistakenly used for write operations.

Now these methods return Error::StorageError when batch is None,
making the failure explicit. Also fixes 4 merk tests that were
unknowingly relying on the silent-discard behavior by providing
a StorageBatch where writes are performed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 8, 2026

Warning

Rate limit exceeded

@QuantumExplorer has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 17 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.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9f8c24a5-fa74-4ecc-9909-62d3e5fb08b0

📥 Commits

Reviewing files that changed from the base of the PR and between 99a2ab0 and a49a934.

📒 Files selected for processing (4)
  • merk/src/element/get.rs
  • merk/src/merk/mod.rs
  • storage/src/rocksdb_storage/storage_context/context_tx.rs
  • storage/src/rocksdb_storage/tests.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/M3-silent-storage-data-loss

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

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 8, 2026

Codecov Report

❌ Patch coverage is 98.01980% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.69%. Comparing base (ad90e2c) to head (a49a934).
⚠️ Report is 9 commits behind head on develop.

Files with missing lines Patch % Lines
.../src/rocksdb_storage/storage_context/context_tx.rs 97.61% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #604      +/-   ##
===========================================
+ Coverage    90.63%   90.69%   +0.06%     
===========================================
  Files          182      182              
  Lines        49801    49966     +165     
===========================================
+ Hits         45136    45318     +182     
+ Misses        4665     4648      -17     
Components Coverage Δ
grovedb-core 88.78% <ø> (+0.03%) ⬆️
merk 92.06% <100.00%> (+0.11%) ⬆️
storage 86.37% <97.61%> (+0.87%) ⬆️
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.

@QuantumExplorer
Copy link
Copy Markdown
Member Author

Okay makes sense

@QuantumExplorer QuantumExplorer merged commit aa7ddff into develop Mar 8, 2026
10 checks passed
@QuantumExplorer QuantumExplorer deleted the fix/M3-silent-storage-data-loss branch March 8, 2026 14:37
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.

1 participant