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

The fix is correct and well-motivated. This is a genuine data-safety bug - silently swallowing IOException during WAL recovery could mask disk failures and lead to silent data loss. Two targeted comments below.

Production code (WALFile.java) - looks good

The read loop and the catch-block split are both correct:

• The readPos tracking variable is cleanly separated from pos (which still advances by deltaSize after the loop), avoiding an off-by-one if the loop takes multiple iterations.
• A blocking FileChannel opened from RandomAccessFile cannot return 0, so the loop terminates on either n == -1 (truncation) or a full buffer - no infinite loop risk.
• WALException is a RuntimeException, so the new throw propagates correctly through TransactionManager.checkIntegrity() (which has only a finally block) and up to LocalDatabase.checkForRecovery() where it surfaces as a database-open failure. That is the right behavior.

Residual short-read risk in readLong / readInt

The private helpers at lines 358-368 use a single channel.read(bufferLong, pos) / channel.read(bufferInt, pos) without checking the return value or looping:

private long readLong(final long pos) throws IOException {
    bufferLong.rewind();
    channel.read(bufferLong, pos);   // return value ignored - same class of bug
    return bufferLong.getLong(0);
}

For 4-8 byte reads on a local disk this is extremely unlikely to cause problems in practice (kernel page cache fills small reads atomically), but it is the same pattern that was just fixed in the delta loop. Worth a follow-up issue or a note so it does not get forgotten.

Test file - minor style issue

The class-level Javadoc is a multi-paragraph comment block:

/**
 * Unit tests for {@link WALFile#getTransaction(long)}.
 *
 * Covers:
 * 1. Valid transactions are read back with all fields intact.
 * ...
 */

The project style guide says no multi-paragraph docstrings. The information is already captured in the test method names and the PR description, so this block can be dropped entirely.

The reflection-based channel close in ioErrorOnChannelThrowsWALException is the right call - there is no other practical way to inject a channel failure without restructuring WALFile. Worth a brief note if the field is ever renamed.

Summary

The core fix is correct and the test coverage is solid (truncation at every byte is a particularly thorough touch). The two items above are minor: the residual readLong/readInt short-read issue is a pre-existing concern worth tracking separately, and the multi-line class Javadoc can be trimmed to match project style. Both are non-blocking.

Codecov Report

❌ Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 33.65%. Comparing base (c4cf514) to head (ffae26e).

@@              Coverage Diff              @@
##               main    #4338       +/-   ##
=============================================
- Coverage     64.21%   33.65%   -30.56%     
- Complexity      448      892      +444     
=============================================
  Files          1645     1645               
  Lines        127540   127550       +10     
  Branches      27350    27353        +3     
=============================================
- Hits          81899    42932    -38967     
- Misses        34102    76635    +42533     
+ Partials      11539     7983     -3556     

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

Code Review - PR #4338: WALFile.getTransaction distinguishes I/O error from truncation

Overall: The fix is correct and well-motivated. Two genuine bugs are addressed with minimal, targeted changes. Test coverage is solid. A few issues worth discussing before merging.

Bugs Fixed (Correct)

Both fixes are accurate:

1. Short-read loop - channel.read() on a NIO FileChannel can legally return fewer bytes than requested even on a local file. The loop-until-full pattern is the correct fix.
2. IOException split - Catching IOException separately and re-throwing as WALException is the right approach. The recovery loop in TransactionManager (lines 250/277) relies on null meaning "no more transactions" - swallowing a real I/O error there would silently drop committed data.

Issues

1. docs/review-deferred-acd9fdf2.md should not be in this PR

This file is an internal AI review tool artifact ("cycle 1, HEAD acd9fdf") and has no place in the repository. There is already another such file (docs/review-deferred-4eefd6c3a.md) in the repo - both should be cleaned up. These are not project documentation and will confuse future contributors.

2. readLong() and readInt() have the same short-read bug (not fixed)

// WALFile.java lines 365-375
private long readLong(final long pos) throws IOException {
  bufferLong.rewind();
  channel.read(bufferLong, pos);   // return value not checked - same bug
  return bufferLong.getLong(0);
}

private int readInt(final long pos) throws IOException {
  bufferInt.rewind();
  channel.read(bufferInt, pos);    // return value not checked - same bug
  return bufferInt.getInt(0);
}

These are called extensively in getTransaction() to read the transaction header and all page headers. A short read here would silently produce a wrong txId, wrong page count, wrong changesFrom/changesTo, etc. - the exact garbage-data-applied-to-disk problem the PR aims to fix. For small aligned reads on a local FileChannel short reads are rare in practice, but the correctness argument for fixing the delta buffer applies equally here.

3. Validation guard comment overstates the protection

// Reject obviously corrupted page headers so ByteBuffer.allocate cannot blow up with a
// negative size and a garbage delta cannot be applied to disk. The outer segment-size
// check above already bounds total memory by file size, so a per-page upper cap is not
// needed here.
if (deltaSize <= 0 || tx.pages[i].changesFrom < 0)
  return null;

