codeant-ai-for-open-source[bot] commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3572451313
##########
superset/initialization/__init__.py:
##########
@@ -849,10 +857,61 @@ def init_versioning(self) -> None:
register_baseline_listener()
register_change_record_listener()
- # Retention pruning runs out-of-band as a scheduled Celery beat
- # task, shipped as a separate stacked PR. The previous
- # synchronous after_commit listener was retired so retention work
- # doesn't add latency to user saves.
+ # Retention is time-based and runs out-of-band as a Celery beat
+ # task — see ``superset/tasks/version_history_retention.py``
+ # and the ``version_history.prune_old_versions`` entry in
+ # ``CeleryConfig.beat_schedule`` (``superset/config.py``). The
+ # previous synchronous after_commit listener was retired so
+ # retention work doesn't add latency to user saves.
+
+ _RETENTION_TASK_NAME: str = "version_history.prune_old_versions"
+
+ def _warn_if_retention_beat_missing(self) -> None:
+ """WARN at startup when the resolved Celery beat schedule has no
+ ``version_history.prune_old_versions`` entry.
+
+ Operators who redefine ``CeleryConfig`` in ``superset_config.py``
+ — instead of subclassing or merging the default — silently lose
+ the retention task. Capture continues writing rows; the prune
+ never runs; disk grows until paged. The default config carries
+ the entry; this check makes the misconfiguration visible in the
+ deploy log before disk pressure makes it visible at 03:00.
+
+ Handles three shapes of ``CELERY_CONFIG``:
+ * ``None`` — Celery deliberately disabled, no retention either
+ way; return without warning.
+ * a class or module with a ``beat_schedule`` attribute — the
+ default ``CeleryConfig`` shape.
+ * a dict — Celery's documented "config as dict" shape, supported
+ by ``celery_app.config_from_object``.
+ """
+ celery_config = self.config.get("CELERY_CONFIG")
+ if celery_config is None:
+ return # Celery disabled entirely; no retention task to warn
about.
+ beat_schedule = (
+ celery_config.get("beat_schedule")
+ if isinstance(celery_config, dict)
+ else getattr(celery_config, "beat_schedule", None)
+ )
+ # Match on the ``task`` each entry runs, not the schedule entry key:
+ # an operator may register the retention task under any key (e.g.
+ # ``{"prune_versions": {"task":
"version_history.prune_old_versions"}}``),
+ # which is still correctly scheduled and must not warn. The default
+ # config happens to use the task name as the key, but that's
incidental.
+ registered_tasks = {
+ entry.get("task")
+ for entry in (beat_schedule or {}).values()
+ if isinstance(entry, dict)
+ }
+ registered_tasks.update(beat_schedule or {}) # tolerate key == task
name
+ if not beat_schedule or self._RETENTION_TASK_NAME not in
registered_tasks:
+ logger.warning(
Review Comment:
**Suggestion:** The startup check assumes `CELERY_CONFIG` is only a
dict/class/module and treats any other valid Celery `config_from_object` input
(notably a dotted import string) as missing `beat_schedule`, which will emit a
false warning on healthy deployments. Resolve string configs before inspection
(or skip this warning path for string/object-ref configs) so valid task
schedules are not incorrectly reported as missing. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Startup logs falsely claim retention beat task missing.
- ⚠️ Operators may mis-tune Celery due to spurious warning.
- ⚠️ Noise hides genuine retention misconfiguration warnings.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset with a Celery config specified as a dotted import
string, e.g. set
`CELERY_CONFIG = "superset.config:CeleryConfig"` in `superset_config.py`
(read later via
`self.config["CELERY_CONFIG"]` in `SupersetAppInitializer.configure_celery`
at
`superset/initialization/__init__.py:134`).
2. Start the application so `SupersetAppInitializer.init_app_in_ctx` (at
`superset/initialization/__init__.py:217-245`) runs, which in turn calls
`init_versioning()` at line 246.
3. Inside `init_versioning`, `_warn_if_retention_beat_missing()` is invoked
(defined at
`superset/initialization/__init__.py:170`), where `celery_config =
self.config.get("CELERY_CONFIG")` returns the string from step 1 and
`beat_schedule` is
computed as shown at lines 891-895; because `celery_config` is a string,
`getattr(celery_config, "beat_schedule", None)` returns `None`.
4. With `beat_schedule` evaluating to falsy, `registered_tasks` remains
empty (lines
202-207 / 891-907), and the condition `if not beat_schedule or
self._RETENTION_TASK_NAME
not in registered_tasks:` at line 908 triggers, emitting the warning that
`CELERY_CONFIG.beat_schedule` is missing the
`"version_history.prune_old_versions"` entry
even though Celery itself successfully resolved the string via
`celery_app.config_from_object` and scheduled the task, leading to a
false-positive
startup warning on a valid configuration.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6ffa039579d344609ad22dca4e608b18&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=6ffa039579d344609ad22dca4e608b18&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/initialization/__init__.py
**Line:** 891:908
**Comment:**
*Api Mismatch: The startup check assumes `CELERY_CONFIG` is only a
dict/class/module and treats any other valid Celery `config_from_object` input
(notably a dotted import string) as missing `beat_schedule`, which will emit a
false warning on healthy deployments. Resolve string configs before inspection
(or skip this warning path for string/object-ref configs) so valid task
schedules are not incorrectly reported as missing.
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%2F41075&comment_hash=48fae129a0e21051879e2d377e4e6fb0a7021e2c7c9ef75a164fa9f31435cba8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=48fae129a0e21051879e2d377e4e6fb0a7021e2c7c9ef75a164fa9f31435cba8&reaction=dislike'>👎</a>
##########
tests/integration_tests/versioning/retention_prune_tests.py:
##########
@@ -0,0 +1,488 @@
+# 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.
+"""Integration coverage for version-history retention pruning.
+
+Exercises ``superset.tasks.version_history_retention`` end-to-end against
+a real database: that an aged-out transaction's shadow rows are pruned
+while the live row is always preserved, and that the SERIALIZABLE pass
+retries on transient serialization failures and gives up after the cap.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+import pytest
+from sqlalchemy_continuum import version_class
+
+from superset.extensions import db
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.versioning.changes.table import version_changes_table
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa:
F401
+ load_birth_names_dashboard_with_slices,
+ load_birth_names_data,
+)
+
+
+def _get_version_rows(dashboard: Dashboard) -> list[Any]:
+ ver_cls = version_class(Dashboard)
+ return (
+ db.session.query(ver_cls)
+ .filter(ver_cls.id == dashboard.id)
+ .order_by(ver_cls.transaction_id.asc())
+ .all()
+ )
+
+
+def _persist_fixture_state() -> None:
+ """Force the fixture's pending INSERTs to commit in their own transaction.
+
+ The birth_names fixture stages charts and the dashboard via session.add()
+ but does not commit. Without this, the test's first commit batches the
+ INSERTs and UPDATEs into the same Continuum transaction, causing the
+ existing version row to be updated in place instead of a new one being
+ created.
+ """
+ db.session.commit()
+
+
+class TestDashboardVersionRetention(SupersetTestCase):
+ """Retention pruning drops shadow rows older than
+ ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` while preserving live rows."""
+
+ @pytest.fixture(autouse=True)
+ def _load_data(
+ self,
+ load_birth_names_dashboard_with_slices: None, # noqa: F811
+ ) -> None: # noqa: PT004
+ """Establish the capture state this integration class requires.
+
+ Continuum stores its option and SQLAlchemy event listeners globally.
+ Initialization unit tests intentionally exercise the kill-switch that
+ detaches those listeners, so a mixed or reordered pytest invocation can
+ otherwise enter these tests with capture disabled. Reasserting the
+ integration suite's prerequisite here makes each test
order-independent.
+ """
+ from sqlalchemy_continuum import versioning_manager
+
+ from superset.initialization import SupersetAppInitializer
+
+ versioning_manager.options["versioning"] = True
+ SupersetAppInitializer._add_continuum_write_listeners()
Review Comment:
**Suggestion:** This autouse fixture mutates global Continuum state
(`options["versioning"]`) but never restores it, so later tests can inherit
capture-enabled state and become order-dependent/flaky. Save the previous value
and restore it in fixture teardown (and similarly restore listener state if
changed) to keep test isolation intact. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Global Continuum options leak across test cases.
- ⚠️ Extra listeners persist, altering later tests’ behavior.
- ⚠️ Test suite may become order-dependent and flaky.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the integration test class `TestDashboardVersionRetention` in
`tests/integration_tests/versioning/retention_prune_tests.py`, which defines
an autouse
fixture `_load_data` at lines 69-88 so it executes before every test in the
class.
2. Observe in `_load_data` that `versioning_manager.options["versioning"] =
True` is set
and `SupersetAppInitializer._add_continuum_write_listeners()` is called
(lines 82-87),
mutating the global Continuum configuration and attaching global SQLAlchemy
event
listeners, as described by `_add_continuum_write_listeners` in
`superset/initialization/__init__.py:4-45`.
3. After each test in this class completes, there is no corresponding
teardown or `yield`
in the fixture to restore `versioning_manager.options["versioning"]` to its
previous value
or to undo the added listeners; the mutated global state therefore persists
for any
subsequent tests in the same process.
4. Because other code paths (for example the kill-switch handling in
`superset/initialization/__init__.py:658-667` and the baseline listener
guard in
`superset/versioning/baseline/listener.py:104`) consult
`versioning_manager.options["versioning"]` and assume it reflects the current
configuration, tests that rely on those paths can see different behavior
depending on
whether they run before or after this class, making the suite
order-dependent and
potentially flaky due to leaked versioning state.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ccba2b63cae541988ac8c5428031cdbb&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=ccba2b63cae541988ac8c5428031cdbb&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:** tests/integration_tests/versioning/retention_prune_tests.py
**Line:** 86:87
**Comment:**
*Logic Error: This autouse fixture mutates global Continuum state
(`options["versioning"]`) but never restores it, so later tests can inherit
capture-enabled state and become order-dependent/flaky. Save the previous value
and restore it in fixture teardown (and similarly restore listener state if
changed) to keep test isolation intact.
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%2F41075&comment_hash=72feb9c356f3f47eb5e0d4535f94b2a1fe1c76ada09428165cda01077e158e69&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=72feb9c356f3f47eb5e0d4535f94b2a1fe1c76ada09428165cda01077e158e69&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]
