codeant-ai-for-open-source[bot] commented on code in PR #41979:
URL: https://github.com/apache/superset/pull/41979#discussion_r3574368604
##########
superset/extensions/metadb.py:
##########
@@ -445,15 +445,16 @@ def insert_row(self, row: Row) -> int:
query = self._table.insert().values(**row)
with self.engine_context() as engine:
- connection = engine.connect()
- result = connection.execute(query)
+ with engine.begin() as connection:
+ result = connection.execute(query)
- # return rowid
- if self._rowid:
- return result.inserted_primary_key[0]
+ # return rowid
+ if self._rowid:
+ return result.inserted_primary_key[0]
query = select(func.count()).select_from(self._table)
- return connection.execute(query).scalar()
+ with engine.connect() as connection:
+ return connection.execute(query).scalar()
Review Comment:
**Suggestion:** When there is no integer primary key, the returned row
identifier is derived from a separate `COUNT(*)` query after the insert
transaction has already committed. Under concurrent inserts/deletes this count
can change between statements, so the method may return a row id that does not
correspond to the row just inserted. Compute and return the fallback identifier
within the same transactional scope (or use a deterministic per-row key) to
avoid cross-request races. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Superset meta DB INSERTs can return wrong row identifiers.
- ⚠️ Shillelagh virtual tables observe nondeterministic row index values.
- ⚠️ Cross-database experimental feature behaves unpredictably under
concurrency.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the Superset meta database feature flag `ENABLE_SUPERSET_META_DB`
(see
superset/config.py:768) and configure a `Database` record with
`allow_dml=True` so that
`SupersetShillelaghAdapter._set_columns`
(superset/extensions/metadb.py:299-308) sets
`self._allow_dml` and leaves `self._rowid` as `None` for a table without a
single integer
primary key (superset/extensions/metadb.py:276, 344-350).
2. Use the experimental `superset://` SQLAlchemy dialect (registered in
setup.py:8-12 and
described in superset/db_engine_specs/superset.py:12-17) to open two
concurrent client
connections that both issue `INSERT` statements into the same Superset meta
DB virtual
table backed by the non-rowid underlying table configured in step 1.
3. For each `INSERT`, the Shillelagh engine invokes
`SupersetShillelaghAdapter.insert_row`
(superset/extensions/metadb.py:428-79). Because `self._rowid` is `None`,
after executing
the insert inside `with engine.begin() as connection:` (lines 68-71),
control falls
through to the fallback branch at lines 455-457, which builds
`select(func.count()).select_from(self._table)` and executes it on a fresh
`engine.connect()` connection after the transaction has already committed.
4. If, between the commit in line 69-71 and the `COUNT(*)` execution in
lines 455-457,
another client inserts or deletes rows on the same table, the returned
scalar count
reflects the concurrent mutations rather than just the row just inserted, so
the integer
returned by `insert_row` no longer uniquely identifies the newly inserted
row and can
instead represent a different row position, yielding a non-deterministic and
potentially
incorrect row identifier under concurrent access.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ec11b598e4e640198b29ead1e7a8aec2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ec11b598e4e640198b29ead1e7a8aec2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/extensions/metadb.py
**Line:** 455:457
**Comment:**
*Race Condition: When there is no integer primary key, the returned row
identifier is derived from a separate `COUNT(*)` query after the insert
transaction has already committed. Under concurrent inserts/deletes this count
can change between statements, so the method may return a row id that does not
correspond to the row just inserted. Compute and return the fallback identifier
within the same transactional scope (or use a deterministic per-row key) to
avoid cross-request races.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41979&comment_hash=055fea0f12b8f7ff7ffa326c69c4cdf60fb0dc1df45d3c859600408f1acd30a0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41979&comment_hash=055fea0f12b8f7ff7ffa326c69c4cdf60fb0dc1df45d3c859600408f1acd30a0&reaction=dislike'>👎</a>
--
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]
