fix(versioning): stop filing every operational failure under the migration race - #42709
Conversation
…ation race The capture path swallows OperationalError/ProgrammingError to survive the pre-migration window where the versioning tables don't exist yet. But those classes also cover deadlocks, lock timeouts, and dropped connections — and the class-based swallow filed all of them under "table missing": no log line, no capture-error metric, while the user's save reported success and its version history quietly didn't exist. That defeats the module's own posture that capture regressions are alertable rather than log-grep-only. Three sites, three consequences: - changes/listener.py: the whole save's change records dropped silently - baseline/collection.py: shadow_row_count returns None, so the caller skips baseline capture for the flush - queries.py (read path): every affected save renders an empty change list Add is_missing_table_error(), which verifies the specific condition — pgcode/sqlstate 42P01 on PostgreSQL, errno 1146 on MySQL, "no such table" on SQLite — instead of inferring it from the exception class. The genuine migration race stays exactly as quiet as before; everything else now logs and, on the persist path, increments the existing capture-error counter. The predicate is deliberately narrow: an ambiguous error gets logged, not swallowed — over-reporting is recoverable, a silent capture drop is not. The transient-failure test fails against the previous listener (no log, no metric); the missing-table test passes both before and after, pinning that the benign case's behaviour is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dosubot
Bot
added
the
logging
Code Review Agent Run #c94b63Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42709 +/- ##
==========================================
- Coverage 65.43% 65.42% -0.01%
==========================================
Files 2810 2812 +2
Lines 159528 159555 +27
Branches 36410 36415 +5
==========================================
+ Hits 104380 104392 +12
- Misses 53102 53116 +14
- Partials 2046 2047 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… the swallow Follow-ups from a clean-code + tidy-first review pass over this PR: - The two except arms in _persist_buffered_records carried byte-identical log+metric bodies; collapsed into one arm with the missing-table early return, so the message and the metric can't drift apart. - The baseline probe logged its losses but never counted them — a blind quadrant in the alerting surface this PR exists to create. The metric helper moves to superset/versioning/metrics.py, shared by the change-record listener and the baseline probe, which now emits shadow_count errors. - The read path's branch normalized to the same polarity as its siblings, and its docstring now describes both return paths instead of only the missing-table one. - The two non-listener classify-then-log branches gained their own tests (missing table silent / locked database logged+counted), so deleting a log line or metric at any of the three sites now fails a test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Nice, this closes a real gap. Swallowing the whole OperationalError/ProgrammingError class for the migration race meant a deadlock or dropped connection would silently look identical to "table not there yet" and the save's version history would just quietly vanish. is_missing_table_error() narrowing to the actual driver codes (with SQLite's message fallback) is the right fix, and the driver-shape tests covering pg/mysql/sqlite plus the negative cases (deadlock, lock timeout, connection drop) are exactly what I'd want to see backing this. LGTM!
Code Review Agent Run #696655Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
The versioning capture path swallows
OperationalError/ProgrammingErrorto survive the pre-migration window where the versioning tables don't exist yet. But those exception classes also cover deadlocks, lock timeouts, and dropped connections — and the class-based swallow filed all of them under "table missing": no log line, no capture-error metric, while the user's save reported success and its version history quietly didn't exist. That defeats the module's own documented posture that capture regressions should be "alertable rather than log-grep-only."Three sites, three consequences:
changes/listener.py— the whole save's change records dropped silentlybaseline/collection.py—shadow_row_countreturnsNone, so the caller skips baseline capture for the flushqueries.py(read path) — every affected save renders an empty change listThis PR adds
is_missing_table_error(), which verifies the specific condition —pgcode/sqlstate42P01on PostgreSQL, errno1146on MySQL,"no such table"on SQLite — instead of inferring it from the exception class. The genuine migration race stays exactly as quiet as before; everything else now logs and, on the persist path, increments the existingsuperset.versioning.capture.*counter. The predicate is deliberately narrow: an ambiguous error gets logged, not swallowed — over-reporting is recoverable, a silent capture drop is not.Disclosure: this change was developed with AI assistance (Claude), on behalf of and reviewed by @mikebridge.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — observability-only change; no user-facing behaviour.
TESTING INSTRUCTIONS
pytest tests/unit_tests/versioning/test_db_errors.py— 9 tests pinning the driver shapes per supported metadata DB (psycopg2pgcode, psycopg3sqlstate, MySQL errno, SQLite message), including the negative cases (deadlock, lock-wait timeout, locked database, connection drop).pytest tests/unit_tests/versioning/test_listener.py— the newtest_transient_persist_failure_is_logged_and_countedfails against the previous listener (no log, no metric);test_missing_table_stays_silent_during_persistpasses before and after, pinning that the benign case's behaviour is unchanged.ADDITIONAL INFORMATION