HDDS-14768. Fix lock leak during snapshot cache cleanup and handle eviction race appropriately.

[SaketaChalamchala]

What changes were proposed in this pull request?

Currently,

1. Eviction race: SnapshotCache cleanup throws an IllegalStateException when it finds stale entries in pendingEvictionQueue for snapshots that have already been removed from dbMap
   Ex., say SnapshotPurge invalidates the entry right before the last thread with a reference to the snapshot just closes adding the snapshotID back to the evictionQueue
2. Inconsistent Bookkeeping: invalidate removes snapshot entry from dbMap but does not remove it from pendingEvictionQueue if it exists.
3. Potential snapshot leak: Snapshot close failure during cleanup removes the snapshotID from eviction queue and throws an exception. This causes the snapshot to remain in cache even is refCount = 0 and the snapshot entry remains in dbMap unless
   some other thread explicitly invalidates it or references it again. This means SnapshotCache.lock() during this time cannot hold the write lock because lock() expects the cache to be drained.
4. Write lock leak: Fix write lock leak in SnapshotCache. If the cache drain cleanup(true) throws an exception write lock is not released in SnapshotCache.lock()

Proposed solution:

1. Handle eviction race appropriately. Log the stale snapshot entry in eviction queue and remove it from the queue.
2. Remove snapshot entry from eviction queu upon successful invalidation.
3. Log snapshot close failure during cleanup but do not remove it from eviction queue so that it's cleanup can be retried later.
4. Catch any unchecked exception during cleanup and release the write lock in SnapshotCache.lock()

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14768

(Please replace this section with the link to the Apache JIRA)

How was this patch tested?

Unit tests.

[Copilot]

Pull request overview

Fixes SnapshotCache eviction/cleanup edge cases that could previously throw during cleanup, leak the snapshot DB write lock, or leave stale eviction entries behind—improving correctness and reliability of snapshot purge / checkpoint coordination in Ozone Manager.

Changes:

• Remove snapshot IDs from the pending eviction queue on invalidate, and tolerate stale eviction entries during cleanup.
• Ensure SnapshotCache write lock is released if cleanup throws (including unchecked exceptions).
• Adjust cleanup behavior so snapshot close failures are logged and retried later, with added unit tests covering these races/failures.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java	Updates eviction bookkeeping, cleanup behavior on stale/missing entries and close failures, and hardens lock() to release locks on exceptions.
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java	Adds unit tests for stale-eviction cleanup, close-failure retry behavior, and write-lock release when cleanup throws.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

The new warning for stale eviction keys (v == null) says the RocksDB instance “may not be closed properly”, but this condition can also occur in the expected invalidate + late close race (snapshot was closed and removed from dbMap, and the callback re-queued the UUID). Consider rewording the log to reflect that this can be a benign stale-queue entry, and include guidance only if it indicates a real leak signal.

Suggested change

	LOG.warn("SnapshotId '" + k + "' does not exist in cache. The RocksDB " +
	"instance of the Snapshot may not be closed properly.");
	LOG.warn("SnapshotId '{}' not found in cache during cleanup. "
	+ "This can happen if the snapshot was already closed and "
	+ "removed from the cache, leaving a stale entry in the eviction "
	+ "queue. If this message appears frequently for the same "
	+ "snapshot or the cache size keeps growing, it may indicate that "
	+ "a RocksDB instance was not closed properly.");

[Copilot]

In cleanup(evictionKey, ...), the pending eviction key is removed even when the snapshot is still referenced (totalRefCount > 0). If another thread decrements the refcount to 0 and the callback re-adds the key concurrently, this removal can win and drop the key permanently, leaving an unreferenced snapshot in dbMap that will never be retried for cleanup. Consider either not removing the key when refcount > 0, or removing it conditionally and re-adding if the refcount becomes 0 after the decision (or otherwise synchronizing queue updates).

[Copilot]

SnapshotCache.lock(...) can return a supplier whose OMLockDetails reports lock not acquired when cleanupFunction returns false (eg, cache not drained). The public lock() / lock(UUID) Javadocs currently describe the lock as ensuring thread-safety, but don’t mention that callers must check isLockAcquired() (some call sites use try-with-resources without checking). Consider either documenting this contract explicitly here or throwing when cleanup can’t satisfy the precondition so callers can’t proceed without the intended lock.

[SaketaChalamchala]

Updated the patch to throw an exception when cleanup returns false so that callers do not proceed without the lock.

[smengcl]

Using reflection to access private field in SnapshotCache is unstable. Why not just expose it via a getPendingEvictionQueue() method in SnapshotCache that has @VisibleForTesting annotation?

[jojochuang]

SnapshotCache.invalidate() is called during OMSnapshotPurgeResponse.

[jojochuang]

pendingEvictionQueue may have the entry k, if snapshot purge response is happening after, and all references of the snapshot is decremented (releasing the SnapshotCache lock of the key), but before the periodic cleanup thread kicks in.

[jojochuang]

note: this case should not happen during checkpointing, because checkpoints holds a write lock of the snapshot cache, and once released, it invokes cleanup() immediately.

[jojochuang]

the lock is released by calling emptyUnlockFunction.get(), if cleanup operation was not successful or if it throws a Throwable.

[SaketaChalamchala]

Thanks for the review @sadanand48 , @smengcl and @jojochuang I addressed your comments and relevant copilot suggestions as well

Additional updates:

• reverted the error log to an exception in cleanup so that it fails fast.
• Throwing an exception when cleanup function returns false so that callers that do not check for isLockAcquired() do not proceed without the lock.