[
https://issues.apache.org/jira/browse/HDDS-16465?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16465:
------------------------------
Description:
h3. Finding
If the named masks were absent, the non-atomic dual-chain update / restart
rebuild could give a surviving snapshot a different effective previous and
misdirect the adjacency reclaim filter, physically reclaiming a still
chain-visible key: externally observable data loss on snapshot reads with no
automatic recovery. Reachable in principle from concurrent purge plus restart
during snapshot GC. Currently masked by atomic single-batch purge persistence,
the fail-closed corruption gate on restart rebuild, and reclaim-time
re-validation (validatePreviousSnapshotId /
validateExistingLastNSnapshotsInChain under SNAPSHOT_GC_LOCK plus the flush
gate).
h3. Classification
* Verdict: MASKED
* Severity: Critical
* Source: Specula TLA+ model checking and confirmation debate, finding CR-4
h3. Reproduce
{noformat}
Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
Specula: v1.1.0 (commit c6aa3dfa)
Target: om-snapshot-gc
Guidance:
campaigns/ozone-9fbf9ee/targets/008-om-snapshot-gc/.prompt-extra.md
{noformat}
{code:none}
specula run --agent=claude-code --effort=high --keep-original --max-parallel=2 \
--enable-reviews --confirm-debate --tlc-memory-limit=28G --tlc-worker-limit=8
\
"om-snapshot-gc|apache/ozone|Java|Use the target-specific .prompt-extra.md"
{code}
Discovered under HDDS-15927 (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
1. SnapshotChainManager keeps two in-memory chains that must agree: the global
chain (all snapshots in creation order) and the per-path chain (snapshots for a
given volume/bucket). GC and reclaim navigate them via previous/next-snapshot
lookups.
2. A snapshot delete calls deleteSnapshot, which ran deleteSnapshotGlobal(id)
&& deleteSnapshotPath(path, id). Because of the short-circuit &&, if the global
delete commits (returns true) but the path delete then returns false or throws,
the global chain is mutated while the path chain is not. If the snapshot is
missing from the global chain, the path delete is never attempted at all.
3. Either way the two chains diverge: one chain no longer contains the snapshot
while the other still links through it. Subsequent previous/next-snapshot
navigation returns inconsistent results, which can misdirect reclaim decisions.
4. addSnapshot had the mirror-image gap: it committed the global insertion
before attempting the path insertion, so a failing path insertion left the
snapshot stranded in the global chain.
5. Independently, latestGlobalSnapshotId and oldestGlobalSnapshotId were
written only under the instance monitor but read by
getLatestGlobalSnapshotId/getOldestGlobalSnapshotId (and the chain iterator)
without holding it, so a concurrent reader could observe a stale value.
h3. Root cause
The dual-chain updates in addSnapshot and deleteSnapshot were not atomic and
had no rollback, and deleteSnapshot's short-circuit && could skip or half-apply
an update, so a failure or absence in one chain left the two chains
inconsistent. The two global head/tail fields were also published without the
memory visibility guarantee their unsynchronized getters rely on.
h3. Suggested fix
Make both chain mutations all-or-nothing. deleteSnapshot now checks membership
in both chains first and refuses a partial delete (throws) when a snapshot is
present in only one of them, and rolls back the global deletion
(restoreSnapshotGlobal) if the path deletion fails. addSnapshot rolls back the
global insertion if the path insertion fails. latestGlobalSnapshotId and
oldestGlobalSnapshotId are made volatile so the unsynchronized getters observe
the last committed value. Two invariant unit tests are added: a failing path
insertion leaves both chains empty (add rollback), and a delete whose path does
not match is refused and leaves the global chain unchanged. See
[^HDDS-16465.001.patch].
Note: the underlying trigger is a concurrency window (a failing or asymmetric
chain update, or a reader racing a writer) that is timing dependent and not
deterministically reproducible without a thread-scheduling seam that shipping
code does not expose. The added tests pin the atomicity and partial-update
invariants directly rather than reproducing a specific interleaving; the
memory-visibility change is asserted by inspection.
Patch is an AI-drafted proposal (Specula + Claude); pending human build, full
test, and review before merge.
was:
h3. Finding
If the named masks were absent, the non-atomic dual-chain update / restart
rebuild could give a surviving snapshot a different effective previous and
misdirect the adjacency reclaim filter, physically reclaiming a still
chain-visible key: externally observable data loss on snapshot reads with no
automatic recovery. Reachable in principle from concurrent purge plus restart
during snapshot GC. Currently masked by atomic single-batch purge persistence,
the fail-closed corruption gate on restart rebuild, and reclaim-time
re-validation (validatePreviousSnapshotId /
validateExistingLastNSnapshotsInChain under SNAPSHOT_GC_LOCK plus the flush
gate).
h3. Classification
* Verdict: MASKED
* Severity: Critical
* Source: Specula TLA+ model checking and confirmation debate, finding CR-4
h3. Reproduce
{noformat}
Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
Specula: v1.1.0 (commit c6aa3dfa)
Target: om-snapshot-gc
Guidance:
campaigns/ozone-9fbf9ee/targets/008-om-snapshot-gc/.prompt-extra.md
{noformat}
{code:none}
specula run --agent=claude-code --effort=high --keep-original --max-parallel=2 \
--enable-reviews --confirm-debate --tlc-memory-limit=28G --tlc-worker-limit=8
\
"om-snapshot-gc|apache/ozone|Java|Use the target-specific .prompt-extra.md"
{code}
Discovered under HDDS-15927 (Specula TLA+ verification effort). The TLA+
specification, counterexample, and confirmation debate live in the Specula run
artifacts.
Generated with Specula (Claude Opus 4.8).
> Non atomic snapshot chain update could misdirect the adjacency reclaim filter
> -----------------------------------------------------------------------------
>
> Key: HDDS-16465
> URL: https://issues.apache.org/jira/browse/HDDS-16465
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Critical
> Attachments: HDDS-16465.001.patch
>
>
> h3. Finding
> If the named masks were absent, the non-atomic dual-chain update / restart
> rebuild could give a surviving snapshot a different effective previous and
> misdirect the adjacency reclaim filter, physically reclaiming a still
> chain-visible key: externally observable data loss on snapshot reads with no
> automatic recovery. Reachable in principle from concurrent purge plus restart
> during snapshot GC. Currently masked by atomic single-batch purge
> persistence, the fail-closed corruption gate on restart rebuild, and
> reclaim-time re-validation (validatePreviousSnapshotId /
> validateExistingLastNSnapshotsInChain under SNAPSHOT_GC_LOCK plus the flush
> gate).
> h3. Classification
> * Verdict: MASKED
> * Severity: Critical
> * Source: Specula TLA+ model checking and confirmation debate, finding CR-4
> h3. Reproduce
> {noformat}
> Ozone commit: 9fbf9ee0cb1bd2f5f5d437b6719ebbe5309351fb
> Specula: v1.1.0 (commit c6aa3dfa)
> Target: om-snapshot-gc
> Guidance:
> campaigns/ozone-9fbf9ee/targets/008-om-snapshot-gc/.prompt-extra.md
> {noformat}
> {code:none}
> specula run --agent=claude-code --effort=high --keep-original
> --max-parallel=2 \
> --enable-reviews --confirm-debate --tlc-memory-limit=28G
> --tlc-worker-limit=8 \
> "om-snapshot-gc|apache/ozone|Java|Use the target-specific .prompt-extra.md"
> {code}
> Discovered under HDDS-15927 (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
> 1. SnapshotChainManager keeps two in-memory chains that must agree: the
> global chain (all snapshots in creation order) and the per-path chain
> (snapshots for a given volume/bucket). GC and reclaim navigate them via
> previous/next-snapshot lookups.
> 2. A snapshot delete calls deleteSnapshot, which ran deleteSnapshotGlobal(id)
> && deleteSnapshotPath(path, id). Because of the short-circuit &&, if the
> global delete commits (returns true) but the path delete then returns false
> or throws, the global chain is mutated while the path chain is not. If the
> snapshot is missing from the global chain, the path delete is never attempted
> at all.
> 3. Either way the two chains diverge: one chain no longer contains the
> snapshot while the other still links through it. Subsequent
> previous/next-snapshot navigation returns inconsistent results, which can
> misdirect reclaim decisions.
> 4. addSnapshot had the mirror-image gap: it committed the global insertion
> before attempting the path insertion, so a failing path insertion left the
> snapshot stranded in the global chain.
> 5. Independently, latestGlobalSnapshotId and oldestGlobalSnapshotId were
> written only under the instance monitor but read by
> getLatestGlobalSnapshotId/getOldestGlobalSnapshotId (and the chain iterator)
> without holding it, so a concurrent reader could observe a stale value.
> h3. Root cause
> The dual-chain updates in addSnapshot and deleteSnapshot were not atomic and
> had no rollback, and deleteSnapshot's short-circuit && could skip or
> half-apply an update, so a failure or absence in one chain left the two
> chains inconsistent. The two global head/tail fields were also published
> without the memory visibility guarantee their unsynchronized getters rely on.
> h3. Suggested fix
> Make both chain mutations all-or-nothing. deleteSnapshot now checks
> membership in both chains first and refuses a partial delete (throws) when a
> snapshot is present in only one of them, and rolls back the global deletion
> (restoreSnapshotGlobal) if the path deletion fails. addSnapshot rolls back
> the global insertion if the path insertion fails. latestGlobalSnapshotId and
> oldestGlobalSnapshotId are made volatile so the unsynchronized getters
> observe the last committed value. Two invariant unit tests are added: a
> failing path insertion leaves both chains empty (add rollback), and a delete
> whose path does not match is refused and leaves the global chain unchanged.
> See [^HDDS-16465.001.patch].
> Note: the underlying trigger is a concurrency window (a failing or asymmetric
> chain update, or a reader racing a writer) that is timing dependent and not
> deterministically reproducible without a thread-scheduling seam that shipping
> code does not expose. The added tests pin the atomicity and partial-update
> invariants directly rather than reproducing a specific interleaving; the
> memory-visibility change is asserted by inspection.
> 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]
