Repository navigation
fix(#4939,#4941,#4956): DatabaseContext lifecycle - abandoned-tx rollback on dead-thread sweep, virtual-thread-correct liveness, thread-safe context maps - #5060
Conversation
… rolls back abandoned txs, virtual-thread-correct liveness, thread-safe context maps Three findings from the 2026-07 engine audit (tracking issue #4962), db-lifecycle group. #4941 - Dead-thread cleanup discarded transactions without rollback; explicit file locks leaked forever. A thread that died with an open transaction holding explicit locks (acquireLock().type(X).lock()) had its CONTEXTS entry pruned by the periodic sweep, dropping the only reference to explicitLockedFiles while the LockManager entries stayed owned by the dead Thread object. Every later commit touching those files failed with LockTimeoutException until restart. The sweep now rolls back the abandoned transactions (releasing the locks through reset()) before dropping the entry. The cross-thread unlock works because TransactionContext.getRequester() now captures the requester ONCE, lazily, at first use - which is always lock-acquisition time on the owning thread - instead of re-resolving Thread.currentThread() at release time (which returned the sweeping/closing thread and made the unlock throw LockException, leaking the locks). The LockManager explicitly supports acquire-on-one-thread/release-on-another for equal requesters. This also makes the pre-existing cross-thread rollback in LocalDatabase.closeInternal release foreign contexts' locks correctly. #4956 - Dead-thread detection used Thread.getAllStackTraces(): wrong for virtual threads and a safepoint hazard. getAllStackTraces() returns platform threads only, so a LIVE virtual thread's context entry (e.g. a GraphAnalyticalView build worker inside its transaction) was pruned as dead while running; combined with the #4941 fix that would have force-rolled-back a live transaction. It also captures the full stack of every platform thread at a global safepoint, injecting a multi-ms latency spike into whichever request was the 1000th init(). Each CONTEXTS entry now carries a WeakReference<Thread>; liveness is Thread.isAlive() on it - O(entries), safepoint-free, correct for both thread kinds, and the weak reference does not pin terminated threads. The three GraphAnalyticalView async tasks (build, rebuild, compaction) now call removeCurrentThreadContexts() in their finally blocks, so their thread-per-task virtual-thread contexts (each pinning Binary temp buffers) no longer accumulate until the next sweep. #4939 - Cross-thread mutation of non-thread-safe context state on close/kill. removeAllContexts() (database close/drop on another thread) mutated the per-thread plain HashMaps concurrently with the owner thread's init()/get()/put(): entries could be lost or the table corrupted. The per-thread map is now a ConcurrentHashMap, and removeAllContexts no longer prunes foreign CONTEXTS registry entries (that removal raced the owner's init() re-registration and could unregister a live context forever); empty entries are pruned by the owner or by the dead-thread sweep. The reported mid-commit interleaving (closeInternal rolling back a tx while its owner is inside commit1stPhase) is NOT reproducible on current main: commit() runs entirely under the database read lock while closeInternal's rollbacks run under the write lock of the same RWLockContext, so they cannot overlap; verdict partially-reproducible, only the map race was real. Red-first evidence (regression test DatabaseContextLifecycleTest, run against the old behavior with only the test hooks kept): - deadPlatformThreadWithExplicitLockIsSweptAndLocksReleased: FAILED with LockTimeoutException "Timeout on locking file 1 (Product_0.1.65536.v0.bucket) during commit (fileIds=[1], timeout=2000ms" (#4941) - deadVirtualThreadWithExplicitLockIsSweptAndLocksReleased: FAILED with the same LockTimeoutException (#4941/#4956) - liveVirtualThreadContextSurvivesSweep: FAILED - live virtual thread's entry pruned by the getAllStackTraces() sweep (#4956) - gavAsyncBuildUnregistersWorkerContext: FAILED - dead GAV worker's context entry lingered after awaitReady() (#4956) - concurrentInitAndRemoveAllContextsDoNotCorruptContexts: contract test for #4939; the HashMap corruption is a pure data race with no deterministic repro, so this hammers the two paths concurrently and asserts the owner never loses its context (green on both, documents the contract) All 5 tests pass with the fix. Focused suites green: com.arcadedb.database.** (102 tests incl. async), ExplicitLockingTransactionTest, LocalTransactionExplicitLockRetryTest, GraphAnalyticalViewTest (126), GraphAnalyticalViewRegistryTest.
|
Tick the box to add this pull request to the merge queue (same as
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high |
🟢 Metrics 5 complexity
Metric Results Complexity 5
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[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hardens the thread-context lifecycle in ArcadeDB by introducing a periodic dead-thread sweep that rolls back abandoned transactions, replacing Thread.getAllStackTraces() with WeakReference to support virtual threads, and using ConcurrentHashMap to prevent data races. The review feedback is highly valuable and identifies several critical concurrency and safety issues: a potential race condition in cleanupDeadThreads() when thread IDs are reused, thread-safety issues in rollbackAbandonedTransactions() during concurrent database closing, memory visibility concerns with the non-volatile requester field, and exception safety risks in GraphAnalyticalView where a failure in context removal could leak permits and hang latches.
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.
| for (final Iterator<ThreadContexts> it = CONTEXTS.values().iterator(); it.hasNext(); ) { | ||
| final ThreadContexts threadContexts = it.next(); | ||
| final Thread owner = threadContexts.owner.get(); | ||
| if (owner == null || !owner.isAlive()) { | ||
| it.remove(); | ||
| for (final DatabaseContextTL tl : threadContexts.contexts.values()) | ||
| rollbackAbandonedTransactions(tl); | ||
| } | ||
| } |
There was a problem hiding this comment.
In cleanupDeadThreads(), iterating over CONTEXTS.values() and calling it.remove() can lead to a race condition if a thread ID is reused. In Java's ConcurrentHashMap, Iterator.remove() on the values iterator removes the mapping for the key unconditionally. If a new thread has reused the same thread ID and overwritten the entry in CONTEXTS with its own ThreadContexts before it.remove() is called, the new thread's active context will be accidentally removed from the registry. This can be prevented by iterating over CONTEXTS.entrySet() and using the atomic, conditional CONTEXTS.remove(key, value) method.
for (final Map.Entry<Long, ThreadContexts> entry : CONTEXTS.entrySet()) {
final ThreadContexts threadContexts = entry.getValue();
final Thread owner = threadContexts.owner.get();
if (owner == null || !owner.isAlive()) {
if (CONTEXTS.remove(entry.getKey(), threadContexts)) {
for (final DatabaseContextTL tl : threadContexts.contexts.values())
rollbackAbandonedTransactions(tl);
}
}
}| private static void rollbackAbandonedTransactions(final DatabaseContextTL tl) { | ||
| for (int i = tl.transactions.size() - 1; i > -1; --i) { | ||
| try { | ||
| tl.transactions.get(i).rollback(); | ||
| } catch (final Throwable e) { | ||
| // IGNORE ERRORS: BEST-EFFORT CLEANUP OF TRANSACTIONS ABANDONED BY A DEAD THREAD | ||
| } | ||
| } | ||
| tl.transactions.clear(); | ||
| } |
There was a problem hiding this comment.
Since cleanupDeadThreads() runs concurrently with database closing/dropping (which calls removeAllContexts() and rolls back the returned contexts), multiple threads can concurrently access and modify the non-thread-safe ArrayList tl.transactions and call rollback() on the same TransactionContext instances. To prevent ConcurrentModificationException and race conditions during concurrent rollbacks, we should synchronize on tl and clear the transactions list atomically before rolling them back.
private static void rollbackAbandonedTransactions(final DatabaseContextTL tl) {
final List<TransactionContext> txs;
synchronized (tl) {
if (tl.transactions.isEmpty())
return;
txs = new ArrayList<>(tl.transactions);
tl.transactions.clear();
}
for (int i = txs.size() - 1; i > -1; --i) {
try {
txs.get(i).rollback();
} catch (final Throwable e) {
// IGNORE ERRORS: BEST-EFFORT CLEANUP OF TRANSACTIONS ABANDONED BY A DEAD THREAD
}
}
}| private Object getRequester() { | ||
| return requester != null ? requester : Thread.currentThread(); | ||
| // CAPTURE THE REQUESTER LAZILY AT FIRST USE, WHICH IS ALWAYS LOCK-ACQUISITION TIME ON THE OWNING THREAD. | ||
| // RESOLVING Thread.currentThread() ON EVERY CALL RETURNED THE WRONG REQUESTER WHEN THE LOCKS WERE RELEASED | ||
| // BY ANOTHER THREAD (DATABASE CLOSE ROLLING BACK FOREIGN CONTEXTS, OR THE DEAD-THREAD SWEEP ROLLING BACK AN | ||
| // ABANDONED TRANSACTION): THE UNLOCK FAILED WITH LockException AND THE FILE LOCKS LEAKED FOREVER (#4941). | ||
| // THE LockManager EXPLICITLY SUPPORTS ACQUIRE-ON-ONE-THREAD/RELEASE-ON-ANOTHER AS LONG AS THE REQUESTER | ||
| // OBJECT IS THE SAME | ||
| if (requester == null) | ||
| requester = Thread.currentThread(); | ||
| return requester; | ||
| } |
There was a problem hiding this comment.
The requester field is a non-volatile field. When getRequester() is called concurrently or across different threads (e.g., by the owner thread during lock acquisition, and later by the sweeper or close thread during rollback), there is no guarantee of memory visibility. The sweeper thread might see requester as null and overwrite it with its own thread object, causing subsequent unlocks to fail with LockException and leak file locks. Making getRequester() synchronized ensures proper memory visibility and thread-safe initialization of the requester.
| private Object getRequester() { | |
| return requester != null ? requester : Thread.currentThread(); | |
| // CAPTURE THE REQUESTER LAZILY AT FIRST USE, WHICH IS ALWAYS LOCK-ACQUISITION TIME ON THE OWNING THREAD. | |
| // RESOLVING Thread.currentThread() ON EVERY CALL RETURNED THE WRONG REQUESTER WHEN THE LOCKS WERE RELEASED | |
| // BY ANOTHER THREAD (DATABASE CLOSE ROLLING BACK FOREIGN CONTEXTS, OR THE DEAD-THREAD SWEEP ROLLING BACK AN | |
| // ABANDONED TRANSACTION): THE UNLOCK FAILED WITH LockException AND THE FILE LOCKS LEAKED FOREVER (#4941). | |
| // THE LockManager EXPLICITLY SUPPORTS ACQUIRE-ON-ONE-THREAD/RELEASE-ON-ANOTHER AS LONG AS THE REQUESTER | |
| // OBJECT IS THE SAME | |
| if (requester == null) | |
| requester = Thread.currentThread(); | |
| return requester; | |
| } | |
| private synchronized Object getRequester() { | |
| // CAPTURE THE REQUESTER LAZILY AT FIRST USE, WHICH IS ALWAYS LOCK-ACQUISITION TIME ON THE OWNING THREAD. | |
| // RESOLVING Thread.currentThread() ON EVERY CALL RETURNED THE WRONG REQUESTER WHEN THE LOCKS WERE RELEASED | |
| // BY ANOTHER THREAD (DATABASE CLOSE ROLLING BACK FOREIGN CONTEXTS, OR THE DEAD-THREAD SWEEP ROLLING BACK AN | |
| // ABANDONED TRANSACTION): THE UNLOCK FAILED WITH LockException AND THE FILE LOCKS LEAKED FOREVER (#4941). | |
| // THE LockManager EXPLICITLY SUPPORTS ACQUIRE-ON-ONE-THREAD/RELEASE-ON-ANOTHER AS LONG AS THE REQUESTER | |
| // OBJECT IS THE SAME | |
| if (requester == null) | |
| requester = Thread.currentThread(); | |
| return requester; | |
| } |
| // #4956: this virtual thread dies right after the task, but its DatabaseContext entry (pinning Binary | ||
| // temp buffers) would otherwise linger until the next periodic sweep. Remove it before the latch is | ||
| // released so callers observing readiness see a clean state. | ||
| DatabaseContext.INSTANCE.removeCurrentThreadContexts(); | ||
| BUILD_PERMITS.release(); | ||
| buildQueued.set(false); | ||
| latch.countDown(); |
There was a problem hiding this comment.
If DatabaseContext.INSTANCE.removeCurrentThreadContexts() throws any unexpected runtime exception, the subsequent critical cleanup statements (BUILD_PERMITS.release(), buildQueued.set(false), and latch.countDown()) will be skipped. This would leak the permit and permanently hang any threads waiting on the latch. To prevent this, wrap the context removal in a try-catch block.
| // #4956: this virtual thread dies right after the task, but its DatabaseContext entry (pinning Binary | |
| // temp buffers) would otherwise linger until the next periodic sweep. Remove it before the latch is | |
| // released so callers observing readiness see a clean state. | |
| DatabaseContext.INSTANCE.removeCurrentThreadContexts(); | |
| BUILD_PERMITS.release(); | |
| buildQueued.set(false); | |
| latch.countDown(); | |
| try { | |
| DatabaseContext.INSTANCE.removeCurrentThreadContexts(); | |
| } catch (final Throwable t) { | |
| LogManager.instance().log(this, Level.WARNING, "Failed to remove current thread contexts", t); | |
| } | |
| BUILD_PERMITS.release(); | |
| buildQueued.set(false); | |
| latch.countDown(); |
| // #4956: drop this worker's DatabaseContext entry (see buildAsync) | ||
| DatabaseContext.INSTANCE.removeCurrentThreadContexts(); | ||
| BUILD_PERMITS.release(); | ||
| compacting.set(false); | ||
| latch.countDown(); |
There was a problem hiding this comment.
If DatabaseContext.INSTANCE.removeCurrentThreadContexts() throws any unexpected runtime exception, the subsequent critical cleanup statements (BUILD_PERMITS.release(), compacting.set(false), and latch.countDown()) will be skipped. This would leak the permit and permanently hang any threads waiting on the latch. To prevent this, wrap the context removal in a try-catch block.
| // #4956: drop this worker's DatabaseContext entry (see buildAsync) | |
| DatabaseContext.INSTANCE.removeCurrentThreadContexts(); | |
| BUILD_PERMITS.release(); | |
| compacting.set(false); | |
| latch.countDown(); | |
| try { | |
| DatabaseContext.INSTANCE.removeCurrentThreadContexts(); | |
| } catch (final Throwable t) { | |
| LogManager.instance().log(this, Level.WARNING, "Failed to remove current thread contexts", t); | |
| } | |
| BUILD_PERMITS.release(); | |
| compacting.set(false); | |
| latch.countDown(); |
Code Review: DatabaseContext lifecycle hardening (#4939, #4941, #4956)Thorough, well-scoped work. The three fixes are logically independent but genuinely related, the code comments explain the why (not just the what), and the red-first test evidence in the PR description is the kind of rigor that makes a lifecycle/concurrency change reviewable. I verified the claims against Correctness - verified sound
Points worth a look
Minor / nits
Test coverageStrong. Red-first evidence for 4 of 5 tests, platform + virtual thread variants for the lock-leak path, a live-virtual-thread survival test, and the GAV worker-unregister assertion. The one gap (by nature, not omission) is the double-rollback race in point 1, which - like the #4939 interleaving - has no deterministic repro. Overall: solid, careful fix with good justification and coverage. The items above are refinements/acknowledgements, not blockers. |
…ead sweep rollback Second independent verification pass over the group's fix (all three findings re-confirmed against current main with the same red-first evidence: the two explicit-lock leak tests fail with LockTimeoutException, the live virtual thread's context is pruned, and the GAV worker context lingers - all on the unfixed code; 5/5 green with the fix). Delta from the review: rollbackAbandonedTransactions now calls rollback() only on ACTIVE transactions, mirroring closeInternal's pattern - every context carries at least one (usually INACTIVE) TransactionContext, and rolling those back was pointless reset() work on the sweeping thread. Also tightened a stale comment in init() and constant alignment. Focused suites re-run on this exact tree, all green: DatabaseContextLifecycleTest (5), com.arcadedb.database.** (108, 37 classes), ExplicitLockingTransactionTest (5), LocalTransactionExplicitLockRetryTest (1), com.arcadedb.graph.olap.** (208).
|
Review: DatabaseContext lifecycle hardening (issues 4939, 4941, 4956) Solid, well-reasoned PR. I traced the fix end-to-end against Correctness - verified
Minor points
Style / conventions
Test coverage Good red-first regression suite covering both thread kinds, the live-virtual-thread survival case, and GAV worker unregistration. The explicit-lock-leak tests are deterministic (explicit Nice work - the reasoning in the PR body matches what the code actually does, and the fix targets the real root cause (requester identity) rather than the symptom. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5060 +/- ##
============================================
- Coverage 65.60% 65.56% -0.04%
+ Complexity 842 841 -1
============================================
Files 1687 1687
Lines 135246 135295 +49
Branches 28941 28952 +11
============================================
- Hits 88723 88710 -13
- Misses 34415 34475 +60
- Partials 12108 12110 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Both #5060 and the merged #5061 reworked TransactionContext.getRequester for different, compatible reasons; the composition preserves both: - #4959 (#5061, merged): PUBLIC visibility, and an explicit requester set via setRequester (e.g. a server-side session) is always the lock identity - executeLockingFiles locks on the transaction's behalf. - #4941 (this PR): when NO explicit requester exists, the owning thread is captured ONCE at first use (lock-acquisition time on the owner) instead of re-resolving Thread.currentThread() per call - so a cross-thread release (dead-thread sweep, close rolling back foreign contexts) unlocks with the same requester object that acquired, and the file locks no longer leak. setRequester still wins whether it runs before or after the lazy capture (it assigns the field directly). Release-notes conflict resolved as the additive union. Union battery green: DatabaseContextLifecycleTest 5/5 (this PR's sweep and liveness tests), Issue4959TransactionCleanupsTest 3/3 (the merged requester-based locking), Issue4940Phase2FailureRollbackTest 3/3, WalCommitOrderingTest 5/5, ExplicitLockingTransactionTest 5/5, LocalTransactionExplicitLockRetryTest, TransactionIsolationContractTest 2/2.
|
Code Review: DatabaseContext lifecycle hardening (issues 4939, 4941, 4956) Thorough, well-reasoned change with excellent red-first test evidence and honest per-issue verdicts (including the "partially reproducible" call on 4939). The design is sound. Below are observations, none of them blockers. Correctness (the tricky parts hold up)
Things worth considering
Test coverage Strong. Deterministic repros for the two lock-leak cases and the live/dead virtual-thread cases, driven by direct cleanupDeadThreads() calls rather than the 1000-init counter (good, avoids flakiness). The concurrentInitAndRemoveAllContextsDoNotCorruptContexts soak test is honestly labeled as a contract test with no deterministic repro; 20k iterations is a reasonable hammer. Style / conventions
Overall: solid, defensible fix with good evidence. My only real asks are the two documentation notes (JMM isAlive() synchronizes-with rationale on the requester read, and the sweep-does-inline-rollback latency note). Nice work. |
Review round 1 responses, verified against the merged branch state: - Taken (upgraded from the reviewer's 'consider a reentrancy guard'): pure re-entrancy was already safe (the entry is unlinked before the rollback loop and the CONTEXTS traversal is weakly consistent), but verifying it surfaced a real race: two threads landing on the periodic init() boundary together both sweep, and values().iterator() gives no atomic claim, so both could roll back the same abandoned transaction (double unlock, concurrent record reloads on shared caches). cleanupDeadThreads() now claims each dead entry atomically via CONTEXTS.remove(key, value) (thread ids are never reused) and only the claimant rolls back. Red-first: the new concurrentSweepsRollBackAbandonedTransactionsExactlyOnce contract test observed a double rollback on its first run against the old code. - Taken (doc): JMM rationale on the requester read across the dead-thread boundary - the isAlive() == false check synchronizes-with the dead owner's final action (JLS 17.4.4), so the sweeper sees the non-volatile requester write; comment warns against replacing it with a bare staleness check. - Taken (doc): cleanupDeadThreads() Javadoc now states the rollbacks run inline on the triggering thread (tail-latency note for operators) and why the re-entrant path is safe. - Taken (comment accuracy): removeAllContexts() no longer over-promises - an empty entry lingers until the owner touches the context API again or dies; tiny and bounded by live-thread count. - Acknowledged, no change: lazy getRequester() seeding order at unlock sites, skip-inactive sweep optimization, and test-hook surface are as analyzed.
Review: DatabaseContext lifecycle hardening (#4939, #4941, #4956)Strong, well-reasoned PR. The root-cause analysis is excellent, the red-first regression suite is convincing, and the inline documentation (JMM rationale for the
Main concern: sweeper-vs-
|
…e rollback Review round 2 responses, verified against LocalDatabase.closeInternal: - Taken (real fix, reviewer's preferred resolution): the round-1 atomic claim covered sweeper-vs-sweeper only. closeInternal claims each tl of the closing database via removeAllContexts()'s remove(databaseName) and rolls it back under the DB write lock, while the sweep reached the same tl through threadContexts.contexts.values() with no claim and no DB lock - both paths could hold the same tl and concurrently roll back / mutate the plain ArrayList. The sweep now additionally claims each per-database context with a value-keyed remove (contexts.remove(dbKey, tl)); removeAllContexts()'s keyed remove already acts as the claim on the close side, so exactly one path wins per tl with no new locks. rollbackAbandonedTransactions() Javadoc updated: exclusivity now rests on the two-level claim, not just owner death. - Test: sweepAndDatabaseCloseRollBackAbandonedTransactionsExactlyOnce, a 100-round hammer racing cleanupDeadThreads() against a faithful closeInternal simulation (removeAllContexts + rollback of returned contexts). Honest labeling: the pre-fix window is a sub-microsecond iteration crossover (both paths traverse CONTEXTS in the same order) and never went red locally even hammered (300+ rounds); it is an exactly-once contract test - post-fix the invariant is guaranteed by construction. CountingTransaction stub factored out and shared with the round-1 test. - Taken (minor, documented rather than changed): removeContext()'s and removeCurrentThreadContexts()'s unconditional CONTEXTS.remove is safe because it runs on the owner thread, thread ids are never reused, only the owner registers under its own id and the sweep only claims dead threads; value-keying would add a CONTEXTS.get for no correctness gain. Comments added at both sites. - Acknowledged, no change: inline-rollback tail latency already documented in round 1; benign worst-case analysis of the race (LockManager tolerates the repeated release) matches ours, fixed anyway as the same defect class.
Review: DatabaseContext lifecycle hardening (#4939, #4941, #4956)Thorough, well-documented work. The three findings are real, the fixes are minimal and targeted, and the regression suite is genuinely red-first for the deterministic cases. The two-level atomic-claim ( Correctness1. The "closeInternal now releases foreign locks correctly" claim only holds for dead owners. The JMM argument documented in
This is not a regression (the old 2. Reentrant full sweep. Design / performance
TestsStrong coverage. The honesty about which tests are true red-first repros vs. exactly-once contract tests ( Style / conventions
Nothing here blocks merge. The main thing worth a second look is qualifying the closeInternal "correctly releases foreign locks" claim for the live-owner case (and deciding whether |
…tee to live-owner closes The round's JMM analysis was exact: the dead-owner path is covered by the isAlive() happens-before (JLS 17.4.4), but closeInternal can roll back a LIVE owner's context, where no such edge existed - the closer could read a stale null requester, re-capture itself and key the unlock wrong, leaking the very locks #4941 fixes. Not a regression (the old per-call Thread.currentThread() was unconditionally wrong there), but the reviewer's cheap closing move is right: requester is volatile, so a live owner's lazy capture (written at lock-acquisition time, well before any close) is visible to the closing thread. Reasoning documented on the field. Also taken: the class doc now notes that a rollback can re-enter init() and trigger a NESTED full sweep on the 1000th call - safe for the same reasons (weakly-consistent traversal + the atomic claim), documented so the recursion is expected rather than surprising. Acknowledged: the exactly-once tests are contract assertions, not race detectors - already labeled as such in round 2. Focused suites green: lifecycle 7/7, explicit locking, tx cleanups, lock retry.
Code Review - PR #5060 (DatabaseContext lifecycle hardening)Reviewed the full diff ( Correctness - looks solid
1.
|
… test, sweep-offload declined, kill() path documented Review round 4 responses, each point verified against the code first: 1. Taken (structural fix): the mutating lazy getter is split. captureRequester() (package-private, so only same-package acquisition code can capture) performs the lazy owner-thread capture and is called from exactly the two acquisition sites verified in earlier rounds: lockFilesInOrder's tryLockFiles and LocalDatabase.executeLockingFiles (which runs on the tx owner thread by construction, getTransactionIfExists is thread-local). getRequester() is now a PURE volatile read used by every release site (reset, kill, the commit unlocks, the failed-attempt unlock inside lockFilesInOrder - all post-capture by construction), falling back to Thread.currentThread() only when null, which at release time means no file lock was ever acquired, so the fallback is inert. An INVARIANT banner on the field documents the two-writer discipline (setRequester always wins, capture is owner-only) and why the non-atomic check-then-set is safe only under it. The #4959 composition is preserved unchanged. Behavior-preserving refactor guarded by the existing red-first suite (dead platform/virtual thread lock release); the residual lost-update race the old getter allowed (two concurrent first calls) needs an in-getter interleaving no test can force deterministically - the split removes it by construction. 2. Taken: gavAsyncBuildUnregistersWorkerContext no longer diffs JVM-wide id snapshots. GAV workers are named (gav-worker-N factory); the assertion is now keyed to that prefix via a new isThreadRegisteredWithNamePrefix test hook, replacing the broader registeredThreadIds hook (net test-surface reduction). Red-first: with main's GraphAnalyticalView (no unregister calls) the keyed assertion fails as expected; green with the fix. 3. Declined (async offload of the sweep rollbacks), reasoning documented in the sweep Javadoc: DatabaseAsyncExecutor is per-database and may itself be closing or saturated exactly when the sweep runs (the sweep reclaims contexts for ANY database, including ones mid-close), and queueing the rollback behind a stalled async queue delays the lock release the sweep exists to guarantee. The sweep is a rare safety net; revisit only with a dedicated JVM-wide cleanup executor if the inline cost shows in practice. 4. Verified and documented: the line-359 removeAllContexts-without-rollback site is LocalDatabase.kill(), the TEST-ONLY JVM-crash simulation (not an open-failure path). Contexts unlinked there are indeed unreachable to the sweep afterwards, but nothing can leak: file locks live in the instance's per-database TransactionManager LockManager, which transactionManager.kill() closes before the removeAllContexts call, and a reopened database starts with a fresh LockManager. Call-site comment added. No live-thread escape: a live thread's next operation on the killed database fails or re-inits against the new instance. Also fixed in passing: the sweep's JMM comment still claimed requester is non-volatile (stale since round 3); reworded to note the dead-owner guarantee does not depend on the volatile.
Code Review: PR #5060 - DatabaseContext lifecycle hardeningReviewed the diff against Strengths
Observations / minor points
Style / conventions
Overall: LGTM. The change is careful, correct as far as I can determine, and the multi-round review history is reflected in genuinely improved code rather than just comments. |
…ntee provenance Round 5 found no blocking issues. Two one-line notes taken: the compaction finally block now states why its unregister ordering deliberately differs from buildAsync/rebuild (applyDelta runs synchronously after taskCompleted() and can re-register on the same thread), and the thread-id-non-reuse comment names its provenance (HotSpot monotonic counter on Java 21+, an implementation guarantee rather than JLS, with the sweep's value-keyed removes as the backstop). Points 1, 4 and 5 were the reviewer's own confirmations of documented trade-offs. Lifecycle suite green.
|
Review: DatabaseContext lifecycle hardening (#4939, #4941, #4956) Thorough, well-reasoned PR. I traced the requester acquisition/release invariant, the two-level atomic claim in the sweep, and the virtual-thread liveness change end-to-end and the core logic holds up. The red-first test evidence and the design-rationale comments make this easy to verify. A few observations, none blocking. Strengths
Observability - worth addressing (medium) Test coverage (minor) Style (minor)
Question (low) Nice work - the fix is correct and the documentation of the concurrency reasoning is exemplary. |
…read in captureRequester The round independently confirmed the acquisition-site completeness across engine/ and server/ (the single-writer discipline is upheld, not just asserted) and found nothing blocking. Both nits taken as asked before merge: comments keep their invariant substance but drop the review-round chronology, and captureRequester reads the volatile once into a local, mirroring getRequester. Lifecycle 7/7, explicit locking, tx cleanups green.
Review: DatabaseContext lifecycle hardening (#4939, #4941, #4956)Thorough, well-reasoned PR. The three findings are distinct and each fix is tight, the concurrency reasoning is documented at the source (not just the PR body), and the regression suite is genuinely red-first for the deterministic cases (2x A few observations, mostly minor / for awareness: 1. Exactly-once rests on thread-id non-reuse (correctness dependency). 2. 3. The inline sweep rollback is JVM-wide across databases. 4. Test isolation - 5. Minor: the Nothing here is blocking. The core fixes look correct and the coverage is strong. |
…, contract tests isolated from the shared db Review round 7 responses: 1. Taken (asked twice, now actually in the code): the thread-id non-reuse guarantee the exactly-once arguments rest on is pinned at the CONTEXTS declaration - on JDK 21+ Thread.threadId() is generated at thread creation and never reused, unlike the old Thread.getId() whose contract explicitly permitted reuse after termination; do not refactor to getId() or any reusable key, re-verify on JDK major bumps. The three dependent comments (sweep keyed claim, removeContext, removeCurrentThreadContexts) now reference that single provenance note instead of repeating a bare 'never reused' assertion. 2. Taken (operability): a sweep that performs real rollback work emits ONE WARNING summarizing rolled-back transaction count, dead thread ids and databases, so the inline tail-latency blip on the triggering thread is attributable; zero-work sweeps (the common case) stay silent and allocate nothing. rollbackAbandonedTransactions now returns the active-rollback count to feed the summary. Red-first: the new sweepEmitsSingleSummaryWarningWhenItRollsBackWork test (captures the DatabaseContext JUL logger; the shared test config caps com.arcadedb at SEVERE, so the test raises that one logger to WARNING) fails against the previous code and passes with the change, covering both the silent zero-work sweep and the single-summary contract. 3. Taken (test hardening): sweepAndDatabaseCloseRollBackAbandonedTransactions- ExactlyOnce now runs its simulated close against a dedicated throwaway database instead of stripping the shared live test database's registry entry every round; concurrentInitAndRemoveAllContextsDoNotCorruptContexts now races removeAllContexts on a GENUINELY REGISTERED foreign database key (re-registered by the owner every iteration), so the same-key remove-vs-put contract on the owner's map is actually exercised - the old never-existing name only exercised the CONTEXTS iteration. 4. Acknowledged, no change: the volatile requester read on the unlock paths is an intentional tradeoff (plain load on x86/AArch64, needed for the live-owner close visibility, negligible next to the unlock itself), and the owner == null weak-ref branch's informal-but-sound reasoning stands as documented.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high |
🟢 Metrics 5 complexity
Metric Results Complexity 5
🟢 Coverage 94.29% diff coverage · -7.26% coverage variation
Metric Results Coverage variation ✅ -7.26% coverage variation Diff coverage ✅ 94.29% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (813cf4b) 135246 100868 74.58% Head commit (c3761d4) 167098 (+31852) 112500 (+11632) 67.33% (-7.26%) 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 (#5060) 70 66 94.29% 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.
Code review: DatabaseContext lifecycle (#4939 / #4941 / #4956)Reviewed the full diff against the current sources. This is a careful, well-reasoned change with excellent root-cause write-ups and strong red-first regression evidence. The core mechanics check out. A few notes below, one of which is worth acting on. What works well
Worth acting on
Minor / optional
Test coverageStrong. Red-first evidence for the deterministic findings (2x Overall: solid, safe to merge after considering item (1). Nice work. |
…immed
The round's actionable point was exact: a tx.rollback() throwing inside
rollbackAbandonedTransactions was swallowed with a bare IGNORE comment -
but a failed rollback means the abandoned transaction's file locks may
STILL be held, the precise leak this sweep exists to fix, and the operator
had no trace of it. Each failure now logs a WARNING with the cause, the
thread id and the database; the sweep continues with the remaining
transactions (best-effort preserved, silence removed).
Point 2 (threadId() spec wording weaker than 'never reused' for the two
owner-side unconditional removes) stays as flagged residual: the
provenance note carries the re-verify-on-JDK-bump instruction and the
sweep path has value-keyed defense-in-depth; adding a get-then-compare to
the owner paths would buy no correctness under the current guarantee.
Point 3: the round-history narration ('review round N') is trimmed from
the comments, invariant statements kept.
Lifecycle 8/8, explicit locking, tx cleanups green.
|
Code Review: DatabaseContext lifecycle hardening (issues 4939, 4941, 4956) Reviewed the full combined diff across the 9 commits. This is a high-quality, thoroughly reasoned change - the invariant comments, red-first regression suite, and the exactly-once claim design are exemplary. The core fixes are correct:
Points worth a second look (none blocking)
Test coverage Excellent. The red-first evidence, the exactly-once contract tests ( Overall: correct, carefully bounded, and well tested. LGTM with the two operational notes above as follow-up considerations rather than blockers. Reviewed by Claude Code |
Review: DatabaseContext lifecycle hardening (#4939, #4941, #4956)Reviewed the full diff against the current source. This is a high-quality, carefully-reasoned change: the three findings are real, the fixes are minimal and correct, and the invariants are documented at exactly the places a future maintainer would need them (the Correctness - looks solid
Minor observations
Performance
Test coverageExcellent - red-first regression suite covering platform + virtual dead-thread lock release, live-virtual-thread survival, GAV worker unregistration, both exactly-once contract races, the #4939 data-race contract, and the operability WARNING. The honesty about which tests are deterministic repros vs. contract tests is appreciated. Overall: LGTM. The |
…try, page-stats math, LSM counter drift, context prune) Four small residuals collected from the 2026-07 audit follow-up PRs (ArcadeData#5060-ArcadeData#5063), plus a file-enumeration audit reported on the issue. 1. TransactionContext.kill() lock symmetry: kill() (test-only API) released explicitLockedFiles but nulled lockedFiles WITHOUT unlocking, so killing a transaction caught in COMMIT_1ST_PHASE (where lockFilesInOrder populates lockedFiles) leaked the LockManager entries until TransactionManager.kill() closed the whole lock manager. Fix mirrors reset()'s release block, using the pure getRequester() like every other release path. Test (red-first): Issue5067KillLockSymmetryTest drives commit1stPhase(true), kills the tx and asserts a foreign requester can lock the bucket file (returned NO on timeout before the fix). 2. LocalBucket.updatePageStatistics free-space math: the threshold denominator was getPageSize() - contentHeaderSize, the same header overstatement ArcadeData#4958 fixed in gatherPageStatistics. Both now measure against the usable content region (physical size minus BasePage.PAGE_HEADER_SIZE minus the content header). Only skews the GATHER_STATS_MIN_SPACE_PERC threshold by the 8-byte header, no deterministic public observation point; covered by the existing bucket/stats suites. 3. LSMTreeIndexMutable currentMutablePages drift on rollback: createNewPage() increments the counter eagerly inside the transaction, so a rollback (e.g. a commit failed in 1st phase after applying index changes) discards the pages but not the increments; the counter drifted upward for the rest of the run (self-correcting only on reload after ArcadeData#5063's onAfterLoad fix) and inflated the auto-compaction scheduling check. onAfterCommit now clamps the counter to the real page count (the transaction's page counters are already published at that point), keeping the drift from scheduling compaction early without touching the rollback path. Test (red-first): Issue5067MutablePagesCounterDriftTest creates index pages via direct mutable-index puts in a rolled-back tx (counter 9 vs 1 real page before the fix) and asserts a subsequent commit re-aligns it. 4. DatabaseContext empty-entry retention (appended on the issue from PR ArcadeData#5060 final review): since the closing thread no longer prunes foreign CONTEXTS entries (that removal raced the owner's init() re-registration, the ArcadeData#4939 fix), an empty per-thread entry lingered for any live thread that opened+closed a database, accumulating on large long-lived pools (~500 Undertow workers) under open/close churn. The periodic sweep (invoked from init() every 1000th call) now opportunistically drops live threads' empty entries with a claim/re-check protocol: value-keyed remove of the exact entry, re-check emptiness, putIfAbsent the same entry back if the owner repopulated concurrently. Ordering argument documented inline: the owner's map.put always precedes its CONTEXTS registration check in init(), so either the re-check sees the repopulation and restores, or the owner sees the removal and re-registers; no live registration can be lost. No rollbacks run in the new branch, so the two-level claim discipline is untouched; the prune is allocation-free. Test (red-first): DatabaseContextLifecycleTest# sweepPrunesEmptyEntryOfLiveThreadAfterForeignClose, including owner re-registration after the prune. 5. Node-local file-enumeration audit (.wal.corrupt evidence files, report only): FileManager registration (extension allowlist), full backup and snapshot serving (registered ComponentFiles + explicit .ts.sealed), WAL adoption (endsWith(".wal") cannot match ".wal.corrupt"), fingerprint (suffix allowlist) and the ha-raft checksum (skip-list already extended in ArcadeData#4958) are all safe; no code changes needed. Details on the issue. Tests: the three new red-first tests plus ACIDTransactionTest, ExplicitLockingTransactionTest, LockTimeoutRetryTest, DatabaseContextLifecycleTest (9/9), Issue4511/4547 lock-leak tests, TransactionContextRemoveFileTest, ProtocolContextTest, Issue4958StorageStatsTest, LocalBucketRecordPositionTest, Issue4960 LSM counter/close tests, ConcurrentCompactionTest, MultipleDatabasesTest - all green.
…caffolding trim
Two deferred items from the audit's final review rounds:
- The invalid-MAX_PAGE_RAM ConfigurationException printed the byte value
with an 'MB' suffix (maxRAM is already multiplied by 1024*1024 when the
message is built); it now divides back so a bad value reads in the unit
it names.
- The review-round chronology left in comments across the audit-touched
files ('#NNNN review round N', 'taken after three rounds', ...) is
trimmed to plain issue references - the invariant statements stay, the
PR-history narration goes, as the final ArcadeData#5063/ArcadeData#5060/ArcadeData#5070 rounds asked.
No behavior change beyond the message unit. Lifecycle, commit-ordering,
context-lifecycle and flush-robustness suites green.
Review round 1 responses, verified against the merged branch state: - Taken (upgraded from the reviewer's 'consider a reentrancy guard'): pure re-entrancy was already safe (the entry is unlinked before the rollback loop and the CONTEXTS traversal is weakly consistent), but verifying it surfaced a real race: two threads landing on the periodic init() boundary together both sweep, and values().iterator() gives no atomic claim, so both could roll back the same abandoned transaction (double unlock, concurrent record reloads on shared caches). cleanupDeadThreads() now claims each dead entry atomically via CONTEXTS.remove(key, value) (thread ids are never reused) and only the claimant rolls back. Red-first: the new concurrentSweepsRollBackAbandonedTransactionsExactlyOnce contract test observed a double rollback on its first run against the old code. - Taken (doc): JMM rationale on the requester read across the dead-thread boundary - the isAlive() == false check synchronizes-with the dead owner's final action (JLS 17.4.4), so the sweeper sees the non-volatile requester write; comment warns against replacing it with a bare staleness check. - Taken (doc): cleanupDeadThreads() Javadoc now states the rollbacks run inline on the triggering thread (tail-latency note for operators) and why the re-entrant path is safe. - Taken (comment accuracy): removeAllContexts() no longer over-promises - an empty entry lingers until the owner touches the context API again or dies; tiny and bounded by live-thread count. - Acknowledged, no change: lazy getRequester() seeding order at unlock sites, skip-inactive sweep optimization, and test-hook surface are as analyzed. (cherry picked from commit 6f17e52)
…e rollback Review round 2 responses, verified against LocalDatabase.closeInternal: - Taken (real fix, reviewer's preferred resolution): the round-1 atomic claim covered sweeper-vs-sweeper only. closeInternal claims each tl of the closing database via removeAllContexts()'s remove(databaseName) and rolls it back under the DB write lock, while the sweep reached the same tl through threadContexts.contexts.values() with no claim and no DB lock - both paths could hold the same tl and concurrently roll back / mutate the plain ArrayList. The sweep now additionally claims each per-database context with a value-keyed remove (contexts.remove(dbKey, tl)); removeAllContexts()'s keyed remove already acts as the claim on the close side, so exactly one path wins per tl with no new locks. rollbackAbandonedTransactions() Javadoc updated: exclusivity now rests on the two-level claim, not just owner death. - Test: sweepAndDatabaseCloseRollBackAbandonedTransactionsExactlyOnce, a 100-round hammer racing cleanupDeadThreads() against a faithful closeInternal simulation (removeAllContexts + rollback of returned contexts). Honest labeling: the pre-fix window is a sub-microsecond iteration crossover (both paths traverse CONTEXTS in the same order) and never went red locally even hammered (300+ rounds); it is an exactly-once contract test - post-fix the invariant is guaranteed by construction. CountingTransaction stub factored out and shared with the round-1 test. - Taken (minor, documented rather than changed): removeContext()'s and removeCurrentThreadContexts()'s unconditional CONTEXTS.remove is safe because it runs on the owner thread, thread ids are never reused, only the owner registers under its own id and the sweep only claims dead threads; value-keying would add a CONTEXTS.get for no correctness gain. Comments added at both sites. - Acknowledged, no change: inline-rollback tail latency already documented in round 1; benign worst-case analysis of the race (LockManager tolerates the repeated release) matches ours, fixed anyway as the same defect class. (cherry picked from commit ba089ef)
…tee to live-owner closes The round's JMM analysis was exact: the dead-owner path is covered by the isAlive() happens-before (JLS 17.4.4), but closeInternal can roll back a LIVE owner's context, where no such edge existed - the closer could read a stale null requester, re-capture itself and key the unlock wrong, leaking the very locks #4941 fixes. Not a regression (the old per-call Thread.currentThread() was unconditionally wrong there), but the reviewer's cheap closing move is right: requester is volatile, so a live owner's lazy capture (written at lock-acquisition time, well before any close) is visible to the closing thread. Reasoning documented on the field. Also taken: the class doc now notes that a rollback can re-enter init() and trigger a NESTED full sweep on the 1000th call - safe for the same reasons (weakly-consistent traversal + the atomic claim), documented so the recursion is expected rather than surprising. Acknowledged: the exactly-once tests are contract assertions, not race detectors - already labeled as such in round 2. Focused suites green: lifecycle 7/7, explicit locking, tx cleanups, lock retry. (cherry picked from commit 7b2d48a)
… test, sweep-offload declined, kill() path documented Review round 4 responses, each point verified against the code first: 1. Taken (structural fix): the mutating lazy getter is split. captureRequester() (package-private, so only same-package acquisition code can capture) performs the lazy owner-thread capture and is called from exactly the two acquisition sites verified in earlier rounds: lockFilesInOrder's tryLockFiles and LocalDatabase.executeLockingFiles (which runs on the tx owner thread by construction, getTransactionIfExists is thread-local). getRequester() is now a PURE volatile read used by every release site (reset, kill, the commit unlocks, the failed-attempt unlock inside lockFilesInOrder - all post-capture by construction), falling back to Thread.currentThread() only when null, which at release time means no file lock was ever acquired, so the fallback is inert. An INVARIANT banner on the field documents the two-writer discipline (setRequester always wins, capture is owner-only) and why the non-atomic check-then-set is safe only under it. The #4959 composition is preserved unchanged. Behavior-preserving refactor guarded by the existing red-first suite (dead platform/virtual thread lock release); the residual lost-update race the old getter allowed (two concurrent first calls) needs an in-getter interleaving no test can force deterministically - the split removes it by construction. 2. Taken: gavAsyncBuildUnregistersWorkerContext no longer diffs JVM-wide id snapshots. GAV workers are named (gav-worker-N factory); the assertion is now keyed to that prefix via a new isThreadRegisteredWithNamePrefix test hook, replacing the broader registeredThreadIds hook (net test-surface reduction). Red-first: with main's GraphAnalyticalView (no unregister calls) the keyed assertion fails as expected; green with the fix. 3. Declined (async offload of the sweep rollbacks), reasoning documented in the sweep Javadoc: DatabaseAsyncExecutor is per-database and may itself be closing or saturated exactly when the sweep runs (the sweep reclaims contexts for ANY database, including ones mid-close), and queueing the rollback behind a stalled async queue delays the lock release the sweep exists to guarantee. The sweep is a rare safety net; revisit only with a dedicated JVM-wide cleanup executor if the inline cost shows in practice. 4. Verified and documented: the line-359 removeAllContexts-without-rollback site is LocalDatabase.kill(), the TEST-ONLY JVM-crash simulation (not an open-failure path). Contexts unlinked there are indeed unreachable to the sweep afterwards, but nothing can leak: file locks live in the instance's per-database TransactionManager LockManager, which transactionManager.kill() closes before the removeAllContexts call, and a reopened database starts with a fresh LockManager. Call-site comment added. No live-thread escape: a live thread's next operation on the killed database fails or re-inits against the new instance. Also fixed in passing: the sweep's JMM comment still claimed requester is non-volatile (stale since round 3); reworded to note the dead-owner guarantee does not depend on the volatile. (cherry picked from commit e1b63e9)
…ntee provenance Round 5 found no blocking issues. Two one-line notes taken: the compaction finally block now states why its unregister ordering deliberately differs from buildAsync/rebuild (applyDelta runs synchronously after taskCompleted() and can re-register on the same thread), and the thread-id-non-reuse comment names its provenance (HotSpot monotonic counter on Java 21+, an implementation guarantee rather than JLS, with the sweep's value-keyed removes as the backstop). Points 1, 4 and 5 were the reviewer's own confirmations of documented trade-offs. Lifecycle suite green. (cherry picked from commit e88ede6)
…read in captureRequester The round independently confirmed the acquisition-site completeness across engine/ and server/ (the single-writer discipline is upheld, not just asserted) and found nothing blocking. Both nits taken as asked before merge: comments keep their invariant substance but drop the review-round chronology, and captureRequester reads the volatile once into a local, mirroring getRequester. Lifecycle 7/7, explicit locking, tx cleanups green. (cherry picked from commit e2a691d)
…, contract tests isolated from the shared db Review round 7 responses: 1. Taken (asked twice, now actually in the code): the thread-id non-reuse guarantee the exactly-once arguments rest on is pinned at the CONTEXTS declaration - on JDK 21+ Thread.threadId() is generated at thread creation and never reused, unlike the old Thread.getId() whose contract explicitly permitted reuse after termination; do not refactor to getId() or any reusable key, re-verify on JDK major bumps. The three dependent comments (sweep keyed claim, removeContext, removeCurrentThreadContexts) now reference that single provenance note instead of repeating a bare 'never reused' assertion. 2. Taken (operability): a sweep that performs real rollback work emits ONE WARNING summarizing rolled-back transaction count, dead thread ids and databases, so the inline tail-latency blip on the triggering thread is attributable; zero-work sweeps (the common case) stay silent and allocate nothing. rollbackAbandonedTransactions now returns the active-rollback count to feed the summary. Red-first: the new sweepEmitsSingleSummaryWarningWhenItRollsBackWork test (captures the DatabaseContext JUL logger; the shared test config caps com.arcadedb at SEVERE, so the test raises that one logger to WARNING) fails against the previous code and passes with the change, covering both the silent zero-work sweep and the single-summary contract. 3. Taken (test hardening): sweepAndDatabaseCloseRollBackAbandonedTransactions- ExactlyOnce now runs its simulated close against a dedicated throwaway database instead of stripping the shared live test database's registry entry every round; concurrentInitAndRemoveAllContextsDoNotCorruptContexts now races removeAllContexts on a GENUINELY REGISTERED foreign database key (re-registered by the owner every iteration), so the same-key remove-vs-put contract on the owner's map is actually exercised - the old never-existing name only exercised the CONTEXTS iteration. 4. Acknowledged, no change: the volatile requester read on the unlock paths is an intentional tradeoff (plain load on x86/AArch64, needed for the live-owner close visibility, negligible next to the unlock itself), and the owner == null weak-ref branch's informal-but-sound reasoning stands as documented. (cherry picked from commit 0ba60d1)
…immed
The round's actionable point was exact: a tx.rollback() throwing inside
rollbackAbandonedTransactions was swallowed with a bare IGNORE comment -
but a failed rollback means the abandoned transaction's file locks may
STILL be held, the precise leak this sweep exists to fix, and the operator
had no trace of it. Each failure now logs a WARNING with the cause, the
thread id and the database; the sweep continues with the remaining
transactions (best-effort preserved, silence removed).
Point 2 (threadId() spec wording weaker than 'never reused' for the two
owner-side unconditional removes) stays as flagged residual: the
provenance note carries the re-verify-on-JDK-bump instruction and the
sweep path has value-keyed defense-in-depth; adding a get-then-compare to
the owner paths would buy no correctness under the current guarantee.
Point 3: the round-history narration ('review round N') is trimmed from
the comments, invariant statements kept.
Lifecycle 8/8, explicit locking, tx cleanups green.
(cherry picked from commit c3761d4)
…caffolding trim
Two deferred items from the audit's final review rounds:
- The invalid-MAX_PAGE_RAM ConfigurationException printed the byte value
with an 'MB' suffix (maxRAM is already multiplied by 1024*1024 when the
message is built); it now divides back so a bad value reads in the unit
it names.
- The review-round chronology left in comments across the audit-touched
files ('#NNNN review round N', 'taken after three rounds', ...) is
trimmed to plain issue references - the invariant statements stay, the
PR-history narration goes, as the final #5063/#5060/#5070 rounds asked.
No behavior change beyond the message unit. Lifecycle, commit-ordering,
context-lifecycle and flush-robustness suites green.
(cherry picked from commit 6c70024)
…stats math, LSM counter drift, context prune) Four small residuals collected from the 2026-07 audit follow-up PRs (#5060-#5063), plus a file-enumeration audit reported on the issue. 1. TransactionContext.kill() lock symmetry: kill() (test-only API) released explicitLockedFiles but nulled lockedFiles WITHOUT unlocking, so killing a transaction caught in COMMIT_1ST_PHASE (where lockFilesInOrder populates lockedFiles) leaked the LockManager entries until TransactionManager.kill() closed the whole lock manager. Fix mirrors reset()'s release block, using the pure getRequester() like every other release path. Test (red-first): Issue5067KillLockSymmetryTest drives commit1stPhase(true), kills the tx and asserts a foreign requester can lock the bucket file (returned NO on timeout before the fix). 2. LocalBucket.updatePageStatistics free-space math: the threshold denominator was getPageSize() - contentHeaderSize, the same header overstatement #4958 fixed in gatherPageStatistics. Both now measure against the usable content region (physical size minus BasePage.PAGE_HEADER_SIZE minus the content header). Only skews the GATHER_STATS_MIN_SPACE_PERC threshold by the 8-byte header, no deterministic public observation point; covered by the existing bucket/stats suites. 3. LSMTreeIndexMutable currentMutablePages drift on rollback: createNewPage() increments the counter eagerly inside the transaction, so a rollback (e.g. a commit failed in 1st phase after applying index changes) discards the pages but not the increments; the counter drifted upward for the rest of the run (self-correcting only on reload after #5063's onAfterLoad fix) and inflated the auto-compaction scheduling check. onAfterCommit now clamps the counter to the real page count (the transaction's page counters are already published at that point), keeping the drift from scheduling compaction early without touching the rollback path. Test (red-first): Issue5067MutablePagesCounterDriftTest creates index pages via direct mutable-index puts in a rolled-back tx (counter 9 vs 1 real page before the fix) and asserts a subsequent commit re-aligns it. 4. DatabaseContext empty-entry retention (appended on the issue from PR #5060 final review): since the closing thread no longer prunes foreign CONTEXTS entries (that removal raced the owner's init() re-registration, the #4939 fix), an empty per-thread entry lingered for any live thread that opened+closed a database, accumulating on large long-lived pools (~500 Undertow workers) under open/close churn. The periodic sweep (invoked from init() every 1000th call) now opportunistically drops live threads' empty entries with a claim/re-check protocol: value-keyed remove of the exact entry, re-check emptiness, putIfAbsent the same entry back if the owner repopulated concurrently. Ordering argument documented inline: the owner's map.put always precedes its CONTEXTS registration check in init(), so either the re-check sees the repopulation and restores, or the owner sees the removal and re-registers; no live registration can be lost. No rollbacks run in the new branch, so the two-level claim discipline is untouched; the prune is allocation-free. Test (red-first): DatabaseContextLifecycleTest# sweepPrunesEmptyEntryOfLiveThreadAfterForeignClose, including owner re-registration after the prune. 5. Node-local file-enumeration audit (.wal.corrupt evidence files, report only): FileManager registration (extension allowlist), full backup and snapshot serving (registered ComponentFiles + explicit .ts.sealed), WAL adoption (endsWith(".wal") cannot match ".wal.corrupt"), fingerprint (suffix allowlist) and the ha-raft checksum (skip-list already extended in #4958) are all safe; no code changes needed. Details on the issue. Tests: the three new red-first tests plus ACIDTransactionTest, ExplicitLockingTransactionTest, LockTimeoutRetryTest, DatabaseContextLifecycleTest (9/9), Issue4511/4547 lock-leak tests, TransactionContextRemoveFileTest, ProtocolContextTest, Issue4958StorageStatsTest, LocalBucketRecordPositionTest, Issue4960 LSM counter/close tests, ConcurrentCompactionTest, MultipleDatabasesTest - all green. (cherry picked from commit 55f3980)
Part of the 2026-07 engine audit follow-up (#4962), db-lifecycle group.
Per-issue verdicts
#4941 - explicit file locks leaked forever by dead threads: FIXED
Re-verified on current main:
cleanupDeadThreads()only pruned the CONTEXTS map, so a thread dying with an open transaction holding explicit locks left theLockManagerfiles owned by the deadThreadobject; every later commit touching them timed out until restart. The sweep now rolls back the abandoned transactions (releasing the locks viareset()). Prerequisite fix:TransactionContext.getRequester()captures the requester once at first use (always lock-acquisition time on the owner thread) instead of re-resolvingThread.currentThread()at release time - the old behavior made any cross-thread unlock (sweep,closeInternal) throwLockExceptionand leak.LockManagerexplicitly supports acquire-on-one-thread/release-on-another for equal requesters.#4956 -
getAllStackTraces()wrong for virtual threads + safepoint spike: FIXEDRe-verified: live virtual threads (e.g. GraphAnalyticalView build workers mid-transaction) were pruned as dead, and every 1000th
init()paid a global-safepoint all-stacks capture. Each CONTEXTS entry now carries aWeakReference<Thread>checked withisAlive()- O(entries), safepoint-free, correct for platform and virtual threads, no pinning of terminated threads. The three GAV async tasks (build, rebuild, compaction) now callremoveCurrentThreadContexts()in theirfinallyblocks so thread-per-task contexts no longer accumulate until the next sweep.#4939 - cross-thread mutation of non-thread-safe context state: PARTIALLY REPRODUCIBLE, the real part FIXED
The map race is real:
removeAllContexts()(close/drop on another thread) mutated per-thread plainHashMaps concurrently with the owner'sinit(). Fix: per-thread maps are nowConcurrentHashMap, andremoveAllContextsno longer prunes foreign CONTEXTS registry entries (that removal raced the owner'sinit()re-registration and could unregister a live context forever). The reported mid-commit interleaving (close rolling back a tx while its owner is insidecommit1stPhase) is NOT reproducible on current main:commit()runs entirely under the database read lock whilecloseInternal's rollbacks run under the write lock of the sameRWLockContext, so they cannot overlap. The pre-existing cross-thread rollback incloseInternalnow also releases foreign locks correctly: guaranteed for terminated owners (isAlive happens-before) and, with the volatile requester from review round 3, for live owners whose locks were acquired before the close.Test evidence (red-first)
New regression suite
DatabaseContextLifecycleTest(5 tests). Run against the old behavior (test hooks kept, behavioral hunks reverted), 4 of 5 fail exactly as the audit predicted:deadPlatformThreadWithExplicitLockIsSweptAndLocksReleased:LockTimeoutException: Timeout on locking file 1 (Product_0.1.65536.v0.bucket) during commit (fileIds=[1], timeout=2000msdeadVirtualThreadWithExplicitLockIsSweptAndLocksReleased: sameLockTimeoutExceptionliveVirtualThreadContextSurvivesSweep: live virtual thread's entry prunedgavAsyncBuildUnregistersWorkerContext: dead GAV worker's entry lingered afterawaitReady()concurrentInitAndRemoveAllContextsDoNotCorruptContexts: contract test for the [tx] Cross-thread mutation of non-thread-safe transaction state on close/kill #4939 race (pure data race, no deterministic repro - stated honestly; green on both, guards the contract)All green with the fix. Focused suites green:
com.arcadedb.database.**(102 tests incl. async),ExplicitLockingTransactionTest,LocalTransactionExplicitLockRetryTest,GraphAnalyticalViewTest(126),GraphAnalyticalViewRegistryTest.Conflict risks with sibling audit branches
docs/release-26.7.2.md(expected, every audit group appends here)engine/src/main/java/com/arcadedb/database/TransactionContext.java(10-line change togetRequester(); tx/MVCC-group branches may touch this file)engine/src/main/java/com/arcadedb/graph/olap/GraphAnalyticalView.java(3 one-linefinallyadditions)Second verification pass (2026-07-06)
The whole group was independently re-verified from scratch against current main before the follow-up commit: the regression suite was re-run red-first against the unfixed code and reproduced all four deterministic failures exactly (2x
LockTimeoutExceptionlock leak for #4941 on both platform and virtual threads, live-virtual-thread context pruned for #4956, GAV worker context leak). Follow-up commit: the sweep's rollback now skips INACTIVE transactions (mirrorscloseInternal; every context holds at least one inactiveTransactionContext, rolling those back was pointless work), plus a stale comment fix. Focused suites re-run on the final tree, all green:DatabaseContextLifecycleTest(5),com.arcadedb.database.**(108 tests, 37 classes),ExplicitLockingTransactionTest(5),LocalTransactionExplicitLockRetryTest(1),com.arcadedb.graph.olap.**(208 tests).