[OPIK-6901] [BE] test: buffered traces cutover deletion-bridge E2E gate + runbook by andrescrz · Pull Request #7572 · comet-ml/opik

andrescrz · 2026-07-22T13:54:39Z

Details

Adds the end-to-end validation gate and operator tooling for the buffered cutover that migrates the ClickHouse traces table to its weekly-partitioned, denullified, sharding-ready successor with near-zero downtime and zero deletion loss. Purely additive — no production-code changes. The mechanism is backfill → delta → deletion-bridge replay → EXCHANGE, using the ingestion async-insert buffer to absorb the brief cutover window.

Integration gate (TracesLocalV2CutoverTest): rehearses the full sequence against a fresh ClickHouse and proves the core property — 0 deletion leaks across the swap. Covers a lightweight-delete leak with a negative control, full-key and workspace-scoped replay branches, the resurrection guard on both branches (parameterized), delta completeness (both anchor arms proven necessary), normalized fidelity plus schema-drift and materialized-column parity guards, EXCHANGE TABLES ... ON CLUSTER, the single-shard Distributed wrapper, and all three rollback stages incl. reverse-replay. The gate independently reimplements the cutover statements inline (to orchestrate seeding + step-by-step assertions) rather than executing the driver .sql — an independent check of the cutover logic, kept aligned with the reference SQL; the shipped SQL itself is exercised end-to-end by running the drivers in the OPIK-7405 prod-clone QA.
Runbook + driver scripts (data-migrations/traces-local-v2-cutover/): estimate, backfill, delta_replay, verify, exchange_and_wrap, rollback, finalize — each reads single-source reference SQL (no copy-paste drift among the driver tools), with capacity/replication/topology guards and a documented Go/No-Go checklist.
docker-compose: env passthroughs for the cutover config knobs (deletion capture, non-nullable schema flag, async-insert buffer ceiling).
Local load-simulation tooling (tests_load/): ad-hoc CLI scripts to seed multi-week history and generate concurrent write/delete traffic for local rehearsal (no CI cost).

The sharding-ready Distributed wrap is gated behind --skip-wrap; retargeting the delete/mutation DAOs to the local shard table is a tracked follow-up that must ship before the wrap is applied.

Change checklist

User facing
Documentation update

Issues

OPIK-6901

AI-WATERMARK

AI-WATERMARK: yes

Tools: Claude Code
Model(s): Claude Opus 4.8
Scope: full implementation (tooling, reference SQL, integration test, runbooks) developed iteratively with human review at each step
Human verification: code review + repeated local end-to-end and rollback rehearsals on a local Opik instance under concurrent traffic

Testing

mvn -o test -Dtest=TracesLocalV2CutoverTest from apps/opik-backend — 8 invocations green (7 methods, one parameterized over both replay branches).
Full forward cutover and all three rollback stages (pre-EXCHANGE, post-EXCHANGE, post-wrap) rehearsed on a local Opik instance with pre-existing multi-week data plus concurrent write/delete traffic; fidelity verified pre- and post-swap; post-cutover app read/write validated; the deferred-wrap path and its delete-gate confirmed live.
Load-sim scripts are CLI tools (not pytest), so they add no CI cost.
Quality checks run in CI.

Documentation

Operator runbook: apps/opik-backend/data-migrations/traces-local-v2-cutover/README.md.
Local simulation tooling guide: tests_load/tests/traces-local-v2-cutover/README.md.
No product/user documentation changes.

github-actions · 2026-07-22T13:56:10Z

⏱️ pre-commit per-hook timing

Hook	Description	Result	Duration
☕ spotless — java backend	Format Java code	✅	4.14s
Total (1 ran)			4.14s

⏭️ 40 skipped (no matching files changed)

