fix(import): wrap per-tag tag operation in SAVEPOINT to prevent Session poisoning - #42919
fix(import): wrap per-tag tag operation in SAVEPOINT to prevent Session poisoning#42919waterWang wants to merge 1 commit into
Conversation
Code Review Agent Run #2711fdActionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if tag is None: | ||
| description = tag_descriptions.get(tag_name, None) | ||
| tag = Tag(name=tag_name, description=description, type="custom") | ||
| db_session.add(tag) | ||
| existing_tags[tag_name] = tag # Update the existing_tags dict |
There was a problem hiding this comment.
Suggestion: When two imports concurrently create the same tag, the losing insert raises a uniqueness error and rolls back this SAVEPOINT. The newly constructed Tag is then no longer persistent, but existing_tags still points to it, so this import skips the association instead of re-querying the tag row created by the winning transaction. Re-fetch the existing tag after the uniqueness conflict before continuing the association operation. [race condition]
Severity Level: Major ⚠️
- ❌ Concurrent imports can omit requested tag associations.
- ⚠️ Asset imports report success with incomplete tagging.
- ⚠️ The losing transaction does not reuse the winning tag row.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/importers/v1/utils.py
**Line:** 325:329
**Comment:**
*Race Condition: When two imports concurrently create the same tag, the losing insert raises a uniqueness error and rolls back this SAVEPOINT. The newly constructed `Tag` is then no longer persistent, but `existing_tags` still points to it, so this import skips the association instead of re-querying the tag row created by the winning transaction. Re-fetch the existing tag after the uniqueness conflict before continuing the association operation.
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 fix| db_session.add(new_tagged_object) | ||
|
|
||
| new_tag_ids.append(tag.id) | ||
| new_tag_ids.append(tag.id) |
There was a problem hiding this comment.
Suggestion: new_tag_ids is updated before the nested transaction has successfully flushed and released its SAVEPOINT. If the pending association insert fails while leaving the context manager, the exception is caught but this ID remains in new_tag_ids, causing cleanup and the returned result to treat the failed tag as successfully imported. [logic error]
Severity Level: Minor 🧹
- ⚠️ `import_tag()` returns IDs for failed associations.
- ⚠️ Concurrent imports produce inaccurate tag results.
- ⚠️ Cleanup decisions use stale success state.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/importers/v1/utils.py
**Line:** 343:343
**Comment:**
*Logic Error: `new_tag_ids` is updated before the nested transaction has successfully flushed and released its SAVEPOINT. If the pending association insert fails while leaving the context manager, the exception is caught but this ID remains in `new_tag_ids`, causing cleanup and the returned result to treat the failed tag as successfully imported.
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 fix
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42919 +/- ##
==========================================
- Coverage 66.42% 66.41% -0.01%
==========================================
Files 2857 2857
Lines 161293 161294 +1
Branches 37134 37134
==========================================
- Hits 107133 107130 -3
- Misses 52135 52138 +3
- Partials 2025 2026 +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:
|
1 participant
Summary
Fixes #42912
import_tagcatchesSQLAlchemyErrorafter a query-triggered autoflush failure and continues using the same SQLAlchemySessionwithout rolling back or isolating the failed per-tag operation in a SAVEPOINT.When two concurrent tag imports race on the same
TaggedObjectassociation (uix_tagged_objectunique constraint), one transaction commits first and the other receives a realUniqueViolation.import_tagcatches it andcontinues, but theSessionremains in SQLAlchemy's pending-rollback state, so the nextSessionoperation raisesPendingRollbackError.The
@transaction()decorator onimport_tagis a no-op when called fromImportAssetsCommand.run()(which is already inside a transaction), so the poisoned session is not recovered before reuse.Fix
Wrap each per-tag operation in a SAVEPOINT via
db_session.begin_nested(). On failure the SAVEPOINT is rolled back, the failed tag is skipped, and theSessionremains usable for subsequent tags — the concurrent import completes without discarding unrelated imports.Why this approach
begin_nested()rolls back only the failed per-tag operation, preserving successfully-imported tagsSession.rollback()would discard the entire import, which the issue explicitly warns againstTesting