diff --git a/docs/en/antalya/cas/architecture/manifests-and-refs.md b/docs/en/antalya/cas/architecture/manifests-and-refs.md index 6e6b1ed667bfa..a73a253f910c2 100644 --- a/docs/en/antalya/cas/architecture/manifests-and-refs.md +++ b/docs/en/antalya/cas/architecture/manifests-and-refs.md @@ -55,6 +55,20 @@ 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 record after its own successful `stageManifest`: the confirmed object +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 diff --git a/docs/en/antalya/cas/architecture/part-lifecycle.md b/docs/en/antalya/cas/architecture/part-lifecycle.md index 267e3f6f8a19e..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->>S3: GET and validate the precommit manifest body + 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 @@ -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/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/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; 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 28a471b673ff6..34e24375556a1 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasOrphanManifestSweep.cpp @@ -45,8 +45,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(); @@ -529,32 +529,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, @@ -562,7 +562,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 @@ -722,7 +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; - for (const ListedKey & listed : page.keys) { ++result.listed; @@ -758,23 +757,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 = mountLeaseForNamespace(op, layout, parsed->ns, &result.floor_reads); + } + if (!isBuildPrefixCoveredByFloor(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 +879,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 @@ -919,6 +912,21 @@ ManifestSweepResult planManifestCursorPage( decided_through = listed.key; } + 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) + { + 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; + }); + } + size_t next_body_hint = 0; for (const ListedManifestObject & candidate : candidates) { 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/Pool/CasPartWriteTxn.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Pool/CasPartWriteTxn.cpp index 516debe79ef7b..29ad177f8a5c7 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,6 +648,8 @@ 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 = blobDependenciesOf(body.entries); + const ManifestId id{owning_ns, ref}; const String key = store->layout().manifestKey(id); @@ -694,8 +706,9 @@ ManifestId PartWriteTxn::stageManifest(std::vector entries) e.reason = "stageManifest: part-manifest body written"; }); - staged_manifests.push_back(id); - staged_manifest_ids.insert(id); /// A3 mint-tightening: `precommitAdd`'s only legal fresh-ownership source + staged_manifests.emplace( + id, + StagedManifest{manifest_incarnation, std::move(blob_dependencies)}); return id; } @@ -712,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_ids` -- + /// 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_ids.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 @@ -821,19 +834,26 @@ 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_manifests.find(id); staged != staged_manifests.end()) + { + 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) + 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 = blobDependenciesOf(body.entries); + } /// 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 +879,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_dependencies = std::move(manifest_blob_dependencies), &repoint_old, &created] (const RefTableState & state) -> std::vector { created = !state.getCommitted().contains(final_ref_name); @@ -908,16 +929,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_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 +958,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 +967,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 — @@ -1219,19 +1238,24 @@ 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, 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 String key = store->layout().manifestKey(id); - if (const auto observed = op.head(key, Retry::standard())) - op.remove(key, observed->etag, 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", + staged.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", 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 953059a76ebc3..fa25db49d6f42 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,8 +8,8 @@ #include #include #include -#include #include +#include #include namespace DB { class WriteBuffer; } @@ -203,17 +204,21 @@ 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. - /// The id is recorded for best-effort `abandon` cleanup. + /// 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 `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); /// 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 @@ -228,15 +233,17 @@ 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. 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 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 @@ -333,6 +340,12 @@ class PartWriteTxn /// dependencies are blob-only, so `ObjectKind` is not part of the key. using DepKey = BlobRef; + struct StagedManifest + { + 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; @@ -379,14 +392,11 @@ 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`, 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; + /// 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_manifests; std::map deps; /// dependencies recorded by this build (blobs only) 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_manifest_sweep.cpp b/src/Disks/tests/gtest_cas_orphan_manifest_sweep.cpp index 2c68bf8492fd0..6bfe22e5f8005 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,120 @@ 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}))); +} + +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 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 diff --git a/src/Disks/tests/gtest_cas_part_manifest_format.cpp b/src/Disks/tests/gtest_cas_part_manifest_format.cpp index dd6eab02c0030..1a3e0fe6018ba 100644 --- a/src/Disks/tests/gtest_cas_part_manifest_format.cpp +++ b/src/Disks/tests/gtest_cas_part_manifest_format.cpp @@ -316,9 +316,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,17 +333,30 @@ 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); }); } } diff --git a/src/Disks/tests/gtest_cas_part_write.cpp b/src/Disks/tests/gtest_cas_part_write.cpp index 461718040aac9..b019784991f1a 100644 --- a/src/Disks/tests/gtest_cas_part_write.cpp +++ b/src/Disks/tests/gtest_cas_part_write.cpp @@ -438,6 +438,55 @@ 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); +} + +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 @@ -1476,6 +1525,50 @@ 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, 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(); @@ -2359,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 —