Hook	Description	Result
🐍 trim trailing whitespace — python sdk	Strip trailing whitespace	⏭️
🐍 fix end of files — python sdk	Ensure files end in a newline	⏭️
🐍 ruff — python sdk	Lint + autofix Python (ruff)	⏭️
🐍 ruff-format — python sdk	Format Python code (ruff)	⏭️
🐍 mypy — python sdk	Static type check	⏭️
🤖 trim trailing whitespace — optimizer	Strip trailing whitespace	⏭️
🤖 fix end of files — optimizer	Ensure files end in a newline	⏭️
🤖 check yaml — optimizer	Validate YAML syntax	⏭️
🤖 check json — optimizer	Validate JSON syntax	⏭️
🤖 check toml — optimizer	Validate TOML syntax	⏭️
🤖 check for added large files — optimizer	Block large files (>1MB)	⏭️
🔐 detect private key — optimizer	Block committed private keys	⏭️
🤖 check for merge conflicts — optimizer	Block merge-conflict markers	⏭️
🤖 check for case conflicts — optimizer	Block case-only name clashes	⏭️
🤖 pyupgrade — optimizer	Modernize Python syntax	⏭️
🤖 ruff — optimizer	Lint + autofix Python (ruff)	⏭️
🤖 ruff-format — optimizer	Format Python code (ruff)	⏭️
🤖 mypy — optimizer	Static type check	⏭️
📓 nbstripout — optimizer notebooks	Strip notebook output	⏭️
📝 markdownlint — optimizer	Lint Markdown	⏭️
🔤 codespell — optimizer	Fix common misspellings	⏭️
📊 radon cc — optimizer	Cyclomatic-complexity gate	⏭️
📊 radon raw — optimizer	Raw size metrics gate	⏭️
📊 xenon — optimizer	Fail on complexity thresholds	⏭️
📊 lizard — optimizer	Cyclomatic-complexity gate	⏭️
🧹 vulture — optimizer	Find dead code	⏭️
🛡️ trim trailing whitespace — guardrails	Strip trailing whitespace	⏭️
🛡️ fix end of files — guardrails	Ensure files end in a newline	⏭️
🛡️ ruff — guardrails	Lint + autofix Python (ruff)	⏭️
🛡️ ruff-format — guardrails	Format Python code (ruff)	⏭️
🛡️ mypy — guardrails	Static type check	⏭️
⚓ helm-docs	Regenerate Helm chart README	⏭️
block non-public FE plugins	Block non-public FE plugins	⏭️
🧪 pre-commit wrapper smoke tests	Self-test the wrapper scripts	⏭️
🌐 eslint — frontend	Lint + autofix JS/TS	⏭️
🌐 typecheck — frontend	Whole-project tsc type check	⏭️
📘 eslint — typescript sdk	Lint + autofix JS/TS	⏭️
📘 typecheck — typescript sdk	Whole-project tsc type check	⏭️
⚙️ actionlint — github workflows	Lint GitHub Actions workflows	⏭️
🐳 hadolint — dockerfiles	Lint Dockerfiles	⏭️

thiagohora

Opik reviewer (mined from your team's review history)

14 findings — 0 high · 3 medium · 11 low. Suppressed by team conventions: see suppressed.md.

React 👍/👎 on each comment — your feedback helps tune what it flags.

…verage claim - exchange_and_wrap.sh: require --confirm-daos-retargeted whenever the wrap is applied (--with-wrap or --wrap-only) — the OPIK-7455 hard prerequisite, else DELETE FROM the Distributed traces returns 500. Fail fast, before ClickHouse. - --wrap-only now also asserts traces_pre_cutover_backup exists, so it can't wrap into a state where stage C rollback has nothing to restore (one-way cutover). - Tag the exchange/wrap DDL (000003) and all rollback stages (000004 A/B/C + reverse-replay) with log_comment via a leading SET (reliable for DDL), for cutover attribution in system.query_log. - README: correct the "already covered" claim — the same-run --with-wrap path shares the brief cross-node ON CLUSTER window; the buffer reduces but does not eliminate INSERT exposure, and SELECTs are unbuffered. Driver-only guards + reference-SQL tags (not exercised by the gate test's embedded SQL); to be exercised in the post-merge local rehearsal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…CHANGE The buffer (asyncInsertBusyTimeoutMaxMs) is the zero-loss mechanism for the forward cutover — it holds writes across the final-delta->EXCHANGE gap so they land on the new table; at the default, a write in that window can commit to the old table and be lost after the swap. It was only a documented/echoed reminder. Add a fail-fast --confirm-buffer-raised gate, required on every EXCHANGE path (default and --with-wrap; not --wrap-only, which does no EXCHANGE), matching the --confirm-maintenance / --confirm-daos-retargeted pattern. It's an operator assertion, not verification (the buffer is a backend per-query setting the migration's clickhouse-client session can't read), so the Go/No-Go load-test item remains the actual "took effect" check. README + prereq #6 + Go/No-Go and the EXCHANGE example updated to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ip-index, rollback restart, replay coverage - Drivers: guard value-taking flags with ${2:?...} so a bare flag prints a clear "requires a value" error instead of a set -u unbound/shift crash; tag every ch() read (and estimate probe, exchange cutover_start, verify compare/drill-down) with --log_comment for system.query_log attribution. - 000005 verify: add use_skip_indexes_if_final=1 to the compare + drill-down FINAL queries so the created_at skip index prunes the scan on the 4 TB table. - README rollback: reverting traceColumnsNonNullable requires a rolling restart (config is a startup snapshot via toInstance); until then the app mixes sentinel and null semantics against the Nullable column. - Test: add workspaceScopedReplaySparesLiveCrossProjectRow — the empty-project replay branch must spare a live cross-project reused id (guard coverage); rename weekInstant's misleading minuteOffset param to secondOffset. 9/9 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…QL, fix flakes/footguns - Gate test: replace parseDateTime64BestEffort with toDateTime64(..., N[, 'UTC']) matching the reference SQL exactly (also drops the OPIK-5694 anti-pattern that pollutes system.errors); derive the during-window delta instants from the CH server clock (via a later now64(6)) instead of the JVM host clock, so host<-> container skew can't push last_updated_at/created_at below backfillStart and flake the delta arms. - delete_traffic.py: tolerate a few consecutive empty refills (delete-mask visibility lag) before concluding the project is drained, so a blip doesn't end the run early. - requirements.txt: drop `opik` (README installs it editable; listing it here lets pip shadow the local unreleased SDK) and pin a clickhouse-connect floor. - verify.sh / estimate.sh: capture the clickhouse-client output before `read <<<` so set -e catches an infra failure instead of it being swallowed and mislabeled (verify: a false MISMATCH; estimate: a false "table is empty"). Gate test 9/9 green on CH 26.3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…afe, tiered gate, version-check, gate-scope note - Deletion replay + reverse-replay: guard the deletion-events subqueries with length(deleted_id)=36 (and project_id) so a malformed bridge row is skipped rather than aborting the whole replay on toFixedString — a >36-char id can't match a real trace id, so skipping loses nothing. Mirrored in the gate test's inline replay to keep it aligned with the reference SQL. - backfill.sh: require --confirm-tiered-headroom when the destination storage_policy is tiered (multi-volume) or differs from the source — the whole-node free-space gate can't see per-volume hot headroom (an accurate auto-check isn't feasible), so force an explicit operator acknowledgment. - 000005 + verify.sh: add a version-collapse check — count old-table keys whose last_updated_at has more ns-distinct values than us-distinct, surfacing where the ns->us truncation could pick a different ReplacingMergeTree version than source FINAL (invisible to the us fingerprint). Reported, not pass/fail. - Gate test Javadoc: document that the inline SQL reimplementation is deliberate (an independent logic check); the drivers are the single-source reference-SQL path, validated end-to-end by the prod-clone QA. Kept inline aligned to the ref. Gate test 9/9 green on CH 26.3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…t mirrors Add a one-line pointer to each reference .sql the gate reimplements inline (000001-000004 family) noting that TracesLocalV2CutoverTest mirrors it and the two must be kept in step. Closes the drift-awareness gap in the direction the test's Javadoc doesn't cover — a maintainer editing the SQL now sees the link without opening the test. 000005 is not pointed at (its fingerprint is validated independently, not reimplemented inline). Comment-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…mpty-old-table guard - seed_history.py: the bad-id log said id_at ~2201, but the successor's 32-bit DateTime wraps to ~2065 (OPIK-7456) — point operators at the ~2065 partition. - liveCount / liveCountScoped: SELECT ... FROM %s FINAL so leak/survivor assertions count ReplacingMergeTree winners (mask-applied), aligning with 000005; 9/9 green. - verify.sh: an empty OLD_TABLE now fails if NEW_TABLE has rows (unexplained successor data) instead of exiting 0 "nothing to verify"; both-empty still exits 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…e fails REVIEW-REQUIRED #687 — close the delete coverage hole: delta_replay.sh replays deletes only up to when it runs, but cutover_start is captured later in exchange_and_wrap.sh, so deletes bridged in [final delta_replay, cutover_start) were covered by neither the forward replay nor the rollback reverse-replay and leaked live across the swap. exchange_and_wrap.sh now takes --backfill-start (required for every EXCHANGE path) and, right after capturing cutover_start, runs a final deletion replay — reusing the single-source 000002 deletion-replay block (now marked so it can be extracted without re-running the delta INSERT; writes in the gap are carried by the async buffer). Runbook + a gate-test case added; residual is the inherent in-flight micro-window, documented. #717 — verify.sh no longer only warns on version-collapse: a non-zero count is now a REVIEW-REQUIRED failure (exit 3, distinct from a fidelity mismatch exit 1) unless the operator reviews and passes --allow-version-collapse; --drill-down lists the offending keys (new 000005 version-check-drill block). Gate test 10/10 green on CH 26.3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@afterall

