sadpandajoe opened a new pull request, #43889:
URL: https://github.com/apache/superset/pull/43889
## SUMMARY
Stopping a SQL Lab query before its DB engine "cancel handle" has been
obtained (e.g.
it's still validating RLS/table checks, or the engine's connection hasn't
opened yet)
previously raised `SupersetCancelQueryException` without changing the
query's stored
status, silently leaving it at `Running` forever if the underlying statement
never
completes on its own. This matches reports going back years (#17011, #20950,
#29475,
#31814) where the Stop button appeared to do nothing.
`superset/sql_lab.py::cancel_query()` now records the same "early cancel"
intent that
`db_engine_specs/trino.py`'s own `prepare_cancel_query()` already uses for a
harder
version of this problem (Trino can only obtain a real cancel handle after
the query
starts) as the *default* fallback, instead of requiring each engine to opt
in.
`execute_sql_statements()` now honors that signal at every point before the
statement
would actually run, so the query is correctly marked `Stopped` instead of
silently
continuing to execute.
Once a query has genuinely been dispatched to the database and the engine
still can't
supply a cancel handle, Stop still raises `SupersetCancelQueryException`
exactly as
before — this change does not claim to cancel a query on an engine with no
real
cancellation support; it only fixes the case where nothing was ever actually
sent to
the database yet.
## BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change, no UI change.
## TESTING INSTRUCTIONS
Covered by `tests/unit_tests/dao/test_query_stop_race.py` (new) and updates
to
`tests/unit_tests/sql_lab_test.py`. These deterministically simulate the
race using
separate scoped SQLAlchemy sessions and a real SQLite-backed
`Query`/`Database` pair
(the actual `cancel_query()`/`execute_sql_statements()` code, not mocked),
rather than
relying on real wall-clock timing:
```
pytest tests/unit_tests/dao/test_query_stop_race.py -v
pytest tests/unit_tests/sql_lab_test.py tests/unit_tests/dao/
tests/unit_tests/models/sql_lab_test.py tests/unit_tests/sql/execution/
tests/unit_tests/commands/sql_lab/ -v
```
Manual repro (needs a database with `allow_run_async` enabled and a
validation step
slow enough to create a window before the connection opens, e.g.
`RLS_IN_SQLLAB`):
run a query from a fresh SQL Lab tab and click Stop immediately after
submitting.
## ADDITIONAL INFORMATION
- [ ] Has associated issue: related to (already closed, but the underlying
gap
remains on master) #17011, #20950, #29475, #31814
- [ ] Requires DB Migration
- [ ] Confirm your changes are compatible with databases that are officially
supported by Superset
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
## Known limitations (disclosed, not fixed in this PR)
- **A narrow check-to-commit race remains.** This change closes the
previously
unconditional "no cancel handle = permanently stuck, and the user isn't
told the
truth" failure mode, but it does not add database-level row locking or
optimistic-concurrency versioning. A stop request landing in the brief
window
between one of several status checks and its corresponding commit
(execution
startup, immediately before building the result payload, in the
results-backend-write-failure path, and in `cancel_query()`'s own
dispatched-key
read) can still, rarely, be missed. Closing that fully needs real
row-level locking
verified against a production-shaped database (Postgres/MySQL) — not
verifiable
against SQLite — and is deferred rather than shipped unverified.
- **The newer async SQL execution framework (`superset/sql/execution/`) has
the
identical gap and is unfixed here.** `SQLExecutor._cancel_query()`
(`superset/sql/execution/executor.py`) is a separate implementation from
`superset.sql_lab.cancel_query()` and isn't touched by this PR.
- Tests are verified against SQLite with thread-based simulation of
concurrent
requests, not a real multi-process/multi-connection database under load.
--
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]
