fix(#4824): make Raft applied-index tracking per-database-aware - #4847

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

• Queue this pull request

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

Review: fix(#4824) per-database Raft applied-index tracking

Nicely scoped fix. The root-cause analysis is correct: the per-database replay-skip in applyBootstrapFingerprintEntry was consulting a value that mixes all databases, and making it consult per-database evidence (strict, no global fallback) is the right call. The legacy-file compatibility, atomic-rename, and lazy double-checked load are all handled cleanly, and the regression test is verified failing-before/passing-after. Comments explain the why well.

A few points worth considering, mostly minor:

1. Concurrent writers race on the fixed temp file name (worth verifying)
persistAppliedIndexFile() always writes to applied-index.tmp and runs OUTSIDE appliedIndexFileLock. There are now two persist call paths:

• applyTransaction -> writePersistedAppliedIndex(index, dbName) on the StateMachineUpdater thread
• notifyInstallSnapshotFromLeader -> writePersistedAppliedIndexForAllDatabases(snapshotIndex)

If those two paths can ever overlap, they race on the same applied-index.tmp (one ATOMIC_MOVE fails because the temp was already moved, the exception is swallowed at FINE, and a write is silently lost). The single-temp-name pattern pre-dates this PR, but the PR adds the second writer. If install-snapshot and apply are guaranteed mutually exclusive in Ratis this is a non-issue - worth a one-line confirmation in the comment. Otherwise, either synchronize persistAppliedIndexFile() on appliedIndexFileLock or use a unique temp name.

2. appliedIndexByDb grows unbounded; dropped DBs are never evicted
applyDropDatabaseEntry removes the database but leaves its entry in appliedIndexByDb. Worse, the drop entry`s own applyTransaction then calls writePersistedAppliedIndex(index, droppedDbName), re-adding the dropped name with the drop index. The correctness impact looks benign because Raft indices are monotonic (a same-name recreated DB gets a higher bootstrap-entry index than any stale value, so no false skip), but the map and persisted JSON grow for the node lifetime. Consider appliedIndexByDb.remove(databaseName) in the drop path (and persisting after).

3. Per-apply allocation on the hot path
The file was already rewritten on every applyTransaction, so write frequency is unchanged - but the payload moved from Long.toString(index) to building a JSONObject and iterating the entire per-DB map every apply. Given the project GC-pressure mantra, this is more allocation per applied entry, scaling with the number of co-located databases. Small in absolute terms, but flagging since this is the apply hot path.

4. Test gap: the snapshot-install path is untested
writePersistedAppliedIndexForAllDatabases is a behavioural change (every present DB recorded at the snapshot index), but no test exercises it - mockServer() does not even stub getDatabaseNames(). A test asserting that a full install records every present DB at snapshotIndex would lock in the documented behaviour and guard against the NPE-on-null-getDatabaseNames() shape.

5. Doc drift (trivial)
docs/4824-...md lists three tests; the suite has four (perDatabaseValuesRoundTripAcrossRestart is missing from the doc).

None of these block the fix - (1) is the only one I would want explicitly confirmed before merge. Good, well-documented work overall.

Note: I was unable to run the ha-raft test suite in this review environment (build commands were not permitted), so this is a static review; the PR reports the suite passing.

Review: per-database Raft applied-index tracking (#4824)

Solid, well-reasoned fix. The core problem is clearly articulated and the chosen approach (strict per-database evidence, no global fallback for the per-database decision) is the right call - falling back to the global value is exactly what produced the original defect. The lazy-load + in-memory cache + atomic-rename write keeps the hot apply path honest, and the inline comments document the why unusually well. Tests cover the regression, the legitimate skip, legacy migration, round-trip, and the full-install path.

A few points worth discussing:

1. Legacy-file first-restart behavior (please confirm acceptable)

Every existing deployment upgrades with a legacy plain-number file, which by design yields -1 for every per-database read. For any BOOTSTRAP_FINGERPRINT_ENTRY that still sits above the latest Ratis snapshot, the replay-skip in applyBootstrapFingerprintEntry will no longer fire on the first post-upgrade restart, so verification re-runs.

The PR says re-verification is "idempotent (a fingerprint match returns immediately)" - but that only holds when the local fingerprint still matches. The scenario the skip was originally introduced for (per the existing comment: "the local database has since been forward-replicated past the baseline") is precisely the case where local lastTxId > chosenLastTxId, which now falls through to the "late newer joiner" SEVERE branch (local lastTxId ... is GREATER than cluster bootstrap lastTxId ... Refusing to overwrite local data) rather than being quietly skipped. A mismatch with local < chosen would instead re-enter installFromLeaderForBootstrap during replay - the leader-discovery race the original guard was added to avoid.

The window is bounded (the bootstrap entry must still be above the latest snapshot at upgrade time, which is mostly an issue for recently-formed / low-snapshot clusters), so this may well be acceptable as a one-time upgrade cost. But it's a behavioral regression relative to the old global-skip, and at minimum it would help to call it out explicitly in the doc/changelog so operators aren't alarmed by a burst of SEVERE "Refusing to overwrite local data" lines on the first restart after upgrade. Worth confirming the local > chosen path really is benign here.

2. DROP path issues two file writes

In applyTransaction, a DROP_DATABASE_ENTRY calls writePersistedAppliedIndex(index, null) (which serialises the file) and then removePersistedAppliedIndexForDatabase(dbName) (which serialises again). That's two temp-file + atomic-rename cycles per drop, and a crash between them leaves a stale per-db entry on disk for a database that no longer exists (harmless, but inconsistent). Drops are rare so the cost is negligible, but folding the eviction into the single write (e.g. a flag/param on writePersistedAppliedIndex that removes instead of puts) would be cleaner and atomic.

Minor / positive

• Thread-safety reasoning is sound: all writers go through appliedIndexFileLock, globalAppliedIndex / appliedIndexLoaded are volatile, reads use ConcurrentHashMap.get, and the lazy load is correct double-checked locking. Indices are monotonic on the apply path so the per-db put never regresses.
• No performance regression: the file was already rewritten on every apply, so the added small-JSON allocation is genuinely dominated by the existing I/O (correctly documented).
• readPersistedAppliedIndex(null) returning -1 and the legacy-format detection are nicely defensive.

Nice work overall - the only thing I'd want resolved before merge is an explicit answer on the legacy-upgrade behavior in (1).

Review: fix(#4824) make Raft applied-index tracking per-database-aware

Nicely scoped fix. The root-cause analysis is correct: a single global scalar consumed by a per-database decision (applyBootstrapFingerprintEntry's replay-skip) is exactly the "value that mixes databases" defect, and splitting the persisted file into global + per-db map while keeping the global value strictly out of the per-database decision is the right call. The regression test that asserts the other database's high index no longer suppresses verification (and was verified failing-before) is the strongest part of the PR.

A few observations, mostly minor:

1. ensureAppliedIndexLoaded() latches loaded=true even when the file was unavailable (latent)

In ensureAppliedIndexLoaded(), when getAppliedIndexFile() returns null (i.e. server == null), the finally still sets appliedIndexLoaded = true. If that method were ever reached before setServer(...), the cache would latch permanently empty and mask a later-available persisted file for the lifetime of the instance. The old readPersistedAppliedIndex() re-read the file on every call, so it had no such latch.

In the current wiring setServer always runs before initialize()/reinitialize(), so this isn't reachable today, but it's a fragile invariant. Consider only marking loaded when the file path was actually resolvable:

final Path file = getAppliedIndexFile();
if (file == null)
    return;            // don't latch; try again once server is wired
...
appliedIndexLoaded = true;

2. Downgrade / forward-compat of the file format

An older binary reading the new JSON document would hit Long.parseLong("{...}") -> NumberFormatException -> FINE log -> global treated as -1, re-running the (idempotent) verification. That's safe, but the PR/doc only covers the upgrade direction. Worth a one-line note that a downgrade degrades gracefully too.

3. docs/4824-...md content

The doc is a good design record, but the trailing "Review cycles" section (referencing gemini/claude bot rounds and linking the PR back to itself) reads as scratch/process notes rather than durable repo documentation. Consider trimming that section. Minor: the file uses em dashes throughout, which the repo generally avoids.

4. Redundancy with lastAppliedIndex (observation only)

globalAppliedIndex (volatile) now mirrors the existing lastAppliedIndex (AtomicLong) after every apply. They're seeded differently (one from the file at load, one set in reinitialize), so keeping both is defensible, but it's worth a comment noting they must stay in sync to avoid future confusion.

Things I checked and look correct

• All JSONObject methods used (getLong(name, default), getJSONObject(name, default), keySet, put) exist and behave as assumed.
• Map mutations (put/remove) all happen under appliedIndexFileLock; reads use ConcurrentHashMap.get - thread-safe.
• DROP folds the global advance + per-db eviction into one atomic write (no two-write crash window). Good.
• INSTALL_DATABASE_ENTRY flows through the normal applyTransaction path, so per-db gets recorded with the entry's db name - correct.
• No external readers of .raft/applied-index exist in the tree, so the format change is self-contained.
• Per-database read is strict (no global fallback), which is the whole point of the fix - verified in legacyPlainNumberFileReadAsGlobalNotPerDatabase.

Test coverage

Good: regression + legitimate-skip + legacy-file + round-trip + full-install paths are all covered. One gap worth considering: a test for the DROP eviction path (writePersistedAppliedIndexDroppingDatabase) asserting the per-db entry is removed while the global advances - it's exercised in production via applyTransaction but not directly asserted.

Overall this looks correct and well-tested. The only item I'd act on before merge is #1 (the load-latch), since it's a cheap hardening of an otherwise easy-to-trip invariant.

Review: fix(#4824) per-database Raft applied-index tracking

Reviewed the full change (ArcadeStateMachine, the new per-database test, and the design doc). This is a well-scoped, well-reasoned fix. The root cause analysis is correct: applyBootstrapFingerprintEntry made a per-database decision off a global scalar, and routing it through readPersistedAppliedIndex(dbName) with strict, no-global-fallback semantics is the right call. The inline rationale comments and docs/4824-...md are excellent and make the intent auditable.

What's solid

• Correctness of the merge-on-write path. writePersistedAppliedIndex calls ensureAppliedIndexLoaded() before mutating, so an existing per-db map (or a legacy file) is loaded and preserved rather than clobbered. This is the easy thing to get wrong and it's handled.
• Lazy double-checked load (appliedIndexLoaded volatile + appliedIndexFileLock) is correct, and deliberately not latching when the path is unresolvable (getAppliedIndexFile() == null) is a nice touch for wiring-order safety.
• DROP folds eviction + global advance into one atomic write (writePersistedAppliedIndexDroppingDatabase), avoiding both map growth and a crash window with a stale entry.
• Legacy plain-number compatibility is honest: read as global, yields -1 per-db, so a co-located DB can never falsely satisfy a per-db skip. Tested directly.
• Thread-safety between the apply thread and the supplyAsync snapshot-install thread is correctly serialized on appliedIndexFileLock; readers use the ConcurrentHashMap + volatile snapshot without the lock, which is fine.
• Test coverage is strong: the regression test fails-before/passes-after, plus round-trip, legacy, full-install, and drop-eviction cases.

Minor points (non-blocking)

1. The globalAppliedIndex / lastAppliedIndex "must stay in sync" comment (lines ~127-129) slightly overstates reality. reinitialize() seeds lastAppliedIndex from the Ratis snapshot while globalAppliedIndex comes from the file, and when snapshotInfo == null they diverge (-1 vs e.g. 100) until the next applyTransaction reconverges them. Not a bug, but the wording reads stronger than the code guarantees - consider softening to note they can be seeded independently and briefly differ after reinitialize().

2. Corrupt-file degradation silently disables snapshot-gap detection. ensureAppliedIndexLoaded() latches loaded=true with globalAppliedIndex staying -1 on a parse failure. That's intentional for the idempotent bootstrap re-verify, but note the side effect: in reinitialize() the gap check persistedApplied >= 0 && ... then evaluates false, so a corrupt applied-index file also suppresses the "snapshot ahead, download from leader" path. This matches prior behavior (a parse failure already returned -1), so it's not a regression - just worth a one-line comment so a future reader knows the coupling is deliberate.

3. writePersistedAppliedIndexForAllDatabases assumes getDatabaseNames() is non-null. It is today (Collections.unmodifiableSet(...)), so this is purely defensive - fine to leave as-is.

4. Test gap (small): the suite covers cross-instance round-trip and same-instance double-write, but not the upgrade-merge path explicitly - i.e. open a state machine over a pre-existing JSON file with entries for {db-a, db-other}, write a new index for only db-a, and assert db-other's entry survives the rewrite. The production code handles it (via load-before-mutate in writePersistedAppliedIndex), but it's the one correctness property without a direct assertion.

Performance

Per-apply cost grows from writing a single number to serializing a small JSON doc, but the dominant cost (createDirectories + writeString + atomic move) was already incurred every apply, and the per-db map is one entry per co-located DB. The doc's assessment is accurate - negligible on the hot path. No GC concern.

Security

No concerns - no sensitive data, file stays under .raft/, atomic-rename write is unchanged.

Nice work overall; the fix is minimal, correct, and unusually well-documented. The points above are polish, not blockers.

Reviewed by Claude Code.

Review: PR #4847 - per-database Raft applied-index tracking

Reviewed the diff against ArcadeStateMachine.java, the new test, and the surrounding apply/reinitialize paths. Overall this is a well-reasoned, well-tested fix for a real correctness bug (issue 4824). The root-cause analysis is accurate and the approach (strict per-database evidence, idempotent re-verification fallback) is the right one.

STRENGTHS

• Correct fix at the right layer. Making applyBootstrapFingerprintEntry consult the per-database applied index instead of the global scalar directly addresses the "value that mixes databases" defect. The strict no-global-fallback choice is the safe one: absent positive evidence it re-verifies, which is idempotent.
• Solid concurrency model. Writers serialize on appliedIndexFileLock (apply thread + snapshot-install thread), keeping the in-memory update and the shared applied-index.tmp write+rename atomic; reads stay lock-free via volatile + ConcurrentHashMap. The double-checked ensureAppliedIndexLoaded() is correct.
• Backward/forward compatible format. Legacy plain-number files degrade honestly to "global only, no per-db evidence", and the load-before-mutate property (proven by writePreservesOtherDatabasesEntriesOnDisk) prevents clobbering co-located databases.
• Strong test coverage. Regression, legacy-file, round-trip, drop-eviction, full-install, and load-before-mutate cases are all present and meaningful. The "was the local DB consulted?" discriminator via Mockito is a clean way to assert the skip decision.
• DROP eviction folded into a single atomic write removes a stale-entry crash window and keeps the map bounded.

SUGGESTIONS / MINOR POINTS (non-blocking)

1. docs/4824-applied-index-per-database-skew.md carries internal process metadata. The "Review cycles" (cycle 1-4, "gemini + claude reviewed", "max-cycles-reached") and "PR" sections describe the review workflow rather than the system, and will become stale noise in the repo docs/. Consider trimming the doc to the Problem/Fix/Tests/Upgrade-behavior technical content (which is genuinely useful) and dropping the meta sections, or moving the whole thing into the PR description.

2. No monotonic guard on globalAppliedIndex. Each writer does globalAppliedIndex = index unconditionally. Safe today because the apply path is monotonic and snapshot indexes only move forward, but a defensive globalAppliedIndex = Math.max(globalAppliedIndex, index) would harden against any future out-of-order writer at zero cost. Same thought for the per-db put. Low priority.

3. writePersistedAppliedIndexForAllDatabases does not evict absent databases. It records index for every present database but leaves entries for databases no longer present in the map, so after a full install the map can retain stale names. Not a correctness issue for the skip logic (a per-db value is only consulted when that db own bootstrap entry applies), but worth a one-line comment noting the asymmetry vs. the DROP path.

4. server.getDatabaseNames() NPE guard. writePersistedAppliedIndexForAllDatabases null-checks server but iterates getDatabaseNames() directly. It returns a Set and is unlikely to be null in practice - just flagging it.

5. Test gap (tiny): no direct unit test that a database-agnostic entry (dbName == null, e.g. SECURITY_USERS_ENTRY) advances the global position without creating a per-db map entry, nor that readPersistedAppliedIndex(null) returns -1. Both are covered indirectly, but a one-liner would lock in the documented behavior.

PERFORMANCE
The file is rewritten via atomic-rename on every applied transaction, which was already true before this change - the only addition is a small JSONObject build + map iteration per apply, correctly noted as dominated by the existing I/O. No regression. (The per-apply atomic-rename itself remains a latent hotspot under high write throughput, but that is pre-existing and out of scope here.)

SECURITY
No concerns: no sensitive data logged, no new external surface, and the format change is self-contained (no external reader of .raft/applied-index in the tree).

Nice work - the fix is correct and the test suite gives good confidence. The only thing I would actually act on before merge is point (1) (the doc process metadata); the rest is optional polish.

Code Review - PR #4847: per-database Raft applied-index tracking

Reviewed ArcadeStateMachine.java, the new regression test, and the docs note. Overall this is a well-targeted, high-quality fix. The root-cause analysis (a global scalar answering a per-database question) is correct, the chosen approach (option 2: global + per-db map, strict per-database with no global fallback) is the right one, and the code is unusually well-commented for the subtleties involved (legacy upgrade, drop eviction, full-install fan-out, the deliberate corrupt-file -> -1 coupling). Nice work.

Strengths

• Correct semantics. Per-database read returns -1 absent positive evidence, so a co-located DB can never falsely satisfy another DB's replay-skip. The legacy plain-number file is honestly mapped to global only. This is exactly what the issue asked for.
• Cache is an improvement, not just a feature. The old readPersistedAppliedIndex() re-read and re-parsed the file on every call (apply path + reinitialize). The new lazy-load-once + in-memory cache is cheaper on the hot path while keeping the same single atomic-rename write.
• Atomicity preserved. Drop folds the global advance and the per-db eviction into one serialized write (no crash window leaving a stale entry), and writePersistedAppliedIndexForAllDatabases fans out under the same lock.
• Test coverage is thorough: the regression case (the actual bug), the legitimate per-db replay-skip, legacy-file handling, round-trip across restart, full-install fan-out, drop eviction, and load-before-mutate preservation of co-located entries. The CountingServer "was the local DB consulted?" discriminator is a clean way to observe the skip decision without mocking.

Minor observations (none blocking)

1. globalAppliedIndex is set unconditionally, not monotonically. All three writers do globalAppliedIndex = index (and per-db put(dbName, index)) with no Math.max guard. On the Ratis apply path indices are strictly increasing, so this is safe in practice. The only theoretical regression would be a writePersistedAppliedIndexForAllDatabases(snapshotIndex) whose snapshotIndex is below the cached global - which shouldn't happen given a snapshot install pauses applies and advances forward. A defensive Math.max(globalAppliedIndex, index) would harden this for ~free, but it's optional.

2. Lock now spans file I/O on the apply thread. appliedIndexFileLock is held across writeString + atomic move in persistAppliedIndexFile(). Since Ratis applies are single-threaded per group and the only other writer is the rare snapshot-install thread (which runs while applies are paused), contention is effectively nil - just flagging that the critical section now includes syscalls. Fine as-is.

3. Downgrade path. After this ships, the file becomes JSON. An older binary's Long.parseLong("{...}") would throw and (as before) degrade to -1, re-running the idempotent verification - safe, but worth a one-line note in docs/4824-...md if downgrade is ever a supported sequence. The forward/upgrade direction is already well documented in the code.

4. Nice-to-have test gaps. Consider (a) asserting the on-disk file is actually the JSON shape (the round-trip test exercises the same read/write code, so a format regression that's internally consistent would still pass), and (b) a corrupt-file -> -1 degradation test, since that coupling with reinitialize()'s gap check is explicitly relied upon in the comments.

Verification

Statically reviewed against the JSONObject API (getLong(name, default), getJSONObject(name, default), put(String, Number) all exist and match usage) and confirmed there are no external callers of the changed read/writePersistedAppliedIndex signatures. Per the PR's test plan the ha-raft state-machine/snapshot suites pass.

LGTM with the minor notes above as optional follow-ups.

Thanks for the careful review. Disposition of the four notes:

4. Test gaps (a + b) - done (8248b99). Added persistedFileIsJsonDocumentOnDisk (reads the raw bytes and asserts the {"global", "db":{...}} shape independently of the reader, so an internally-consistent format regression can no longer slip past the round-trip test) and corruptFileDegradesToMinusOne (truncated-JSON file -> -1 for both global and per-db reads, pinning the coupling reinitialize()'s gap check relies on). Suite is now 9/9.

3. Downgrade note - already covered. docs/4824-applied-index-per-database-skew.md lines 79-80 already document the downgrade path (older binary fails to parse the JSON as a plain number, logs at FINE, treats global as -1, re-runs the idempotent verification).

2. Lock spans file I/O - leaving as-is, per your read: single-threaded Ratis apply + the rare snapshot-install thread (which runs while applies are paused) means contention is effectively nil.

1. Math.max global guard - leaving as-is, with reasoning. A guard on globalAppliedIndex alone isn't free: the per-db map still does a raw put(dbName, index), so in the exact pathological case the guard targets (index below the cached global) global would pin high while the per-db entry moves backward - global and per-db would diverge. A consistent guard would have to wrap the per-db puts too (including the drop/fan-out writers), adding complexity to defend against a case Ratis's monotonic single-threaded apply already prevents. The partial guard is a net negative rather than a hardening, so I'd rather keep the writers' "set to the index Ratis just applied" semantics uniform.

Review: PR #4847 - per-database Raft applied-index tracking

Thorough, well-scoped fix for #4824. The core change is correct: the bootstrap replay-skip now consults a per-database value with no global fallback, so a co-located database can no longer suppress another database's bootstrap verification. The docs, commit history, and test suite are unusually complete (regression + round-trip + corrupt-file degradation + DROP eviction + on-disk JSON shape pinning). Below are the findings.

Should fix - empty-string database name pollutes the per-database map

SECURITY_USERS_ENTRY decodes its databaseName to the empty string "", not null (RaftLogEntryCodec.decodeSecurityUsersEntry, line 503: new DecodedEntry(RaftLogEntryType.SECURITY_USERS_ENTRY, "", ...)). Every other entry type carries a real name. That has two consequences in this PR:

1. The map gets a permanent "" key. In applyTransaction (line 461) the security-users path calls writePersistedAppliedIndex(index, ""), and the guard in writePersistedAppliedIndex only checks if (dbName != null):

   if (dbName != null)
     appliedIndexByDb.put(dbName, index);

   So appliedIndexByDb.put("", index) runs on every security-users entry. That "" entry is then serialised into the persisted JSON db map, advanced for the node lifetime, and never evicted (there is no DROP for ""). This is exactly the "map growing for the node lifetime" problem the DROP eviction was added to avoid, reintroduced through the back door.

2. The code comment is factually wrong. Line 454 states: "decoded.databaseName() is null only for database-agnostic entries (e.g. SECURITY_USERS_ENTRY), which advance the global position only." SECURITY_USERS_ENTRY does not produce null - it produces "", so it does not advance "the global position only"; it also writes a "" per-db entry.

Suggested fix: treat empty as absent everywhere dbName is consumed:

if (dbName != null && !dbName.isEmpty())
  appliedIndexByDb.put(dbName, index);

and the symmetric guard in writePersistedAppliedIndexDroppingDatabase / the lookup in readPersistedAppliedIndex(String). Then fix the comment to say "null or empty". There is no correctness impact on the bootstrap skip itself (no real database is named ""), but the persisted file and in-memory map are polluted, which the PR otherwise explicitly works to prevent.

Minor

• globalAppliedIndex is assigned unconditionally rather than monotonically. All three writers do globalAppliedIndex = index;. On the real apply path indices are monotonic and the install path sets a high snapshot index, so this is fine today, but a max(...) would make the field robust against any future out-of-order writer and matches the "highest applied index across all databases" semantics described in the class comment. Optional.

• Reads can block behind write I/O. readPersistedAppliedIndex(...) -> ensureAppliedIndexLoaded() takes appliedIndexFileLock, which a concurrent persistAppliedIndexFile() (createDirectories + writeString + atomic move) also holds. These reads only happen on bootstrap/reinitialize paths, so contention is negligible - noting it only for completeness.

• New test creates real on-disk databases but is not tagged. ArcadeStateMachineAppliedIndexPerDatabaseTest spins up real LocalDatabase instances via DatabaseFactory.create() per test. If wall time is noticeable, per CLAUDE.md consider @Tag("slow"); if it stays sub-second, leave it.

Nits

• The DCL in ensureAppliedIndexLoaded() (volatile latch + double check) and the decision not to latch when the path is unresolvable are both correct and well-commented.
• The single-atomic-write DROP eviction removing the prior two-write crash window is a good catch.

Nice work overall - the empty-string guard is the one item I'd want addressed before merge.

Codecov Report

❌ Patch coverage is 84.05797% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.25%. Comparing base (d38a9fc) to head (8248b99).
⚠️ Report is 2 commits behind head on main.

@@             Coverage Diff              @@
##               main    #4847      +/-   ##
============================================
- Coverage     65.72%   65.25%   -0.47%     
- Complexity      772      790      +18     
============================================
  Files          1681     1681              
  Lines        133643   133711      +68     
  Branches      28534    28551      +17     
============================================
- Hits          87834    87259     -575     
- Misses        33727    34479     +752     
+ Partials      12082    11973     -109     

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