… teardown - deleteThenResurrectSurvivesTheReplay: source the recreated row's last_updated_at from the CH server clock (now64(6) -> Instant) instead of JVM Instant.now(), so host<->container skew can't put it below backfillStart and make the delta miss the resurrection path (same fix already applied to the main test). - Add an @afterall (PER_CLASS, non-static) that stops the ClickHouse + ZooKeeper containers and closes the network, so the dedicated non-reused containers don't rely solely on the Ryuk reaper across reruns / a shared JVM. Gate test 10/10 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…_traffic best-effort Address Baz round: - exchange_and_wrap.sh / rollback.sh: require --confirm-retention-paused on every EXCHANGE path and on rollback stages B/C. Retention deletes bypass the deletion bridge, so a sweep in the cutover/rollback window leaks live across the swap (forward) or resurrects a deleted row from the backup (reverse-replay). It is an operator assertion — a backend setting the script can't read — so the README Go/No-Go keeps the real "actually paused on every backend" verification. - delete_traffic.py (#851): larger newest-page refill and more generous empty-refill tolerance so mask-visibility lag doesn't end the run early; docstring states it is a best-effort traffic generator, not a guaranteed full-drain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…st hygiene Address Baz round (items 1-5): - backfill.sh (#916, high): resume skips a window only when dst >= src. DIVERGENCE is no longer a resume criterion — a partially-copied window within tolerance would be skipped and its pre-anchor rows lost forever (delta only re-copies at/after backfill_start). The INSERT is idempotent, so re-copying a short window is safe; DIVERGENCE still governs the post-copy abort. - README (#666, high): the pre-EXCHANGE fidelity gate must be a full compare (--sample-mod 1 --weeks-stride 1, no week narrowing); a single cross-project workspace-scoped residual row can be hashed out or week-skipped by sampling. - delete_traffic.py (#890): comment referenced newest 500; now references REFILL_FETCH. - TracesLocalV2CutoverTest (#901): wrapInDistributed() uses a text block + .formatted(). - _common.py (#922): delete the cutover-anchor trace in a finally block so discovery leaves no extra live trace skewing seeded counts. Gate test 10/10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…-loss Address Baz #919 (high, rollback safety). Promoting the frozen pre-cutover backup makes traces the successor accepted after cutover_start non-live. They are not destroyed — the successor is parked as traces_local_v2 until finalize.sh, recoverable during the soak — but the live table stops serving them. This is inherent to promoting a point-in-time backup and is deliberately not auto-repaired (merging the successor's post-cutover writes back would re-import the data the rollback exists to discard). - rollback.sh: stages B/C now require --accept-post-cutover-write-loss and print the recovery pointer before the promote. Named --accept-* (not --confirm-*) because it acknowledges an unavoidable consequence rather than a precondition the operator fixes. - README: scope the "no data loss" claim to "no data-bearing table dropped"; add the post-cutover-write caveat with the recovery path; show the flag in the B/C examples. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…irm anchor cleanup Address Baz round: - TracesLocalV2CutoverTest (#808): the workspace-scoped resurrection row stamped last_updated_at from Instant.now() (JVM clock) while backfillStart uses server now64(6); host/container skew could put it below backfillStart and flake the delta. Now parses nowMicros() like the other two window inserts. Swept the file: this was the last host-clock stamp feeding a server-clock comparison. Gate test 10/10. - README (#792): the opening claimed "zero deletion loss" while the cutover-tail section documents a bounded residual micro-window; reworded to "near-zero deletion loss" with a pointer to that section's mitigation. - _common.py (#789): delete_traces returns before ClickHouse applies the mask, so the #922 cleanup could still race the seeder; poll for the anchor's disappearance (bounded, best-effort) before returning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…XCHANGE rollback Address Baz round (rollback robustness): - #801: a stage B/C promote and its reverse-replay run as two statements; if the replay is interrupted, the promote has already restored the canonical shape, so re-running the stage is refused by the topology guard — stranding post-cutover deletes unreplayed (they resurrect). This hole is symmetric across B and C. Add --reverse-replay-only: re-applies just 000004_rollback_reverse_replay.sql against the live traces (asserts non-Distributed MergeTree; requires --cutover-start and --confirm-retention-paused). Idempotent, safe to re-run, closes both stages. - #798: if the forward EXCHANGE succeeded but its post-swap RENAME did not, the parked original is still under traces_local_v2 and stage B aborts "backup not found". Stage B now detects that state and prints the one-line RENAME to finish it (same remediation exchange_and_wrap.sh gives), then proceeds. Not auto-completed: rollback does exactly one thing per run. - README: document both recovery paths in the rollback section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…residual Address Baz round: - TracesLocalV2CutoverTest (#444): the replay wall-time assertion (< 60s) gated a non-correctness, environment-sensitive property (container startup, CI contention), making it a flaky gate; correctness is already asserted (the mask is applied). Drop the hard bound and the unused REPLAY_BUDGET_SECONDS; keep the measurement as a log. - README (#447): the workspace-scoped replay branch was called a "faithful mirror" that "ends matching the source" with no caveat, but 000002 documents a known residual (the (workspace_id, id) resurrection guard lets a live copy in one project shield the same reused id's delete in another). Disclose it, note 000005 flags it (ok=0), and point at OPIK-7483 which retires the arm. Gate test 10/10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Address Baz #3657021887: "the source delete it replays already removed" -> "the source deletion that it replays already removed". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

thiagohora

Great job here!

JetoPistola · 2026-07-27T14:05:48Z

👋 @andrescrz — post-merge follow-up flag (no action needed before any real cutover, but worth a tracked fix).

Re-reviewing the merged state, one item is worth surfacing. It's a recovery-path issue, not the happy path — so nothing is at risk from this having merged (the cutover runbook hasn't been executed), but the fix should land before the runbook is ever used in anger.

1. rollback.sh --reverse-replay-only guard is too weak → can mask-delete live rows (data loss).

The recovery path guards only that traces exists and isn't Distributed:

reverse_replay_engine="$(traces_engine traces)"
[[ -n "$reverse_replay_engine" ]] || { ...no traces... }
[[ "$reverse_replay_engine" != "Distributed" ]] || { ...roll wrap back with stage C... }
run_file 000004_rollback_reverse_replay.sql

Its comment assumes "the promote (stage B/C) already ran, so traces is the restored original" — but nothing asserts that. In the post-EXCHANGE / pre-rollback state, traces is the live successor (non-Distributed MergeTree) and passes both guards. 000004_rollback_reverse_replay.sql then runs — and it deliberately carries no resurrection guard (documented as correct only because traces is assumed to be the restored original). Pointed at the live successor, it unconditionally masks every id deleted-then-recreated post-cutover → silent deletion of live rows.

assert_topology (stages A/B/C) already knows how to distinguish these states via the end_time type and the presence of traces_pre_cutover_backup. Suggested fix: before running the replay in --reverse-replay-only, assert the canonical post-promote shape — traces_pre_cutover_backup absent, or traces.end_time is Nullable* (the original schema). In the dangerous state traces_pre_cutover_backup still exists and end_time is non-Nullable, so either check rejects it.

