sadpandajoe opened a new pull request, #43535:
URL: https://github.com/apache/superset/pull/43535
### SUMMARY
The SQL Lab Query History pane can show a query as "Running" or "Scheduled"
forever, even after it has finished and the Results pane and the Redux store
both show it as succeeded or failed. It is most visible on a newly opened
tab.
`QueryHistory` merges two sources for a tab's query list:
- the live Redux `sqlLab.queries` slice, which `QueryAutoRefresh` keeps
current
while a query is within `MAX_QUERY_AGE_TO_POLL`, and
- a snapshot fetched once from the `editorQueries` RTK Query endpoint
(`src/hooks/apiResources/queries.ts`), which has no polling, no
refetch-on-focus, and — although it declares `providesTags:
['EditorQueries']`
— no invalidation anywhere in the codebase.
The merge previously discarded the live Redux copy for any id also present in
the snapshot and rendered the snapshot row instead. Because that snapshot is
fetched once per tab and never invalidated, whatever status the backend
reported at that single GET was rendered for the lifetime of the tab,
regardless of what Redux said afterwards. A newly opened tab is the worst
case:
its `QueryHistory` mounts and fires that one-shot fetch at roughly the
moment a
query starts, so it is likely to capture a non-terminal row and freeze it.
This adds `mergeQueryStatus`, which corrects exactly one case: **the live
Redux
copy has reached a concluded state (`concludedQueryStateList`) and the
snapshot
has not.** In that case the live row supplies `state`, `progress`, `rows`,
`startDttm`, `endDttm`, `resultsKey` and `errorMessage`. Every other
combination — both concluded, both non-concluded, or the snapshot already
concluded — returns the snapshot row **unchanged and by reference**, so
behaviour outside the one broken case is identical to before.
Two deliberate details:
- **Timestamps move as a pair.** The backend records `startDttm`/`endDttm` in
server time, while the client stamps `endDttm` from the browser clock when
a
query concludes locally. Taking one from each side can make `endDttm`
earlier than `startDttm`, and `fDuration` has no negative-delta guard, so
the
Duration column would render roughly `23:59:xx`. Whichever side wins
supplies
both.
- **Where both sides have concluded and disagree, the snapshot still wins** —
the pre-existing behaviour. Deciding which of two concluded states is
authoritative is a separate question with real cases on both sides, and
this
change deliberately does not answer it.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no visual or layout change. The fix affects which of two
already-rendered
values populates the existing State, Progress, Rows and Duration cells.
### TESTING INSTRUCTIONS
1. Enable `SQLLAB_BACKEND_PERSISTENCE`.
2. Run a query against a database with `allow_run_async` enabled, from a
newly
opened SQL Lab tab, so the Query History pane's one-shot fetch lands while
the query is still running.
3. Once the query completes, confirm the Query History row shows Success (or
Failed) rather than remaining on Running/Scheduled.
Automated checks run locally:
- `npx jest src/SqlLab/components/QueryHistory/` — 15/15 (8 component, 7
unit)
- `npx jest src/SqlLab/components/QueryTable/QueryTable.test.tsx` — 9/9
- `npx oxlint` and `npx oxfmt --check` on the changed files — clean
The regression guard was verified to fail against the unfixed component for
the
intended reason, and the unit test was verified by mutation: substituting the
wrong merge base makes it fail, naming `queryId`, `tab` and `executedSql`.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags: `SQLLAB_BACKEND_PERSISTENCE`; reproduction also
needs a database with `allow_run_async` enabled
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
**Known limitation, not addressed here.** The `editorQueries` endpoint still
has
no cache invalidation tied to query-state transitions. If a query is evicted
from the Redux store by `CLEAR_INACTIVE_QUERIES` before it concludes, there
is
no live row left to merge and the frozen snapshot row renders as before. That
is a different mechanism with a different fix site, so it is deliberately
out of
scope; this change does not claim to close it.
**Reviewer note.** The `QueryHistory` component tests synchronise on a
snapshot-only `sql` value rather than on the fetch call count, because the
component renders a Redux-only fallback row before the response is applied
and
assertions made at that point can pass without the merge having run at all.
**Known test-coverage residuals (deliberate, not oversights).**
- `mergeQueryStatus`'s `!== undefined` fallback is pinned for `startDttm`,
`endDttm`, `resultsKey` and `errorMessage`, but not for `progress`/`rows` —
every fixture defines both. The `progress` guard is load-bearing, since
`QueryTable` calls `progress.toFixed(0)` unguarded, so dropping it would
crash the row rather than blank it. Closing this is a ~4-line addition to
the
existing fallback test in `mergeQueryStatus.test.ts`.
- The `state` fallback branch in `mergeQueryStatus.ts` is unreachable: the
guard
above it returns early when the local `state` is `undefined`, because
`concludedQueryStateList.includes(undefined)` is `false`. Harmless, and a
candidate for simplification in a later pass.
- Not covered here: the `CLEAR_INACTIVE_QUERIES` eviction path (a query
evicted
from the store leaves no live row to merge, so the frozen snapshot still
renders `Running` — same symptom, different cause, unchanged from
`master`),
the local-only row path, and paging.
--
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]
