Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

test comment please ignore

Code Review - fix(#4333): persist migratedFileIds in schema.json

The fix is correct in its core approach: serializing migratedFileIds to schema.json and restoring it on open so WAL recovery can distinguish safe compaction-migration skips from genuinely missing files. A few issues need attention before merge.

Bug: double saveConfiguration() per compaction

LSMTreeIndex.java and LSMVectorIndex.java both already call saveConfiguration() immediately after setMigratedFileId():

// LSMTreeIndex.java ~line 675
((LocalSchema) database.getSchema()).setMigratedFileId(fileId, newMutableIndex.getFileId());
database.getSchema().getEmbedded().saveConfiguration();  // still here

// LSMVectorIndex.java ~line 4618
((LocalSchema) getDatabase().getSchema()).setMigratedFileId(fileId, newMutableIndex.getFileId());
getDatabase().getSchema().getEmbedded().saveConfiguration();  // still here

With this PR, setMigratedFileId now internally calls saveConfiguration() too, resulting in two consecutive schema-file writes per compaction. Each write re-serializes the full schema JSON. This isn't a correctness bug but it is a needless performance hit on what is already an I/O-intensive path.

Fix: remove the explicit saveConfiguration() calls from LSMTreeIndex and LSMVectorIndex (they are now redundant), or remove it from setMigratedFileId and document that callers are responsible.

Missing assertion in migratedFileIdsRoundTrip

migratedFileIdsArePersisted correctly guards against compaction being a no-op:

final boolean compacted = bucketIndex.compact();
if (!compacted)
  error.set(new AssertionError("compact() returned false"));

migratedFileIdsRoundTrip omits this check - compact() return value is silently discarded. If compaction is skipped for any reason (not enough pages, concurrent activity), getMigratedFileId() returns null and the test fails with a misleading NullPointerException-style message rather than a clear "compaction did not run" message. Add the same guard used in the first test.

docs/4333-persist-migrated-file-ids.md should not be committed

This file is an AI review-cycle tracking document ("Review Cycles", "Cycle 1 - a47f16b", "gemini-code-assist reviewed", "State: timeout"). That kind of meta-tracking belongs in the PR description or issue, not in the repository. It will rot immediately and confuse future contributors. Please drop this file from the PR.

Design note: migratedFileIds grows without bound

Once an old->new file mapping is written to schema.json it is never removed - not even after the old file has been dropped and WAL segments predating the last checkpoint are irrelevant. For a long-lived database with frequent compactions this map will accumulate stale entries forever.

This is a pre-existing design question (the map already lived in memory indefinitely), but persisting it makes the problem permanent. A follow-up should add a cleanup pass - e.g., remove entries whose oldFileId no longer exists on disk after WAL recovery completes.

Minor: log message accuracy at INFO level

The INFO log reads:

"Skipping page for missing file %d during transaction apply, file was deleted or migration map is incomplete"

"migration map is incomplete" suggests a partial write/corruption, which is alarming. Given that the most common benign cause is a schema DROP, something like "file was deleted (schema change or DROP TYPE/INDEX)" reads more clearly for operators.

Summary

The core logic (serialize/deserialize migratedFileIds, consult map in applyChanges) is sound. The double-save and the docs file are the most important things to fix before merge.

Code Review

Summary: This PR correctly identifies and fixes a real bug - LocalSchema.migratedFileIds was heap-only and lost on restart, causing WAL recovery to silently drop compaction-related pages. The fix is straightforward and the approach is sound. A few concerns worth addressing before merge.

Bug: WARNING log for intentional DROP TYPE / DROP INDEX

The original comment in TransactionManager.applyChanges() said:

Referencing a deleted file is expected during Raft replay and crash recovery - schema changes may delete files before their prior TX entries are replayed.

That case (DROP TYPE / DROP INDEX) is still completely valid and now logs at WARNING since the dropped file won't be in migratedFileIds. This will produce operator noise on any cluster that does Raft replay or crash-recovers after a DROP. Example: a user drops an index, the server crashes, and on restart the WAL replay logs a WARNING for every page of that index.

The intent of the new WARNING is good (flag truly unexpected missing files), but currently it fires on both unexpected files and intentional schema-drop files. Consider whether a separate mechanism - such as recording intentionally dropped file IDs in the schema JSON as a droppedFileIds set - is needed to distinguish the two cases, or at minimum add a comment acknowledging the false-positive risk.

Unbounded map growth in schema.json

migratedFileIds has no eviction. Every compaction appends a new oldId -> newId entry. With persistence, the map now grows in schema.json across the lifetime of the database. Long-lived databases with many compactions will see this bloat accumulate indefinitely.

Consider pruning entries whose old file ID no longer appears in any live WAL file, or at least document this as a known limitation so it can be tracked as a follow-up.

Test: second test ignores compact() return value

In migratedFileIdsRoundTrip, the compaction thread does not check whether compact() returned true. If compaction silently bails out (e.g. not enough pages), getMigratedFileId() will return null and assertThat(newFidBefore).isNotNull() fails with a misleading message. The first test handles this correctly - replicate the error.set(new AssertionError("compact() returned false")) guard.

Also, assertThat(done.await(30, TimeUnit.SECONDS)).isTrue() and assertThat(error.get()).isNull() in the second test lack descriptive messages, unlike the first test. Small but inconsistent.

Test code duplication

The two test methods are nearly identical (same schema setup, insert 1000 docs, run compaction in a thread, close/reopen, assert). Consider extracting shared setup into a helper method that takes the type name and property name as parameters. This would make both test bodies much shorter and the intent clearer.

Docs file

docs/4333-persist-migrated-file-ids.md is an issue-specific design document. The PR description already covers this content well. Committing issue-specific analysis docs to the repo tends to become dead weight as the code evolves. Worth considering whether this belongs in the PR description / commit message instead.

Minor: the PR body references a test named migratedFileIdsAreLoadedOnReopen but the actual test method is migratedFileIdsRoundTrip.

What works well

• Backward-compatible serialization (key omitted when empty, load handles missing key with no-op)
• migratedFileIds.clear() before repopulation in readConfiguration() ensures idempotency on re-read
• The serialization uses string keys (required by JSON) with Integer.parseInt deserialization - correct
• readConfiguration() is synchronized so the clear+populate sequence is safe under concurrent access
• The getMigratedFileId call in TransactionManager is consistent with existing usage in TransactionContext
• Test coverage proves the key scenario (persistence survives close+reopen)

Code Review - fix(#4333): persist migratedFileIds in schema.json to survive restart

Overview

This PR fixes a real correctness bug: LocalSchema.migratedFileIds was heap-only, so after a restart the WAL recovery path in TransactionManager.applyChanges could not distinguish a safe compaction-skip from an unknown missing-file condition. The fix serializes the map to schema.json and restores it on open. Good find and clear root cause analysis.

Issues

1. Double saveConfiguration() after compaction (redundant write)

Both callers of setMigratedFileId already call saveConfiguration() immediately after:

• LSMTreeIndex.java:677: database.getSchema().getEmbedded().saveConfiguration();
• LSMVectorIndex.java:4619: getDatabase().getSchema().getEmbedded().saveConfiguration();

Now that setMigratedFileId itself calls saveConfiguration(), those two call sites each trigger two serializations of the schema in quick succession. This is idempotent and not a correctness problem, but it is wasteful since schema serialization is a synchronized operation. Consider removing the now-redundant explicit saveConfiguration() calls in the two index classes, or removing it from setMigratedFileId and documenting that callers are responsible for persisting.

2. migratedFileIds map grows without bounds

There is no eviction or pruning of entries from migratedFileIds. Every compaction permanently adds an entry that persists across restarts. For a long-lived database with frequent compactions, this map will grow indefinitely (unbounded memory and schema.json bloat). There should be a lifecycle event where stale entries can be removed - for example, when the old file is confirmed to no longer appear in any recoverable WAL segment, or when a subsequent compaction replaces oldId -> newId with newId -> newerNewId. At minimum, this should be noted as a known limitation with a follow-up issue.

3. docs/4333-persist-migrated-file-ids.md should not be committed

This file contains internal review-cycle notes ("gemini-code-assist reviewed (COMMENTED)", "State: timeout", links back to the PR being merged). It is meta-documentation about the review process, not documentation about the feature or schema format. It will become stale immediately after merge. PR descriptions and issue comments are the right place for this content.

Minor Points

4. Second test does not assert on compact() return value

In migratedFileIdsRoundTrip, the compaction thread calls bucketIndex.compact() without checking the return value. If compaction is skipped (returns false), the test will proceed and fail with a confusing "newFidBefore is null" message rather than a clear "compaction did not run" failure. The first test (migratedFileIdsArePersisted) handles this correctly - apply the same pattern to the second test.

5. The two tests are nearly identical

Both tests: create a schema, insert 1000 records, compact, close, reopen, and assert the migration entry survived. The only difference is the entity name. Consider merging into a single test or giving the second a distinct scenario (e.g., multiple sequential compactions, or a migration followed by a type drop).

What Works Well

• The !root.isNull("migratedFileIds") null guard is correct and the backward-compatibility approach (omitting the key when empty) keeps existing schema files compatible.
• Restoring with migratedFileIds.clear() before repopulating is correct for idempotent reloads.
• Downgrading the applyChanges log from WARNING to INFO for the unknown-file case is reasonable - this path fires for every DROP TYPE/INDEX replay and WARNING would be noisy.
• The setMigratedFileId -> saveConfiguration addition correctly addresses the race where a crash between compaction and the next schema change left the map unpersisted.

Codecov Report

❌ Patch coverage is 58.82353% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.96%. Comparing base (128872a) to head (2789ec0).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main    #4342      +/-   ##
==========================================
- Coverage   65.37%   64.96%   -0.42%     
==========================================
  Files        1610     1610              
  Lines      123789   123803      +14     
  Branches    26776    26781       +5     
==========================================
- Hits        80925    80426     -499     
- Misses      31429    32048     +619     
+ Partials    11435    11329     -106     

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