[
https://issues.apache.org/jira/browse/HDDS-16445?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16445:
------------------------------
Description:
h3. Finding
A split-lock read in processOverReplicatedContainer issues an extra delete
against a live replica, dropping a CLOSED RF=3 container below RF
(DurableReplicationFloor safety violation). Reachable from normal concurrent
over-replication processing racing a container-report handler; harm is bounded
because a later cycle re-replicates, so High rather than Critical.
h3. Classification
* Verdict: REPRODUCED
* Severity: High
* Source: Specula TLA+ model checking and confirmation debate, finding MC-1
h3. Reproduce
{noformat}
Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
Specula: v1.1.0 (commit c6aa3dfa)
Target: scm-ratis-replication
Guidance:
campaigns/ozone-9fbf9ee/targets/012-scm-ratis-replication/.prompt-extra.md
{noformat}
{code:none}
specula run --agent=claude-code --effort=medium --keep-original
--max-parallel=2 \
--enable-reviews --confirm-debate --tlc-memory-limit=28G --tlc-worker-limit=8
\
"scm-ratis-replication|apache/ozone|Java|Use the target-specific
.prompt-extra.md"
{code}
Discovered under HDDS-16431 (Specula TLA+ verification effort). The TLA+
specification, counterexample, and confirmation debate live in the Specula run
artifacts.
Generated with Specula (Claude Opus 4.8).
h3. Steps to reproduce
Precondition: a CLOSED Ratis container (RF=3) with 4 healthy replicas
(legitimately over-replicated by 1). A delete for one replica is already in
flight and tracked as a pending DELETE op.
Interleaving (two SCM threads, the ReplicationManager over-replication
processor and the container-report / ICR handler):
1. ReplicationManager.processOverReplicatedContainer reads the replica set: 4
replicas.
2. The in-flight delete is confirmed:
ContainerStateManagerImpl.removeContainerReplica removes the replica AND
completes its pending DELETE op, atomically under the container-state-manager
lock.
3. processOverReplicatedContainer then reads the pending ops: the delete is
gone (empty list).
Observed wrong outcome: the handler sees 4 replicas and 0 pending deletes,
computes an excess of 1, and issues an extra delete against a still-live
replica. Only 3 replicas physically remain, so applying that command drops the
container to 2 replicas, below the replication factor (a
DurableReplicationFloor safety violation). A later replication cycle
re-replicates, so the harm is bounded but real.
h3. Root cause
processOverReplicatedContainer
(hadoop-hdds/server-scm/.../ReplicationManager.java) reads the replica set
(guarded by the container-state-manager striped lock) and the pending ops
(guarded by the separate ContainerReplicaPendingOps striped lock) with no
enclosing lock and no re-validation. A confirmed delete removes the replica and
completes its pending DELETE op atomically
(ContainerStateManagerImpl.removeContainerReplica calls completeDeleteReplica
while holding the container-state-manager write lock), but that atomic unit can
land in the gap between the two unsynchronized reads. The resulting view still
counts the removed replica while its in-flight delete has vanished from the
pending set, so RatisContainerReplicaCount.getExcessRedundancy is inflated by
one and an extra delete is issued.
h3. Suggested fix
The attached patch [^HDDS-16445.001.patch] reads the pending ops before the
replica set in processOverReplicatedContainer. Because a confirmed delete
removes the replica and its pending DELETE op together, reading pending ops
first guarantees that if such a delete lands between the two reads the view is
either fully consistent or conservative (a still-pending delete for a replica
that has already been removed), never the dangerous combination of a stale
replica counted while its in-flight delete has disappeared. In the conservative
case the still-tracked delete keeps the effective count at the replication
factor, so no extra delete is scheduled; a later cycle removes any genuine
excess. A test is added to TestReplicationManager that models the confirmed
delete landing at the replica read and asserts no extra delete command is
issued (it fails before this change, issuing one over-delete). A fully atomic
snapshot of both structures under a single lock is a heavier alternative that
would change the ContainerManager read interface.
Patch is an AI-drafted proposal (Specula + Claude); pending human build, full
test, and review before merge.
was:
h3. Finding
A split-lock read in processOverReplicatedContainer issues an extra delete
against a live replica, dropping a CLOSED RF=3 container below RF
(DurableReplicationFloor safety violation). Reachable from normal concurrent
over-replication processing racing a container-report handler; harm is bounded
because a later cycle re-replicates, so High rather than Critical.
h3. Classification
* Verdict: REPRODUCED
* Severity: High
* Source: Specula TLA+ model checking and confirmation debate, finding MC-1
h3. Reproduce
{noformat}
Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
Specula: v1.1.0 (commit c6aa3dfa)
Target: scm-ratis-replication
Guidance:
campaigns/ozone-9fbf9ee/targets/012-scm-ratis-replication/.prompt-extra.md
{noformat}
{code:none}
specula run --agent=claude-code --effort=medium --keep-original
--max-parallel=2 \
--enable-reviews --confirm-debate --tlc-memory-limit=28G --tlc-worker-limit=8
\
"scm-ratis-replication|apache/ozone|Java|Use the target-specific
.prompt-extra.md"
{code}
Discovered under HDDS-16431 (Specula TLA+ verification effort). The TLA+
specification, counterexample, and confirmation debate live in the Specula run
artifacts.
Generated with Specula (Claude Opus 4.8).
> SCM over replication split lock read can delete a live replica and drop a
> CLOSED container below RF
> ---------------------------------------------------------------------------------------------------
>
> Key: HDDS-16445
> URL: https://issues.apache.org/jira/browse/HDDS-16445
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments: HDDS-16445.001.patch
>
>
> h3. Finding
> A split-lock read in processOverReplicatedContainer issues an extra delete
> against a live replica, dropping a CLOSED RF=3 container below RF
> (DurableReplicationFloor safety violation). Reachable from normal concurrent
> over-replication processing racing a container-report handler; harm is
> bounded because a later cycle re-replicates, so High rather than Critical.
> h3. Classification
> * Verdict: REPRODUCED
> * Severity: High
> * Source: Specula TLA+ model checking and confirmation debate, finding MC-1
> h3. Reproduce
> {noformat}
> Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
> Specula: v1.1.0 (commit c6aa3dfa)
> Target: scm-ratis-replication
> Guidance:
> campaigns/ozone-9fbf9ee/targets/012-scm-ratis-replication/.prompt-extra.md
> {noformat}
> {code:none}
> specula run --agent=claude-code --effort=medium --keep-original
> --max-parallel=2 \
> --enable-reviews --confirm-debate --tlc-memory-limit=28G
> --tlc-worker-limit=8 \
> "scm-ratis-replication|apache/ozone|Java|Use the target-specific
> .prompt-extra.md"
> {code}
> Discovered under HDDS-16431 (Specula TLA+ verification effort). The TLA+
> specification, counterexample, and confirmation debate live in the Specula
> run artifacts.
> Generated with Specula (Claude Opus 4.8).
> h3. Steps to reproduce
> Precondition: a CLOSED Ratis container (RF=3) with 4 healthy replicas
> (legitimately over-replicated by 1). A delete for one replica is already in
> flight and tracked as a pending DELETE op.
> Interleaving (two SCM threads, the ReplicationManager over-replication
> processor and the container-report / ICR handler):
> 1. ReplicationManager.processOverReplicatedContainer reads the replica set: 4
> replicas.
> 2. The in-flight delete is confirmed:
> ContainerStateManagerImpl.removeContainerReplica removes the replica AND
> completes its pending DELETE op, atomically under the container-state-manager
> lock.
> 3. processOverReplicatedContainer then reads the pending ops: the delete is
> gone (empty list).
> Observed wrong outcome: the handler sees 4 replicas and 0 pending deletes,
> computes an excess of 1, and issues an extra delete against a still-live
> replica. Only 3 replicas physically remain, so applying that command drops
> the container to 2 replicas, below the replication factor (a
> DurableReplicationFloor safety violation). A later replication cycle
> re-replicates, so the harm is bounded but real.
> h3. Root cause
> processOverReplicatedContainer
> (hadoop-hdds/server-scm/.../ReplicationManager.java) reads the replica set
> (guarded by the container-state-manager striped lock) and the pending ops
> (guarded by the separate ContainerReplicaPendingOps striped lock) with no
> enclosing lock and no re-validation. A confirmed delete removes the replica
> and completes its pending DELETE op atomically
> (ContainerStateManagerImpl.removeContainerReplica calls completeDeleteReplica
> while holding the container-state-manager write lock), but that atomic unit
> can land in the gap between the two unsynchronized reads. The resulting view
> still counts the removed replica while its in-flight delete has vanished from
> the pending set, so RatisContainerReplicaCount.getExcessRedundancy is
> inflated by one and an extra delete is issued.
> h3. Suggested fix
> The attached patch [^HDDS-16445.001.patch] reads the pending ops before the
> replica set in processOverReplicatedContainer. Because a confirmed delete
> removes the replica and its pending DELETE op together, reading pending ops
> first guarantees that if such a delete lands between the two reads the view
> is either fully consistent or conservative (a still-pending delete for a
> replica that has already been removed), never the dangerous combination of a
> stale replica counted while its in-flight delete has disappeared. In the
> conservative case the still-tracked delete keeps the effective count at the
> replication factor, so no extra delete is scheduled; a later cycle removes
> any genuine excess. A test is added to TestReplicationManager that models the
> confirmed delete landing at the replica read and asserts no extra delete
> command is issued (it fails before this change, issuing one over-delete). A
> fully atomic snapshot of both structures under a single lock is a heavier
> alternative that would change the ContainerManager read interface.
> Patch is an AI-drafted proposal (Specula + Claude); pending human build, full
> test, and review before merge.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
