Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code review for PR 4325 - MutablePage.move WAL range fix. The one-line fix (Math.min for lower bound) is correct and the root cause analysis is accurate. Four items before merging: (1) Remove docs/4319-mutablepage-move-wal-tracking.md - it is an analysis doc that belongs only in the PR description, not the source tree. (2) Pre-existing: move() passes destPosition+length as exclusive end to updateModifiedRange, but the convention elsewhere (checkBoundariesOnWrite line 235) is inclusive (start+length-1). Side effects: one extra byte over-tracked in WAL (harmless), and a potential bounds-check throw if bytes are moved to the very last position in the page. Not introduced by this PR but worth a follow-up fix changing the upper bound to destPosition+length-1. (3) Comment in forwardShiftModifiedRangeStartsAtSource is ambiguous - it says 'written bytes start at destPosition but range from source' which could mislead a reader into thinking the range should start at destPosition. Suggest: 'range starts at min(start,dest)=start for forward shifts; verifies the fix does not regress forward-shift behaviour.' (4) Upper bound assertion in backwardShiftModifiedRangeCoversWrittenBytes is loose - it only checks range[1] >= destPosition+length-1 with no upper cap, so it would pass even if modifiedRangeTo were MAX_VALUE. Adding range[1] <= destPosition+length would tighten it. What is good: backwardShiftModifiedRangeStartsAtDest directly catches the regression (old code gives range[0]=108, fix gives 58), Mockito is consistent with SQLMethodSizeTest, tests are in the right package for package-private access, no new dependencies.

Code Review - PR #4325: fix(#4319): MutablePage.move mis-tracked WAL range for backward shifts

Overview

This PR fixes a data-corruption bug in MutablePage.move(): for backward shifts (destPosition < startPosition), the WAL delta was missing the byte range [destPosition, startPosition), which would be silently dropped on crash recovery. The fix is targeted, correct, and well-motivated.

Core Fix (MutablePage.java)

The change is correct. The new formula:

if (length > 0)
  updateModifiedRange(Math.min(startPosition, destPosition), Math.max(startPosition, destPosition) + length - 1);

• Correctly uses Math.min to anchor the lower bound at the first written byte regardless of direction.
• The length > 0 guard prevents an inverted range (start > end) being passed to updateModifiedRange when nothing is moved - good defensive code.

Bonus off-by-one fix: The original code used destPosition + length as the upper bound, which is exclusive-end semantics, while updateModifiedRange expects an inclusive last-byte index. The original was over-tracking by exactly 1 byte upward for forward shifts - benign but incorrect. The new + length - 1 is consistent with every other updateModifiedRange call site (e.g. checkBoundariesOnWrite uses start + length - 1). This is a correct incidental cleanup.

Conservative over-tracking for forward shifts: For dest > start, the tracked range is [startPosition, destPosition + length - 1], while only [destPosition, destPosition + length - 1] is actually written. This was also true in the original code and is harmless - the WAL includes the source region as well. Not a problem.

Test Coverage (MutablePageMoveTest.java)

Good targeted regression tests. A few observations:

1. forwardShiftModifiedRangeStartsAtSource - misleading comment: The inline comment says "written bytes start at destPosition" but the assertion checks that range[0] == source + HEADER. This is correct behavior (conservative tracking from source), but the comment contradicts what the test actually validates. Consider revising to: "range tracks from source for forward shifts (conservative over-tracking)".

2. Missing length == 0 test: The if (length > 0) guard means updateModifiedRange is skipped entirely for a zero-length move, leaving modifiedRangeFrom/modifiedRangeTo at their sentinel values. A test like:

   @Test
   void zeroLengthMoveDoesNotMarkModified() {
     final MutablePage page = freshPage();
     page.move(50, 100, 0);
     // modifiedRangeFrom stays at Integer.MAX_VALUE
     assertThat(page.getModifiedRange()[0]).isEqualTo(Integer.MAX_VALUE);
   }

   would document the intended no-op behavior.

3. RecordRecyclingTest reference: The PR description mentions this test continues to pass, but it isn't in the diff. Confirming it was run is enough - no change needed.

Documentation File (docs/4319-mutablepage-move-wal-tracking.md)

