From a2c21b6c6d7562751bf1563ea9b7a591a213ad08 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 29 Jul 2026 09:07:10 +0200 Subject: [PATCH 1/5] feat(data delete): reap the ingestor's bookkeeping rows with the table (RFC-0003 I6, backend#1209) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dropping a table stranded its run-journal rows (tracebloc_ingest_runs) and pseudonymization-salt row (tracebloc_ingest_meta). Under per-ingestion tables (data-ingestors#408) every dataset is its own table, so every delete would leak one husk row of each kind, unbounded. Teardown now DELETEs both best-effort after the DROP — separately per bookkeeping table (either may be absent on clusters that never ran a journal-aware ingestor), never failing a teardown whose DROP succeeded (TeardownResult.BookkeepingCleaned reports it). plan.Table has passed ValidateTableName, so it cannot escape the quoted literal. Benefits legacy label tables identically. Co-Authored-By: Claude Fable 5 --- .../cli/testdata/golden/zz-all-strings.golden | 1 + internal/push/teardown.go | 28 ++++++ internal/push/teardown_test.go | 92 +++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index f2341e53..58217f2a 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -140,6 +140,7 @@ screen. %s/%d are runtime placeholders. "Ctrl-C to cancel" "Ctrl-C to stop watching — the run keeps going on the cluster" "DB failures" +"DELETE FROM `%s`.`%s` WHERE table_name='%s'" "DROP TABLE IF EXISTS `%s`.`%s`" "Datasets in %s (0)" "Datasets in %s — %d" diff --git a/internal/push/teardown.go b/internal/push/teardown.go index b917d6f6..edf6cae1 100644 --- a/internal/push/teardown.go +++ b/internal/push/teardown.go @@ -66,6 +66,11 @@ func PlanTeardown(table string) TeardownPlan { type TeardownResult struct { DroppedTable bool RemovedPaths []string + // BookkeepingCleaned reports whether the ingestor's bookkeeping rows + // for the table (run-journal + pseudonymization salt) were deleted + // alongside it. Best-effort: false on clusters whose ingestor never + // created those tables — the teardown itself still succeeds. + BookkeepingCleaned bool } // Teardown performs the in-cluster teardown described by plan: @@ -107,6 +112,29 @@ func Teardown(ctx context.Context, cs kubernetes.Interface, exec Executor, names } res.DroppedTable = true + // 1b. Best-effort bookkeeping cleanup (RFC-0003 I6 — tracebloc/backend#1209): + // the ingestor keeps one run-journal row per ingest and one + // pseudonymization-salt row per table; dropping the table alone + // strands them. Under per-ingestion tables (data-ingestors#408) + // every dataset is its own table, so every delete would leave one + // husk row of each kind — an unbounded slow leak. plan.Table passed + // ValidateTableName ([A-Za-z_][A-Za-z0-9_]*), so it cannot escape + // the single-quoted literal. Each DELETE runs separately and + // best-effort: either bookkeeping table may be absent on clusters + // that never ran a journal-aware ingestor, and these are metadata + // rows, not data — never fail a teardown whose DROP succeeded. + res.BookkeepingCleaned = true + for _, bookkeeping := range []string{ingestRunsTable, ingestMetaTable} { + cleanupSQL := fmt.Sprintf("DELETE FROM `%s`.`%s` WHERE table_name='%s'", + plan.Database, bookkeeping, plan.Table) + cleanupScript := fmt.Sprintf(`mysql -uroot -p"$MYSQL_ROOT_PASSWORD" -e '%s'`, cleanupSQL) + var cleanupStderr bytes.Buffer + if err := exec.Exec(ctx, namespace, mysqlPod, mysqlContainer, + []string{"sh", "-c", cleanupScript}, nil, nil, &cleanupStderr); err != nil { + res.BookkeepingCleaned = false + } + } + // 2. rm the PVC dirs from an ephemeral stage-identity pod (see the // doc note above + #259). The pod owns the staging files it // deletes, so this works on hostPath and CSI. diff --git a/internal/push/teardown_test.go b/internal/push/teardown_test.go index df514ed8..70aea6cf 100644 --- a/internal/push/teardown_test.go +++ b/internal/push/teardown_test.go @@ -6,6 +6,8 @@ import ( "strings" "testing" + "fmt" + "io" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -183,3 +185,93 @@ func TestCleanStaging_PodCreateFailureReturnsError(t *testing.T) { t.Errorf("rm ran (%v) despite the pod never being created", fe.gotCmd) } } + +// TestTeardown_CleansBookkeepingRows pins the RFC-0003 I6 half of teardown +// (tracebloc/backend#1209): after the DROP, the ingestor's run-journal and +// salt rows for the table are deleted best-effort — and a failure there +// never fails a teardown whose DROP already succeeded. +func TestTeardown_CleansBookkeepingRows(t *testing.T) { + newCS := func() *fake.Clientset { + cs := fake.NewClientset(&corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "mysql-0", Namespace: "tracebloc"}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "mysql"}}}, + Status: corev1.PodStatus{Phase: corev1.PodRunning}, + }) + readyOnNextGet(cs) + return cs + } + opts := PodSpecOptions{ + Namespace: "tracebloc", + PVCClaimName: "client-pvc", + PVCMountPath: "/data/shared", + Table: "ds_0f2ab1de3c444e558f66778899aabbcc", + } + plan := PlanTeardown("ds_0f2ab1de3c444e558f66778899aabbcc") + + t.Run("journal and salt rows are deleted for the dropped table", func(t *testing.T) { + rec := &recordingExecutor{} + res, err := Teardown(context.Background(), newCS(), rec, "tracebloc", plan, opts) + if err != nil { + t.Fatalf("Teardown: %v", err) + } + if !res.BookkeepingCleaned { + t.Error("BookkeepingCleaned = false, want true") + } + var journal, salt bool + for _, call := range rec.calls { + joined := strings.Join(call.cmd, " ") + if strings.Contains(joined, "DELETE FROM") && + strings.Contains(joined, ingestRunsTable) && + strings.Contains(joined, plan.Table) { + journal = true + } + if strings.Contains(joined, "DELETE FROM") && + strings.Contains(joined, ingestMetaTable) && + strings.Contains(joined, plan.Table) { + salt = true + } + } + if !journal { + t.Errorf("no DELETE against %s for %s observed", ingestRunsTable, plan.Table) + } + if !salt { + t.Errorf("no DELETE against %s for %s observed", ingestMetaTable, plan.Table) + } + }) + + t.Run("bookkeeping failure never fails the teardown", func(t *testing.T) { + rec := &recordingExecutor{failWhenCmdContains: "DELETE FROM"} + res, err := Teardown(context.Background(), newCS(), rec, "tracebloc", plan, opts) + if err != nil { + t.Fatalf("Teardown should tolerate bookkeeping failures, got: %v", err) + } + if !res.DroppedTable { + t.Error("DroppedTable = false, want true") + } + if res.BookkeepingCleaned { + t.Error("BookkeepingCleaned = true, want false when the DELETEs fail") + } + if len(res.RemovedPaths) == 0 { + t.Error("PVC rm did not run — bookkeeping failure must not short-circuit step 2") + } + }) +} + +// recordingExecutor records every Exec call and can fail selected ones. +type recordingExecutor struct { + calls []execCall + failWhenCmdContains string +} + +type execCall struct { + pod, container string + cmd []string +} + +func (r *recordingExecutor) Exec(ctx context.Context, namespace, pod, container string, cmd []string, stdin io.Reader, stdout, stderr io.Writer) error { + r.calls = append(r.calls, execCall{pod: pod, container: container, cmd: cmd}) + if r.failWhenCmdContains != "" && strings.Contains(strings.Join(cmd, " "), r.failWhenCmdContains) { + return fmt.Errorf("simulated bookkeeping failure") + } + return nil +} From 98c0a561762e144046a9b7d3e666b5b628a774f1 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 29 Jul 2026 10:01:58 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(teardown):=20feed=20bookkeeping=20SQL?= =?UTF-8?q?=20on=20stdin=20=E2=80=94=20shell=20quoting=20ate=20the=20strin?= =?UTF-8?q?g=20literal=20(Bugbot,=20High)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DELETEs embedded a single-quoted SQL literal inside a single-quoted sh -c string: the shell stripped the inner quotes, mysql saw an unquoted identifier, and the best-effort cleanup silently no-opped forever — exactly the leak this PR exists to stop. SQL now rides stdin (the runMySQLQuery pattern), sidestepping shell quoting entirely. The recording executor now captures stdin, and the test asserts the quoted literal arrives intact AND that no DELETE ever appears as a shell argument — pinning the whole bug class, not just this instance. Co-Authored-By: Claude Fable 5 --- internal/push/teardown.go | 9 ++++++-- internal/push/teardown_test.go | 40 +++++++++++++++++++++------------- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/internal/push/teardown.go b/internal/push/teardown.go index edf6cae1..9e0f6f63 100644 --- a/internal/push/teardown.go +++ b/internal/push/teardown.go @@ -123,14 +123,19 @@ func Teardown(ctx context.Context, cs kubernetes.Interface, exec Executor, names // best-effort: either bookkeeping table may be absent on clusters // that never ran a journal-aware ingestor, and these are metadata // rows, not data — never fail a teardown whose DROP succeeded. + // The SQL is fed on STDIN (the runMySQLQuery pattern), never through a + // shell -e argument: the string literal's single quotes would terminate + // a single-quoted shell string and mysql would see an unquoted + // identifier — the DELETEs would silently fail forever (Bugbot on the + // PR). Stdin sidesteps shell quoting entirely. res.BookkeepingCleaned = true for _, bookkeeping := range []string{ingestRunsTable, ingestMetaTable} { cleanupSQL := fmt.Sprintf("DELETE FROM `%s`.`%s` WHERE table_name='%s'", plan.Database, bookkeeping, plan.Table) - cleanupScript := fmt.Sprintf(`mysql -uroot -p"$MYSQL_ROOT_PASSWORD" -e '%s'`, cleanupSQL) var cleanupStderr bytes.Buffer if err := exec.Exec(ctx, namespace, mysqlPod, mysqlContainer, - []string{"sh", "-c", cleanupScript}, nil, nil, &cleanupStderr); err != nil { + []string{"sh", "-c", `mysql -uroot -p"$MYSQL_ROOT_PASSWORD"`}, + strings.NewReader(cleanupSQL), nil, &cleanupStderr); err != nil { res.BookkeepingCleaned = false } } diff --git a/internal/push/teardown_test.go b/internal/push/teardown_test.go index 70aea6cf..10a8854d 100644 --- a/internal/push/teardown_test.go +++ b/internal/push/teardown_test.go @@ -217,30 +217,34 @@ func TestTeardown_CleansBookkeepingRows(t *testing.T) { if !res.BookkeepingCleaned { t.Error("BookkeepingCleaned = false, want true") } + // The SQL must arrive on STDIN with its quoted literal intact — + // never through a shell -e argument, where the literal's single + // quotes would be eaten by the shell (Bugbot: the DELETEs would + // silently no-op forever). var journal, salt bool for _, call := range rec.calls { - joined := strings.Join(call.cmd, " ") - if strings.Contains(joined, "DELETE FROM") && - strings.Contains(joined, ingestRunsTable) && - strings.Contains(joined, plan.Table) { + if strings.Contains(strings.Join(call.cmd, " "), "DELETE FROM") { + t.Errorf("DELETE passed as a shell argument (%q) — must be fed on stdin", call.cmd) + } + stdin := string(call.stdin) + want := "WHERE table_name='" + plan.Table + "'" + if strings.Contains(stdin, "DELETE FROM") && strings.Contains(stdin, ingestRunsTable) && strings.Contains(stdin, want) { journal = true } - if strings.Contains(joined, "DELETE FROM") && - strings.Contains(joined, ingestMetaTable) && - strings.Contains(joined, plan.Table) { + if strings.Contains(stdin, "DELETE FROM") && strings.Contains(stdin, ingestMetaTable) && strings.Contains(stdin, want) { salt = true } } if !journal { - t.Errorf("no DELETE against %s for %s observed", ingestRunsTable, plan.Table) + t.Errorf("no stdin DELETE against %s with a quoted literal for %s observed", ingestRunsTable, plan.Table) } if !salt { - t.Errorf("no DELETE against %s for %s observed", ingestMetaTable, plan.Table) + t.Errorf("no stdin DELETE against %s with a quoted literal for %s observed", ingestMetaTable, plan.Table) } }) t.Run("bookkeeping failure never fails the teardown", func(t *testing.T) { - rec := &recordingExecutor{failWhenCmdContains: "DELETE FROM"} + rec := &recordingExecutor{failWhenStdinContains: "DELETE FROM"} res, err := Teardown(context.Background(), newCS(), rec, "tracebloc", plan, opts) if err != nil { t.Fatalf("Teardown should tolerate bookkeeping failures, got: %v", err) @@ -257,20 +261,26 @@ func TestTeardown_CleansBookkeepingRows(t *testing.T) { }) } -// recordingExecutor records every Exec call and can fail selected ones. +// recordingExecutor records every Exec call (command AND stdin) and can +// fail calls whose stdin matches a marker. type recordingExecutor struct { - calls []execCall - failWhenCmdContains string + calls []execCall + failWhenStdinContains string } type execCall struct { pod, container string cmd []string + stdin []byte } func (r *recordingExecutor) Exec(ctx context.Context, namespace, pod, container string, cmd []string, stdin io.Reader, stdout, stderr io.Writer) error { - r.calls = append(r.calls, execCall{pod: pod, container: container, cmd: cmd}) - if r.failWhenCmdContains != "" && strings.Contains(strings.Join(cmd, " "), r.failWhenCmdContains) { + var in []byte + if stdin != nil { + in, _ = io.ReadAll(stdin) + } + r.calls = append(r.calls, execCall{pod: pod, container: container, cmd: cmd, stdin: in}) + if r.failWhenStdinContains != "" && strings.Contains(string(in), r.failWhenStdinContains) { return fmt.Errorf("simulated bookkeeping failure") } return nil From 12605205ce7b998da90b37adbd4d26e595619196 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 29 Jul 2026 10:43:02 +0200 Subject: [PATCH 3/5] chore: goimports grouping in teardown_test (CI lint) Co-Authored-By: Claude Fable 5 --- internal/push/teardown_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/push/teardown_test.go b/internal/push/teardown_test.go index 10a8854d..1b2bc506 100644 --- a/internal/push/teardown_test.go +++ b/internal/push/teardown_test.go @@ -3,11 +3,11 @@ package push import ( "context" "errors" + "fmt" + "io" "strings" "testing" - "fmt" - "io" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" From 61967b773a135877b94eced5d47f95412e4e8eaa Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 29 Jul 2026 10:56:31 +0200 Subject: [PATCH 4/5] fix(teardown): surface bookkeeping failures + reuse runMySQLQuery + pin the column contract (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Observability: TeardownResult gains BookkeepingErrs (per-table failure with mysql stderr folded in via runMySQLQuery); data delete prints a warning on incomplete cleanup and --output-json gains bookkeeping_cleaned — schema drift is now diagnosable in the field instead of collapsing into a silent false. 2. Column contract pinned in a comment against data-ingestors database.py: both bookkeeping tables key by table_name (tracebloc_ingest_runs indexed, tracebloc_ingest_meta PK). 3. The inline stdin exec is gone — DELETEs ride runMySQLQuery; its error prose neutralized to 'running mysql query' (the 'querying datasets' wording lives on only in list.go's own exec, whose test asserts it). Co-Authored-By: Claude Fable 5 --- internal/cli/data_delete.go | 33 ++++++++++++------- .../cli/testdata/golden/zz-all-strings.golden | 3 ++ internal/push/list_detailed.go | 2 +- internal/push/teardown.go | 30 ++++++++++++----- 4 files changed, 47 insertions(+), 21 deletions(-) diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 7b1e6d38..c3f27c40 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -212,7 +212,7 @@ undone — re-ingesting the data is the only way back.`) p.Newline() p.Successf("Dry-run — nothing was deleted.") if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "dry-run", resolved.Namespace, release.ReleaseName, plan, nil) + writeDataDeleteJSON(a.JSONOut, "dry-run", resolved.Namespace, release.ReleaseName, plan, nil, true) jsonEmitted = true } return nil @@ -234,7 +234,7 @@ undone — re-ingesting the data is the only way back.`) // exit 0. One closure so the pair can't drift apart. declined := func() error { if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) + writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil, true) jsonEmitted = true } return cleanCancel(p, "nothing was deleted.") @@ -283,9 +283,15 @@ undone — re-ingesting the data is the only way back.`) p.Newline() p.Successf("Deleted %s.%s and %d PVC path(s).", plan.Database, plan.Table, len(res.RemovedPaths)) + if !res.BookkeepingCleaned { + // Best-effort cleanup failed — say so, or a schema-drift regression + // (a renamed keying column) is indistinguishable from a legacy + // cluster without the bookkeeping tables (review, Saqlain). + p.Warnf("Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s", strings.Join(res.BookkeepingErrs, "; ")) + } p.Infof("The dataset's catalog metadata is kept as a record on tracebloc, marked unavailable — never removed.") if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "deleted", resolved.Namespace, release.ReleaseName, plan, res.RemovedPaths) + writeDataDeleteJSON(a.JSONOut, "deleted", resolved.Namespace, release.ReleaseName, plan, res.RemovedPaths, res.BookkeepingCleaned) jsonEmitted = true } return nil @@ -302,12 +308,16 @@ type dataDeleteJSON struct { Table string `json:"table"` // the REAL (case-resolved) spelling, not the raw argument PVCPaths []string `json:"pvc_paths"` RemovedPaths []string `json:"removed_paths"` + // BookkeepingCleaned mirrors push.TeardownResult: whether the + // run-journal/salt rows were removed with the table. Always true for + // dry-run/declined (nothing was attempted). + BookkeepingCleaned bool `json:"bookkeeping_cleaned"` } // writeDataDeleteJSON serializes the delete result to w (stdout in // --output-json mode). Marshal errors are dropped: marshaling our own // struct can't fail in practice, and the exit code remains the contract. -func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan push.TeardownPlan, removed []string) { +func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan push.TeardownPlan, removed []string, bookkeepingCleaned bool) { pvcPaths := plan.PVCPaths if pvcPaths == nil { pvcPaths = []string{} // emit [] not null @@ -316,13 +326,14 @@ func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan pu removed = []string{} // emit [] not null } res := dataDeleteJSON{ - Status: status, - Namespace: namespace, - Release: release, - Database: plan.Database, - Table: plan.Table, - PVCPaths: pvcPaths, - RemovedPaths: removed, + Status: status, + Namespace: namespace, + Release: release, + Database: plan.Database, + Table: plan.Table, + PVCPaths: pvcPaths, + RemovedPaths: removed, + BookkeepingCleaned: bookkeepingCleaned, } b, err := json.MarshalIndent(res, "", " ") if err != nil { diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 58217f2a..d41c2f39 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -60,6 +60,7 @@ screen. %s/%d are runtime placeholders. "%s — %s" "%s, … and %d more" "%s/%s" +"%s: %v" "%s: %w" "%s=%s,%s=%s" "%v (policy: %v)" @@ -96,6 +97,7 @@ screen. %s/%d are runtime placeholders. "Already signed out." "Applies to your next training run; a run already going keeps its size." "Ask one of these admins (or ask them to grant you access)" +"Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s" "CPU cores for one run (1–%d)" "CSV %s has no columns" "Can't reach tracebloc from here." @@ -554,6 +556,7 @@ screen. %s/%d are runtime placeholders. "resource env" "restarted ≥%d times — check logs: %v" "root" +"running mysql query: %w%s" "scanning the cluster for tracebloc clients: %w" "schema" "schema entry %q must be col:TYPE (e.g. age:INT,price:FLOAT)" diff --git a/internal/push/list_detailed.go b/internal/push/list_detailed.go index 2e0cd4f6..1ce5811e 100644 --- a/internal/push/list_detailed.go +++ b/internal/push/list_detailed.go @@ -288,7 +288,7 @@ func runMySQLQuery(ctx context.Context, exec Executor, namespace, pod, container script := `mysql -uroot -p"$MYSQL_ROOT_PASSWORD" -N` if err := exec.Exec(ctx, namespace, pod, container, []string{"sh", "-c", script}, strings.NewReader(query), &stdout, &stderr); err != nil { - return "", fmt.Errorf("querying datasets: %w%s", err, stderrSuffix(&stderr)) + return "", fmt.Errorf("running mysql query: %w%s", err, stderrSuffix(&stderr)) } return stdout.String(), nil } diff --git a/internal/push/teardown.go b/internal/push/teardown.go index 9e0f6f63..e81487f9 100644 --- a/internal/push/teardown.go +++ b/internal/push/teardown.go @@ -71,6 +71,13 @@ type TeardownResult struct { // alongside it. Best-effort: false on clusters whose ingestor never // created those tables — the teardown itself still succeeds. BookkeepingCleaned bool + // BookkeepingErrs carries the per-table failure detail (which + // bookkeeping table, mysql's stderr folded into the error) so callers + // can SURFACE it: a silent false is indistinguishable from the + // schema-drift regression this cleanup exists to prevent — e.g. a + // renamed keying column would otherwise no-op invisibly, reopening + // the husk-row leak (review, Saqlain). + BookkeepingErrs []string } // Teardown performs the in-cluster teardown described by plan: @@ -123,20 +130,25 @@ func Teardown(ctx context.Context, cs kubernetes.Interface, exec Executor, names // best-effort: either bookkeeping table may be absent on clusters // that never ran a journal-aware ingestor, and these are metadata // rows, not data — never fail a teardown whose DROP succeeded. - // The SQL is fed on STDIN (the runMySQLQuery pattern), never through a - // shell -e argument: the string literal's single quotes would terminate - // a single-quoted shell string and mysql would see an unquoted - // identifier — the DELETEs would silently fail forever (Bugbot on the - // PR). Stdin sidesteps shell quoting entirely. + // The SQL rides runMySQLQuery — stdin, never a shell -e argument: the + // string literal's single quotes would terminate a single-quoted shell + // string and mysql would see an unquoted identifier, silently no-oping + // the DELETEs forever (Bugbot on the PR). Column contract, pinned + // against data-ingestors tracebloc_ingestor/database.py: BOTH + // bookkeeping tables key these rows by `table_name` — + // RUNS_TABLE tracebloc_ingest_runs (ingestor_id PK, table_name + // indexed via ix_tracebloc_ingest_runs_table) + // SALT_TABLE tracebloc_ingest_meta (table_name PK, salt) + // Each DELETE stays a separate best-effort call: batched on one stdin, + // a missing first table would abort the second (mysql stops on error). res.BookkeepingCleaned = true for _, bookkeeping := range []string{ingestRunsTable, ingestMetaTable} { cleanupSQL := fmt.Sprintf("DELETE FROM `%s`.`%s` WHERE table_name='%s'", plan.Database, bookkeeping, plan.Table) - var cleanupStderr bytes.Buffer - if err := exec.Exec(ctx, namespace, mysqlPod, mysqlContainer, - []string{"sh", "-c", `mysql -uroot -p"$MYSQL_ROOT_PASSWORD"`}, - strings.NewReader(cleanupSQL), nil, &cleanupStderr); err != nil { + if _, err := runMySQLQuery(ctx, exec, namespace, mysqlPod, mysqlContainer, cleanupSQL); err != nil { res.BookkeepingCleaned = false + res.BookkeepingErrs = append(res.BookkeepingErrs, + fmt.Sprintf("%s: %v", bookkeeping, err)) } } From 79ee13c310397faf3c4eeab47505458ba38abe2b Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 29 Jul 2026 11:18:02 +0200 Subject: [PATCH 5/5] fix(overwrite): surface bookkeeping-cleanup failures on the ingest pre-clean too (Bugbot) + JSON cosmetic (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit data ingest --overwrite runs the identical teardown but discarded the result — a bookkeeping failure printed unconditional success, hiding on this path the exact schema-drift signal data delete now surfaces. The overwrite pre-clean warns the same way. Also: dry-run/declined emit bookkeeping_cleaned=false (nothing was attempted — a strict consumer must never read 'cleanup happened' out of a run that deleted nothing). Co-Authored-By: Claude Fable 5 --- internal/cli/data.go | 10 +++++++++- internal/cli/data_delete.go | 9 +++++---- internal/cli/testdata/golden/zz-all-strings.golden | 1 + 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/internal/cli/data.go b/internal/cli/data.go index 98421b86..cc7f8b86 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "strings" "github.com/spf13/cobra" @@ -165,7 +166,7 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr // DROP/rm and then "succeed". plan := push.PlanTeardown(existingTable) rmSpin := a.Printer.Spinner(fmt.Sprintf("Removing the existing %q first", existingTable), "") - _, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ + tres, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ Namespace: resolved.Namespace, PVCClaimName: pvc.ClaimName, PVCMountPath: pvc.MountPath, @@ -185,6 +186,13 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr "first, then re-run this ingest. Nothing new was staged. (%w)", existingTable, existingTable, terr)} } + if !tres.BookkeepingCleaned { + // Same surfacing `data delete` does (Bugbot on the PR): the + // overwrite pre-clean runs the identical teardown, and a silent + // bookkeeping failure here would hide the same schema-drift + // regression on this path. + a.Printer.Warnf("Bookkeeping cleanup incomplete — the old table is gone, but its run-journal/salt rows may remain: %s", strings.Join(tres.BookkeepingErrs, "; ")) + } a.Printer.Successf("Removed the old %q — ingesting the new data.", existingTable) } diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index c3f27c40..99614916 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -212,7 +212,7 @@ undone — re-ingesting the data is the only way back.`) p.Newline() p.Successf("Dry-run — nothing was deleted.") if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "dry-run", resolved.Namespace, release.ReleaseName, plan, nil, true) + writeDataDeleteJSON(a.JSONOut, "dry-run", resolved.Namespace, release.ReleaseName, plan, nil, false) jsonEmitted = true } return nil @@ -234,7 +234,7 @@ undone — re-ingesting the data is the only way back.`) // exit 0. One closure so the pair can't drift apart. declined := func() error { if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil, true) + writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil, false) jsonEmitted = true } return cleanCancel(p, "nothing was deleted.") @@ -309,8 +309,9 @@ type dataDeleteJSON struct { PVCPaths []string `json:"pvc_paths"` RemovedPaths []string `json:"removed_paths"` // BookkeepingCleaned mirrors push.TeardownResult: whether the - // run-journal/salt rows were removed with the table. Always true for - // dry-run/declined (nothing was attempted). + // run-journal/salt rows were removed with the table. Always false for + // dry-run/declined — nothing was attempted, and a strict consumer must + // never read "cleanup happened" out of a run that deleted nothing. BookkeepingCleaned bool `json:"bookkeeping_cleaned"` } diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index d41c2f39..46dd8d40 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -97,6 +97,7 @@ screen. %s/%d are runtime placeholders. "Already signed out." "Applies to your next training run; a run already going keeps its size." "Ask one of these admins (or ask them to grant you access)" +"Bookkeeping cleanup incomplete — the old table is gone, but its run-journal/salt rows may remain: %s" "Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s" "CPU cores for one run (1–%d)" "CSV %s has no columns"