feat(data delete): reap ingestor bookkeeping rows with the table (RFC-0003 I6, backend#1209)

[LukasWodka]

Summary

The tb data delete half of I6 (RFC-0003 D17, tracebloc/backend#1209): teardown now reaps the ingestor's bookkeeping rows together with the table.

• After the DROP TABLE, two best-effort DELETEs remove the table's rows from tracebloc_ingest_runs (run journal) and tracebloc_ingest_meta (pseudonymization salts) — the rows that previously outlived every deleted dataset. Under per-ingestion tables (feat(ingestor): per-ingestion immutable tables ds_<uuid4().hex> behind PER_INGESTION_TABLES (backend#1205) data-ingestors#408) each dataset is its own table, so each delete would otherwise leak one husk row of each kind, unbounded; legacy label tables get the same cleanup for free.
• Best-effort by design: each DELETE runs separately (either bookkeeping table may be absent on clusters that never ran a journal-aware ingestor), and a failure never fails a teardown whose DROP already succeeded — TeardownResult.BookkeepingCleaned records the outcome, no new user-facing output (no copy churn beyond the string catalog's one-line addition for the SQL literal).
• Injection-safe by the same argument as the existing DROP: plan.Table has passed ValidateTableName ([A-Za-z_][A-Za-z0-9_]*), so it cannot escape the quoted literal — ds_<uuid4().hex> names pass that grammar natively.

The rest of I6 (context)

Investigation results recorded on tracebloc/backend#1209: the backend needs no change — deletion is table-keyed and edge-driven, the hourly heartbeat enumerates tables via information_schema (so ds_ tables are reported automatically), and the existing ingestor-id tombstone cascade already gives correct shared-handle semantics (dependents of a dropped table get tombstoned — no refcounting needed). The remaining half, the dead-run husk-table sweep, lands in the jobs-manager (client-runtime PR, next).

Test plan

• New TestTeardown_CleansBookkeepingRows: both DELETEs observed against the right tables for a ds_<hex> name; a failing DELETE leaves err=nil, DroppedTable=true, PVC rm still runs, BookkeepingCleaned=false.
• Existing teardown tests unchanged (the rm remains the last exec). Full go test ./... green; gofmt/go vet clean; string catalog regenerated (+1 line, the SQL literal).

Epic: tracebloc/backend#1151 · Ticket: tracebloc/backend#1209 · Ingestor half: tracebloc/data-ingestors#408

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes the destructive teardown path and in-cluster MySQL deletes, but cleanup is best-effort, table names are validated, and DROP/PVC removal behavior is unchanged on bookkeeping failure.

Overview
Dataset teardown now reaps ingestor bookkeeping after DROP TABLE: best-effort DELETEs remove rows for the dataset from tracebloc_ingest_runs (run journal) and tracebloc_ingest_meta (pseudonymization salts), keyed by table_name. SQL is sent through runMySQLQuery on stdin so shell quoting cannot silently no-op the deletes.

TeardownResult gains BookkeepingCleaned and BookkeepingErrs; bookkeeping failures do not fail teardown once the drop succeeded. tracebloc data delete and ingest --overwrite pre-clean warn when cleanup is incomplete, and --output-json for delete adds bookkeeping_cleaned (always false for dry-run/declined).

Tests cover both DELETEs, stdin delivery, and that PVC removal still runs when bookkeeping fails; the shared MySQL exec error string is generalized to "running mysql query".

^{Reviewed by Cursor Bugbot for commit 79ee13c. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

bugbot run

[LukasWodka]

👋 Heads-up — Code review queue is at 48 / 30

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

• averaging-service#220 — ci: code-quality caller (advisory) · author: @LukasWodka · no reviewer assigned
• backend#1201 — docs(rfc): Data spaces & federated dataset composition (draft) · author: @LukasWodka · reviewer: @saadqbal
• backend#1217 — docs(rfc): Dataset integrity & score reproducibility (draft) · author: @LukasWodka · reviewer: @saadqbal
• backend#1261 — RFC-BACKEND-0008: board & process reset (decision RFC — for markup) · author: @LukasWodka · reviewer: @saadqbal
• backend#1284 — docs(deploy): backend deploy runbook skeleton (#1263) · author: @LukasWodka · no reviewer assigned
• backend#1293 — fix(heartbeat): tolerate unknown experiment_key in training_info (#1287) · author: @divyasinghds · no reviewer assigned
• backend#1297 — feat(db): gate MSSQL-only ODBC driver per engine + add psycopg (#1164) · author: @saadqbal · reviewer: @aptracebloc
• backend#1298 — fix(guardrails): reuse existing dimensional anomaly monitor (#1160) · author: @saadqbal · reviewer: @saqlainsyed007, @aptracebloc
• backend#1300 — feat(datasets): physical-table/label split + dataset→table resolution in the edge payload (#1206) · author: @LukasWodka · no reviewer assigned
• backend#1309 — ci: code-quality caller (advisory) · author: @LukasWodka · no reviewer assigned

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

[LukasWodka]

Bugbot's finding (both threads — same bug) is correct and fixed in 98c0a56: the single-quoted SQL literal inside the single-quoted sh -c string was shell-stripped, so mysql received an unquoted identifier and the best-effort DELETEs silently no-opped — on a real cluster the cleanup never worked, and the fake-executor tests couldn't see it. The SQL now rides stdin (the runMySQLQuery pattern), which sidesteps shell quoting entirely. The tests now pin the bug class: the recording executor captures stdin, asserts the quoted literal arrives byte-intact, and fails if any DELETE ever appears as a shell argument again. Full suite green, no catalog churn.

bugbot run

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 1260520. Configure here.}

[saqlainsyed007]

Review

Well-scoped change (+136/-0), isolated to the teardown path with new regression tests. The core reasoning holds up:

• Injection safety is sound — plan.Table passed ValidateTableName ([A-Za-z_][A-Za-z0-9_]*) and plan.Database is a constant, so nothing can escape the single-quoted literal.
• stdin vs -e is the right fix — the WHERE table_name='…' literal would break the existing -e '%s' single-quoted shell wrapper; feeding SQL on stdin sidesteps shell quoting, and the test pins it by asserting DELETE FROM never appears in the command args.
• Separate best-effort DELETEs are justified — batching both into one stdin means the first missing table aborts the second (mysql stops on error without --force); running them independently is correct and the comment explains why.

Findings

1. Observability gap — the leak this PR fixes could silently return (most important).
cleanupStderr is captured but never read or logged, and no production caller reads BookkeepingCleaned (only the tests reference it). So on failure there is zero signal anywhere. A benign "table absent on a legacy cluster" and a real regression — e.g. a future ingestor renames the keying column and every DELETE silently no-ops — both collapse to the same silent false. The second case reintroduces exactly the unbounded husk-row leak this PR closes, invisibly. Suggest logging the err/cleanupStderr at warn/debug on the failure branch so schema drift is at least diagnosable.

2. Cross-repo column assumption is only half-verified.
tracebloc_ingest_runs having a table_name column is corroborated in-repo (list_detailed_test.go shows its columns as ingestor_id,table_name,registered). But tracebloc_ingest_meta (the salt table) has no schema evidence in this repo — the WHERE table_name=… key is assumed. If the salt table keys by a different column, the DELETE silently no-ops (best-effort hides it, per #1). Worth confirming against data-ingestors database.py SALT_TABLE and pinning the column name in a comment beside the constants.

3. Reuse the existing runMySQLQuery helper.
The comment cites "the runMySQLQuery pattern," but the code reimplements the same stdin-exec inline rather than calling runMySQLQuery (internal/push/list_detailed.go), which already encapsulates the mysql -uroot -p"$…" + stdin + stderr-into-error invariant. Reusing it (discarding the returned stdout) keeps the "SQL over stdin, never -e" rule in one place. The only reason to inline is the per-call cleanupStderr buffer — which #1 suggests you'd want to act on anyway.

4. Minor — import grouping in teardown_test.go.
"fmt" and "io" (stdlib) were added to the third-party import block above corev1. It's gofmt-clean but goimports would relocate them to the stdlib group.

5. Minor — test-fake proliferation.
recordingExecutor is the third stdin-capturing executor fake in this package (alongside fakeExecutor and the anonymous one in list_detailed_test.go). Consolidating would reduce drift.

Risk

Low-to-medium as labeled — the DROP is unchanged and cleanup is strictly additive/best-effort. The real residual risk is silent: if the column assumption is wrong for the salt table, teardown keeps reporting success while the leak persists undetected. Fixing #1 (log on failure) is the highest-value change and makes #2 self-diagnosing in the field.

[LukasWodka]

All five addressed (latest commit):

1. Observability (your "most important" — agreed): TeardownResult gains BookkeepingErrs — per-table failure detail with mysql's stderr folded in (free, via Promote kubeconfig flags (--kubeconfig/--context/--namespace) to persistent on root #3). tb data delete now warns on incomplete cleanup ("the table is gone, but its run-journal/salt rows may remain: …") and --output-json gains bookkeeping_cleaned. A schema-drift regression is now diagnosable in the field instead of collapsing into the same silent false as a legacy cluster; the failure-path test asserts the detail is carried.
2. Salt-table column confirmed and pinned. From data-ingestors tracebloc_ingestor/database.py: SALT_TABLE = "tracebloc_ingest_meta" with table_name VARCHAR(64) NOT NULL PRIMARY KEY, salt CHAR(64) — table_name is the key (and RUNS_TABLE indexes it via ix_tracebloc_ingest_runs_table). Both DDLs are now cited in the comment beside the loop.
3. runMySQLQuery reused — the inline stdin exec is gone. Its error prose was neutralized to "running mysql query" so it reads correctly for both callers (data list's own exec in list.go keeps the "querying datasets" wording its test asserts). Your point about the per-call stderr buffer resolved itself: the helper folds stderr into the error, which is exactly what Phase 1: embed ingest.v1.json + tracebloc ingest validate #1 wants to surface.
4. Import grouping — this raced my lint fix: 1260520 (pushed ~40 min before your review) already merged fmt/io into the stdlib group; CI's goimports -local gate is green since.
5. Test-fake consolidation — agreed it's the third stdin-capturing fake; kept out of this PR for scope, happy to do a small internal/push test-fake unification as a follow-up chore if you want it ticketed.

Full suite green, lint clean, catalog +3 literals (the warn line, the neutral helper prose, the %s: %v join).

bugbot run

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 61967b7. Configure here.}

[saqlainsyed007]

Re-review (commit 61967b7)

All five points from the earlier review are addressed. (Note: I couldn't run go build/vet/test in my environment — the Go toolchain isn't installed there — so this is static analysis; I did confirm strings is already imported in data_delete.go.)

1 — Observability (the one that mattered). TeardownResult now carries BookkeepingErrs []string with per-table detail (mysql's stderr folded in), and data_delete.go surfaces it: a Warnf on failure ("the table is gone, but its run-journal/salt rows may remain: …") plus a bookkeeping_cleaned field in the JSON output. A schema-drift regression is now loud instead of silently reopening the husk-row leak. 👍

2 — Column contract. Pinned in the comment against data-ingestors database.py: RUNS_TABLE (ingestor_id PK, table_name indexed) and SALT_TABLE (table_name PK). Both keyed by table_name, so the shared WHERE table_name=… is correct for both.

3 — Helper reuse. Now calls runMySQLQuery(...) instead of the inline exec, and generalized that helper's error string from "querying datasets" → "running mysql query" since it's shared now (golden updated). -N is a harmless no-op for a DELETE.

4 — Imports. fmt/io moved into the stdlib group.

Delta looks correct

• Failure invariant holds: BookkeepingCleaned=false and the BookkeepingErrs append happen together in the loop, so the Warnf's strings.Join is never empty when it fires.
• Test coverage still valid against the runMySQLQuery path — DELETE rides stdin, cmd stays ["sh","-c","mysql … -N"], so the "not a shell argument" assertion still holds.

One tiny, non-blocking note

Dry-run/declined pass bookkeeping_cleaned: true (documented as "nothing was attempted"). Consistent with the existing []-not-null conventions and status disambiguates it, but a strict JSON consumer could read true as "cleanup happened." Emitting false for those statuses would be airtight — purely cosmetic, not worth blocking.

LGTM from my side.

[LukasWodka]

Both follow-ups in 79ee13c:

• Bugbot's overwrite gap (correct): data ingest --overwrite runs the identical teardown but discarded the result — the pre-clean now surfaces the same warning data delete does, so the schema-drift signal can't hide on either teardown path. (The "semantics match" comment in data.go holds again.)
• @saqlainsyed007's cosmetic note, taken: dry-run/declined now emit bookkeeping_cleaned: false — nothing was attempted, and a strict consumer should never read "cleanup happened" out of a run that deleted nothing; doc comment updated to say exactly that.

Suite green, lint clean, catalog +1 (the overwrite warn line). Thanks for the LGTM — nothing further queued on this branch.

bugbot run