fix(ingest): reconcile orphan rows from hard-killed runs before retry ingest#369
Open
LukasWodka wants to merge 1 commit into
Open
fix(ingest): reconcile orphan rows from hard-killed runs before retry ingest#369LukasWodka wants to merge 1 commit into
LukasWodka wants to merge 1 commit into
Conversation
… ingest The #227 compensating delete only runs on a CAUGHT failure. An OOMKill / SIGKILL mid-ingest bypasses it, leaving the dead run's rows in MySQL with its dataset never registered - and the k8s Job retry then re-ingests the same source next to them under fresh data_ids, duplicating every row. Add a cluster-local run journal (tracebloc_ingest_runs, lazily created like the #225 salt store) recording each run's lifecycle: - record_ingest_started: journaled before the run's first row insert - mark_ingest_registered: right after send_ingest_summary returns - reclaim_dead_run_rows: at ingest start (under the table lock), delete rows whose ingestor_id was journaled as started but never registered Rows are reclaimed ONLY when the journal witnessed the run start and never saw it register, so registered runs and pre-journal (legacy) rows are never touched. The reclaim pass is idempotent, excludes the current run, and a reconciliation failure degrades to today's status quo instead of blocking the ingest. Ref tracebloc/backend#1028 (item 2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
👋 Heads-up — Code review queue is at 52 / 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):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An OOMKill/SIGKILL mid-ingest bypasses the #227 compensating delete (it only runs on a caught failure), so the dead run's rows stay in MySQL while its dataset was never registered — and the k8s Job retry then re-ingests the same source next to them under fresh
data_ids, duplicating every row. This adds a cluster-local run journal and a reconcile-on-start pass: every ingest first reclaims rows left by prior runs that were journaled as started but never registered, so a retry converges instead of duplicating.Related
Ref tracebloc/backend#1028 (item 2)
Type of change
Design
"Registered" exists only in the central backend (
send_ingest_summary, no lookup endpoint), so the journal lives in the same cluster MySQL the rows land in — atracebloc_ingest_runstable created lazily like the #225 salt store (no migration step):record_ingest_started— journaled before the run's first row insert, so a hard kill at any later point leaves a started-but-unregistered entry behind (INSERT IGNORE, idempotent).mark_ingest_registered— right aftersend_ingest_summaryreturns, at the same point that flips the Atomicity gap: committed MySQL rows can outlive a failed dataset registration (from backend#772 P0.2) #227dataset_registeredguard.reclaim_dead_run_rows— at ingest start (aftercreate_table, under the existing table lock): deletes rows whoseingestor_idwas journaled as started and never registered, via the existing Atomicity gap: committed MySQL rows can outlive a failed dataset registration (from backend#772 P0.2) #227delete_by_ingestor_id, with a WARNING naming the reclaimed runs and row counts.Why it's safe:
ingestor_id). Amark_ingest_registeredfailure after successful registration is likewise swallowed with CRITICAL — raising would fail a healthy run and hand the retry a journal entry telling it to purge a registered dataset's rows.send_ingest_summaryreturning and the one-row journal UPDATE committing → the retry reclaims a backend-registered dataset's rows and re-registers under a new id): strictly narrower than the Atomicity gap: committed MySQL rows can outlive a failed dataset registration (from backend#772 P0.2) #227 window already documented inbase.pyfor the caught-failure path, with the same recovery (the retry re-ingests from source).Known limitation (called out, not hidden): orphans left by hard kills before this ships have no journal entry and are deliberately not reclaimed — locally they are indistinguishable from legacy registered datasets, and constraint one is never deleting registered rows. They remain sweepable by
ingestor_idas before. Also note the reclaim runs when the next ingest on that table actually starts; on PVC-backed clusters a hard-killed run's stale table lock can delay that retry (existing 12h stale-reclaim behavior, unchanged here).Test plan
pytest --cov=tracebloc_ingestor --cov-fail-under=95(mirrorstests.yml, Python 3.11): 1626 passed, 1 xfailed, coverage 96.09% — all new lines covered.docker compose -f e2e/docker-compose.yml up -d+pytest e2e/ -vagainst real MySQL 8 (mirrorse2e.yml): 47 passed, including 4 new tests:run.main()on the bundled tabular template: 8 rows, not 8+3);create_table→ reclaim → journal start → first insert; mark-registered only after a successful summary) and the failure isolation (reclaim/mark failures never fail a healthy run,#227delete still fires on caught failures).Deployment notes
No migration: the journal table is created lazily on first use, and the first deploy sees an empty journal (reconcile no-ops). No config/env changes. The two bookkeeping table names are now rejected as dataset table names.
Checklist
🤖 Generated with Claude Code
Note
Medium Risk
Deletes rows by
ingestor_idwhen the journal says started-but-unregistered; mis-journaling or the narrow post-registration window could affect live data, though registered and pre-journal rows are explicitly excluded.Overview
Fixes duplicate rows on k8s Job retry when a prior ingest was hard-killed (OOM/SIGKILL) after inserting into MySQL but before backend registration — a path that bypasses the existing #227 compensating delete.
Adds a cluster-local
tracebloc_ingest_runsjournal (lazy DDL, like the salt store) withrecord_ingest_started,mark_ingest_registered, andreclaim_dead_run_rows.BaseIngestor.ingestnow runs reconcile → journal start → inserts, then marks registered after a successfulsend_ingest_summary. Reclaim only targets journaled started + never registeredingestor_ids (join with data rows); legacy rows without journal entries and registered runs are excluded.create_tablealso rejectstracebloc_ingest_runsas a dataset name.Failure isolation: reconcile errors are logged CRITICAL and ingest continues; post-registration journal failures are swallowed so a registered dataset is not failed or deleted. Unit and MySQL e2e tests cover ordering, safety, idempotency, and full-engine retry convergence.
Reviewed by Cursor Bugbot for commit ab4481c. Bugbot is set up for automated code reviews on this repo. Configure here.