From 6b3cb390f1c4f833bf691136212bac0c45cd1072 Mon Sep 17 00:00:00 2001 From: PeterYurkovich Date: Thu, 3 Sep 2026 14:11:51 -0400 Subject: [PATCH 1/6] fix: validate return object, don't abort requests, don't poll while request is pending --- web/cypress/e2e/integration/logs-page.cy.ts | 74 +++++++++++++++++++ web/eslint.config.ts | 6 ++ web/src/__tests__/loki-client.spec.ts | 22 +++++- web/src/components/virtualized-logs-table.tsx | 2 + web/src/hooks/useLogs.ts | 45 ++++++----- web/src/logs.types.ts | 6 ++ web/src/loki-client.ts | 42 ++++++++++- web/src/pages/logs-detail-page.tsx | 10 ++- web/src/pages/logs-dev-page.tsx | 10 ++- web/src/pages/logs-page.tsx | 10 ++- web/src/test-ids.ts | 1 + 11 files changed, 197 insertions(+), 31 deletions(-) diff --git a/web/cypress/e2e/integration/logs-page.cy.ts b/web/cypress/e2e/integration/logs-page.cy.ts index 44005dea2..f314f8166 100644 --- a/web/cypress/e2e/integration/logs-page.cy.ts +++ b/web/cypress/e2e/integration/logs-page.cy.ts @@ -163,6 +163,61 @@ describe('Logs Page', () => { }); }); + it('displays a Loki error payload returned with HTTP 200', () => { + cy.intercept(QUERY_RANGE_STREAMS_URL_MATCH, { + statusCode: 200, + body: { + status: 'error', + errorType: 'bad_data', + error: 'parse error at line 1, col 1: unexpected IDENTIFIER', + }, + }).as('queryRangeStreams'); + + cy.visit(LOGS_PAGE_URL); + + cy.wait('@queryRangeStreams'); + + cy.byTestID(TestIds.LogsTable) + .should('exist') + .within(() => { + cy.contains(/bad_data/i); + cy.contains('parse error at line 1, col 1: unexpected IDENTIFIER'); + }); + }); + + it('keeps the latest query results when an earlier request completes late', () => { + let requestCount = 0; + + cy.intercept(QUERY_RANGE_STREAMS_URL_MATCH, (req) => { + requestCount += 1; + const body = queryRangeStreamsValidResponse({ + message: + requestCount === 1 + ? 'initial result' + : requestCount === 2 + ? 'stale result' + : 'latest result', + }); + + req.reply(requestCount === 2 ? { body, delay: 500 } : body); + }).as('queryRangeStreams'); + + cy.visit(LOGS_PAGE_URL); + cy.wait('@queryRangeStreams'); + + cy.byTestID(TestIds.SyncButton).click(); + cy.wait(100); + cy.byTestID(TestIds.SyncButton).click(); + + cy.contains('latest result').should('exist'); + cy.byTestID(TestIds.LoadMoreLogs).should('exist'); + + cy.wait(600); + cy.contains('latest result').should('exist'); + cy.contains('stale result').should('not.exist'); + cy.byTestID(TestIds.LoadMoreLogs).should('exist'); + }); + it('executes a query when "run query" is pressed', () => { cy.intercept( QUERY_RANGE_STREAMS_URL_MATCH, @@ -318,6 +373,25 @@ describe('Logs Page', () => { cy.byTestID(TestIds.TimeRangeDropdown).find('button').should('contain', 'Last 6 hours'); }); + it('does not refresh while a log request is pending', () => { + cy.intercept(QUERY_RANGE_STREAMS_URL_MATCH, (req) => { + req.reply({ + body: queryRangeStreamsValidResponse({ message: TEST_MESSAGE }), + delay: 30_000, + }); + }).as('queryRangeStreams'); + + cy.visit(LOGS_PAGE_URL); + cy.get('@queryRangeStreams.all').should('have.length', 1); + cy.clock(); + + cy.byTestID(TestIds.RefreshIntervalDropdown).click(); + cy.contains('15 seconds').click(); + cy.tick(15_000); + + cy.get('@queryRangeStreams.all').should('have.length', 1); + }); + it('disables query executors when the query is empty', () => { cy.intercept( QUERY_RANGE_STREAMS_URL_MATCH, diff --git a/web/eslint.config.ts b/web/eslint.config.ts index d32630364..5c6b134dc 100644 --- a/web/eslint.config.ts +++ b/web/eslint.config.ts @@ -20,6 +20,12 @@ const compat = new FlatCompat({ export default defineConfig([ { + linterOptions: { + // eslint --fix will get in a loop where there is no error so it deletes the directive, + // which in turn causes the error to then be shown. + reportUnusedDisableDirectives: 'off', + }, + extends: fixupConfigRules( compat.extends( 'eslint:recommended', diff --git a/web/src/__tests__/loki-client.spec.ts b/web/src/__tests__/loki-client.spec.ts index 5addc369b..db880e697 100644 --- a/web/src/__tests__/loki-client.spec.ts +++ b/web/src/__tests__/loki-client.spec.ts @@ -1,5 +1,5 @@ import { SchemaConfig } from '../logs.types'; -import { getFetchConfig } from '../loki-client'; +import { getFetchConfig, validateQueryRangeResponse } from '../loki-client'; jest.mock('@openshift-console/dynamic-plugin-sdk', () => ({ consoleFetchJSON: jest.fn(), @@ -64,4 +64,24 @@ describe('Loki Client', () => { expect(getFetchConfig(config)).toEqual(expectedFetchConfig); }); }); + + it('rejects Loki error responses', () => { + expect(() => + validateQueryRangeResponse({ + status: 'error', + errorType: 'bad_data', + error: 'parse error at line 1, col 1', + }), + ).toThrow('bad_data: parse error at line 1, col 1'); + }); + + it('rejects malformed successful responses', () => { + expect(() => validateQueryRangeResponse({ status: 'success', data: {} })).toThrow( + 'Invalid Loki query response: missing data.result', + ); + }); + + it('rejects array responses', () => { + expect(() => validateQueryRangeResponse([])).toThrow('Invalid Loki query response'); + }); }); diff --git a/web/src/components/virtualized-logs-table.tsx b/web/src/components/virtualized-logs-table.tsx index 871b3addf..5b5e00b75 100644 --- a/web/src/components/virtualized-logs-table.tsx +++ b/web/src/components/virtualized-logs-table.tsx @@ -27,6 +27,7 @@ import { import { useTranslation } from 'react-i18next'; import { LogTableData, Schema } from '../logs.types'; import { getSeverityColor, Severity } from '../severity'; +import { TestIds } from '../test-ids'; import { CenteredContainer } from './centered-container'; import { ErrorMessage } from './error-message'; @@ -420,6 +421,7 @@ export const VirtualizedLogsTable = ({ { setScrollToIndex(data.length - 1); onLoadMore?.(); diff --git a/web/src/hooks/useLogs.ts b/web/src/hooks/useLogs.ts index 0db62c714..db573b8f6 100644 --- a/web/src/hooks/useLogs.ts +++ b/web/src/hooks/useLogs.ts @@ -159,6 +159,7 @@ const reducer = (state: State, action: Action): State => { return { ...state, isLoadingLogsData: true, + isLoadingMoreLogsData: false, logsData: undefined, logsError: undefined, hasMoreLogsData: false, @@ -258,7 +259,7 @@ export const useLogs = ( volume: undefined, }); const currentDirection = useRef('backward'); - const logsAbort = useRef<() => void | undefined>(); + const logsRequestID = useRef(0); const histogramAbort = useRef<() => void | undefined>(); const volumeAbort = useRef<() => void | undefined>(); const ws = useRef(); @@ -311,8 +312,12 @@ export const useLogs = ( direction?: Direction; schema: Schema; }) => { + const requestID = ++logsRequestID.current; + if (query.length === 0) { - dispatch({ type: 'logsError', payload: { error: new Error('Query is empty') } }); + if (requestID === logsRequestID.current) { + dispatch({ type: 'logsError', payload: { error: new Error('Query is empty') } }); + } return; } @@ -336,13 +341,9 @@ export const useLogs = ( dispatch({ type: 'moreLogsRequest' }); - if (logsAbort.current) { - logsAbort.current(); - } - const config = configRef.current; - const { request, abort } = executeQueryRange({ + const { request } = executeQueryRange({ query, startNs, endNs, @@ -353,16 +354,16 @@ export const useLogs = ( schema, }); - logsAbort.current = abort; - const queryResponse = await request(); - dispatch({ - type: 'moreLogsResponse', - payload: { logsData: queryResponse, config }, - }); + if (requestID === logsRequestID.current) { + dispatch({ + type: 'moreLogsResponse', + payload: { logsData: queryResponse, config }, + }); + } } catch (error) { - if (!isAbortError(error)) { + if (requestID === logsRequestID.current && !isAbortError(error)) { dispatch({ type: 'logsError', payload: { error } }); } } @@ -393,6 +394,8 @@ export const useLogs = ( return; } + const requestID = ++logsRequestID.current; + try { currentQuery.current = query; currentTenant.current = tenant ?? currentTenant.current; @@ -404,13 +407,9 @@ export const useLogs = ( dispatch({ type: 'logsRequest' }); - if (logsAbort.current) { - logsAbort.current(); - } - const config = configRef.current; - const { request, abort } = executeQueryRange({ + const { request } = executeQueryRange({ query, startNs: msToNs(start), endNs: msToNs(end), @@ -421,13 +420,13 @@ export const useLogs = ( schema, }); - logsAbort.current = abort; - const queryResponse = await request(); - dispatch({ type: 'logsResponse', payload: { logsData: queryResponse, config } }); + if (requestID === logsRequestID.current) { + dispatch({ type: 'logsResponse', payload: { logsData: queryResponse, config } }); + } } catch (error) { - if (!isAbortError(error)) { + if (requestID === logsRequestID.current && !isAbortError(error)) { dispatch({ type: 'logsError', payload: { error } }); } } diff --git a/web/src/logs.types.ts b/web/src/logs.types.ts index 49320a343..4412b5257 100644 --- a/web/src/logs.types.ts +++ b/web/src/logs.types.ts @@ -79,6 +79,12 @@ export type QueryRangeResponse = { }; }; +export type LokiErrorResponse = { + status: 'error'; + errorType?: string; + error?: string; +}; + export type VolumeRangeResponse = QueryRangeResponse; export type Rule = { diff --git a/web/src/loki-client.ts b/web/src/loki-client.ts index 695b7e50b..8cab00a6d 100644 --- a/web/src/loki-client.ts +++ b/web/src/loki-client.ts @@ -5,6 +5,7 @@ import { Config, Direction, LabelValueResponse, + LokiErrorResponse, MatrixResult, QueryRangeResponse, RulesResponse, @@ -61,6 +62,35 @@ type LokiTailQueryParams = { const MAX_RANGE_REQUEST_NS = 21_600_000_000_000n; // 6 hours in nanoseconds +export const validateQueryRangeResponse = (response: any): QueryRangeResponse => { + // consoleFetchJSON can return null, empty arrays, strings and other primitives + // Check specifically to make sure the value is a js object + // This is a typical LLM-ism, but here is actually needed for us to check + // for invalid responses to avoid setting `hasMoreLogsData` incorrectly + if (typeof response !== 'object' || response === null || Array.isArray(response)) { + throw new Error('Invalid Loki query response'); + } + + if (response.status === 'error') { + const { errorType, error } = response as LokiErrorResponse; + throw new Error([errorType, error].filter(Boolean).join(': ') || 'Loki query failed'); + } + + if (response.status !== 'success') { + throw new Error(`Invalid Loki query response status: ${String(response.status)}`); + } + + if (!Array.isArray(response.data?.result)) { + throw new Error('Invalid Loki query response: missing data.result'); + } + + if (response.data?.resultType !== 'streams' && response.data?.resultType !== 'matrix') { + throw new Error('Invalid Loki query response: invalid data.resultType'); + } + + return response as QueryRangeResponse; +}; + export const getFetchConfig = ({ config, tenant, @@ -147,7 +177,7 @@ export const executeQueryRange = ({ namespace, direction, schema, -}: QueryRangeParams): CancellableFetch => { +}: QueryRangeParams): { request: () => Promise } => { const extendedQuery = queryWithNamespace({ query, namespace, @@ -167,11 +197,15 @@ export const executeQueryRange = ({ const { endpoint, requestInit, timeout } = getFetchConfig({ config, tenant }); - return cancellableFetch( + const { request } = cancellableFetch( `${endpoint}/loki/api/v1/query_range?${new URLSearchParams(params)}`, requestInit, timeout, ); + + return { + request: async () => validateQueryRangeResponse(await request()), + }; }; export const executeVolumeRange = ({ @@ -242,8 +276,8 @@ export const executeHistogramQuery = ({ schema, }); - // eslint-disable-next-line max-len - const histogramQuery = `sum by (${labelSeverity}) (count_over_time(${extendedQuery} [${intervalString}]))`; + const histogramQuery = + `sum by (${labelSeverity}) ` + `(count_over_time(${extendedQuery} [${intervalString}]))`; const params = { query: histogramQuery, diff --git a/web/src/pages/logs-detail-page.tsx b/web/src/pages/logs-detail-page.tsx index ca538dd9a..edce9854a 100644 --- a/web/src/pages/logs-detail-page.tsx +++ b/web/src/pages/logs-detail-page.tsx @@ -221,7 +221,15 @@ const LogsDetailPage: FC = ({ isDisabled={isQueryEmpty} /> )} - + { + // do not start a new data refresh while one is pending + if (!isLoadingLogsData && !isLoadingMoreLogsData) { + runQuery(); + } + }} + isDisabled={isQueryEmpty} + /> Refresh}>