scan-next: count the EOF tail's shared block as data, not freeable

The unaligned-EOF clip in block_map_fetch splits one physical block into
a reachable region (data / zero / prealloc) that ends mid-block at the
file's EOF, plus an inserted unreachable region covering the past-EOF
remainder of that same block.  The cost accounting then rounded every
region's byte span up to a whole block independently, so this single
physical block was counted twice: once as a data block (copied) and once
as an unreachable block (freed).

The phantom unreachable block corrupted two gates at once.  It inflated
good_blocks, so free-min saw 1 freed / 2 total = 50% and admitted a
rewrite that frees nothing; and it fed physical_freed = clone_alignment
into the debt, flipping a net-zero relocate into an apparent win.  A
production instance: a 4K uncompressed dst holding 2990 bytes of data
with a mid-block EOF planned as 0d1c0p1u with debt -3787 (ref-op-cost
128), when the truthful accounting is one data block, nothing freeable.

Count cost metrics in whole blocks with the reachable side owning the
shared EOF block: round data / zero / prealloc up so they claim it, and
round the unreachable remainder down so it claims nothing.  The
unaligned-EOF split is the only source of a non-block-aligned region
length, and the inserted unreachable region is always the far side of
that one boundary, so flooring it can never zero out a genuinely freeable
block-aligned region -- only the phantom tail.  Every other region is
block-aligned, where floor and ceil coincide.

The rule lives in one pure helper, scan_next_region_block_count, shared
by scan_next_plan_init and the diagnostic probe so the two can't drift;
matched-block counting already rounds up (matches are data), keeping
bad_blocks = initial_data - matched balanced at the EOF block.

Assisted-by: Claude-Code:claude-opus-4-8

diff --git a/src/bees-plan.cc b/src/bees-plan.cc
index cd1f35556e83a7a364183ef1ae742a1f202a4d36..cca41a298160ead0a3f7ca5a3bfbde5de98509e0 100644 (file)
--- a/src/bees-plan.cc
+++ b/src/bees-plan.cc
@@ -91,6 +91,17 @@ scan_next_debt_fields(uint64_t dst_ref_count, size_t frag_count, size_t copy_ops
        return c;
 }
 
