diff --git a/docs/troubleshooting/dolt-bloat-recovery.md b/docs/troubleshooting/dolt-bloat-recovery.md index 2395f369d1..bf92f195d1 100644 --- a/docs/troubleshooting/dolt-bloat-recovery.md +++ b/docs/troubleshooting/dolt-bloat-recovery.md @@ -206,8 +206,8 @@ should treat these strings as the current vocabulary: | `post-flatten row count decreased` | A table lost rows after flatten. | | `post-flatten row count probe failed` | The post-flatten row-count query failed or returned a non-number. | | `post-flatten table value hash probe failed` | A post-flatten table hash query failed or returned empty. | -| `post-flatten table value hash changed with row-count increase` | A table gained rows and its value hash changed. | -| `post-flatten table value hash changed without row-count increase` | A table's value hash changed without a row-count gain. | +| `post-flatten table value hash changed with row-count increase` | A table gained rows and its value hash changed, and rows were removed across the flatten (or the removal probe failed). | +| `post-flatten table value hash changed without row-count increase` | A table's value hash changed without a row-count gain, and rows were removed across the flatten (or the removal probe failed). | | `post-flatten table list changed` | A table appeared or an invalid table name was observed after preflight. | | `post-flatten table list probe failed` | The post-flatten `information_schema.tables` query failed. | | `post-flatten value hash probe failed` | The database hash query failed after flatten. | @@ -215,6 +215,20 @@ should treat these strings as the current vocabulary: | `post-flatten value hash changed with row-count increase` | The database hash changed after at least one stable-table row-count gain. | | `post-flatten value hash changed without row-count increase` | The database hash changed without a row-count gain. | +Table value-hash drift on its own is **not** a quarantine reason. Against a +running city both row counts and value hashes drift on every pass from ordinary +traffic — an append-only event log always gains rows, and an in-place update +(any `bd update`) shifts a table's hash at an unchanged row count — so neither +can tell row loss from concurrent writes. Drift is only the trigger. The verdict +comes from `DOLT_DIFF(, , )`: the run +quarantines only when rows were **removed** (or the diff probe itself failed), +and defers to the next run when nothing was removed. Added and modified rows are +expected on a live store. + +That verdict decides drift alone. A run that also saw a row-count decrease, a +table appear or disappear, or any probe fail never reaches the diff and +quarantines on that reason regardless of what was removed. + Quarantine markers also carry structured evidence. New markers include the database name, the preflight/flatten/post-verify HEADs, preflight and postflight database value hashes when available, `integrity_table_drift` for diff --git a/examples/bd/dolt/assets/scripts/compact-gain-drift-proof.sh b/examples/bd/dolt/assets/scripts/compact-gain-drift-proof.sh index db50ae4e35..3ed27eda46 100644 --- a/examples/bd/dolt/assets/scripts/compact-gain-drift-proof.sh +++ b/examples/bd/dolt/assets/scripts/compact-gain-drift-proof.sh @@ -1,31 +1,87 @@ #!/bin/sh -# compact-gain-drift-proof.sh — Option A row-preservation proof for the -# post-flatten gain+drift case (gastownhall/gascity#2846). +# compact-gain-drift-proof.sh — row-preservation proofs for post-flatten value +# hash drift (gastownhall/gascity#2846, generalized in gc-800l/gc-i52hj). # -# When verify_counts sees a table gain rows AND its value hash drift, the -# safety property at stake ("pre-flight rows remain reachable") cannot be -# inferred from HEAD movement alone: a concurrent writer whose commit is -# ABSORBED into the flatten commit moves no HEAD, so the HEAD-proven gate -# misses it and a benign race is hard-quarantined — which then blocks all -# future GC of a busy DB (the memory-exhaustion failure the code calls out). +# When verify_counts sees a table's value hash drift, the safety property at +# stake ("pre-flight rows remain reachable") cannot be inferred from HEAD +# movement alone: a concurrent writer whose commit is ABSORBED into the flatten +# commit moves no HEAD, so the HEAD-proven gate misses it and a benign race is +# hard-quarantined — which then blocks all future GC of a busy DB (the +# memory-exhaustion failure the code calls out). # -# This proves preservation DIRECTLY: for each gained+drifted table, diff the -# pre-flight snapshot HEAD against the flatten commit. If the only change is -# `added` rows (no `removed`/`modified`), every pre-flight row survived and the -# gain is concurrent-writer data — defer, exactly as the HEAD-proven path does. -# It is strictly more rigorous than the HEAD proxy: it proves reachability -# instead of inferring it. Any removed/modified row, or any probe failure, -# fails closed and falls through to quarantine. +# These prove preservation DIRECTLY: for each drifted table, diff the pre-flight +# snapshot HEAD against the flatten commit. That is strictly more rigorous than +# the HEAD proxy — it proves reachability instead of inferring it — and any +# probe failure fails closed and falls through to quarantine. +# +# Two predicates, because the two callers ask different questions: +# +# * drift_preserves_preflight_rows — counts `removed` rows only. Used for +# tables that existed at the pre-flight root, where the question is whether +# the flatten dropped any of the rows it was asked to preserve. +# * diff_is_additive_only — counts every non-`added` row. Used for a table +# the flatten FIRST-COMMITTED, which has no pre-flight rows to preserve; +# there anything but a pure add is unexplained. +# +# Why the drift proof counts removals and not "everything except added" +# (gc-800l): a live city writes in place as well as appending. `bd update` +# rewrites an existing row, which DOLT_DIFF reports as `modified` — the row is +# still reachable, nothing was lost. Counting `modified` as unprovable made the +# single most common bd write indistinguishable from corruption, so a 24h +# unattended compaction quarantined on essentially every pass and each one cost +# a human review to clear. Removal is the only diff_type that answers the +# question the check actually asks. +# +# Scope note: this reads `removed` as "a pre-flight row is gone", which holds +# for keyed tables — the shape bd uses. A KEYLESS table has no identity to +# track a row by, so Dolt reports an in-place update there as a removed/added +# pair; such a table still fails this proof and falls through to quarantine. +# That is the safe direction and matches the behaviour before this proof +# existed, so it is a limitation rather than a regression. # # Depends on `query_single_cell` and `valid_table_name` from run.sh. +# diff_has_no_removed_rows
+# Returns 0 iff the table's .. content diff contains no `removed` +# rows. Returns non-zero (fail closed) if either commit endpoint is missing, +# the table name is missing or invalid, the diff probe fails or returns a +# non-numeric result, or the table shows removed rows. +# +# Deliberately weaker than diff_is_additive_only: `added` and `modified` rows +# are ordinary concurrent-writer traffic on a live store and leave every +# pre-flight row reachable. Only a removal answers "a row we were asked to +# preserve is gone". +diff_has_no_removed_rows() { + _dr_db="$1" + _dr_from="$2" + _dr_to="$3" + _dr_t="$4" + # Without both commit endpoints there is nothing to diff against — fail closed. + [ -n "$_dr_from" ] && [ -n "$_dr_to" ] && [ -n "$_dr_t" ] || return 1 + valid_table_name "$_dr_t" || return 1 + # Count rows that stopped being reachable between the two commits. Zero means + # every row present at is still present at , however its values + # may have been rewritten in place since. + if ! _dr_removed=$(query_single_cell "$_dr_db" \ + "drift preservation diff probe failed for table=$_dr_t" \ + "SELECT COUNT(*) FROM DOLT_DIFF('$_dr_from', '$_dr_to', '$_dr_t') WHERE diff_type = 'removed'"); then + return 1 + fi + case "$_dr_removed" in + 0) return 0 ;; # no removals — this table's rows preserved + ''|*[!0-9]*) return 1 ;; # empty/non-numeric probe result — fail closed + *) return 1 ;; # one or more removed rows — not preservable + esac +} + # diff_is_additive_only
# Returns 0 iff the table's .. content diff contains only `added` # rows. Returns non-zero (fail closed) if either commit endpoint is missing, # the table name is missing or invalid, the diff probe fails or returns a -# non-numeric result, or the table shows removed/modified rows. Shared by the -# gain+drift preservation proof and the committed-root drift proof's -# first-committed table case (run.sh db_root_drift_within_verified_tables). +# non-numeric result, or the table shows removed/modified rows. Used by the +# committed-root drift proof's first-committed table case (run.sh +# db_root_drift_within_verified_tables), where the table exists in no +# pre-flatten commit so every row must be an add. diff_is_additive_only() { _da_db="$1" _da_from="$2" @@ -49,11 +105,11 @@ diff_is_additive_only() { esac } -# gain_drift_is_additive_only -# Returns 0 iff every listed table's .. content diff contains only -# `added` rows. Returns non-zero (fail closed) if the table list is empty or -# any table fails diff_is_additive_only. -gain_drift_is_additive_only() { +# drift_preserves_preflight_rows +# Returns 0 iff every listed table's .. content diff contains no +# `removed` rows. Returns non-zero (fail closed) if the table list is empty or +# any table fails diff_has_no_removed_rows. +drift_preserves_preflight_rows() { _gd_db="$1" _gd_from="$2" _gd_to="$3" @@ -61,7 +117,7 @@ gain_drift_is_additive_only() { _gd_seen=0 for _gd_t in $_gd_tables; do _gd_seen=1 - diff_is_additive_only "$_gd_db" "$_gd_from" "$_gd_to" "$_gd_t" || return 1 + diff_has_no_removed_rows "$_gd_db" "$_gd_from" "$_gd_to" "$_gd_t" || return 1 done # An empty table list is not a proof of preservation. [ "$_gd_seen" = "1" ] || return 1 diff --git a/examples/bd/dolt/commands/compact/run.sh b/examples/bd/dolt/commands/compact/run.sh index 5c6c2ed74c..c370af48c1 100755 --- a/examples/bd/dolt/commands/compact/run.sh +++ b/examples/bd/dolt/commands/compact/run.sh @@ -22,11 +22,14 @@ # 2. Soft-reset to the root commit; all data stays staged. # 3. Commit everything as a single "compaction: flatten history" commit. # 4. Re-check post-flatten row counts, table value hashes, and database -# value hash. Row-count increases are treated as concurrent-writer -# evidence and allowed to continue only when table and database value -# hashes stay stable. Same-count table hash drift, table-list drift, -# or row-count decrease without a proven concurrent writer is -# quarantined before full GC. +# value hash. Counts and hashes are the cheap TRIGGER, not the verdict: +# against a live city both drift on every pass from ordinary traffic +# (an append-only event log always gains rows; an in-place update +# shifts a table hash at an unchanged count), so neither can tell row +# loss from concurrent writes. Any table hash drift is therefore +# settled by the removal-based proof in step 4b. Table-list drift, +# probe failures, and row-count decrease without a proven concurrent +# writer are quarantined before full GC. # 4a. Local-verify HEAD-stability gate. The pre-flight stability loop cannot # close the residual window between its final HEAD check and the flatten, # nor the window during post-flatten verify, so a normal MVCC writer (the @@ -45,7 +48,17 @@ # flatten's own commit (a writer landed during/after verify). All other # failures — and gain+drift or row-decrease with a stable HEAD — still # quarantine. Probe failure leaves the race unproven and quarantines. -# 4b. Committed-root drift gate. When per-table verification passed but the +# 4b. Row-preservation proof. HEAD movement is only a proxy for the property +# the check actually wants ("did any pre-flight row stop being +# reachable"), and it misses the absorbed-writer race entirely — a +# writer whose commit is folded into the flatten moves no HEAD. So any +# table value-hash drift, with or without a row-count gain, is settled +# by diffing the pre-flight snapshot HEAD against the flatten commit +# per drifted table and counting `removed` rows. Zero removals proves +# preservation directly and defers to the next run; added and modified +# rows are ordinary live-city traffic. Any removed row, or a probe +# failure, fails closed to the quarantine. +# 4c. Committed-root drift gate. When per-table verification passed but the # whole-database hash still drifted, DOLT_DIFF_STAT names the tables that # differ across the flatten. Drift is benign only when every named table # is either already verified or a table the -Am first-committed whose @@ -1093,8 +1106,9 @@ preflight_counts() { # verify_counts — re-count/re-hash and compare against the pre-flight file. # Row-count decreases fail. Row-count increases are recorded as concurrent # writer evidence only when the table value hash stays stable. Any table hash -# drift is quarantined before full GC because row-count gain alone cannot prove -# pre-flight rows remain reachable. Sets category flags plus +# drift fails here because count and hash alone cannot prove pre-flight rows +# remain reachable; the caller then decides between quarantine and defer on +# direct DOLT_DIFF evidence. Sets category flags, the drifted-table list, plus # verify_counts_failure_reason and verify_counts_failure_guidance for callers. verify_counts() { db="$1" @@ -1102,7 +1116,7 @@ verify_counts() { fail=0 verify_counts_saw_gain=0 verify_counts_saw_gain_hash_drift=0 - verify_counts_gain_drift_tables="" + verify_counts_drift_tables="" verify_counts_saw_row_decrease=0 verify_counts_saw_decrease_hash_drift=0 verify_counts_saw_same_count_hash_drift=0 @@ -1185,9 +1199,9 @@ verify_counts() { if [ "$actual_hash" != "$expected_hash" ]; then if [ "$table_gained_rows" = "1" ]; then verify_counts_saw_gain_hash_drift=1 - verify_counts_gain_drift_tables="$verify_counts_gain_drift_tables $t" + verify_counts_drift_tables="$verify_counts_drift_tables $t" verify_counts_drift_details="${verify_counts_drift_details};table=$t,before_rows=$expected,after_rows=$actual,before_hash=$expected_hash,after_hash=$actual_hash,category=row_count_gain_hash_drift" - printf 'compact: db=%s table=%s value hash changed with row-count increase before=%s after=%s — quarantine and investigate before GC\n' \ + printf 'compact: db=%s table=%s value hash changed with row-count increase before=%s after=%s — pending row-preservation verification\n' \ "$db" "$t" "$expected_hash" "$actual_hash" >&2 if [ "$fail" -ne 1 ]; then fail=1 @@ -1201,8 +1215,9 @@ verify_counts() { "$db" "$t" "$expected_hash" "$actual_hash" >&2 else verify_counts_drift_details="${verify_counts_drift_details};table=$t,before_rows=$expected,after_rows=$actual,before_hash=$expected_hash,after_hash=$actual_hash,category=same_row_count_hash_drift" - printf 'compact: db=%s table=%s value hash changed after flatten without row-count increase before=%s after=%s — quarantine and investigate before GC\n' \ + printf 'compact: db=%s table=%s value hash changed after flatten without row-count increase before=%s after=%s — pending row-preservation verification\n' \ "$db" "$t" "$expected_hash" "$actual_hash" >&2 + verify_counts_drift_tables="$verify_counts_drift_tables $t" verify_counts_saw_same_count_hash_drift=1 if [ "$fail" -ne 1 ]; then fail=1 @@ -2095,7 +2110,7 @@ flatten_database() { db="$1" verify_counts_saw_gain=0 verify_counts_saw_gain_hash_drift=0 - verify_counts_gain_drift_tables="" + verify_counts_drift_tables="" verify_counts_saw_row_decrease=0 verify_counts_saw_decrease_hash_drift=0 verify_counts_saw_same_count_hash_drift=0 @@ -2598,24 +2613,38 @@ flatten_database() { rm -f "$preflight_tmp" return 0 fi - # Option A (#2846): HEAD movement is only a proxy for "pre-flight rows - # remain reachable". When gain+drift is the only failure category but no - # concurrent writer was HEAD-proven — the absorbed-writer race, where the - # writer's commit was folded into the flatten and left no HEAD fingerprint - # — prove preservation directly by diffing the pre-flight snapshot HEAD - # against the flatten commit for each gained+drifted table. Purely additive - # (no removed/modified rows) proves every pre-flight row survived; defer - # exactly as the HEAD-proven path above does. Any removed/modified row, or - # a diff-probe failure, fails closed and falls through to the quarantine. - if [ "${verify_counts_saw_gain:-0}" = "1" ] && \ - [ "${verify_counts_saw_gain_hash_drift:-0}" = "1" ] && \ + # Row-preservation proof (#2846, generalized in gc-800l/gc-i52hj). HEAD + # movement and value-hash equality are both only proxies for the property + # that actually matters — "every pre-flight row is still reachable" — and on + # a live city neither proxy can answer. An absorbed writer leaves no HEAD + # fingerprint, and ordinary traffic drifts the hash on every pass: an + # append-only event log gains rows continuously, and a bd update rewrites a + # row in place, which drifts the hash at an unchanged row count. + # Quarantining those cost a human review per scheduled run and reclaimed + # nothing — the same end state as never compacting, plus recurring false + # alarms that train the reader to clear markers without reading them. + # + # So decide on direct evidence instead: diff the pre-flight snapshot HEAD + # against the flatten commit for every drifted table and count rows that + # stopped being reachable. Zero removals proves the flatten preserved + # everything it was asked to preserve, whether the drift came with a row + # gain or at an unchanged count; defer exactly as the HEAD-proven path above + # does. Any removed row, or a diff-probe failure, fails closed and falls + # through to the quarantine. + # + # Row-count DECREASE stays excluded and keeps its own HEAD-proven arm below: + # fewer rows with no removals is a contradiction, so the probe could only + # ever fail closed there, and leaving the category out keeps that path's + # semantics unchanged. Table-list drift and probe failures are excluded for + # the same fail-closed reason as before. + if { [ "${verify_counts_saw_gain_hash_drift:-0}" = "1" ] || \ + [ "${verify_counts_saw_same_count_hash_drift:-0}" = "1" ]; } && \ [ "${verify_counts_saw_row_decrease:-0}" != "1" ] && \ - [ "${verify_counts_saw_same_count_hash_drift:-0}" != "1" ] && \ [ "${verify_counts_saw_table_list_change:-0}" != "1" ] && \ [ "${verify_counts_saw_probe_failure:-0}" != "1" ] && \ - gain_drift_is_additive_only "$db" "$head" "$flatten_head" "$verify_counts_gain_drift_tables"; then - printf 'compact: db=%s gain+drift proven additive-only via DOLT_DIFF(%s..%s) for tables [%s] — pre-flight rows preserved (absorbed-writer race), not corruption; deferring, will retry next run\n' \ - "$db" "$head" "$flatten_head" "${verify_counts_gain_drift_tables# }" >&2 + drift_preserves_preflight_rows "$db" "$head" "$flatten_head" "$verify_counts_drift_tables"; then + printf 'compact: db=%s value-hash drift removed no pre-flight rows, proven via DOLT_DIFF(%s..%s) for tables [%s] — concurrent-writer appends/updates, not corruption; deferring, will retry next run\n' \ + "$db" "$head" "$flatten_head" "${verify_counts_drift_tables# }" >&2 if ! defer_writer_race_after_flatten "$db" "$flatten_head" \ "$remote" "$expected_remote_head" "$expected_remote_head_verified" \ "${compacted_from_head:-}" "$local_branch" "$remote_branch"; then @@ -2649,6 +2678,7 @@ flatten_database() { fi if [ "$writer_race_detected" = "1" ] && \ { [ "${verify_counts_saw_gain_hash_drift:-0}" = "1" ] || \ + [ "${verify_counts_saw_same_count_hash_drift:-0}" = "1" ] || \ [ "${verify_counts_saw_row_decrease:-0}" = "1" ]; }; then printf 'compact: db=%s writer race detected during flatten (snapshot_HEAD=%s pre_reset_HEAD=%s flatten_HEAD=%s post_verify_HEAD=%s), but additional integrity failure category prevents defer; quarantine unchanged\n' \ "$db" "$head" "${head_before_reset:-}" "$flatten_head" "${post_verify_head:-}" >&2 diff --git a/examples/bd/dolt/dog_exec_scripts_test.go b/examples/bd/dolt/dog_exec_scripts_test.go index f785281d13..ae404182b3 100644 --- a/examples/bd/dolt/dog_exec_scripts_test.go +++ b/examples/bd/dolt/dog_exec_scripts_test.go @@ -826,7 +826,11 @@ case "$query" in print_cell hash-beads-after-writer exit 0 fi - if [ "$mode" = "same_row_count_writer" ] && [ "$(current_head)" = "compactcommit" ]; then + # same_row_count_writer and same_row_count_row_loss share this shape: the + # row count is unchanged and the table value hash drifts. They differ only + # in what DOLT_DIFF reports — an in-place update (nothing removed) versus + # real row loss — which is what the gate must decide on. + if { [ "$mode" = "same_row_count_writer" ] || [ "$mode" = "same_row_count_row_loss" ]; } && [ "$(current_head)" = "compactcommit" ]; then print_cell hash-beads-after-writer exit 0 fi @@ -987,6 +991,31 @@ case "$query" in print_cell beads exit 0 ;; + *"DOLT_DIFF("*"diff_type = 'removed'"*) + # Row-preservation proof (gc-800l). Counts rows that stopped being + # reachable between the pre-flight snapshot HEAD and the flatten commit. + # MUST precede the SELECT COUNT(*) FROM
arms below, whose patterns + # also match this query's text. Matching the diff_type = 'removed' clause + # rather than DOLT_DIFF alone keeps this clear of the additive-only probe + # on a first-committed table, which asks diff_type <> 'added' and is + # answered by its own arm further down. + # + # The modes listed here model genuine row loss — rows replaced or dropped + # across the flatten. Every other mode models ordinary live-city traffic + # (appends and in-place updates), which produces the same row-count and + # value-hash drift but removes nothing, so the drift is proven benign. + # That opposition is the whole point of the removal-based gate: identical + # count/hash evidence, opposite verdicts, discriminated by removals alone. + case "$mode" in + same_table_replacement_with_row_gain|writer_race_with_mixed_same_count_hash_drift|same_row_count_row_loss) + print_cell 2 + ;; + *) + print_cell 0 + ;; + esac + exit 0 + ;; *"SELECT COUNT(*) FROM"*"blocked_issues"*) if [ "$db" = "blocked_issues" ]; then printf 'database not found: blocked_issues\n' >&2 @@ -2311,11 +2340,19 @@ func TestCompactScriptQuarantinesSameTableRowGainWithValueHashDriftBeforeFullGC( } } -func TestCompactScriptQuarantinesMixedRowGainAndSameCountHashDriftBeforeFullGC(t *testing.T) { +// The exact signature the lx city produced on the first scheduled run after +// auto-compaction was enabled (gc-800l): an append-only table gains a row and +// drifts, while a second table is updated in place and drifts at an unchanged +// row count. Both were ordinary traffic — DOLT_DIFF across the recorded HEADs +// showed zero removals in either table. Every defer arm excluded same-count +// drift categorically, so this quarantined and cost a human review. With no +// rows removed, the flatten preserved everything it was asked to preserve: +// defer and retry next run. +func TestCompactScriptDefersMixedRowGainAndSameCountHashDrift(t *testing.T) { fixture := newCompactScriptFixture(t) out, err := fixture.run(t, "mixed_row_count_gain_and_same_count_hash_drift", "GC_DOLT_COMPACT_THRESHOLD_COMMITS=500") - if err == nil { - t.Fatalf("compact succeeded despite mixed row gain and same-count hash drift:\n%s", out) + if err != nil { + t.Fatalf("live-writer append plus in-place update must defer, not fail: %v\n%s", err, out) } if !strings.Contains(out, "table=beads gained rows during flatten") { t.Fatalf("output missing row-count gain evidence:\n%s", out) @@ -2323,6 +2360,10 @@ func TestCompactScriptQuarantinesMixedRowGainAndSameCountHashDriftBeforeFullGC(t if !strings.Contains(out, "table=notes value hash changed after flatten without row-count increase") { t.Fatalf("output missing same-count hash drift warning:\n%s", out) } + if !strings.Contains(out, "removed no pre-flight rows") || + !strings.Contains(out, "deferring, will retry next run") { + t.Fatalf("output missing removal-based preservation defer message:\n%s", out) + } logData, err := os.ReadFile(fixture.doltLog) if err != nil { t.Fatalf("read dolt log: %v", err) @@ -2331,12 +2372,18 @@ func TestCompactScriptQuarantinesMixedRowGainAndSameCountHashDriftBeforeFullGC(t if !strings.Contains(log, "DOLT_HASHOF_TABLE('beads')") || !strings.Contains(log, "DOLT_HASHOF_TABLE('notes')") { t.Fatalf("mixed drift test should probe table value hashes:\n%s", log) } + // Both drifted tables must be proven, not just the gained one: the + // same-count table is the half that had no proof path before. + if !strings.Contains(log, "DOLT_DIFF('headcommit', 'compactcommit', 'beads')") || + !strings.Contains(log, "DOLT_DIFF('headcommit', 'compactcommit', 'notes')") { + t.Fatalf("every drifted table must be covered by the removal probe:\n%s", log) + } if strings.Contains(log, "DOLT_GC") { - t.Fatalf("mixed row gain and same-count hash drift must block full GC:\n%s", log) + t.Fatalf("preservation defer must skip GC this run:\n%s", log) } marker := filepath.Join(fixture.cityPath, ".gc", "runtime", "packs", "dolt", "compact-quarantine", "beads") - if reason := compactMarkerValue(t, marker, "reason"); reason != "post-flatten table value hash changed with row-count increase" { - t.Fatalf("quarantine reason should identify first table hash drift, got %q", reason) + if _, statErr := os.Stat(marker); !os.IsNotExist(statErr) { + t.Fatalf("live-writer drift must NOT write a quarantine marker; stat=%v", statErr) } } @@ -2585,9 +2632,11 @@ func TestCompactScriptWriterRaceGateUsesFlagNotReasonText(t *testing.T) { } } -// Control: the same gain+drift signal with a STABLE HEAD (no writer proven) is a -// genuine anomaly and must still write the blocking quarantine marker and fail. -// This guards against the writer-race gate weakening real-corruption detection. +// Control: the same gain+drift signal with a STABLE HEAD (no writer proven), +// where the diff also shows rows removed, is a genuine anomaly and must still +// write the blocking quarantine marker and fail. This guards against the +// writer-race gate and the preservation proof weakening real-corruption +// detection: neither a HEAD proxy nor a removal probe may pass this run. func TestCompactScriptStillQuarantinesGainAndHashDriftWithStableHead(t *testing.T) { fixture := newCompactScriptFixture(t) out, err := fixture.run(t, "same_table_replacement_with_row_gain", "GC_DOLT_COMPACT_THRESHOLD_COMMITS=500") @@ -3190,29 +3239,83 @@ func TestCompactScriptIntegrityReasonOutranksEarlierProbeFailure(t *testing.T) { } } -func TestCompactScriptQuarantinesSameRowCountWriterBeforeFullGC(t *testing.T) { +// Production incident (lx 2026-08-09 and again 2026-08-15, gc-800l/gc-i52hj): +// a concurrent `bd update` rewrites an existing row in place. The row count is +// unchanged and the table value hash drifts — the signature of the single most +// common write in a running city, which the check used to treat as unexplained +// and quarantine on, with no proof path at all. Every defer arm was guarded off +// by that one category, so an unattended 24h timer generated a human review per +// pass and never reclaimed; the second incident blocked reclaim for 4 days and +// let one database reach 7-11x its peers. An in-place update removes nothing, +// so the removal-based proof settles it directly: defer and retry next run. +func TestCompactScriptDefersSameRowCountWriterDrift(t *testing.T) { fixture := newCompactScriptFixture(t) out, err := fixture.run(t, "same_row_count_writer", "GC_DOLT_COMPACT_THRESHOLD_COMMITS=500") + if err != nil { + t.Fatalf("in-place update drift must defer, not fail: %v\n%s", err, out) + } + if !strings.Contains(out, "value hash changed after flatten") { + t.Fatalf("output missing value-hash drift warning:\n%s", out) + } + if !strings.Contains(out, "removed no pre-flight rows") || + !strings.Contains(out, "deferring, will retry next run") { + t.Fatalf("output missing removal-based preservation defer message:\n%s", out) + } + marker := filepath.Join(fixture.cityPath, ".gc", "runtime", "packs", "dolt", "compact-quarantine", "beads") + if _, statErr := os.Stat(marker); !os.IsNotExist(statErr) { + t.Fatalf("in-place update drift must NOT write a quarantine marker; stat=%v", statErr) + } + pendingGC := filepath.Join(fixture.cityPath, ".gc", "runtime", "packs", "dolt", "compact-pending-gc", "beads") + if reason := compactMarkerValue(t, pendingGC, "reason"); reason != "writer race during flatten deferred full GC" { + t.Fatalf("preservation defer should record pending-GC retry marker, got reason %q", reason) + } + logData, err := os.ReadFile(fixture.doltLog) + if err != nil { + t.Fatalf("read dolt log: %v", err) + } + log := string(logData) + if !strings.Contains(log, "DOLT_DIFF(") || !strings.Contains(log, "diff_type = 'removed'") { + t.Fatalf("defer must be justified by a removal probe, not by the hash comparison:\n%s", log) + } + if strings.Contains(log, "DOLT_GC") { + t.Fatalf("preservation defer must skip GC this run:\n%s", log) + } +} + +// The other half of the positive control: the SAME count/hash signature as the +// benign in-place update above, but the diff reports rows that stopped being +// reachable. Removing the false alarm must not remove the alarm — this is real +// row loss and must still quarantine before full GC, with the drift evidence +// recorded for the human who clears the marker. +func TestCompactScriptQuarantinesSameRowCountDriftWithRemovedRows(t *testing.T) { + fixture := newCompactScriptFixture(t) + out, err := fixture.run(t, "same_row_count_row_loss", "GC_DOLT_COMPACT_THRESHOLD_COMMITS=500") if err == nil { - t.Fatalf("compact succeeded despite same-row-count value-hash drift:\n%s", out) + t.Fatalf("compact succeeded despite rows removed across the flatten:\n%s", out) } if !strings.Contains(out, "value hash changed after flatten") { t.Fatalf("output missing value-hash drift warning:\n%s", out) } + if strings.Contains(out, "deferring, will retry next run") { + t.Fatalf("row loss must never defer:\n%s", out) + } + if !strings.Contains(out, "post-flatten INTEGRITY check failed") { + t.Fatalf("row loss should escalate as an integrity failure:\n%s", out) + } logData, err := os.ReadFile(fixture.doltLog) if err != nil { t.Fatalf("read dolt log: %v", err) } log := string(logData) - if !strings.Contains(log, "DOLT_HASHOF_DB") { - t.Fatalf("same-row-count writer test should probe database value hash:\n%s", log) + if !strings.Contains(log, "diff_type = 'removed'") { + t.Fatalf("row-loss quarantine should be decided by the removal probe:\n%s", log) } if strings.Contains(log, "DOLT_GC") { - t.Fatalf("same-row-count value-hash drift must block full GC:\n%s", log) + t.Fatalf("row loss must block full GC:\n%s", log) } marker := filepath.Join(fixture.cityPath, ".gc", "runtime", "packs", "dolt", "compact-quarantine", "beads") if _, err := os.Stat(marker); err != nil { - t.Fatalf("same-row-count value-hash drift should write quarantine marker: %v", err) + t.Fatalf("row loss should write quarantine marker: %v", err) } assertCompactMarkerHasEvidence(t, marker, "reason=post-flatten table value hash changed without row-count increase", diff --git a/test/dolt/compact_gain_drift_proof_test.sh b/test/dolt/compact_gain_drift_proof_test.sh old mode 100644 new mode 100755 index 703906c893..753f5a4344 --- a/test/dolt/compact_gain_drift_proof_test.sh +++ b/test/dolt/compact_gain_drift_proof_test.sh @@ -1,11 +1,18 @@ #!/bin/sh -# Unit test for gain_drift_is_additive_only (Option A preservation proof, #2846). +# Unit test for the post-flatten row-preservation proofs (#2846, generalized to +# removal-based for the drift path in gc-800l/gc-i52hj). # Lib under test: examples/bd/dolt/assets/scripts/compact-gain-drift-proof.sh # # Stubs the run.sh-provided dependencies (query_single_cell, valid_table_name) -# so the additive-only classification is exercised without a live Dolt server. +# so the preservation classification is exercised without a live Dolt server. # The full flatten path needs a concurrent-writer race that cannot be reproduced # deterministically in a unit test (see #2846); this covers the decision logic. +# +# Two predicates are covered because run.sh asks two different questions: +# drift_preserves_preflight_rows (did the flatten drop a row it was asked to +# preserve — removals only) and diff_is_additive_only (a table the flatten +# first-committed, which has no pre-flight rows, so anything but an add is +# unexplained). Collapsing them would re-break the case gc-800l fixed. set -u HERE=$(unset CDPATH; cd -- "$(dirname "$0")" && pwd) @@ -14,9 +21,14 @@ LIB="$HERE/../../examples/bd/dolt/assets/scripts/compact-gain-drift-proof.sh" # --- stubs for run.sh-provided helpers -------------------------------------- # query_single_cell : extract the table from the DOLT_DIFF -# query and echo the canned non-added count stub_count_
(default 0). +# query and echo the canned count stub_count_
(default 0) — read as +# removed rows or as non-added rows depending on which predicate is calling. # STUB_FAIL_TABLE forces a probe failure; STUB_EMPTY_TABLE forces an empty result. +# The query text is appended to $QUERY_LOG — a file, not a variable, because the +# proof calls this from a command substitution, which runs it in a subshell +# whose variable assignments never reach the harness. query_single_cell() { + printf '%s\n' "$3" >> "$QUERY_LOG" _t=$(printf '%s\n' "$3" | sed -n "s/.*DOLT_DIFF('[^']*', *'[^']*', *'\([^']*\)').*/\1/p") if [ -n "${STUB_FAIL_TABLE:-}" ] && [ "$_t" = "$STUB_FAIL_TABLE" ]; then return 1 @@ -45,52 +57,91 @@ valid_table_name() { # --- harness ---------------------------------------------------------------- pass=0 fail=0 +QUERY_LOG=$(mktemp) +trap 'rm -f "$QUERY_LOG"' EXIT ok() { pass=$((pass + 1)); printf 'ok - %s\n' "$1"; } no() { fail=$((fail + 1)); printf 'FAIL - %s\n' "$1"; } reset() { + : > "$QUERY_LOG" unset STUB_FAIL_TABLE STUB_EMPTY_TABLE STUB_INVALID_TABLE \ stub_count_issues stub_count_mail 2>/dev/null || true } -# 1. single table, purely additive -> preserved (defer) +# --- drift_preserves_preflight_rows: removals decide ------------------------ + +# 1. single table, no removed rows -> preserved (defer) reset; stub_count_issues=0 -if gain_drift_is_additive_only db H1 H2 "issues"; then ok "additive-only single table -> defer"; else no "additive-only single table -> defer"; fi +if drift_preserves_preflight_rows db H1 H2 "issues"; then ok "no removed rows single table -> defer"; else no "no removed rows single table -> defer"; fi -# 2. single table with removed/modified rows -> not preservable (quarantine) +# 2. single table with removed rows -> not preservable (quarantine) reset; stub_count_issues=2 -if gain_drift_is_additive_only db H1 H2 "issues"; then no "removed/modified -> quarantine"; else ok "removed/modified -> quarantine"; fi +if drift_preserves_preflight_rows db H1 H2 "issues"; then no "removed rows -> quarantine"; else ok "removed rows -> quarantine"; fi -# 3. two tables, both additive -> preserved +# 3. two tables, neither with removals -> preserved reset; stub_count_issues=0; stub_count_mail=0 -if gain_drift_is_additive_only db H1 H2 "issues mail"; then ok "two additive tables -> defer"; else no "two additive tables -> defer"; fi +if drift_preserves_preflight_rows db H1 H2 "issues mail"; then ok "two clean tables -> defer"; else no "two clean tables -> defer"; fi -# 4. two tables, one not additive -> not preservable +# 4. two tables, one with removals -> not preservable reset; stub_count_issues=0; stub_count_mail=5 -if gain_drift_is_additive_only db H1 H2 "issues mail"; then no "mixed tables -> quarantine"; else ok "mixed tables -> quarantine"; fi +if drift_preserves_preflight_rows db H1 H2 "issues mail"; then no "mixed tables -> quarantine"; else ok "mixed tables -> quarantine"; fi # 5. diff probe failure on a table -> fail closed reset; stub_count_issues=0; STUB_FAIL_TABLE=mail -if gain_drift_is_additive_only db H1 H2 "issues mail"; then no "probe failure -> quarantine"; else ok "probe failure -> quarantine"; fi +if drift_preserves_preflight_rows db H1 H2 "issues mail"; then no "probe failure -> quarantine"; else ok "probe failure -> quarantine"; fi # 6. empty / non-numeric probe result -> fail closed reset; STUB_EMPTY_TABLE=issues -if gain_drift_is_additive_only db H1 H2 "issues"; then no "empty probe result -> quarantine"; else ok "empty probe result -> quarantine"; fi +if drift_preserves_preflight_rows db H1 H2 "issues"; then no "empty probe result -> quarantine"; else ok "empty probe result -> quarantine"; fi # 7. empty table list -> not a proof reset -if gain_drift_is_additive_only db H1 H2 ""; then no "empty table list -> quarantine"; else ok "empty table list -> quarantine"; fi +if drift_preserves_preflight_rows db H1 H2 ""; then no "empty table list -> quarantine"; else ok "empty table list -> quarantine"; fi # 8. missing from-head -> fail closed reset; stub_count_issues=0 -if gain_drift_is_additive_only db "" H2 "issues"; then no "missing from-head -> quarantine"; else ok "missing from-head -> quarantine"; fi +if drift_preserves_preflight_rows db "" H2 "issues"; then no "missing from-head -> quarantine"; else ok "missing from-head -> quarantine"; fi # 9. missing to-head -> fail closed reset; stub_count_issues=0 -if gain_drift_is_additive_only db H1 "" "issues"; then no "missing to-head -> quarantine"; else ok "missing to-head -> quarantine"; fi +if drift_preserves_preflight_rows db H1 "" "issues"; then no "missing to-head -> quarantine"; else ok "missing to-head -> quarantine"; fi # 10. invalid table name -> fail closed reset; stub_count_issues=0; STUB_INVALID_TABLE=issues -if gain_drift_is_additive_only db H1 H2 "issues"; then no "invalid table name -> quarantine"; else ok "invalid table name -> quarantine"; fi +if drift_preserves_preflight_rows db H1 H2 "issues"; then no "invalid table name -> quarantine"; else ok "invalid table name -> quarantine"; fi + +# 11. The proof must count REMOVED rows, not every non-added row. A concurrent +# in-place UPDATE (the lx production case: issues modified=1, removed=0) leaves +# every pre-flight row reachable, so it must not fail the proof. Asserting the +# query text keeps this from silently reverting to a diff_type <> 'added' +# count, which would classify an ordinary bd update as unprovable. +reset; stub_count_issues=0 +drift_preserves_preflight_rows db H1 H2 "issues" || true +if grep -q "diff_type = 'removed'" "$QUERY_LOG"; then + ok "drift proof counts removed rows only" +else + no "drift proof counts removed rows only (queries were: $(cat "$QUERY_LOG"))" +fi + +# --- diff_is_additive_only: the first-committed table keeps a stricter bar --- + +# 12. purely additive first-commit diff -> provable +reset; stub_count_issues=0 +if diff_is_additive_only db H1 H2 "issues"; then ok "additive-only first-commit table -> provable"; else no "additive-only first-commit table -> provable"; fi + +# 13. any non-added row on a first-committed table -> fail closed +reset; stub_count_issues=1 +if diff_is_additive_only db H1 H2 "issues"; then no "non-added row on first-commit table -> quarantine"; else ok "non-added row on first-commit table -> quarantine"; fi + +# 14. The two predicates must keep asking different questions. If this one ever +# narrows to removals, a table that exists in no pre-flatten commit could show +# modified rows and still read as proven. +reset; stub_count_issues=0 +diff_is_additive_only db H1 H2 "issues" || true +if grep -q "diff_type <> 'added'" "$QUERY_LOG"; then + ok "first-commit proof still counts every non-added row" +else + no "first-commit proof still counts every non-added row (queries were: $(cat "$QUERY_LOG"))" +fi printf '\n%d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ]