Skip to content

Commit 4f760b4

Browse files
andrescrzclaude
andcommitted
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. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 8349610 commit 4f760b4

2 files changed

Lines changed: 70 additions & 8 deletions

File tree

apps/opik-backend/data-migrations/traces-local-v2-cutover/scripts/finalize.sh

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,14 @@
1414
# the exact 000101 shadow — schema, codecs (000106/000107) and replica path — so the
1515
# estate matches the applied Liquibase state and a retry starts from a clean shadow.
1616
# Both are `*_backup` names — retained until this script runs; the working `traces_local_v2` shadow is never detected as a
17-
# backup. This detects whichever parked table is present and never touches the live `traces` / `traces_local` shard. It
18-
# refuses if the live `traces` is empty while the backup is not (the live table may be unhealthy and the "backup" the only
19-
# copy), and if BOTH parked names exist (an ambiguous, unexpected state that a human must resolve).
17+
# backup. This detects whichever parked table is present and never touches the live `traces` / `traces_local` shard.
18+
# Detection is CLUSTER-WIDE (via clusterAllReplicas, like exchange_and_wrap.sh's settle gate): because finalize is the one
19+
# irreversible step and production is multi-replica, a name present on only SOME replicas means an ON CLUSTER DDL has not
20+
# finished propagating, so acting on the connected node's partial view could recycle/drop mid-transition — it refuses
21+
# loudly instead. It also refuses if the live `traces` is empty while the backup is not (the live table may be unhealthy
22+
# and the "backup" the only copy), if BOTH parked names exist (an ambiguous state a human must resolve), and — before a
23+
# recycle — if `traces_local_v2` already exists (recycle renames the backup INTO that name; a stray shadow means a retry
24+
# cutover started before the rollback was finalized).
2025
#
2126
# Connection: clickhouse-client env vars (CLICKHOUSE_HOST, CLICKHOUSE_PORT, CLICKHOUSE_USER, CLICKHOUSE_PASSWORD).
2227
#
@@ -45,16 +50,39 @@ ch() {
4550
clickhouse-client --database "$DATABASE" --log_comment 'traces_local_v2_cutover:finalize' --query "$1"
4651
}
4752

