From 5a44354c93e198d599785780f969bdce6db62639 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 13:45:32 +0200 Subject: [PATCH 01/19] Add test for redundant manifest read during promote --- src/Disks/tests/gtest_cas_part_write.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/Disks/tests/gtest_cas_part_write.cpp b/src/Disks/tests/gtest_cas_part_write.cpp index 461718040aac9..680f36ceb63ce 100644 --- a/src/Disks/tests/gtest_cas_part_write.cpp +++ b/src/Disks/tests/gtest_cas_part_write.cpp @@ -438,6 +438,23 @@ TEST(CASPartWriteTxn, StageManifestUsesPerBuildOrdinals) EXPECT_EQ(next.ref.manifest_ordinal, 1u); } +TEST(CASPartWriteTxn, PromoteOwnStagedManifestDoesNotReadBody) +{ + auto backend = std::make_shared(); + auto counting = std::make_shared(backend); + auto store = Pool::open(counting, PoolConfig{.pool_prefix = "p", .server_root_id = "test"}); + const RootNamespace ns{"srv1/no-promote-manifest-get"}; + auto build = startBuildFor(store, ns, "part"); + + const ManifestId id = build->stageManifest({}); + build->precommitAdd(ns, "part", id); + + const String manifest_key = store->layout().manifestKey(id); + const size_t gets_before = counting->getCountFor(manifest_key); + EXPECT_TRUE(build->promote(ns, "part", build->buildId(), id)); + EXPECT_EQ(counting->getCountFor(manifest_key) - gets_before, 0u); +} + /// B171: the `cas_owner` owner-triple stamping (`PartWriteTxn::ownerMeta`) was DELETED — protection is now /// the build-root precommit edge (reachability), not revocable object metadata GC reads per-candidate. /// The old `CASPartWriteTxn.BlobCarriesOwnerTripleInMetadata` asserted that stamping; its coverage is replaced From ff7d99f8e09ea762878412b0df44b2a826233079 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 13:47:25 +0200 Subject: [PATCH 02/19] Avoid rereading staged manifests during promote --- .../cas/architecture/manifests-and-refs.md | 5 ++ .../cas/architecture/part-lifecycle.md | 11 +-- .../ContentAddressed/Pool/CasPartWriteTxn.cpp | 69 ++++++++++++------- .../ContentAddressed/Pool/CasPartWriteTxn.h | 24 ++++--- 4 files changed, 73 insertions(+), 36 deletions(-) diff --git a/docs/en/antalya/cas/architecture/manifests-and-refs.md b/docs/en/antalya/cas/architecture/manifests-and-refs.md index 6e6b1ed667bfa..c1eb9069f7b68 100644 --- a/docs/en/antalya/cas/architecture/manifests-and-refs.md +++ b/docs/en/antalya/cas/architecture/manifests-and-refs.md @@ -55,6 +55,11 @@ before any owner transition names it. Rewriting a part therefore writes a **new* the **same** blobs and moves the ref in one ref-log record — a repoint, covered in full on the [part-lifecycle page](/antalya/cas/architecture/part-lifecycle#repoint). +The writer retains a compact proof after its own successful `stageManifest`: the confirmed object +incarnation and the ordered blob dependencies. The matching `promote` consumes this proof without +reading the immutable body back. Promotion of a manifest not staged by that `PartWriteTxn` still +performs an exact `GET` and validates `RefMatchesBody` and `ManifestNamespaceMatches` fail-closed. + ## Manifest lifecycle and the orphan sweep {#manifest-lifecycle} ```mermaid diff --git a/docs/en/antalya/cas/architecture/part-lifecycle.md b/docs/en/antalya/cas/architecture/part-lifecycle.md index 267e3f6f8a19e..3625eb9591e78 100644 --- a/docs/en/antalya/cas/architecture/part-lifecycle.md +++ b/docs/en/antalya/cas/architecture/part-lifecycle.md @@ -53,7 +53,7 @@ sequenceDiagram end PW->>PW: merge upload results on the owning thread, one no-throw swap TX->>PW: promote - PW->>S3: GET and validate the precommit manifest body + PW->>PW: validate the staged manifest proof and blob dependencies PW->>S3: append ref-log txn: retire old committed, Precommit to Committed, SetPublishedAt Note over PW: commit durable, then retire the build sequence end @@ -93,9 +93,12 @@ explicit `Materialized` proof. A trusted source manifest instead yields `Trusted blob I/O. Pool size is the server setting `cas_blob_upload_pool_size` (default 16). -**Step 4 — `promote`.** Reads and revalidates the precommit manifest body once; sets the commit -state to Uncertain before the append — past that point, failure is no longer proof of the negative -— then checks that the precommit is still the live owner and validates explicit dependency proofs. +**Step 4 — `promote`.** A manifest staged by the same `PartWriteTxn` uses the in-memory proof produced +by the successful conditional create: the confirmed object incarnation and ordered blob dependencies. +There is no manifest `GET` on this normal path. A manifest not staged by this transaction is read and +validated fail-closed. Promotion then sets the commit state to Uncertain before the append — past that +point, failure is no longer proof of the negative — checks that the precommit is still the live owner, +and validates explicit dependency proofs. `Materialized` leaves are edge-protected; `TrustedManifest` leaves are trusted through the durable source-manifest edge with no per-file `HEAD`; anything else is a `LOGICAL_ERROR`. The whole thing lands as one ref-log record: optional retirement of the old committed binding, the pure diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp index 516debe79ef7b..d9c297a956ee2 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp @@ -638,6 +638,12 @@ ManifestId PartWriteTxn::stageManifest(std::vector entries) "stageManifest: encoded manifest {} bytes exceeds cap {}", encoded_text.size(), kMaxManifestEncodedBytes); const String encoded = sealObject(FormatId::PartManifest, encoded_text); + std::vector blob_dependencies; + blob_dependencies.reserve(body.entries.size()); + for (const ManifestEntry & entry : body.entries) + if (entry.placement == EntryPlacement::Blob) + blob_dependencies.push_back(entry.ref); + const ManifestId id{owning_ns, ref}; const String key = store->layout().manifestKey(id); @@ -695,7 +701,9 @@ ManifestId PartWriteTxn::stageManifest(std::vector entries) }); staged_manifests.push_back(id); - staged_manifest_ids.insert(id); /// A3 mint-tightening: `precommitAdd`'s only legal fresh-ownership source + staged_manifest_proofs.emplace( + id, + StagedManifestProof{manifest_incarnation, std::move(blob_dependencies)}); return id; } @@ -712,13 +720,13 @@ void PartWriteTxn::precommitAdd(const RootNamespace & target_ns, const String & /// A3 mint-tightening (ABA barrier for the relink confirm's exact-`ManifestRef` equality): an /// unowned `ManifestId` may enter ownership only from the transaction that freshly staged it. - /// Evaluated HERE (synchronously, off this transaction's own private `staged_manifest_ids` -- + /// Evaluated HERE (synchronously, off this transaction's own private `staged_manifest_proofs` -- /// never mutated concurrently, so no snapshot races) rather than read live inside the closure /// below, because it does not depend on ledger state at all. The bool is captured BY VALUE into /// the closure -- consistent with every other capture there -- rather than capturing `this`, /// which would let a closure that outlives this stack (see the capture comment below) dereference /// a dead transaction. Enforcement, however, is NOT done here: see the closure for why. - const bool id_staged_by_this_txn = staged_manifest_ids.contains(id); + const bool id_staged_by_this_txn = staged_manifest_proofs.contains(id); /// One `owner_transition` create-precommit op. /// No body HEAD — a missing body is a legal fail-closed, non-activating intent, unchanged from the @@ -821,19 +829,33 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final "promote: manifest namespace '{}' != target namespace '{}'", id.root_namespace.string(), target_ns.string()); - /// Read + validate the manifest body ONCE (O(manifest entries), one streaming read). Absent or - /// invalid ⇒ fail closed: a committed ref must never name a missing/mismatched manifest. const String manifest_key = store->layout().manifestKey(id); - CasOperation op = store->mountRequests().admit(); - const auto body_got = op.read(manifest_key, Retry::standard()); - if (!body_got) - throwCasWriteRetryLater(fmt::format( - "promote: manifest body absent at {} — failing closed (retry with a fresh ManifestId)", manifest_key)); - const PartManifest body = decodePartManifest(openObject(FormatId::PartManifest, body_got->bytes)); - if (!refMatchesBody(id.ref, body)) - throwCasWriteRetryLater(fmt::format("promote: RefMatchesBody failed for {}", manifest_key)); - if (!manifestNamespaceMatches(target_ns, body)) - throwCasWriteRetryLater(fmt::format("promote: ManifestNamespaceMatches failed for {}", manifest_key)); + std::vector manifest_blob_dependencies; + if (const auto staged = staged_manifest_proofs.find(id); staged != staged_manifest_proofs.end()) + { + if (staged->second.incarnation.key() != manifest_key) + throw Exception(ErrorCodes::LOGICAL_ERROR, + "promote: staged manifest proof names key '{}', expected '{}'", + staged->second.incarnation.key(), manifest_key); + manifest_blob_dependencies = staged->second.blob_dependencies; + } + else + { + CasOperation op = store->mountRequests().admit(); + const auto body_got = op.read(manifest_key, Retry::standard()); + if (!body_got) + throwCasWriteRetryLater(fmt::format( + "promote: manifest body absent at {} — failing closed (retry with a fresh ManifestId)", manifest_key)); + const PartManifest body = decodePartManifest(openObject(FormatId::PartManifest, body_got->bytes)); + if (!refMatchesBody(id.ref, body)) + throwCasWriteRetryLater(fmt::format("promote: RefMatchesBody failed for {}", manifest_key)); + if (!manifestNamespaceMatches(target_ns, body)) + throwCasWriteRetryLater(fmt::format("promote: ManifestNamespaceMatches failed for {}", manifest_key)); + manifest_blob_dependencies.reserve(body.entries.size()); + for (const ManifestEntry & entry : body.entries) + if (entry.placement == EntryPlacement::Blob) + manifest_blob_dependencies.push_back(entry.ref); + } /// Blob readiness is checked inside the append closure below, after owner liveness. It consumes /// only this build's explicit dependency proofs and performs no backend copy-forward. @@ -859,13 +881,14 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final commit_state = CommitState::Uncertain; store->appendRefOps(target_ns, MutationScope::ref(final_ref_name), /// Capture the closure's INPUTS by value (ref name, manifest id, promote build id, repoint flag, - /// and the manifest `body` it revalidates) rather than `[&]`, as defense-in-depth against a + /// and the manifest blob dependencies it revalidates) rather than `[&]`, as defense-in-depth against a /// closure outliving this stack. Unlike `precommitAdd`, this closure cannot be made fully /// self-contained: `dependencyProof` reads this build's `deps` member (so it must /// keep `this`), and `repoint_old`/`created` are OUTPUTS read after the call returns (so they stay /// references). The ref-lane leadership guard is the real fix; both residual by-reference captures /// are safe because the guard guarantees the closure is never invoked after this frame unwinds. - [this, final_ref_name, id, promote_build_id, allow_repoint, body, &repoint_old, &created] + [this, final_ref_name, id, promote_build_id, allow_repoint, + manifest_blob_dependencies = std::move(manifest_blob_dependencies), &repoint_old, &created] (const RefTableState & state) -> std::vector { created = !state.getCommitted().contains(final_ref_name); @@ -908,16 +931,14 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final /// build's precommit edge is durable — matching the relink trust model (ordinary /// ReplicatedMergeTree interserver trust). A genuinely-absent adopted blob is an invariant /// violation caught by fsck, not here. - for (const ManifestEntry & e : body.entries) + for (const BlobRef & ref : manifest_blob_dependencies) { - if (e.placement != EntryPlacement::Blob) - continue; - const auto proof = dependencyProof(e.ref); + const auto proof = dependencyProof(ref); if (!proof) throw Exception(ErrorCodes::LOGICAL_ERROR, "promote: blob leaf {} has no dependency proof at commit — a pending upload " "never completed; failing closed", - store->layout().blobKey(e.ref)); + store->layout().blobKey(ref)); switch (*proof) { @@ -939,7 +960,7 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final { ev.type = CasEventType::BlobReuseAdopt; ev.object_kind = CasEventObjectKind::Blob; - ev.object_hash = blobIdOf(e.ref); + ev.object_hash = blobIdOf(ref); ev.outcome = "adopt"; ev.reason = "manifest-trust"; /// distinguishable trusted-adopt class (empty token) }); @@ -948,7 +969,7 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final throw Exception(ErrorCodes::LOGICAL_ERROR, "promote: blob leaf {} has an unnamed dependency proof at commit; failing closed", - store->layout().blobKey(e.ref)); + store->layout().blobKey(ref)); } /// BUG 1a: refuse to overwrite a live committed ref that already names a DIFFERENT manifest — diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h index 953059a76ebc3..ab21b7a6f1cac 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h @@ -1,4 +1,5 @@ #pragma once +#include #include #include #include @@ -7,7 +8,6 @@ #include #include #include -#include #include #include @@ -203,8 +203,9 @@ class PartWriteTxn /// `cas/manifests////000001.zst` via the pool's shared request /// controller. It uses budgeted attempts with resolve-before-reissue and performs no preliminary HEAD, /// because `manifest_ordinal` is monotone within this build. It enforces manifest-size caps before the body - /// write returns and therefore before any owner transition is published. The body is not retained after a - /// successful write; on retry the caller re-stages from source. Every call uses a fresh manifest ordinal. + /// write returns and therefore before any owner transition is published. After a successful write it retains + /// only the confirmed incarnation and blob dependencies needed by `promote`; on retry the caller re-stages + /// from source. Every call uses a fresh manifest ordinal. /// The id is recorded for best-effort `abandon` cleanup. ManifestId stageManifest(std::vector entries); @@ -228,7 +229,7 @@ class PartWriteTxn /// ref-log entry. /// 1. `Materialized` leaves are already protected by the durable precommit edge, so no writer-side retired-view /// refresh is needed; - /// 2. stream-read the precommit manifest body; validate RefMatchesBody / ManifestNamespaceMatches; + /// 2. consume this transaction's staged proof, or stream-read and validate a manifest not staged here; /// 3. `TrustedManifest` leaves use the durable source-manifest edge with no per-file HEAD/loadMeta /// probe; a genuinely absent trusted blob is an invariant violation caught by fsck, not here; /// 4. a body-absent precommit or lost owner-liveness ⇒ ABORTED; a missing dependency proof ⇒ @@ -333,6 +334,12 @@ class PartWriteTxn /// dependencies are blob-only, so `ObjectKind` is not part of the key. using DepKey = BlobRef; + struct StagedManifestProof + { + Etag incarnation; + std::vector blob_dependencies; + }; + /// Own the complete bounded `HEAD`/metadata/publication/reconciliation state machine. Build-neutral: /// returns the materialized dependency and diagnostics without folding into `deps`. BlobUploadResult ensureBlobPresent(const BlobUploadRequest & req) const; @@ -381,12 +388,13 @@ class PartWriteTxn std::vector staged_manifests; /// for best-effort abandon cleanup - /// Every `ManifestId` this transaction has minted via `stageManifest`, checked by `precommitAdd`: + /// Every `ManifestId` this transaction has minted via `stageManifest`, with the proof consumed by `promote` + /// and checked by `precommitAdd`: /// an unowned id may enter ownership only from the transaction that freshly staged it. Kept separate from /// `staged_manifests` above -- that vector's role (best-effort abandon cleanup) is unrelated and - /// could change independently; this set exists purely as the ABA barrier's identity check, so it - /// stays correct even if the cleanup vector's contents or lifetime ever change. - std::set staged_manifest_ids; + /// could change independently; this map also supplies the ABA barrier's identity check, so it stays + /// correct even if the cleanup vector's contents or lifetime ever change. + std::map staged_manifest_proofs; std::map deps; /// dependencies recorded by this build (blobs only) From 8c608147192d9167a66d1b114601090227e8b1f0 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 13:54:05 +0200 Subject: [PATCH 03/19] Fix staged manifest capture shadowing --- .../ContentAddressed/Pool/CasPartWriteTxn.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp index d9c297a956ee2..6a18d38c920bb 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp @@ -888,7 +888,7 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final /// references). The ref-lane leadership guard is the real fix; both residual by-reference captures /// are safe because the guard guarantees the closure is never invoked after this frame unwinds. [this, final_ref_name, id, promote_build_id, allow_repoint, - manifest_blob_dependencies = std::move(manifest_blob_dependencies), &repoint_old, &created] + manifest_dependencies = std::move(manifest_blob_dependencies), &repoint_old, &created] (const RefTableState & state) -> std::vector { created = !state.getCommitted().contains(final_ref_name); @@ -931,7 +931,7 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final /// build's precommit edge is durable — matching the relink trust model (ordinary /// ReplicatedMergeTree interserver trust). A genuinely-absent adopted blob is an invariant /// violation caught by fsck, not here. - for (const BlobRef & ref : manifest_blob_dependencies) + for (const BlobRef & ref : manifest_dependencies) { const auto proof = dependencyProof(ref); if (!proof) From 750c789b6ac258c4a16ac9d5cc13ce80651e45ff Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 16:02:10 +0200 Subject: [PATCH 04/19] Add test for staged manifest cleanup without head --- src/Disks/tests/gtest_cas_part_write.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/Disks/tests/gtest_cas_part_write.cpp b/src/Disks/tests/gtest_cas_part_write.cpp index 680f36ceb63ce..eb0b5fe86db95 100644 --- a/src/Disks/tests/gtest_cas_part_write.cpp +++ b/src/Disks/tests/gtest_cas_part_write.cpp @@ -1493,6 +1493,25 @@ TEST(CASPartWriteTxn, AbandonRemovesStagedDebrisAndDisables) "has been abandoned"); } +TEST(CASPartWriteTxn, AbandonDeletesStagedManifestWithoutHead) +{ + auto backend = std::make_shared(); + auto counting = std::make_shared(backend); + auto store = Pool::open(counting, PoolConfig{.pool_prefix = "p", .server_root_id = "test"}); + const RootNamespace ns{"srv1/abandon-without-head"}; + auto build = startBuildFor(store, ns, "part"); + + const ManifestId id = build->stageManifest({}); + const String manifest_key = store->layout().manifestKey(id); + const size_t heads_before = counting->headCountFor(manifest_key); + + build->abandon(); + + EXPECT_EQ(counting->headCountFor(manifest_key) - heads_before, 0u); + OperationForTest op(*backend); + EXPECT_FALSE((*op).head(manifest_key, Retry::once()).has_value()); +} + TEST(CASPartWriteTxn, PublishHappyPathRoundTrip) { auto b = std::make_shared(); From 45ee5a7365853004a952a8bbce7c77e30541ffd8 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 16:25:17 +0200 Subject: [PATCH 05/19] Use staged manifest proofs for cleanup --- .../ContentAddressed/Pool/CasPartWriteTxn.cpp | 17 +++++------------ .../ContentAddressed/Pool/CasPartWriteTxn.h | 11 ++--------- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp index 6a18d38c920bb..1c1343669645e 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp @@ -700,7 +700,6 @@ ManifestId PartWriteTxn::stageManifest(std::vector entries) e.reason = "stageManifest: part-manifest body written"; }); - staged_manifests.push_back(id); staged_manifest_proofs.emplace( id, StagedManifestProof{manifest_incarnation, std::move(blob_dependencies)}); @@ -829,18 +828,14 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final "promote: manifest namespace '{}' != target namespace '{}'", id.root_namespace.string(), target_ns.string()); - const String manifest_key = store->layout().manifestKey(id); std::vector manifest_blob_dependencies; if (const auto staged = staged_manifest_proofs.find(id); staged != staged_manifest_proofs.end()) { - if (staged->second.incarnation.key() != manifest_key) - throw Exception(ErrorCodes::LOGICAL_ERROR, - "promote: staged manifest proof names key '{}', expected '{}'", - staged->second.incarnation.key(), manifest_key); manifest_blob_dependencies = staged->second.blob_dependencies; } else { + const String manifest_key = store->layout().manifestKey(id); CasOperation op = store->mountRequests().admit(); const auto body_got = op.read(manifest_key, Retry::standard()); if (!body_got) @@ -1115,7 +1110,7 @@ void PartWriteTxn::abandon() e.outcome = "cancelled"; e.reason = "cancelForNamespaceRemoval: owning namespace removed (dropNamespace); best-effort " "deleted staged _manifests debris; precommit body (if any) left for GC"; - e.detail = {{"build_seq", std::to_string(build_seq)}, {"staged", std::to_string(staged_manifests.size())}}; + e.detail = {{"build_seq", std::to_string(build_seq)}, {"staged", std::to_string(staged_manifest_proofs.size())}}; }); } catch (...) @@ -1217,7 +1212,7 @@ void PartWriteTxn::abandon() e.outcome = "abandoned"; e.reason = "abandon: appended a precommit-removal event for the live precommit (body left for GC); " "best-effort deleted the never-precommitted _manifests debris; remainder reaped by the orphan sweep"; - e.detail = {{"build_seq", std::to_string(build_seq)}, {"staged", std::to_string(staged_manifests.size())}}; + e.detail = {{"build_seq", std::to_string(build_seq)}, {"staged", std::to_string(staged_manifest_proofs.size())}}; }); } catch (...) @@ -1240,15 +1235,13 @@ void PartWriteTxn::cleanupStagedManifestDebrisBestEffort() /// while keeping it is not: an unreferenced body is ordinary orphan-sweep debris. const bool precommit_attempted = precommit_state != PrecommitState::NotAttempted; CasOperation op = store->mountRequests().admit(); - for (const ManifestId & id : staged_manifests) + for (const auto & [id, proof] : staged_manifest_proofs) { if (precommit_attempted && id.ref == precommit_manifest && id.root_namespace == precommit_target_ns) continue; /// the (possibly) live precommit body — left for GC (delete-after-sealed-decrements) try { - const String key = store->layout().manifestKey(id); - if (const auto observed = op.head(key, Retry::standard())) - op.remove(key, observed->etag, Retry::standard()); + op.remove(proof.incarnation.key(), proof.incarnation, Retry::standard()); } catch (...) // NOLINT(bugprone-empty-catch) { diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h index ab21b7a6f1cac..ce7ca7c6b8830 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h @@ -9,6 +9,7 @@ #include #include #include +#include #include namespace DB { class WriteBuffer; } @@ -386,15 +387,7 @@ class PartWriteTxn String precommit_final_ref; ManifestRef precommit_manifest; - std::vector staged_manifests; /// for best-effort abandon cleanup - - /// Every `ManifestId` this transaction has minted via `stageManifest`, with the proof consumed by `promote` - /// and checked by `precommitAdd`: - /// an unowned id may enter ownership only from the transaction that freshly staged it. Kept separate from - /// `staged_manifests` above -- that vector's role (best-effort abandon cleanup) is unrelated and - /// could change independently; this map also supplies the ABA barrier's identity check, so it stays - /// correct even if the cleanup vector's contents or lifetime ever change. - std::map staged_manifest_proofs; + std::unordered_map staged_manifest_proofs; std::map deps; /// dependencies recorded by this build (blobs only) From 7fca3928e2612b7dbc3adc79b21d6c4bc085acf7 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 16:49:34 +0200 Subject: [PATCH 06/19] Document staged manifest proof contract `promote` no longer re-reads a manifest staged by the same `PartWriteTxn`, so the "body-absent precommit is rejected" contract now applies only to manifests not staged there. Also document that `staged_manifest_proofs` entries must never be erased, because `precommitAdd` uses them as the A3 barrier. Co-Authored-By: Claude Opus 5.5 --- .../antalya/cas/architecture/replication.md | 4 ++-- .../ContentAddressed/Pool/CasPartWriteTxn.h | 23 +++++++++++++------ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/docs/en/antalya/cas/architecture/replication.md b/docs/en/antalya/cas/architecture/replication.md index 4a3bce92213a5..e832f0811336c 100644 --- a/docs/en/antalya/cas/architecture/replication.md +++ b/docs/en/antalya/cas/architecture/replication.md @@ -69,7 +69,7 @@ sequenceDiagram | 4 | The receiver trusts nothing from the wire but the entry list | The sender's manifest id, namespace and payload digest are ignored; the target namespace and ref come from the receiver's own router, and manifest path hygiene is validated at decode | | 5 | The confirm is I/O-free and fail-closed | A cold, evicted, unfenced or terminal mount answers `Unknown`. `No` and `Unknown` both go on the wire as `unproven`, because the fence check is evaluated last, so a `No` cannot be distinguished from "cannot prove it right now" | | 6 | Only the literal `yes` authorizes promotion | Everything else — including a timeout — is one outcome: throw and retry later | -| 7 | Promote outcomes are three-way | `Committed` proceeds; a **proven** not-committed state (body-absent precommit, precommit no longer live owner, ref conflict) falls back to a byte fetch; `Unresolved` **throws**, because returning "fall back" there would publish the part twice | +| 7 | Promote outcomes are three-way | `Committed` proceeds; a **proven** not-committed state (precommit no longer live owner, ref conflict) falls back to a byte fetch; `Unresolved` **throws**, because returning "fall back" there would publish the part twice. The receiver stages its manifest in the same build, so `promote` does not read the body back | The byte-fetch fallback is bounded: it re-invokes the fetch with relink disabled, which stops the receiver advertising its pool uuid, which stops the sender offering relink — so the relink path @@ -102,7 +102,7 @@ The part is never quietly placed on another disk instead. This is the behaviour content-addressed policy always had, and a mixed policy now shares it. The byte-fetch fallback after a relink that failed for a mechanism reason (a corrupted manifest, a -body-absent precommit, a ref conflict) re-requests the bytes on the same pool disk, where they +ref conflict) re-requests the bytes on the same pool disk, where they content-address and deduplicate against the pool — the placement outlives the relink. A manifest of a newer format generation is not degraded to bytes today (a tracked gap, `[relink-fallback-unknown-format-version]` in the backlog). diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h index ce7ca7c6b8830..44ac55f7304bc 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h @@ -207,15 +207,18 @@ class PartWriteTxn /// write returns and therefore before any owner transition is published. After a successful write it retains /// only the confirmed incarnation and blob dependencies needed by `promote`; on retry the caller re-stages /// from source. Every call uses a fresh manifest ordinal. - /// The id is recorded for best-effort `abandon` cleanup. + /// The recorded proof is the A3 ownership barrier of `precommitAdd`, the no-read path of `promote`, + /// and the exact-incarnation target of best-effort `abandon` cleanup. ManifestId stageManifest(std::vector entries); /// Add this transaction's precommit owner intent; there is no `_precommits` namespace. One /// `appendRefOps` call appending an OwnerTransition `RefOp` (new_binding = {Precommit, /// final_ref_name, id.ref}) to final_ref_name's ref-log entry, so the later promote is an atomic - /// owner move over that same entry. Needs NO body-exists HEAD as a safety authority: GC and - /// promotion handle a missing precommit manifest body by failing closed (a missing-body precommit - /// is a non-activating, non-promotable intent). + /// owner move over that same entry. Needs NO body-exists HEAD as a safety authority: GC handles a + /// missing precommit manifest body by failing closed (a missing-body precommit is a non-activating + /// intent). `promote` re-reads the body only for a manifest not staged by this transaction; for its + /// own staged manifest it relies on the put-if-absent incarnation and on the orphan sweep never + /// deleting bodies of a still-active build sequence. /// /// A3 mint-tightening: `id` must either be one THIS transaction minted via `stageManifest`, or /// already be `final_ref_name`'s current committed manifest (the idempotent re-drive -- see the @@ -233,12 +236,14 @@ class PartWriteTxn /// 2. consume this transaction's staged proof, or stream-read and validate a manifest not staged here; /// 3. `TrustedManifest` leaves use the durable source-manifest edge with no per-file HEAD/loadMeta /// probe; a genuinely absent trusted blob is an invariant violation caught by fsck, not here; - /// 4. a body-absent precommit or lost owner-liveness ⇒ ABORTED; a missing dependency proof ⇒ - /// LOGICAL_ERROR (fail closed); + /// 4. a body-absent or invalid manifest not staged here ⇒ retry-later exception before the append; + /// lost owner-liveness ⇒ ABORTED; a missing dependency proof ⇒ LOGICAL_ERROR (fail closed). + /// A manifest staged here is not re-read: a body removed outside the protocol after a confirmed + /// `stageManifest` is not detected here and is reported later by fsck as dangling; /// 5. atomically replace precommit(build_id) owner with committed(final_ref_name) owner by appending /// ONE pure-move `RefOp` (old_binding={Precommit,final_ref_name,T}, new_binding={Committed,final_ref_name,T}, /// same manifest_ref T) and setting refs[final_ref_name]; - /// 6. promotion never emits blob deltas. A missing-body precommit + /// 6. promotion never emits blob deltas. A missing-body precommit of a manifest not staged here /// is non-activating and was rejected at step 4 (the writer re-stages with a fresh ManifestId). /// /// PROCESS-RESTART INVARIANT: a `PartWriteTxn` is a plain in-memory C++ object owned by the wiring's @@ -387,6 +392,10 @@ class PartWriteTxn String precommit_final_ref; ManifestRef precommit_manifest; + /// Every `ManifestId` this transaction staged, with its confirmed incarnation and blob dependencies. + /// Entries are never erased while the transaction lives: membership is the A3 ABA barrier checked + /// by `precommitAdd` and the reason `promote` may skip the body read. Erasing an entry (for example + /// after cleanup or after promote) would silently weaken both. std::unordered_map staged_manifest_proofs; std::map deps; /// dependencies recorded by this build (blobs only) From 2005aafe3b4398c9433d27ba93c73d2c5b5cc7f3 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 16:56:34 +0200 Subject: [PATCH 07/19] Share blob dependency extraction and log staged manifest cleanup misses Extract `blobDependenciesOf` so `stageManifest` and the non-staged path of `promote` use the same filter. Cleanup of staged manifests now logs at debug level when the exact-incarnation remove returns `Gone` or `Mismatch`, or throws, instead of silently swallowing it. Co-Authored-By: Claude Opus 5.5 --- .../ContentAddressed/Pool/CasPartWriteTxn.cpp | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp index 1c1343669645e..ea27150a2c434 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp @@ -97,6 +97,16 @@ bool isDeterministicBlobPublicationFailure(const std::exception & error) return dynamic_cast(&error) == nullptr; } +std::vector blobDependenciesOf(const std::vector & entries) +{ + std::vector dependencies; + dependencies.reserve(entries.size()); + for (const ManifestEntry & entry : entries) + if (entry.placement == EntryPlacement::Blob) + dependencies.push_back(entry.ref); + return dependencies; +} + } /// The pool-wide content-hash convention, with the selected `algo`: `Cas::blobHashHexOneShot` uses the same @@ -638,11 +648,7 @@ ManifestId PartWriteTxn::stageManifest(std::vector entries) "stageManifest: encoded manifest {} bytes exceeds cap {}", encoded_text.size(), kMaxManifestEncodedBytes); const String encoded = sealObject(FormatId::PartManifest, encoded_text); - std::vector blob_dependencies; - blob_dependencies.reserve(body.entries.size()); - for (const ManifestEntry & entry : body.entries) - if (entry.placement == EntryPlacement::Blob) - blob_dependencies.push_back(entry.ref); + std::vector blob_dependencies = blobDependenciesOf(body.entries); const ManifestId id{owning_ns, ref}; const String key = store->layout().manifestKey(id); @@ -846,10 +852,7 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final throwCasWriteRetryLater(fmt::format("promote: RefMatchesBody failed for {}", manifest_key)); if (!manifestNamespaceMatches(target_ns, body)) throwCasWriteRetryLater(fmt::format("promote: ManifestNamespaceMatches failed for {}", manifest_key)); - manifest_blob_dependencies.reserve(body.entries.size()); - for (const ManifestEntry & entry : body.entries) - if (entry.placement == EntryPlacement::Blob) - manifest_blob_dependencies.push_back(entry.ref); + manifest_blob_dependencies = blobDependenciesOf(body.entries); } /// Blob readiness is checked inside the append closure below, after owner liveness. It consumes @@ -1241,11 +1244,18 @@ void PartWriteTxn::cleanupStagedManifestDebrisBestEffort() continue; /// the (possibly) live precommit body — left for GC (delete-after-sealed-decrements) try { - op.remove(proof.incarnation.key(), proof.incarnation, Retry::standard()); + const Removal removal = op.remove(proof.incarnation.key(), proof.incarnation, Retry::standard()); + if (removal != Removal::Removed) + LOG_DEBUG(getLogger("CasPartWriteTxn"), + "abandon: staged manifest {} was not removed ({}), left for the orphan sweep", + proof.incarnation.key(), removal == Removal::Gone ? "gone" : "incarnation mismatch"); } - catch (...) // NOLINT(bugprone-empty-catch) + catch (...) { /// Best-effort cleanup: the GC backstop sweep is the durable guarantee. + tryLogCurrentException(getLogger("CasPartWriteTxn"), + fmt::format("abandon: staged manifest {} cleanup failed, left for the orphan sweep", proof.incarnation.key()), + LogsLevel::debug); } } } From 9f9c13bf0134022c05908b0d32d9448bcc31533f Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 17:01:24 +0200 Subject: [PATCH 08/19] Test staged manifest proof dependencies and precommit cleanup skip Add a test that promotes its own staged manifest with Inline and Blob entries: it fails with `no dependency proof` while one blob is missing, then commits without a manifest read. Add a test that `abandon` keeps the live precommit body and removes another staged manifest without `HEAD`. Co-Authored-By: Claude Opus 5.5 --- src/Disks/tests/gtest_cas_part_write.cpp | 57 ++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/Disks/tests/gtest_cas_part_write.cpp b/src/Disks/tests/gtest_cas_part_write.cpp index eb0b5fe86db95..f570e7f725e32 100644 --- a/src/Disks/tests/gtest_cas_part_write.cpp +++ b/src/Disks/tests/gtest_cas_part_write.cpp @@ -455,6 +455,38 @@ TEST(CASPartWriteTxn, PromoteOwnStagedManifestDoesNotReadBody) EXPECT_EQ(counting->getCountFor(manifest_key) - gets_before, 0u); } +TEST(CASPartWriteTxn, PromoteOwnStagedManifestChecksBlobDependenciesWithoutBodyRead) +{ + auto backend = std::make_shared(); + auto counting = std::make_shared(backend); + auto store = Pool::open(counting, PoolConfig{.pool_prefix = "p", .server_root_id = "test"}); + const RootNamespace ns{"srv1/promote-staged-deps"}; + auto build = startBuildFor(store, ns, "part"); + + ManifestEntry inline_entry; + inline_entry.path = "small"; + inline_entry.placement = EntryPlacement::Inline; + inline_entry.inline_bytes = "abc"; + const ManifestId id = build->stageManifest( + {inline_entry, blobManifestEntry("a.bin", "blob-a"), blobManifestEntry("b.bin", "blob-b")}); + build->precommitAdd(ns, "part", id); + build->putBlob(idOf("blob-a"), BlobSource::fromString("blob-a")); + + EXPECT_DEATH( + { + DB::abort_on_logical_error.store(true, std::memory_order_relaxed); + build->promote(ns, "part", build->buildId(), id); + }, + "no dependency proof"); + + build->putBlob(idOf("blob-b"), BlobSource::fromString("blob-b")); + const String manifest_key = store->layout().manifestKey(id); + const size_t gets_before = counting->getCountFor(manifest_key); + EXPECT_TRUE(build->promote(ns, "part", build->buildId(), id)); + EXPECT_EQ(counting->getCountFor(manifest_key) - gets_before, 0u); + EXPECT_TRUE(store->resolveRef(ns, "part").has_value()); +} + /// B171: the `cas_owner` owner-triple stamping (`PartWriteTxn::ownerMeta`) was DELETED — protection is now /// the build-root precommit edge (reachability), not revocable object metadata GC reads per-candidate. /// The old `CASPartWriteTxn.BlobCarriesOwnerTripleInMetadata` asserted that stamping; its coverage is replaced @@ -1512,6 +1544,31 @@ TEST(CASPartWriteTxn, AbandonDeletesStagedManifestWithoutHead) EXPECT_FALSE((*op).head(manifest_key, Retry::once()).has_value()); } +TEST(CASPartWriteTxn, AbandonKeepsPrecommitBodyAndRemovesOtherStagedWithoutHead) +{ + auto backend = std::make_shared(); + auto counting = std::make_shared(backend); + auto store = Pool::open(counting, PoolConfig{.pool_prefix = "p", .server_root_id = "test"}); + const RootNamespace ns{"srv1/abandon-keeps-precommit"}; + auto build = startBuildFor(store, ns, "part"); + + const ManifestId owner = build->stageManifest({}); + build->precommitAdd(ns, "part", owner); + const ManifestId other = build->stageManifest({}); + const String owner_key = store->layout().manifestKey(owner); + const String other_key = store->layout().manifestKey(other); + const size_t owner_heads_before = counting->headCountFor(owner_key); + const size_t other_heads_before = counting->headCountFor(other_key); + + build->abandon(); + + EXPECT_EQ(counting->headCountFor(owner_key) - owner_heads_before, 0u); + EXPECT_EQ(counting->headCountFor(other_key) - other_heads_before, 0u); + OperationForTest op(*backend); + EXPECT_TRUE((*op).head(owner_key, Retry::once()).has_value()); + EXPECT_FALSE((*op).head(other_key, Retry::once()).has_value()); +} + TEST(CASPartWriteTxn, PublishHappyPathRoundTrip) { auto b = std::make_shared(); From 30ccc8d437055ef5d116ea15d04ddfc7aba1c5bb Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 17:11:30 +0200 Subject: [PATCH 09/19] Rename `StagedManifestProof` to `StagedManifest` The record only says what a confirmed put-if-absent returned in `stageManifest`. It does not prove that the body still exists at `promote` time, so "proof" overstated it. Rename the member to `staged_manifests` and say "record" in the docs. Co-Authored-By: Claude Opus 5.5 --- .../cas/architecture/manifests-and-refs.md | 4 ++-- .../cas/architecture/part-lifecycle.md | 2 +- .../ContentAddressed/Pool/CasPartWriteTxn.cpp | 22 +++++++++---------- .../ContentAddressed/Pool/CasPartWriteTxn.h | 8 +++---- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/docs/en/antalya/cas/architecture/manifests-and-refs.md b/docs/en/antalya/cas/architecture/manifests-and-refs.md index c1eb9069f7b68..63a7cd0a5196a 100644 --- a/docs/en/antalya/cas/architecture/manifests-and-refs.md +++ b/docs/en/antalya/cas/architecture/manifests-and-refs.md @@ -55,8 +55,8 @@ before any owner transition names it. Rewriting a part therefore writes a **new* the **same** blobs and moves the ref in one ref-log record — a repoint, covered in full on the [part-lifecycle page](/antalya/cas/architecture/part-lifecycle#repoint). -The writer retains a compact proof after its own successful `stageManifest`: the confirmed object -incarnation and the ordered blob dependencies. The matching `promote` consumes this proof without +The writer retains a compact record after its own successful `stageManifest`: the confirmed object +incarnation and the ordered blob dependencies. The matching `promote` uses this record without reading the immutable body back. Promotion of a manifest not staged by that `PartWriteTxn` still performs an exact `GET` and validates `RefMatchesBody` and `ManifestNamespaceMatches` fail-closed. diff --git a/docs/en/antalya/cas/architecture/part-lifecycle.md b/docs/en/antalya/cas/architecture/part-lifecycle.md index 3625eb9591e78..42c274944f5d0 100644 --- a/docs/en/antalya/cas/architecture/part-lifecycle.md +++ b/docs/en/antalya/cas/architecture/part-lifecycle.md @@ -53,7 +53,7 @@ sequenceDiagram end PW->>PW: merge upload results on the owning thread, one no-throw swap TX->>PW: promote - PW->>PW: validate the staged manifest proof and blob dependencies + PW->>PW: check the staged manifest record and blob dependencies PW->>S3: append ref-log txn: retire old committed, Precommit to Committed, SetPublishedAt Note over PW: commit durable, then retire the build sequence end diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp index ea27150a2c434..29ad177f8a5c7 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp @@ -706,9 +706,9 @@ ManifestId PartWriteTxn::stageManifest(std::vector entries) e.reason = "stageManifest: part-manifest body written"; }); - staged_manifest_proofs.emplace( + staged_manifests.emplace( id, - StagedManifestProof{manifest_incarnation, std::move(blob_dependencies)}); + StagedManifest{manifest_incarnation, std::move(blob_dependencies)}); return id; } @@ -725,13 +725,13 @@ void PartWriteTxn::precommitAdd(const RootNamespace & target_ns, const String & /// A3 mint-tightening (ABA barrier for the relink confirm's exact-`ManifestRef` equality): an /// unowned `ManifestId` may enter ownership only from the transaction that freshly staged it. - /// Evaluated HERE (synchronously, off this transaction's own private `staged_manifest_proofs` -- + /// Evaluated HERE (synchronously, off this transaction's own private `staged_manifests` -- /// never mutated concurrently, so no snapshot races) rather than read live inside the closure /// below, because it does not depend on ledger state at all. The bool is captured BY VALUE into /// the closure -- consistent with every other capture there -- rather than capturing `this`, /// which would let a closure that outlives this stack (see the capture comment below) dereference /// a dead transaction. Enforcement, however, is NOT done here: see the closure for why. - const bool id_staged_by_this_txn = staged_manifest_proofs.contains(id); + const bool id_staged_by_this_txn = staged_manifests.contains(id); /// One `owner_transition` create-precommit op. /// No body HEAD — a missing body is a legal fail-closed, non-activating intent, unchanged from the @@ -835,7 +835,7 @@ bool PartWriteTxn::promote(const RootNamespace & target_ns, const String & final id.root_namespace.string(), target_ns.string()); std::vector manifest_blob_dependencies; - if (const auto staged = staged_manifest_proofs.find(id); staged != staged_manifest_proofs.end()) + if (const auto staged = staged_manifests.find(id); staged != staged_manifests.end()) { manifest_blob_dependencies = staged->second.blob_dependencies; } @@ -1113,7 +1113,7 @@ void PartWriteTxn::abandon() e.outcome = "cancelled"; e.reason = "cancelForNamespaceRemoval: owning namespace removed (dropNamespace); best-effort " "deleted staged _manifests debris; precommit body (if any) left for GC"; - e.detail = {{"build_seq", std::to_string(build_seq)}, {"staged", std::to_string(staged_manifest_proofs.size())}}; + e.detail = {{"build_seq", std::to_string(build_seq)}, {"staged", std::to_string(staged_manifests.size())}}; }); } catch (...) @@ -1215,7 +1215,7 @@ void PartWriteTxn::abandon() e.outcome = "abandoned"; e.reason = "abandon: appended a precommit-removal event for the live precommit (body left for GC); " "best-effort deleted the never-precommitted _manifests debris; remainder reaped by the orphan sweep"; - e.detail = {{"build_seq", std::to_string(build_seq)}, {"staged", std::to_string(staged_manifest_proofs.size())}}; + e.detail = {{"build_seq", std::to_string(build_seq)}, {"staged", std::to_string(staged_manifests.size())}}; }); } catch (...) @@ -1238,23 +1238,23 @@ void PartWriteTxn::cleanupStagedManifestDebrisBestEffort() /// while keeping it is not: an unreferenced body is ordinary orphan-sweep debris. const bool precommit_attempted = precommit_state != PrecommitState::NotAttempted; CasOperation op = store->mountRequests().admit(); - for (const auto & [id, proof] : staged_manifest_proofs) + for (const auto & [id, staged] : staged_manifests) { if (precommit_attempted && id.ref == precommit_manifest && id.root_namespace == precommit_target_ns) continue; /// the (possibly) live precommit body — left for GC (delete-after-sealed-decrements) try { - const Removal removal = op.remove(proof.incarnation.key(), proof.incarnation, Retry::standard()); + const Removal removal = op.remove(staged.incarnation.key(), staged.incarnation, Retry::standard()); if (removal != Removal::Removed) LOG_DEBUG(getLogger("CasPartWriteTxn"), "abandon: staged manifest {} was not removed ({}), left for the orphan sweep", - proof.incarnation.key(), removal == Removal::Gone ? "gone" : "incarnation mismatch"); + staged.incarnation.key(), removal == Removal::Gone ? "gone" : "incarnation mismatch"); } catch (...) { /// Best-effort cleanup: the GC backstop sweep is the durable guarantee. tryLogCurrentException(getLogger("CasPartWriteTxn"), - fmt::format("abandon: staged manifest {} cleanup failed, left for the orphan sweep", proof.incarnation.key()), + fmt::format("abandon: staged manifest {} cleanup failed, left for the orphan sweep", staged.incarnation.key()), LogsLevel::debug); } } diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h index 44ac55f7304bc..fa25db49d6f42 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.h @@ -207,7 +207,7 @@ class PartWriteTxn /// write returns and therefore before any owner transition is published. After a successful write it retains /// only the confirmed incarnation and blob dependencies needed by `promote`; on retry the caller re-stages /// from source. Every call uses a fresh manifest ordinal. - /// The recorded proof is the A3 ownership barrier of `precommitAdd`, the no-read path of `promote`, + /// The recorded `StagedManifest` is the A3 ownership barrier of `precommitAdd`, the no-read path of `promote`, /// and the exact-incarnation target of best-effort `abandon` cleanup. ManifestId stageManifest(std::vector entries); @@ -233,7 +233,7 @@ class PartWriteTxn /// ref-log entry. /// 1. `Materialized` leaves are already protected by the durable precommit edge, so no writer-side retired-view /// refresh is needed; - /// 2. consume this transaction's staged proof, or stream-read and validate a manifest not staged here; + /// 2. use this transaction's `StagedManifest`, or stream-read and validate a manifest not staged here; /// 3. `TrustedManifest` leaves use the durable source-manifest edge with no per-file HEAD/loadMeta /// probe; a genuinely absent trusted blob is an invariant violation caught by fsck, not here; /// 4. a body-absent or invalid manifest not staged here ⇒ retry-later exception before the append; @@ -340,7 +340,7 @@ class PartWriteTxn /// dependencies are blob-only, so `ObjectKind` is not part of the key. using DepKey = BlobRef; - struct StagedManifestProof + struct StagedManifest { Etag incarnation; std::vector blob_dependencies; @@ -396,7 +396,7 @@ class PartWriteTxn /// Entries are never erased while the transaction lives: membership is the A3 ABA barrier checked /// by `precommitAdd` and the reason `promote` may skip the body read. Erasing an entry (for example /// after cleanup or after promote) would silently weaken both. - std::unordered_map staged_manifest_proofs; + std::unordered_map staged_manifests; std::map deps; /// dependencies recorded by this build (blobs only) From e98f1286d1b4c7ceccf746df416ad0041cc3832c Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 9 Oct 2026 17:29:18 +0200 Subject: [PATCH 10/19] Document staged manifest record order and external body loss The blob dependencies of a `StagedManifest` keep the order the caller passed the entries, which can differ from the path-sorted encoded body. `promote` does not detect a staged body removed outside the protocol; the read path and `fsck` report it later. Co-Authored-By: Claude Opus 5.5 --- .../en/antalya/cas/architecture/manifests-and-refs.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/en/antalya/cas/architecture/manifests-and-refs.md b/docs/en/antalya/cas/architecture/manifests-and-refs.md index 63a7cd0a5196a..a73a253f910c2 100644 --- a/docs/en/antalya/cas/architecture/manifests-and-refs.md +++ b/docs/en/antalya/cas/architecture/manifests-and-refs.md @@ -56,10 +56,19 @@ the **same** blobs and moves the ref in one ref-log record — a repoint, covere [part-lifecycle page](/antalya/cas/architecture/part-lifecycle#repoint). The writer retains a compact record after its own successful `stageManifest`: the confirmed object -incarnation and the ordered blob dependencies. The matching `promote` uses this record without +incarnation and the blob dependencies in the order the caller passed the entries. The encoded body +sorts entries by path, so this order can differ from a decoded body. Only the order of dependency +checks and `BlobReuseAdopt` events depends on it. The matching `promote` uses this record without reading the immutable body back. Promotion of a manifest not staged by that `PartWriteTxn` still performs an exact `GET` and validates `RefMatchesBody` and `ManifestNamespaceMatches` fail-closed. +The record only shows that the body existed when the conditional create was confirmed. Inside the +protocol nothing deletes it before `promote`: the orphan sweep does not touch the prefix of a +build sequence that is still active. A body removed outside the protocol (a bucket lifecycle or +expiry rule on `cas/manifests/`, a manual delete) is not detected by `promote`, which commits the +ref anyway. The read path then fails with `FILE_DOESNT_EXIST`, and `fsck` reports the reference +as dangling. Do not put lifecycle or expiry rules on the pool prefix. + ## Manifest lifecycle and the orphan sweep {#manifest-lifecycle} ```mermaid From a923533e8909ef9b03ac391b8a4565163480c7f9 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Mon, 12 Oct 2026 19:05:05 +0200 Subject: [PATCH 11/19] Fix CAS orphan sweep for staged manifests --- .../Gc/CasOrphanManifestSweep.cpp | 52 +++++++---- .../tests/gtest_cas_orphan_manifest_sweep.cpp | 92 +++++++++++++++++-- 2 files changed, 120 insertions(+), 24 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp index 28a471b673ff6..bda2c0ff9cb67 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp @@ -722,6 +722,8 @@ ManifestSweepResult planManifestCursorPage( /// catalog cut: a manifest key is write-once, so its bytes do not depend on when they are read, /// and the only thing a later read can observe differently is absence, which retains. std::vector candidates; + /// Candidate namespaces that need a second ref_catalog GET after reading the floor. + std::set catalog_recheck_namespaces; for (const ListedKey & listed : page.keys) { @@ -758,23 +760,21 @@ ManifestSweepResult planManifestCursorPage( continue; } - const CatalogEntry * catalog_entry = catalogEntryOf(catalog_cut, parsed->ns); - if (catalog_entry) + auto [floor_it, floor_inserted] = floor_by_ns.emplace(parsed->ns.string(), std::nullopt); + if (floor_inserted) { - auto [floor_it, floor_inserted] = floor_by_ns.emplace(parsed->ns.string(), std::nullopt); - if (floor_inserted) - { - ++result.floor_lookups; - floor_it->second = floorForNamespace(op, layout, parsed->ns, &result.floor_reads); - } - if (!prefixEligibleUnder(floor_it->second, parsed->prefix)) - { - ++result.skipped; - decided_through = listed.key; - continue; - } + ++result.floor_lookups; + floor_it->second = floorForNamespace(op, layout, parsed->ns, &result.floor_reads); + } + if (!prefixEligibleUnder(floor_it->second, parsed->prefix)) + { + ++result.skipped; + decided_through = listed.key; + continue; } + const CatalogEntry * catalog_entry = catalogEntryOf(catalog_cut, parsed->ns); + auto [view_it, view_inserted] = view_by_ns.emplace(parsed->ns.string(), NamespaceFoldView{}); auto [active_it, inserted] = active_by_ns.emplace(parsed->ns.string(), std::set{}); if (inserted) @@ -882,10 +882,6 @@ ManifestSweepResult planManifestCursorPage( continue; } - /// If the post-observation catalog cut has no row, this candidate was necessarily created by a - /// now-dead life: creation publishes its row before any life-owned object. No current life can - /// name it, and a concurrent later creation can only replace its token after this observation. - /// A catalog-named row instead needs the ordinary coverage/owner proof below. if (catalog_entry) { /// THE §6 SAFETY FLOOR, the same predicate `sweepNamespace` calls, under the same watermark @@ -915,10 +911,30 @@ ManifestSweepResult planManifestCursorPage( } } + if (!catalog_entry) + catalog_recheck_namespaces.insert(parsed->ns.string()); candidates.push_back(*parsed); decided_through = listed.key; } + /// Between a successful stageManifest body PUT and a successful precommitAdd owner transition, the + /// body already exists without a durable owner edge; the initial catalog cut may still miss its namespace. + /// Recheck it after the floor read before deleting the body as orphaned. + if (!catalog_recheck_namespaces.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) + { + if (!catalog_recheck_namespaces.contains(candidate.ns.string())) + return false; + if (!catalogEntryOf(post_floor_catalog, candidate.ns)) + return false; + ++result.skipped; + return true; + }); + } + size_t next_body_hint = 0; for (const ListedManifestObject & candidate : candidates) { diff --git a/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp b/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp index 2c68bf8492fd0..3d6ac4d52f9c2 100644 --- a/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp +++ b/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp @@ -11,6 +11,7 @@ #include "cas_sweep_test_support.h" #include "cas_test_helpers.h" #include +#include #include #include #include @@ -159,6 +160,35 @@ class ReplacingManifestAfterObservationBackend : public InMemoryBackend bool listed_page = false; bool replaced_manifest = false; }; + +class CatalogChangesOnFloorReadBackend : public InMemoryBackend +{ +public: + void arm(const String & mount_key_, std::function callback_) + { + mount_key = mount_key_; + callback = std::move(callback_); + fired = false; + } + + bool didFire() const { return fired; } + + std::optional read(const String & key, TransportAccess & access) override + { + if (!fired && callback && key == mount_key) + { + fired = true; + auto current_callback = std::move(callback); + current_callback(); + } + return InMemoryBackend::read(key, access); + } + +private: + String mount_key; + std::function callback; + bool fired = false; +}; } /// A staged-but-unowned body in an ELIGIBLE prefix, absent from the owner view, is deleted (#7). @@ -370,25 +400,75 @@ TEST(CASOrphanManifestSweep, CursorPageRespectsDeleteBudget) EXPECT_NE(first_exists, second_exists); } -/// A physical manifest captured before a catalog cut which omits its name is dead-life debris: a live -/// creation cannot publish a life-owned object before its catalog row. It therefore has an eventual -/// page-sweep owner without trying to reconstruct a deleted incarnation from the key. -TEST(CASOrphanManifestSweep, CursorPageDeletesObservedBodyWhenCatalogOmitsNamespace) +TEST(CASOrphanManifestSweep, CursorPageRetainsRowlessBodyWithoutMountLease) { auto backend = std::make_shared(); auto store = openPoolForTest(backend); const RootNamespace ns{"00/catalog-absent-debris@cas@"}; const ManifestRef r = ref(5, 0xA9); writeManifestRaw(*backend, store->layout(), ns, r, {blobEntryFor("debris", DB::UInt128(9))}); - /// No mount lease/watermark exists: after legal catalog-row deletion there may be no server-root - /// state left to supply one. The post-observation absent row is the complete dead-life proof. const ManifestSweepResult result = sweepManifestCursorPageForTest(*store, "", /*list_budget=*/100, /*delete_budget=*/10); + EXPECT_EQ(result.floor_lookups, 1u); + EXPECT_EQ(result.deleted, 0u); + EXPECT_TRUE(headExists(*backend, store->layout().manifestKey(ManifestId{ns, r}))); +} + +TEST(CASOrphanManifestSweep, CursorPageRetainsRowlessBodyOfActiveBuild) +{ + auto backend = std::make_shared(); + auto store = openPoolForTest(backend); + const RootNamespace ns{"00/catalog-absent-active@cas@"}; + const ManifestRef r = ref(5, 0xA9); + writeManifestRaw(*backend, store->layout(), ns, r, {blobEntryFor("active", DB::UInt128(9))}); + setWatermarkMinActive(*backend, store->layout(), kServerRoot, kWriterEpoch, 5); + + const ManifestSweepResult result = sweepManifestCursorPageForTest(*store, "", /*list_budget=*/100, /*delete_budget=*/10); + + EXPECT_EQ(result.floor_lookups, 1u); + EXPECT_EQ(result.deleted, 0u); + EXPECT_TRUE(headExists(*backend, store->layout().manifestKey(ManifestId{ns, r}))); +} + +TEST(CASOrphanManifestSweep, CursorPageDeletesRowlessBodyBelowFloor) +{ + auto backend = std::make_shared(); + auto store = openPoolForTest(backend); + const RootNamespace ns{"00/catalog-absent-retired@cas@"}; + const ManifestRef r = ref(5, 0xA9); + writeManifestRaw(*backend, store->layout(), ns, r, {blobEntryFor("retired", DB::UInt128(9))}); + setWatermarkMinActive(*backend, store->layout(), kServerRoot, kWriterEpoch, 6); + + const ManifestSweepResult result = sweepManifestCursorPageForTest(*store, "", /*list_budget=*/100, /*delete_budget=*/10); + + EXPECT_EQ(result.floor_lookups, 1u); EXPECT_EQ(result.deleted, 1u); EXPECT_FALSE(headExists(*backend, store->layout().manifestKey(ManifestId{ns, r}))); } +TEST(CASOrphanManifestSweep, CursorPageRetainsRowlessBodyWhenNamespaceAppearsAfterCatalogCut) +{ + auto backend = std::make_shared(); + auto store = openPoolForTest(backend); + const RootNamespace ns{"00/catalog-appears-after-cut@cas@"}; + const ManifestRef r = ref(5, 0xA9); + writeManifestRaw(*backend, store->layout(), ns, r, {blobEntryFor("committed", DB::UInt128(9))}); + setWatermarkMinActive(*backend, store->layout(), kServerRoot, kWriterEpoch, 5); + backend->arm(store->layout().mountKey(kServerRoot), [&] + { + publishCommittedTransition(*backend, store->layout(), ns, "part", std::nullopt, r); + setWatermarkMinActive(*backend, store->layout(), kServerRoot, kWriterEpoch, 6); + }); + + const ManifestSweepResult result = sweepManifestCursorPageForTest(*store, "", /*list_budget=*/100, /*delete_budget=*/10); + + EXPECT_TRUE(backend->didFire()); + EXPECT_EQ(result.floor_lookups, 1u); + EXPECT_EQ(result.deleted, 0u); + EXPECT_TRUE(headExists(*backend, store->layout().manifestKey(ManifestId{ns, r}))); +} + /// Manifest keys are write-once, so a same-key rewrite after the observation below never happens in /// production; this backend forces one anyway to prove the page does not need the old freeze-before-cut /// discipline to stay correct. The body is read only after the catalog cut, so it sees whatever From f79cc0df93fd54525c92a471171fce62038d68a6 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Mon, 12 Oct 2026 19:07:35 +0200 Subject: [PATCH 12/19] Use unordered set for catalog rechecks --- .../ContentAddressed/Gc/CasOrphanManifestSweep.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp index bda2c0ff9cb67..402c19867e942 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp @@ -20,6 +20,7 @@ #include #include #include +#include namespace ProfileEvents { @@ -723,7 +724,7 @@ ManifestSweepResult planManifestCursorPage( /// and the only thing a later read can observe differently is absence, which retains. std::vector candidates; /// Candidate namespaces that need a second ref_catalog GET after reading the floor. - std::set catalog_recheck_namespaces; + std::unordered_set catalog_recheck_namespaces; for (const ListedKey & listed : page.keys) { From e198e5ee7023e43820c9cb584b4acbfc5a4ae1ac Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Tue, 13 Oct 2026 13:07:15 +0200 Subject: [PATCH 13/19] Rename CAS orphan sweep floor helpers --- .../ContentAddressed/Gc/CasGc.cpp | 4 +-- .../Gc/CasOrphanManifestSweep.cpp | 30 +++++++++---------- .../Gc/CasOrphanManifestSweep.h | 20 ++++++------- .../Tools/CasDecommission.cpp | 2 +- .../ContentAddressed/Tools/CasFsck.cpp | 2 +- .../tests/gtest_cas_orphan_sweep_requests.cpp | 14 ++++----- 6 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasGc.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasGc.cpp index 58dfd377cca81..cecc0e69c6f88 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasGc.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasGc.cpp @@ -2989,7 +2989,7 @@ Gc::FoldResult Gc::fold(GcState & state, std::optional & /*state_etag*/, /// clamps every round with no terminal resolution. Fail-closed: no durable watermark (the /// build cannot be proven dead) keeps the barrier. if (edge.change > 0 && edge.owner_kind == RefOwnerKind::Precommit - && prefixEligible(*store, ns, + && isBuildPrefixCoveredByFloor(*store, ns, BuildPrefix{.writer_epoch = edge.manifest_id.ref.writer_epoch, .build_sequence = edge.manifest_id.ref.build_sequence})) { @@ -4846,7 +4846,7 @@ RebuildReport Gc::rebuildBaseline(bool force) if (!parsed) return true; /// foreign key shape — debris const ManifestRef & mref = parsed->ref; - if (prefixEligible(*store, ns, BuildPrefix{mref.writer_epoch, mref.build_sequence})) + if (isBuildPrefixCoveredByFloor(*store, ns, BuildPrefix{mref.writer_epoch, mref.build_sequence})) return true; /// provably dead — the orphan sweep's territory, never an edge const ManifestId id{ns, mref}; if (foldManifestEdges(reads, id, +1, deltas, mf_cleanup_unused, /*txn_ordinal=*/0)) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp index 402c19867e942..72831dbc90954 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp @@ -46,8 +46,8 @@ void onGcEnumerationPage() /// mount there is no deletion authority, so the caller must leave the prefix untouched. The mount's /// `writer_epoch` and `min_active_build_sequence` are the single durable epoch/floor pair used for eligibility, including /// across process replacement and the retired sentinel. -std::optional floorForNamespace(CasOperation & op, const Layout & layout, const RootNamespace & ns, - uint64_t * reads = nullptr) +std::optional mountLeaseForNamespace(CasOperation & op, const Layout & layout, const RootNamespace & ns, + uint64_t * reads = nullptr) { const String & value = ns.string(); size_t pos = value.size(); @@ -530,32 +530,32 @@ namespace /// do not replace that authority check with a frozen-sequence or judged-dead guess. Compare /// `writer_epoch` first, then `build_sequence`, so old-epoch /// debris drains after a process restart even when its build_sequence is above the current min_active_build_sequence. -bool prefixEligibleOn(CasOperation & op, const Layout & layout, const RootNamespace & ns, const BuildPrefix & prefix) +bool isBuildPrefixCoveredByFloorOn( + CasOperation & op, const Layout & layout, const RootNamespace & ns, const BuildPrefix & prefix) { - return prefixEligibleUnder(floorForNamespace(op, layout, ns), prefix); + return isBuildPrefixCoveredByFloor(mountLeaseForNamespace(op, layout, ns), prefix); } } -bool prefixEligibleUnder(const std::optional & floor, const BuildPrefix & prefix) +bool isBuildPrefixCoveredByFloor(const std::optional & floor, const BuildPrefix & prefix) { if (!floor) return false; - const MountLease & w = *floor; - if (prefix.writer_epoch < w.writer_epoch) + if (floor->writer_epoch > prefix.writer_epoch) return true; - if (prefix.writer_epoch > w.writer_epoch) + if (floor->writer_epoch < prefix.writer_epoch) return false; - if (w.min_active_build_sequence == std::numeric_limits::max()) + if (floor->min_active_build_sequence == std::numeric_limits::max()) return true; /// farewell/retired sentinel: every seq is retired - return w.min_active_build_sequence > prefix.build_sequence; + return floor->min_active_build_sequence > prefix.build_sequence; } -bool prefixEligible(Pool & store, const RootNamespace & ns, const BuildPrefix & prefix) +bool isBuildPrefixCoveredByFloor(Pool & store, const RootNamespace & ns, const BuildPrefix & prefix) { CasOperation op = store.openRequests().admit(); - return prefixEligibleOn(op, store.layout(), ns, prefix); + return isBuildPrefixCoveredByFloorOn(op, store.layout(), ns, prefix); } uint64_t sweepNamespace(Pool & store, const RootNamespace & ns, const BuildPrefix & prefix, @@ -563,7 +563,7 @@ uint64_t sweepNamespace(Pool & store, const RootNamespace & ns, const BuildPrefi { const Layout & layout = store.layout(); CasOperation op = store.openRequests().admit(); - if (!prefixEligibleOn(op, layout, ns, prefix)) + if (!isBuildPrefixCoveredByFloorOn(op, layout, ns, prefix)) return 0; /// not eligible by the durable watermark fact — delete nothing (controls #8/#9) /// Build the protection view. A missing snapshot body, an invalid transaction, or an incomplete @@ -765,9 +765,9 @@ ManifestSweepResult planManifestCursorPage( if (floor_inserted) { ++result.floor_lookups; - floor_it->second = floorForNamespace(op, layout, parsed->ns, &result.floor_reads); + floor_it->second = mountLeaseForNamespace(op, layout, parsed->ns, &result.floor_reads); } - if (!prefixEligibleUnder(floor_it->second, parsed->prefix)) + if (!isBuildPrefixCoveredByFloor(floor_it->second, parsed->prefix)) { ++result.skipped; decided_through = listed.key; diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.h b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.h index d4da7e38f5a5a..f0c8332173c5a 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.h @@ -190,17 +190,17 @@ struct ManifestSweepResult uint64_t sweepNamespace(Pool & store, const RootNamespace & ns, const BuildPrefix & prefix, std::vector * warnings = nullptr); -/// Whether `prefix` is sweep-eligible by the durable watermark fact alone. The floor is read from the -/// mount lease identified by the namespace's server-root prefix, not inferred from the manifest key or a -/// judged-dead heuristic. A missing lease provides no deletion authority, so the prefix is not eligible. -bool prefixEligible(Pool & store, const RootNamespace & ns, const BuildPrefix & prefix); - -/// The pure half of `prefixEligible`: whether `prefix` is retired under one observation of the mount -/// floor. `nullopt` (no mount body under any prefix of the namespace) admits nothing. Retirement is -/// permanent -- the epoch and the acknowledgement floor only grow and the farewell is terminal -- so -/// an admission derived from any observation stays true afterwards, which is what lets a page judge +/// Whether `prefix` is covered by the durable watermark fact alone. The floor is read from the mount +/// lease identified by the namespace's server-root prefix, not inferred from the manifest key or a +/// judged-dead heuristic. A missing lease provides no deletion authority, so the prefix is not covered. +bool isBuildPrefixCoveredByFloor(Pool & store, const RootNamespace & ns, const BuildPrefix & prefix); + +/// The pure overload: whether `prefix` is covered by one observation of the mount floor. `nullopt` +/// (no mount body under any prefix of the namespace) covers nothing. Coverage is permanent -- the epoch +/// and the acknowledgement floor only grow and the farewell is terminal -- so a result derived from +/// any observation stays true afterwards, which is what lets a page judge /// every build of a namespace against one read. -bool prefixEligibleUnder(const std::optional & floor, const BuildPrefix & prefix); +bool isBuildPrefixCoveredByFloor(const std::optional & floor, const BuildPrefix & prefix); /// Plan one cursor page without deleting. A key is decided from key-derived facts first; only a /// candidate's body is read, after the catalog cut, then decoded and identity-validated; its exact diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Tools/CasDecommission.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Tools/CasDecommission.cpp index eef2ba04d9bf4..56ff9fd565df9 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Tools/CasDecommission.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Tools/CasDecommission.cpp @@ -269,7 +269,7 @@ DecommissionReport decommissionPoolMember(BackendPtr backend, PoolConfig config, } /// Manifest debris must be removed before the mount slot: deleting the mount body removes the - /// watermark authority, after which `floorForNamespace` returns no value and the ordinary orphan + /// watermark authority, after which `mountLeaseForNamespace` returns no value and the ordinary orphan /// sweep cannot prove that old-epoch debris is eligible. The decommission claim has advanced the /// writer epoch, so every build prefix with `prefix.writer_epoch < w.writer_epoch` is eligible here. /// Group the listed keys by namespace and build prefix so each group can use the exact-token orphan diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Tools/CasFsck.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Tools/CasFsck.cpp index 8b67d00251a17..9811314844972 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Tools/CasFsck.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Tools/CasFsck.cpp @@ -1115,7 +1115,7 @@ void runFsckImpl(Pool & store, bool detail, const FsckProgress & on_progress, co o.kind = ObjectKind::Blob; o.size = sz; o.cls = FsckClass::Unreachable; - if (parsed && prefixEligible(store, ns, prefix)) + if (parsed && isBuildPrefixCoveredByFloor(store, ns, prefix)) o.reachable_from = {"reclaimable-pre-precommit"}; else o.reachable_from = {"in-flight-pre-precommit"}; diff --git a/src/Disks/tests/gtest_cas_orphan_sweep_requests.cpp b/src/Disks/tests/gtest_cas_orphan_sweep_requests.cpp index 34e87af705ed4..eb69fa3ec018e 100644 --- a/src/Disks/tests/gtest_cas_orphan_sweep_requests.cpp +++ b/src/Disks/tests/gtest_cas_orphan_sweep_requests.cpp @@ -136,18 +136,18 @@ TEST(CASOrphanSweepRequests, RetainedKeysCostNoBodyRead) EXPECT_EQ(f.backend->getCount(layout.manifestKey(ManifestId{kNs, build(seq)})), 0u) << seq; } -TEST(CASOrphanSweepRequests, PrefixEligibleUnderIsTheFourComparisons) +TEST(CASOrphanSweepRequests, BuildPrefixCoveredByFloorUsesTheFourComparisons) { MountLease floor; floor.writer_epoch = 3; floor.min_active_build_sequence = 10; - EXPECT_TRUE(prefixEligibleUnder(floor, BuildPrefix{.writer_epoch = 2, .build_sequence = 999})); - EXPECT_FALSE(prefixEligibleUnder(floor, BuildPrefix{.writer_epoch = 4, .build_sequence = 1})); - EXPECT_TRUE(prefixEligibleUnder(floor, BuildPrefix{.writer_epoch = 3, .build_sequence = 9})); - EXPECT_FALSE(prefixEligibleUnder(floor, BuildPrefix{.writer_epoch = 3, .build_sequence = 10})); + EXPECT_TRUE(isBuildPrefixCoveredByFloor(floor, BuildPrefix{.writer_epoch = 2, .build_sequence = 999})); + EXPECT_FALSE(isBuildPrefixCoveredByFloor(floor, BuildPrefix{.writer_epoch = 4, .build_sequence = 1})); + EXPECT_TRUE(isBuildPrefixCoveredByFloor(floor, BuildPrefix{.writer_epoch = 3, .build_sequence = 9})); + EXPECT_FALSE(isBuildPrefixCoveredByFloor(floor, BuildPrefix{.writer_epoch = 3, .build_sequence = 10})); floor.min_active_build_sequence = std::numeric_limits::max(); - EXPECT_TRUE(prefixEligibleUnder(floor, BuildPrefix{.writer_epoch = 3, .build_sequence = 10})); - EXPECT_FALSE(prefixEligibleUnder(std::nullopt, BuildPrefix{.writer_epoch = 1, .build_sequence = 1})); + EXPECT_TRUE(isBuildPrefixCoveredByFloor(floor, BuildPrefix{.writer_epoch = 3, .build_sequence = 10})); + EXPECT_FALSE(isBuildPrefixCoveredByFloor(std::nullopt, BuildPrefix{.writer_epoch = 1, .build_sequence = 1})); } namespace From 68cdc1d92e05d458f488ceae66e975512230210e Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Tue, 13 Oct 2026 13:23:46 +0200 Subject: [PATCH 14/19] Add CAS orphan sweep namespace rebirth test Reproduce a namespace changing incarnation after the sweep catalog cut. The old owner view is empty while the successor life commits the same manifest, so the stale candidate must be retained. --- .../tests/gtest_cas_orphan_manifest_sweep.cpp | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp b/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp index 3d6ac4d52f9c2..6bfe22e5f8005 100644 --- a/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp +++ b/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp @@ -469,6 +469,51 @@ TEST(CASOrphanManifestSweep, CursorPageRetainsRowlessBodyWhenNamespaceAppearsAft EXPECT_TRUE(headExists(*backend, store->layout().manifestKey(ManifestId{ns, r}))); } +TEST(CASOrphanManifestSweep, CursorPageRetainsBodyWhenNamespaceIsRebornAfterCatalogCut) +{ + auto backend = std::make_shared(); + auto store = openPoolForTest(backend); + const RootNamespace ns{"00/catalog-life-changes-after-cut@cas@"}; + fixture::admitLive(*backend, store->layout(), ns); + seedEmptyRecoveryAuthority(*backend, store->layout(), ns); + + CasOperation op = store->openRequests().admit(); + const CasRefCatalog::Snapshot initial_catalog = CasRefCatalog::read(op, store->layout()); + ASSERT_EQ(initial_catalog.catalog.entries.size(), 1u); + const CatalogEntry predecessor = initial_catalog.catalog.entries.front(); + CatalogEntry successor = predecessor; + successor.incarnation = DB::UInt128(0xBEEF); + + const ManifestRef r = ref(5, 0xAA); + const String key = store->layout().manifestKey(ManifestId{ns, r}); + writeManifestRaw(*backend, store->layout(), ns, r, {blobEntryFor("committed", DB::UInt128(9))}); + setWatermarkMinActive(*backend, store->layout(), kServerRoot, kWriterEpoch, 6); + seedConsumedSealCursor(*backend, store->layout(), ns); + backend->arm(store->layout().mountKey(kServerRoot), [&] + { + OperationForTest replace_op(*backend); + const CasRefCatalog::Snapshot current = CasRefCatalog::read(*replace_op, store->layout()); + if (!current.etag) + throw std::runtime_error("test catalog has no token"); + RefCatalog reborn = current.catalog; + const auto it = std::find(reborn.entries.begin(), reborn.entries.end(), predecessor); + if (it == reborn.entries.end()) + throw std::runtime_error("test predecessor catalog row disappeared"); + *it = successor; + if (!std::holds_alternative((*replace_op).replace( + store->layout().refCatalogKey(), encodeRefCatalog(reborn), *current.etag, Retry::standard()))) + throw std::runtime_error("test catalog replacement conflicted"); + publishCommittedTransition(*backend, store->layout(), ns, "part", std::nullopt, r); + }); + + const ManifestSweepResult result = sweepManifestCursorPageForTest(*store, "", 100, 10); + + EXPECT_TRUE(backend->didFire()); + EXPECT_TRUE(result.nominations.empty()); + EXPECT_EQ(result.deleted, 0u); + EXPECT_TRUE(headExists(*backend, key)); +} + /// Manifest keys are write-once, so a same-key rewrite after the observation below never happens in /// production; this backend forces one anyway to prove the page does not need the old freeze-before-cut /// discipline to stay correct. The body is read only after the catalog cut, so it sees whatever From 593311dcb922ce797d8b79721b5f59f3fc2b33cf Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Tue, 13 Oct 2026 13:34:50 +0200 Subject: [PATCH 15/19] Protect CAS orphan sweep across namespace rebirth A namespace can change incarnation after the sweep catalog cut while the successor life commits the candidate manifest. Recheck every candidate row by full value and retain it when the catalog life changed. --- .../Gc/CasOrphanManifestSweep.cpp | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp index 72831dbc90954..34e24375556a1 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp @@ -20,7 +20,6 @@ #include #include #include -#include namespace ProfileEvents { @@ -723,9 +722,6 @@ ManifestSweepResult planManifestCursorPage( /// catalog cut: a manifest key is write-once, so its bytes do not depend on when they are read, /// and the only thing a later read can observe differently is absence, which retains. std::vector candidates; - /// Candidate namespaces that need a second ref_catalog GET after reading the floor. - std::unordered_set catalog_recheck_namespaces; - for (const ListedKey & listed : page.keys) { ++result.listed; @@ -912,24 +908,19 @@ ManifestSweepResult planManifestCursorPage( } } - if (!catalog_entry) - catalog_recheck_namespaces.insert(parsed->ns.string()); candidates.push_back(*parsed); decided_through = listed.key; } - /// Between a successful stageManifest body PUT and a successful precommitAdd owner transition, the - /// body already exists without a durable owner edge; the initial catalog cut may still miss its namespace. - /// Recheck it after the floor read before deleting the body as orphaned. - if (!catalog_recheck_namespaces.empty()) + 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) { - if (!catalog_recheck_namespaces.contains(candidate.ns.string())) - return false; - if (!catalogEntryOf(post_floor_catalog, candidate.ns)) + const CatalogEntry * before = catalogEntryOf(catalog_cut, candidate.ns); + const CatalogEntry * after = catalogEntryOf(post_floor_catalog, candidate.ns); + if ((!before && !after) || (before && after && *before == *after)) return false; ++result.skipped; return true; From 7d7fcc951490e24e8f59a1ec2d09f6106d861733 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Wed, 14 Oct 2026 11:51:10 +0200 Subject: [PATCH 16/19] Add red test for invalid CAS manifest paths --- src/Disks/tests/gtest_cas_part_write.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/Disks/tests/gtest_cas_part_write.cpp b/src/Disks/tests/gtest_cas_part_write.cpp index f570e7f725e32..b019784991f1a 100644 --- a/src/Disks/tests/gtest_cas_part_write.cpp +++ b/src/Disks/tests/gtest_cas_part_write.cpp @@ -2452,6 +2452,27 @@ TEST(CASPartWriteTxn, ManifestCapEncodedBytesOverThrowsBeforeBodyWrite) << "stageManifest must fail closed before writing the manifest body, leaving no new objects"; } +TEST(CASPartWriteTxn, StageManifestRejectsTraversalPathBeforeBodyWrite) +{ + auto backend = std::make_shared(); + auto store = openPool(backend); + const RootNamespace ns{"srv1/traversal-path"}; + auto build = startBuildFor(store, ns, "part"); + + ManifestEntry entry; + entry.path = "../evil"; + entry.placement = EntryPlacement::Inline; + entry.inline_bytes = "payload"; + + expectThrowsCode(DB::ErrorCodes::CORRUPTED_DATA, [&] + { + build->stageManifest({entry}); + }); + + OperationForTest op(*backend); + EXPECT_TRUE((*op).list(store->layout().manifestNamespacePrefix(ns), "", 10, Retry::once()).keys.empty()); +} + /// spec §9.9 (mixed-algo pools, Phase 3 T2) — the W-DEP-SET cross-satisfaction crux: a manifest with /// two entries carrying the SAME digest VALUE under TWO DIFFERENT algos (`ch128:X` / `xxh3:X`). Only /// `ch128:X`'s body is ever putBlob'd; `xxh3:X`'s body never lands anywhere. Promote MUST fail closed — From 470fb0e4d03da6cd0133fc6693c2c57f51214415 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 16 Oct 2026 14:24:47 +0200 Subject: [PATCH 17/19] Add CAS manifest validation regression tests --- .../tests/gtest_cas_part_manifest_format.cpp | 40 ++++-- src/Disks/tests/gtest_cas_part_write.cpp | 123 ++++++++++-------- 2 files changed, 99 insertions(+), 64 deletions(-) diff --git a/src/Disks/tests/gtest_cas_part_manifest_format.cpp b/src/Disks/tests/gtest_cas_part_manifest_format.cpp index dd6eab02c0030..b8571a5855724 100644 --- a/src/Disks/tests/gtest_cas_part_manifest_format.cpp +++ b/src/Disks/tests/gtest_cas_part_manifest_format.cpp @@ -7,6 +7,11 @@ #include +namespace DB::ErrorCodes +{ +extern const int LIMIT_EXCEEDED; +} + using namespace DB::Cas; namespace @@ -316,9 +321,6 @@ TEST(CASPartManifestFormat, MixedAlgoEntriesRoundTrip) EXPECT_EQ(got.entries[1].blob_size, 200u); } -/// Builds a single-Blob-entry manifest whose entry path is exactly `path` -- `encodePartManifest` -/// itself does not validate path shape (only ordering/duplicates), so this lets the negative cases -/// below reach `decodePartManifest`'s shape check unobstructed. static PartManifest manifestWithSinglePath(std::string_view path) { PartManifest m; @@ -336,20 +338,40 @@ static PartManifest manifestWithSinglePath(std::string_view path) return m; } -/// T11: manifest bytes arrive over the interserver relink channel, so decode enforces the same path -/// hygiene as CasLayout::checkNamespace -- relative, no empty/'.'/'..' segments, no leading '/'. -/// `encodePartManifest` does not itself reject these (see `manifestWithSinglePath`), so each case -/// must fail closed at decode time instead. +TEST(CASPartManifestFormat, EncodeRejectsMalformedEntryPaths) +{ + for (const char * path : {"../evil", "/abs", "", "a//b", "a/./b"}) + { + SCOPED_TRACE(path); + PartManifest manifest = manifestWithSinglePath("safe"); + manifest.entries[0].path = path; + expectThrowsCode(DB::ErrorCodes::CORRUPTED_DATA, [&] { encodePartManifest(manifest); }); + } +} + TEST(CASPartManifestFormat, DecodeRejectsMalformedEntryPaths) { + const String valid = encodePartManifest(manifestWithSinglePath("safe")); + const String needle = R"("path":"safe")"; + const size_t pos = valid.find(needle); + ASSERT_NE(pos, String::npos); + for (const char * path : {"../evil", "/abs", "", "a//b", "a/./b"}) { SCOPED_TRACE(path); - const String encoded = encodePartManifest(manifestWithSinglePath(path)); - expectThrowsCode(DB::ErrorCodes::CORRUPTED_DATA, [&] { decodePartManifest(encoded); }); + String forged = valid; + forged.replace(pos, needle.size(), "\"path\":\"" + String(path) + "\""); + expectThrowsCode(DB::ErrorCodes::CORRUPTED_DATA, [&] { decodePartManifest(forged); }); } } +TEST(CASPartManifestFormat, EncodeRejectsEntryRecordOverLineCap) +{ + PartManifest manifest = manifestWithSinglePath("safe"); + manifest.entries[0].path = String(70 * 1024, 'a'); + expectThrowsCode(DB::ErrorCodes::LIMIT_EXCEEDED, [&] { encodePartManifest(manifest); }); +} + /// Legal projection subdirectories (`.proj/`) must not be caught by the shape /// check above -- it is syntactic only, not a directory-depth restriction. TEST(CASPartManifestFormat, DecodeAcceptsLegalProjectionSubdirPath) diff --git a/src/Disks/tests/gtest_cas_part_write.cpp b/src/Disks/tests/gtest_cas_part_write.cpp index b019784991f1a..8517cf4f2b760 100644 --- a/src/Disks/tests/gtest_cas_part_write.cpp +++ b/src/Disks/tests/gtest_cas_part_write.cpp @@ -1,6 +1,5 @@ #include #include -#include #include #include #include @@ -2325,62 +2324,57 @@ namespace /// cap ever changes, update this one to match. constexpr uint64_t kExpectedManifestEncodedCap = 256ULL << 20; -/// The exact encoded size `PartWriteTxn::stageManifest` would compute for a single Blob-placement entry whose -/// path is `path_len` bytes long, staged under `ns` — measured through the SAME `encodePartManifest` -/// codec `stageManifest` calls, so this is an exact reproduction rather than a hand-derived estimate. -/// `ref` and `payload_digest` are fixed-width fields (20 and 16 bytes respectively): their VALUES don't -/// affect the encoded size, only their presence does, so the zero-valued placeholders here reproduce -/// the exact same byte count `stageManifest` would produce with its real (non-zero) values. -size_t manifestEncodedSizeForPathLen(const RootNamespace & ns, size_t path_len) +constexpr size_t kWideManifestPathBytes = 60'000; + +ManifestEntry wideBlobManifestEntry(size_t index) { - PartManifest probe; - probe.root_namespace_id = ns; ManifestEntry e; - e.path = String(path_len, 'a'); + e.path = std::to_string(index) + "/"; + e.path.resize(kWideManifestPathBytes, 'a'); e.placement = EntryPlacement::Blob; - e.ref = DB::Cas::BlobRef{DB::Cas::BlobHashAlgo::CityHash128, DB::Cas::BlobDigest::fromU128(UInt128{})}; - + e.ref = DB::Cas::BlobRef{DB::Cas::BlobHashAlgo::CityHash128, DB::Cas::BlobDigest::fromU128(UInt128{0x42})}; e.blob_size = 12345; - probe.entries = {std::move(e)}; - return encodePartManifest(probe).size(); + return e; } -/// Finds the exact boundary: the SMALLEST `path_len` whose single-entry manifest encodes to MORE than -/// `kExpectedManifestEncodedCap` bytes under `ns`. `path_len - 1` therefore encodes to AT MOST the cap -/// (the encoding is monotonic in `path_len` — a longer path can only grow the encoded size). Starts -/// from a linear estimate (the encoding is affine in `path_len`: fixed framing overhead plus a constant -/// number of bytes per path byte) and walks to the exact crossing, so this stays correct even if the -/// framing overhead changes, without needing a full binary search over a ~256 MiB range. -size_t findManifestEncodedCapBoundaryPathLen(const RootNamespace & ns) +std::vector wideBlobManifestEntries(size_t count) { - constexpr size_t probe_lo = 1000; - constexpr size_t probe_hi = 2'000'000; - const size_t size_lo = manifestEncodedSizeForPathLen(ns, probe_lo); - const size_t size_hi = manifestEncodedSizeForPathLen(ns, probe_hi); - const double slope = static_cast(size_hi - size_lo) / static_cast(probe_hi - probe_lo); - const double intercept = static_cast(size_lo) - slope * static_cast(probe_lo); + std::vector entries; + entries.reserve(count); + for (size_t i = 0; i < count; ++i) + entries.push_back(wideBlobManifestEntry(i)); + return entries; +} + +size_t estimatedManifestEncodedSizeForEntryCount(const RootNamespace & ns, size_t count) +{ + PartManifest empty; + empty.root_namespace_id = ns; + const size_t empty_size = encodePartManifest(empty).size(); - size_t path_len = static_cast(std::ceil( - (static_cast(kExpectedManifestEncodedCap) - intercept) / slope)) + 1; + PartManifest one = empty; + one.entries.push_back(wideBlobManifestEntry(0)); + const size_t entry_size = encodePartManifest(one).size() - empty_size; - while (manifestEncodedSizeForPathLen(ns, path_len) <= kExpectedManifestEncodedCap) - ++path_len; - while (path_len > 1 && manifestEncodedSizeForPathLen(ns, path_len - 1) > kExpectedManifestEncodedCap) - --path_len; - return path_len; + return empty_size + count * entry_size + std::to_string(count).size() - 1; } -/// A one-entry Blob ManifestEntry with a synthetic `path_len`-byte path (used only to inflate the -/// encoded manifest size towards the OQ7 cap). -ManifestEntry wideBlobManifestEntry(size_t path_len) +size_t firstManifestEntryCountOverEncodedCap(const RootNamespace & ns) { - ManifestEntry e; - e.path = String(path_len, 'a'); - e.placement = EntryPlacement::Blob; - e.ref = DB::Cas::BlobRef{DB::Cas::BlobHashAlgo::CityHash128, DB::Cas::BlobDigest::fromU128(UInt128{0x42})}; + size_t count = kExpectedManifestEncodedCap / kWideManifestPathBytes; + while (estimatedManifestEncodedSizeForEntryCount(ns, count) <= kExpectedManifestEncodedCap) + ++count; + while (estimatedManifestEncodedSizeForEntryCount(ns, count - 1) > kExpectedManifestEncodedCap) + --count; + return count; +} - e.blob_size = 12345; - return e; +size_t manifestEncodedSizeForEntryCount(const RootNamespace & ns, size_t count) +{ + PartManifest manifest; + manifest.root_namespace_id = ns; + manifest.entries = wideBlobManifestEntries(count); + return encodePartManifest(manifest).size(); } } @@ -2403,30 +2397,27 @@ TEST(CASPartWriteTxn, ManifestCapEncodedBytesJustUnderStagesSuccessfully) auto s = Pool::open(b, PoolConfig{.pool_prefix = "p", .server_root_id = "test", .boot_ms_fn = [] { return uint64_t{0}; }}); const RootNamespace ns{"srv1/tbl"}; - const size_t path_len_over = findManifestEncodedCapBoundaryPathLen(ns); - ASSERT_GT(path_len_over, 1u); - const size_t path_len_under = path_len_over - 1; - ASSERT_LE(manifestEncodedSizeForPathLen(ns, path_len_under), kExpectedManifestEncodedCap); + const size_t entry_count_over = firstManifestEntryCountOverEncodedCap(ns); + ASSERT_GT(entry_count_over, 1u); + const size_t entry_count_under = entry_count_over - 1; + ASSERT_LE(manifestEncodedSizeForEntryCount(ns, entry_count_under), kExpectedManifestEncodedCap); auto build = startBuildFor(s, ns, "wide_part"); - const ManifestId id = build->stageManifest({wideBlobManifestEntry(path_len_under)}); + const ManifestId id = build->stageManifest(wideBlobManifestEntries(entry_count_under)); EXPECT_EQ(id.root_namespace, ns); OperationForTest op(*b); EXPECT_TRUE((*op).head(s->layout().manifestKey(id), Retry::once()).has_value()) << "a just-under-cap manifest must actually be written"; } -/// Boundary case 2/2: a manifest whose encoded size exceeds the cap by the smallest possible margin -/// (one more path byte than the passing case above) throws `LIMIT_EXCEEDED` fail-closed, BEFORE the body -/// write — no manifest object lands in the backend for the rejected attempt. TEST(CASPartWriteTxn, ManifestCapEncodedBytesOverThrowsBeforeBodyWrite) { auto b = std::make_shared(); auto s = openPool(b); const RootNamespace ns{"srv1/tbl"}; - const size_t path_len_over = findManifestEncodedCapBoundaryPathLen(ns); - ASSERT_GT(manifestEncodedSizeForPathLen(ns, path_len_over), kExpectedManifestEncodedCap); + const size_t entry_count_over = firstManifestEntryCountOverEncodedCap(ns); + ASSERT_GT(manifestEncodedSizeForEntryCount(ns, entry_count_over), kExpectedManifestEncodedCap); auto build = startBuildFor(s, ns, "wide_part"); @@ -2435,7 +2426,7 @@ TEST(CASPartWriteTxn, ManifestCapEncodedBytesOverThrowsBeforeBodyWrite) bool threw = false; try { - build->stageManifest({wideBlobManifestEntry(path_len_over)}); + build->stageManifest(wideBlobManifestEntries(entry_count_over)); } catch (const DB::Exception & e) { @@ -2473,6 +2464,28 @@ TEST(CASPartWriteTxn, StageManifestRejectsTraversalPathBeforeBodyWrite) EXPECT_TRUE((*op).list(store->layout().manifestNamespacePrefix(ns), "", 10, Retry::once()).keys.empty()); } +TEST(CASPartWriteTxn, StageManifestRejectsEntryRecordOverLineCapBeforeBodyWrite) +{ + auto backend = std::make_shared(); + auto store = openPool(backend); + const RootNamespace ns{"srv1/entry-line-cap"}; + auto build = startBuildFor(store, ns, "part"); + + ManifestEntry entry; + entry.path = String(70 * 1024, 'a'); + entry.placement = EntryPlacement::Blob; + entry.ref = BlobRef{BlobHashAlgo::CityHash128, BlobDigest::fromU128(UInt128{0x42})}; + entry.blob_size = 1; + + expectThrowsCode(DB::ErrorCodes::LIMIT_EXCEEDED, [&] + { + build->stageManifest({entry}); + }); + + OperationForTest op(*backend); + EXPECT_TRUE((*op).list(store->layout().manifestNamespacePrefix(ns), "", 10, Retry::once()).keys.empty()); +} + /// spec §9.9 (mixed-algo pools, Phase 3 T2) — the W-DEP-SET cross-satisfaction crux: a manifest with /// two entries carrying the SAME digest VALUE under TWO DIFFERENT algos (`ch128:X` / `xxh3:X`). Only /// `ch128:X`'s body is ever putBlob'd; `xxh3:X`'s body never lands anywhere. Promote MUST fail closed — From 17dc11cc95248bbf18fd02c6758c2d7cf8096dca Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 16 Oct 2026 14:32:03 +0200 Subject: [PATCH 18/19] Narrow CAS manifest tests to path validation --- .../tests/gtest_cas_part_manifest_format.cpp | 12 -- src/Disks/tests/gtest_cas_part_write.cpp | 123 ++++++++---------- 2 files changed, 55 insertions(+), 80 deletions(-) diff --git a/src/Disks/tests/gtest_cas_part_manifest_format.cpp b/src/Disks/tests/gtest_cas_part_manifest_format.cpp index b8571a5855724..1a3e0fe6018ba 100644 --- a/src/Disks/tests/gtest_cas_part_manifest_format.cpp +++ b/src/Disks/tests/gtest_cas_part_manifest_format.cpp @@ -7,11 +7,6 @@ #include -namespace DB::ErrorCodes -{ -extern const int LIMIT_EXCEEDED; -} - using namespace DB::Cas; namespace @@ -365,13 +360,6 @@ TEST(CASPartManifestFormat, DecodeRejectsMalformedEntryPaths) } } -TEST(CASPartManifestFormat, EncodeRejectsEntryRecordOverLineCap) -{ - PartManifest manifest = manifestWithSinglePath("safe"); - manifest.entries[0].path = String(70 * 1024, 'a'); - expectThrowsCode(DB::ErrorCodes::LIMIT_EXCEEDED, [&] { encodePartManifest(manifest); }); -} - /// Legal projection subdirectories (`.proj/`) must not be caught by the shape /// check above -- it is syntactic only, not a directory-depth restriction. TEST(CASPartManifestFormat, DecodeAcceptsLegalProjectionSubdirPath) diff --git a/src/Disks/tests/gtest_cas_part_write.cpp b/src/Disks/tests/gtest_cas_part_write.cpp index 8517cf4f2b760..b019784991f1a 100644 --- a/src/Disks/tests/gtest_cas_part_write.cpp +++ b/src/Disks/tests/gtest_cas_part_write.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -2324,57 +2325,62 @@ namespace /// cap ever changes, update this one to match. constexpr uint64_t kExpectedManifestEncodedCap = 256ULL << 20; -constexpr size_t kWideManifestPathBytes = 60'000; - -ManifestEntry wideBlobManifestEntry(size_t index) +/// The exact encoded size `PartWriteTxn::stageManifest` would compute for a single Blob-placement entry whose +/// path is `path_len` bytes long, staged under `ns` — measured through the SAME `encodePartManifest` +/// codec `stageManifest` calls, so this is an exact reproduction rather than a hand-derived estimate. +/// `ref` and `payload_digest` are fixed-width fields (20 and 16 bytes respectively): their VALUES don't +/// affect the encoded size, only their presence does, so the zero-valued placeholders here reproduce +/// the exact same byte count `stageManifest` would produce with its real (non-zero) values. +size_t manifestEncodedSizeForPathLen(const RootNamespace & ns, size_t path_len) { + PartManifest probe; + probe.root_namespace_id = ns; ManifestEntry e; - e.path = std::to_string(index) + "/"; - e.path.resize(kWideManifestPathBytes, 'a'); + e.path = String(path_len, 'a'); e.placement = EntryPlacement::Blob; - e.ref = DB::Cas::BlobRef{DB::Cas::BlobHashAlgo::CityHash128, DB::Cas::BlobDigest::fromU128(UInt128{0x42})}; - e.blob_size = 12345; - return e; -} + e.ref = DB::Cas::BlobRef{DB::Cas::BlobHashAlgo::CityHash128, DB::Cas::BlobDigest::fromU128(UInt128{})}; -std::vector wideBlobManifestEntries(size_t count) -{ - std::vector entries; - entries.reserve(count); - for (size_t i = 0; i < count; ++i) - entries.push_back(wideBlobManifestEntry(i)); - return entries; + e.blob_size = 12345; + probe.entries = {std::move(e)}; + return encodePartManifest(probe).size(); } -size_t estimatedManifestEncodedSizeForEntryCount(const RootNamespace & ns, size_t count) +/// Finds the exact boundary: the SMALLEST `path_len` whose single-entry manifest encodes to MORE than +/// `kExpectedManifestEncodedCap` bytes under `ns`. `path_len - 1` therefore encodes to AT MOST the cap +/// (the encoding is monotonic in `path_len` — a longer path can only grow the encoded size). Starts +/// from a linear estimate (the encoding is affine in `path_len`: fixed framing overhead plus a constant +/// number of bytes per path byte) and walks to the exact crossing, so this stays correct even if the +/// framing overhead changes, without needing a full binary search over a ~256 MiB range. +size_t findManifestEncodedCapBoundaryPathLen(const RootNamespace & ns) { - PartManifest empty; - empty.root_namespace_id = ns; - const size_t empty_size = encodePartManifest(empty).size(); + constexpr size_t probe_lo = 1000; + constexpr size_t probe_hi = 2'000'000; + const size_t size_lo = manifestEncodedSizeForPathLen(ns, probe_lo); + const size_t size_hi = manifestEncodedSizeForPathLen(ns, probe_hi); + const double slope = static_cast(size_hi - size_lo) / static_cast(probe_hi - probe_lo); + const double intercept = static_cast(size_lo) - slope * static_cast(probe_lo); - PartManifest one = empty; - one.entries.push_back(wideBlobManifestEntry(0)); - const size_t entry_size = encodePartManifest(one).size() - empty_size; + size_t path_len = static_cast(std::ceil( + (static_cast(kExpectedManifestEncodedCap) - intercept) / slope)) + 1; - return empty_size + count * entry_size + std::to_string(count).size() - 1; + while (manifestEncodedSizeForPathLen(ns, path_len) <= kExpectedManifestEncodedCap) + ++path_len; + while (path_len > 1 && manifestEncodedSizeForPathLen(ns, path_len - 1) > kExpectedManifestEncodedCap) + --path_len; + return path_len; } -size_t firstManifestEntryCountOverEncodedCap(const RootNamespace & ns) +/// A one-entry Blob ManifestEntry with a synthetic `path_len`-byte path (used only to inflate the +/// encoded manifest size towards the OQ7 cap). +ManifestEntry wideBlobManifestEntry(size_t path_len) { - size_t count = kExpectedManifestEncodedCap / kWideManifestPathBytes; - while (estimatedManifestEncodedSizeForEntryCount(ns, count) <= kExpectedManifestEncodedCap) - ++count; - while (estimatedManifestEncodedSizeForEntryCount(ns, count - 1) > kExpectedManifestEncodedCap) - --count; - return count; -} + ManifestEntry e; + e.path = String(path_len, 'a'); + e.placement = EntryPlacement::Blob; + e.ref = DB::Cas::BlobRef{DB::Cas::BlobHashAlgo::CityHash128, DB::Cas::BlobDigest::fromU128(UInt128{0x42})}; -size_t manifestEncodedSizeForEntryCount(const RootNamespace & ns, size_t count) -{ - PartManifest manifest; - manifest.root_namespace_id = ns; - manifest.entries = wideBlobManifestEntries(count); - return encodePartManifest(manifest).size(); + e.blob_size = 12345; + return e; } } @@ -2397,27 +2403,30 @@ TEST(CASPartWriteTxn, ManifestCapEncodedBytesJustUnderStagesSuccessfully) auto s = Pool::open(b, PoolConfig{.pool_prefix = "p", .server_root_id = "test", .boot_ms_fn = [] { return uint64_t{0}; }}); const RootNamespace ns{"srv1/tbl"}; - const size_t entry_count_over = firstManifestEntryCountOverEncodedCap(ns); - ASSERT_GT(entry_count_over, 1u); - const size_t entry_count_under = entry_count_over - 1; - ASSERT_LE(manifestEncodedSizeForEntryCount(ns, entry_count_under), kExpectedManifestEncodedCap); + const size_t path_len_over = findManifestEncodedCapBoundaryPathLen(ns); + ASSERT_GT(path_len_over, 1u); + const size_t path_len_under = path_len_over - 1; + ASSERT_LE(manifestEncodedSizeForPathLen(ns, path_len_under), kExpectedManifestEncodedCap); auto build = startBuildFor(s, ns, "wide_part"); - const ManifestId id = build->stageManifest(wideBlobManifestEntries(entry_count_under)); + const ManifestId id = build->stageManifest({wideBlobManifestEntry(path_len_under)}); EXPECT_EQ(id.root_namespace, ns); OperationForTest op(*b); EXPECT_TRUE((*op).head(s->layout().manifestKey(id), Retry::once()).has_value()) << "a just-under-cap manifest must actually be written"; } +/// Boundary case 2/2: a manifest whose encoded size exceeds the cap by the smallest possible margin +/// (one more path byte than the passing case above) throws `LIMIT_EXCEEDED` fail-closed, BEFORE the body +/// write — no manifest object lands in the backend for the rejected attempt. TEST(CASPartWriteTxn, ManifestCapEncodedBytesOverThrowsBeforeBodyWrite) { auto b = std::make_shared(); auto s = openPool(b); const RootNamespace ns{"srv1/tbl"}; - const size_t entry_count_over = firstManifestEntryCountOverEncodedCap(ns); - ASSERT_GT(manifestEncodedSizeForEntryCount(ns, entry_count_over), kExpectedManifestEncodedCap); + const size_t path_len_over = findManifestEncodedCapBoundaryPathLen(ns); + ASSERT_GT(manifestEncodedSizeForPathLen(ns, path_len_over), kExpectedManifestEncodedCap); auto build = startBuildFor(s, ns, "wide_part"); @@ -2426,7 +2435,7 @@ TEST(CASPartWriteTxn, ManifestCapEncodedBytesOverThrowsBeforeBodyWrite) bool threw = false; try { - build->stageManifest(wideBlobManifestEntries(entry_count_over)); + build->stageManifest({wideBlobManifestEntry(path_len_over)}); } catch (const DB::Exception & e) { @@ -2464,28 +2473,6 @@ TEST(CASPartWriteTxn, StageManifestRejectsTraversalPathBeforeBodyWrite) EXPECT_TRUE((*op).list(store->layout().manifestNamespacePrefix(ns), "", 10, Retry::once()).keys.empty()); } -TEST(CASPartWriteTxn, StageManifestRejectsEntryRecordOverLineCapBeforeBodyWrite) -{ - auto backend = std::make_shared(); - auto store = openPool(backend); - const RootNamespace ns{"srv1/entry-line-cap"}; - auto build = startBuildFor(store, ns, "part"); - - ManifestEntry entry; - entry.path = String(70 * 1024, 'a'); - entry.placement = EntryPlacement::Blob; - entry.ref = BlobRef{BlobHashAlgo::CityHash128, BlobDigest::fromU128(UInt128{0x42})}; - entry.blob_size = 1; - - expectThrowsCode(DB::ErrorCodes::LIMIT_EXCEEDED, [&] - { - build->stageManifest({entry}); - }); - - OperationForTest op(*backend); - EXPECT_TRUE((*op).list(store->layout().manifestNamespacePrefix(ns), "", 10, Retry::once()).keys.empty()); -} - /// spec §9.9 (mixed-algo pools, Phase 3 T2) — the W-DEP-SET cross-satisfaction crux: a manifest with /// two entries carrying the SAME digest VALUE under TWO DIFFERENT algos (`ch128:X` / `xxh3:X`). Only /// `ch128:X`'s body is ever putBlob'd; `xxh3:X`'s body never lands anywhere. Promote MUST fail closed — From 657c521c48b320a976f3991f8b567b9e41a0d6c5 Mon Sep 17 00:00:00 2001 From: Konstantin Morozov Date: Fri, 16 Oct 2026 14:38:31 +0200 Subject: [PATCH 19/19] Validate CAS manifest entry paths before encoding Review: https://gist.github.com/k-morozov/7b34b402f18b051ced88f457c95d5df4 --- .../Formats/CasPartManifestFormat.cpp | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Formats/CasPartManifestFormat.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Formats/CasPartManifestFormat.cpp index 7c5109c0e4b7d..cbc5854c4e5f7 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Formats/CasPartManifestFormat.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Formats/CasPartManifestFormat.cpp @@ -75,6 +75,20 @@ String bannerFor(std::string_view path, uint64_t n) return std::move(w).take(); } +void validateManifestEntryPath(std::string_view path) +{ + if (path.empty() || path.front() == '/') + throw Exception(ErrorCodes::CORRUPTED_DATA, "CAS part manifest: invalid entry path '{}'", path); + for (std::string_view rest = path; !rest.empty();) + { + const size_t slash = rest.find('/'); + const std::string_view segment = rest.substr(0, slash); + if (segment.empty() || segment == "." || segment == "..") + throw Exception(ErrorCodes::CORRUPTED_DATA, "CAS part manifest: invalid entry path '{}'", path); + rest = slash == std::string_view::npos ? std::string_view{} : rest.substr(slash + 1); + } +} + } std::string_view entryPlacementToWireWord(EntryPlacement placement) @@ -94,7 +108,10 @@ String encodePartManifest(const PartManifest & m) std::vector sorted; sorted.reserve(m.entries.size()); for (const auto & e : m.entries) + { + validateManifestEntryPath(e.path); sorted.push_back(&e); + } std::sort(sorted.begin(), sorted.end(), [](const ManifestEntry * a, const ManifestEntry * b) { return a->path < b->path; }); for (size_t i = 1; i < sorted.size(); ++i) @@ -208,20 +225,7 @@ PartManifest decodePartManifest(std::string_view data) throw Exception(ErrorCodes::CORRUPTED_DATA, "PartManifest: record must start with \"path\""); ManifestEntry e; e.path = r.readString(); - - /// Manifest bytes arrive over the interserver relink channel: enforce the same path hygiene - /// as CasLayout::checkNamespace so no future consumer can inherit a traversal. Relative, - /// no empty/'.'/'..' segments. (Syntactic only — legal projection subdirs pass.) - if (e.path.empty() || e.path.front() == '/') - throw Exception(ErrorCodes::CORRUPTED_DATA, "CAS part manifest: invalid entry path '{}'", e.path); - for (std::string_view rest = e.path; !rest.empty();) - { - const size_t slash = rest.find('/'); - const std::string_view seg = rest.substr(0, slash); - if (seg.empty() || seg == "." || seg == "..") - throw Exception(ErrorCodes::CORRUPTED_DATA, "CAS part manifest: invalid entry path '{}'", e.path); - rest = (slash == std::string_view::npos) ? std::string_view{} : rest.substr(slash + 1); - } + validateManifestEntryPath(e.path); std::optional pm; BlobRefFields blob_ref;