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.}

Review posted via file

Code Review

Overview

This PR fixes a real data-corruption bug in WAL crash recovery (fixes #4331). The root cause is well-diagnosed: checkIntegrity() passed ignoreErrors=true to applyChanges(), which caused a version gap to silently skip a single page while continuing to apply sibling pages in the same transaction - a torn transaction. All WAL files were then unconditionally dropped, making the corruption unrecoverable. The fix is conceptually correct.

Positive Aspects

• The core fix (switching to ignoreErrors=false + catching WALVersionGapException) is minimal and targeted.
• Preserving WAL files instead of dropping them on a gap is the right operational behavior.
• The lastTxId != -1 guard for transactionIds.set() silently fixes a second bug: the old code would call set(0) when no transactions were replayed (e.g., when the first WAL transaction itself has a gap), resetting the counter to 1 and risking ID reuse. Good catch.
• The null checks added to both WAL file iteration loops are correct - activeWALFilePool[i] is legitimately nullable (set to null when moved to the inactive pool).

Issues

1. Duplicate SEVERE log messages

When a gap is detected, two separate SEVERE messages are emitted: one inside the catch block (has txId) and one after the close loop (has path). Consolidate into one message to avoid operator confusion - include both the txId and the path in the catch block message, and remove the second SEVERE log after the close loop.

2. Test: database.close() after db.kill() is ambiguous

In checkIntegrityPreservesWALOnVersionGap, both db and database reference the same object. After db.kill(), calling database.close() on the already-killed instance may silently succeed or throw depending on internal state. Other TestHelper-based tests in the codebase use kill() OR close(), not both in sequence. Verify this does not cause resource leaks or spurious failures, and that the database = factory.open() reassignment at the end does not interfere with TestHelper.endTest() cleanup.

3. Tests 1 and 2 are nearly identical

applyChangesThrowsOnVersionGap and versionGapPageNotApplied build the same gapped WAL page and both assert WALVersionGapException is thrown. The second only adds a page-version-unchanged assertion. The two tests could be merged into one. Keeping both as-is adds noise without meaningful additional coverage.

4. writeGapWALFile uses hardcoded currentPageSize = 512

The injected gap WAL entry uses 512 for currentPageSize, which may not match the actual page size on disk. If applyChanges uses this field before reaching the version-gap check, the test could fail for a different reason or pass for the wrong reason. Consider using the actual page size already obtained earlier in the test (file.getPageSize()).

5. Documentation file does not belong in the repo

docs/4331-wal-version-gap-recovery.md contains implementation notes already captured in the PR description. Per project conventions, analysis and fix-explanation docs are not committed to the repository. Please remove this file.

6. Inline comments in TransactionManager.java are overly verbose

The multi-line block comments explaining the lastTxId != -1 guard and the transactionIds.set() call are longer than the code they annotate. One short line expressing the why (avoid ID reuse when no transactions were replayed) is sufficient.

Test Coverage Gap

The tests confirm WAL files are preserved after a gap but do not assert that records written before the gap are still readable after reopening the database. A post-reopen count or query assertion would confirm partial recovery is actually correct, not just that WAL files survive on disk.

Summary

The fix is sound and addresses a genuine corruption risk. Main items to address before merging:

1. Remove docs/4331-wal-version-gap-recovery.md
2. Consolidate the two duplicate SEVERE log messages into one (txId + path in the catch block)
3. Verify the kill() + close() sequence in the integration test is safe with TestHelper lifecycle
4. Use the real page size in writeGapWALFile instead of hardcoded 512

Code Review

The fix is well-targeted and the reasoning in the PR description is clear. The two transaction-ID bugs caught by the earlier Gemini review were correctly addressed. A few remaining issues:

Critical - NPE in walPositions initialization loop (incomplete null-check fix)

The Gemini review flagged missing null-checks on activeWALFilePool, and the response added null-checks to the drop and close loops. But the initialization loop right above them still dereferences without a guard:

// TransactionManager.java lines 247-251
final WALFile.WALTransaction[] walPositions = new WALFile.WALTransaction[activeWALFilePool.length];
for (int i = 0; i < activeWALFilePool.length; ++i) {
    final WALFile file = activeWALFilePool[i];
    walPositions[i] = file.getFirstTransaction();  // NPE if activeWALFilePool[i] is null
}

activeWALFilePool[i] is set to null when the constructor throws FileNotFoundException (lines 240-243), so file.getFirstTransaction() will NPE in that case. The fix needed here is the same pattern already used in the cleanup loops:

for (int i = 0; i < activeWALFilePool.length; ++i) {
    final WALFile file = activeWALFilePool[i];
    walPositions[i] = file != null ? file.getFirstTransaction() : null;
}

The main while loop already handles null entries via the walTx != null guard at line 262, so this one-liner fix is sufficient.

Minor - Two SEVERE messages are emitted for the same gap event

When a gap is detected, both the catch block (line 280) and the post-loop section (line 319) emit a SEVERE log. Operators will see two alarm-level lines referring to the same event. Fold the path info into the catch-block message and remove the second SEVERE:

} catch (WALVersionGapException e) {
    LogManager.instance().log(this, Level.SEVERE,
        "Recovery aborted for database '%s': version gap in WAL (txId=%d). WAL files preserved in '%s' for manual inspection. No further transactions will be replayed.",
        null, database, lowerTxId, database.getDatabasePath());
    walGapDetected = true;
    break;
}

Then the else-branch after the cleanup close loop can just log at FINE or be removed.

