Repository navigation
Conversation
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| connection.execute( | ||
| _DATABASES.update() | ||
| .where(_DATABASES.c.id == database_id) | ||
| .values(extra=json.dumps(extra)) |
There was a problem hiding this comment.
Suggestion: A concurrent edit to dbs.extra after the SELECT can be overwritten by this stale snapshot, losing the newer configuration.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Race condition
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts.py
**Line:** 77:80
**Comment:**
*Race Condition: A concurrent edit to `dbs.extra` after the SELECT can be overwritten by this stale snapshot, losing the newer configuration.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b0c8ee0.
The update now matches both the database ID and the originally selected extra value. If a concurrent edit changed extra, the update affects no row and the migration raises an error instead of overwriting the newer configuration.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of ebeb64e.
The update now matches both the database ID and the originally selected extra value. If a concurrent edit changed extra, the update affects no row and the migration raises an error instead of overwriting the newer configuration.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 2cbe591.
The update now matches both the database ID and the originally selected extra value. If a concurrent edit changed extra, the update affects no row and the migration raises an error instead of overwriting the newer configuration.
If that's not right, unresolve this thread and CodeAnt will leave it open.
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45790 +/- ##
==========================================
- Coverage 84.57% 84.56% -0.01%
==========================================
Files 3069 3068 -1
Lines 205227 205106 -121
Branches 48222 48180 -42
==========================================
- Hits 173561 173455 -106
+ Misses 28362 28347 -15
Partials 3304 3304
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:
|
|
|
SUMMARY
Fixes #45460, following the agreed migration approach in #45460 (comment).
Older database forms stored metadata cache timeouts as strings. With cachelib 0.17, a stored value such as
"schema_cache_timeout": "3600"reaches the cache backend and raises a TypeError. Add a data-only migration that converts non-negative integer strings incatalog_cache_timeout,schema_cache_timeoutandtable_cache_timeoutto integers, including"0"to0.Only rows with a conversion are updated. Existing integers, missing keys, negative/invalid values, malformed or unrepresentable JSON, non-object configurations and unknown settings are preserved. Invalid configurations are left for operator repair rather than assigned a guessed default. Decimal-aware JSON decoding preserves unrelated high-precision and very large numeric settings when an affected row is serialized again; the shared JSON wrapper does not expose this decoding option. A JSON number outside Decimal's representable range leaves the whole row unchanged rather than interrupting the migration.
The SELECT requests row locks on backends that support them. Each UPDATE also matches the original
extravalue; if it changed after the read, the migration raises so Alembic can roll back instead of overwriting an edit or silently finishing with an unconverted row.Revision
9670ab7f6bc6followsf72b6e9c104a, the migration head at merged master1aceca35ed218288dcf2b20ed09dbc053c204a47. Its revision ID is unchanged. This PR does not choose a 7.0 RC cherry-pick order; release-branch migration ordering remains a release-manager decision.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; data migration only. A legacy string timeout causes the actual catalog/schema/table cached model methods to fail before migration. Reading the migrated row through the same methods writes successfully to a real cachelib SimpleCache.
TESTING INSTRUCTIONS
InvalidOperationbefore those preservation fixes.python -m pytest tests/unit_tests/migrations/test_normalize_metadata_cache_timeouts.py tests/unit_tests/migrations/test_single_migration_head.py tests/unit_tests/utils/cache_test.py -q— 48 passed on2cbe5915. Covers all three cache paths, zero, mixed connections, invalid/unrelated data, Decimal preservation and parse-range errors, idempotent no-write behavior, transaction rollback, rejection of a stale configuration snapshot and downgrade.superset db headsreports exactly9670ab7f6bc6 (head); the project's single-head test also passes.python -m py_compilepassed. Staged-file pre-commit passed all applicable checks except the three native Windowssignal.SIGALRM/signal.alarmerrors in untouchedsuperset/utils/core.py. The same mypy hook environment with--platform linux --check-untyped-defspassed both new Python files. Pylint excludes migration and test files under the repository configuration.For deployment-level verification, use a disposable metadata database with legacy string timeouts in
dbs.extra, runsuperset db upgrade, inspect the integer values and load the catalog/schema/table lists with metadata caching configured. Local tests use actual SQLite rows, Alembic MigrationContext/Operations and project model/cache code. The stale-row test injects a real row update through an execution event on the same connection; it proves conditional-update behavior, not two-connection concurrency. The actual migration SELECT was also captured and compiled with the PostgreSQL/MySQL/SQLite dialects. A full production metadata upgrade, live warehouse, browser flow, two-connection test and MySQL/PostgreSQL migration/row-lock execution were not exercised locally.Maintainer-requested rollback proof: the new two-row case observes a completed first UPDATE, changes the remaining row before the second UPDATE, lets the conflict exception propagate out of
engine.begin(), and verifies both exact original texts through a new connection. It passes with the existing runtime logic. The rollback test was added inebeb64e8. The latest follow-up ordinarily merges master1aceca35and changes onlyRevises/down_revision: the CLI first reportedf72b6e9c104aand9670ab7f6bc6as two heads, then only9670ab7f6bc6after reparenting. The revision ID, upgrade/downgrade logic and existing tests are unchanged.ADDITIONAL INFORMATION
Transaction and downgrade: updates use the Alembic connection without an independent commit; a real SQLite test verifies that the enclosing transaction can roll back the changes. Downgrade deliberately leaves integer values intact: older versions already accept integers, and the migration cannot distinguish an original integer from a converted value or a value saved after upgrade. The combined reversibility checkbox above is left unchecked because the original JSON types are not reconstructed.
Runtime/locks: one SELECT over
dbs.id/dbs.extra, followed by one conditional UPDATE per affected database connection: O(number of configured connections), independent of warehouse row counts. A bounded local timing sample of the locking/CAS runtime body atb0c8ee0(Python 3.12.14, SQLAlchemy 2.0.55, in-memory SQLite) took 0.148 seconds for the actual upgrade and commit of 1,000 synthetic connection rows with three string timeouts each, excluding app startup, seeding and verification. This is a local reference, not a production estimate or upper bound. The test/chain-only follow-up did not rerun this sample. There is no DDL or table rebuild. On PostgreSQL/MySQL the SELECT can lock all selected connection rows, including those not ultimately changed, until the Alembic transaction completes; pause configuration edits during the usual upgrade window. A conditional-update conflict aborts the migration and calls for retrying with edits paused. Production wall-clock time and downtime were not measured.AI assistance disclosure: OpenAI Codex assisted with investigation, implementation, test preparation, automated review and local validation. This PR does not claim human-executed validation or prior human review of the new migration.