bikash-barnwal opened a new pull request, #43187:
URL: https://github.com/apache/superset/pull/43187
### SUMMARY
`import_tag` catches `SQLAlchemyError` per tag and continues on the same
Session, with the comment *"No need for manual rollback, handled by transaction
decorator"*. That assumption doesn't hold. When a concurrent import commits the
same `uix_tagged_object(tag_id, object_id, object_type)` row first, this
Session's flush raises — and the Session is then in SQLAlchemy's
pending-rollback state, so the very next statement, the lookup for an entirely
unrelated tag, raises:
```
PendingRollbackError: This Session's transaction has been rolled back due to
a previous
exception during flush. ... Original exception was: (sqlite3.IntegrityError)
UNIQUE
constraint failed: tagged_object.tag_id, tagged_object.object_id,
tagged_object.object_type
```
`ImportAssetsCommand.run()` then converts that into `ImportFailedError` and
the whole import is lost, exactly as reported.
Each tag now runs inside `db_session.begin_nested()` with an explicit flush,
so a lost race rolls back that tag alone. A plain `db_session.rollback()` would
discard the rest of the import along with it — and `begin_nested` is already
the established pattern for this in the codebase
(`commands/deletion_retention/purge_cascade.py:217`,
`security/password_change.py:113`, `subjects/utils.py:348`).
Two consequences of a tag genuinely failing are handled:
- the cached `Tag` is dropped from `existing_tags`, since the SAVEPOINT
rollback invalidates whatever that iteration added and a later lookup must not
reuse it;
- its id is remembered, so the *"remove old tags not in the new config"*
sweep at the end doesn't delete the existing association of a tag that **is**
in the config and merely lost a race. Without that, the fix would trade a crash
for silent data loss — which the new test caught.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — backend correctness fix.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/commands/importers/v1/import_tag_savepoint_test.py
pytest tests/unit_tests/commands/importers
tests/unit_tests/charts/commands/importers \
tests/unit_tests/dashboards/commands/importers tests/unit_tests/tags
```
The new test reproduces the race by committing the association up front and
making the existence check miss it once — precisely what the losing importer
observes. On `master` it fails with the `PendingRollbackError` quoted above;
with this change both tests pass, and **136 tests pass** across the importer
and tag suites.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #42912
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
