feat(async): cancel running chart queries under GLOBAL_ASYNC_QUERIES#42305
feat(async): cancel running chart queries under GLOBAL_ASYNC_QUERIES#42305msyavuz wants to merge 1 commit into
Conversation
dosubot
Bot
added
api
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #0d77a5
Actionable Suggestions - 1
-
superset-frontend/src/middleware/asyncEvent.test.ts - 1
- Missing microtask flush for fire-and-forget POST · Line 166-166
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/middleware/asyncEvent.test.ts - 2
- Missing type annotations on test · Line 133-154
- Missing type annotations on test · Line 156-167
-
superset/async_events/cache_backend.py - 1
- Duplicated get method across classes · Line 102-109
Review Details
-
Files reviewed - 9 · Commit Range:
464aa02..464aa02- superset-frontend/src/components/Chart/chartAction.ts
- superset-frontend/src/middleware/asyncEvent.test.ts
- superset-frontend/src/middleware/asyncEvent.ts
- superset/async_events/api.py
- superset/async_events/async_query_manager.py
- superset/async_events/cache_backend.py
- superset/tasks/async_queries.py
- tests/unit_tests/async_events/async_query_manager_tests.py
- tests/unit_tests/tasks/test_async_queries.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| await expect( | ||
| asyncEvent.waitForAsyncData(asyncPendingEvent, controller.signal), | ||
| ).rejects.toMatchObject({ name: 'AbortError' }); | ||
| expect(fetchMock.callHistory.calls(CANCEL_ENDPOINT)).toHaveLength(1); |
There was a problem hiding this comment.
The cancel POST is fire-and-forget per implementation (cancelAsyncJob at asyncEvent.ts:105-112), but this test doesn't flush microtasks before asserting the endpoint was called, unlike the equivalent test at line 152. This inconsistency can cause flaky test failures in CI environments where async operations complete after assertions run.
Code Review Run #0d77a5
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| raw = self._cache.get(key) | ||
| if raw is None: | ||
| raise AsyncQueryJobException("Job not found or already completed") | ||
|
|
||
| record = json.loads(raw) | ||
| if record.get("channel_id") != channel_id or record.get("user_id") != user_id: | ||
| raise AsyncQueryTokenException("Not authorized to cancel this job") | ||
|
|
||
| # Flag before revoking so the worker's timeout handler, which may fire | ||
| # almost immediately, reliably sees the cancellation. | ||
| self._cache.set( | ||
| key, | ||
| json.dumps({**record, "cancelled": True}), | ||
| ex=self._jwt_expiration_seconds or None, | ||
| ) |
There was a problem hiding this comment.
Suggestion: There is a race condition between reading the job record and writing the cancellation flag: if the worker finishes and deletes the registry key in between, this code recreates the key and still returns success, so callers can “cancel” already-finished jobs and revoke is sent for stale state. Make this update atomic (for example, conditional write only if the key still exists and fail otherwise, ideally in a Redis transaction/Lua script). [race condition]
Severity Level: Major ⚠️
- ❌ Cancel endpoint reports success cancelling already-completed async jobs.
- ⚠️ Workers may see stale cancellation flag for finished jobs.Steps of Reproduction ✅
1. In `superset/async_events/async_query_manager.py:338-363`, a client starts an async
chart query via `AsyncQueryManager.submit_chart_data_job(...)`, which calls
`init_job(...)` at `281-286`; `init_job` registers the cancellable job in Redis through
`_register_cancellable_job(...)` at `292-307`.
2. The Celery task created by `_load_chart_data_into_cache_job.apply_async(...)` at
`352-362` runs and, on successful completion, calls `AsyncQueryManager.update_job(...)` at
`392-421` with `status=self.STATUS_DONE`, which emits the terminal event and deletes the
job registry key via `self._cache.delete(self._job_registry_key(job_id))` at `419-421`.
3. Around the same time the client presses Stop in Explore, hitting the new cancel
endpoint described in the PR, which calls `AsyncQueryManager.cancel_job(...)` at
`435-475`; inside `cancel_job`, `key = self._job_registry_key(job_id)` (line 452) and `raw
= self._cache.get(key)` (line 453) read the current record before the `update_job` call in
step 2 performs its delete.
4. After `update_job` deletes the key, `cancel_job` still proceeds to
`self._cache.set(...)` at `463-467`, recreating the key with `{"cancelled": True}` and
returning success while the job is already completed; `celery_app.control.revoke(job_id,
...)` at `475` is then issued for a finished task, so from the caller’s perspective a
“cancel” appears to succeed based on stale state instead of failing with “Job not found or
already completed”.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/async_events/async_query_manager.py
**Line:** 453:467
**Comment:**
*Race Condition: There is a race condition between reading the job record and writing the cancellation flag: if the worker finishes and deletes the registry key in between, this code recreates the key and still returns success, so callers can “cancel” already-finished jobs and revoke is sent for stale state. Make this update atomic (for example, conditional write only if the key still exists and fail otherwise, ideally in a Redis transaction/Lua script).
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
464aa02 to
27d02a4
Compare
There was a problem hiding this comment.
Code Review Agent Run #670063
Actionable Suggestions - 1
-
superset/async_events/api.py - 1
- Missing test coverage for cancel API · Line 107-171
Additional Suggestions - 2
-
superset/async_events/api.py - 1
-
CWE-285: Reused permission on cancel · Line 107-113The `cancel` endpoint reuses `permission_name="list"`, which is the same permission used by the `events` read-only endpoint. Since these are distinct operations (read vs. cancel), the cancel action should have its own permission name to allow fine-grained access control.
-
-
superset-frontend/src/middleware/asyncEvent.test.ts - 1
-
Duplicated mock setup across tests · Line 133-154Lines 134-135 in the first test and lines 157-158 in the second test duplicate identical CANCEL_ENDPOINT constant and fetchMock.post setup. Consider moving this to a beforeEach within the describe block for consistency with the EVENTS_ENDPOINT and CACHED_DATA_ENDPOINT patterns at lines 88-89.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/middleware/asyncEvent.ts - 1
- Promise hangs on unexpected status · Line 158-158
-
tests/unit_tests/tasks/test_async_queries.py - 1
- Missing cancellation test for explore_json path · Line 73-91
Review Details
-
Files reviewed - 9 · Commit Range:
27d02a4..27d02a4- superset-frontend/src/components/Chart/chartAction.ts
- superset-frontend/src/middleware/asyncEvent.test.ts
- superset-frontend/src/middleware/asyncEvent.ts
- superset/async_events/api.py
- superset/async_events/async_query_manager.py
- superset/async_events/cache_backend.py
- superset/tasks/async_queries.py
- tests/unit_tests/async_events/async_query_manager_tests.py
- tests/unit_tests/tasks/test_async_queries.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| @expose("/<job_id>/cancel", methods=("POST",)) | ||
| @event_logger.log_this | ||
| @protect() | ||
| @safe | ||
| @statsd_metrics | ||
| @permission_name("list") | ||
| def cancel(self, job_id: str) -> Response: | ||
| """Cancel a running async query job. | ||
| --- | ||
| post: | ||
| summary: Cancel a running async query job | ||
| description: >- | ||
| Revokes the Celery task backing an in-flight async query. The | ||
| caller is authorized against the job's original owner (channel and | ||
| user), both resolved server-side from the request, so a client | ||
| cannot cancel a job it did not submit. | ||
| parameters: | ||
| - in: path | ||
| name: job_id | ||
| required: true | ||
| description: The job ID returned when the async query was submitted | ||
| schema: | ||
| type: string | ||
| responses: | ||
| 200: | ||
| description: Job cancelled | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| result: | ||
| type: object | ||
| properties: | ||
| job_id: | ||
| type: string | ||
| status: | ||
| type: string | ||
| 401: | ||
| $ref: '#/components/responses/401' | ||
| 403: | ||
| $ref: '#/components/responses/403' | ||
| 404: | ||
| $ref: '#/components/responses/404' | ||
| 500: | ||
| $ref: '#/components/responses/500' | ||
| """ | ||
| try: | ||
| async_channel_id = async_query_manager.parse_channel_id_from_request( | ||
| request | ||
| ) | ||
| except AsyncQueryTokenException: | ||
| return self.response_401() | ||
|
|
||
| try: | ||
| async_query_manager.cancel_job(job_id, async_channel_id, get_user_id()) | ||
| except AsyncQueryTokenException: | ||
| return self.response_403() | ||
| except AsyncQueryJobException: | ||
| return self.response_404() | ||
|
|
||
| return self.response( | ||
| 200, | ||
| result={"job_id": job_id, "status": async_query_manager.STATUS_CANCELLED}, | ||
| ) |
There was a problem hiding this comment.
The new cancel endpoint has no test coverage in api_tests.py. The existing async_query_manager_tests.py only tests the manager layer, not the API's request parsing, exception handling, and response mapping. Without API-level tests, regression in the cancel flow (error handling, response codes, authorization) will go undetected.
Code Review Run #670063
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42305 +/- ##
==========================================
- Coverage 65.15% 56.61% -8.54%
==========================================
Files 2789 2789
Lines 157572 157650 +78
Branches 35872 35887 +15
==========================================
- Hits 102662 89251 -13411
- Misses 52939 67566 +14627
+ Partials 1971 833 -1138
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
SUMMARY
Under
GLOBAL_ASYNC_QUERIES(GAQ), the Stop button in Explore did nothing: once a query returns202and the client is polling for the async result, aborting the request has no effect — the fetch already settled, so noAbortErrorfires,chartUpdateStoppednever dispatches, and the button stays stuck while the query keeps running server-side. This is the case the reporting customer (GAQ enabled) actually hits.This makes Stop work under GAQ, on both ends:
Frontend — thread the request's
AbortSignalintowaitForAsyncData. On abort it stops listening for the job, POSTs the cancel endpoint, and rejects with anAbortErrorso the existing abort handling dispatcheschartUpdateStopped. The stale-response guards now checkcontroller.signal.aborted(authoritative — the reducer nullsqueryControlleron stop) instead of only comparing controllers.Backend — a new
POST /api/v1/async_event/<job_id>/cancelrevokes the Celery task. The job'sjob_idis now used as the Celerytask_id, so the id the client already holds is directly revocable.revoke(..., signal="SIGUSR1")surfaces asSoftTimeLimitExceededin the worker, which reads a cancel flag and emits a single terminalcancelledevent.Authorization — the endpoint never trusts the client-supplied
job_id. At submit time a Redis record bindsjob_id → {channel_id, user_id}; cancel resolves the caller's channel and user id server-side and must match both. Channel-and-user (not channel alone) matters for embedded guests, whose channel can be shared across viewers.The resulting
stoppedstate is rendered by #42270 (merged).BEFORE/AFTER
Before (GAQ): Stop → button stuck on "Stop", query keeps running, eventually a blank chart.
After (GAQ): Stop → button returns to "Update chart", the query is cancelled server-side, and the stopped message shows (with #42270).
TESTING INSTRUCTIONS
GLOBAL_ASYNC_QUERIESwith Redis + a Celery worker.SELECT pg_sleep(20)).Cachedbadge) and click Stop while it loads.Unit tests:
pytest tests/unit_tests/async_events/ tests/unit_tests/tasks/test_async_queries.py;npm run test -- asyncEvent.test.ts chartActions.test.ts.ADDITIONAL INFORMATION
GLOBAL_ASYNC_QUERIESPOST /api/v1/async_event/<job_id>/cancel