Skip to content

fix(storage): correct CombinedDevice allocated() to sum inner devices - #13

Closed
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-storage-correct-combineddevice-allocated-to-su-707f85
Closed

detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-storage-correct-combineddevice-allocated-to-su-707f85

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 11, 2026

Copy link
Copy Markdown

Detail bug report: View on Detail

Bug

CombinedDevice::allocated() in foyer-storage/src/io/device/combined.rs miscounted allocated space. It treated inner.next (a device index) as a partition count by slicing inner.partitions.iter().take(inner.next), then added self.devices[inner.next].allocated(). This assumed an invariant — that inner.next equals the number of partitions created by prior devices — that was never maintained:

  • On NoSpace, create_partition advances inner.next without recording a partition.
  • On success, inner.next is not advanced per partition created.

So allocated() could under-report (a multi-partition device: take(1) sums only the first partition) or over-report (leading sub-block_size devices: partitions get double-counted via both the take slice and the devices[next] term). Over-reporting can exceed capacity(), causing the default free() (capacity() - allocated(), unsigned usize) to underflow — panicking in debug builds, wrapping in release — and can make the block-manager loop stop one block early, silently losing one block_size of cache.

Fix

Replace allocated() with the sum of each inner device's own allocated():

fn allocated(&self) -> usize {
    self.devices.iter().map(|d| d.allocated()).sum()
}

Each inner device (FileDevice, FsDevice) already tracks its own partitions accurately as the sum of partition sizes, matching the existing FileDevice/FsDevice semantics. This removes the fragile coupling between the device index and the flat partition vector and fixes all three failure directions (under-count, over-count silent loss, over-count underflow). The inner lock is no longer needed for this method; capacity() and create_partition() account for their own state independently.

Testing

Added unit tests in combined.rs using a self-contained in-memory MockDevice that mirrors FileDevice/FsDevice semantics (allocated() = sum of partition sizes; NoSpace iff allocated + size > capacity), so no filesystem is required. Each test guards a distinct failure mode of the old arithmetic and would catch a regression if allocated() were ever rewritten to reintroduce take(inner.next) logic:

  • test_combined_device_one_leading_small_silent_loss — one leading sub-block_size device (4 MiB) + a 48 MiB trailing device; asserts the loop carves all 3 blocks (not 2) via the free() >= block_size loop the block manager uses.
  • test_combined_device_two_leading_allocated_exceeds_capacity — two leading sub-block_size devices + a trailing device; after 3 allocations asserts allocated() <= capacity() (no underflow).
  • test_combined_device_multi_partition_under_count — a 3-partition device + a 1-partition device; after 4 allocations asserts allocated() == 4 * block_size (not the halved value the old take(inner.next) produced).

Verification run:

  • New combined unit tests (3): pass in both debug and --release (the release run confirms free() does not wrap).
  • cargo check, cargo clippy -p foyer-storage, cargo fmt --check, and the nightly "fastidious" format check (rustfmt.nightly.toml): all clean.
  • Regression: the existing engine::block:: suite (13 tests, including the real-FsDevice test_aggregated_device consumer) passes — no regression in the block engine paths that consume CombinedDevice.
  • Full foyer-storage lib suite (28 tests): pass.
  • Broader regression: foyer lib tests + gated storage_fuzzy_test (real-FsDevice end-to-end store paths, via --features test_utils,strict_assertions) pass.
  • Workspace-wide: cargo nextest run --all --features strict_assertions (115 pass, 1 #[ignore]d skip), cargo test --doc(4 doctests pass), andcargo clippy --all-targets` all pass.

The only warnings observed are pre-existing and unrelated to this change: a dead_code warning on store.rs::load_throttle_switch and an MSRV-toolchain clippy::chunks_exact_to_as_chunks unknown-lint note.


Automatic Fixes PRs can be configured here.

@detail-app

detail-app Bot commented Oct 12, 2026

Copy link
Copy Markdown
Author

Closing this pull request because it has had no human activity in 30 days (since 2026-09-11) and was opened 30 days ago. Detail closes its pull requests once they are more than 30 days old and have had no human activity for 14 days.

If this change is still useful, reopen the pull request and Detail won't close it again. To keep a Detail pull request open, add the detail-keep-open label to it.

@detail-app detail-app Bot closed this Oct 12, 2026
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.

0 participants