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 - PR #4345: fsync data files before deleting WAL on clean close

This is a well-targeted fix for a real durability gap. The root cause analysis is accurate and the placement of syncFiles() is correct. A few observations below.

Core fix - correctness

The fix is placed correctly: inside executeInWriteLock, after waitAllPagesOfDatabaseAreFlushed, before fileManager.close() and transactionManager.close(drop). The !drop guard is also correct - there is no durability requirement when the database is being permanently removed.

Concern: silently swallowing fsync failures still deletes WAL

FileManager.syncFiles() catches IOException per-file and logs a WARNING, then the caller proceeds to transactionManager.close(drop) which deletes WAL files. If a force() call fails (disk full, I/O error), WAL files are still deleted, leaving committed data unrecoverable.

// FileManager.java - syncFiles() logs warning and continues on failure
} catch (final IOException e) {
    LogManager.instance().log(this, Level.WARNING, "Error on syncing file '%s' to disk", e, f.getFileName());
}

The fix correctly closes the gap for the normal case, but a failed fsync followed by WAL deletion is just as unrecoverable as the original bug. Consider whether the log level should be SEVERE or whether a failure should propagate to prevent WAL deletion.

Minor: null-safety inconsistency in test

noWalFilesAfterCleanClose does not guard against a null return from listFiles():

// line 54 - no null check
final File[] walFiles = new File(dbPath).listFiles((dir, name) -> name.endsWith(".wal"));
assertThat(walFiles).isEmpty(); // NPE / assertion error if dbPath is not a directory

The second test method handles this correctly with if (walFiles != null). The path comes from the running database so it should always be a valid directory, but the inconsistency is worth fixing for symmetry and to avoid confusing failures if the path ever differs:

assertThat(walFiles).as("No WAL files must remain after a clean close").isNotNull().isEmpty();

Minor: import ordering in test does not match project convention

Looking at WALFileGetTransactionTest.java in the same package, the project convention is JUnit/third-party imports first, then java.*, then static. The new test has com.arcadedb.* first, then java.*, then org.junit.*, then static, which differs from the convention used in the surrounding test files.

instanceof PaginatedComponentFile pattern note

The pattern check is a sound defensive practice, though in practice all files registered via FileManager.registerFile are already PaginatedComponentFile instances. No action needed, just an observation.

Test coverage

The two regression tests cover the intended scenarios well. dataReadableAfterReopenWithoutWALRecovery is particularly useful as a smoke test. The limitation is inherent - proving actual fsync behavior would require simulating a power cut, which is impractical in unit tests.

Summary

The fix is correct and addresses the described bug. The main actionable items are:

1. Consider propagating or logging at SEVERE when force() fails in syncFiles(), since silently continuing to delete WAL files after a failed fsync leaves committed data unrecoverable.
2. Add null check in noWalFilesAfterCleanClose for consistency with the second test method.

Codecov Report

❌ Patch coverage is 52.63158% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.29%. Comparing base (128872a) to head (b5b2285).
⚠️ Report is 1 commits behind head on main.

@@             Coverage Diff              @@
##               main    #4345      +/-   ##
============================================
- Coverage     65.37%   64.29%   -1.09%     
- Complexity        0      444     +444     
============================================
  Files          1610     1647      +37     
  Lines        123789   127678    +3889     
  Branches      26776    27365     +589     
============================================
+ Hits          80925    82085    +1160     
- Misses        31429    34017    +2588     
- Partials      11435    11576     +141     

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