[
https://issues.apache.org/jira/browse/HDDS-16465?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18116272#comment-18116272
]
Andy Huang commented on HDDS-16465:
-----------------------------------
[~smeng] Could you review the PR, please?
> 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
> Assignee: Andy Huang
> Priority: Critical
> Labels: pull-request-available
> 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]