+size_t
+scan_next_region_block_count(bool unreachable, uint64_t byte_count,
+       uint64_t block_size)
+{
+       // Unreachable rounds down; every other state rounds up.  See the header
+       // for why only the unaligned-EOF tail is affected.
+       return static_cast<size_t>(unreachable
+               ? byte_count / block_size
+               : (byte_count + block_size - 1) / block_size);
+}
+
 double
 scan_next_covering_debt(const PlanSearchInputs &in,
        const BeesRewritePolicy &policy,

diff --git a/src/bees-plan.h b/src/bees-plan.h
index df4ac8ed915668ac84edff42ff13ac5d8ccca7cc..0f98c91a690ee2e84eaa42b5ed76a34f5682524c 100644 (file)
--- a/src/bees-plan.h
+++ b/src/bees-plan.h
@@ -248,6 +248,21 @@ scan_next_debt_fields(uint64_t dst_ref_count, size_t frag_count, size_t copy_ops
        bool dst_compressed, uint64_t dst_phys_size,
        uint64_t good_blocks, uint64_t clone_alignment);
 
+/// Whole-block count of a block-map region's byte span for cost metrics.
+/// Rounds @p byte_count up to a whole block, except an unreachable region,
+/// which rounds down.  The unaligned-EOF clip in block_map_fetch is the only
+/// source of a non-block-aligned region length: it ends the reachable
+/// (data / zero / prealloc) region mid-block at the file's EOF and inserts an
+/// unreachable region for the past-EOF remainder of that same physical block.
+/// The reachable side rounds up and claims the shared EOF block; the
+/// unreachable remainder rounds down to zero so it is not double-counted as a
+/// freeable block (rewriting it frees nothing — the copy re-materializes an
+/// identically-shaped block).  Every other region is block-aligned, where
+/// floor and ceil coincide, so only the EOF tail is affected.
+size_t
+scan_next_region_block_count(bool unreachable, uint64_t byte_count,
+       uint64_t block_size);
+
 /// Compute the signed space-debt (do-nothing-plan.md §4) of a covering with
 /// the given operation/block counts.  Thin wrapper: marshals the search's
 /// inputs into scan_next_debt_fields() (the shared builder) and prices the

diff --git a/src/bees-scan-next.cc b/src/bees-scan-next.cc
index 2510ea9de318962f4a41fe5cab85ba54c393f7ef..f75e68a36683c16c440570755579ce2b1f57f430 100644 (file)
--- a/src/bees-scan-next.cc
+++ b/src/bees-scan-next.cc
@@ -240,11 +240,16 @@ void
 scan_next_plan_init(ChosenPlan &plan, const BeesExtent &dst)
 {
        const auto bm = dst.full_block_map(&Borrower::current().layer());
+       const auto bs = ranged_cast<uint64_t>(BLOCK_SIZE_SUMS);
        for (const auto &region : bm->m_regions) {
                const auto byte_count = region.m_end - region.m_begin;
-               const auto block_count =
-                       (byte_count + ranged_cast<uint64_t>(BLOCK_SIZE_SUMS) - 1)
-                       / ranged_cast<uint64_t>(BLOCK_SIZE_SUMS);
+               // Whole-block cost accounting.  scan_next_region_block_count
+               // rounds every state up except unreachable, so the unaligned-EOF
+               // tail (its only non-block-aligned input) does not claim a
+               // freeable block it shares with reachable data.
+               const auto block_count = scan_next_region_block_count(
+                       region.m_state == BeesTreeBlockState::unreachable,
+                       byte_count, bs);
                switch (region.m_state) {
                case BeesTreeBlockState::data:
                        plan.m_initial_data_blocks += block_count;
@@ -261,6 +266,8 @@ scan_next_plan_init(ChosenPlan &plan, const BeesExtent &dst)
                        // Pure gain: no plan-tree op charged.  The composer
                        // emits a HoleExtent for unreachable regions, but the
                        // rewrite cost model treats freeing them as costless.
+                       // A sub-block past-EOF tail floors to zero: it shares its
+                       // block with reachable data and frees nothing.
                        break;
                default:
                        break;
@@ -583,8 +590,14 @@ scan_next_log_plan(const BeesExtent &start, size_t n_candidates)
        size_t total_blocks = 0;
        const auto start_bm = start.full_block_map(&Borrower::current().layer());
        for (const auto &region : start_bm->m_regions) {
-               const auto count = ranged_cast<size_t>(
-                       (region.m_end - region.m_begin + BLOCK_SIZE_SUMS - 1) / BLOCK_SIZE_SUMS);
+               // Whole-block cost accounting, matching scan_next_plan_init: an
+               // unreachable region rounds down so a sub-block past-EOF tail is
+               // not miscounted as a freeable block it shares with reachable
+               // data; every other region rounds up.
+               const auto count = scan_next_region_block_count(
+                       region.m_state == BeesTreeBlockState::unreachable,
+                       region.m_end - region.m_begin,
+                       ranged_cast<uint64_t>(BLOCK_SIZE_SUMS));
                total_blocks += count;
                switch (region.m_state) {
                case BeesTreeBlockState::zero:

diff --git a/test/test-bees-plan.cc b/test/test-bees-plan.cc
index 25c30070218aebfb6dfb758fde62a662d200f44e..c0a56f04eebf8f4a6fd58e1ae0823bc923cc102b 100644 (file)
--- a/test/test-bees-plan.cc
+++ b/test/test-bees-plan.cc
@@ -1023,9 +1023,63 @@ test_debt_fields_shared_builder()
        }
 }
 
+// The whole-block rounding rule for cost metrics, and the production
+// regression it fixes: an unaligned-EOF block that is entirely reachable data
+// must not manufacture a freeable "unreachable" block out of its past-EOF tail.
+static void
+test_region_block_count_eof_tail()
+{
+       const uint64_t bs = 4096;
+
+       // Block-aligned lengths: floor == ceil for every state, reachable or not.
+       assert(scan_next_region_block_count(false, 0, bs) == 0);
+       assert(scan_next_region_block_count(true,  0, bs) == 0);
+       assert(scan_next_region_block_count(false, bs, bs) == 1);
+       assert(scan_next_region_block_count(true,  bs, bs) == 1);
+       assert(scan_next_region_block_count(false, 8 * bs, bs) == 8);
+       assert(scan_next_region_block_count(true,  8 * bs, bs) == 8);
+
+       // The unaligned-EOF split of one physical block: a reachable region of
+       // 2990 data bytes plus the inserted 1106-byte past-EOF unreachable tail.
+       // The reachable side rounds up and claims the block; the tail rounds down
+       // to zero.  Together they count the one physical block exactly once.
+       const uint64_t data_bytes = 2990;
+       const uint64_t tail_bytes = bs - data_bytes;   // 1106
+       assert(scan_next_region_block_count(false, data_bytes, bs) == 1);
+       assert(scan_next_region_block_count(true,  tail_bytes, bs) == 0);
+
+       // A multi-block past-EOF tail still yields only its whole blocks: the
+       // shared EOF block belongs to the data side; the rest are real holes.
+       assert(scan_next_region_block_count(true, bs + tail_bytes, bs) == 1);
+
+       // End-to-end debt.  Production ref-op-cost = 128, other costs default.
+       // The dst is one uncompressed 4K data block with a mid-block EOF, so the
+       // data block is copied (relocated) and the only candidate for good_blocks
+       // is the past-EOF tail.  Pre-fix the tail counted as 1 unreachable block
+       // -> physical_freed = 4096 -> debt = 2*128 + 53 - 4096 = -3787 (a bogus
+       // win that also cleared free-min at 50%).  Post-fix the tail is 0 blocks.
+       auto policy = default_policy();
+       policy.m_ref_op_cost = 128;
+       const uint64_t data_blocks = scan_next_region_block_count(false, data_bytes, bs);
+       const uint64_t good_blocks = scan_next_region_block_count(true, tail_bytes, bs);
+       assert(data_blocks == 1);
+       assert(good_blocks == 0);
+       const auto fixed = scan_next_debt_fields(
+               /*dst_ref_count=*/2, /*frag_count=*/1, /*copy_ops=*/1,
+               /*dst_compressed=*/false, /*dst_phys_size=*/0,
+               good_blocks, bs);
+       assert(fixed.m_physical_freed == 0);
+       assert(fixed.space_debt(policy) > 0.0);   // do-nothing (0.0) now wins
+
+       // The pre-fix miscount reproduces the exact -3787 seen in production.
+       const auto buggy = scan_next_debt_fields(2, 1, 1, false, 0, /*good=*/1, bs);
+       assert(buggy.space_debt(policy) == -3787.0);
+}
+
 int
 main(int, char **)
 {
+       RUN_A_TEST(test_region_block_count_eof_tail());
        RUN_A_TEST(test_space_debt_worked_example());
        RUN_A_TEST(test_space_debt_zero_cost_policy());
        RUN_A_TEST(test_space_debt_no_overflow());