[WIP] CAS draft (adopting to CI/CD, not for review / merge)#2073
Draft
filimonov wants to merge 2297 commits into
Draft
[WIP] CAS draft (adopting to CI/CD, not for review / merge)#2073filimonov wants to merge 2297 commits into
filimonov wants to merge 2297 commits into
Conversation
`test_cas_replicated_relink` mounted its shared pool over `with_minio`. `CasProbe`'s
fail-closed startup capability check (B31) requires the object-storage backend to enforce
conditional-PUT/DELETE semantics; MinIO does not enforce conditional deletes, so a CA pool
over MinIO refuses to mount at all — the test hung at cluster startup ("Timed out while
waiting for instance `node1'") because both replicas' `CasProbe` rejected the pool. This is
`CasProbe` correctly doing its job, not a bug: MinIO is not (and has never been) a supported
CA pool backend, matching the other CA integration tests, which already use `with_rustfs`.
Migrated to `with_rustfs` (`rustfs1:11121`, bucket `test`, matching the convention in
`test_content_addressed_drop_pool_member`/`test_content_addressed_ref_snaplog`) and updated
`count_blobs`'s S3-listing helper (`cluster.minio_client`/`minio_bucket` →
`cluster.rustfs_client`/`rustfs_bucket`) accordingly. No product change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
`CachedPartFolderAccess::repointRef` unconditionally logged `Repointed committed ref...` at `LOG_WARNING` on every repoint. Post-all-tree, a standalone write/remove on a committed part is the DESIGNED mechanism (Task 4's committed-file write, Task 8's removal-mark resolution, Task 9's `uuid.txt`/`metadata_version.txt`/`txn_version.txt` fill-ins all resolve through `repointRef`), not an anomaly — so the `WARNING` fires on every ordinary `DROP`/`REPLACE`/`ATTACH`/freeze, training operators to ignore warnings and failing `clickhouse-test`'s stderr-cleanliness check. This was the shared root cause behind 4 of 6 T12 pre-flight stateless failures: `04280_content_addressed_clone_partition_works`, `05003_content_addressed_freeze`, `02271_replace_partition_many_tables`, `01901_test_attach_partition_from`. Split by whether the repoint is routine or genuinely anomalous, using the `resolved` check already in the function rather than inventing a new one: `resolved` (the key already had a committed ref — the routine case above) now logs at `LOG_DEBUG`. `!resolved` (repoint published against a key with no prior committed ref to repoint) is currently unreachable — every caller only invokes `repointRef` on an already-resolving key — but stays at `LOG_WARNING` with a corrected message, since it would be a genuine anomaly if it ever happened (see BACKLOG "repointRef non-resolving-key audit gap", which already flagged this branch as suspect). The `ProfileEvents::CasRefRepoint` counter stays unconditional either way — it is the operator-facing signal now. Verified: all 4 previously-failing tests now pass clean (Failed: 0, Passed: 4, Skipped: 0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
Two tasks: the Formats/CasBlobEnvelopeFormat codec (exact-budget ref truncation, derived-header_len pad verification, critical-key fail-close) and the CasBuild/CasInspect rewire with CasEnvelope deletion (CityHash64 leaves the envelope). Gate resolution recorded: the ref value uses a LOCAL frozen escaper, not writeStringValue — the 256-byte budget arithmetic must be codec-owned, independent even of the phase-2 pinned settings; guarded by an alphabet-pinning test. Battery golden constructed from the hand-pinned json literal rather than self-computed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
Scoping-spec plan (8 tasks, INDEPENDENT vs PHASE-2-DEPENDENT tagged; the code draft is deliberately deferred until phase 2 is integrated + soaked, so task bodies fix scope/interfaces/consumer file:line lists/RED tests rather than transcribed C++). Grounded in the run-subsystem survey: whole-file seal-checksum verification is NET-NEW (RunRef.checksum is computed at 2 sites, verified nowhere today); the seek/inDegreeInGeneration/ seekPrefix chain is fully dead; the part-manifest-cleanup run has no reader; RunKinds 2 and 5 are dead; the embedded ManifestEntries stream is phase-6 surface (explicit boundary). Hard gates: byte-determinism (writer + merge), per-consumer checksum-abort RED tests, per-deletion consumer sweeps, the 2x-byte-cost soak measurement with the re-binarize-cas_run-alone fallback. Fixed inline at gate: header example stamps v=3 (G_BUILD), not v=1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
Three examples (cas_gc_hb, cas_run, cas_part_manifest) showed v:1 while writeHeaderLine always stamps currentCompatibilityVersion() = G_BUILD = 3; the cas_blob example already said 3. The v:1 examples had already leaked into the phase-5 plan draft (caught at its gate) — fixing the source so the phase-6/8 plans do not copy the same mistake. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
All-tree Task 12 §Step 4: wire `CasRefRepoint == 0` into the ca-soak green-path assertion set so an unexpected standalone repoint trips the harness. `repointRef` (`CachedPartFolderAccess.cpp`, downgraded to `LOG_DEBUG` in 6fb325d) fires only for the designed trigger set — a standalone write/remove on an already-committed part (all-tree Tasks 4/8/9: freeze, `ATTACH`/`DETACH`, `MOVE`/`REPLACE PARTITION`-shaped ops). A pure `INSERT`/merge/`GC` workload with none of those ops should never take that path, so `CasRefRepoint` staying at 0 across such a run is a genuine correctness property, not an incidental observation. Added the check to S03/S04/S05 (`utils/ca-soak/scenarios/cards/s03_s05_scale.py`) — confirmed via `grep` that this file contains no `FREEZE`/`ATTACH PART`/`DETACH PART`/`MOVE PARTITION`/ `REPLACE PARTITION`/`CLONE PARTITION`, i.e. exactly the "non-transactional profile" this check is sound for. Pattern-matched the existing `CasBlobList == 0 for journal-driven GC` check (same file): reuses each card's already-collected `_common.counters_window(ctx)` delta, one `Verdict.check` per card. S15/S18 (`ATTACH`/`DETACH` lifecycle) legitimately repoint and are deliberately excluded — this check is scoped to the cards that should never see one, not applied blanket to every card. Verified functionally, not just by inspection: ran `python3 -m scenarios.run --scenario S03 --seed 1 --scale dev` end to end. New verdict `CasRefRepoint == 0 on the non-transactional profile` recorded `pass` (observed 0) alongside the existing 16 verdicts; smoke cluster torn down after (`docker compose down -v`) so nothing is left running ahead of the T12 20-minute soak gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
All-tree Task 12 §Step 5. Dated worklog entry (docs/superpowers/worklogs/2026-07-15-all-tree-part-files-t12-validation-gate.md) summarizing Tasks 7-11 + codecs-v3 Phase 1 + Task 9's mutable-file deletion sweep + the T12 pre-flight closure (rustfs migration, repointRef log downgrade), and the Task 12 full validation-gate evidence: CA unit sweep (922/922), server build + relink integration + txn coverage, the 7-test CA-default stateless family run, and the soak assertion wiring — all 4 gate steps PASS. Also records the Task 12 coverage-gap decision as a BACKLOG TEST-debt line (docs/superpowers/cas/BACKLOG.md §10): REPLACE PARTITION/ATTACH PARTITION ... FROM queue-clone relink on a Replicated CA table (RPL-5 slice) has no dedicated test and is not cheap enough to add inside a validation gate — resolving which relink-eligibility branch a queue-cloned REPLACE_RANGE fetch takes needs its own small investigation first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
From the 2026-07-15 20-min phase-3 soak (live introspection while running): [PART-REMOVAL-REPOINT] 17 707 repoints, all on delete_tmp_* refs — the part-removal flow pays a full repoint (~3 PUTs, ~22% of writer PUT class) on a ref removed the next step; cross-txn flow misses the T8 supersede-clear. [GC-EMPTY-SHARD-PROBES] constant ~1 174 404-probes/round independent of work (structural per-shard probe floor; Lever B removes). [REF-QUEUE-WAIT-MEASURE] ~48 ms/insert ref-lane queue wait (refsnaplog Phase-2 data point). [RUSTFS-ERROR-XML] SDK cannot parse RustFS error names; PreconditionFailed matched by message text (brittle). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
python3 -m soak.run --seed 20260715 --phase 3 --duration 1200 on a fresh cluster (T12 rebuilt binary): SOAK_EXIT=0, PHASE3 OK, fsck unreachable=0/dangling=0 at all checkpoints (2 recovery + final converge), 0 ABORTED-retried inserts, 214 transport-retried op attempts (node_down class, availability not correctness). Four measured findings from live introspection during the run landed separately on BACKLOG as db28579. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…e-run, codecs drafts parked) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
Self-contained guide for a fresh session with no memory: sources of truth, standing rules, the #49 stateless-gate re-run, the draft-worktree integration pipeline for the codecs-v3 drafts (order + gates), team topology with known coordination hazards, watchdog recreation prompt, and the campaign queue. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…re/Build decomposition) Approved brainstorm outcome: post-codecs-v3 baseline, layered directory tree with a one-direction include rule, 8 file merges, CasStore/CasBuild decomposed along lock-ownership boundaries (CasRefLedger, CasMountRuntime, CasManifestReader, CasPlainObjects, CasBlobUploader), Store->Pool and Build->PartWriteTxn renames. Zero behavior change; phased with per-phase gtest gates and a final stateless+soak gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on (fix column-TTL merge abort) `moveDirectory` on a content-addressed disk dispatches EAGERLY (B151) while `removeFile` stayed deferred to commit replay. A column-TTL merge removes the expired column's files from the tmp part dir after finalize (`MergedBlockOutputStream::Finalizer`), so the deferred unlinks (captured with the `tmp_merge_*` path) replayed AFTER the eager tmp->final `moveDirectory` had already re-keyed and published the staging bucket: `stagingFor` resurrected the tmp bucket, `staged_here` was false, and the unlink staged a removal mark into a bucket with no `Build` and no committed ref - `publishStaging`'s fail-loud LOGICAL_ERROR exception at commit (`01603_remove_column_ttl`, deterministic; found by the full CA-default stateless gate run). The published manifest also still contained the removed files (`checksums.txt` no longer listed them). Fix: part-file unlinks in all five remove variants now dispatch eagerly on a CA disk, mirroring the `writeFile`/`createHardLink`/`moveDirectory` eager-dispatch precedent - a part-file unlink only manipulates the transaction's in-memory staging, so program order is the correct order. The unlink now finds the staged entry (`staged_here` true), drops it, and the manifest publishes without the expired column. Non-part-file paths (verbatim table files, loose mountpoint files) stay deferred: their `unlinkFile` branches delete durable objects immediately, which must not happen before the commit decision. Validation: 01603_remove_column_ttl RED (server abort) -> GREEN on the CA lane; CA unit sweep Cas*:Ca* 922/922. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…rser's (review fix) Review finding on `de8a38b1e87` (Medium/latent): the eager-dispatch gate classified paths with `parsePartFilePath(path).file` non-empty, but `unlinkFile`'s in-memory-vs-durable split branches on the ROUTER's `route().file`. For a single-component detached path (table dir / "detached" / x) the parser reports a non-empty file while `route()` re-splits it to ref="detached/x", file="" - `unlinkFile` takes a DURABLE branch there (`removeNamespaceFile` / `removeMountpointObject`), so the parser-based gate eager-dispatched a durable delete that an aborted transaction would not roll back. The gate is now `ContentAddressedTransaction::stagesPartFileUnlink` - the same predicate `unlinkFile` itself branches on - called via a dynamic_cast helper (`isEagerContentAddressedUnlink`), replacing the parser-level `isContentAddressedPartFile`. Also adds the missing one-line dispatch comment on `removeFile` (review nit) and a unit test pinning the gate to the router's classification, including the divergence case. Validation: CaWiringOps.StagedUnlinkGateMirrorsUnlinkRouting green; CA unit sweep Cas*:Ca* 923/923. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…ption B target shape) Filed from the 01603 column-TTL abort root cause (`de8a38b1e87`, `725dbc7d83c`): the eager-vs-deferred dispatch split in `DiskObjectStorageTransaction` is a structural inversion hazard. Target shape per user direction: durable effects become staged intents, CA gets a `ContentAddressedDiskTransaction` subclass dispatching every op in program order, and the per-method `isContentAddressed()` branches are deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…tion contract User direction: standard `IDiskTransaction::precommit` (noop for ordinary disks); CA precommit = whole publish before ZK-multi / before the `data_parts` lock; commit = durable intents only; `moveDirectory` stops publishing, B151 rename-window publish + `rename_published_refs` die. Grounded: `PreActive` gating means no owner reads pre-commit, and the fetch handler's `PreActive` window is an accepted upstream race (plain-s3 fails+retries the same fetch), not a protocol obligation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
`commit` implicitly runs a not-yet-run `precommit` (idempotent, flag-guarded) with a ProfileEvent + debug log for observability; staging mutations after `precommit` are fail-loud `LOGICAL_ERROR`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…orkarounds The refactoring must also remove the accumulated upstream-code workarounds around eager-dispatch/read-your-writes; the classified inventory lives in `docs/superpowers/cas/upstream-patch-inventory.md`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…INE de-patching) Hunk-by-hunk inventory of the branch's patches to upstream code over the user's curated diff scope (48 files, +1663/-47): 12 class-A hunks die with the one-pipeline refactoring (11 of them = the four per-method `isContentAddressed` branches in `DiskObjectStorageTransaction`), 67 class-B integration-surface hunks stay (several flagged may-shrink), 3 class-C hunks are independent cleanup candidates. Ends with the recommended de-patching order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…overlay DB Gate-49 cluster A (19 deterministic `TABLE_ALREADY_EXISTS` fails): repo-root `test`/`test1`/`test2` debris shadows `default.test` through `clickhouse-local`'s filesystem-overlay default database. Find the producing test; consider a pre-run debris sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
… finding) Make CAS own the JSON escaping alphabet for its text codecs instead of inheriting the ClickHouse-wide `FormatSettings::JSON` default. `jsonWriteSettings` now pins `escape_forward_slashes = false` so a `/` in a CAS string value stays `/` — CAS values are dense with `/` (ref-paths, fold-seal map keys), and `cas_fold_seal` byte-determinism plus every golden text must be independent of a global default. Adds `writeBoolValue` (symmetric with the existing `readBool`) and a drift-guard test that goes red if the global default ever leaks back in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…/enum words) Frozen sub-object JSON rendering consumed by cas_gc_outcomes (this phase) and refsnaplog/runs/blob-envelope (phases 3/5/7), per the codecs-v3 design. Enum values render as full words with fail-closed reverse maps; `Token` and `BlobRef` get canonical sibling-key writers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
Move `PoolMeta` and its codec into `Core/Formats/CasPoolMetaFormat` as a header line + one JSON body object; delete the protobuf encoding and `Core/CasPoolMeta.h`. `PoolMeta::createOrValidate` and the pool protocol logic stay in `Core/CasPoolMeta.cpp`. The phase-1 toy battery case is replaced by the real `cas_pool_meta` case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…tover Move the three server-root singletons and their codecs into `Core/Formats/CasServerRootFormats` as header line + one JSON body object; delete the protobuf encodings. Protocol logic (`claimMount`, `allocateWriterEpoch`, `MountLeaseKeeper`, ...) stays in `Core/CasServerRoot`. The `min_active` farewell sentinel (`UINT64_MAX`) survives as a decimal string; `gc_fenced` as a JSON bool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
… unversioned object dies) Move `GcState`/`GcLease`/`GcHeartbeat` and their codecs into `Core/Formats/CasGcStateFormat`; the heartbeat gains a header/version line, killing the last unversioned binary object. Relocate the in-memory-only `RetiredEntry` into `Core/CasBlobInDegree.h` and delete `Core/CasGcFormats`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
Move the outcome log and its codec into `Core/Formats/CasGcOutcomesFormat` as a
header line + one flat JSON record per entry + an `{"n":count}` trailer, reusing
the shared wire vocabulary. `Always` compression means the stored object is one
zstd frame, so `outcomesKey` now appends the `.zst` stored suffix and `CasGc`
seals/opens the object. Delete `Core/CasGcOutcomes`.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…ecord body) Move the fold-seal structs and codec into `Core/Formats/CasFoldSealFormat` as a header line + meta line + tagged record lines in fixed section order + trailer. The encoder sorts every collection by key so the `PinnedRaw`/`putDeterministicArtifact` byte-equality contract holds. Delete `Core/CasGenerationSeal` and the old combined gc-formats/generation-seal test files; rewire the remaining shared GC includers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
All eight control-plane objects now use the v3 text codecs, so nothing references `cas_format.proto` anymore. Delete `Core/Proto/`, drop the `clickhouse_cas_proto` build wiring from `src/CMakeLists.txt`, flip the `Formats/README.md` rows to done, and re-point the pluggable-hash backward-floor test at the text codec. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…resh) Address the phase-2 review's non-blocking findings: - `decodeGcState` now fails closed on an absent `gcs` key (throws `CORRUPTED_DATA`) instead of silently keeping the struct default of 1, matching the old proto behaviour and the no-fallback principle; adds a red-able unit test. - Refresh the stale protobuf-era comment in `CasPoolMeta.FailClosed` and drop the dead `CasHeaderGolden.PoolMetaFailClosedOnGarbage` reference. - Document `openObject`'s uncompressed-repair passthrough vs the `Always` compression policy asymmetry in the formats README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EfRKU1Tt1CnaiFYh3kue27
…pect every DeleteOutcome (triage #9) decommissionPoolMember released its admin claim, then re-read epoch/mount tokens and deleted the slot's control objects (mount, epoch, owner) without inspecting DeleteOutcome. A concurrently-restarting victim could rewrite mount+epoch with fresh tokens (a legitimate successor reclaim) in the gap; the old code deleted those fresh successor objects anyway and reported slot_removed=1 — an epoch-monotonicity violation. Fix: capture the epoch token+value and the farewell mount token while the claim is held; delete mount FIRST with the exact farewell token (a successor rewrite -> TokenMismatch -> abort), only then epoch with the under-claim token (successor bump -> TokenMismatch -> abort), owner LAST (safe only once mount+epoch deletions proved no successor exists). Every delete's DeleteOutcome is inspected; any non-Deleted outcome aborts the tail and reports slot_removed=0, fail closed. New deterministic tests (successor-interleaving + uncontended-tail). Implemented by codex (gpt-5.6-sol); build/battery independently re-verified by Claude (922/922, 0 failures) after the codex sandbox could not commit (read-only .git). Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…s (arm_tidy gap) gtest_cas_probe.cpp's DialectGatedCountingBackend (added by T2's fix round, after the T13 tidy audit ran) had 6 google-default-arguments sites the audit never saw. Same pattern as the T13 batch-1 sweep: no caller invokes the shortened arity on the concrete type (only through Backend&), so no using-declarations are needed. Focused CasProbe.* battery: 12/12 pass. Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…triage #22) MetadataStorageFactory accepted gc_interval_sec=0 / gc_shards=0 without validation, and CasGcStateFormat's encodeGcState only chasserted gc_shards >= 1 -- compiled out in a RelWithDebInfo build, so it silently persisted an unreadable gc/state. Both now throw: the factory rejects zero values with BAD_ARGUMENTS before the pool opens, and encodeGcState refuses to encode gc_shards < 1 with LOGICAL_ERROR as a last-resort guard. Implemented by codex (gpt-5.6-luna); build/battery independently re-verified by Claude (923/923, 0 failures) after the codex sandbox could not commit (read-only .git). Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…s S3_ERROR (logical-error triage) FailingDeleteBackend::maybeFailUnreadable (gtest_cas_decommission.cpp) used LOGICAL_ERROR merely to simulate an unavailable backend protection view -- a generic injected-fault stand-in, not a genuine invariant violation. Under debug/sanitizer builds this aborted the whole process before sweepNamespace's catch (Exception &) could run, per the governing rule that LOGICAL_ERROR must fire ONLY for invalid state, never a fault-injection stub. sweepNamespace (CasOrphanManifestSweep.cpp) and the per-key delete path catch Exception generically and never branch on the code, so the injected code is freely swappable. S3_ERROR matches the existing convention in this same test file (FailDeletesUnderPrefixBackend uses S3_ERROR for an injected transient delete failure). No production change; behavior under test (warn-and-continue) is unchanged, and the abort-before-catch problem under sanitizer/debug builds is gone. Per docs/superpowers/reports/2026-07-18-logical-error-asan-abort-triage.md. Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…anitizer-safe) Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
Each test deliberately drives production code into a genuine invariant violation and expects a LOGICAL_ERROR (source-edge ID zero reserved, out-of-order source-edge append, zero-component RefTxnId render, seal sealed_from exceeding snapshot_id, abandon-then-stage lifecycle violation). Correct production behavior in all five; under debug/ sanitizer builds LOGICAL_ERROR aborts before EXPECT_THROW/EXPECT_ANY_THROW can observe it. Converted to EXPECT_DEATH with DB::abort_on_logical_error forced true inside each child (same technique as the part_write.cpp batch) for cross-build-type determinism. Positive controls and the out-of-scope Class-D test (EncodeAllowsExactlyMaxRemovalBytes, same file as RenderRejectsZeroComponent) left untouched. Implemented by codex (gpt-5.6-luna); build/battery independently re-verified by Claude (924/924, 0 failures) after the codex sandbox could not commit (read-only .git). Per docs/superpowers/reports/2026-07-18-logical-error-asan-abort-triage.md. Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…s for sanitizer safety Seven tests reach a genuine, correct production LOGICAL_ERROR guard but in a way that plain EXPECT_DEATH wrapping couldn't safely cover: - Three (CasGcRound.OrphanManifestCursorSweepDeletesAndPersistsCursor, CasMountStartup.StaleSelfMountReclaimedAfterWait, CasPoolRemount.ForeignOwnerIsNeverTakenOver) abort INCIDENTALLY during implicit object teardown (MountLeaseKeeper::terminate correctly refusing to release against a foreign incarnation), not as the test's own stated assertion. Restructured so each test's primary behavior keeps asserting normally in the parent process, while the invalid object's entire construction-to-destruction lifecycle is isolated inside its own EXPECT_DEATH -- the parent process never holds or destructs the invalid state. - CasHeartbeat.BackgroundLoopFencesImmediatelyOnConfirmedMismatch throws on a background renewal thread. Default fork-style EXPECT_DEATH forking a multi-threaded process is unsafe and caused a real stall; scoped to gtest's re-exec-based death-test style for just this test (save/restore the flag around it). - Three plain conversions (CasHeartbeat.ForeignTouchMakesRenewThrow, CasMountAudit.KeeperForeignConflictRefusesAndNamesHolder, CasMountLease.KeeperStartAdoptsOurOwnClaimNotDoubleStart) using the same abort_on_logical_error-forcing technique as the earlier batches. Implemented by codex (gpt-5.6-sol, judgment-heavy fixture work); build/ battery independently re-verified by Claude (924/924, 0 failures) after the codex sandbox could not commit (read-only .git). Per docs/superpowers/reports/2026-07-18-logical-error-asan-abort-triage.md. This completes the Class-B remediation (19/19 tests); only Class-D (CasRefCodec.EncodeAllowsExactlyMaxRemovalBytes) remains, pending a live isolated run to determine its cause. Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…ismatch (triage #7) The receiver advertised its CAS pool UUID to the sender at request time, but never re-verified it once the disk reservation actually completed. A reservation landing on a different disk (multi-disk policies, tiering, concurrent reservation changes) could be backed by no CAS pool at all, or a DIFFERENT pool than what was advertised -- and the code would still attempt to consume the relink payload for that reservation, risking a relink that points at content in the wrong pool. Capture the advertised pool UUID into a local at both advertise sites, and re-check it against the actually-reserved disk's CAS pool immediately before consuming the relink payload: any mismatch (no CAS exchange at all, or a different pool UUID) now falls back to a plain byte fetch, with a LOG_INFO explaining why. This also replaces the previous unconditional for a non-content-addressed target disk with the same fallback -- per the fix contract, neither failure mode should abort the fetch, both should degrade to bytes. No new unit test: this guard needs a live multi-disk fetch to exercise meaningfully; runtime coverage is deferred to the R5 scenario campaign (S38-style integration scenario). Validated by full build + the CAS gtest battery staying green. Implemented by codex (gpt-5.6-sol); build/battery independently re-verified by Claude (full clickhouse binary + 924/924 unit battery) after the codex sandbox could not commit (read-only .git). Per docs/superpowers/reports/2026-07-17-codex-review-triage.md §3.7. Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…ured (triage #16) tryBuildReaderExecutor's fallback condition checked distributed_cache, memory_cache, filesystem_caches, decryption_stages, and async_prefetch, but not file_view -- a configured byte-window stage would be silently dropped rather than triggering a fallback to the legacy read path, since the executor doesn't implement it. Added `file_view` to the condition and to the fallback log message. Full Cas* battery + IO/ReadPipeline/ReaderExecutor/FileView unit tests: 924/924 + 70/70, zero failures. Per docs/superpowers/reports/2026-07-17-codex-review-triage.md §2.16. Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
#1) Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…reation, event sink installed before threads (triage #8, #10) runGarbageCollectionRoundNow/runOneGcRoundForTest now hold gc_scheduler_mutex for the whole round (mirroring gcHealth), instead of snapshotting a raw CasGcScheduler* and releasing the lock before calling it -- the previous shape let shutdown()'s gc_scheduler.reset() race the escaped pointer. A new shutdown_called flag (TSA_GUARDED_BY the same mutex) stops the lazy-creation path from resurrecting a scheduler after shutdown has begun. Scheduler construction now reads the already-guarded cas_store member directly instead of recursively calling store() (which also takes gc_scheduler_mutex), so holding the mutex across construction can't self-deadlock. store() now returns a Cas::PoolPtr by value (a shared-ownership snapshot taken under the same mutex) rather than `const PoolPtr&` into the live member; partAccess()'s null-check-and-return is now guarded by the same mutex too. Both cas_store and part_access were previously read/reset with no synchronization at all, racing shutdown()'s unlocked resets. Separately (triage #10): CasEventSink is now threaded into PoolConfig and installed on the Pool before Pool::open/openForDecommission ever call mountWritable, which spawns the mount's renewal thread. Previously the sink was installed via setEventSink() after open() returned, so an event fired during the open call itself (or by a very fast renewal thread) was silently dropped by the still-null sink, and the assignment raced the thread's reads of event_sink_. CasPool::setEventSink() remains for tests/pre-open wiring only, documented as such. Verified: full clean rebuild of clickhouse + unit_tests_dbms, and the CAS gtest battery (Cas*:CA*:ContentAddressedLog*:CountingBackendShape*:RefSnapshotCodec*:RefTableCacheEviction*:RefWriter*) at 924/924 passing, independently of the implementer's own report. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 3.8, 3.10 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
-narrow) If a transaction stages new content for a part (create) and then removes that same part's directory before commit, removeDirectory only cleared content_removed marks -- the staged manifest entries and any in-flight PartWriteTxn build were left behind. The entries would never be published (the ref-drop wins), but the build's precommit/blob edges had nothing left to ever publish or abandon them. removeDirectory now also clears st->entries and, when a build is present, calls abandon() and resets it, so a same-transaction create-then-remove publishes nothing and leaves no live precommit. Verified: full clean rebuild of unit_tests_dbms and the CAS gtest battery at 924/924 passing, independently of the implementer's own report. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 2.12 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…f deleting it (T5, triage #9) Prior hardening rounds fenced the decommission tail's mount-lease and epoch-counter deletes to the exact tokens/values observed under the decommission claim, closing the successor race for both. The owner anchor (OwnerObject) resisted the same approach: it holds only server_uuid, which is byte-identical whether written by the decommissioned run or by a legitimate successor reclaiming the same srid, so no amount of delete-side token fencing can tell them apart. Decision (user-confirmed): stop deleting the owner object. Decommission's last step becomes a conditional rewrite in place (retired_at_ms tombstone) using the same exact-token conditional-write machinery already used elsewhere in this file. A racing successor's own write simply fails the conditional overwrite -- no bespoke protocol, ordinary CAS semantics settle it. claimOwnerOrThrow refuses to silently resume a tombstoned identity. This intentionally does not chase the last microsecond of the decommission-vs-concurrent-recreate race; it removes the destructive half (a live successor anchor being deleted out from under it). Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 3.9 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…#25) If commit() threw (some parts already published, compensating rollback ran best-effort), nothing prevented a caller from calling commit()/tryCommit() again on the same transaction object -- a second attempt would re-iterate parts and call publishStaging again in an inconsistent, never-tested state. Added a failed flag, set in the catch block before the rollback loop runs (so it's set even if the rollback loop itself throws); commit() now refuses outright with LOGICAL_ERROR if a prior attempt already failed. tryCommit delegates to commit(), so the one guard covers both entry points. Verified: rebuild of unit_tests_dbms and the CAS gtest battery at 924/924 passing, independently of the implementer's own report. The existing CaWiringWrite.PartialCommitRollsBackPublishedParts test (the closest existing coverage for the rollback path) also passes unchanged. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 2.25 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
truncateFile silently pretended to succeed without ever touching the file -- content-addressed blobs are immutable, so a caller relying on a real truncate got silent data corruption (the file kept its old content/size). Replaced the no-op with a NOT_IMPLEMENTED throw explaining why (blobs are immutable; whole-file rewrites replace the staged entry instead). Verified: rebuild of unit_tests_dbms and the CAS gtest battery at 924/924 passing, independently of the implementer's own report. Added CaTransactionOps.TruncateFileIsNotSupported asserting the thrown error code. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 2.23 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…(triage #9) Prior hardening rounds fenced the decommission tail's mount-lease and epoch-counter deletes to the exact tokens/values observed under the decommission claim, closing the successor race for both. The owner anchor (OwnerObject) resisted the same approach: it holds only server_uuid, which is byte-identical whether written by the decommissioned run or a legitimate successor reclaiming the same srid, so no amount of delete-side token fencing can tell them apart. OwnerObject gains an optional retired_at_ms tombstone (omitted from the encoded bytes when unset, so a never-retired owner's on-disk format is byte-identical to before). Decommission's final step is now a conditional rewrite (putOverwrite against the exact token it just read) that sets the tombstone instead of deleting the object; a racing successor's own write simply fails the conditional overwrite and the tail aborts closed with a "successor reclaimed" warning, exactly like the existing mount/epoch fencing. claimOwnerOrThrow refuses to silently resume a tombstoned identity even when the uuid matches, with a message distinct from the existing foreign-owner case and manual-recovery guidance (clear the tombstone and restart) mirroring the anchor-lost case already documented there. report.slot_removed keeps its name and truthiness semantics (mount+epoch deleted, owner tombstoned) since ~14 existing assertions depend on it; only its doc comment changed. This deliberately does not chase the last microsecond of the decommission-vs-concurrent-recreate race -- per-user direction, that scenario is not the priority. It removes the destructive half: a live successor anchor being deleted out from under it. Design note: docs/superpowers/specs/2026-07-18-t5-owner-tombstone-design.md (804cbbf). Verified: full clean rebuild of clickhouse + unit_tests_dbms (after two rebuild attempts were externally interrupted mid-build with no relation to this change; the third attempt completed cleanly) and the CAS gtest battery at 928/928 passing, independently of the implementer's own report. Reviewed the format, decommission, and claim-side diffs personally, including the dedicated SuccessorOwnerRewriteBeforeTombstoneBackend fixture that exercises the exact race deterministically. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 3.9 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…tity field (triage #20a)
decodeMountLease and decodeGcHeartbeat had no check that their identity
fields ("su"/"we" and "by"/"seq" respectively) were actually present in the
decoded body -- an absent field silently left the corresponding struct
member at its default-constructed value instead of failing on
corrupted/truncated persisted data. Mirrors the saw_* + throw shape
decodeOwner already used in the same file family.
Verified: rebuild of unit_tests_dbms and the CAS gtest battery at 930/930
passing, independently of the implementer's own report.
Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 3.20
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…an requested (triage #20c) decodeFoldSeal never cross-checked the decoded body's own generation field against the (generation, attempt) pair encoded in the object's key -- a corrupted or mis-keyed store object would silently be accepted as if it were the requested generation. Added an optional expected_generation parameter (default nullopt, so the existing debug-tool and test call sites need no change) that, when passed, throws CORRUPTED_DATA on a mismatch. The three production call sites that already know the requested generation (Gc::readFoldSeal, CasOrphanManifestSweep::sealedRefCursor, CasFsck's invariant loop) now pass it; CasInspect.cpp's arbitrary-key debug render is intentionally left unchanged. Verified: rebuild of unit_tests_dbms and the CAS gtest battery at 931/931 passing, independently of the implementer's own report. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 3.20 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…d of narrowing (triage #21) The three header "v" fields were read as a u64 and narrowed with a plain static_cast<uint32_t>, so a persisted value like 4294967299 (2^32 + 3) silently became 3 and passed as if it were a normal, low format version. Added JsonObjectReader::readU32Number, which reads through readU64Number and throws CORRUPTED_DATA if the value exceeds uint32 range. Replaced the three header-version narrowing sites (CasTextFormat.cpp, the cas_run header in CasRecordStreamFormat.cpp, the blob envelope header in CasBlobEnvelopeFormat.cpp) with calls to it. Left every other readU64Number call in these files unchanged -- they're genuinely u64 fields (sizes, counts, timestamps, the blob provenance ch_version), not narrowed header versions. Verified: rebuild of unit_tests_dbms and the CAS gtest battery at 931/931 passing, independently of the implementer's own report. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 2.21 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…ish (triage #13) promote()'s BuildPublish and RefRepoint events fire AFTER store->appendRefOps has already durably committed the promotion. A throwing sink (e.g. a system-log queue push failing under OOM) at either point turned an already-durably-succeeded promote into a caller-visible failure, even though the promotion itself is fine and idempotent re-drive already handles retries correctly. Both post-durable emissions are now wrapped in try/catch(...) that logs and swallows the failure via tryLogCurrentException. The BlobReuseAdopt emission inside the appendRefOps closure is unchanged -- it fires while the durable append is still being computed, before it commits, so a throw there correctly aborts the operation. Verified: rebuild of clickhouse + unit_tests_dbms and the CAS gtest battery at 933/933 passing, independently of the implementer's own report. Includes a fault-injection regression test (a sink that throws only for BuildPublish) confirming promote() no longer propagates the sink's exception and the ref still resolves to the promoted manifest. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 2.13 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…epting a nonexistent path (triage #24) Both if_exists and should_remove_objects were discarded parameters -- if_exists was never checked, so unlinking a path that doesn't exist anywhere (not staged, not committed) silently succeeded regardless of what the caller asked for. All three unlinkFile branches now check existence before mutating state: the part-file branch checks the committed manifest via the same getView idiom publishStaging already uses (view->hasFile); the verbatim table-file branch checks getNamespaceFile; the loose mountpoint branch checks getMountpointObject. Absence with if_exists=true is a no-op; absence with if_exists=false throws FILE_DOESNT_EXIST. should_remove_objects stays unused -- this fix doesn't need it. Verified: rebuild of clickhouse + unit_tests_dbms and the CAS gtest battery at 933/933 passing, independently of the implementer's own report. Includes a regression test covering all three if_exists/exists combinations for the part-file branch. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 2.24 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…triage #28) If PartWriteTxn's constructor threw, the build-seq allocateBuildSeq() had just minted was never retired -- retireBuildSeq is only called on publish/abandon/dtor of a successfully constructed and registered build. A construction failure permanently pinned the GC watermark floor at that seq forever. Added a SCOPE_EXIT that retires the seq unless a registered flag was set (only set after registerInflightBuild succeeds), mirroring the same dismissed-flag SCOPE_EXIT idiom already used elsewhere in this codebase (ContentAddressedTransaction.cpp's staged-temp-file cleanup). Verified: rebuild of clickhouse + unit_tests_dbms and the CAS gtest battery at 933/933 passing, independently of the implementer's own report. Includes a fault-injection regression test (a sink that throws on BuildStart, forcing construction to fail) confirming the failed seq no longer pins minActive and the next build gets the following monotone sequence. Triage doc: docs/superpowers/reports/2026-07-17-codex-review-triage.md sec 2.28 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
Blob body uploads are fault-tolerant: they run through the pool's shared CasRequestController, which resolves an ambiguous S3 response (SlowDown, 429, 5xx) by re-reading the key and reissuing within a budget. The blob freshness-meta sidecar (putMetaIfAbsent/casMeta in CasBlobMeta.cpp) was not on that path -- it called backend.casPut directly, with a single SDK attempt. Under S22's fault injection (fault_rate=0.2), a SlowDown on a .meta PUT had a flat ~20% chance of escaping to the INSERT/merge client as HTTP 500 (S3_ERROR). Added two new controller primitives (CasRequestControl.h/.cpp): - putOverwriteControlled: a controlled If-Match overwrite for a mutable marker, resolving an ambiguous attempt with one GET -- unchanged token means the attempt never applied (retry); matching bytes means an earlier attempt already landed (Committed); anything else is a genuine competing write (Conflict, returned as a value, never thrown). - putIfAbsentControlledMutable: the create-side sibling. The existing putIfAbsentControlled deliberately was not reused here -- its resolve (resolveByExactGet) throws CORRUPTED_DATA on any different bytes at the key, which is correct for the ref-log lane's immutable content-addressed keys but wrong for a mutable state marker, where a pre-existing different value (e.g. a stale Condemned marker still present when a vanished blob body is freshly re-uploaded) is a normal, expected outcome, not corruption. This gap surfaced as a real battery failure (CasPartWriteTxn.PutBlobResurrectVanishedReUploadsHeldBody) during implementation and was fixed by adding the dedicated primitive rather than weakening the ref-log lane's contract. Both are exposed through CasRefLedger/Pool wrappers (stagingConditionalOverwrite, stagingPutIfAbsentMutable) mirroring the existing stagingPutIfAbsent/ stagingConditionalCreate pattern. CasBlobMeta.cpp's putMetaIfAbsent/casMeta now take a Pool& and route through these; every production call site (CasPartWriteTxn.cpp's fresh/resurrect/adopt-backfill writers, CasGc.cpp's condemn-marker writer) was updated. loadMeta stays a plain GET; deleteMetaExact is deliberately left unchanged (its sole caller is a GC condemn-cleanup path, not the S22 hot path, and GC's own outer-round retry/idempotence model already tolerates a failed cleanup delete as a silent no-op). Verified: full clean rebuild of clickhouse + unit_tests_dbms and the CAS gtest battery at 938/938 passing, independently confirmed (not just the implementer's report) after finding and fixing the putIfAbsentControlled reuse gap myself. RCA: docs/superpowers/reports/2026-07-18-s22-throttle-retry-rca.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…er blocks behind a GC round Final whole-branch review of the fix wave (docs/superpowers/reports for 2026-07-17/2026-07-18) found two real gaps in T9's lifecycle-locking fix: 1. Critical: partAccess() still returned `CachedPartFolderAccess&` obtained from a raw part_access.get() taken under gc_scheduler_mutex, then released before the caller's first dereference. shutdown() could acquire the mutex and destroy the object in that window -- the exact UAF class T9 was meant to close, just moved from store() (already fixed as a by-value shared_ptr snapshot) to partAccess() (missed). Fixed identically: part_access is now a shared_ptr, and partAccess() returns a by-value snapshot taken under a lock. Every call site (36 across ContentAddressedMetadataStorage.cpp and ContentAddressedTransaction.cpp) already used the temporary in one full expression (`partAccess().method(...)`), so this is a mechanical `.` -> `->` rewrite with no caller logic changes. 2. Important: gcHealth() -- an unprivileged SELECT on system.content_addressed_mounts -- held gc_scheduler_mutex for its whole call, which a manual GC round (runGarbageCollectionRoundNow/ runOneGcRoundForTest) ALSO holds for its entire, unbounded-duration scan-and-delete workload. A health query could therefore stall behind an in-progress round. The scheduler's own gcHealth() is lock-free (atomic reads plus wedgedRefLaneCount), so serializing it behind the round bought no safety. Split the single lifecycle mutex into two: gc_scheduler_mutex now serializes ONLY one round at a time and makes shutdown wait for an in-flight round to finish (unchanged priority: clean completion over fast shutdown); a new pointer_mutex guards brief reads/writes of cas_store/part_access/gc_scheduler themselves (snapshot, create-if-absent, reset) and is never held across a round or a caller's use of a returned snapshot. gc_scheduler is now a shared_ptr for the same reason as part_access: a round takes its snapshot under pointer_mutex, releases it, and runs via the snapshot -- safe even if shutdown concurrently resets the member, since the snapshot's own refcount keeps the object alive. startup() is marked TSA_NO_THREAD_SAFETY_ANALYSIS: it runs exactly once, single-threaded, strictly before this object is exposed to any other thread, so the pointer_mutex/gc_scheduler_mutex discipline (which exists for concurrent access after startup) does not apply and would only add meaningless lock/unlock pairs. Verified: full clean rebuild of clickhouse + unit_tests_dbms and the CAS gtest battery at 938/938 passing. Final review: .superpowers/sdd/final-whole-branch-review.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…tion instead of discarding it
Final whole-branch review of the fix wave found that T12's S22 controller
plumbing (putMetaIfAbsent/casMeta now return Committed/Conflict/Unresolved)
was wired to the controller correctly, but its two real callers in
uploadFromSource discarded the contract:
- writeFreshMetaClean ignored putMetaIfAbsent's result entirely. Its
comment claimed a Conflict there only ever means "a racing writer
already created the same Clean state" -- but a stale Condemned marker
can genuinely be present at this exact call site (a body that vanished
and was freshly re-uploaded), proven reachable by
CasPartWriteTxn.PutBlobResurrectVanishedReUploadsHeldBody. Left
unhandled, that marker stayed Condemned forever, misleading every
future point-reader.
- writeResurrectMetaClean's 8-attempt outer loop already reloaded and
retried on any non-Committed result, but silently gave up after
exhausting all 8 attempts ("best-effort... leave the meta as-is") --
contradicting the RCA's explicit requirement that budget exhaustion
reach the caller as a controlled retry signal, not a silent skip.
writeFreshMetaClean now delegates to writeResurrectMetaClean with no
prior point-read, so a fresh upload gets the SAME reload-and-reconcile
handling a resurrect already had. writeResurrectMetaClean's loop now
throws throwCasWriteRetryLater (NETWORK_ERROR, the same signal used
elsewhere on this write path) after exhausting all attempts instead of
silently succeeding with a stale marker.
Verified: full clean rebuild of clickhouse + unit_tests_dbms and the CAS
gtest battery at 939/939 passing. Extended
PutBlobResurrectVanishedReUploadsHeldBody to assert the meta ends up
Clean (previously unchecked); added
PutBlobFreshMetaExhaustionThrowsRetryLater covering the exhaustion path
with a persistently-faulting backend, confirming the body PUT is
unaffected and only the meta write's failure surfaces.
Final review: .superpowers/sdd/final-whole-branch-review.md
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
… instead of failing outright Final whole-branch review found that the owner-tombstone step (T5, triage #9) used a bare pool_backend->putOverwrite(...) wrapped in a plain catch(...): any exception at all -- including one where the write actually landed and only the response was lost -- was reported as a hard failure (report.slot_removed = false), with no way to tell "genuinely didn't apply" from "applied, ack lost". A retry in the latter case has no way to resolve the ambiguity either. The owner-tombstone attempt now goes through a standalone CasRequestController (decommission is an administrative, non-hot-path operation with no mount-lease fence of its own -- the exact-token CAS itself is the safety mechanism, same as the mount/epoch deletes right above it) calling putOverwriteControlled, the same primitive T12 added for the S22 freshness-meta fix. An ambiguous attempt is now resolved with one GET: unchanged token means the write never applied (was never the concern here since decommission doesn't retry within one invocation, but correctly reported as Unresolved rather than a hard failure); matching bytes means this exact tombstone already landed (Committed, not a failure); a genuinely different owner (a real successor reclaim) is Conflict, preserving the existing "successor reclaimed" warning unchanged. This closes the practical resumability gap for the common case (a transient transport blip after the write landed); it does not attempt to solve the deeper "a retry's own claim path meets an already-tombstoned owner mid-decommission" edge case, which would require changes to the claim/open path itself, not just this write -- left as a known, narrow residual risk consistent with the "no big complications" scope already set for T5's related decisions. Verified: full clean rebuild of clickhouse + unit_tests_dbms and the CAS gtest battery at 940/940 passing. Added CasDecommission.OwnerTombstoneAmbiguousSuccessResolvesToCommitted, a backend that throws immediately after the write actually lands, confirming the fix resolves it to Committed instead of the old unconditional-failure behavior. Final review: .superpowers/sdd/final-whole-branch-review.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…mmission (minor, final review) The final whole-branch review noted the production comment above the liveness recheck read as though it closed the same-UUID successor race completely -- it doesn't: a successor can still recreate epoch/mount in the narrow gap strictly after this recheck but before the owner CAS reads its own token, and its (not-yet-retired) owner anchor then gets tombstoned by this decommission run. Document this explicitly as an accepted, narrow window rather than leaving it implicit -- no code change, no behavior change. Final review: .superpowers/sdd/final-whole-branch-review.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…hrow (minor, final review) The fail-closed throw added for triage #1 (conditional copy must never fall back to an unconditional write) dropped the S3 exception name, unlike its sibling throw in the same file which preserves it. A 412 on a conditional CopyObject maps to S3Errors::UNKNOWN, so the exception name is the only reliable discriminator classifyConditionalWriteResult has -- without it, a permanent rejection like EntityTooLarge would be classified Unresolved instead of DefiniteFailure, causing pointless budget-long retries for something no retry can fix. Final review: .superpowers/sdd/final-whole-branch-review.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
… (final review)
emu_token_state's only prior cleanup path erased an entry inside
deleteExact, gated on that SAME key's own etag already being comfortably
stale at delete time. A key created and deleted within the 2-second
stale-age window -- a normal pattern for short-lived staging/tmp objects
-- never passed that check, and since the key is never revisited
afterward, nothing ever pruned it later: the map grew without bound for
the backend's lifetime under a workload with many distinct short-lived
unique keys.
deleteExact now queues a recently-deleted (not-yet-stale) entry's exact
{etag, disambiguator} snapshot onto a small FIFO instead of leaving it
untouched. Every emuMintToken call runs a bounded sweep (at most 16
records) from the front of that FIFO: a record whose exact snapshot no
longer matches the current map entry (the key was re-minted since) is
simply discarded without touching the map; a record that still matches
and has aged past the existing 2-second safety window is finally erased.
This is deliberately NOT "queue on every mint" (which would risk pruning
a still-live key's same-mtime-quantum disambiguator state prematurely,
changing the observed token for a live incarnation) -- only successful
deletions are queued, keeping expiry purely additive to the existing
token-minting semantics.
Verified: full clean rebuild of clickhouse + unit_tests_dbms and the CAS
gtest battery at 941/941 passing, independently confirmed. Added
CasObjectStorageBackend.EmuTokenStateEventuallyPrunesDistinctShortLivedKeys
with an injectable clock (setEmuNowNsForTest): 128 distinct short-lived
keys created+deleted, then one trigger mint past the stale window --
retained state stays within a small bound instead of tracking all 128.
Final review: .superpowers/sdd/final-whole-branch-review.md
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LN7m5KvCqwnfutnPDNUncF
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
content addressable storage - draft PR
Documentation entry for user-facing changes
TBD.