[k-morozov]

1'000 parts were inserted:

  ┌──────────────┬──────────┬───────┬────────┬───────────┐
  │    Metric    │ Baseline │ Patch │ Delta  │ Reduction │
  ├──────────────┼──────────┼───────┼────────┼───────────┤
  │ S3 GetObject │    7,997 │ 6,997 │ -1,000 │    12.50% │
  └──────────────┴──────────┴───────┴────────┴───────────┘

Changelog category (leave one):

• Performance Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Reduced CAS object storage requests by reusing the proof recorded when a manifest is staged. Promotion no longer reads the manifest back, and cleanup deletes abandoned staged manifests using the recorded incarnation without a preliminary HEAD request.
Fixed a race between CAS manifest staging and orphan cleanup that could delete a committed manifest and leave a dangling reference.

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Unit tests
• Performance tests
• Aarch64 tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• CAS (content-addressed storage; Antalya only)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• OAuth (5m)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [08a9087]

[alsugiliazova]

PR #2528 Audit Review

AI audit note: This review was generated by AI (Claude Opus 5).

Verdict

No High-severity defect. The central safety argument holds, and the orphan-sweep change that makes it hold is the right companion fix.

Two Medium findings about what the pool leaks and what it no longer detects, and four Low findings.

What the change does

promote used to read the precommit manifest body back and revalidate it (RefMatchesBody,
ManifestNamespaceMatches) before every commit. That GET is the 1,000 requests the PR's
measurement table removes over 1,000 inserted parts.

The PR replaces it with a proof the writer already holds. A successful stageManifest now records,
per staged ManifestId, a StagedManifest{Etag incarnation, std::vector<BlobRef> blob_dependencies}
— the incarnation confirmed by the conditional create, and the blob leaves in the order the caller
passed them. promote looks the id up in that map: a hit skips the read entirely and feeds the
recorded dependencies to the append closure; a miss (a manifest this transaction did not stage) takes
the old read-and-validate path unchanged.

Two things fall out of the same record:

• precommitAdd's A3 ABA barrier now checks staged_manifests.contains(id) instead of the separate
  staged_manifest_ids set, which is deleted.
• cleanupStagedManifestDebrisBestEffort deletes by the recorded incarnation
  (op.remove(staged.incarnation.key(), staged.incarnation, …)) instead of head-then-remove,
  dropping one HEAD per abandoned staged manifest.

Model

Actors	The writer's PartWriteTxn, the GC leader's orphan manifest sweep, and the object store
Channels	Conditional PUT/DELETE on cas/manifests/<ns>/<epoch>/<seq>/<ordinal>, the ref-log append lane, and the sweep's LIST plus per-namespace mount-lease GET
Authoritative state	The write-once manifest body and its incarnation; the mount lease's (writer_epoch, min_active_build_sequence) pair
Failure model assumed by the code	Crash-recover processes, unbounded store latency, read-after-write consistency on a single key, a fence generation that every request re-checks
Safe failure criterion	No committed ref ever names a manifest that is not there

The central argument, and why it holds

Trusting an in-memory proof needs exactly one thing: nothing in the protocol may delete a staged
body before its promote.

The PR earns this by moving the floor gate in planManifestCursorPage ahead of the catalog lookup.
A listed manifest key is now retained unless isBuildPrefixCoveredByFloor proves its build prefix
retired. Before the PR, a candidate whose namespace had no row in the catalog cut was deleted as
"dead-life debris" with no floor read at all — the deleted test
CursorPageDeletesObservedBodyWhenCatalogOmitsNamespace asserted exactly that. Under the new
promote that path would have been a silent commit over a deleted manifest, so closing it is not
cleanup, it is the fix.

A live build cannot be covered by the floor. build_sequence is monotone within an epoch, so an
active build's sequence is always >= min_active_build_sequence, and the epoch arm only fires after
the mount has been re-claimed or decommissioned — at which point the old transaction's appendRefOps
is refused by the fence anyway, so the commit cannot land.

The second half of the sweep change covers the window the first half opens. The floor read now
happens after the catalog cut for namespaces that previously never took one, so a namespace could be
born in between. The new post-loop re-read discards any candidate whose catalog row differs between
the cut and post_floor_catalog:

if (!candidates.empty())
{
    const CasRefCatalog::Snapshot post_floor_catalog = CasRefCatalog::read(op, layout);
    post_floor_catalog.life_index.throwIfAmbiguous("CAS orphan manifest sweep");
    std::erase_if(candidates, [&](const ListedManifestObject & candidate) { … });
}

CursorPageRetainsRowlessBodyWhenNamespaceAppearsAfterCatalogCut and
CursorPageRetainsBodyWhenNamespaceIsRebornAfterCatalogCut drive both shapes through a backend that
mutates the catalog from inside the mount-lease read.

I checked the other deletion paths — sweepNamespace, Gc::fold, decommissionPoolMember,
cancelForNamespaceRemoval — and none can reach a pre-precommit staged body of a live build prefix.

Findings

Recovery and rollback

