codeant-ai-for-open-source[bot] commented on code in PR #43463:
URL: https://github.com/apache/superset/pull/43463#discussion_r3844737335
##########
superset/tasks/async_queries.py:
##########
@@ -122,6 +122,9 @@ def execute_chart_query(
"""
with override_user(_resolve_user(user_id, guest_token), force=False):
query_context = load_serialized_query(serialized_query)
+ # Floor the result-cache TTL: async caches the result for a follow-up
+ # request to read back (see get_cache_timeout).
+ query_context.is_async_execution = True
Review Comment:
**Suggestion:** Setting `is_async_execution` does not ensure that the cache
entry is written with the floored TTL. If the initial cache miss races with
another request that populates the same key, `get_df_payload_result` will
accept that existing entry and skip `set_query_result`, leaving its original
short expiration. The follow-up request can then miss the cache and schedule
another query. The async task should refresh or rewrite the entry using the
async timeout when it finds an existing result. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Async chart re-fetch can miss completed results.
- ⚠️ Identical queries may be scheduled repeatedly.
- ⚠️ Users can see delayed or failed chart rendering.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tasks/async_queries.py
**Line:** 125:127
**Comment:**
*Race Condition: Setting `is_async_execution` does not ensure that the
cache entry is written with the floored TTL. If the initial cache miss races
with another request that populates the same key, `get_df_payload_result` will
accept that existing entry and skip `set_query_result`, leaving its original
short expiration. The follow-up request can then miss the cache and schedule
another query. The async task should refresh or rewrite the entry using the
async timeout when it finds an existing result.
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%2F43463&comment_hash=f6011f8749fbd3f9a3169df0090b57377eba391fab6e4c17921004ef143e4d69&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43463&comment_hash=f6011f8749fbd3f9a3169df0090b57377eba391fab6e4c17921004ef143e4d69&reaction=dislike'>👎</a>
##########
superset/common/query_context_processor.py:
##########
@@ -599,6 +607,21 @@ def get_cache_timeout(self) -> int:
# Step 5: Global fallback.
return current_app.config["CACHE_DEFAULT_TIMEOUT"]
+ def _apply_async_min_cache_ttl(self, timeout: int) -> int:
+ """Floor an async execution's result-cache TTL (no-op otherwise).
+
+ Only applies when this query context runs on the async path; a longer
+ timeout is kept as-is, and ``0`` (flask-caching "cache forever") is
already
+ above any floor so it is left untouched. Synchronous requests are never
+ floored, even when GLOBAL_ASYNC_QUERIES is enabled.
+ """
+ if self._query_context.is_async_execution is not True:
+ return timeout
+ min_ttl: int =
current_app.config.get("GLOBAL_ASYNC_QUERIES_MIN_CACHE_TTL", 0)
+ if 0 < timeout < min_ttl:
+ return min_ttl
+ return timeout
Review Comment:
**Suggestion:** The post-resolution floor also applies to
`NATIVE_FILTER_OPTIONS_CACHE_TIMEOUT`, despite that timeout being an
independent freshness policy for native filter option queries. An async
native-filter request configured with a short value such as 30 seconds will be
cached for 300 seconds and can serve stale filter values. Exclude native filter
option queries from this floor or apply the floor before selecting their
dedicated timeout. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Native filter options remain stale beyond configured freshness.
- ⚠️ RLS-constrained filter values can remain outdated.
- ⚠️ Dashboard filter controls may show unavailable values.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_context_processor.py
**Line:** 621:623
**Comment:**
*Cache: The post-resolution floor also applies to
`NATIVE_FILTER_OPTIONS_CACHE_TIMEOUT`, despite that timeout being an
independent freshness policy for native filter option queries. An async
native-filter request configured with a short value such as 30 seconds will be
cached for 300 seconds and can serve stale filter values. Exclude native filter
option queries from this floor or apply the floor before selecting their
dedicated timeout.
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%2F43463&comment_hash=d4334ea6e9c468a549e31658e30410247aeb60dca425417f5b45eaeb776b17fb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43463&comment_hash=d4334ea6e9c468a549e31658e30410247aeb60dca425417f5b45eaeb776b17fb&reaction=dislike'>👎</a>
##########
tests/unit_tests/common/test_async_min_cache_ttl.py:
##########
@@ -0,0 +1,67 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import MagicMock
+
+from flask import current_app
+from pytest_mock import MockerFixture
+
+from superset.common.query_context_processor import QueryContextProcessor
+
+
+def _processor(
+ *, is_async: bool, resolved_timeout: int | None
+) -> QueryContextProcessor:
+ """A processor over a stub context whose slice/datasource timeout is
fixed."""
+ query_context = MagicMock()
+ query_context.custom_cache_timeout = None
+ query_context.form_data = {}
+ query_context.get_cache_timeout.return_value = resolved_timeout
+ query_context.is_async_execution = is_async
+ return QueryContextProcessor(query_context)
+
+
+def test_async_execution_floors_short_cache_timeout(
+ app_context: None, mocker: MockerFixture
+) -> None:
+ mocker.patch.dict(current_app.config,
{"GLOBAL_ASYNC_QUERIES_MIN_CACHE_TTL": 300})
+ # 60s < 300s floor → raised to the floor so the result survives until
re-fetch.
+ assert _processor(is_async=True, resolved_timeout=60).get_cache_timeout()
== 300
+
+
+def test_async_execution_keeps_longer_cache_timeout(
+ app_context: None, mocker: MockerFixture
+) -> None:
+ mocker.patch.dict(current_app.config,
{"GLOBAL_ASYNC_QUERIES_MIN_CACHE_TTL": 300})
+ # A longer configured timeout is not lowered.
+ assert _processor(is_async=True,
resolved_timeout=3600).get_cache_timeout() == 3600
+
+
+def test_async_execution_leaves_zero_timeout_untouched(
+ app_context: None, mocker: MockerFixture
+) -> None:
+ mocker.patch.dict(current_app.config,
{"GLOBAL_ASYNC_QUERIES_MIN_CACHE_TTL": 300})
+ # 0 means "cache forever" (flask-caching), already above any floor.
+ assert _processor(is_async=True, resolved_timeout=0).get_cache_timeout()
== 0
+
+
+def test_sync_execution_is_not_floored(
+ app_context: None, mocker: MockerFixture
+) -> None:
+ mocker.patch.dict(current_app.config,
{"GLOBAL_ASYNC_QUERIES_MIN_CACHE_TTL": 300})
+ # The floor applies only to async execution; a sync request keeps its
timeout
+ # even when GLOBAL_ASYNC_QUERIES is enabled.
Review Comment:
**Suggestion:** The test claims to verify synchronous behavior while
`GLOBAL_ASYNC_QUERIES` is enabled, but it only patches the minimum TTL. If the
implementation incorrectly floors synchronous requests whenever the global
async feature is enabled, this test still passes because that feature remains
at its default value. Patch `GLOBAL_ASYNC_QUERIES` to true in this test so the
asserted sync-path gating is actually exercised. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Sync-path gating is not tested with async globally enabled.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_async_min_cache_ttl.py
**Line:** 64:66
**Comment:**
*Incomplete Implementation: The test claims to verify synchronous
behavior while `GLOBAL_ASYNC_QUERIES` is enabled, but it only patches the
minimum TTL. If the implementation incorrectly floors synchronous requests
whenever the global async feature is enabled, this test still passes because
that feature remains at its default value. Patch `GLOBAL_ASYNC_QUERIES` to true
in this test so the asserted sync-path gating is actually exercised.
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%2F43463&comment_hash=ad32b2cd14d1cf009f2fe03ed83dcf494dac4407f3d6119d051ac5f68ceba48e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43463&comment_hash=ad32b2cd14d1cf009f2fe03ed83dcf494dac4407f3d6119d051ac5f68ceba48e&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]
