aminghadersohi commented on PR #45283: URL: https://github.com/apache/superset/pull/45283#issuecomment-6190755176
The previous commit protected worker terminal writes after Stop committed, but the production Stop path still cancelled the engine before its conditional STOPPED write. The new Postgres reproduction lets the cancelled worker finish before the Stop request resumes; at `e5421d693dddcccac4edc90e486c27a0475c8dd1`, the worker records FAILED and the conditional stop refuses it. Fixed that ordering in `f2e834d1025124a90941a6bd0b668a2b0e4991f0`: commit the guarded STOPPED write before engine cancellation, and skip cancellation if the initial guard refuses an already-completed query. A second guarded write preserves early-cancel metadata without replacing a durable terminal outcome. Legacy and Celery startup RUNNING writes also use the stop guard. Regression results: | Test | Before | After | | --- | --- | --- | | `test_stop_request_precedes_postgres_cancellation_failure` | 12 failed at e5421d69 | 12 passed | | `test_get_sql_results_preserves_stop_during_postgres_cancellation` | 4 failed with the previous legacy production hunks reverted | 4 passed | | `test_legacy_terminal_write_preserves_concurrent_stop` | 2 failed with those hunks reverted, including backend_failure | 2 passed | | `test_legacy_cancel_error_preserves_committed_stop` | 8 failed with those hunks reverted, including all cursor variants | 8 passed | | `test_legacy_startup_preserves_stop_before_running_write` | 1 failed before the startup change | 1 passed | | `test_celery_startup_preserves_stop_before_running_write` | 2 failed before the startup change | 2 passed | The production-order matrix covers QueryDAO and SQLExecutor async cancellation, real `pg_cancel_backend` and `pg_terminate_backend`, and post-cancel delays of **0, 20, and 100 ms**. Each delay also waits for worker completion to exercise the losing schedule deterministically. Replacing only the legacy shared guard with an unconditional commit also makes all 6 unit guard cases and all 8 Postgres guard cases fail. Validation commands and outputs (local Python environment; manual/llm_compliance tests excluded): - `pytest tests/unit_tests/sql_lab_test.py tests/unit_tests/sql/ tests/unit_tests/daos/ -q -m 'not manual and not llm_compliance'` — **2690 passed**. - `pytest tests/testcontainers/db_engine_specs/test_postgres.py -m testcontainers -q` — **22 passed**; selecting `-k test_stop_request_precedes_postgres_cancellation_failure` separately — **12 passed, 10 deselected**. - `pytest tests/integration_tests/queries/api_tests.py -k test_stop_query -q` — **3 passed, 12 deselected**. - `pre-commit run` on the staged changed files — **passed**, including mypy, ruff, and pylint. Siblings checked: - `superset/sql/execution/executor.py` — fixed in f2e834d1025124a90941a6bd0b668a2b0e4991f0 - `superset/sql_lab.py` — fixed in f2e834d1025124a90941a6bd0b668a2b0e4991f0 - `superset/sql/execution/celery_task.py` — fixed in f2e834d1025124a90941a6bd0b668a2b0e4991f0 - `superset/db_engine_specs/trino.py` — not affected: early-cancel preparation remains guarded by both stop callers; terminal metadata preservation and successful early cancellation are tested. - `superset/db_engine_specs/impala.py` — not affected: the same guarded early-cancel paths are tested. - `superset/queries/api.py` — not affected: completed-query HTTP 422 responses remain covered by unit and integration stop tests. -- 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]
