fix(#4278): lockFilesInOrder silently continues on compaction-induced file migration

[robfrank]

Closes #4278

Summary

TransactionContext.lockFilesInOrder() silently fell through when getMigratedFileId() returned a non-null value (i.e., when a file had been migrated by LSM index compaction). This left the new mutable file unlocked during the commit, allowing concurrent transactions to race on the same index file without proper serialization.

The sibling path checkExplicitLocks() already handled this correctly by throwing ConcurrentModificationException. This PR makes lockFilesInOrder() mirror that behavior: unlock all files, rollback the transaction, emit a FINE log identifying the old and new file IDs, and throw with a migration-specific message so callers can retry.

The fix is a minimal three-line structural change: always unlock+rollback first when any file is missing, then distinguish "migrated" from "simply removed" to produce the right exception message.

Test plan

• New regression test LockFilesInOrderFileMigrationTest: creates a type with a small-page LSM index, inserts 1000 records to fill multiple mutable pages, begins a transaction that injects the pre-compaction mutable file ID into modifiedPages, runs compaction from a background thread to trigger file migration, then commits and asserts ConcurrentModificationException is thrown with a message containing "migrated".
• ConcurrentCompactionTest (3 tests) - all pass
• ExplicitLockingTransactionTest (5 tests) - all pass
• LSMTreeIndexCompactionTest (5 tests) - all pass
• Test fails on the pre-fix code (throws from checkPageVersion with "does not exist anymore", not from lockFilesInOrder with "migrated") and passes with the fix applied.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Coverage 85.71% diff coverage · -7.84% coverage variation

Metric	Results
Coverage variation	✅ -7.84% coverage variation
Diff coverage	✅ 85.71% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (26eeb40)	127474	93959	73.71%
Head commit (473fef5)	159154 (+31680)	104830 (+10871)	65.87% (-7.84%)

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 (#4279)	7	6	85.71%

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 addresses issue #4278, where TransactionContext.lockFilesInOrder() would silently continue if a locked file was migrated during LSM index compaction, potentially leading to data loss. The fix ensures that a ConcurrentModificationException is thrown when a migration is detected, providing a descriptive error message and ensuring proper rollback. The changes include the fix in TransactionContext.java, a new regression test, and documentation. Review feedback suggests using getEmbedded() instead of an explicit cast to LocalSchema for consistency.

[gemini-code-assist]

For consistency with other parts of this class (e.g., lines 143 and 665) and to avoid an explicit cast, it is preferred to use the getEmbedded() method on the schema to access LocalSchema specific methods.

Suggested change

	final Integer migrated = ((LocalSchema) database.getSchema()).getMigratedFileId(f);
	final Integer migrated = database.getSchema().getEmbedded().getMigratedFileId(f);

[claude]

Code Review

Overview

This PR fixes a genuine bug in TransactionContext.lockFilesInOrder(): when a locked file was missing due to LSM compaction-induced file migration, the original code fell through silently (leaving the new mutable file unlocked during commit). The fix restructures the block so unlock+rollback always runs first, then distinguishes the migration vs. plain-removal cases to produce the right exception. This mirrors the behavior of the sibling checkExplicitLocks() path.

The fix is correct, minimal, and well-targeted.

---

The Fix (TransactionContext.java)

Correct. Moving unlockFilesInOrder + rollback before the null-check eliminates the silent fallthrough. Both exception paths now properly release locks and roll back before throwing - callers using database.transaction() will retry automatically on ConcurrentModificationException, and direct begin()/commit() callers now receive the retryable exception they were previously denied.

Pre-existing double-rollback - the outer catch at line 709-710 also calls rollback() after catching ConcurrentModificationException. This was already the case for the "file removed without migration" branch before this PR, so it is not introduced here, and rollback() must be safe to call twice (idempotent). Still worth noting to future readers.

Minor style inconsistency - the new log call uses an explicit null Throwable:

LogManager.instance().log(this, Level.FINE, "Found upgraded file '%d' to '%d' during transaction commit", null, f, migrated);

The sibling checkExplicitLocks() call skips the Throwable parameter entirely and uses the non-Throwable overload:

LogManager.instance().log(this, Level.FINE, "Found upgraded file '%d' to '%d' during transaction lock", f, newFileId);

Both are correct (different overloads, same result), but the inconsistency is distracting. Consider dropping the explicit null to match the sibling style.

---

Test (LockFilesInOrderFileMigrationTest.java)

Good test. The approach is sound - inject a stale file ID via getPageToModify, compact from a background thread with a CountDownLatch barrier, commit on the main thread, assert the exception. This reliably exercises the exact race without depending on real concurrency timing.

Suggestion: use assertThatThrownBy for the exception assertion. Per project style (CLAUDE.md prefers AssertJ assertions), the idiomatic form is:

assertThatThrownBy(() -> database.commit())
    .isInstanceOf(ConcurrentModificationException.class)
    .hasMessageContaining("migrated");

This replaces the try/catch + null-variable pattern and is more readable.

---

Docs file (docs/4278-lockfilesinorder-silent-migration.md)

The analysis document is thorough and useful for understanding the root cause, but the project convention says documentation files should not be created unless explicitly requested. The content is already well-covered by the PR description - consider dropping this file or merging its content into the commit message instead.

---

Summary

Bug fix correctness	Correct - silent fallthrough eliminated
Behavior parity with sibling	Achieved
Test coverage	Good regression test, reliably reproducible
Minor issues	Log style inconsistency; exception assertion style; docs file convention

The core change is solid. The two small suggestions (log style + assertThatThrownBy) are optional polish. Ready to merge after addressing (or knowingly skipping) those points.

[claude]

Code Review - PR #4279: lockFilesInOrder silently continues on compaction-induced file migration

Summary

This PR fixes a real correctness bug in TransactionContext.lockFilesInOrder(). Before the fix, when getMigratedFileId() returned a non-null value (meaning a file had been migrated by LSM index compaction), the method fell through silently without locking the new mutable file, potentially allowing concurrent transactions to race on the same index file without proper serialization. The fix makes lockFilesInOrder() mirror the behavior of checkExplicitLocks(): unlock all files, rollback the transaction, log at FINE, and throw ConcurrentModificationException with a descriptive message.

Positive aspects

• The fix is minimal and precisely targeted.
• The unlock+rollback happens unconditionally before the migration check, which is correct. Previously the "no migration" branch did unlock+rollback+throw while the "has migration" branch fell through silently. The reordering is clean.
• The exception message is descriptive and actionable, naming both the old and new file IDs.
• The new test correctly exercises the actual code path by injecting the old mutable file ID into modifiedPages using the real getPageToModify API, rather than mocking.
• Using a CountDownLatch and AtomicReference<Throwable> for the background compaction thread is idiomatic and correct.
• The pre/post compaction state assertions (migration entry present, old file gone) serve as useful intermediate checkpoints.
• TestHelper.afterTest() safely handles the rolled-back transaction: since lockFilesInOrder calls rollback() before throwing, the database.commit() guard in afterTest() is not triggered.
• License header is present and correct. Follows final keyword conventions on parameters and local variables.

Issues found

1. Em dash in an error message (test). The project style guide says not to use the em dash character (-). The test uses it in an AssertionError message:

compactionError.set(new AssertionError("compact() returned false — mutable must have >= 2 pages"));

Use a regular dash or a comma instead.

2. Inconsistent LocalSchema access style. The fix in lockFilesInOrder uses:

database.getSchema().getEmbedded().getMigratedFileId(f)

The sibling method checkExplicitLocks uses:

((LocalSchema) database.getSchema()).getMigratedFileId(f)

Since getMigratedFileId is not on the Schema interface, both styles ultimately reach LocalSchema. The getEmbedded() approach avoids the explicit cast and is cleaner - consider normalising checkExplicitLocks to use getEmbedded() as well for consistency.

3. Inconsistent LogManager.log call signature. The new log line passes null explicitly as the Throwable argument:

LogManager.instance().log(this, Level.FINE, "...", null, f, migrated);

The existing sibling log at line 936 uses the two-arg overload without a throwable:

LogManager.instance().log(this, Level.FINE, "...", f, newFileId);

The new line should use the same overload as the existing one to remain consistent.

4. Exception capture pattern in the test. The test uses a manual try/catch to capture the exception:

ConcurrentModificationException thrown = null;
try {
  database.commit();
} catch (final ConcurrentModificationException e) {
  thrown = e;
}
assertThat(thrown).as("...").isNotNull();
assertThat(thrown.getMessage()).contains("migrated");

The idiomatic AssertJ pattern used elsewhere in this codebase is assertThatThrownBy:

assertThatThrownBy(() -> database.commit())
    .isInstanceOf(ConcurrentModificationException.class)
    .hasMessageContaining("migrated");

This is cleaner, avoids the mutable thrown variable, and gives a better failure message when no exception is thrown.

5. Documentation file in the tree. The docs/4278-lockfilesinorder-silent-migration.md file appears to be an internal design note added to document reasoning. The project guidelines say not to proactively create documentation files. Consider removing it - the relevant context belongs in the PR description or the linked issue rather than the repository tree.

Correctness assessment

The core fix is correct and safe. The unlock-before-check reordering eliminates the window where the new mutable file was unlocked during commit. The ConcurrentModificationException thrown is already handled by the retry machinery in database.transaction() lambdas, so callers using that API are unaffected. Direct begin()/commit() callers now receive a clear, retryable exception rather than silent continuation.

The test reliably exercises the fixed code path. The 30-second timeout on compactionDone.await is generous and appropriate.

Overall assessment

The fix is correct, well-scoped, and includes a solid regression test. The items above are polish/style concerns - the em dash, the assertThatThrownBy pattern, and the documentation file are the items most worth addressing before merge.

[claude]

Code Review

Overview

This PR fixes a real correctness bug in TransactionContext.lockFilesInOrder(): when a locked file was missing because LSM compaction had migrated it to a new file ID, the original code fell through silently, leaving the new mutable file unlocked during commit. The fix restructures the guard block so unlock+rollback always runs first, then distinguishes migration from plain removal to produce the right exception. This correctly mirrors the sibling checkExplicitLocks() path.

The fix is correct and minimal.

---

TransactionContext.java

Structurally correct. Moving unlockFilesInOrder + rollback before the null-check means both exception paths now clean up properly before throwing. Callers using database.transaction() will retry automatically on ConcurrentModificationException; direct begin()/commit() callers now receive the retryable exception they were silently denied before.

Minor: drop the explicit null Throwable argument. The new log call (line 909) uses:

LogManager.instance().log(this, Level.FINE, "Found upgraded file '%d' to '%d' during transaction commit", null, f, migrated);

The sibling call in checkExplicitLocks() (line 936) uses the two-arg overload without a Throwable:

LogManager.instance().log(this, Level.FINE, "Found upgraded file '%d' to '%d' during transaction lock", f, newFileId);

Both overloads work, but passing an explicit null Throwable is inconsistent with the rest of the file. Prefer dropping the null to match the sibling style.

Pre-existing inconsistency (not introduced here): checkExplicitLocks() at line 933 still uses the old cast style ((LocalSchema) database.getSchema()).getMigratedFileId(f) while the new code at line 907 correctly uses database.getSchema().getEmbedded(). Since this pre-dates the PR it does not block merge, but that line is worth cleaning up in a follow-on.

Pre-existing double-rollback (not introduced here): The outer catch at line 709-710 calls rollback() again after catching ConcurrentModificationException. This was already true for the plain-removal branch before this PR, so rollback() is evidently idempotent; no action needed.

---

LockFilesInOrderFileMigrationTest.java

Sound test design. The approach - inject a stale file ID via getPageToModify, compact from a background thread behind a CountDownLatch, then commit on the main thread - exercises the exact race without relying on real timing. The precondition assertions (>= 2 pages, migration entry present, old file gone) make setup failures obvious rather than silent.

Suggestion: use assertThatThrownBy for the exception assertion. The test uses a manual try/catch + null check, which is valid but not the idiomatic AssertJ style the project prefers. The idiomatic form:

assertThatThrownBy(database::commit)
    .isInstanceOf(ConcurrentModificationException.class)
    .hasMessageContaining("migrated");

This is shorter, avoids the thrown == null trap, and matches the style encouraged by the project.

Consider @Tag("slow") if compaction plus a 1000-record insert takes noticeably long on CI. Probably fine, but worth validating on a slow machine.

---

docs/4278-lockfilesinorder-silent-migration.md

The "Review cycles" section documents bot response times and loop timeouts - this is ephemeral workflow metadata, not reference documentation. It will be stale immediately after merge. The technical root-cause analysis and fix description are genuinely useful, but the review-cycle tracking belongs in the PR description rather than the repo's docs/ folder. Consider trimming that section before merging.

---

Summary

Area	Assessment
Bug fix correctness	Correct - the silent fallthrough is eliminated
Exception message quality	Good - includes both old and new file IDs, actionable hint
Test reliability	Good - deterministic injection, no timing dependency
Style	Minor: drop null Throwable arg; use assertThatThrownBy in test
Docs file	Trim the review-cycle metadata before merge

No blocking issues. The fix is safe to merge after addressing the minor style points.

[claude]

Code Review

The fix is correct and well-motivated. The bug is real: the original lockFilesInOrder silently fell through when getMigratedFileId() returned non-null, leaving the new mutable file unlocked and open to concurrent writes. The restructuring (always unlock+rollback first, then distinguish migration from removal) mirrors checkExplicitLocks correctly.

Issues to address

1. License header typo in LockFilesInOrderFileMigrationTest.java (line 18)

STML-License-Identifier: Apache-2.0

should be:

SPDX-License-Identifier: Apache-2.0

2. Inconsistency in getMigratedFileId call style

The new code in lockFilesInOrder uses:

database.getSchema().getEmbedded().getMigratedFileId(f)

while the sibling method checkExplicitLocks still uses:

((LocalSchema) database.getSchema()).getMigratedFileId(f)

Both call sites should use the same pattern. Since getEmbedded() is the cleaner approach (no cast), checkExplicitLocks should be updated to match, or a note should explain the intentional difference.

3. docs file belongs elsewhere

docs/4278-lockfilesinorder-silent-migration.md contains issue-specific analysis that is better captured in the GitHub issue/PR description (where it already lives in full). Adding per-issue design docs to the source tree creates clutter that doesn't age well. The high-level rationale from this file should live in the issue/PR, not as a committed file.

4. Multi-line class-level Javadoc on the test

The test class carries a 7-line Javadoc block. The project style is one short line max (or none). The method name already conveys the scenario - the existing comment would be better reduced to a single short sentence or removed entirely.

Minor observations

• The for loop in lockFilesInOrder always throws on the first missing file, so calling unlockFilesInOrder(locked, ...) from inside the same iteration is safe (execution never continues). The logic is correct.
• The getPageToModify(..., false) followed by writeByte(0, readByte(0)) to force a dirty page is a reasonable injection point for a regression test, but it is inherently coupled to internal commit mechanics. A comment explaining why the read-modify-write is needed (to pass the range[1] > 0 gate in commit1stPhase) would help future readers - the existing inline comment partially covers this but could be tightened.
• The isGreaterThanOrEqualTo(2) pre-condition assertion is good defensive test design.

Summary

The core fix is sound and minimal (3 lines changed, matching existing conventions). Address the license typo and the getMigratedFileId call-style inconsistency before merging; the docs-file concern is lower priority but worth discussing.

[claude]

Code Review

Overview

This PR fixes a real concurrency bug: lockFilesInOrder() silently fell through when a file had been migrated by LSM compaction, leaving the new mutable index file unguarded during commit. The fix is structurally correct - always unlock and rollback first, then distinguish migration from plain removal. The accompanying regression test and putDuplicates stabilisation are both well-reasoned.

---

Issues

1. API inconsistency in getMigratedFileId call style

lockFilesInOrder() (line 907) now uses:

database.getSchema().getEmbedded().getMigratedFileId(f);

But checkExplicitLocks() (line 933) still uses the old cast form:

((LocalSchema) database.getSchema()).getMigratedFileId(f);

These two sibling methods should use the same pattern. Since getEmbedded() already avoids the cast and is used elsewhere in TransactionContext (lines 143-144, 665, 907), checkExplicitLocks() should be updated to match.

2. Docs file should not be committed

docs/4278-lockfilesinorder-silent-migration.md is a planning/decision document that duplicates the PR description and the in-code comments. Per project guidelines, such artifacts do not belong in the repository - the information is already preserved in git history and the PR body. This file will become stale as soon as the code evolves.

3. Multi-paragraph comment block in putDuplicates

The block comment added at the top of the try block in LSMTreeIndexTest.putDuplicates is a multi-paragraph explanation. Project guidelines say to keep comments to one short line when the why is non-obvious. The key point - "disable auto-compaction to prevent ConcurrentModificationException from interfering with DuplicatedKeyException assertion" - can fit in one line. The rest describes the mechanism rather than the constraint.

---

Positive aspects

• Fix is minimal and correct. The 3-line restructuring in lockFilesInOrder() is the right shape: unlock + rollback always come first (preventing any window where files are left locked after detecting a bad state), then the exception message distinguishes migration from removal.
• No double-unlock risk. rollback() calls reset() which only unlocks this.lockedFiles and this.explicitLockedFiles. Since the throw happens before lockFilesInOrder() returns, this.lockedFiles is never set to the (already-unlocked) locked list - the paths don't conflict.
• Regression test is well-structured. CountDownLatch + AtomicReference<Throwable> is the right pattern for asserting background thread outcomes. The test correctly verifies that migration state exists and the old file is gone before committing, then asserts the exception message contains "migrated" rather than just the exception type.
• putDuplicates config restore is correct. The finally block guarantees the original INDEX_COMPACTION_MIN_PAGES_SCHEDULE value is always restored, even on test failure.
• Exception message is actionable. "file 'X' has been migrated to 'Y' (likely by an index compaction). Please retry the operation." gives callers the old and new IDs and explicitly signals retryability - better than the previous generic "file has been removed".

---

Summary

The fix and test are sound. Three things to address before merging:

1. Update checkExplicitLocks() line 933 to use database.getSchema().getEmbedded().getMigratedFileId(f) for consistency with lockFilesInOrder().
2. Delete docs/4278-lockfilesinorder-silent-migration.md.
3. Collapse the multi-paragraph comment in putDuplicates to a single line.

[codecov]

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.17%. Comparing base (26eeb40) to head (473fef5).

Files with missing lines	Patch %	Lines
...java/com/arcadedb/database/TransactionContext.java	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4279      +/-   ##
============================================
- Coverage     64.54%   64.17%   -0.38%     
- Complexity      445      453       +8     
============================================
  Files          1645     1645              
  Lines        127474   127476       +2     
  Branches      27324    27324              
============================================
- Hits          82280    81803     -477     
- Misses        33546    34146     +600     
+ Partials      11648    11527     -121     

☔ View full report in Codecov by Sentry.
📢 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.