2. Minor doc nit (bundle into the same PR): 000005_verify_migration.sql comments (lines ~5, 27, 191) describe the source last_updated_at as DateTime64(9)/nanosecond, but migration 000019 reduced that column to DateTime64(6). The version-collapse guard itself is still correct (unmerged legacy on-disk parts retain ns values, so it's a genuine backstop) — only the comment is stale and could mislead an operator investigating a version-check hit.

For context: all 9 findings from the pre-merge review were resolved (verified) — this is purely the re-review of the new recovery machinery. Happy to open the follow-up ticket if useful.

🤖 Review posted via /review-github-pr

… comments Two comment accuracy fixes from a colleague review (JetoPistola). 000005_verify_migration.sql — the version-collapse guard comment claimed it detects legacy sub-microsecond version-selection on pre-000019 parts. It cannot: ClickHouse coerces last_updated_at to its DECLARED DateTime64(6) at read time, so raw ns digits are unrecoverable through the column and uniqExact(last_updated_at) == uniqExact(toDateTime64(last_updated_at,6)) always — collapse_keys is structurally 0 on a (6)-declared schema (empirically confirmed; inert since #7572). Reword to say it is inert defense-in-depth that can only fire if the column is ever re-declared at higher precision, and note that dropping the guard + --allow-version-collapse as dead code is a reasonable follow-up (kept out of scope here). finalize.sh — the recycle comment said re-running finalize "recovers" a crash between the ordered TRUNCATE and RENAME. That's only true single-host; under partial ON CLUSTER propagation the re-run's cluster-wide classify sees a mixed state and refuses loudly (finish the RENAME by hand), never silently corrupting. Reword to reflect both. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…y.sh The version-collapse guard (version-check + version-check-drill blocks in 000005, the --allow-version-collapse flag, and the exit-3 REVIEW-REQUIRED path in verify.sh) is dead code: it reads last_updated_at through a DateTime64(6)-declared column, and ClickHouse coerces to the declared type at read time, so uniqExact(last_updated_at) always equals uniqExact(toDateTime64(last_updated_at, 6)) and collapse_keys is structurally 0 on the post-000019 schema — the exit-3 path is unreachable. It has been inert since #7572. The team is accepting the (vanishingly small) risk of sub-microsecond version-selection collision, and the product is standardizing on microsecond dates, so the guard has no future either. Remove it rather than keep dead code with a misleading comment. The microsecond fidelity fingerprint (count + normalized checksum, FINAL + mask-honored) is unchanged and remains the verify gate; verify.sh now exits 0 (PASS) / 1 (fidelity mismatch) only. Validated live: backfill traces -> traces_local_v2 then verify PASSES all windows (exit 0) with the simplified final line; the mismatch path still FAILS (exit 1); no version-collapse output remains. bash -n clean; gate test unaffected (it does not reference the guard). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ook (#7645) * [OPIK-6901] [BE] fix: harden traces cutover rollback tooling and runbook Follow-up to the merged buffered-cutover runbook (OPIK-6901), from rehearsing the whole procedure — forward and every rollback stage — end-to-end on a local Opik and re-reviewing the recovery machinery. Driver logic was sound; this fixes tooling/runbook gaps, one recovery-path data-loss footgun, and a post-rollback Liquibase drift. Rollback safety: - Park the rolled-back successor as `traces_post_rollback_backup` (symmetric with `traces_pre_cutover_backup`) instead of reusing `traces_local_v2`. `traces_local_v2` had double-duty as both the disposable pre-cutover shadow and the retained rollback backup — topologically identical states — so a rolled-back estate was ambiguous and `--stage A` (discard the shadow) could TRUNCATE the retained backup. The distinct `_backup` name makes the estate self-describing and closes that footgun. Clean taxonomy: `*_backup` = retained (finalize-only); `traces_local_v2`/`traces_local` = working/live successor. - Gate `--reverse-replay-only` on the post-promote shape (traces = restored original with `traces_post_rollback_backup` present), not just "non-Distributed". The guard-less reverse replay pointed at the live successor (post-EXCHANGE, pre-rollback) would mask deleted-then-recreated rows — silent data loss. - Correct the stage-A closing message (shadow discarded, nothing to finalize) — the shared "parked backup / finalize" message applied only to stages B/C. Finalize recycles the rollback backup into an empty traces_local_v2: - A rollback + finalize used to DROP the parked successor, leaving no `traces_local_v2` — but migration 000101 (ALTERed by 000106/000107) created it, so Liquibase records it as present. That drift blocked a retry (the runbook's "traces_local_v2 exists empty" prereq) and left the DB inconsistent with the applied changesets. - The parked `traces_post_rollback_backup` IS the 000101 object, renamed (a ReplicatedMergeTree replica path is fixed at CREATE and survives renames). So finalize, in the rollback case, now recycles it back into an empty `traces_local_v2` (TRUNCATE + RENAME) instead of DROP: discards the successor data but restores the exact 000101 shadow — schema, codecs and replica path — so the estate matches the applied Liquibase state and a retry starts from a clean shadow. TRUNCATE-then-RENAME (each atomic, ON CLUSTER; no single truncate-and-rename exists) so a crash between them only ever leaves an empty backup that re-running finalize recovers. The successful-cutover case still DROPs `traces_pre_cutover_backup` (unchanged). Retention is unchanged: the backup is kept through the soak; the recycle only runs on finalize.sh --confirm. Tooling / operability: - Decouple `clickhouse-client` from any bundled ClickHouse container: it is an operator prerequisite (native, or the official-image wrapper `scripts/clickhouse-client-docker.sh`) that connects over the network via CLICKHOUSE_* env — the same mechanism in every environment. The local-rehearsal README used a compose-container `docker exec` that neither reached the driver scripts nor represents a real run. - `exchange_and_wrap.sh` deferred-wrap hint and the runbook now show `--confirm-daos-retargeted` (required for any wrap); the load-test EXCHANGE example shows the required gates. Docs: - Add a local "Rehearsing rollback" section; fix the stale `000005` version-check comment (`last_updated_at` is DateTime64(6) since migration 000019; the guard remains a backstop for legacy pre-000019 parts). Tests: - Gate test updated (rollback reimplementation, post-rollback assertions, reset recovery), and the stage-B test now re-runs the reverse replay after the rollback to pin its idempotence — the contract `--reverse-replay-only` rests on (post-cutover delete stays masked, no live survivor dropped). Validated 10/10 on the gate test and end-to-end on local docker: the full forward procedure plus every rollback stage (A discard-shadow, B swap-back, C promote-original), each followed by finalize — the rollback case recycling to an empty `traces_local_v2` whose SHOW CREATE is byte-identical to the pristine 000101 table (incl. the replica path), a fresh backfill+delta+verify then passing against it (retry-ready); the successful-cutover DROP path still finalizing to ['traces']; and `--reverse-replay-only` allowed+idempotent post-rollback and correctly rejected otherwise. * docs(cutover): clarify per-host vs cross-host ON CLUSTER atomicity in rollback/finalize Baz flagged the stage-B rollback comment as claiming RENAME ... ON CLUSTER is globally atomic. It doesn't (the "one atomic statement" point is per-host, contrasting the two-statement EXCHANGE+RENAME window), but the wording read as an absolute cross-cluster guarantee while the runbook is careful to say "gapless per node". Align the stage-B/C rollback SQL comments and the finalize.sh recycle comment with the runbook: state the per-host atomicity as the real guarantee, and name the cross-host behavior explicitly — ON CLUSTER is eventually-consistent (DDL queue converges a lagging host, or the call fails loudly), not globally atomic; the same accepted skew as any ON CLUSTER DDL and a non-issue in the default single-node deployment. Comment-only; no behavior change. * docs(cutover): frame cross-replica ON CLUSTER skew for multi-replica production + maintenance note The prior comment-precision pass described the cross-host ON CLUSTER case as "a non-issue in the default single-node deployment", which downplays it: production is multi-replica. Correct the framing in the stage-B/C rollback SQL, finalize.sh and the runbook, and add the operator maintenance note. - Cross-replica: ON CLUSTER runs synchronously (the client blocks until every reachable replica applies it, or throws naming a laggard that then converges via the DDL queue), so there is no durable mixed topology — only a sub-second cross-replica skew as it propagates, the same accepted skew as the wrap. Not globally atomic (no ClickHouse primitive for that); nil on a single replica. - Rollback stages B/C rename the LIVE `traces`, so reads hit the skew: rollback.sh now prints a maintenance NOTE and the runbook says to run the rollback in a maintenance moment / with reads quiesced on a multi-replica cluster. - finalize is EXEMPT: its TRUNCATE+RENAME touch only the parked backup / disposable shadow, never the live `traces`, so there is no live-read skew and no maintenance window is needed — stated explicitly in finalize.sh and the runbook. Comment/doc-only; no behavior change. * fix(cutover): cluster-aware finalize detection + assert parked-backup contents in rollback tests Addresses two later Baz review comments on PR #7645. finalize.sh (severity high) — production is multi-replica, and finalize is the one irreversible step, so resolve the parked-backup detection CLUSTER-WIDE via clusterAllReplicas (mirroring exchange_and_wrap.sh's settle gate) instead of the connected node only: a name present on some-but-not-all replicas means an ON CLUSTER DDL has not finished propagating, so acting on one node's partial view could recycle/drop mid-transition — refuse loudly instead of a silent "Nothing to finalize". Also guard the recycle destination: refuse (cluster-wide) if `traces_local_v2` already exists before the TRUNCATE, since recycle renames the backup INTO that name and RENAME will not overwrite — a stray shadow means a retry cutover started before the rollback was finalized. Declined the broader schema/replica-path validation Baz suggested: `traces_post_rollback_backup` is created only by the rollback RENAME (it IS the 000101 object by construction), so validating engine/columns guards a manufactured state the code-style rule says to avoid. TracesLocalV2CutoverTest (severity low) — the stage-B/C rollback tests asserted the parked backup only EXISTS, so a rename that produced an empty or wrong table would pass while losing the successor data the parking exists to preserve. Assert the backup actually holds the successor data (survivors present, post-cutover delete stays masked) and carries the successor (non-Nullable end_time) schema. 10/10 green. * docs(cutover): clarify verify.sh --from-week/--to-week are 0-based week offsets Full forward E2E rehearsal finding: the runbook's post-EXCHANGE compare hint wrote `--to-week <last-full-week>`, which reads like a date, but verify.sh takes a 0-based integer week offset from the anchor Monday. Passing a date fails with "must be a non-negative integer". Clarify both mentions (the sealed-weeks compare tip and the feasibility bullet) so operators pass an integer offset (e.g. --to-week 3). Doc-only; found while validating the runbook against a live run. * fix(cutover): make finalize.sh empty-row guard cluster-aware Follow-up to the cluster-aware detection: the LIVE_ROWS/BACKUP_ROWS emptiness guard still counted on the connected replica only, so the "refuse if live is empty but the backup holds data" check reflected one node, not the cluster — inconsistent with the clusterAllReplicas detection in the same script (Baz review). Add a max_rows() helper that takes the max row count across replicas (clusterAllReplicas grouped by host; fail-loud on a down replica, like classify) and use it for both counts. Behavior is unchanged on a single replica (max over one host = that host's count); on multi-replica it now reflects the most-caught-up replica, so a lagging/empty connected node can no longer skew the guard. The post-cutover DROP path's Distributed `traces` already aggregates the cluster and the max-over-hosts still yields its true total. Guard still fails safe either way. Validated live: max_rows returns the correct counts against the single-node estate; bash -n clean. * docs(cutover): correct verify version-check + finalize crash-recovery comments Two comment accuracy fixes from a colleague review (JetoPistola). 000005_verify_migration.sql — the version-collapse guard comment claimed it detects legacy sub-microsecond version-selection on pre-000019 parts. It cannot: ClickHouse coerces last_updated_at to its DECLARED DateTime64(6) at read time, so raw ns digits are unrecoverable through the column and uniqExact(last_updated_at) == uniqExact(toDateTime64(last_updated_at,6)) always — collapse_keys is structurally 0 on a (6)-declared schema (empirically confirmed; inert since #7572). Reword to say it is inert defense-in-depth that can only fire if the column is ever re-declared at higher precision, and note that dropping the guard + --allow-version-collapse as dead code is a reasonable follow-up (kept out of scope here). finalize.sh — the recycle comment said re-running finalize "recovers" a crash between the ordered TRUNCATE and RENAME. That's only true single-host; under partial ON CLUSTER propagation the re-run's cluster-wide classify sees a mixed state and refuses loudly (finish the RENAME by hand), never silently corrupting. Reword to reflect both. Comment-only; no behavior change. * refactor(cutover): remove the inert version-collapse guard from verify.sh The version-collapse guard (version-check + version-check-drill blocks in 000005, the --allow-version-collapse flag, and the exit-3 REVIEW-REQUIRED path in verify.sh) is dead code: it reads last_updated_at through a DateTime64(6)-declared column, and ClickHouse coerces to the declared type at read time, so uniqExact(last_updated_at) always equals uniqExact(toDateTime64(last_updated_at, 6)) and collapse_keys is structurally 0 on the post-000019 schema — the exit-3 path is unreachable. It has been inert since #7572. The team is accepting the (vanishingly small) risk of sub-microsecond version-selection collision, and the product is standardizing on microsecond dates, so the guard has no future either. Remove it rather than keep dead code with a misleading comment. The microsecond fidelity fingerprint (count + normalized checksum, FINAL + mask-honored) is unchanged and remains the verify gate; verify.sh now exits 0 (PASS) / 1 (fidelity mismatch) only. Validated live: backfill traces -> traces_local_v2 then verify PASSES all windows (exit 0) with the simplified final line; the mismatch path still FAILS (exit 1); no version-collapse output remains. bash -n clean; gate test unaffected (it does not reference the guard). ---------

github-actions Bot assigned andrescrz Jul 22, 2026

baz-reviewer Bot added the baz: pending label Jul 22, 2026

github-actions Bot added the 🔴 size/XL label Jul 22, 2026

andrescrz marked this pull request as ready for review July 22, 2026 13:56

andrescrz requested review from a team as code owners July 22, 2026 13:56