HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas

[Gargi-jais11]

What changes were proposed in this pull request?

On a datanode, some work runs under a container read lock (or otherwise changes which replica directory / in-memory Container is authoritative) while other threads look up a container once and later take a write lock. If the in-memory container mapping or on-disk location changes in between, the second thread can still use a stale Container or KeyValueContainerData reference. That is a classic TOCTOU problem: the map and the caller disagree about where the replica lives.

Worst cases include wrong ContainerSet updates, deleting or updating the wrong paths, ghost / orphan replica data on disk that SCM no longer tracks, and block deletion targeting the wrong RocksDB/chunks tree so pending deletes on the live replica are never applied (space not reclaimed).

Applies to CLOSED and QUASI_CLOSED (and any path where balancing/replication overlaps lifecycle commands), not a single state.

This can be easily explained with help of DiskBalancer as an example, although it applies to other read write races as well:
While DiskBalancer moves a container between volumes on the same datanode, it holds the container read lock, copies data, then calls ContainerSet.updateContainer(...) to point the in-memory map at a new Container instance (new volume / paths).
Whereas it is seen that other threads often look up the container once, then block on writeLock() until the move finishes. After they unblock, they still hold a reference to the old container (source volume). They then run logic and/or removeContainer(containerId) using stale paths and stale object identity.

Impacts:

1. Replication Manager DeleteContainerCommand: can remove the wrong map entry (live replica on the destination volume), delete/move source files, and leave an orphan replica on disk that SCM no longer sees — plus wrong volume accounting and ICRs keyed off the old replica.
2. BlockDeletingTask: uses a task-scoped KeyValueContainerData snapshot (source paths) while getContainer() may return the new replica; block cleanup can target the wrong chunks path and never clear pending-deletion state on the live replica → space not reclaimed.

Suggested Fix:

For RM side: After acquiring writeLock(), re-fetch from the map and compare by identity. If they differ, the container was moved — abort and let the caller retry.

container.writeLock();
try {    
Container<?> current containerSet.getContainer( container.getContainerData().getContainerID());
if (current != container) { 
// Container was relocated by DiskBalancer while we waited for the lock.Our reference is stale; the operation will be retried on the next cycle. 
 return;    
 } 
 // ... proceed with the actual operation
}

For BlockDeletingService:

container.writeLock();

// Re-fetch AFTER lock: map already has disk-B in scenarios.
// current.getContainerData() is disk-B's ContainerData ≠ this.containerData 
(disk-A snapshot).

Container<?> current = containerSet.getContainer(containerData
.getContainerID());
if (current == null || current.getContainerData() != containerData) {   
// containerData is stale — DiskBalancer relocated this container. BlockDeletingService will reschedule with disk-B's fresh containerData.

return;
} 

For DiskBalancer:
Move the container state check after acquiring readLock() to prevent stale references about container.

