codeant-ai-for-open-source[bot] commented on code in PR #45861:
URL: https://github.com/apache/superset/pull/45861#discussion_r4459432083
##########
superset/models/core.py:
##########
@@ -1740,16 +1740,34 @@ def _evict_engine_cache(
"""Evict all cached engines for a database when it is updated or deleted.
URL/kwargs changes already produce a new cache key, so stale engines are
- never served to callers. This eviction step is purely to reclaim memory:
- without it, old engines for a renamed host or rotated password would linger
- in _ENGINE_CACHE until the process restarted.
+ never served to callers. Eviction reclaims memory (old engines for a
+ renamed host or rotated password would otherwise linger in _ENGINE_CACHE
+ until the process restarted). Engines on a ``QueuePool`` are also disposed,
+ which closes the connections their pool holds idle. Some databases, such as
+ DuckDB, allow only one configuration per database file per process, so an
+ idle connection kept by an engine with the previous settings would block
+ opening the edited ones.
+
+ Engines on pools whose disposal also closes checked-out connections
+ (``StaticPool``, ``SingletonThreadPool``) are not disposed, so that a query
+ running at that moment is not broken: their connection is released when the
+ engine is garbage collected. Connections of a ``QueuePool`` that are
checked
+ out stay on its previous pool and are not released deterministically.
+ Disposal runs after the cache lock is released, and a failure to dispose
+ never interrupts the update or delete.
"""
if target.id is None:
return
with _ENGINE_CACHE_LOCK:
stale = [k for k in _ENGINE_CACHE if k[0] == target.id]
- for k in stale:
- _ENGINE_CACHE.pop(k, None)
+ evicted = [_ENGINE_CACHE.pop(k) for k in stale]
Review Comment:
**Suggestion:** A concurrent cache miss can insert an old-settings engine
after this snapshot, leaving it cached with an idle connection that blocks
opening the edited configuration.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d144baabf83143c295f495ef3b21e673&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=d144baabf83143c295f495ef3b21e673&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/core.py
**Line:** 1763:1763
**Comment:**
*Race Condition: A concurrent cache miss can insert an old-settings
engine after this snapshot, leaving it cached with an idle connection that
blocks opening the edited 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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45861&comment_hash=7134c2433a8fbd94ec0abdee1179a7ab764822e48023f2c9e6886024bddeaaee&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45861&comment_hash=7134c2433a8fbd94ec0abdee1179a7ab764822e48023f2c9e6886024bddeaaee&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]