48-
exists() {
49-
ch "SELECT count() FROM system.tables WHERE database = '$DATABASE' AND name = '$1'"
53+
# Cluster-wide detection. finalize is the one irreversible step and production is multi-replica, so a table's presence is
54+
# resolved across ALL replicas (clusterAllReplicas, mirroring exchange_and_wrap.sh's settle gate), not just the connected
55+
# node. Resolve the cluster and its replica count once; a down replica makes clusterAllReplicas throw — correct here,
56+
# since finalizing against an estate we cannot fully see would be unsafe.
57+
CLUSTER="$(ch "SELECT getMacro('cluster')")"
58+
[[ -n "$CLUSTER" ]] || { echo "ERROR: could not resolve the '{cluster}' macro (getMacro('cluster') was empty)." >&2; exit 1; }
59+
REPLICAS="$(ch "SELECT count() FROM clusterAllReplicas('$CLUSTER', system.one)")"
60+
61+
# Classify a table across the cluster: sets CLUSTER_HAS=1 if present on ALL replicas, 0 if on none, and refuses loudly on
62+
# a mixed (present on some) state — an unfinished ON CLUSTER propagation the connected-node view would hide. Call it
63+
# directly (NOT in "$(...)"), so its refuse-exit stops the whole script rather than only a subshell.
64+
CLUSTER_HAS=0
65+
classify() {
66+
local n
67+
n="$(ch "SELECT count() FROM clusterAllReplicas('$CLUSTER', system.tables) WHERE database = '$DATABASE' AND name = '$1'")"
68+
if [[ "$n" == "0" ]]; then
69+
CLUSTER_HAS=0
70+
elif [[ "$n" == "$REPLICAS" ]]; then
71+
CLUSTER_HAS=1
72+
else
73+
echo "ERROR: '$1' exists on $n of $REPLICAS replicas — an ON CLUSTER DDL has not finished propagating." >&2
74+
echo " Refusing to finalize a mid-transition cluster; let it settle (or fix the unfinished host), then re-run." >&2
75+
exit 1
76+
fi
5077
}
5178

52-
[[ "$(exists traces)" != "0" ]] || { echo "ERROR: live 'traces' table not found in '$DATABASE'." >&2; exit 1; }
79+
classify traces
80+
[[ "$CLUSTER_HAS" == "1" ]] || { echo "ERROR: live 'traces' table not found on all replicas in '$DATABASE'." >&2; exit 1; }
5381

5482
# Detect the parked backup by name: traces_pre_cutover_backup (post-successful-cutover) or traces_post_rollback_backup
5583
# (post-rollback). They never co-exist in a clean flow; if both are present the estate is ambiguous — refuse.
56-
HAS_PRECUTOVER="$([[ "$(exists traces_pre_cutover_backup)" != "0" ]] && echo 1 || echo 0)"
57-
HAS_POST_ROLLBACK="$([[ "$(exists traces_post_rollback_backup)" != "0" ]] && echo 1 || echo 0)"
84+
classify traces_pre_cutover_backup; HAS_PRECUTOVER="$CLUSTER_HAS"
85+
classify traces_post_rollback_backup; HAS_POST_ROLLBACK="$CLUSTER_HAS"
5886

5987
if [[ "$HAS_PRECUTOVER" == "1" && "$HAS_POST_ROLLBACK" == "1" ]]; then
6088
echo "ERROR: both 'traces_pre_cutover_backup' and 'traces_post_rollback_backup' exist — ambiguous state." >&2
@@ -94,6 +122,17 @@ if [[ "$BACKUP" == "traces_post_rollback_backup" ]]; then
94122
# naming a laggard that then converges via the DDL queue), NOT globally atomic. Both statements touch only the parked
95123
# backup / disposable shadow — never the live `traces` — so unlike the rollback promote and the wrap (which rename live
96124
# `traces`) the brief cross-replica skew is invisible to readers, and finalize needs no maintenance window.
125+
#
126+
# Guard the destination first: recycle renames the backup INTO `traces_local_v2`, and ClickHouse RENAME fails on an
127+
# existing target. A stray `traces_local_v2` here means a retry cutover started before this rollback was finalized —
128+
# refuse (cluster-wide) BEFORE truncating, so we fail early with a clear message instead of after the TRUNCATE.
129+
classify traces_local_v2
130+
if [[ "$CLUSTER_HAS" != "0" ]]; then
131+
echo "ERROR: 'traces_local_v2' already exists — cannot recycle '$BACKUP' into it (RENAME will not overwrite)." >&2
132+
echo " This usually means a retry cutover began before the rollback was finalized. Resolve the estate" >&2
133+
echo " (inspect/drop 'traces_local_v2') before recycling." >&2
134+
exit 1
135+
fi
97136
if [[ "$CONFIRM" != "1" ]]; then
98137
echo "DRY RUN: would recycle $DATABASE.$BACKUP into an empty $DATABASE.traces_local_v2 (TRUNCATE + RENAME)."
99138
echo " Re-run with --confirm."

apps/opik-backend/src/test/java/com/comet/opik/infrastructure/TracesLocalV2CutoverTest.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,16 @@ void bufferedCutoverPreservesEveryDeletionAcrossExchange() {
496496
assertThat(tableExists("traces_post_rollback_backup"))
497497
.as("rollback ends in the canonical state: successor data parked as traces_post_rollback_backup")
498498
.isTrue();
499+
assertThat(liveCount("traces_post_rollback_backup", postWrapDeleted, workspaceId))
500+
.as("parked backup keeps the successor's post-wrap delete masked (not resurrected in the backup)")
501+
.isZero();
502+
assertThat(liveCount("traces_post_rollback_backup", idStrings(survivors.subList(1, survivors.size())),
503+
workspaceId))
504+
.as("parked backup actually holds the successor data (survivors), not an empty or wrong table")
505+
.isEqualTo(survivors.size() - 1);
506+
assertThat(columnType("traces_post_rollback_backup", "end_time"))
507+
.as("parked backup carries the successor's non-Nullable schema, confirming the right table was parked")
508+
.doesNotStartWith("Nullable");
499509
assertThat(tableExists("traces_local_v2"))
500510
.as("the disposable shadow name is free after rollback (so stage A cannot truncate the backup)")
501511
.isFalse();
@@ -591,6 +601,19 @@ void rollbackAfterExchangeSwapsBackWithoutResurrectingDeletes() {
591601
assertThat(tableExists("traces_post_rollback_backup"))
592602
.as("canonical state: successor parked as traces_post_rollback_backup")
593603
.isTrue();
604+
assertThat(liveCount("traces_post_rollback_backup", postCutoverDeleted, workspaceId))
605+
.as("parked backup keeps the post-cutover delete masked (not resurrected in the backup)")
606+
.isZero();
607+
assertThat(liveCount("traces_post_rollback_backup", idStrings(windowDeleted), workspaceId))
608+
.as("parked backup does not resurrect window deletes")
609+
.isZero();
610+
assertThat(liveCount("traces_post_rollback_backup", idStrings(survivors.subList(1, survivors.size())),
611+
workspaceId))
612+
.as("parked backup actually holds the successor data (survivors), not an empty or wrong table")
613+
.isEqualTo(survivors.size() - 1);
614+
assertThat(columnType("traces_post_rollback_backup", "end_time"))
615+
.as("parked backup carries the successor's non-Nullable schema")
616+
.doesNotStartWith("Nullable");
594617
assertThat(tableExists("traces_local_v2"))
595618
.as("the disposable shadow name is free after rollback (so stage A cannot truncate the backup)")
596619
.isFalse();

0 commit comments

Comments
 (0)