[
https://issues.apache.org/jira/browse/HDDS-16451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16451:
------------------------------
Description:
h3. Finding
Cross-lock lost update of a shared ContainerInfo sequenceId (report path under
synchronized(containerInfo), Ratis-apply path under striped writeLock(id)) is a
real race but its harm is masked: for OPEN/QUASI_CLOSED the periodic report
resend re-raises the value via max(), and the durable/follower value is
serialized under writeLock so an in-memory clobber cannot corrupt it. Internal
invariant violation with downstream risk (an understated in-memory seqId
feeding a resurrection/close decision in a narrow window) but no consumer
observed the wrong outcome.
h3. Classification
* Verdict: MASKED
* Severity: Medium
* Source: Specula TLA+ model checking and confirmation debate, finding CR-4
h3. Reproduce
{noformat}
Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
Specula: v1.1.0 (commit c6aa3dfa)
Target: scm-container-lifecycle
Guidance:
campaigns/ozone-9fbf9ee/targets/015-scm-container-lifecycle/.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-container-lifecycle|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
This is a latent (masked) defect. The lost update is real but its harm is
masked for the common cases; a lasting wrong value needs the unmasking window
below.
1. On the leader SCM a report-processing thread runs
AbstractContainerReportHandler.processContainerReplica under
synchronized(containerInfo) and calls
containerInfo.updateSequenceId(replicaBcsid).
2. Concurrently the Ratis state-machine apply thread runs
ContainerStateManagerImpl.updateContainerStateWithSequenceId under the striped
writeLock(id) and calls containerInfo.updateSequenceId(sequenceId) on the same
ContainerInfo instance.
3. updateSequenceId is a non-atomic read-modify-write (sequenceId = max(param,
sequenceId)) on a non-volatile long. The two lock domains (object monitor vs
striped read-write lock) do not exclude each other, so the two updates
interleave: one thread reads the old value, the other writes the higher value,
and the first thread then writes back its lower max, clobbering the higher
value.
Unmasking window: for OPEN / QUASI_CLOSED containers the periodic
full/incremental container report resend re-invokes updateSequenceId and
re-raises the value via max(), and the durable/follower value is written under
the writeLock, so an in-memory clobber self-corrects. The residual un-masked
case (not injectable in a plain unit test) is a CLOSED container that is frozen
at a clobbered sequenceId at close time, which then feeds a resurrection/close
or replica-deletion decision keyed on that value.
h3. Root cause
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java:
updateSequenceId performs a plain read-modify-write on a non-volatile long
field, while the same instance is updated under two different locks (a
documented TODO in AbstractContainerReportHandler notes the lock discipline is
not unified). The RMW is therefore neither atomic nor guaranteed visible across
threads, so a concurrent higher value can be lost.
h3. Suggested fix
Make the max update atomic without introducing a new lock. Mark sequenceId
volatile (for cross-thread read visibility) and perform the read-modify-write
through a lock-free AtomicLongFieldUpdater.accumulateAndGet(this, sequenceID,
Math::max), which retries via compare-and-set until the stored value is the
maximum. A lock-free CAS is used rather than synchronizing updateSequenceId on
the ContainerInfo monitor, because the report path already holds that monitor
and can acquire writeLock(id) beneath it while the apply path acquires
writeLock(id) first, so taking the monitor inside updateSequenceId could
deadlock. A concurrency invariant test asserts that after many concurrent
updates the stored value equals the maximum of all inputs. See
[^HDDS-16451.001.patch].
Patch is an AI-drafted proposal (Specula + Claude); pending human build, full
test, and review before merge.
was:
h3. Finding
Cross-lock lost update of a shared ContainerInfo sequenceId (report path under
synchronized(containerInfo), Ratis-apply path under striped writeLock(id)) is a
real race but its harm is masked: for OPEN/QUASI_CLOSED the periodic report
resend re-raises the value via max(), and the durable/follower value is
serialized under writeLock so an in-memory clobber cannot corrupt it. Internal
invariant violation with downstream risk (an understated in-memory seqId
feeding a resurrection/close decision in a narrow window) but no consumer
observed the wrong outcome.
h3. Classification
* Verdict: MASKED
* Severity: Medium
* Source: Specula TLA+ model checking and confirmation debate, finding CR-4
h3. Reproduce
{noformat}
Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
Specula: v1.1.0 (commit c6aa3dfa)
Target: scm-container-lifecycle
Guidance:
campaigns/ozone-9fbf9ee/targets/015-scm-container-lifecycle/.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-container-lifecycle|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).
> Cross lock lost update of ContainerInfo sequenceId (masked by report resend
> and writeLock)
> ------------------------------------------------------------------------------------------
>
> Key: HDDS-16451
> URL: https://issues.apache.org/jira/browse/HDDS-16451
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Minor
> Attachments: HDDS-16451.001.patch
>
>
> h3. Finding
> Cross-lock lost update of a shared ContainerInfo sequenceId (report path
> under synchronized(containerInfo), Ratis-apply path under striped
> writeLock(id)) is a real race but its harm is masked: for OPEN/QUASI_CLOSED
> the periodic report resend re-raises the value via max(), and the
> durable/follower value is serialized under writeLock so an in-memory clobber
> cannot corrupt it. Internal invariant violation with downstream risk (an
> understated in-memory seqId feeding a resurrection/close decision in a narrow
> window) but no consumer observed the wrong outcome.
> h3. Classification
> * Verdict: MASKED
> * Severity: Medium
> * Source: Specula TLA+ model checking and confirmation debate, finding CR-4
> h3. Reproduce
> {noformat}
> Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
> Specula: v1.1.0 (commit c6aa3dfa)
> Target: scm-container-lifecycle
> Guidance:
> campaigns/ozone-9fbf9ee/targets/015-scm-container-lifecycle/.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-container-lifecycle|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
> This is a latent (masked) defect. The lost update is real but its harm is
> masked for the common cases; a lasting wrong value needs the unmasking window
> below.
> 1. On the leader SCM a report-processing thread runs
> AbstractContainerReportHandler.processContainerReplica under
> synchronized(containerInfo) and calls
> containerInfo.updateSequenceId(replicaBcsid).
> 2. Concurrently the Ratis state-machine apply thread runs
> ContainerStateManagerImpl.updateContainerStateWithSequenceId under the
> striped writeLock(id) and calls containerInfo.updateSequenceId(sequenceId) on
> the same ContainerInfo instance.
> 3. updateSequenceId is a non-atomic read-modify-write (sequenceId =
> max(param, sequenceId)) on a non-volatile long. The two lock domains (object
> monitor vs striped read-write lock) do not exclude each other, so the two
> updates interleave: one thread reads the old value, the other writes the
> higher value, and the first thread then writes back its lower max, clobbering
> the higher value.
> Unmasking window: for OPEN / QUASI_CLOSED containers the periodic
> full/incremental container report resend re-invokes updateSequenceId and
> re-raises the value via max(), and the durable/follower value is written
> under the writeLock, so an in-memory clobber self-corrects. The residual
> un-masked case (not injectable in a plain unit test) is a CLOSED container
> that is frozen at a clobbered sequenceId at close time, which then feeds a
> resurrection/close or replica-deletion decision keyed on that value.
> h3. Root cause
> hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java:
> updateSequenceId performs a plain read-modify-write on a non-volatile long
> field, while the same instance is updated under two different locks (a
> documented TODO in AbstractContainerReportHandler notes the lock discipline
> is not unified). The RMW is therefore neither atomic nor guaranteed visible
> across threads, so a concurrent higher value can be lost.
> h3. Suggested fix
> Make the max update atomic without introducing a new lock. Mark sequenceId
> volatile (for cross-thread read visibility) and perform the read-modify-write
> through a lock-free AtomicLongFieldUpdater.accumulateAndGet(this, sequenceID,
> Math::max), which retries via compare-and-set until the stored value is the
> maximum. A lock-free CAS is used rather than synchronizing updateSequenceId
> on the ContainerInfo monitor, because the report path already holds that
> monitor and can acquire writeLock(id) beneath it while the apply path
> acquires writeLock(id) first, so taking the monitor inside updateSequenceId
> could deadlock. A concurrency invariant test asserts that after many
> concurrent updates the stored value equals the maximum of all inputs. See
> [^HDDS-16451.001.patch].
> 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]