container.readLock();
try {
// Double check container state before acquiring lock to start move process.
// Container state may have changed after selection.
State containerState = container.getContainerData().getState();
if (!movableContainerStates.contains(containerState)) {
  LOG.warn("Container {} is in {} state, skipping move process.", containerId, containerState);
  postCall(false, startTime);
  return BackgroundTaskResult.EmptyTaskResult.newResult();
}   // Step 1: Copy container to new Volume's tmp Dir
  diskBalancerTmpDir = getDiskBalancerTmpDir(destVolume)
      .resolve(String.valueOf(containerId));
  ozoneContainer.getController().copyContainer(containerData, diskBalancerTmpDir); 
....

What is the link to the Apache JIRA

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

How was this patch tested?

Added unit tests on diskBalancer side as an example for the race condition which can happen with any services read/write Lock race.
Test file: TestDiskBalancerWithConcurrentBackgroundTasks

[Gargi-jais11]

@ChenSammi @sadanand48 Please review the patch as per your convinence.

[adoroszlai]

Thanks @Gargi-jais11 for the patch.

[adoroszlai]

Thanks @Gargi-jais11 for updating the patch.

[Gargi-jais11]

@sodonnel Please review this PR whenever you are free.

[sodonnel]

This change looks good and I think it will solve the problem. However I think it reveals a problem with the code structure within the datanode. These services should not have to perform such complex locking to get a consistent view of a container and it would be very easy in the future for some other service to come along and not do things correctly, or indeed some other existing part of the DN code may also be doing things incorrectly already.

I think that all the container "logic" should be hidden behind an interface and all the services like disk balancer, or block deletion should call it, eg:

containerManager.moveContainer(container_id, destination)
containerManager.deleteBlockList(container_id, blocksToDelete)
...
etc

Then all the locking etc happens behind the scenes in the same place where it can be controlled more closely.

Fixing this is a large exercise and not something we would want to take on in this PR.

There are other places in KeyValueHandler that take the container lock - are we sure they are OK or is any change needed in them too?

[Gargi-jais11]

This change looks good and I think it will solve the problem. However I think it reveals a problem with the code structure within the datanode. These services should not have to perform such complex locking to get a consistent view of a container and it would be very easy in the future for some other service to come along and not do things correctly, or indeed some other existing part of the DN code may also be doing things incorrectly already.

I think that all the container "logic" should be hidden behind an interface and all the services like disk balancer, or block deletion should call it, eg:

containerManager.moveContainer(container_id, destination)
containerManager.deleteBlockList(container_id, blocksToDelete)
...
etc

Then all the locking etc happens behind the scenes in the same place where it can be controlled more closely.

Fixing this is a large exercise and not something we would want to take on in this PR.

There are other places in KeyValueHandler that take the container lock - are we sure they are OK or is any change needed in them too?

I agree with u @sodonnel that all locking mechanism should be hidden behind an interface that it happens at the same place.
I was going through other write locks in the code base. I see that when EC Under-replication + DiskBalancer there can be failure however it cannot be the problem but we can minimise the effort here.
Setup: Container C (RS-6-3), shard index=2 exists only on DN1 (under-replicated). DiskBalancer on DN1 simultaneously moves shard idx=2 from Disk1 → Disk2.

The race:

DiskBalancer (DN1)                    EC Reconstruction (DN5 coordinator)
─────────────────────────────────     ────────────────────────────────────────
container.readLock()                  
  copy Disk1 → tmp/                                              pulls idx=0 from DN3 ✓
  atomic move tmp → Disk2                                 pulls idx=1 from DN4 ✓
  containerSet.updateContainer()                   containerSet.getContainer(C) → OLD obj (Disk1)
  readUnlock()                                                   tries writeLock → was blocked, now unblocked
  markContainerForDelete(OLD)                        OLD.state = DELETED
                                                                            exportContainerData checks state → DELETED
                                                                           throws IllegalStateException → FAILS
                                                                         ← entire ReconstructECContainersCommand fails
                                                                           bandwidth for idx=0, idx=1 already wasted

Net result: EC reconstruction fails just because the shard at DN1 has been marked as DELETED while it correctly pulled index from other DNs. Container C still has shard idx=2 missing. RM re-queues it next monitor cycle.

Here we have two options:
Option 1 — Rely on RM to re-send (current behavior)

• RM re-queues the container after command failure/timeout
• Next cycle: DN1's heartbeat has already reported the new path for idx=2 on Disk2
• RM sends a fresh ReconstructECContainersCommand — this time export succeeds on the NEW container
• Cost: 1 extra monitor cycle delay + bandwidth for idx=0, idx=1 already wasted in the failed attempt
• Safe? Yes — no data loss, just delayed fix. But every failed attempt wastes bandwidth for all N-1 other shards too.

Option 2 — Re-fetch container from ContainerSet after getting the lock like others
In exportContainerData, after acquiring the lock, re-fetch the container from ContainerSet by ID, so even if the old container is marked DELETED the new container moved by diskbalancer will be fetched and will not fail ECReconstruction.

Please let me know which option do @sodonnel @ChenSammi you both prefer ? RM retry is simpler and already works — it just costs one extra cycle and wasted network I/O for every time the race window is hit. I just wanted to bring this in your knowledge and discuss.

With Ratis UnderReplication and diskbalancer working on same container there is no issue as it will replicate only from any one of the container replica so even if the contaienr state is DELETED it will be fail or if before changing to delete UnderReplication acquires lock then it will be successful. But for EC just because of one shard entire operation needs to be repeated again as shared above.

[Gargi-jais11]

Second Finding: For QUASI_CLOSED container if SCM sends force Close command and DiskBalancer is working on same container
QUASI_CLOSED containers can be force-closed by SCM (CloseContainerCommand with force=true). That goes through controller.closeContainer().

Here is the exact timeline:

DiskBalancer (DN1)                                                              CloseContainerCommandHandler
─────────────────────────────────────────        ─────────────────────────────────────────
T1: container = containerSet.getContainer(C)
    → OLD container (QUASI_CLOSED, Disk1)

T2: container.readLock() on OLD

T3: copy Disk1 → Disk2 ...                                       T3a: container = containerSet.getContainer(C)
                                                                                   → OLD container (before updateContainer)
                                                                                   T3b: switch(container.getContainerState())
                                                                                   → QUASI_CLOSED + force=true
                                                                                    → controller.closeContainer(id)
                                                                                  → containerSet.getContainer(id)
                                                                                  → OLD container (still, before T5)
                                                                                  → container.close() 
                                                                                  → writeLock() → BLOCKED     ←-------   readLock held

T4: copy done, atomic move to Disk2
T5: importContainer → 
newContainer (QUASI_CLOSED, Disk2)
T6: containerSet.updateContainer(newContainer)
    ← ContainerSet now maps C → newContainer
T7: container.readUnlock()  ← releases
 OLD readLock
                                                                                      T7a: writeLock ACQUIRED on OLD container
                                                                                       → OLD: QUASI_CLOSED → CLOSED
                                                                                       → sendICR(OLD=CLOSED) → SCM told C is CLOSED

T8: container.markContainerForDelete(OLD)
    → writeLock → OLD: CLOSED → DELETED

after T8

	                                       State	                                                In ContainerSet?
---------------------------------------------------------------------------------------
OLD container (Disk1)         DELETED                                             No (updateContainer removed it)

NEW container (Disk2)       QUASI_CLOSED                                  Yes — this is the live replica

SCM's view: Container C on DN1 = CLOSED (from ICR sent at T7a), Reality: Container C on DN1 = QUASI_CLOSED (newContainer).

This is a kind of regression:
SCM thinks it's CLOSED. But DN1's next container report says QUASI_CLOSED. SCM sees a state "regression" (CLOSED → QUASI_CLOSED). Depending on the FCR sent to SCM, it may:

Re-send a force close command → controller.closeContainer(id) now re-fetches from ContainerSet → gets NEW container → closes it correctly → CLOSED. Eventually converges.
Or treat it as an unhealthy/inconsistent replica.
No data loss — the data is intact on Disk2. But there is a state inconsistency window where SCM's cached state (CLOSED) differs from reality (QUASI_CLOSED on the new disk).

I think here as well we need to re-fetch the container .

[Gargi-jais11]

for markContainerForUnhealthy with DiskBalancer parallely working . I believe this also needs to refetch the container after writeLock.
Here it works for any container state to mark all — CLOSED, QUASI_CLOSED, OPEN, RECOVERING — all can become UNHEALTHY.

Case 1: Container CLOSED/QUASI_CLOSED + DiskBalancer + Scanner in parallel
DiskBalancer (DN1)                                                                Container Scanner (DN1)
──────────────────────────────                                                    ──────────────────────────────────────────
T1: selects C (CLOSED, Disk1)
    added to inProgressContainers

T2: container.readLock() on OLD

T3: copy Disk1 → Disk2                                                            scanner reads OLD files on Disk1
    (I/O in progress)                                                             finds checksum failure
                                                                                  controller.markContainerUnhealthy(id, reason)
                                                                                    containerSet.getContainer(id)
                                                                                    → OLD container (Disk1)  ← stale ref
                                                                                    handler.markContainerUnhealthy(OLD, reason)
                                                                                    → writeLock() → BLOCKED (readLock held)

T4: copy done, checksum verified
T5: importContainer → NEW (CLOSED, Disk2)
T6: containerSet.updateContainer(NEW)
    ← ContainerSet maps C → NEW (Disk2)

T7: readUnlock() on OLD

                                                                                    writeLock ACQUIRED on OLD
                                                                                    state = CLOSED, not UNHEALTHY, volume not failed
                                                                                    OLD: CLOSED → UNHEALTHY   ← wrong container
                                                                                    writeUnlock
                                                                                    sendICR: C on DN1 = UNHEALTHY  ← stale, wrong

T8: markContainerForDelete(OLD) → DELETED

Final state:
  OLD (Disk1): DELETED
  NEW (Disk2): CLOSED  ← healthy, valid
  SCM view of container C:   C on DN1 = UNHEALTHY  ← wrong

What SCM/RM does in response :

T9:  RM next cycle — ECUnderReplicationHandler.checkAndRemoveUnhealthyReplica()
       SCM replica record: DN1 has UNHEALTHY replica of C
       checks: is there a CLOSED replica for same index on another DN? 
         → if YES: "prefer deleting the UNHEALTHY over CLOSED" → sendThrottledDeleteCommand(DN1)
         → if NO CLOSED elsewhere: "delete any UNHEALTHY" → sendThrottledDeleteCommand(DN1)
T10: DN1 receives DeleteContainerCommand for C
       containerSet.getContainer(id) → NEW container (Disk2, CLOSED)
       NEW container DELETED  ← healthy valid replica gone
       
     Now container C is genuinely under-replicated.
     RM tries to fix it by replicating — but the replica it just deleted was the source.

Outcome:  A healthy replica on Disk2 gets deleted. Container C becomes genuinely under-replicated.
The window between readUnlock (T7) and markContainerForDelete (T8) is the critical period. If the scanner's sendICR reaches SCM and RM processes it before FCR corrects the state, the delete command lands on DN1 and hits the healthy NEW container. This is why markContainerUnhealthy needs the re-fetch — the consequences of operating on the wrong container are irreversible.

[Gargi-jais11]

@sodonnel @ChenSammi above are more new areas which need same fix. Rest all do not have any issue like this. I have done thorough code checking.

[sodonnel]

In exportContainerData, after acquiring the lock, re-fetch the container from ContainerSet by ID, so even if the old container is marked DELETED the new container moved by diskbalancer will be fetched and will not fail ECReconstruction.

For EC Reconstruction I don't think it exports the entire container like this. More it reads the blocks out of the container block by block using the normal read path through the datanode. So the question is - can the normal read path be impacted by the balancer moving a container from disk 1 to disk 2? Ideally, the DN should be safe to do this using its locking as it is not ideal that reads would fail randomly. If its Ratis, it can just try another replica with s small delay. With EC it would fall back to reconstruction reads, which are slower and use more resources.

[sodonnel]

This problem is kind of getting difficult to see if we have solved all the cases or not.

The root cause is poor interface design, and further inadequate locking. Fixing this is going to be difficult.

The locking is only inadequate because the lock is inside the container object and the container object can be replaced by a new object against the same ID.

What are all the scenarios that replace that container object? What is we instead updated the existing object in place under a lock? Would that solve all these edge cases more simple? It sounds like these problems all come from the disk balancer moving and replacing the container object, so if we fix just that, it may be a cleaner solution to this whole problem?

[sodonnel]

Looking at the containerSet class, the updateContainer() method was added only for Diskbalancer and is only called by disk balancer. The only other place items are added to the containerMap is in the addContainer() method, so it seems like this could all be solved by updating the Container object fields rather than creating a new instance and replacing it. It doesn't appear to be an immutable object so this would be a much smaller fix and I think will solve all the issues?

[ChenSammi]

Generally we should choose the one with less complexity, and less cost. If replacing the fields inline, there are at least 4 fields' swaps, one by one. If replacing the container reference, there is one swap. Basically, hot swap one pointer has less complex result than hot swap four pointers, right?

I don't think updating fields in an object, which can be done atomically under an existing lock is complex.

Swapping the object for another is simple, but look at this bug it causes and the fix which is bleeding out into lots of code areas. And even after this fix, if someone adds a new service, they have to know they need to do these strange things (get a container, then get it again and check it hasn't changed) to avoid a race condition they shouldn't really be concerned about.

If we fix this at source, by locking the container object, updating the fields atomically and unlocking, then its fixed once and for all in a single place

@sodonnel , currently we swap one container pointer under container's read lock protection, we got one stale container reference. If we swap four fields under the same container's read lock protection, we got four stale fields reference. We need to double check all code places where these four stale fields reference can happen to know how many new race conditions and what's their consequences, they might be less severe or more severe than this jira.
For example, the chunk read doesn't hold any container lock, once the thread has the container object, it will assume volume, chunk, db path will not change. If multiple fields are swapped, it could end up with some path point to new location, some path point to old location, which seems more dangerous than the issues described by this jira, which are mainly there could be some blocks left in the replica, or some replicas are left not deleted in the first place(I believe this replica will be eventually deleted, a new replica for a deleted container will be deleted eventually). Another case is container scanner, doesn't hold any container lock too.
And even if we implement the swap four fields solution under the read lock, it's also not once and for all, every new code added, we also need to verify if there is new stale reference case.

The root cause of issue described in this JIRA, is because we do the container pointer swap in containerSet
under a read lock protection after container is moved to destination volume, if it's under a write lock protection, then some blocks left in the replica, or some replicas are left not deleted in the first place, none of these will happen. But consider moving a container from one volume might take a substantial time, we cannot afford to write lock the container, as it will make other threads waiting for operation on this container wait for a considerable amount of time.

The current approach in this jira, is not a perfect approach, the perfect approach is use write lock, but it's a approach with relative low complexity, as double check after get lock is common in multi-thread environment, easy to understand, and it's easy to find out where the write lock is held, and do the double check after the lock acquired, which is easier than identify all the places where volume, db, chunk path, meta path are used, and handle the stale reference.

The problem is, containerData is used in 100's of places across the code base - mostly in tests but over 100 in non-test code too.

Only write lock need be checked. Read on old replica is still supported. And disk balancer only moves closed and quasi-closed containers, so some write lock cases can also be skipped, such as closeContainer, left are mark container for deleted/unhealthy/closed, delete replica and delete blocks in replica.

[Gargi-jais11]

@sodonnel @ChenSammi
Yesterday night I tried implementing above diskbalancer flow but it seems to be complicated and bug prone.
(save old paths → readUnlock → writeLock → update volume/metadata/chunks/db paths → unlock → cleanup Disk1 from saved locals). @sodonnel direction is right in principle: one live Container object means no more updateContainer-style staleness for anything that always goes through getContainer(id) and the container lock.

In practice I found it quite easy to get wrong, especially Phase C (marking/deleting the old replica): the live object must never be used for Disk1 cleanup, V3 DB/cache handling needs care, crash recovery between phases has to be spelled out, and the readUnlock → writeLock gap still needs a post-lock state check .

Separately, @ChenSammi already called out the bigger issue: many call sites read KeyValueContainerData paths without holding the container lock. In-place updates to several fields are atomic under the write lock, but not automatically safe for those unsynchronized readers unless we address visibility and torn reads (e.g. volatile/immutable snapshot) and review a lot of DN code — so the “fix in one place” story is not complete without that wider work.
I’m leaning to keep the re-fetch/retry fix for this staleness issue.

[ChenSammi]

In-place updates to several fields are atomic under the write lock

It cannot be under the write lock. If write lock is feasible, this jira is no longer needed.

[Gargi-jais11]

In-place updates to several fields are atomic under the write lock

It cannot be under the write lock. If write lock is feasible, this jira is no longer needed.

When I said in-place updates to several fields are atomic under the write lock, I meant only the phase where we would swap metadataPath / chunksPath / volume (etc.) on the existing object — not the full DiskBalancer copy. The entire copy obviously cannot run under the container write lock; that was never the proposal.

You’re still right about the harder issue: many call sites read KeyValueContainerData paths (and related state) without holding the container read lock. So even if those field writes are done under writeLock(), we don’t get a clean story for unsynchronized readers (visibility / torn reads). That’s a big part of why the in-place approach felt fragile in practice.
Given that, I’m staying with the re-fetch / acquireContainerLock + retry direction for this JIRA instead of mutating paths in place on the live object.

[sodonnel]

Just proceed with the re-fetch idea. The architecture of the code in the datanode is so bad, its impossible to fix this any other way. I still think this leaves the door open for similar bugs in the future as its far from obvious on how this should be used if a new service comes along.

[Copilot]

Pull request overview

This PR addresses a TOCTOU-style race on DataNodes where threads may hold stale Container / KeyValueContainerData references after DiskBalancer (or similar flows) swaps the authoritative replica in ContainerSet, potentially leading to operations being applied to the wrong on-disk paths and leaving orphan replicas.

Changes:

• Add ContainerSet#acquireContainerLock(containerId) to obtain a write-locked container while verifying the containerId -> Container mapping remains stable (retrying on map swaps).
• Update key container operations to re-resolve/lock the live container after map swaps (eg, KeyValueHandler delete/close/unhealthy paths, BlockDeletingTask).
• Add/adjust unit tests to reproduce and validate the race scenarios (including a new concurrent DiskBalancer/background-tasks test).

Reviewed changes

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

Show a summary per file

File	Description
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java	Introduces acquireContainerLock with retry-on-swap semantics to prevent stale-instance write-locking.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java	Uses acquireContainerLock for markContainerUnhealthy, closeContainer, and delete path to avoid operating on stale replicas.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java	Re-resolves live container + container data under a stable write lock to ensure correct DB/paths after replica relocation.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java	Adds post-lock re-check to detect stale containerData and trigger retry when DiskBalancer has relocated the container.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java	Moves movable-state check under read lock and adds a reusable injector pause hook to open a deterministic race window for tests.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java	Adds focused unit tests validating acquireContainerLock behavior across stable, removed, swapped, and max-retry cases.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerWithConcurrentBackgroundTasks.java	New concurrency tests exercising DiskBalancer swap vs delete/block-delete/unhealthy/close operations to validate fixes.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java	Adds test validating retry behavior when container mapping/data becomes stale mid-operation.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java	Updates invalid-close test setup to create containers via handler/ContainerSet to match production flow.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java	Adjusts unhealthy-on-failed-volume test to use acquireContainerLock path.

---

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

[Gargi-jais11]

@ChenSammi

• I have reverted the commit and kept retry approach.
• Also improved the container not found and exhausted retry case. Now in case of container not found StorageContainerException CONTAINER_NOT_FOUND is thrown. And if acquireContainerLock throws null then it means retry has exhausted.
• Retry is reduced to 5.

Please re-review the Pr.

[ChenSammi]

KeyValueHandler.markContainerForClosed need be covered too.

this will not have an issue as this is only called for container in OPEN or RECOVERING state to move it to CLOSED state. So diskBalancer will not choose OPEN or RECOVERING container to perform move so that race with diskbalancer will never happen.

[Gargi-jais11]

And for keyValueHandler.quasiCloseContainer this also does not require refetching as this will only be called when container state is in CLOSING. DiskBalancer avoids CLOSING so no updation of container set, no race.

[ChenSammi]

The last patch looks good to me.

[Gargi-jais11]

@sodonnel would you like to take another look?

[Gargi-jais11]

Thanks @ChenSammi, @sodonnel and @adoroszlai for reviewing the patch.