Skip to content

fix(costs): version default-section storage removal accounting#726

Open
QuantumExplorer wants to merge 2 commits into
developfrom
codex/fix-683-storage-removal-default-section
Open

fix(costs): version default-section storage removal accounting#726
QuantumExplorer wants to merge 2 commits into
developfrom
codex/fix-683-storage-removal-default-section

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

  • add a versioned storage-cost behavior for basic + sectioned removal arithmetic
  • keep v1/v2 on the legacy behavior and enable the fixed behavior in the existing latest GroveDB version (v3)
  • run the versioned storage-removal guard through Merk apply and GroveDB delete/batch paths that aggregate sectioned removals
  • add cost, version, and GroveDB-level regressions for default-section removal preservation

Fixes #683.

Verification

  • cargo test -p grovedb-costs storage_removed_bytes --test coverage_regression
  • cargo test -p grovedb-version v3_uses_fixed_basic_to_sectioned_storage_removal_addition
  • cargo test -p grovedb latest_delete_preserves_basic_plus_default_sectioned_removal_cost --lib
  • cargo check -p grovedb-merk --lib
  • cargo check -p grovedb --lib
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes
    • Corrected storage removal cost calculations in batch and delete operations.
    • Added version-aware behavior to ensure accurate calculations across GroveDB versions while maintaining backward compatibility with earlier versions.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f771d40c-28a1-4ac1-8c1a-790079d57865

📥 Commits

Reviewing files that changed from the base of the PR and between a2129a9 and 5d827ed.

📒 Files selected for processing (11)
  • costs/src/storage_cost/removal.rs
  • costs/tests/coverage_regression.rs
  • grovedb-version/src/tests.rs
  • grovedb-version/src/version/grovedb_versions.rs
  • grovedb-version/src/version/v1.rs
  • grovedb-version/src/version/v2.rs
  • grovedb-version/src/version/v3.rs
  • grovedb/src/batch/mod.rs
  • grovedb/src/batch/single_deletion_cost_tests.rs
  • grovedb/src/operations/delete/mod.rs
  • merk/src/merk/apply.rs

📝 Walkthrough

Walkthrough

This PR fixes a bug in storage removal arithmetic that dropped default-section entries when combining basic and sectioned removals. It introduces version-gated behavior using a thread-local selector: V1–V2 retain the buggy path for replay compatibility, while V3 uses corrected logic that preserves default sections. Version configuration, guards, and integration tests ensure correct behavior across all GroveDB operations.

Changes

Storage Removal Addition Versioning

Layer / File(s) Summary
Storage removal arithmetic with version selector
costs/src/storage_cost/removal.rs
Adds thread-local BASIC_SECTIONED_REMOVAL_ADDITION_VERSION with RAII BasicSectionedRemovalAdditionVersionGuard and public API. Implements add_basic_to_sectioned_removal_preserving_default_section (corrected) and legacy_add_basic_to_sectioned_removal (buggy but retained). Updates StorageRemovedBytes Add and AddAssign to route three historically-buggy arms through version gating.
Version system extension for storage costs
grovedb-version/src/version/grovedb_versions.rs, grovedb-version/src/version/v1.rs, grovedb-version/src/version/v2.rs, grovedb-version/src/version/v3.rs
Adds storage_costs: GroveDBStorageCostVersions field to GroveDBVersions. New struct contains add_basic_storage_removal_to_sectioned_storage_removal: FeatureVersion initialized to 0 for V1–V2 and 1 for V3.
Coverage test assertions for removal combinations
costs/tests/coverage_regression.rs
Updates existing assertions to verify total_removed_bytes() equality for basic + sectioned combinations with/without default sections. Adds latest_storage_removed_bytes_add_preserves_default_section test wrapping assertions in with_basic_sectioned_removal_addition_version(1, || {...}).
Version differentiation test
grovedb-version/src/tests.rs
Adds v3_uses_fixed_basic_to_sectioned_storage_removal_addition asserting version field is 0 for V1–V2 and 1 for V3.
Runtime version guard integration across operations
grovedb/src/batch/mod.rs, grovedb/src/operations/delete/mod.rs, merk/src/merk/apply.rs
Creates _storage_removal_version_guard at entry points of batch application, deletion, and tree apply functions, parameterized by grove_version.grovedb_versions.storage_costs.add_basic_storage_removal_to_sectioned_storage_removal.
End-to-end deletion cost validation
grovedb/src/batch/single_deletion_cost_tests.rs
Adds latest_delete_preserves_basic_plus_default_sectioned_removal_cost test inserting, deleting via custom removal-by-epoch closure, and asserting deletion total equals insertion total.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🐰 A rabbit hops through the removal code,
Thread-local guards now steer the load,
V3 keeps its defaults safe and sound,
While legacy paths replay, version-bound.
Each operation tested, each branch aligned—
A cleaner storage arithmetic design!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title accurately describes the main change: versioning storage removal accounting to fix default-section preservation across GroveDB versions.
Linked Issues check ✅ Passed Code changes fully address issue #683 requirements: introduces version-gating for correct basic-to-sectioned removal arithmetic, adds regression tests for all suggested scenarios (Basic+Sectioned, Sectioned+Basic, AddAssign), and implements default-section preservation.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #683: versioning storage-removal arithmetic, applying guards in merk/batch/delete paths, adding version definitions for v1/v2/v3, and implementing associated regression tests.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-683-storage-removal-default-section

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.30508% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.46%. Comparing base (60f2968) to head (5d827ed).
⚠️ Report is 5 commits behind head on develop.