Medium: pre-precommit manifest debris becomes permanently unreclaimable when the server root has no
mount body.

• Impact — the objects are never collected by any automatic path, and nothing counts them.
• Anchor — planManifestCursorPage (Gc/CasOrphanManifestSweep.cpp), the floor gate that now
  precedes catalogEntryOf; mountLeaseForNamespace in the same file.
• Trigger — a pool member removed without decommissionPoolMember, or a renamed
  server_root_id, leaving manifest keys whose namespace server root has no mount slot.
• Why defect — mountLeaseForNamespace returns nullopt when no mount object exists under any
  prefix of the namespace, and nullopt covers nothing, so the sweep retains forever. The replaced
  test said this case was reachable in so many words ("after legal catalog-row deletion there may be
  no server-root state left to supply one"); its successor,
  CursorPageRetainsRowlessBodyWithoutMountLease, asserts deleted == 0. The retention itself is
  the correct conservative choice — the problem is that it is silent. The path only does
  ++result.skipped, with none of the retained_no_coverage / retained_hold /
  retained_work_budget classes that every other retain reason gets, so an operator has no way to
  see the pool accumulating objects nothing will ever collect.
• Fix direction — add a retained_no_mount_lease counter to ManifestSweepResult and a periodic
  warning; state in the decommission docs that skipping decommission strands manifest debris.

Partial and gray failure

Medium: an out-of-protocol body deletion now produces a committed ref to a missing manifest instead
of failing closed.

• Impact — the part is published and the failure first appears much later on the read path as
  FILE_DOESNT_EXIST, with fsck reporting the reference as dangling.
• Anchor — PartWriteTxn::promote (Pool/CasPartWriteTxn.cpp), the staged_manifests.find(id)
  branch.
• Trigger — a bucket lifecycle or expiry rule on cas/manifests/, or a manual delete, landing
  between a confirmed stageManifest and its promote.
• Why defect — the GET plus refMatchesBody / manifestNamespaceMatches used to run before
  commit_state = CommitState::Uncertain, so a vanished body was a clean proven negative and the
  relink receiver fell back to a byte fetch. On the staged path there is now no check at all. The PR
  declares the hazard and names the trigger in manifests-and-refs.md, which is why this is not
  High — but that declaration is the only thing standing between an operator's lifecycle rule and
  silently dangling refs.
• Fix direction — give it a detectable form: an fsck class for a committed ref whose manifest
  body is absent, or a mount-open check that the pool prefix carries no expiry rule.

Backpressure and resource limits

Low: rowless namespaces now consume the per-page namespace work budget.

• Anchor — planManifestCursorPage, the if (inserted) block, which used to sit inside
  if (catalog_entry) and now runs for every namespace.
• Why — work_budget->sweep_namespaces_used is spent on namespaces that need no protection view
  at all. A page dominated by rowless debris can exhaust the budget and retain catalog-named
  namespaces that genuinely needed it.
• Fix direction — charge the budget only on the branch that actually builds a view
  (if (catalog_entry)).

Low: per-transaction memory now scales with staged entries.

• Anchor — PartWriteTxn::staged_manifests (Pool/CasPartWriteTxn.h).
• Why — the map holds a std::vector<BlobRef> per staged manifest and, by design, never erases
  (membership is the A3 ABA barrier). A build that re-stages after retries accumulates a full
  dependency list per attempt, bounded only by kMaxManifestOrdinal (999,999) and
  kMaxManifestEntries (1,048,576). Previously each stage cost one ManifestId.
• Fix direction — if this ever matters, split the barrier (ids only, kept forever) from the
  promote proof (dependencies, droppable once the id is committed or abandoned). The current header
  comment correctly forbids erasing the combined entry, so the split has to come first.

Observability and diagnosability

Low: the GC side pays back part of the writer-side saving, and the PR does not measure it.

• Anchor — the post_floor_catalog read in planManifestCursorPage, and the floor lookup that
  now fires ahead of the catalog check.
• Why — each page with candidates costs one extra CasRefCatalog::read, and namespaces that
  previously cost no floor lookup now take one. The PR's measurement table covers the writer path
  only, so the net pool-wide request delta is unknown.
• Fix direction — report floor_lookups / floor_reads deltas from a GC round in the PR body.

Low: blob dependency checks run in caller order, not path-sorted order.

• Anchor — blobDependenciesOf(body.entries) in stageManifest, versus the sorted body a decode
  produces on the non-staged path.
• Why — this changes which blob the "no dependency proof at commit" LOGICAL_ERROR names, and
  the order of BlobReuseAdopt events, so the staged and non-staged paths are no longer
  log-comparable. Documented in manifests-and-refs.md; no correctness consequence.

Checked and clean

• std::hash<ManifestId> is specialized in CasTypes.h, so the switch from std::set to
  std::unordered_map compiles and keys correctly.
• Etag::key() exists (Backend/CasEtag.h) and the Conflict arm of stagingPutIfAbsent yields an
  incarnation for the same key, so the exact-incarnation delete in
  cleanupStagedManifestDebrisBestEffort is strictly safer than the old HEAD-then-delete: a
  write-once key cannot change incarnation, and a conditional delete refuses a mismatch instead of
  removing a stranger's object.
• Removal is exactly {Removed, Gone, Mismatch} (Backend/CasWriteResult.h), so the new log
  line's Gone ? "gone" : "incarnation mismatch" ternary is exhaustive and accurate.
• blobDependenciesOf(body.entries) reads body.entries after encodePartManifest(body) and before
  the PUT, so it is not reading a moved-from vector.
• Skipping refMatchesBody / manifestNamespaceMatches on the staged path loses nothing: the body
  was built with body.ref = ref and body.root_namespace_id = owning_ns from the same
  ManifestId, and promote still rejects id.root_namespace != target_ns up front.
• Moving validateManifestEntryPath into encodePartManifest cannot reject anything the decode side
  would have accepted, so no legacy manifest and no relink offer breaks. It does tighten
  stageManifest, which the new StageManifestRejectsTraversalPathBeforeBodyWrite test locks in
  along with the "no object written" property.
• The relink promote-outcome consumer is unaffected. PreparedRelinkOverPartWrite::promote
  (ContentAddressedMetadataStorage.cpp) distinguishes Unresolved from
  MechanismFallbackAllowed through commitIsUnresolved(), which reads
  commit_state == CommitState::Uncertain. Removing a pre-append I/O can only reduce the
  Unresolved cases, never add one. The matching edit to replication.md (dropping "body-absent
  precommit" from the proven-not-committed list) is correct, because the relink receiver stages its
  own manifest in prepareEntries and therefore always takes the no-read path.

Test gap

Every new test is unit-level and single-threaded. The invariant the PR actually rests on — a sweep
round running concurrently with a live stageManifest/promote never deletes the body — has no
test. CursorPageRetainsRowlessBodyOfActiveBuild proves the floor predicate in isolation, and
PromoteOwnStagedManifestDoesNotReadBody proves the request is gone, but nothing exercises the two
together.

A regression test in this repository's CAS suite would be the natural home: run an insert-heavy
workload against a pool with an aggressive GC round interval and assert that no committed ref ever
resolves to an absent manifest.

[filimonov]

The CAS-36 sweep race happened in CI. It left a dangling ref after a successful INSERT. The sweep fix in this PR, a923533 ("Fix CAS orphan sweep for staged manifests"), closes it. Can we land that commit, plus 593311d for namespace rebirth, as a separate PR into antalya-26.6 and antalya-26.6-20261008, ahead of the rest of this PR?

Evidence

• PR Merge antalya-26.6 into antalya-26.6-20261008 #2614 (merge commit 18cb495), tiered_storage/with cas, scenario freeze: https://altinity-build-artifacts.s3.amazonaws.com/REFs/2614/merge/18cb4952478ba892537d679ec87cd6581597d0c2/regression/x86_64/with_analyzer/zookeeper/without_thread_fuzzer/tiered_storage/with_cas/report.html
• FREEZE at 15:36:34 failed with Code: 107 live ref names manifest at data/cas/manifests/tiered-cas-clickhouse1/store/0df/0dfec3a5-…@cas@/0000000000000007-0000000000000067/000001.zst but its object is missing.

Sequence

1. stageManifest PUT the body before precommitAdd created the namespace catalog row (ContentAddressedTransaction.cpp:418-419). The row PUT got a 412, and conflictBackoff widened the window.
2. GC round 4 on clickhouse2 listed the body and read a catalog with no row for the namespace. Before this fix the floor is checked only if (catalog_entry) (Gc/CasOrphanManifestSweep.cpp:761-776), so the body was nominated. The comment at :885-889 assumed the row is created before any object the namespace owns. The writer order in step 1 breaks that.
3. The INSERT committed its ref. The sweep then deleted the body (CasGc.cpp:1518).

Impact beyond the lost part

• GC rounds 5-86 (15:37-17:08) logged destructive work SUPPRESSED, and namespace removal is stuck was logged 72 times. One dangling ref stopped destructive GC for the whole pool for 1.5 h.

Why a923533 covers it

• It reads the floor for every listed manifest, rowless ones included. The writer's active build keeps the prefix ineligible, and CursorPageRetainsRowlessBodyOfActiveBuild pins that.
• It re-reads the catalog after the floor read and keeps candidates whose namespace appeared meanwhile (CursorPageRetainsRowlessBodyWhenNamespaceAppearsAfterCatalogCut).
• It replaces CursorPageDeletesObservedBodyWhenCatalogOmitsNamespace, which pins the unsafe behaviour on both base branches today.

Not caused by #2614

• Gc/CasOrphanManifestSweep.cpp is identical in both parents of 18cb495. The race is timing-dependent: GC runs every 60 s and the window is milliseconds.

CAS-36 spec §2.2 says this race fails closed at the final commit step. That is wrong: the sweep plans before the row exists and deletes after the commit. I will fix §2.2 and cite this run.