The outer check at line 170 validates the declared segmentSize from the header against the file length. It does not bound individual deltaSize values within the segment. If changesTo is a garbage large value (e.g. from a partially-overwritten WAL), deltaSize could still be Integer.MAX_VALUE and ByteBuffer.allocate() would throw OutOfMemoryError (an Error, not caught by catch (Exception e)). Consider adding an upper bound: deltaSize > segmentSize is a cheap sanity check that would catch this case.

Also, changesTo < changesFrom is already covered by deltaSize <= 0, but checking changesTo < 0 is missing. It is implicitly safe only because a negative changesTo with a non-negative changesFrom would produce deltaSize <= 0. Fine, but worth noting.

4. Comment verbosity

Per project conventions, comments should only explain the non-obvious WHY. The 3-line comment on the validation guard documents what the code visibly does ("so ByteBuffer.allocate cannot blow up"). One line ("guard against negative delta from corrupted changesTo/From") is enough; the assertion in writeTransactionToBuffer at line 270 shows these are expected to be positive.

Test Coverage

Good overall. A few notes:

• truncatedWalReturnsNull creates 70 temp files. Functionally fine (runs in ~74ms), but a single file truncated with FileChannel.truncate() in a loop would be cleaner and faster.
• Reflection usage (extractChannel) is fragile - a field rename in WALFile silently breaks the test. Consider exposing a package-private closeChannelForTesting() helper, or restructuring the test to use a MockFileChannel wrapper. Given the test is in the same package (com.arcadedb.engine), package-private access is available.
• Header size constants in the test duplicate the private constants from WALFile.java. If the WAL format changes, the tests could silently become wrong. Expose the constants as package-private or document the duplication risk clearly.
• No test for readLong/readInt short reads - follows naturally from issue Bump gremlin.version from 3.4.10 to 3.5.1 #2 above.

Minor

• docs/4320-walfile-io-error-vs-truncation.md duplicates the PR description almost verbatim. The docs folder already has several per-issue files (4274-..., 4275-...) so this follows a precedent, but the PR body is a better home for this content. Up to maintainer preference.

Summary

The core fix is correct. Addressing the readLong/readInt gap and removing the review artifact would make this ready to merge.

Code Review - PR #4338: WALFile.getTransaction I/O error vs truncation fix

Overview

This PR fixes two real bugs in WALFile.getTransaction():

1. Unchecked short reads from FileChannel.read() could leave a delta buffer partially populated, silently applying garbage page content on recovery.
2. A broad catch (Exception e) { return null; } made IOException indistinguishable from clean EOF, causing recovery to silently stop mid-replay.

The fix is well-targeted and minimal. The test coverage is solid.

Issues

1. Multi-line comment violates project style (WALFile.java:203-208)

The 4-line block comment above the validation guard conflicts with the project style guide: no multi-line comment blocks - one short line max.

The guard is self-explanatory; at most a single-line note is warranted:

if (deltaSize <= 0 || tx.pages[i].changesFrom < 0)  // reject corrupted header
  return null;

2. docs/ files appear to be AI review-workflow artifacts

docs/review-deferred-acd9fdf2.md contains tooling-internal state (review cycle IDs, bot deferred-item tracking). docs/4320-walfile-io-error-vs-truncation.md largely duplicates the PR description with additional review-cycle prose. This metadata belongs in the PR itself, not in the repository. Consider dropping these files from the commit, particularly review-deferred-acd9fdf2.md.

3. Test layout constants duplicate WALFile private statics

WALFileGetTransactionTest re-declares TX_HEADER_SIZE = 24, PAGE_HEADER_SIZE = 24, TX_FOOTER_SIZE = 12 as hard-coded values mirroring WALFile's private constants. If the WAL on-disk format ever changes, these silently drift and tests could pass while testing the wrong layout.

Options:

• Widen WALFile's layout constants to package-private (same package: com.arcadedb.engine) so the test can reference them directly.
• Or make the test format-agnostic by roundtripping through WALFile.writeTransactionToBuffer rather than building the binary manually.

Nits

Read loop: potential spin on n == 0

For a blocking FileChannel on a regular file this is not a practical concern, but the NIO contract does not formally exclude a 0-byte return in non-blocking mode. A defensive if (n == 0) continue; inside the loop would make the intent explicit at zero cost.

Reflection in ioErrorOnChannelThrowsWALException

getDeclaredField("channel") is fragile if the field is ever renamed. A brief comment noting that no public API exposes the channel would help future maintainers understand why reflection is used here.

What is correct

• The read loop correctly handles partial reads and treats EOF mid-delta as truncation (returns null).
• IOException is caught before the broad Exception catch - correct ordering, and the compiler enforces it.
• deltaSize <= 0 || changesFrom < 0 correctly prevents ByteBuffer.allocate(negative), and the outer segmentSize check already bounds total memory allocation to the file size.
• AssertJ assertions and JUnit 5 style match project conventions.
• Temp files are cleaned up in finally blocks.
• No new dependencies introduced.

Summary

The core fix is correct and the test coverage is good. Three actionable items before merging:

1. Reduce the 4-line production comment to one line (or remove it).
2. Remove docs/review-deferred-acd9fdf2.md (and optionally docs/4320-walfile-io-error-vs-truncation.md) - this metadata belongs in the PR, not the repository.
3. Consider making WALFile's layout constants package-private so the test does not duplicate them with magic numbers.