Files with missing lines Patch % Lines
costs/src/storage_cost/removal.rs 96.96% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #726      +/-   ##
===========================================
+ Coverage    91.42%   91.46%   +0.03%     
===========================================
  Files          236      236              
  Lines        67053    67159     +106     
===========================================
+ Hits         61305    61427     +122     
+ Misses        5748     5732      -16     
Components Coverage Δ
grovedb-core 88.95% <100.00%> (+0.01%) ⬆️
merk 92.27% <100.00%> (+<0.01%) ⬆️
storage 86.36% <ø> (ø)
commitment-tree 96.43% <ø> (ø)
mmr 96.79% <ø> (+0.03%) ⬆️
bulk-append-tree 89.39% <ø> (+0.13%) ⬆️
element 97.38% <ø> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@QuantumExplorer QuantumExplorer force-pushed the codex/fix-683-storage-removal-default-section branch from 877c46b to 0b58b11 Compare May 21, 2026 00:42
@QuantumExplorer QuantumExplorer changed the title Preserve default section storage removals fix(costs): version default-section storage removal accounting May 21, 2026
The default-section-drop bug only ever affected three of the four
basic-into-sectioned arms (Add: Basic+Sectioned, Add: Sectioned+Basic,
AddAssign: Basic+=Sectioned). The fourth, AddAssign: Sectioned+=Basic,
reinserted the default section correctly in every shipped version. The
prior rework routed all four through the version selector, which made the
legacy/v0 path *drop* the default section for the fourth arm too —
regressing shipped v1/v2 output (its own test asserted the wrong 0).

Route Sectioned+=Basic through the always-correct helper unconditionally
and assert the preserved total (13). Document why the thread-local
selector exists and which arms it governs.

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

Copy link
Copy Markdown
Member Author

This is Claude (AI-assisted). Pushed a rework (commit 5d827ed) addressing the consensus-safety problem in the previous version.

The bug being fixed: the default-section-drop only ever affected three of the four basic-into-sectioned arms — Add: Basic+Sectioned, Add: Sectioned+Basic, and AddAssign: Basic+=Sectioned. The fourth, AddAssign: Sectioned+=Basic, already reinserted the default section correctly in every shipped version.

The prior version routed all four arms through the version selector, so the legacy/v0 path dropped the default section for the fourth arm too — i.e. it changed shipped v1/v2 output (its own regression test even asserted the wrong 0 for that case). Since storage-removal bytes feed fee/state accounting, that's a v1/v2 replay divergence.

Change:

  • Sectioned += Basic now calls the default-section-preserving helper unconditionally (it was never broken, so it must not be gated). The three genuinely-buggy arms remain gated to v3.
  • Fixed the regression test to assert the preserved total (13) on the legacy path, with a comment explaining why that arm is version-independent.
  • Documented why the thread-local selector exists (the version-sensitive combination happens inside Add/AddAssign, reached through ~hundreds of version-less StorageCost/OperationCost aggregation sites; the costs crate has no grovedb-version dep) and that its default (0/legacy) is the safe one.

On the thread-local: I looked at threading the version explicitly instead, but the combination flows through StorageCost/OperationCost operator overloads at ~951 version-less call sites, so explicit threading would require a large, risky refactor of the cost system. The thread-local is the pragmatic localization; it's now documented and the legacy default is safe.

Verified: grovedb-costs + grovedb-version tests pass (incl. v3_uses_fixed_basic_to_sectioned_storage_removal_addition and the corrected regression test); grovedb deletion-cost tests pass (incl. latest_delete_preserves_basic_plus_default_sectioned_removal_cost); clippy clean across all three.

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.

[audit][medium] Storage removal addition drops default-section removals

1 participant