fix(#4928,#4930,#4931,#4932,#4955): storage/engine robustness bundle

[lvca]

#4928: a plain IOException from flushPage escaped the per-page loop and aborted the batch: the remaining pages were never flushed or retried, yet their pageIndex entries survived, so waitAllPagesOfDatabaseAreFlushed spun forever and close()/rename()/backup-suspend hung. The failure is now contained per page with the same policy as the #4965 interrupt double-fault (SEVERE log; the page's WAL entry was never acked, so it is recovered on the next open) and the batch continues. waitAllPagesOfDatabaseAreFlushed is bounded (60s) with a SEVERE escalation instead of an infinite spin.

#4930: the ClosedChannelException reopen path could not tell 'closed by interrupt' from 'closed/dropped on purpose': a page mid-flight in the flush thread racing a DDL drop made the reopen RE-CREATE the deleted file (open() uses RandomAccessFile 'rw'), leaving a one-page ghost that FileManager re-registered on the next open. The reopen now refuses when the component was closed intentionally (open=false) or the OS file no longer exists, surfacing the condition to the caller.

#4931: check() computed RIDs with int*int arithmetic that overflows for buckets beyond 2^31 positions (a 64GB bucket at 64KB pages); with fix=true the wrong-but-positive RID deleted an innocent record. Widened to long as scan() already does; the two log sites with the same overflow (plus a string-concat-instead-of-add bug) fixed too.

#4932: deleteRecordInternal deliberately throws
ConcurrentModificationException as a retry signal for concurrently-modified multi-page chunk chains, then swallowed it 60 lines later in its own generic catch: the slot pointer was zeroed, the delete reported success, the retry never happened and the remaining NEXT_CHUNK records were orphaned (permanent space leak). The retry signal is now rethrown, mirroring the existing RecordNotFoundException clause.

#4955: getBestSlot/getRandomSlot snapshotted executorThreads without a null check, so scheduling async work concurrently with close()/kill() threw a raw NPE instead of the intended DatabaseOperationException that scheduleTask already produced for the same race.

Tests: the batch-containment + no-resurrection test is red-first verified (on unfixed code the dropped file IS re-created on disk and the exception escapes); the bounded-wait test drives the timeout deterministically; the async-slot test asserts the intended exception after kill(). #4931 (needs a 64GB bucket) and #4932 (needs a mid-delete race) are trace-verified fixes mirroring existing correct code in the same file. Regression: engine + database packages green (114 classes, 562 tests).

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Related issues

A list of issues either fixed, containing architectural discussions, otherwise relevant
for this Pull Request.

Additional Notes

Anything else we should know when reviewing?

Checklist

• I have run the build using mvn clean package command
• My unit tests cover both failure and success scenarios

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 78.26% diff coverage · -7.23% coverage variation

Metric	Results
Coverage variation	✅ -7.23% coverage variation
Diff coverage	✅ 78.26% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (d85c4db)	134450	99979	74.36%
Head commit (bb5037b)	166272 (+31822)	111621 (+11642)	67.13% (-7.23%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5012)	23	18	78.26%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

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

[gemini-code-assist]

Code Review

This pull request introduces a robustness bundle for the storage engine, addressing several critical defects: it bounds the page flush wait time to prevent infinite hangs during database close, contains per-page I/O errors to avoid aborting flush batches, prevents the recreation of deleted files during channel reopens, fixes integer overflows in RID arithmetic for large buckets, and ensures concurrent modification retry signals are not swallowed during multi-page record deletions. Additionally, it resolves a shutdown race in the async executor that could lead to a NullPointerException. Feedback on the changes suggests marking the mutable static field WAIT_FLUSH_TIMEOUT_MS as volatile to guarantee thread visibility when modified dynamically during tests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Since WAIT_FLUSH_TIMEOUT_MS is a mutable static field that is modified dynamically in tests (such as in FlushRobustnessTest.java), it should be marked volatile to ensure thread visibility. Without volatile, updates to this field made by a test thread might not be immediately visible to other threads executing waitAllPagesOfDatabaseAreFlushed, which could lead to flaky test results or incorrect timeout behavior in concurrent environments.

Suggested change

	static long WAIT_FLUSH_TIMEOUT_MS = 60_000L; // non-final and package-private for the #4928 regression test
	static volatile long WAIT_FLUSH_TIMEOUT_MS = 60_000L; // non-final and package-private for the #4928 regression test

[claude]

PR Review: storage/engine robustness bundle (#4928, #4930, #4931, #4932, #4955)

Thanks for this bundle - the fixes are well-scoped, the comments explain the "why" clearly, and the red-first tests are a nice touch. Most changes are clean wins. I have one substantive correctness concern on the #4928 bounded-wait, plus a few smaller notes.

🔴 Main concern: the "recovered from the WAL on next open" guarantee does not hold on the close() path (#4928)

Both the new bounded-wait timeout and the per-page IOException swallow rest on the same promise: an unflushed page's WAL entry was never acked, so it is replayed on the next open. That is true at runtime, but not on a clean close, and waitAllPagesOfDatabaseAreFlushed(...) is called precisely from LocalDatabase.close() (LocalDatabase.java:2062).

Trace for the timeout branch:

1. waitAllPagesOfDatabaseAreFlushed times out with pages still pending in pageIndex and returns.
2. close() continues and sets open=false (LocalDatabase.java:2067). From here PageManager.flushPage early-returns on !database.isOpen() (PageManager.java:394-397), so the pending pages are never written.
3. transactionManager.close(false) runs and, after moving all active WAL files to the inactive pool, OS-deletes every .wal file unconditionally (TransactionManager.java:154,163,167-172; with dropFiles=false the !dropFiles guard in cleanWALFiles at TransactionManager.java:815 closes the files regardless of getPendingPagesToFlush()).

Net effect: on the timeout branch the still-pending pages are lost, not recovered - the opposite of what the SEVERE log and code comment promise. The old infinite spin, however ugly, never let close() proceed with unflushed pages, so this is a real behavior change from "hang" to "silent loss of the most recent pages."

This is admittedly the pathological branch (dead flush thread / unwritable disk), and hanging forever is also bad. Two things would make it safer and honest:

• Make the timeout a GlobalConfiguration value rather than a hardcoded 60_000L. On a large DB on slow storage a legitimate flush backlog at close could exceed 60s, turning a healthy close into data loss. A config knob (defaulting high) lets operators tune it and removes the "shared mutable static poked by a test" smell (below).
• Reword the SEVERE log / comment so it does not over-promise WAL recovery on the close path, since close deletes the WAL that would do the recovering.

The same caveat applies to the per-page IOException swallow (PageManagerFlushThread.java:282-289): at runtime the WAL file is retained because getPendingPagesToFlush() > 0 blocks its drop, but the page is also never re-flushed (notifyPageFlushed never reached), so once the read cache evicts it, readers see stale on-disk content until a restart replays the WAL. This mirrors the pre-existing interrupt double-fault policy (:268-278), so it is a consistent accepted tradeoff, but worth being explicit that the page is only durable-via-WAL, not repaired in place.

🟡 Minor

• WAIT_FLUSH_TIMEOUT_MS as a mutable package-private static (PageManagerFlushThread.java:71): JVM-wide and mutated by FlushRobustnessTest in a try/finally. If any other flush-related test runs concurrently in the same JVM it would observe the 500ms value and could give up early. A GlobalConfiguration entry read per-call (or an instance field) avoids both the shared-global hazard and the test reaching into production statics.
• getRandomSlot/getBestSlot ([engine] Async getBestSlot NPE races database shutdown instead of throwing the intended DatabaseOperationException #4955): null-snapshot fix is correct and the message matches scheduleTask. Very minor: ThreadLocalRandom.nextInt(threads.length) throws IllegalArgumentException if threads.length == 0; presumably unreachable, but a comment on the non-empty invariant would help if it can ever be zero-length pre-shutdown.

✅ Looks good

• [storage] LocalBucket.check() integer overflow in RID computation for buckets > 2^31 positions #4931 widening (long) pageId * maxRecordsInPage + positionInPage is correct and matches scan(). Good catch on the two log sites too - the original ... + (getPageNumber() * maxRecordsInPage) + positionInPage string-concatenated positionInPage onto an already-stringified product (e.g. :1005 instead of :105); the fix restores numeric addition.
• [storage] deleteRecordInternal swallows the retry ConcurrentModificationException it deliberately throws (orphaned chunk chains) #4932 rethrowing ConcurrentModificationException before the generic catch (Exception) is the right ordering and mirrors the existing RecordNotFoundException clause.
• [storage] ClosedChannelException recovery in PaginatedComponentFile resurrects intentionally closed/dropped files #4930 the !open / !file.exists() guards in reopenChannelUnderWriteLock are sensible defense-in-depth; the extra File.exists() stat only runs on the rare ClosedChannelException reopen path, so no hot-path cost.
• Tests are focused, and the red-first verification for the resurrection/containment case is exactly what this kind of fix needs.

Test coverage

Well covered for #4928/#4930/#4955. Given the concern above, a test that drives LocalDatabase.close() (not just the standalone flush thread) with a page that cannot be flushed, asserting the intended durability outcome, would have surfaced the WAL-deleted-on-close interaction. #4931/#4932 are reasonably left as trace-verified given the setup cost.

Overall: solid, low-risk fixes for four of the five issues. I'd like the #4928 close-path durability semantics clarified (config-driven timeout + honest log/comment, and ideally not deleting WAL that still protects pending pages) before merge.

[codecov]

Codecov Report

❌ Patch coverage is 69.56522% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.36%. Comparing base (d85c4db) to head (bb5037b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/main/java/com/arcadedb/engine/LocalBucket.java	40.00%	3 Missing ⚠️
...dedb/database/async/DatabaseAsyncExecutorImpl.java	66.66%	1 Missing and 1 partial ⚠️
...va/com/arcadedb/engine/PaginatedComponentFile.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5012      +/-   ##
============================================
- Coverage     65.36%   65.36%   -0.01%     
- Complexity      815      816       +1     
============================================
  Files          1683     1683              
  Lines        134450   134469      +19     
  Branches      28764    28769       +5     
============================================
+ Hits          87887    87891       +4     
- Misses        34508    34528      +20     
+ Partials      12055    12050       -5     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.