Recommend removing this file. It's an ad-hoc analysis document tied to a specific issue number. This type of write-up belongs in the PR description (where it already lives, well-written) rather than checked into the source tree permanently. Files like this tend to become stale and clutter the docs/ directory with issue-scoped content that has no audience after the fix ships.

Additionally, there is a minor inaccuracy in the doc:

"The upper bound stays destPosition + length"

The actual upper bound in the fix is Math.max(startPosition, destPosition) + length - 1, not destPosition + length. This is contradicted by the diff shown two lines later in the same doc.

Summary

The core code change is solid. The main ask before merging is dropping the docs/4319-*.md file, and optionally adding a length == 0 test and fixing the misleading comment in forwardShiftModifiedRangeStartsAtSource.

Code Review

The bug fix itself is correct and important - a crash after defragPage would replay WAL without bytes [destPosition, startPosition), silently corrupting the bucket page. Good catch.

Fix Analysis

The one-liner change in MutablePage.java:223-224 is sound:

if (length > 0)
  updateModifiedRange(Math.min(startPosition, destPosition), Math.max(startPosition, destPosition) + length - 1);

• length > 0 guard - correct; avoids passing an inverted range when length is zero (the underlying Binary.move already short-circuits for length == 0).
• Lower bound Math.min(start, dest) - correct for backward shifts; the written region starts at destPosition.
• Upper bound Math.max(start, dest) + length - 1 - fixes two bugs at once: the original used destPosition + length (exclusive, off-by-one for updateModifiedRange's inclusive semantics) and the wrong anchor for backward shifts.

The range is slightly conservative - for non-overlapping moves it includes the source region in addition to the written region. That is safe for WAL (recovery replays the wider slice, all bytes are consistent) and the simplicity of min/max is worth the minor extra WAL bytes.

Issues to Address

1. Remove docs/4319-mutablepage-move-wal-tracking.md (blocker)

This file should not be committed to the repository. It is an artifact of the AI-assisted development cycle, not maintenance documentation. Specific problems:

• The "Review cycles" section records claude bot is not configured in this repository - this is process meta-information that has no value to future readers.
• It embeds the PR URL, which becomes a dead external link in the source tree.
• The root-level docs/ directory does not appear to be the project's documentation home; merging this creates an orphaned pattern others may follow.
• The PR description and commit message already capture everything useful here.

Please drop this file from the PR.

2. Strengthen weak assertions in MutablePageMoveTest (minor)

backwardShiftModifiedRangeCoversWrittenBytes uses isLessThanOrEqualTo / isGreaterThanOrEqualTo where the exact values are deterministic:

// current - too loose, will not catch regressions that are still "in range"
assertThat(range[0]).isLessThanOrEqualTo(50 + BasePage.PAGE_HEADER_SIZE);
assertThat(range[1]).isGreaterThanOrEqualTo(50 + BasePage.PAGE_HEADER_SIZE + 60 - 1);

// preferred - exact contract, catches any drift
assertThat(range[0]).isEqualTo(50 + BasePage.PAGE_HEADER_SIZE);
assertThat(range[1]).isEqualTo(100 + BasePage.PAGE_HEADER_SIZE + 60 - 1); // Math.max(50,100)+60-1

The CLAUDE.md style guide prefers assertThat(x).isEqualTo(...) over inequality checks when the value is known.

3. Misleading test comment in forwardShiftModifiedRangeStartsAtSource (nit)

// dest(100) > start(50): forward shift — written bytes start at destPosition but range from source

The comment is correct but could confuse maintainers: the written bytes start at destPosition (100+H), not at startPosition. The tracked range conservatively starts at startPosition. Consider rewording to make the "conservative range" intent explicit:

// dest(100) > start(50): forward shift — actual writes go to [dest, dest+len-1],
// but updateModifiedRange conservatively starts at startPosition (the read region)

Summary

The fix is ready to merge once the documentation file is removed and the test assertions tightened.

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.11%. Comparing base (d1b1632) to head (aa361a3).

@@            Coverage Diff            @@
##               main    #4325   +/-   ##
=========================================
  Coverage     64.11%   64.11%           
+ Complexity      423      419    -4     
=========================================
  Files          1645     1645           
  Lines        127537   127538    +1     
  Branches      27348    27349    +1     
=========================================
+ Hits          81766    81775    +9     
- Misses        34257    34261    +4     
+ Partials      11514    11502   -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.