codeant-ai-for-open-source[bot] commented on code in PR #37574:
URL: https://github.com/apache/superset/pull/37574#discussion_r2746291178
##########
tests/integration_tests/celery_tests.py:
##########
@@ -575,6 +575,22 @@ def my_task(self):
)
+def test_teardown_without_app_context():
+ """Test teardown signal handler doesn't raise when called outside app
context.
+
+ Regression test for https://github.com/apache/superset/issues/36892
+ The task_postrun signal can fire after the app context is torn down,
+ so teardown() must check has_app_context() before calling
db.session.remove().
+ """
+ from superset.tasks.celery_app import teardown
+
+ # Ensure we're outside of an app context
+ assert not has_app_context(), "Test must run outside of app context"
+
+ # This should not raise RuntimeError: Working outside of application
context
+ teardown(retval="success")
Review Comment:
**Suggestion:** RuntimeError risk: the test calls
`teardown(retval="success")` while relying on being outside an app context, but
if `flask_app.config["SQLALCHEMY_COMMIT_ON_TEARDOWN"]` is True the `teardown`
implementation will call `db.session.commit()` (which requires an app context)
and raise `RuntimeError`. Temporarily disable that config key for the duration
of the call and restore it afterwards to avoid the commit path and to avoid
leaking config changes to other tests. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌
tests/integration_tests/celery_tests.py::test_teardown_without_app_context fails
- ❌ Celery task_postrun may raise RuntimeError in workers
- ⚠️ CI flaky due to intermittent test failures
```
</details>
```suggestion
from superset.tasks.celery_app import teardown, flask_app
# Ensure we're outside of an app context
assert not has_app_context(), "Test must run outside of app context"
# Temporarily disable commit-on-teardown to avoid calling
db.session.commit() outside app context
_orig = flask_app.config.get("SQLALCHEMY_COMMIT_ON_TEARDOWN", False)
flask_app.config["SQLALCHEMY_COMMIT_ON_TEARDOWN"] = False
try:
# This should not raise RuntimeError: Working outside of application
context
teardown(retval="success")
finally:
flask_app.config["SQLALCHEMY_COMMIT_ON_TEARDOWN"] = _orig
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the integration test function defined at
tests/integration_tests/celery_tests.py:578-591
(test_teardown_without_app_context).
2. The test imports teardown from superset/tasks/celery_app.py and calls
teardown(retval="success").
3. In superset/tasks/celery_app.py (function teardown in file range lines
53-75), the code
first checks flask_app.config.get("SQLALCHEMY_COMMIT_ON_TEARDOWN") and, if
true and retval
is not an Exception, executes db.session.commit().
4. If the process is currently outside an application context (the test
asserts this via
has_app_context()), calling db.session.commit() will raise RuntimeError:
Working outside
of application context.
5. Temporarily disabling flask_app.config["SQLALCHEMY_COMMIT_ON_TEARDOWN"]
around the
teardown() call (as proposed) guarantees the commit path is not taken,
preventing the
RuntimeError during the test run and avoiding leaking config changes.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/celery_tests.py
**Line:** 585:591
**Comment:**
*Possible Bug: RuntimeError risk: the test calls
`teardown(retval="success")` while relying on being outside an app context, but
if `flask_app.config["SQLALCHEMY_COMMIT_ON_TEARDOWN"]` is True the `teardown`
implementation will call `db.session.commit()` (which requires an app context)
and raise `RuntimeError`. Temporarily disable that config key for the duration
of the call and restore it afterwards to avoid the commit path and to avoid
leaking config changes to other tests.
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.
```
</details>
--
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]
