aminghadersohi commented on code in PR #45232:
URL: https://github.com/apache/superset/pull/45232#discussion_r4276476071
##########
superset/daos/query.py:
##########
@@ -70,26 +70,50 @@ def stop_query(client_id: str) -> None:
if not query:
raise QueryNotFoundException(f"Query with client_id {client_id}
not found")
- if query.status in [
- QueryStatus.FAILED,
- QueryStatus.SUCCESS,
- QueryStatus.TIMED_OUT,
- ]:
+ if query.status in FINISHED_QUERY_STATUSES:
logger.warning(
"Query with client_id could not be stopped: query already
complete",
)
return
- if not sql_lab.cancel_query(query):
+ cancelled = sql_lab.cancel_query(query)
+ if query.status in FINISHED_QUERY_STATUSES:
+ # Preparing the stop can observe the worker's terminal commit.
+ logger.warning(
+ "Query with client_id could not be stopped: query already
complete",
+ )
+ return
+ if not cancelled:
raise SupersetCancelQueryException("Could not cancel query")
+ if query.status == QueryStatus.STOPPED:
+ # The engine spec already committed the stop while preparing it
+ # (see ImpalaEngineSpec.prepare_cancel_query).
+ return
+
# cancel_query() may have staged an early-cancel flag on query.extra
# without committing it (see its docstring/comments); the
# @transaction decorator commits it together with status=STOPPED
# below in one transaction, closing the window where another
# request could observe the flag set but the status still RUNNING.
- query.status = QueryStatus.STOPPED
- query.end_time = now_as_float()
+ #
+ # The write is conditional: the worker can commit a terminal status
+ # after the check above, and that result must not be overwritten.
+ stopped = (
+ db.session.query(Query)
+ .filter(
+ Query.id == query.id,
+ Query.status.notin_(FINISHED_QUERY_STATUSES),
Review Comment:
I fixed the async executor cancel chain in
ccb2d13b5987c2e581c6e7a419af85bffe88075e: successful cancellation commits
STOPPED, replaces a FAILED caused by the kill, preserves SUCCESS/TIMED_OUT, and
never writes STOPPED for a failed cancel.
Regression tests: `test_async_external_cancel_commits_only_successful_stop`
uses real Postgres/MySQL cancel methods and a second metadata session;
`test_async_cancel_commits_implicit_or_early_stop`,
`test_async_cancel_exception_does_not_commit_stop`, and
`test_async_cancel_leaves_finished_query_untouched` cover the other branches.
`PYTHONPATH=.:superset-core/src /tmp/w45232-venv/bin/python -m pytest
tests/unit_tests/dao/query_stop_isolation_test.py -k test_async_ -q` at
4a2cc30492ddd960bca72388122c119fd9a914eb: 10 failed, 12 passed; the complete
module after: 32 passed.
Validation: the same pytest prefix with
`tests/unit_tests/dao/query_stop_isolation_test.py
tests/unit_tests/sql/execution/test_executor.py
tests/unit_tests/sql/execution/test_celery_task.py -q`: 355 passed, 3 skipped;
staged-file `PATH=/tmp/w45232-venv/bin:$PATH pre-commit run`: passed, including
mypy.
Siblings checked: superset/sql/execution/executor.py — fixed in
ccb2d13b5987c2e581c6e7a419af85bffe88075e; superset/sql_lab.py — not affected:
QueryDAO persists successful sync stops; superset/daos/query.py — not affected:
conditional write already preserves SUCCESS/TIMED_OUT; superset/queries/api.py
— not affected: delegates to QueryDAO.stop_query;
superset/tasks/query_cancel.py — not affected: chart/MCP cancellation stub has
no SQL Lab Query row; superset/mcp_service/worker.py — not affected: delegates
to cancel_chart_query; superset/tasks/async_queries.py — not affected: separate
chart task lifecycle; superset/commands/tasks/reap.py — not affected: separate
orphan task lifecycle. `rg` found no other production callers of
SQLExecutor._cancel_query.
Remaining: the celery_task._handle_query_error overwrite of committed
STOPPED, including async Trino early kills, belongs exclusively to
https://github.com/apache/superset/pull/45283
At that PR's fetched fork/sql-stop-race-finalization head
ba1fcc6b893a15a33f36c33fec060274b41e840c, `git show
...:superset/sql/execution/celery_task.py` shows _handle_query_error preserving
committed STOPPED when durable=False, with a conditional commit guard for a
concurrent stop; durable=True intentionally retains FAILED after warehouse
commit. I did not change that handler here.
--
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]