Minor - docs/4331-wal-version-gap-recovery.md should not be committed

Per project conventions, analysis and fix-explanation documents belong in the PR description, not in the repository tree. This file duplicates the PR description almost verbatim and adds permanent maintenance overhead. Please remove it.

Nit - Inline comments in TransactionManager.java are overly descriptive

The project style reserves comments for non-obvious WHY (hidden constraints, subtle invariants). Several of the new inline comments explain WHAT the code does rather than why - those should be dropped. The one explaining the lastTxId != -1 guard is borderline useful; the others (e.g. // Only advance lastTxId after a successful apply..., // Close WAL files without deleting:) describe behavior the code already expresses and add noise.

The core logic change is correct - switching to ignoreErrors=false, catching WALVersionGapException, preserving WAL files, and guarding transactionIds.set() are all the right moves. The NPE in the init loop is the only remaining correctness issue.

Code Review

Summary: This PR correctly fixes a real data-corruption bug: checkIntegrity() was calling applyChanges(ignoreErrors=true), which skipped gapped pages with continue but kept applying sibling pages of the same transaction, leaving torn transactions on disk. The core logic changes are sound.

Core logic - correct

• Moving lastTxId = lowerTxId after a successful applyChanges (instead of before) is the right fix; a failed apply must not advance the transaction counter.
• Guarding transactionIds.set(lastTxId + 1) with lastTxId != -1 is correct: without it, an all-gap recovery would reset the counter to 0 and risk transaction-id reuse against the persistedLastTxId loaded by the constructor.
• Null-checks added in the WAL cleanup loops (if (file == null) continue;) address the FileNotFoundException path in createWALFilePool().

Issues

1. WAL preservation window is shorter than the log implies (important)

checkIntegrity() closes the gap WAL files without deleting them and logs a SEVERE that says they are "preserved for manual inspection." However, close() contains an OS-level sweep that deletes every .wal file in the database directory on a clean shutdown:

// TransactionManager.java close()
else {
    File[] walFiles = dir.listFiles((dir1, name) -> name.endsWith(".wal"));
    if (walFiles != null)
        Stream.of(walFiles).forEach(File::delete);   // deletes preserved gap WALs too

The preserved files are not in inactiveWALFilePool, so cleanWALFiles returns true and the sweep runs unconditionally. An operator who restarts the database service (clean shutdown + restart) will find no WAL files to inspect, contradicting the SEVERE message.

Consider either:

• Appending to the log message: "WAL files will be removed on next clean shutdown - copy them now if manual inspection is needed."
• Or renaming them to a non-.wal extension (e.g. .wal.bad) so they survive the sweep. This also avoids re-triggering recovery on a subsequent kill+open cycle if any new WAL is written between the gap and the clean close.

2. Pre-existing null-safety gap in the early close loop (minor, but the PR is adjacent)

Lines 226-234 iterate activeWALFilePool and call file.close() without a null check:

for (final WALFile file : activeWALFilePool) {
    try {
        file.close();   // NPE if createWALFilePool() left this slot null
    } catch (final IOException e) {
        // NullPointerException is not IOException - would escape
    }
}

The PR added null checks in the two cleanup loops lower in the same method but missed this one. Worth fixing in the same pass.

3. docs/4331-wal-version-gap-recovery.md should not be committed

Per CLAUDE.md: "NEVER create documentation files (*.md) or README files unless explicitly requested." The file also contains review-cycle notes (gemini-code-assist comments, loop timeouts) that belong in the PR description, not in the repository history. Please remove it.

4. Multi-line Javadoc in the test file

CLAUDE.md style: "don't add multi-line comment blocks - one short line max." The test class has a multi-paragraph Javadoc block, and writeGapWALFile has a Javadoc with a detailed format spec. Both should be reduced to a single short comment or removed.

Minor observations

• applyChangesThrowsOnVersionGap and versionGapPageNotApplied are almost identical (same setup, same assertion, the second adds one extra page-version check). Merging them into a single test would reduce duplication without losing coverage.
• The writeGapWALFile helper hard-codes the WAL binary layout. If the format changes, the injected file will be silently unreadable (treated as empty) rather than causing a gap, so the test will stop catching the regression it was written for. A short comment pointing to WALFile.writeTransactionToBuffer as the canonical format reference would help a future reader.
• boolean walGapDetected = false; - explicit initialisation of a local variable is correct Java (locals have no default), no issue here.

Test coverage

The three new tests cover the key cases well:

• applyChangesThrowsOnVersionGap - unit: exception is thrown
• versionGapPageNotApplied - unit: on-disk version is unchanged
• checkIntegrityPreservesWALOnVersionGap - integration: WAL files survive after reopen

One missing scenario: what happens on a second open after the gap (e.g. kill, gap injected, open, kill again, open again)? Given the OS-level sweep in close() this cycle terminates on the first clean close, but a test confirming that behaviour would add confidence.

Overall this is a solid fix for a genuine corruption path. Addressing the preservation-window documentation and removing the docs file are the two changes I would consider blocking.

Codecov Report

❌ Patch coverage is 65.51724% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.37%. Comparing base (61177a8) to head (a58ea0d).

@@             Coverage Diff              @@
##               main    #4356      +/-   ##
============================================
- Coverage     64.72%   64.37%   -0.35%     
- Complexity      446      457      +11     
============================================
  Files          1647     1647              
  Lines        127719   127739      +20     
  Branches      27375    27380       +5     
============================================
- Hits          82660    82227     -433     
- Misses        33384    33945     +561     
+ Partials      11675    11567     -108     

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