aminghadersohi commented on PR #45232: URL: https://github.com/apache/superset/pull/45232#issuecomment-6157202346
AI code review of this PR at 386b5e0b (full diff against master), follow-up to the review at ab5df9b3. **Finding (LOW–MEDIUM, fixed in 970341c265):** when `ImpalaEngineSpec.prepare_cancel_query`'s conditional `UPDATE ... SET status=STOPPED` matched no row because the worker had already committed SUCCESS, FAILED, or TIMED_OUT, the plain `db.session.refresh(query)` could still return the RUNNING snapshot from the stop request's first SELECT. This only happens when the metadata DB runs under REPEATABLE READ, which is not the default but is configurable through `SQLALCHEMY_ENGINE_OPTIONS`. In that case the post-prepare re-checks in `sql_lab.cancel_query`, `QueryDAO.stop_query`, and `SQLExecutor._cancel_query` did not see the terminal status. They fell through to the HTTP cancel, and `stop_query` then overwrote the finished query with STOPPED. The fix uses `refresh(query, with_for_update=True)`, the same locking read `_commit_query_unless_stopped` already uses. The regression test `test_finished_query_stop_bypasses_repeatable_read_snapshot` covers the SQL Lab and executor callers for all three terminal statuses. All 6 cases fail on 386b5e0b and pass after the fix. **Checked, no further findings:** - Stop vs. worker terminal commit: both are conditional UPDATEs, so the row lock serialises them and the losing side re-evaluates against the committed row. - Callers after prepare: `sql_lab.cancel_query` returns True for a finished query and `QueryDAO.stop_query` returns without writing STOPPED. `SQLExecutor._cancel_query` returns False. No other caller of `sql_lab.cancel_query` exists. - Trino, Presto, and Hive: their cancel paths are unchanged by this PR. Trino's `prepare_cancel_query` keeps its flag-only behaviour. MCP and chart cancellation go through `tasks/query_cancel.py` and still use the Impala HTTP cancel, which matches the `has_implicit_cancel` docstring. - Celery stop checks (`preserve_pending`, preserving TIMED_OUT in `_handle_query_error`, `no_autoflush` around finalization). **Tests:** test_impala_pending, test_impala, dao/queries_test, sql/execution/test_celery_task, test_executor, test_sqllab_error_stacktrace, and sql_lab_test: 430 passed. The 8 `generate_series` executor failures come from `duckdb` not being installed locally and are unrelated to this PR. `pre-commit run` on the changed files passes. -- 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]
