You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/08/26 05:23:23 UTC
(superset) branch fix-sqllab-query-history-stuck-running-new-tab created (now 08ca8ff7fdb)
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a change to branch fix-sqllab-query-history-stuck-running-new-tab
in repository https://gitbox.apache.org/repos/asf/superset.git
at 08ca8ff7fdb test(sqllab): make query history merge tests lifecycle-aware and enforce their own invariants
This branch includes the following new commits:
new d894b135aba test(sqllab): add failing guard for query history status precedence
new 6ba21f8d83e docs: add RCA for SQL Lab query history stuck in Running/Scheduled
new 1cea2282bfa fix(sqllab): prefer live query state over stale history snapshot
new bb88af6f8f6 fix(sqllab): make query history precedence lifecycle-aware
new f3ae25fd4d6 fix(sqllab): narrow query history merge to one quadrant, extract and unit-test it
new 45940aacb0a test(sqllab): make query history merge tests distinguish base object and backend-realistic
new 08ca8ff7fdb test(sqllab): make query history merge tests lifecycle-aware and enforce their own invariants
The 7 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/07: test(sqllab): add failing guard for query history status precedence
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-sqllab-query-history-stuck-running-new-tab
in repository https://gitbox.apache.org/repos/asf/superset.git
commit d894b135abafea989bd057a9d43437f33c4f7d7b
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Aug 26 00:14:24 2026 +0000
test(sqllab): add failing guard for query history status precedence
QueryHistory merges a one-shot backend query-history snapshot over the
live Redux state, letting the snapshot win for any query id present in
both. Since the snapshot's RTK Query cache entry is never invalidated,
whatever status the backend reported at mount time renders forever,
even after Redux has correctly moved the query to a terminal state.
This adds a regression guard that seeds Redux with a finished query and
mocks the backend snapshot with an in-flight copy of the same id, then
asserts the live value renders. It currently fails, reproducing the bug.
---
.../components/QueryHistory/QueryHistory.test.tsx | 52 ++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
index cea251cb9cc..da1735a7746 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
@@ -252,6 +252,58 @@ test('displays multiple queries with newest query first', async () => {
isFeatureEnabledMock.mockClear();
});
+test('renders the live Redux state instead of a stale backend snapshot for the same query', async () => {
+ const isFeatureEnabledMock = mockedIsFeatureEnabled.mockImplementation(
+ featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
+ );
+
+ const staleApiResult = {
+ count: 1,
+ ids: [692],
+ result: [
+ {
+ ...fakeApiResult.result[0],
+ client_id: 'stuckClientId',
+ status: QueryState.Running,
+ progress: 0,
+ rows: 0,
+ sql_editor_id: defaultQueryEditor.id,
+ },
+ ],
+ };
+
+ const editorQueryApiRoute = `glob:*/api/v1/query/?q=*`;
+ fetchMock.get(editorQueryApiRoute, staleApiResult);
+
+ const stateWithLiveQuery = {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ stuckClientId: {
+ id: 'stuckClientId',
+ sqlEditorId: defaultQueryEditor.id,
+ sql: 'SELECT 1',
+ state: QueryState.Success,
+ startDttm: 1710273662445,
+ progress: 100,
+ rows: 443,
+ },
+ },
+ },
+ };
+
+ render(setup(), { useRedux: true, initialState: stateWithLiveQuery });
+
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
+ );
+
+ expect(screen.getByText('443')).toBeInTheDocument();
+
+ isFeatureEnabledMock.mockClear();
+});
+
test('renders contributed toolbar action in queryHistory slot', () => {
registerToolbarAction(
ViewLocations.sqllab.queryHistory,
(superset) 06/07: test(sqllab): make query history merge tests distinguish base object and backend-realistic
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-sqllab-query-history-stuck-running-new-tab
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 45940aacb0a4ebe884341f5d633cb62b21b135b8
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Aug 26 05:05:01 2026 +0000
test(sqllab): make query history merge tests distinguish base object and backend-realistic
The unit test fixtures differed only on the seven fields the merge
overwrites, so a wrong-base implementation ({...localQuery, <bundle>}
instead of {...remoteQuery, <bundle>}) was structurally equal to the
correct one and passed all 6 unit tests and all 8 component tests
undetected, silently stripping backend-only metadata (queryId, user,
db, executedSql...) in production. Verified with a mutation test
(temporarily swapping the base, confirming 14/14 still passed) before
fixing it: give the two fixtures distinct backend-only metadata
outside the override bundle and add an explicit test asserting the
merged row keeps the snapshot's queryId/tab/executedSql. Re-ran the
same mutation afterward to confirm it now fails.
Also replace the component guard's Duration-cell/endDttm barrier with
a sql-text barrier. A real Redux row that has concluded always has an
endDttm (QUERY_SUCCESS sets both together), so the previous barrier's
premise - that the live-only fixture has no endDttm - was already
false of production data and would go vacuous the moment a fixture
became realistic. `sql` is never part of the merge's overlay bundle,
so a merged row's sql always comes from the snapshot base regardless
of whether an override happened, making it a barrier that holds
independent of any fixture's timestamp fields. Made the `Running`
snapshot fixtures backend-realistic (end_time: null) now that the
barrier no longer depends on them carrying a value, and added
realistic endDttm to the concluded live-query fixtures to match what
QUERY_SUCCESS actually produces.
---
.../components/QueryHistory/QueryHistory.test.tsx | 44 ++++++++++++++++------
.../QueryHistory/mergeQueryStatus.test.ts | 26 +++++++++++++
2 files changed, 58 insertions(+), 12 deletions(-)
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
index 5b091156302..bbe93af78d2 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
@@ -252,21 +252,28 @@ test('displays multiple queries with newest query first', async () => {
isFeatureEnabledMock.mockClear();
});
-// The `QueryTable` Duration cell only renders when `q.endDttm` is set, and
-// the live-only Redux fixtures below deliberately omit `endDttm` (a
-// client-started query never has one until the backend supplies it). So
-// `findByText(/^00:00:00\./)` can only resolve once the backend snapshot
-// has actually loaded *and* been folded into the rendered row - unlike
-// `waitFor(() => calls.length === 1)`, which resolves as soon as the
-// request is issued, while `data` is still `undefined` and the component
-// is still rendering the pre-merge, Redux-only fallback.
-const findDurationCell = () => screen.findByText(/^00:00:00\./);
+// `sql` is never part of the merge's overlay bundle, so a merged row's `sql`
+// always comes from the `{...remoteQuery}` base, whether or not an override
+// happened. Every live-only Redux fixture below uses `sql: 'SELECT 1'`,
+// while the backend snapshot uses this distinctive query text - so this can
+// only resolve once the backend response has actually loaded *and* been
+// folded into the rendered row, unlike `waitFor(() => calls.length === 1)`,
+// which resolves as soon as the request is issued, while `data` is still
+// `undefined` and the component is still rendering the pre-merge,
+// Redux-only fallback. Deliberately not a Duration-cell/`endDttm` barrier:
+// a real Redux row that has concluded always has an `endDttm` (see
+// `QUERY_SUCCESS` in `reducers/sqlLab.ts`), so that barrier would silently
+// go vacuous the moment a fixture became realistic about timestamps.
+const findRemoteSqlCell = () => screen.findByText(/FCC 2018 Survey/);
test('overrides a stale non-concluded backend snapshot with a concluded live Redux state', async () => {
const isFeatureEnabledMock = mockedIsFeatureEnabled.mockImplementation(
featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
);
+ // A non-concluded row's `end_time` is never set by the backend (every
+ // write of `end_time` is paired with a concluded status - see
+ // `superset/sql_lab.py` and `superset/daos/query.py`).
const staleApiResult = {
count: 1,
ids: [692],
@@ -277,6 +284,7 @@ test('overrides a stale non-concluded backend snapshot with a concluded live Red
status: QueryState.Running,
progress: 0,
rows: 0,
+ end_time: null,
sql_editor_id: defaultQueryEditor.id,
},
],
@@ -296,6 +304,9 @@ test('overrides a stale non-concluded backend snapshot with a concluded live Red
sql: 'SELECT 1',
state: QueryState.Success,
startDttm: 1710273662445,
+ // A real Redux row at Success always has an endDttm too -
+ // QUERY_SUCCESS sets both together.
+ endDttm: 1710273662500,
progress: 100,
rows: 443,
},
@@ -308,7 +319,7 @@ test('overrides a stale non-concluded backend snapshot with a concluded live Red
await waitFor(() =>
expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
);
- await findDurationCell();
+ await findRemoteSqlCell();
const row = screen.getByText('443').closest('tr') as HTMLElement;
expect(within(row).getByLabelText('check')).toBeInTheDocument();
@@ -367,7 +378,7 @@ test('does not override an already-concluded backend snapshot with a non-conclud
await waitFor(() =>
expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
);
- await findDurationCell();
+ await findRemoteSqlCell();
const row = screen.getByText('443').closest('tr') as HTMLElement;
expect(within(row).getByLabelText('check')).toBeInTheDocument();
@@ -391,6 +402,8 @@ test('renders a backend-only historical query the client never ran, alongside a
status: QueryState.Running,
progress: 0,
rows: 0,
+ // Non-concluded: the backend never sets end_time for this status.
+ end_time: null,
sql_editor_id: defaultQueryEditor.id,
},
{
@@ -402,6 +415,10 @@ test('renders a backend-only historical query the client never ran, alongside a
rows: 12,
sql_editor_id: defaultQueryEditor.id,
start_time: '1710273660000.000000',
+ // A different table than the live row's, so findRemoteSqlCell's
+ // target text is unique to that row, not duplicated on this one.
+ sql: 'SELECT * from "Population"',
+ executed_sql: 'SELECT * from "Population"\nLIMIT 1001',
},
],
};
@@ -420,6 +437,9 @@ test('renders a backend-only historical query the client never ran, alongside a
sql: 'SELECT 1',
state: QueryState.Success,
startDttm: 1710273662445,
+ // A real Redux row at Success always has an endDttm too -
+ // QUERY_SUCCESS sets both together.
+ endDttm: 1710273662500,
progress: 100,
rows: 443,
},
@@ -435,7 +455,7 @@ test('renders a backend-only historical query the client never ran, alongside a
await waitFor(() =>
expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
);
- await findDurationCell();
+ await findRemoteSqlCell();
const tableRows = container.querySelectorAll(
'table > tbody > tr:not(.ant-table-measure-row)',
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts b/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts
index 72b93dd800d..c1afe6a2b84 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts
@@ -19,8 +19,18 @@
import { QueryState, testQueryResponse } from '@superset-ui/core';
import { mergeQueryStatus } from './mergeQueryStatus';
+// remoteBase and localBase deliberately differ on backend-only metadata
+// (queryId, tab, executedSql) that the merge bundle never touches, not just
+// on the seven bundle fields. If they only differed on the bundle fields,
+// `{...remoteQuery, <bundle>}` and `{...localQuery, <bundle>}` would be
+// structurally equal and toEqual/toBe could not tell a correct base from a
+// wrong one — see the "keeps the snapshot's backend-only metadata" test
+// below, which exists specifically to catch that class of regression.
const remoteBase = {
...testQueryResponse,
+ queryId: 692,
+ tab: 'Untitled Query 16',
+ executedSql: 'SELECT * from "FCC 2018 Survey"\nLIMIT 1001',
state: QueryState.Running,
progress: 0,
rows: 0,
@@ -32,6 +42,9 @@ const remoteBase = {
const localBase = {
...testQueryResponse,
+ queryId: undefined as unknown as number,
+ tab: 'stale local tab',
+ executedSql: undefined as unknown as string,
state: QueryState.Success,
progress: 100,
rows: 443,
@@ -117,3 +130,16 @@ test('local concluded, remote not: a null local field overrides a remote value (
expect(merged.resultsKey).toBeNull();
expect(merged.errorMessage).toBeNull();
});
+
+test('local concluded, remote not: keeps the snapshot-only metadata (queryId, tab, executedSql)', () => {
+ const remote = { ...remoteBase, state: QueryState.Running };
+ const local = { ...localBase, state: QueryState.Success };
+
+ const merged = mergeQueryStatus(remote, local);
+
+ expect(merged.queryId).toBe(692);
+ expect(merged.tab).toBe('Untitled Query 16');
+ expect(merged.executedSql).toBe(
+ 'SELECT * from "FCC 2018 Survey"\nLIMIT 1001',
+ );
+});
(superset) 07/07: test(sqllab): make query history merge tests lifecycle-aware and enforce their own invariants
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-sqllab-query-history-stuck-running-new-tab
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 08ca8ff7fdb24489aae8427cd07ec2ef9789bc2e
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Aug 26 05:16:37 2026 +0000
test(sqllab): make query history merge tests lifecycle-aware and enforce their own invariants
The backend-realistic fixture pass removed the last coverage of the
!== undefined fallback branches: every component fixture now defines
startDttm/endDttm, so the mutation `endDttm: localQuery.endDttm`
(guard dropped) survived all 15 tests. Widen the existing "undefined
local fields fall back" unit test to also cover startDttm/endDttm,
which protects a real path - getInitialState.ts rehydrates endDttm
conditionally, so a concluded row can reach the merge with it
undefined and must fall back to the snapshot's value or its Duration
column silently blanks. Verified by re-running the same
guard-dropped mutation: it now fails, confirming the widened test
catches it; reverted afterward.
The component tests' sql-text barrier holds only because the live
fixtures' sql differs from the snapshot's; in production both are the
same query and would carry the same sql, so a contributor making a
fixture more realistic could silently re-vacuum the barrier the same
way the previous Duration-based one went vacuous. Add an assertion
enforcing that invariant before each render, so a future violation
fails loudly instead of silently passing. Also note in the fixture
comments that end_time: null maps to endDttm: 0, not undefined, per
mapQueryResponse's Number(query.end_time) coercion.
---
.../components/QueryHistory/QueryHistory.test.tsx | 22 ++++++++++++++++++++--
.../QueryHistory/mergeQueryStatus.test.ts | 6 ++++++
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
index bbe93af78d2..a56782d0909 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
@@ -266,6 +266,12 @@ test('displays multiple queries with newest query first', async () => {
// go vacuous the moment a fixture became realistic about timestamps.
const findRemoteSqlCell = () => screen.findByText(/FCC 2018 Survey/);
+// The barrier above holds only while the live fixture's sql differs from the
+// snapshot's. If they ever match, findRemoteSqlCell() resolves pre-merge and
+// every assertion after it goes vacuous. Fail loudly rather than silently.
+const assertLiveSqlDiffersFromSnapshot = (q: { sql: string }) =>
+ expect(q.sql).not.toMatch(/FCC 2018 Survey/);
+
test('overrides a stale non-concluded backend snapshot with a concluded live Redux state', async () => {
const isFeatureEnabledMock = mockedIsFeatureEnabled.mockImplementation(
featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
@@ -273,7 +279,9 @@ test('overrides a stale non-concluded backend snapshot with a concluded live Red
// A non-concluded row's `end_time` is never set by the backend (every
// write of `end_time` is paired with a concluded status - see
- // `superset/sql_lab.py` and `superset/daos/query.py`).
+ // `superset/sql_lab.py` and `superset/daos/query.py`). Note this maps to
+ // `endDttm: 0`, not `undefined` - `mapQueryResponse` does
+ // `Number(query.end_time)` and `Number(null) === 0`.
const staleApiResult = {
count: 1,
ids: [692],
@@ -314,6 +322,9 @@ test('overrides a stale non-concluded backend snapshot with a concluded live Red
},
};
+ assertLiveSqlDiffersFromSnapshot(
+ stateWithLiveQuery.sqlLab.queries.stuckClientId,
+ );
render(setup(), { useRedux: true, initialState: stateWithLiveQuery });
await waitFor(() =>
@@ -373,6 +384,9 @@ test('does not override an already-concluded backend snapshot with a non-conclud
},
};
+ assertLiveSqlDiffersFromSnapshot(
+ stateWithScheduledQuery.sqlLab.queries.scheduledClientId,
+ );
render(setup(), { useRedux: true, initialState: stateWithScheduledQuery });
await waitFor(() =>
@@ -402,7 +416,8 @@ test('renders a backend-only historical query the client never ran, alongside a
status: QueryState.Running,
progress: 0,
rows: 0,
- // Non-concluded: the backend never sets end_time for this status.
+ // Non-concluded: the backend never sets end_time for this status
+ // (maps to endDttm: 0, not undefined - see the comment above).
end_time: null,
sql_editor_id: defaultQueryEditor.id,
},
@@ -447,6 +462,9 @@ test('renders a backend-only historical query the client never ran, alongside a
},
};
+ assertLiveSqlDiffersFromSnapshot(
+ stateWithOnlyOneLiveQuery.sqlLab.queries.liveClientId,
+ );
const { container } = render(setup(), {
useRedux: true,
initialState: stateWithOnlyOneLiveQuery,
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts b/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts
index c1afe6a2b84..74f8136eaf5 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts
@@ -95,18 +95,24 @@ test('local concluded, remote not: undefined local fields fall back to the remot
const remote = {
...remoteBase,
state: QueryState.Running,
+ startDttm: 1000,
+ endDttm: 1500,
resultsKey: 'remote-results-key',
errorMessage: 'remote error',
};
const local = {
...localBase,
state: QueryState.Success,
+ startDttm: undefined as unknown as number,
+ endDttm: undefined as unknown as number,
resultsKey: undefined as unknown as string,
errorMessage: undefined as unknown as string,
};
const merged = mergeQueryStatus(remote, local);
+ expect(merged.startDttm).toBe(1000);
+ expect(merged.endDttm).toBe(1500);
expect(merged.resultsKey).toBe('remote-results-key');
expect(merged.errorMessage).toBe('remote error');
});
(superset) 04/07: fix(sqllab): make query history precedence lifecycle-aware
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-sqllab-query-history-stuck-running-new-tab
in repository https://gitbox.apache.org/repos/asf/superset.git
commit bb88af6f8f6c0c5a65bfa2bd8a4610a505c511f9
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Aug 26 04:20:52 2026 +0000
fix(sqllab): make query history precedence lifecycle-aware
The prior fix let the live Redux copy win unconditionally, which just
reversed the original defect: once QueryAutoRefresh stops polling a
query (past MAX_QUERY_AGE_TO_POLL), a Redux row stuck at Running could
now mask a genuinely concluded backend snapshot. Make the merge
lifecycle-aware instead: whichever side (backend snapshot or live
Redux) reports the more advanced (concluded) state supplies the whole
status bundle (state/progress/rows/endDttm/resultsKey/errorMessage/
executedSql) together, so a row is never rendered with a concluded
state paired with another source's stale progress or row count.
Replace the prior whole-object spread, which let backend-only
metadata (queryId/user/db) survive only by accident and clobbered
backend startDttm/endDttm with browser-clock values, with this
explicit status-field overlay.
Extend the regression guard to assert the State column directly
(the reported symptom) rather than only the rows count, and add
coverage for the reverse precedence direction and for a backend-only
historical row rendered alongside a live one.
Remove RCA.md; its content belongs in the PR description.
---
RCA.md | 124 -------------------
.../components/QueryHistory/QueryHistory.test.tsx | 134 ++++++++++++++++++++-
.../src/SqlLab/components/QueryHistory/index.tsx | 72 +++++++++--
3 files changed, 195 insertions(+), 135 deletions(-)
diff --git a/RCA.md b/RCA.md
deleted file mode 100644
index 30b9c4aa938..00000000000
--- a/RCA.md
+++ /dev/null
@@ -1,124 +0,0 @@
-## What Happened
-
-In SQL Lab, a query run in a new/second tab can display "Running" or
-"Scheduled" in the Query History panel forever, even though the query has
-long since finished and the Results pane / Redux store correctly show it as
-succeeded (or failed). (verified — reproduced via the regression guard added
-in this branch)
-
-## Root Cause
-
-`QueryHistory` (`superset-frontend/src/SqlLab/components/QueryHistory/index.tsx:85-103`)
-merges two sources of truth for the query list of a tab:
-
-- the live Redux `sqlLab.queries` slice, which `QueryAutoRefresh` keeps
- correctly up to date as a query transitions `running -> success`/`failed`
- (verified), and
-- a one-shot snapshot fetched from the `useEditorQueriesQuery` RTK Query
- endpoint (`superset-frontend/src/hooks/apiResources/queries.ts:112-176`).
-
-The merge `useMemo` computes, for any query id that appears in the backend
-snapshot, `omit(liveQueries, snapshotIds).concat(snapshotResult)` — i.e. it
-drops the live Redux copy for that id and renders the snapshot row instead
-(verified, `index.tsx:88-95`). Because `editorQueries` (the query-history RTK
-API slice, tag `EditorQueries`) is never invalidated anywhere in the codebase
-— no mutation, action, or component calls `invalidateTags(['EditorQueries'])`,
-and the endpoint has no `pollingInterval`, `refetchOnFocus`, or
-`refetchOnMountOrArgChange` (verified, `queries.ts:110-175`) — the snapshot is
-fetched exactly once when the tab/editor mounts and is cached by RTK Query for
-the lifetime of that cache entry. Whatever status the backend happened to
-report at that single GET is what renders in the State/Progress/Rows columns
-for as long as the tab stays open, regardless of what Redux says afterward.
-
-`QueryTable` (`superset-frontend/src/SqlLab/components/QueryTable/index.tsx:264`)
-maps the (now-frozen) status to a label: `running` -> "Running", and both
-`pending` and `scheduled` -> "Scheduled" (verified, `index.tsx:185-239`),
-which is exactly the reported symptom.
-
-This is most visible on a **new tab** because a freshly opened SQL Lab tab is
-the case most likely to mount `QueryHistory` (and therefore fire the one-shot
-snapshot fetch) at almost the same moment a query is started, maximizing the
-odds that the snapshot is captured while the query is still `running`/
-`pending`/`scheduled` before it ever gets a second chance to refresh
-(inferred — consistent with the "stuck on new tab" framing of the symptom,
-not independently reproduced against the mount-timing race itself).
-
-## Why It Wasn't Caught
-
-- The existing tests in `QueryHistory.test.tsx` only exercise the case where
- the backend snapshot and Redux state agree (same `client_id`, same
- `status`, same `rows`) or where they cover disjoint ids. None of them seed
- Redux with a *live, more-current* copy of a row that also appears in the
- mocked backend snapshot, which is the only shape that exposes this bug
- (verified — read the full pre-existing test file before adding the new
- test).
-- `apache/superset#42896` fixed the Redux reducer to correctly allow a
- `Running -> Success` transition, which fixed the underlying Redux state.
- That fix is real and necessary, but it doesn't help here because
- `QueryHistory` doesn't render Redux state for any id the backend snapshot
- also mentions — it renders the snapshot's copy instead (verified).
-- There is no integration/e2e test that opens a new tab, starts a query, and
- asserts the History panel status transitions all the way to a terminal
- state (inferred — no such test was found in the areas searched).
-
-## The Fix
-
-Not applied in this branch (see PR description / Critical boundary note).
-Intended production change, to be made in a follow-up PR: in
-`superset-frontend/src/SqlLab/components/QueryHistory/index.tsx`, the merge
-should let the live Redux copy win over the backend snapshot for any id
-present in both, e.g. build the merged list from
-`data.result` overlaid with `queries` (live wins) rather than
-`omit(queries, snapshotIds).concat(data.result)` (snapshot wins). The
-`editorId`/`status` fields also need to come from Redux when a live entry
-exists, not just `rows`/`progress`, since the State and Progress columns
-render from the same merged object.
-
-## Latent Bugs Found
-
-The RCA panel identified the following issues while investigating this bug.
-All are deliberately out of scope for this change and are recorded here for
-future follow-up:
-
-- `superset-frontend/src/hooks/apiResources/queries.ts` — the `merge`
- function (`currentCache.result.push(...newItems.result)`) always appends
- rather than replacing/de-duplicating by id. A refetch of a page whose
- underlying rows have changed (e.g. pagination re-fetch racing a mutation)
- would duplicate rows in the RTK Query cache rather than reconcile them.
-- `superset-frontend/src/SqlLab/components/QueryHistory/index.tsx` — `pageIndex`
- state is not reset when `editorId` changes, so switching tabs after having
- scrolled to page N+1 on the previous tab carries the stale page index into
- the new tab's query.
-- `superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx` — the
- periodic (5s) save timer that POSTs to `/tabstateview/` has no in-flight
- guard; a slow response can let a second save fire before the first
- resolves, creating a duplicate TabState.
-- `superset-frontend/src/SqlLab/reducers/sqlLab.ts:119` — the
- `CLONE_QUERY_TO_NEW_TAB` reducer case handler is unreachable dead code: the
- corresponding thunk in `actions/sqlLab.ts` has drifted and no longer
- dispatches that action type, so the reducer branch never executes.
-- `superset-frontend/src/SqlLab/components/QueryTable/index.tsx:335,340` —
- `progress.toFixed(0)` is called unguarded; a `null`/`undefined` `progress`
- value (as can occur on some query states) will throw.
-- `syncQueryEditor` snapshots `queriesToMigrate` before awaiting the
- `POST /tabstateview/` call, so a query started inside that async window is
- never migrated. This is narrow and out of scope for the bug fixed here
- (`EditorAutoSync`'s save timer fires roughly every 5s, well after
- `autorun` would have started a query), but is a real, separate race.
-
-## Prevention
-
-- The added regression guard
- (`superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx`)
- seeds Redux with a live, terminal-state copy of a query whose id also
- appears in a mocked in-flight backend snapshot, and asserts the live value
- renders. This is the minimal repro shape that would have caught this bug
- and will catch any regression of the same kind.
-- Once the production fix lands, add an assertion (or a follow-up test) that
- the merged list also carries over the live `status`/`state`, not just
- `rows`, since a fix that only reconciles one field could reintroduce the
- same class of bug for the State/Progress columns.
-- Consider invalidating the `EditorQueries` RTK Query tag whenever a query's
- Redux state reaches a terminal status, so the backend snapshot itself
- self-heals instead of relying solely on the client-side merge to paper over
- a stale cache entry.
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
index da1735a7746..5972a844837 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
@@ -293,13 +293,145 @@ test('renders the live Redux state instead of a stale backend snapshot for the s
},
};
- render(setup(), { useRedux: true, initialState: stateWithLiveQuery });
+ const { container } = render(setup(), {
+ useRedux: true,
+ initialState: stateWithLiveQuery,
+ });
+
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
+ );
+
+ expect(screen.getByText('443')).toBeInTheDocument();
+ expect(container.querySelector('.anticon-check')).toBeInTheDocument();
+ expect(container.querySelector('.anticon-loading')).not.toBeInTheDocument();
+ expect(screen.queryByText('0%')).not.toBeInTheDocument();
+
+ isFeatureEnabledMock.mockClear();
+});
+
+test('lets a concluded backend snapshot win over a stale non-concluded Redux state', async () => {
+ const isFeatureEnabledMock = mockedIsFeatureEnabled.mockImplementation(
+ featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
+ );
+
+ const concludedApiResult = {
+ count: 1,
+ ids: [692],
+ result: [
+ {
+ ...fakeApiResult.result[0],
+ client_id: 'unpolledClientId',
+ status: QueryState.Success,
+ progress: 100,
+ rows: 443,
+ sql_editor_id: defaultQueryEditor.id,
+ },
+ ],
+ };
+
+ const editorQueryApiRoute = `glob:*/api/v1/query/?q=*`;
+ fetchMock.get(editorQueryApiRoute, concludedApiResult);
+
+ // Redux never observed the query leave Running, e.g. because
+ // QueryAutoRefresh stopped polling it (MAX_QUERY_AGE_TO_POLL elapsed).
+ const stateWithStaleRunningQuery = {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ unpolledClientId: {
+ id: 'unpolledClientId',
+ sqlEditorId: defaultQueryEditor.id,
+ sql: 'SELECT 1',
+ state: QueryState.Running,
+ startDttm: 1710273662445,
+ progress: 0,
+ rows: 0,
+ },
+ },
+ },
+ };
+
+ const { container } = render(setup(), {
+ useRedux: true,
+ initialState: stateWithStaleRunningQuery,
+ });
+
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
+ );
+
+ expect(screen.getByText('443')).toBeInTheDocument();
+ expect(container.querySelector('.anticon-check')).toBeInTheDocument();
+ expect(container.querySelector('.anticon-loading')).not.toBeInTheDocument();
+ expect(screen.queryByText('0%')).not.toBeInTheDocument();
+
+ isFeatureEnabledMock.mockClear();
+});
+
+test('renders a backend-only historical query the client never ran, alongside a live one', async () => {
+ const isFeatureEnabledMock = mockedIsFeatureEnabled.mockImplementation(
+ featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
+ );
+
+ const twoRowApiResult = {
+ count: 2,
+ ids: [692, 700],
+ result: [
+ {
+ ...fakeApiResult.result[0],
+ client_id: 'liveClientId',
+ status: QueryState.Running,
+ progress: 0,
+ rows: 0,
+ sql_editor_id: defaultQueryEditor.id,
+ },
+ {
+ ...fakeApiResult.result[0],
+ id: 700,
+ client_id: 'historicalOnlyClientId',
+ status: QueryState.Success,
+ progress: 100,
+ rows: 12,
+ sql_editor_id: defaultQueryEditor.id,
+ start_time: '1710273660000.000000',
+ },
+ ],
+ };
+
+ const editorQueryApiRoute = `glob:*/api/v1/query/?q=*`;
+ fetchMock.get(editorQueryApiRoute, twoRowApiResult);
+
+ const stateWithOnlyOneLiveQuery = {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ queries: {
+ liveClientId: {
+ id: 'liveClientId',
+ sqlEditorId: defaultQueryEditor.id,
+ sql: 'SELECT 1',
+ state: QueryState.Success,
+ startDttm: 1710273662445,
+ progress: 100,
+ rows: 443,
+ },
+ },
+ },
+ };
+
+ render(setup(), {
+ useRedux: true,
+ initialState: stateWithOnlyOneLiveQuery,
+ });
await waitFor(() =>
expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
);
expect(screen.getByText('443')).toBeInTheDocument();
+ expect(screen.getByText('12')).toBeInTheDocument();
isFeatureEnabledMock.mockClear();
});
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
index fd812b887c7..5bbf815f5b4 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
@@ -21,7 +21,12 @@ import { shallowEqual, useSelector } from 'react-redux';
import { useInView } from 'react-intersection-observer';
import { EmptyState, Skeleton } from '@superset-ui/core/components';
import { t } from '@apache-superset/core/translation';
-import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import {
+ FeatureFlag,
+ isFeatureEnabled,
+ concludedQueryStateList,
+ QueryResponse,
+} from '@superset-ui/core';
import { styled, css } from '@apache-superset/core/theme';
import QueryTable from 'src/SqlLab/components/QueryTable';
import { SqlLabRootState } from 'src/SqlLab/types';
@@ -56,6 +61,56 @@ const getEditorQueries = (
({ sqlEditorId }) => String(sqlEditorId) === String(queryEditorId),
);
+const isConcludedState = (state: QueryResponse['state']) =>
+ concludedQueryStateList.includes(state);
+
+// Merges a backend history row with the matching live Redux query.
+// Backend-only metadata (queryId, user, db, startDttm, ...) is always kept
+// from the snapshot. The status fields below move together as a query
+// progresses, so whichever side reports the more advanced (concluded)
+// state supplies all of them together, rather than e.g. pairing a
+// concluded state from one side with a stale progress/rows from the
+// other. When neither side has concluded (or both have), the live Redux
+// copy wins field-by-field, skipping any field it hasn't populated yet
+// (e.g. a query that was just started client-side and hasn't been
+// polled).
+const mergeQueryStatus = (
+ remoteQuery: QueryResponse,
+ localQuery: QueryResponse,
+): QueryResponse => {
+ const statusSource =
+ isConcludedState(remoteQuery.state) && !isConcludedState(localQuery.state)
+ ? remoteQuery
+ : localQuery;
+ return {
+ ...remoteQuery,
+ state:
+ statusSource.state !== undefined ? statusSource.state : remoteQuery.state,
+ progress:
+ statusSource.progress !== undefined
+ ? statusSource.progress
+ : remoteQuery.progress,
+ rows:
+ statusSource.rows !== undefined ? statusSource.rows : remoteQuery.rows,
+ endDttm:
+ statusSource.endDttm !== undefined
+ ? statusSource.endDttm
+ : remoteQuery.endDttm,
+ resultsKey:
+ statusSource.resultsKey !== undefined
+ ? statusSource.resultsKey
+ : remoteQuery.resultsKey,
+ errorMessage:
+ statusSource.errorMessage !== undefined
+ ? statusSource.errorMessage
+ : remoteQuery.errorMessage,
+ executedSql:
+ statusSource.executedSql !== undefined
+ ? statusSource.executedSql
+ : remoteQuery.executedSql,
+ };
+};
+
const QueryHistory = ({
queryEditorId,
displayLimit,
@@ -85,16 +140,13 @@ const QueryHistory = ({
if (!data) {
return getEditorQueries(queries, editorId);
}
- // Overlay live Redux state onto the backend snapshot per query id, so a
- // terminal status reached after the snapshot was fetched isn't masked by
- // the stale value, while backend-only metadata (e.g. queryId, user) is
- // preserved for queries this client never ran.
const remoteIds = new Set(data.result.map(({ id }) => id));
- const mergedRemoteQueries = data.result.map(remoteQuery =>
- queries[remoteQuery.id]
- ? { ...remoteQuery, ...queries[remoteQuery.id] }
- : remoteQuery,
- );
+ const mergedRemoteQueries = data.result.map(remoteQuery => {
+ const localQuery = queries[remoteQuery.id];
+ return localQuery
+ ? mergeQueryStatus(remoteQuery, localQuery)
+ : remoteQuery;
+ });
return getEditorQueries(queries, editorId)
.filter(({ id }) => !remoteIds.has(id))
.concat(mergedRemoteQueries)
(superset) 02/07: docs: add RCA for SQL Lab query history stuck in Running/Scheduled
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-sqllab-query-history-stuck-running-new-tab
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6ba21f8d83e9517fc7ed5992e0fb4f403f3256c8
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Aug 26 00:14:29 2026 +0000
docs: add RCA for SQL Lab query history stuck in Running/Scheduled
Root-cause analysis for the query history status bug reproduced by the
preceding regression guard, including latent issues found nearby that
are deliberately left out of scope for this change.
---
RCA.md | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 124 insertions(+)
diff --git a/RCA.md b/RCA.md
new file mode 100644
index 00000000000..30b9c4aa938
--- /dev/null
+++ b/RCA.md
@@ -0,0 +1,124 @@
+## What Happened
+
+In SQL Lab, a query run in a new/second tab can display "Running" or
+"Scheduled" in the Query History panel forever, even though the query has
+long since finished and the Results pane / Redux store correctly show it as
+succeeded (or failed). (verified — reproduced via the regression guard added
+in this branch)
+
+## Root Cause
+
+`QueryHistory` (`superset-frontend/src/SqlLab/components/QueryHistory/index.tsx:85-103`)
+merges two sources of truth for the query list of a tab:
+
+- the live Redux `sqlLab.queries` slice, which `QueryAutoRefresh` keeps
+ correctly up to date as a query transitions `running -> success`/`failed`
+ (verified), and
+- a one-shot snapshot fetched from the `useEditorQueriesQuery` RTK Query
+ endpoint (`superset-frontend/src/hooks/apiResources/queries.ts:112-176`).
+
+The merge `useMemo` computes, for any query id that appears in the backend
+snapshot, `omit(liveQueries, snapshotIds).concat(snapshotResult)` — i.e. it
+drops the live Redux copy for that id and renders the snapshot row instead
+(verified, `index.tsx:88-95`). Because `editorQueries` (the query-history RTK
+API slice, tag `EditorQueries`) is never invalidated anywhere in the codebase
+— no mutation, action, or component calls `invalidateTags(['EditorQueries'])`,
+and the endpoint has no `pollingInterval`, `refetchOnFocus`, or
+`refetchOnMountOrArgChange` (verified, `queries.ts:110-175`) — the snapshot is
+fetched exactly once when the tab/editor mounts and is cached by RTK Query for
+the lifetime of that cache entry. Whatever status the backend happened to
+report at that single GET is what renders in the State/Progress/Rows columns
+for as long as the tab stays open, regardless of what Redux says afterward.
+
+`QueryTable` (`superset-frontend/src/SqlLab/components/QueryTable/index.tsx:264`)
+maps the (now-frozen) status to a label: `running` -> "Running", and both
+`pending` and `scheduled` -> "Scheduled" (verified, `index.tsx:185-239`),
+which is exactly the reported symptom.
+
+This is most visible on a **new tab** because a freshly opened SQL Lab tab is
+the case most likely to mount `QueryHistory` (and therefore fire the one-shot
+snapshot fetch) at almost the same moment a query is started, maximizing the
+odds that the snapshot is captured while the query is still `running`/
+`pending`/`scheduled` before it ever gets a second chance to refresh
+(inferred — consistent with the "stuck on new tab" framing of the symptom,
+not independently reproduced against the mount-timing race itself).
+
+## Why It Wasn't Caught
+
+- The existing tests in `QueryHistory.test.tsx` only exercise the case where
+ the backend snapshot and Redux state agree (same `client_id`, same
+ `status`, same `rows`) or where they cover disjoint ids. None of them seed
+ Redux with a *live, more-current* copy of a row that also appears in the
+ mocked backend snapshot, which is the only shape that exposes this bug
+ (verified — read the full pre-existing test file before adding the new
+ test).
+- `apache/superset#42896` fixed the Redux reducer to correctly allow a
+ `Running -> Success` transition, which fixed the underlying Redux state.
+ That fix is real and necessary, but it doesn't help here because
+ `QueryHistory` doesn't render Redux state for any id the backend snapshot
+ also mentions — it renders the snapshot's copy instead (verified).
+- There is no integration/e2e test that opens a new tab, starts a query, and
+ asserts the History panel status transitions all the way to a terminal
+ state (inferred — no such test was found in the areas searched).
+
+## The Fix
+
+Not applied in this branch (see PR description / Critical boundary note).
+Intended production change, to be made in a follow-up PR: in
+`superset-frontend/src/SqlLab/components/QueryHistory/index.tsx`, the merge
+should let the live Redux copy win over the backend snapshot for any id
+present in both, e.g. build the merged list from
+`data.result` overlaid with `queries` (live wins) rather than
+`omit(queries, snapshotIds).concat(data.result)` (snapshot wins). The
+`editorId`/`status` fields also need to come from Redux when a live entry
+exists, not just `rows`/`progress`, since the State and Progress columns
+render from the same merged object.
+
+## Latent Bugs Found
+
+The RCA panel identified the following issues while investigating this bug.
+All are deliberately out of scope for this change and are recorded here for
+future follow-up:
+
+- `superset-frontend/src/hooks/apiResources/queries.ts` — the `merge`
+ function (`currentCache.result.push(...newItems.result)`) always appends
+ rather than replacing/de-duplicating by id. A refetch of a page whose
+ underlying rows have changed (e.g. pagination re-fetch racing a mutation)
+ would duplicate rows in the RTK Query cache rather than reconcile them.
+- `superset-frontend/src/SqlLab/components/QueryHistory/index.tsx` — `pageIndex`
+ state is not reset when `editorId` changes, so switching tabs after having
+ scrolled to page N+1 on the previous tab carries the stale page index into
+ the new tab's query.
+- `superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx` — the
+ periodic (5s) save timer that POSTs to `/tabstateview/` has no in-flight
+ guard; a slow response can let a second save fire before the first
+ resolves, creating a duplicate TabState.
+- `superset-frontend/src/SqlLab/reducers/sqlLab.ts:119` — the
+ `CLONE_QUERY_TO_NEW_TAB` reducer case handler is unreachable dead code: the
+ corresponding thunk in `actions/sqlLab.ts` has drifted and no longer
+ dispatches that action type, so the reducer branch never executes.
+- `superset-frontend/src/SqlLab/components/QueryTable/index.tsx:335,340` —
+ `progress.toFixed(0)` is called unguarded; a `null`/`undefined` `progress`
+ value (as can occur on some query states) will throw.
+- `syncQueryEditor` snapshots `queriesToMigrate` before awaiting the
+ `POST /tabstateview/` call, so a query started inside that async window is
+ never migrated. This is narrow and out of scope for the bug fixed here
+ (`EditorAutoSync`'s save timer fires roughly every 5s, well after
+ `autorun` would have started a query), but is a real, separate race.
+
+## Prevention
+
+- The added regression guard
+ (`superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx`)
+ seeds Redux with a live, terminal-state copy of a query whose id also
+ appears in a mocked in-flight backend snapshot, and asserts the live value
+ renders. This is the minimal repro shape that would have caught this bug
+ and will catch any regression of the same kind.
+- Once the production fix lands, add an assertion (or a follow-up test) that
+ the merged list also carries over the live `status`/`state`, not just
+ `rows`, since a fix that only reconciles one field could reintroduce the
+ same class of bug for the State/Progress columns.
+- Consider invalidating the `EditorQueries` RTK Query tag whenever a query's
+ Redux state reaches a terminal status, so the backend snapshot itself
+ self-heals instead of relying solely on the client-side merge to paper over
+ a stale cache entry.
(superset) 05/07: fix(sqllab): narrow query history merge to one quadrant, extract and unit-test it
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-sqllab-query-history-stuck-running-new-tab
in repository https://gitbox.apache.org/repos/asf/superset.git
commit f3ae25fd4d66048f900e43332c3573befff92c1b
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Aug 26 04:43:07 2026 +0000
fix(sqllab): narrow query history merge to one quadrant, extract and unit-test it
Prior iterations tried to make the backend-snapshot/live-Redux merge
correct in general, and each pass found another mishandled
combination (reverse precedence, a start/end timestamp split across
two clocks, a both-concluded tie, rerun-field inheritance). Replace
that generalized merge with a narrow rule: override the backend
snapshot only when the live Redux copy has concluded and the
snapshot has not (the one case QueryAutoRefresh's polling cutoff can
actually strand). When overriding, the live copy supplies the status
fields and both timestamps together, since they must come from a
single clock. Every other combination - including both sides
concluded - returns the snapshot unchanged, matching prior behavior.
Drop executedSql from the overlay; it never transitions.
Extract mergeQueryStatus into its own module and add a table-driven
unit test covering all four (local-concluded x remote-concluded)
combinations plus the undefined-vs-null skip fallback, since the
function's field-level fallbacks had no coverage reachable through
the full QueryHistory component render path.
Fix the component regression guard: synchronize on a render-only
observable (the Duration cell, which only appears once the backend
snapshot has loaded and been merged in) instead of on the fetch being
issued, since the previous synchronization point resolved before the
merge could have run at all. Drop the vacuous 0% assertions
(unreachable independently of the state-icon assertion), use
aria-label icon queries instead of undocumented antd class names,
correct a test fixture to a state CLEAR_INACTIVE_QUERIES cannot evict
in production, and scope the multi-row test's assertions per row.
---
.../components/QueryHistory/QueryHistory.test.tsx | 77 ++++++++-----
.../src/SqlLab/components/QueryHistory/index.tsx | 58 +---------
.../QueryHistory/mergeQueryStatus.test.ts | 119 +++++++++++++++++++++
.../components/QueryHistory/mergeQueryStatus.ts | 71 ++++++++++++
4 files changed, 240 insertions(+), 85 deletions(-)
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
index 5972a844837..5b091156302 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
@@ -18,7 +18,7 @@
*/
import fetchMock from 'fetch-mock';
import { FeatureFlag, isFeatureEnabled, QueryState } from '@superset-ui/core';
-import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
import QueryHistory from 'src/SqlLab/components/QueryHistory';
import {
initialState,
@@ -252,7 +252,17 @@ test('displays multiple queries with newest query first', async () => {
isFeatureEnabledMock.mockClear();
});
-test('renders the live Redux state instead of a stale backend snapshot for the same query', async () => {
+// The `QueryTable` Duration cell only renders when `q.endDttm` is set, and
+// the live-only Redux fixtures below deliberately omit `endDttm` (a
+// client-started query never has one until the backend supplies it). So
+// `findByText(/^00:00:00\./)` can only resolve once the backend snapshot
+// has actually loaded *and* been folded into the rendered row - unlike
+// `waitFor(() => calls.length === 1)`, which resolves as soon as the
+// request is issued, while `data` is still `undefined` and the component
+// is still rendering the pre-merge, Redux-only fallback.
+const findDurationCell = () => screen.findByText(/^00:00:00\./);
+
+test('overrides a stale non-concluded backend snapshot with a concluded live Redux state', async () => {
const isFeatureEnabledMock = mockedIsFeatureEnabled.mockImplementation(
featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
);
@@ -293,24 +303,21 @@ test('renders the live Redux state instead of a stale backend snapshot for the s
},
};
- const { container } = render(setup(), {
- useRedux: true,
- initialState: stateWithLiveQuery,
- });
+ render(setup(), { useRedux: true, initialState: stateWithLiveQuery });
await waitFor(() =>
expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
);
+ await findDurationCell();
- expect(screen.getByText('443')).toBeInTheDocument();
- expect(container.querySelector('.anticon-check')).toBeInTheDocument();
- expect(container.querySelector('.anticon-loading')).not.toBeInTheDocument();
- expect(screen.queryByText('0%')).not.toBeInTheDocument();
+ const row = screen.getByText('443').closest('tr') as HTMLElement;
+ expect(within(row).getByLabelText('check')).toBeInTheDocument();
+ expect(within(row).queryByLabelText('loading')).not.toBeInTheDocument();
isFeatureEnabledMock.mockClear();
});
-test('lets a concluded backend snapshot win over a stale non-concluded Redux state', async () => {
+test('does not override an already-concluded backend snapshot with a non-concluded Redux state', async () => {
const isFeatureEnabledMock = mockedIsFeatureEnabled.mockImplementation(
featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
);
@@ -321,7 +328,7 @@ test('lets a concluded backend snapshot win over a stale non-concluded Redux sta
result: [
{
...fakeApiResult.result[0],
- client_id: 'unpolledClientId',
+ client_id: 'scheduledClientId',
status: QueryState.Success,
progress: 100,
rows: 443,
@@ -333,18 +340,20 @@ test('lets a concluded backend snapshot win over a stale non-concluded Redux sta
const editorQueryApiRoute = `glob:*/api/v1/query/?q=*`;
fetchMock.get(editorQueryApiRoute, concludedApiResult);
- // Redux never observed the query leave Running, e.g. because
- // QueryAutoRefresh stopped polling it (MAX_QUERY_AGE_TO_POLL elapsed).
- const stateWithStaleRunningQuery = {
+ // Redux hasn't observed this query conclude yet: it's still Scheduled.
+ // Deliberately not Running/Pending with progress 0, which is the tuple
+ // CLEAR_INACTIVE_QUERIES evicts once stale - that combination can't
+ // actually reach this merge in production.
+ const stateWithScheduledQuery = {
...initialState,
sqlLab: {
...initialState.sqlLab,
queries: {
- unpolledClientId: {
- id: 'unpolledClientId',
+ scheduledClientId: {
+ id: 'scheduledClientId',
sqlEditorId: defaultQueryEditor.id,
sql: 'SELECT 1',
- state: QueryState.Running,
+ state: QueryState.Scheduled,
startDttm: 1710273662445,
progress: 0,
rows: 0,
@@ -353,19 +362,16 @@ test('lets a concluded backend snapshot win over a stale non-concluded Redux sta
},
};
- const { container } = render(setup(), {
- useRedux: true,
- initialState: stateWithStaleRunningQuery,
- });
+ render(setup(), { useRedux: true, initialState: stateWithScheduledQuery });
await waitFor(() =>
expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
);
+ await findDurationCell();
- expect(screen.getByText('443')).toBeInTheDocument();
- expect(container.querySelector('.anticon-check')).toBeInTheDocument();
- expect(container.querySelector('.anticon-loading')).not.toBeInTheDocument();
- expect(screen.queryByText('0%')).not.toBeInTheDocument();
+ const row = screen.getByText('443').closest('tr') as HTMLElement;
+ expect(within(row).getByLabelText('check')).toBeInTheDocument();
+ expect(within(row).queryByLabelText('loading')).not.toBeInTheDocument();
isFeatureEnabledMock.mockClear();
});
@@ -421,7 +427,7 @@ test('renders a backend-only historical query the client never ran, alongside a
},
};
- render(setup(), {
+ const { container } = render(setup(), {
useRedux: true,
initialState: stateWithOnlyOneLiveQuery,
});
@@ -429,9 +435,22 @@ test('renders a backend-only historical query the client never ran, alongside a
await waitFor(() =>
expect(fetchMock.callHistory.calls(editorQueryApiRoute).length).toBe(1),
);
+ await findDurationCell();
- expect(screen.getByText('443')).toBeInTheDocument();
- expect(screen.getByText('12')).toBeInTheDocument();
+ const tableRows = container.querySelectorAll(
+ 'table > tbody > tr:not(.ant-table-measure-row)',
+ );
+ expect(tableRows).toHaveLength(2);
+
+ const liveRow = screen.getByText('443').closest('tr') as HTMLElement;
+ expect(within(liveRow).getByLabelText('check')).toBeInTheDocument();
+ expect(within(liveRow).queryByLabelText('loading')).not.toBeInTheDocument();
+
+ const historicalRow = screen.getByText('12').closest('tr') as HTMLElement;
+ expect(within(historicalRow).getByLabelText('check')).toBeInTheDocument();
+ expect(
+ within(historicalRow).queryByLabelText('loading'),
+ ).not.toBeInTheDocument();
isFeatureEnabledMock.mockClear();
});
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
index 5bbf815f5b4..14560d2ffb5 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
@@ -21,12 +21,7 @@ import { shallowEqual, useSelector } from 'react-redux';
import { useInView } from 'react-intersection-observer';
import { EmptyState, Skeleton } from '@superset-ui/core/components';
import { t } from '@apache-superset/core/translation';
-import {
- FeatureFlag,
- isFeatureEnabled,
- concludedQueryStateList,
- QueryResponse,
-} from '@superset-ui/core';
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
import { styled, css } from '@apache-superset/core/theme';
import QueryTable from 'src/SqlLab/components/QueryTable';
import { SqlLabRootState } from 'src/SqlLab/types';
@@ -35,6 +30,7 @@ import useEffectEvent from 'src/hooks/useEffectEvent';
import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
import PanelToolbar from 'src/components/PanelToolbar';
import { ViewLocations } from 'src/SqlLab/contributions';
+import { mergeQueryStatus } from './mergeQueryStatus';
interface QueryHistoryProps {
queryEditorId: string | number;
@@ -61,56 +57,6 @@ const getEditorQueries = (
({ sqlEditorId }) => String(sqlEditorId) === String(queryEditorId),
);
-const isConcludedState = (state: QueryResponse['state']) =>
- concludedQueryStateList.includes(state);
-
-// Merges a backend history row with the matching live Redux query.
-// Backend-only metadata (queryId, user, db, startDttm, ...) is always kept
-// from the snapshot. The status fields below move together as a query
-// progresses, so whichever side reports the more advanced (concluded)
-// state supplies all of them together, rather than e.g. pairing a
-// concluded state from one side with a stale progress/rows from the
-// other. When neither side has concluded (or both have), the live Redux
-// copy wins field-by-field, skipping any field it hasn't populated yet
-// (e.g. a query that was just started client-side and hasn't been
-// polled).
-const mergeQueryStatus = (
- remoteQuery: QueryResponse,
- localQuery: QueryResponse,
-): QueryResponse => {
- const statusSource =
- isConcludedState(remoteQuery.state) && !isConcludedState(localQuery.state)
- ? remoteQuery
- : localQuery;
- return {
- ...remoteQuery,
- state:
- statusSource.state !== undefined ? statusSource.state : remoteQuery.state,
- progress:
- statusSource.progress !== undefined
- ? statusSource.progress
- : remoteQuery.progress,
- rows:
- statusSource.rows !== undefined ? statusSource.rows : remoteQuery.rows,
- endDttm:
- statusSource.endDttm !== undefined
- ? statusSource.endDttm
- : remoteQuery.endDttm,
- resultsKey:
- statusSource.resultsKey !== undefined
- ? statusSource.resultsKey
- : remoteQuery.resultsKey,
- errorMessage:
- statusSource.errorMessage !== undefined
- ? statusSource.errorMessage
- : remoteQuery.errorMessage,
- executedSql:
- statusSource.executedSql !== undefined
- ? statusSource.executedSql
- : remoteQuery.executedSql,
- };
-};
-
const QueryHistory = ({
queryEditorId,
displayLimit,
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts b/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts
new file mode 100644
index 00000000000..72b93dd800d
--- /dev/null
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.test.ts
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { QueryState, testQueryResponse } from '@superset-ui/core';
+import { mergeQueryStatus } from './mergeQueryStatus';
+
+const remoteBase = {
+ ...testQueryResponse,
+ state: QueryState.Running,
+ progress: 0,
+ rows: 0,
+ startDttm: 1000,
+ endDttm: undefined as unknown as number,
+ resultsKey: null,
+ errorMessage: null,
+};
+
+const localBase = {
+ ...testQueryResponse,
+ state: QueryState.Success,
+ progress: 100,
+ rows: 443,
+ startDttm: 2000,
+ endDttm: 3000,
+ resultsKey: 'a-results-key',
+ errorMessage: null,
+};
+
+test('both non-concluded: returns the remote row unchanged', () => {
+ const remote = { ...remoteBase, state: QueryState.Running };
+ const local = { ...localBase, state: QueryState.Scheduled };
+
+ expect(mergeQueryStatus(remote, local)).toBe(remote);
+});
+
+test('remote concluded, local not: returns the remote row unchanged', () => {
+ const remote = { ...remoteBase, state: QueryState.Success };
+ const local = { ...localBase, state: QueryState.Running };
+
+ expect(mergeQueryStatus(remote, local)).toBe(remote);
+});
+
+test('both concluded: declines to override, returns the remote row unchanged', () => {
+ const remote = { ...remoteBase, state: QueryState.Success };
+ const local = { ...localBase, state: QueryState.Stopped };
+
+ expect(mergeQueryStatus(remote, local)).toBe(remote);
+});
+
+test('local concluded, remote not: local supplies status fields and both timestamps together', () => {
+ const remote = { ...remoteBase, state: QueryState.Running };
+ const local = { ...localBase, state: QueryState.Success };
+
+ expect(mergeQueryStatus(remote, local)).toEqual({
+ ...remote,
+ state: QueryState.Success,
+ progress: 100,
+ rows: 443,
+ startDttm: 2000,
+ endDttm: 3000,
+ resultsKey: 'a-results-key',
+ errorMessage: null,
+ });
+});
+
+test('local concluded, remote not: undefined local fields fall back to the remote value', () => {
+ const remote = {
+ ...remoteBase,
+ state: QueryState.Running,
+ resultsKey: 'remote-results-key',
+ errorMessage: 'remote error',
+ };
+ const local = {
+ ...localBase,
+ state: QueryState.Success,
+ resultsKey: undefined as unknown as string,
+ errorMessage: undefined as unknown as string,
+ };
+
+ const merged = mergeQueryStatus(remote, local);
+
+ expect(merged.resultsKey).toBe('remote-results-key');
+ expect(merged.errorMessage).toBe('remote error');
+});
+
+test('local concluded, remote not: a null local field overrides a remote value (does not fall back)', () => {
+ const remote = {
+ ...remoteBase,
+ state: QueryState.Running,
+ resultsKey: 'remote-results-key',
+ errorMessage: 'remote error',
+ };
+ const local = {
+ ...localBase,
+ state: QueryState.Failed,
+ resultsKey: null,
+ errorMessage: null,
+ };
+
+ const merged = mergeQueryStatus(remote, local);
+
+ expect(merged.resultsKey).toBeNull();
+ expect(merged.errorMessage).toBeNull();
+});
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.ts b/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.ts
new file mode 100644
index 00000000000..e1dd83840f2
--- /dev/null
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/mergeQueryStatus.ts
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { concludedQueryStateList, QueryResponse } from '@superset-ui/core';
+
+export const isConcludedState = (state: QueryResponse['state']) =>
+ concludedQueryStateList.includes(state);
+
+// The backend history snapshot fetched by `useEditorQueriesQuery` is a
+// one-shot fetch that is never invalidated, so it can strand a query at a
+// non-terminal state forever once `QueryAutoRefresh` stops polling it (see
+// `QueryAutoRefresh.MAX_QUERY_AGE_TO_POLL`). This function corrects only
+// that one case: when the live Redux copy has concluded and the snapshot
+// has not, the live row supplies the status fields and both timestamps
+// together, since they must come from a single clock (the backend records
+// both `startDttm`/`endDttm` in server time, while the client stamps
+// `endDttm` from the browser clock when a query concludes locally). Every
+// other combination — including both sides concluded, or the snapshot
+// already concluded — returns the snapshot row unchanged.
+export const mergeQueryStatus = (
+ remoteQuery: QueryResponse,
+ localQuery: QueryResponse,
+): QueryResponse => {
+ if (
+ !isConcludedState(localQuery.state) ||
+ isConcludedState(remoteQuery.state)
+ ) {
+ return remoteQuery;
+ }
+ return {
+ ...remoteQuery,
+ state:
+ localQuery.state !== undefined ? localQuery.state : remoteQuery.state,
+ progress:
+ localQuery.progress !== undefined
+ ? localQuery.progress
+ : remoteQuery.progress,
+ rows: localQuery.rows !== undefined ? localQuery.rows : remoteQuery.rows,
+ startDttm:
+ localQuery.startDttm !== undefined
+ ? localQuery.startDttm
+ : remoteQuery.startDttm,
+ endDttm:
+ localQuery.endDttm !== undefined
+ ? localQuery.endDttm
+ : remoteQuery.endDttm,
+ resultsKey:
+ localQuery.resultsKey !== undefined
+ ? localQuery.resultsKey
+ : remoteQuery.resultsKey,
+ errorMessage:
+ localQuery.errorMessage !== undefined
+ ? localQuery.errorMessage
+ : remoteQuery.errorMessage,
+ };
+};
(superset) 03/07: fix(sqllab): prefer live query state over stale history snapshot
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-sqllab-query-history-stuck-running-new-tab
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 1cea2282bfa51b2463385acb7fbf9daff5c90cbb
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Aug 26 00:24:46 2026 +0000
fix(sqllab): prefer live query state over stale history snapshot
The editorQueries useMemo in QueryHistory dropped the live Redux copy
of a query in favor of the backend snapshot whenever both shared an
id, so a query that reached a terminal status client-side (via
QueryAutoRefresh) kept rendering its stale Running/Scheduled snapshot
forever. Overlay live Redux fields onto the backend row instead so
status updates propagate while backend-only metadata (queryId, user,
db, executedSql, resultsKey) is preserved for rows the client never
ran.
---
.../src/SqlLab/components/QueryHistory/index.tsx | 43 ++++++++++++----------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
index d41c13fe750..fd812b887c7 100644
--- a/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
+++ b/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
@@ -19,7 +19,6 @@
import { useEffect, useMemo, useState } from 'react';
import { shallowEqual, useSelector } from 'react-redux';
import { useInView } from 'react-intersection-observer';
-import { omit } from 'lodash-es';
import { EmptyState, Skeleton } from '@superset-ui/core/components';
import { t } from '@apache-superset/core/translation';
import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
@@ -82,25 +81,29 @@ const QueryHistory = ({
skip: !isFeatureEnabled(FeatureFlag.SqllabBackendPersistence),
},
);
- const editorQueries = useMemo(
- () =>
- data
- ? getEditorQueries(
- omit(
- queries,
- data.result.map(({ id }) => id),
- ),
- editorId,
- )
- .concat(data.result)
- .sort((a, b) => {
- const aTime = a.startDttm || 0;
- const bTime = b.startDttm || 0;
- return aTime - bTime;
- })
- : getEditorQueries(queries, editorId),
- [queries, data, editorId],
- );
+ const editorQueries = useMemo(() => {
+ if (!data) {
+ return getEditorQueries(queries, editorId);
+ }
+ // Overlay live Redux state onto the backend snapshot per query id, so a
+ // terminal status reached after the snapshot was fetched isn't masked by
+ // the stale value, while backend-only metadata (e.g. queryId, user) is
+ // preserved for queries this client never ran.
+ const remoteIds = new Set(data.result.map(({ id }) => id));
+ const mergedRemoteQueries = data.result.map(remoteQuery =>
+ queries[remoteQuery.id]
+ ? { ...remoteQuery, ...queries[remoteQuery.id] }
+ : remoteQuery,
+ );
+ return getEditorQueries(queries, editorId)
+ .filter(({ id }) => !remoteIds.has(id))
+ .concat(mergedRemoteQueries)
+ .sort((a, b) => {
+ const aTime = a.startDttm || 0;
+ const bTime = b.startDttm || 0;
+ return aTime - bTime;
+ });
+ }, [queries, data, editorId]);
const loadNext = useEffectEvent(() => {
setPageIndex(pageIndex + 1);