From 6bc335ea4e69d6199fb61b0f570674d83ba75a74 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Thu, 19 Oct 2023 08:21:37 -0700 Subject: [PATCH 01/23] [data.search.bsearch] Forward request abortSignal to search strategy (#169041) ## Summary Creates an `abortSignal` from the request disconnected event that is forwarded to the search strategy. In practice this means that when a bsearch call is disconnected (either due to client disconnect or server timeout) the corresponding call to ES is also cancelled. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: Stratoula Kalafateli Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- src/plugins/data/common/search/utils.ts | 4 +- .../data/server/search/routes/bsearch.ts | 4 +- .../ese_search/ese_search_strategy.test.ts | 70 +++++++++++++++++++ .../ese_search/ese_search_strategy.ts | 2 +- 4 files changed, 77 insertions(+), 3 deletions(-) diff --git a/src/plugins/data/common/search/utils.ts b/src/plugins/data/common/search/utils.ts index a3695019813280..eeb65c04ecc582 100644 --- a/src/plugins/data/common/search/utils.ts +++ b/src/plugins/data/common/search/utils.ts @@ -23,7 +23,9 @@ export const isAbortResponse = (response?: IKibanaSearchResponse) => { /** * @returns true if request is still running */ -export const isRunningResponse = (response?: IKibanaSearchResponse) => response?.isRunning ?? false; +export const isRunningResponse = (response?: IKibanaSearchResponse) => { + return response?.isRunning ?? false; +}; export const getUserTimeZone = ( getConfig: AggTypesDependencies['getConfig'], diff --git a/src/plugins/data/server/search/routes/bsearch.ts b/src/plugins/data/server/search/routes/bsearch.ts index cbbe5dedb03cfa..bf1aa4aaa3cbcf 100644 --- a/src/plugins/data/server/search/routes/bsearch.ts +++ b/src/plugins/data/server/search/routes/bsearch.ts @@ -11,6 +11,7 @@ import { catchError } from 'rxjs/operators'; import { BfetchServerSetup } from '@kbn/bfetch-plugin/server'; import type { ExecutionContextSetup } from '@kbn/core/server'; import apm from 'elastic-apm-node'; +import { getRequestAbortedSignal } from '../..'; import { IKibanaSearchRequest, IKibanaSearchResponse, @@ -28,6 +29,7 @@ export function registerBsearchRoute( IKibanaSearchResponse >('/internal/bsearch', (request) => { const search = getScoped(request); + const abortSignal = getRequestAbortedSignal(request.events.aborted$); return { /** * @param requestOptions @@ -39,7 +41,7 @@ export function registerBsearchRoute( apm.addLabels(executionContextService.getAsLabels()); return firstValueFrom( - search.search(requestData, restOptions).pipe( + search.search(requestData, { ...restOptions, abortSignal }).pipe( catchError((err) => { // Re-throw as object, to get attributes passed to the client // eslint-disable-next-line no-throw-literal diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts index 3b2c5e8e0e5c8b..6c1746984c86bc 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts @@ -260,6 +260,38 @@ describe('ES search strategy', () => { expect(mockApiCaller).toBeCalledTimes(0); }); + + it('should delete when aborted', async () => { + mockSubmitCaller.mockResolvedValueOnce({ + ...mockAsyncResponse, + body: { + ...mockAsyncResponse.body, + is_running: true, + }, + }); + + const params = { index: 'logstash-*', body: { query: {} } }; + const esSearch = await enhancedEsSearchStrategyProvider( + mockLegacyConfig$, + mockSearchConfig, + mockLogger + ); + const abortController = new AbortController(); + const abortSignal = abortController.signal; + + // Abort after an incomplete first response is returned + setTimeout(() => abortController.abort(), 100); + + let err: KbnServerError | undefined; + try { + await esSearch.search({ params }, { abortSignal }, mockDeps).toPromise(); + } catch (e) { + err = e; + } + expect(mockSubmitCaller).toBeCalled(); + expect(err).not.toBeUndefined(); + expect(mockDeleteCaller).toBeCalled(); + }); }); describe('with sessionId', () => { @@ -367,6 +399,44 @@ describe('ES search strategy', () => { expect(request).toHaveProperty('wait_for_completion_timeout'); expect(request).not.toHaveProperty('keep_alive'); }); + + it('should not delete a saved session when aborted', async () => { + mockSubmitCaller.mockResolvedValueOnce({ + ...mockAsyncResponse, + body: { + ...mockAsyncResponse.body, + is_running: true, + }, + }); + + const params = { index: 'logstash-*', body: { query: {} } }; + const esSearch = await enhancedEsSearchStrategyProvider( + mockLegacyConfig$, + mockSearchConfig, + mockLogger + ); + const abortController = new AbortController(); + const abortSignal = abortController.signal; + + // Abort after an incomplete first response is returned + setTimeout(() => abortController.abort(), 100); + + let err: KbnServerError | undefined; + try { + await esSearch + .search( + { params }, + { abortSignal, sessionId: '1', isSearchStored: true, isStored: true }, + mockDeps + ) + .toPromise(); + } catch (e) { + err = e; + } + expect(mockSubmitCaller).toBeCalled(); + expect(err).not.toBeUndefined(); + expect(mockDeleteCaller).not.toBeCalled(); + }); }); it('throws normalized error if ResponseError is thrown', async () => { diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index c4b8933f2c7c35..460d8f95ed3e4d 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -83,7 +83,7 @@ export const enhancedEsSearchStrategyProvider = ( }; const cancel = async () => { - if (id) { + if (id && !options.isStored) { await cancelAsyncSearch(id, esClient); } }; From d31c579a022c881f8b9334b316fc4f55449cb17b Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 27 Oct 2023 08:17:19 -0700 Subject: [PATCH 02/23] Return result of cancel --- .../search/strategies/ese_search/ese_search_strategy.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 460d8f95ed3e4d..79e29e30fcd519 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -82,9 +82,9 @@ export const enhancedEsSearchStrategyProvider = ( return toAsyncKibanaSearchResponse({ ...body, response }, headers?.warning); }; - const cancel = async () => { + const cancel = () => { if (id && !options.isStored) { - await cancelAsyncSearch(id, esClient); + return cancelAsyncSearch(id, esClient); } }; From 2b13f4405816a3092ed8fcda8cc669e861e29c89 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 27 Oct 2023 13:15:19 -0700 Subject: [PATCH 03/23] Fix other cancel --- .../search/strategies/ese_search/ese_search_strategy.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 79e29e30fcd519..4e52cbe989e6c6 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -168,9 +168,9 @@ export const enhancedEsSearchStrategyProvider = ( * @returns `Promise` * @throws `KbnServerError` */ - cancel: async (id, options, { esClient }) => { + cancel: (id, options, { esClient }) => { logger.debug(`cancel ${id}`); - await cancelAsyncSearch(id, esClient); + return cancelAsyncSearch(id, esClient); }, /** * From d8395bb1feb5e6d5dacaeef4dcffe2263dc724cd Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 27 Oct 2023 13:17:00 -0700 Subject: [PATCH 04/23] Update search service cancel --- src/plugins/data/server/search/search_service.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/plugins/data/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts index 71a335ce515926..7ed1bf32156075 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -437,11 +437,7 @@ export class SearchService implements Plugin { } }; - private cancel = async ( - deps: SearchStrategyDependencies, - id: string, - options: ISearchOptions = {} - ) => { + private cancel = (deps: SearchStrategyDependencies, id: string, options: ISearchOptions = {}) => { const strategy = this.getSearchStrategy(options.strategy); if (!strategy.cancel) { throw new KbnServerError( From ba6d8e28561e1107efab80a8ca596f4c06da0338 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 27 Oct 2023 16:53:52 -0700 Subject: [PATCH 05/23] Swallow cancellation errors in search interceptor --- .../public/search/search_interceptor/search_interceptor.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/plugins/data/public/search/search_interceptor/search_interceptor.ts b/src/plugins/data/public/search/search_interceptor/search_interceptor.ts index 24b2e1c41216a8..a8a32aa76ee425 100644 --- a/src/plugins/data/public/search/search_interceptor/search_interceptor.ts +++ b/src/plugins/data/public/search/search_interceptor/search_interceptor.ts @@ -299,7 +299,9 @@ export class SearchInterceptor { }); const sendCancelRequest = once(() => { - this.deps.http.delete(`/internal/search/${strategy}/${id}`, { version: '1' }); + this.deps.http.delete(`/internal/search/${strategy}/${id}`, { version: '1' }).catch((e) => { + // Swallow cancellation errors + }); }); const cancel = () => id && !isSavedToBackground && sendCancelRequest(); From 312ae343a497467ef557dd90d026c204cfbff0c7 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 27 Oct 2023 17:50:00 -0700 Subject: [PATCH 06/23] Swallow errors in delete route --- src/plugins/data/server/search/routes/search.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plugins/data/server/search/routes/search.ts b/src/plugins/data/server/search/routes/search.ts index 8b302a81aea1ab..5326fc5dbc147d 100644 --- a/src/plugins/data/server/search/routes/search.ts +++ b/src/plugins/data/server/search/routes/search.ts @@ -102,6 +102,7 @@ export function registerSearchRoute(router: DataPluginRouter): void { await search.cancel(id, { strategy }); return res.ok(); } catch (err) { + // return reportServerError(res, err); return reportServerError(res, err); } } From ab50c10b4953705842916f23a4581a5d6371308c Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 27 Oct 2023 21:31:24 -0700 Subject: [PATCH 07/23] Clean up changes --- src/plugins/data/common/search/utils.ts | 4 +--- .../search/search_interceptor/search_interceptor.ts | 8 +++----- src/plugins/data/server/search/routes/search.ts | 1 - .../search/strategies/ese_search/ese_search_strategy.ts | 4 ++-- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/plugins/data/common/search/utils.ts b/src/plugins/data/common/search/utils.ts index eeb65c04ecc582..a3695019813280 100644 --- a/src/plugins/data/common/search/utils.ts +++ b/src/plugins/data/common/search/utils.ts @@ -23,9 +23,7 @@ export const isAbortResponse = (response?: IKibanaSearchResponse) => { /** * @returns true if request is still running */ -export const isRunningResponse = (response?: IKibanaSearchResponse) => { - return response?.isRunning ?? false; -}; +export const isRunningResponse = (response?: IKibanaSearchResponse) => response?.isRunning ?? false; export const getUserTimeZone = ( getConfig: AggTypesDependencies['getConfig'], diff --git a/src/plugins/data/public/search/search_interceptor/search_interceptor.ts b/src/plugins/data/public/search/search_interceptor/search_interceptor.ts index a8a32aa76ee425..63bd973b60bae7 100644 --- a/src/plugins/data/public/search/search_interceptor/search_interceptor.ts +++ b/src/plugins/data/public/search/search_interceptor/search_interceptor.ts @@ -298,11 +298,9 @@ export class SearchInterceptor { isSavedToBackground = true; }); - const sendCancelRequest = once(() => { - this.deps.http.delete(`/internal/search/${strategy}/${id}`, { version: '1' }).catch((e) => { - // Swallow cancellation errors - }); - }); + const sendCancelRequest = once(() => + this.deps.http.delete(`/internal/search/${strategy}/${id}`, { version: '1' }) + ); const cancel = () => id && !isSavedToBackground && sendCancelRequest(); diff --git a/src/plugins/data/server/search/routes/search.ts b/src/plugins/data/server/search/routes/search.ts index 5326fc5dbc147d..8b302a81aea1ab 100644 --- a/src/plugins/data/server/search/routes/search.ts +++ b/src/plugins/data/server/search/routes/search.ts @@ -102,7 +102,6 @@ export function registerSearchRoute(router: DataPluginRouter): void { await search.cancel(id, { strategy }); return res.ok(); } catch (err) { - // return reportServerError(res, err); return reportServerError(res, err); } } diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 4e52cbe989e6c6..cc4182d386b1d6 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -82,9 +82,9 @@ export const enhancedEsSearchStrategyProvider = ( return toAsyncKibanaSearchResponse({ ...body, response }, headers?.warning); }; - const cancel = () => { + const cancel = async () => { if (id && !options.isStored) { - return cancelAsyncSearch(id, esClient); + return await cancelAsyncSearch(id, esClient); } }; From a5cd1b3577b733595842849e52466451f47b5abf Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 27 Oct 2023 22:50:17 -0700 Subject: [PATCH 08/23] Swallow delete errors --- src/plugins/data/server/search/routes/search.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/data/server/search/routes/search.ts b/src/plugins/data/server/search/routes/search.ts index 8b302a81aea1ab..8ef4f8ad7fa38b 100644 --- a/src/plugins/data/server/search/routes/search.ts +++ b/src/plugins/data/server/search/routes/search.ts @@ -8,7 +8,6 @@ import { first } from 'rxjs/operators'; import { schema } from '@kbn/config-schema'; -import { reportServerError } from '@kbn/kibana-utils-plugin/server'; import { reportSearchError } from '../report_search_error'; import { getRequestAbortedSignal } from '../../lib'; import type { DataPluginRouter } from '../types'; @@ -102,7 +101,8 @@ export function registerSearchRoute(router: DataPluginRouter): void { await search.cancel(id, { strategy }); return res.ok(); } catch (err) { - return reportServerError(res, err); + // return reportServerError(res, err); + return res.ok(); } } ); From 39d61366afbf7ca222f9c7e2d023fd36a21b0c0d Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 00:00:21 -0700 Subject: [PATCH 09/23] More async fixes --- .../data/server/search/routes/search.ts | 4 ++-- .../data/server/search/search_service.ts | 18 ++++++++++------- .../eql_search/eql_search_strategy.ts | 14 +++++-------- .../ese_search/ese_search_strategy.ts | 16 ++++----------- .../sql_search/sql_search_strategy.ts | 20 ++++++------------- 5 files changed, 28 insertions(+), 44 deletions(-) diff --git a/src/plugins/data/server/search/routes/search.ts b/src/plugins/data/server/search/routes/search.ts index 8ef4f8ad7fa38b..8b302a81aea1ab 100644 --- a/src/plugins/data/server/search/routes/search.ts +++ b/src/plugins/data/server/search/routes/search.ts @@ -8,6 +8,7 @@ import { first } from 'rxjs/operators'; import { schema } from '@kbn/config-schema'; +import { reportServerError } from '@kbn/kibana-utils-plugin/server'; import { reportSearchError } from '../report_search_error'; import { getRequestAbortedSignal } from '../../lib'; import type { DataPluginRouter } from '../types'; @@ -101,8 +102,7 @@ export function registerSearchRoute(router: DataPluginRouter): void { await search.cancel(id, { strategy }); return res.ok(); } catch (err) { - // return reportServerError(res, err); - return res.ok(); + return reportServerError(res, err); } } ); diff --git a/src/plugins/data/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts index 7ed1bf32156075..48c1692b9c1d3b 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -464,14 +464,18 @@ export class SearchService implements Plugin { private cancelSessionSearches = async (deps: SearchStrategyDependencies, sessionId: string) => { const searchIdMapping = await deps.searchSessionsClient.getSearchIdMapping(sessionId); await Promise.allSettled( - Array.from(searchIdMapping).map(([searchId, strategyName]) => { - const searchOptions = { - sessionId, - strategy: strategyName, - isStored: true, - }; + Array.from(searchIdMapping).map(async ([searchId, strategyName]) => { + try { + const searchOptions = { + sessionId, + strategy: strategyName, + isStored: true, + }; - return this.cancel(deps, searchId, searchOptions); + await this.cancel(deps, searchId, searchOptions); + } catch (e) { + // Ignore errors while cancelling searches + } }) ); }; diff --git a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts index d6f5d948c784a5..95fc9dec54eaa5 100644 --- a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts @@ -27,15 +27,15 @@ export const eqlSearchStrategyProvider = ( searchConfig: SearchConfigSchema, logger: Logger ): ISearchStrategy => { - async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = esClient.asCurrentUser.eql; - await client.delete({ id }); + return client.delete({ id }).then(() => {}); } return { - cancel: async (id, options, { esClient }) => { + cancel: (id, options, { esClient }) => { logger.debug(`_eql/delete ${id}`); - await cancelAsyncSearch(id, esClient); + return cancelAsyncSearch(id, esClient); }, search: ({ id, ...request }, options: IAsyncSearchOptions, { esClient, uiSettingsClient }) => { @@ -80,11 +80,7 @@ export const eqlSearchStrategyProvider = ( return toEqlKibanaSearchResponse(response as TransportResult); }; - const cancel = async () => { - if (id) { - await cancelAsyncSearch(id, esClient); - } - }; + const cancel = () => void (id && !options.isStored && cancelAsyncSearch(id, esClient)); return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index cc4182d386b1d6..2d8e5b47f64985 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -43,13 +43,9 @@ export const enhancedEsSearchStrategyProvider = ( usage?: SearchUsage, useInternalUser: boolean = false ): ISearchStrategy => { - async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { - try { - const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - await client.asyncSearch.delete({ id }); - } catch (e) { - throw getKbnServerError(e); - } + function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; + return client.asyncSearch.delete({ id }).then(() => {}); } function asyncSearch( @@ -82,11 +78,7 @@ export const enhancedEsSearchStrategyProvider = ( return toAsyncKibanaSearchResponse({ ...body, response }, headers?.warning); }; - const cancel = async () => { - if (id && !options.isStored) { - return await cancelAsyncSearch(id, esClient); - } - }; + const cancel = () => void (id && !options.isStored && cancelAsyncSearch(id, esClient)); return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, diff --git a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts index 34134a1491cd0f..51faa9eab5aaf7 100644 --- a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts @@ -28,13 +28,9 @@ export const sqlSearchStrategyProvider = ( logger: Logger, useInternalUser: boolean = false ): ISearchStrategy => { - async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { - try { - const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - await client.sql.deleteAsync({ id }); - } catch (e) { - throw getKbnServerError(e); - } + function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; + return client.sql.deleteAsync({ id }).then(() => {}); } function asyncSearch( @@ -83,11 +79,7 @@ export const sqlSearchStrategyProvider = ( return toAsyncKibanaSearchResponse(body, startTime, headers?.warning); }; - const cancel = async () => { - if (id) { - await cancelAsyncSearch(id, esClient); - } - }; + const cancel = () => void (id && !options.isStored && cancelAsyncSearch(id, esClient)); return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, @@ -120,9 +112,9 @@ export const sqlSearchStrategyProvider = ( * @returns `Promise` * @throws `KbnServerError` */ - cancel: async (id, options, { esClient }) => { + cancel: (id, options, { esClient }) => { logger.debug(`sql search: cancel async_search_id=${id}`); - await cancelAsyncSearch(id, esClient); + return cancelAsyncSearch(id, esClient); }, /** * From 22795ca4191c7571a116ba979740eca3e6cf5633 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 08:31:01 -0700 Subject: [PATCH 10/23] Remove unnecessary changes --- .../search_interceptor/search_interceptor.ts | 6 +++--- src/plugins/data/server/search/search_service.ts | 16 ++++++---------- .../strategies/ese_search/ese_search_strategy.ts | 8 ++++++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/plugins/data/public/search/search_interceptor/search_interceptor.ts b/src/plugins/data/public/search/search_interceptor/search_interceptor.ts index 63bd973b60bae7..24b2e1c41216a8 100644 --- a/src/plugins/data/public/search/search_interceptor/search_interceptor.ts +++ b/src/plugins/data/public/search/search_interceptor/search_interceptor.ts @@ -298,9 +298,9 @@ export class SearchInterceptor { isSavedToBackground = true; }); - const sendCancelRequest = once(() => - this.deps.http.delete(`/internal/search/${strategy}/${id}`, { version: '1' }) - ); + const sendCancelRequest = once(() => { + this.deps.http.delete(`/internal/search/${strategy}/${id}`, { version: '1' }); + }); const cancel = () => id && !isSavedToBackground && sendCancelRequest(); diff --git a/src/plugins/data/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts index 48c1692b9c1d3b..08e7c34a225e83 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -465,17 +465,13 @@ export class SearchService implements Plugin { const searchIdMapping = await deps.searchSessionsClient.getSearchIdMapping(sessionId); await Promise.allSettled( Array.from(searchIdMapping).map(async ([searchId, strategyName]) => { - try { - const searchOptions = { - sessionId, - strategy: strategyName, - isStored: true, - }; + const searchOptions = { + sessionId, + strategy: strategyName, + isStored: true, + }; - await this.cancel(deps, searchId, searchOptions); - } catch (e) { - // Ignore errors while cancelling searches - } + return this.cancel(deps, searchId, searchOptions); }) ); }; diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 2d8e5b47f64985..4f7245d60eaf72 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -160,9 +160,13 @@ export const enhancedEsSearchStrategyProvider = ( * @returns `Promise` * @throws `KbnServerError` */ - cancel: (id, options, { esClient }) => { + cancel: async (id, options, { esClient }) => { logger.debug(`cancel ${id}`); - return cancelAsyncSearch(id, esClient); + try { + await cancelAsyncSearch(id, esClient); + } catch (e) { + throw getKbnServerError(e); + } }, /** * From f3f083980059ae2a0f617e4b23195512b92c7302 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 09:48:10 -0700 Subject: [PATCH 11/23] Use async/await again --- src/plugins/data/server/search/search_service.ts | 2 +- .../search/strategies/eql_search/eql_search_strategy.ts | 6 +++--- .../search/strategies/ese_search/ese_search_strategy.ts | 6 +++--- .../search/strategies/sql_search/sql_search_strategy.ts | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/plugins/data/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts index 08e7c34a225e83..7ed1bf32156075 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -464,7 +464,7 @@ export class SearchService implements Plugin { private cancelSessionSearches = async (deps: SearchStrategyDependencies, sessionId: string) => { const searchIdMapping = await deps.searchSessionsClient.getSearchIdMapping(sessionId); await Promise.allSettled( - Array.from(searchIdMapping).map(async ([searchId, strategyName]) => { + Array.from(searchIdMapping).map(([searchId, strategyName]) => { const searchOptions = { sessionId, strategy: strategyName, diff --git a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts index 95fc9dec54eaa5..2670d7e5c7f420 100644 --- a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts @@ -27,9 +27,9 @@ export const eqlSearchStrategyProvider = ( searchConfig: SearchConfigSchema, logger: Logger ): ISearchStrategy => { - function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = esClient.asCurrentUser.eql; - return client.delete({ id }).then(() => {}); + await client.delete({ id }); } return { @@ -80,7 +80,7 @@ export const eqlSearchStrategyProvider = ( return toEqlKibanaSearchResponse(response as TransportResult); }; - const cancel = () => void (id && !options.isStored && cancelAsyncSearch(id, esClient)); + const cancel = () => id && !options.isStored && cancelAsyncSearch(id, esClient); return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 4f7245d60eaf72..9f0cabc44ccb0c 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -43,9 +43,9 @@ export const enhancedEsSearchStrategyProvider = ( usage?: SearchUsage, useInternalUser: boolean = false ): ISearchStrategy => { - function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - return client.asyncSearch.delete({ id }).then(() => {}); + await client.asyncSearch.delete({ id }); } function asyncSearch( @@ -78,7 +78,7 @@ export const enhancedEsSearchStrategyProvider = ( return toAsyncKibanaSearchResponse({ ...body, response }, headers?.warning); }; - const cancel = () => void (id && !options.isStored && cancelAsyncSearch(id, esClient)); + const cancel = () => id && !options.isStored && cancelAsyncSearch(id, esClient); return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, diff --git a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts index 51faa9eab5aaf7..d333d60e42c1c9 100644 --- a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts @@ -28,9 +28,9 @@ export const sqlSearchStrategyProvider = ( logger: Logger, useInternalUser: boolean = false ): ISearchStrategy => { - function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - return client.sql.deleteAsync({ id }).then(() => {}); + await client.sql.deleteAsync({ id }); } function asyncSearch( @@ -79,7 +79,7 @@ export const sqlSearchStrategyProvider = ( return toAsyncKibanaSearchResponse(body, startTime, headers?.warning); }; - const cancel = () => void (id && !options.isStored && cancelAsyncSearch(id, esClient)); + const cancel = () => id && !options.isStored && cancelAsyncSearch(id, esClient); return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, From 4450a9b34632dd11dc87361209ea6f576aade434 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 16:50:54 -0700 Subject: [PATCH 12/23] Change one little thing --- .../search/strategies/ese_search/ese_search_strategy.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 9f0cabc44ccb0c..53459fd62b166d 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -43,9 +43,9 @@ export const enhancedEsSearchStrategyProvider = ( usage?: SearchUsage, useInternalUser: boolean = false ): ISearchStrategy => { - async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - await client.asyncSearch.delete({ id }); + return client.asyncSearch.delete({ id }).then(() => {}); } function asyncSearch( From 0a84ff18e12faaa641de1611b64bbf717819946c Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 17:41:24 -0700 Subject: [PATCH 13/23] Another test --- .../server/search/strategies/ese_search/ese_search_strategy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 53459fd62b166d..4f7245d60eaf72 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -78,7 +78,7 @@ export const enhancedEsSearchStrategyProvider = ( return toAsyncKibanaSearchResponse({ ...body, response }, headers?.warning); }; - const cancel = () => id && !options.isStored && cancelAsyncSearch(id, esClient); + const cancel = () => void (id && !options.isStored && cancelAsyncSearch(id, esClient)); return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, From 79697de17152d2f2da79aa8f9bbec783a1bdaef7 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 18:58:03 -0700 Subject: [PATCH 14/23] another thingy --- .../search/strategies/ese_search/ese_search_strategy.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 4f7245d60eaf72..15faea79091ad9 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -45,7 +45,7 @@ export const enhancedEsSearchStrategyProvider = ( ): ISearchStrategy => { function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - return client.asyncSearch.delete({ id }).then(() => {}); + return client.asyncSearch.delete({ id }); } function asyncSearch( @@ -78,7 +78,9 @@ export const enhancedEsSearchStrategyProvider = ( return toAsyncKibanaSearchResponse({ ...body, response }, headers?.warning); }; - const cancel = () => void (id && !options.isStored && cancelAsyncSearch(id, esClient)); + const cancel = () => { + if (id && !options.isStored) cancelAsyncSearch(id, esClient); + }; return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, From 04eccf7c8a8bd7d604a6155bdd293b33334abad6 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 20:16:02 -0700 Subject: [PATCH 15/23] Re-add .then --- .../server/search/strategies/ese_search/ese_search_strategy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 15faea79091ad9..6cfd00e411316c 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -45,7 +45,7 @@ export const enhancedEsSearchStrategyProvider = ( ): ISearchStrategy => { function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - return client.asyncSearch.delete({ id }); + return client.asyncSearch.delete({ id }).then(() => {}); } function asyncSearch( From 0a13dbe413ff8a3ae7c32d45447d87191adcea1c Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 21:47:54 -0700 Subject: [PATCH 16/23] Consistent handling in search strategies --- .../eql_search/eql_search_strategy.ts | 20 ++++++++++++++----- .../ese_search/ese_search_strategy.ts | 7 +++++-- .../sql_search/sql_search_strategy.ts | 19 +++++++++++++----- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts index 2670d7e5c7f420..627eb716171850 100644 --- a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts @@ -9,6 +9,7 @@ import type { TransportResult } from '@elastic/elasticsearch'; import { tap } from 'rxjs/operators'; import type { IScopedClusterClient, Logger } from '@kbn/core/server'; +import { getKbnServerError } from '@kbn/kibana-utils-plugin/server'; import { SearchConfigSchema } from '../../../../config'; import { EqlSearchStrategyRequest, @@ -27,15 +28,19 @@ export const eqlSearchStrategyProvider = ( searchConfig: SearchConfigSchema, logger: Logger ): ISearchStrategy => { - async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = esClient.asCurrentUser.eql; - await client.delete({ id }); + return client.delete({ id }); } return { - cancel: (id, options, { esClient }) => { + cancel: async (id, options, { esClient }) => { logger.debug(`_eql/delete ${id}`); - return cancelAsyncSearch(id, esClient); + try { + await cancelAsyncSearch(id, esClient); + } catch (e) { + throw getKbnServerError(e); + } }, search: ({ id, ...request }, options: IAsyncSearchOptions, { esClient, uiSettingsClient }) => { @@ -80,7 +85,12 @@ export const eqlSearchStrategyProvider = ( return toEqlKibanaSearchResponse(response as TransportResult); }; - const cancel = () => id && !options.isStored && cancelAsyncSearch(id, esClient); + const cancel = () => { + if (!id || options.isStored) return; + cancelAsyncSearch(id, esClient).catch(() => { + // Swallow errors from cancellation + }); + }; return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 6cfd00e411316c..860969fb7d853d 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -45,7 +45,7 @@ export const enhancedEsSearchStrategyProvider = ( ): ISearchStrategy => { function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - return client.asyncSearch.delete({ id }).then(() => {}); + return client.asyncSearch.delete({ id }); } function asyncSearch( @@ -79,7 +79,10 @@ export const enhancedEsSearchStrategyProvider = ( }; const cancel = () => { - if (id && !options.isStored) cancelAsyncSearch(id, esClient); + if (!id || options.isStored) return; + cancelAsyncSearch(id, esClient).catch(() => { + // Swallow errors from cancellation + }); }; return pollSearch(search, cancel, { diff --git a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts index d333d60e42c1c9..7301d1c320e3dc 100644 --- a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts @@ -28,9 +28,9 @@ export const sqlSearchStrategyProvider = ( logger: Logger, useInternalUser: boolean = false ): ISearchStrategy => { - async function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { + function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; - await client.sql.deleteAsync({ id }); + return client.sql.deleteAsync({ id }); } function asyncSearch( @@ -79,7 +79,12 @@ export const sqlSearchStrategyProvider = ( return toAsyncKibanaSearchResponse(body, startTime, headers?.warning); }; - const cancel = () => id && !options.isStored && cancelAsyncSearch(id, esClient); + const cancel = () => { + if (!id || options.isStored) return; + cancelAsyncSearch(id, esClient).catch(() => { + // Swallow errors from cancellation + }); + }; return pollSearch(search, cancel, { pollInterval: searchConfig.asyncSearch.pollInterval, @@ -112,9 +117,13 @@ export const sqlSearchStrategyProvider = ( * @returns `Promise` * @throws `KbnServerError` */ - cancel: (id, options, { esClient }) => { + cancel: async (id, options, { esClient }) => { logger.debug(`sql search: cancel async_search_id=${id}`); - return cancelAsyncSearch(id, esClient); + try { + await cancelAsyncSearch(id, esClient); + } catch (e) { + throw getKbnServerError(e); + } }, /** * From 78355841cca5e0863a6977b613555b6a01053f91 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Sat, 28 Oct 2023 23:11:25 -0700 Subject: [PATCH 17/23] Swallow errors during cancelSessionSearches --- src/plugins/data/server/search/search_service.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plugins/data/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts index 7ed1bf32156075..924470689ad48e 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -464,14 +464,16 @@ export class SearchService implements Plugin { private cancelSessionSearches = async (deps: SearchStrategyDependencies, sessionId: string) => { const searchIdMapping = await deps.searchSessionsClient.getSearchIdMapping(sessionId); await Promise.allSettled( - Array.from(searchIdMapping).map(([searchId, strategyName]) => { + Array.from(searchIdMapping).map(async ([searchId, strategyName]) => { const searchOptions = { sessionId, strategy: strategyName, isStored: true, }; - return this.cancel(deps, searchId, searchOptions); + return this.cancel(deps, searchId, searchOptions).catch(() => { + // Some strategies don't support cancellation - swallow these errors + }); }) ); }; From aa3817f30dfd3feeafbe5ba812879dd3dff40de8 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Thu, 2 Nov 2023 19:38:14 -0700 Subject: [PATCH 18/23] Review feedback --- .../data/server/search/search_service.ts | 12 +++++-- .../eql_search/eql_search_strategy.test.ts | 33 +++++++++++++++++++ .../ese_search/ese_search_strategy.ts | 8 +++-- .../sql_search/sql_search_strategy.test.ts | 27 +++++++++++++++ 4 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/plugins/data/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts index 924470689ad48e..3bbefc3a9957e0 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -471,9 +471,15 @@ export class SearchService implements Plugin { isStored: true, }; - return this.cancel(deps, searchId, searchOptions).catch(() => { - // Some strategies don't support cancellation - swallow these errors - }); + try { + await this.cancel(deps, searchId, searchOptions); + } catch (e) { + // A 404 means either this search request does not exist, or that it is already cancelled + if (e.meta?.statusCode === 404) return; + + // Log all other (unexpected) error messages + this.logger.error(`cancelSessionSearches error: ${e.message}`); + } }) ); }; diff --git a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.test.ts b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.test.ts index 6d61f62cc79abb..475c43a5daed68 100644 --- a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.test.ts +++ b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.test.ts @@ -15,6 +15,7 @@ import { getMockSearchConfig } from '../../../../config.mock'; const getMockEqlResponse = () => ({ body: { + id: 'my-search-id', is_partial: false, is_running: false, took: 162, @@ -54,6 +55,7 @@ describe('EQL search strategy', () => { describe('search()', () => { let mockEqlSearch: jest.Mock; let mockEqlGet: jest.Mock; + let mockEqlDelete: jest.Mock; let mockDeps: SearchStrategyDependencies; let params: Required['params']; let options: Required['options']; @@ -61,6 +63,8 @@ describe('EQL search strategy', () => { beforeEach(() => { mockEqlSearch = jest.fn().mockResolvedValueOnce(getMockEqlResponse()); mockEqlGet = jest.fn().mockResolvedValueOnce(getMockEqlResponse()); + mockEqlDelete = jest.fn(); + mockDeps = { uiSettingsClient: { get: jest.fn(), @@ -70,6 +74,7 @@ describe('EQL search strategy', () => { eql: { get: mockEqlGet, search: mockEqlSearch, + delete: mockEqlDelete, }, }, }, @@ -124,6 +129,34 @@ describe('EQL search strategy', () => { }); }); + it('should delete when aborted', async () => { + const response = getMockEqlResponse(); + mockEqlSearch.mockReset().mockResolvedValueOnce({ + ...response, + body: { + ...response.body, + is_running: true, + }, + }); + const eqlSearch = await eqlSearchStrategyProvider(mockSearchConfig, mockLogger); + const abortController = new AbortController(); + const abortSignal = abortController.signal; + + // Abort after an incomplete first response is returned + setTimeout(() => abortController.abort(), 100); + + let err: any; + try { + await eqlSearch.search({ options, params }, { abortSignal }, mockDeps).toPromise(); + } catch (e) { + err = e; + } + + expect(mockEqlSearch).toBeCalled(); + expect(err).not.toBeUndefined(); + expect(mockEqlDelete).toBeCalled(); + }); + describe('arguments', () => { it('sends along async search options', async () => { const eqlSearch = await eqlSearchStrategyProvider(mockSearchConfig, mockLogger); diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 860969fb7d853d..97a17168508583 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -80,8 +80,12 @@ export const enhancedEsSearchStrategyProvider = ( const cancel = () => { if (!id || options.isStored) return; - cancelAsyncSearch(id, esClient).catch(() => { - // Swallow errors from cancellation + cancelAsyncSearch(id, esClient).catch((e) => { + // A 404 means either this search request does not exist, or that it is already cancelled + if (e.meta?.statusCode === 404) return; + + // Log all other (unexpected) error messages + logger.error(`cancelAsyncSearch error: ${e.message}`); }); }; diff --git a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.test.ts b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.test.ts index 700c658de10c04..36fb43a34894ff 100644 --- a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.test.ts +++ b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.test.ts @@ -124,6 +124,33 @@ describe('SQL search strategy', () => { signal: undefined, }); }); + + it('should delete when aborted', async () => { + mockSqlQuery.mockResolvedValueOnce({ + ...mockSqlResponse, + body: { + ...mockSqlResponse.body, + is_running: true, + }, + }); + const esSearch = await sqlSearchStrategyProvider(mockSearchConfig, mockLogger); + const abortController = new AbortController(); + const abortSignal = abortController.signal; + + // Abort after an incomplete first response is returned + setTimeout(() => abortController.abort(), 100); + + let err: any; + try { + await esSearch.search({ params: {} }, { abortSignal }, mockDeps).toPromise(); + } catch (e) { + err = e; + } + + expect(mockSqlQuery).toBeCalled(); + expect(err).not.toBeUndefined(); + expect(mockSqlDelete).toBeCalled(); + }); }); // skip until full search session support https://github.com/elastic/kibana/issues/127880 From 34aa4dc1be794654e10797ac374f1c66d2f31032 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 3 Nov 2023 15:34:22 -0700 Subject: [PATCH 19/23] Add changes to other search strategies --- src/plugins/data/server/search/search_service.ts | 4 ---- .../strategies/eql_search/eql_search_strategy.ts | 10 +++++++--- .../strategies/sql_search/sql_search_strategy.ts | 10 +++++++--- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/plugins/data/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts index 3bbefc3a9957e0..188f853e6a2ce2 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -474,10 +474,6 @@ export class SearchService implements Plugin { try { await this.cancel(deps, searchId, searchOptions); } catch (e) { - // A 404 means either this search request does not exist, or that it is already cancelled - if (e.meta?.statusCode === 404) return; - - // Log all other (unexpected) error messages this.logger.error(`cancelSessionSearches error: ${e.message}`); } }) diff --git a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts index 627eb716171850..398b757f530ef4 100644 --- a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts @@ -86,9 +86,13 @@ export const eqlSearchStrategyProvider = ( }; const cancel = () => { - if (!id || options.isStored) return; - cancelAsyncSearch(id, esClient).catch(() => { - // Swallow errors from cancellation + if (!id) return; + cancelAsyncSearch(id, esClient).catch((e) => { + // A 404 means either this search request does not exist, or that it is already cancelled + if (e.meta?.statusCode === 404) return; + + // Log all other (unexpected) error messages + logger.error(`cancelEqlSearch error: ${e.message}`); }); }; diff --git a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts index 7301d1c320e3dc..bea08dc11cf505 100644 --- a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts @@ -80,9 +80,13 @@ export const sqlSearchStrategyProvider = ( }; const cancel = () => { - if (!id || options.isStored) return; - cancelAsyncSearch(id, esClient).catch(() => { - // Swallow errors from cancellation + if (!id) return; + cancelAsyncSearch(id, esClient).catch((e) => { + // A 404 means either this search request does not exist, or that it is already cancelled + if (e.meta?.statusCode === 404) return; + + // Log all other (unexpected) error messages + logger.error(`cancelSqlSearch error: ${e.message}`); }); }; From 7fc746a203dd7063e5bb898d0c5f62ae517fc9f3 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 3 Nov 2023 16:46:59 -0700 Subject: [PATCH 20/23] Add API integration test --- .../ese_search/ese_search_strategy.ts | 5 +- .../api_integration/apis/search/search.ts | 65 +++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 97a17168508583..131cb4691af65f 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -57,7 +57,10 @@ export const enhancedEsSearchStrategyProvider = ( const search = async () => { const params = id - ? getDefaultAsyncGetParams(searchConfig, options) + ? { + ...getDefaultAsyncGetParams(searchConfig, options), + ...request.params, + } : { ...(await getDefaultAsyncSubmitParams(uiSettingsClient, searchConfig, options)), ...request.params, diff --git a/x-pack/test/api_integration/apis/search/search.ts b/x-pack/test/api_integration/apis/search/search.ts index 391923601d7c59..976c9339d0c82c 100644 --- a/x-pack/test/api_integration/apis/search/search.ts +++ b/x-pack/test/api_integration/apis/search/search.ts @@ -186,6 +186,71 @@ export default function ({ getService }: FtrProviderContext) { expect(resp2.body.isRunning).to.be(true); }); + it('should cancel an async search without server crash', async function () { + await markRequiresShardDelayAgg(this); + + const resp = await supertest + .post(`/internal/search/ese`) + .set(ELASTIC_HTTP_VERSION_HEADER, '1') + .set('kbn-xsrf', 'foo') + .send({ + params: { + body: { + query: { + match_all: {}, + }, + ...shardDelayAgg('10s'), + }, + wait_for_completion_timeout: '1ms', + }, + }) + .expect(200); + + const { id } = resp.body; + expect(id).not.to.be(undefined); + expect(resp.body.isPartial).to.be(true); + expect(resp.body.isRunning).to.be(true); + + // Send a follow-up request that waits up to 10s for completion + const req = supertest + .post(`/internal/search/ese/${id}`) + .set(ELASTIC_HTTP_VERSION_HEADER, '1') + .set('kbn-xsrf', 'foo') + .send({ params: { wait_for_completion_timeout: '10s' } }) + .expect(200); + + // After 2s, abort and send the cancellation (to result in a race towards cancellation) + // This should be swallowed and not kill the Kibana server + await new Promise((resolve) => + setTimeout(() => { + req.abort(); + resolve(null); + }, 2000) + ); + await supertest + .delete(`/internal/search/ese/${id}`) + .set(ELASTIC_HTTP_VERSION_HEADER, '1') + .set('kbn-xsrf', 'foo') + .expect(200); + + let err: Error; + try { + await req; + } catch (e) { + err = e; + } + + expect(err).not.to.be(undefined); + + // Ensure the search was succesfully cancelled + await supertest + .post(`/internal/search/ese/${id}`) + .set(ELASTIC_HTTP_VERSION_HEADER, '1') + .set('kbn-xsrf', 'foo') + .send({}) + .expect(404); + }); + it('should fail without kbn-xref header', async () => { const resp = await supertest .post(`/internal/search/ese`) From 901983a80228c76b274b5bad80f87680026d3862 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Tue, 7 Nov 2023 13:32:40 -0700 Subject: [PATCH 21/23] Fix undefined check --- x-pack/test/api_integration/apis/search/search.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/test/api_integration/apis/search/search.ts b/x-pack/test/api_integration/apis/search/search.ts index 976c9339d0c82c..15c774eef34ef9 100644 --- a/x-pack/test/api_integration/apis/search/search.ts +++ b/x-pack/test/api_integration/apis/search/search.ts @@ -233,7 +233,7 @@ export default function ({ getService }: FtrProviderContext) { .set('kbn-xsrf', 'foo') .expect(200); - let err: Error; + let err: Error | undefined; try { await req; } catch (e) { From b9fcdc34d6ce3255f421e5d5d238ca233abef304 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Tue, 7 Nov 2023 15:44:57 -0700 Subject: [PATCH 22/23] Clean up cancel methods --- .../search/strategies/eql_search/eql_search_strategy.ts | 8 +++++--- .../search/strategies/ese_search/ese_search_strategy.ts | 8 +++++--- .../search/strategies/sql_search/sql_search_strategy.ts | 8 +++++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts index e52dc775e0862f..00b8cfdeb52e5c 100644 --- a/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts @@ -89,15 +89,17 @@ export const eqlSearchStrategyProvider = ( ); }; - const cancel = () => { + const cancel = async () => { if (!id) return; - cancelAsyncSearch(id, esClient).catch((e) => { + try { + await cancelAsyncSearch(id, esClient); + } catch (e) { // A 404 means either this search request does not exist, or that it is already cancelled if (e.meta?.statusCode === 404) return; // Log all other (unexpected) error messages logger.error(`cancelEqlSearch error: ${e.message}`); - }); + } }; return pollSearch(search, cancel, { diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index e5c92d19168b6f..4efd28b4c1c554 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -87,15 +87,17 @@ export const enhancedEsSearchStrategyProvider = ( ); }; - const cancel = () => { + const cancel = async () => { if (!id || options.isStored) return; - cancelAsyncSearch(id, esClient).catch((e) => { + try { + await cancelAsyncSearch(id, esClient); + } catch (e) { // A 404 means either this search request does not exist, or that it is already cancelled if (e.meta?.statusCode === 404) return; // Log all other (unexpected) error messages logger.error(`cancelAsyncSearch error: ${e.message}`); - }); + } }; return pollSearch(search, cancel, { diff --git a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts index 7a850049ec30a0..9e04675d12247f 100644 --- a/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/sql_search/sql_search_strategy.ts @@ -87,15 +87,17 @@ export const sqlSearchStrategyProvider = ( ); }; - const cancel = () => { + const cancel = async () => { if (!id) return; - cancelAsyncSearch(id, esClient).catch((e) => { + try { + await cancelAsyncSearch(id, esClient); + } catch (e) { // A 404 means either this search request does not exist, or that it is already cancelled if (e.meta?.statusCode === 404) return; // Log all other (unexpected) error messages logger.error(`cancelSqlSearch error: ${e.message}`); - }); + } }; return pollSearch(search, cancel, { From cce4cd1b65801b46aa06011ccc8bb7e56ae935d1 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Tue, 7 Nov 2023 16:48:49 -0700 Subject: [PATCH 23/23] Fix overriding of keep_alive and wait_for_completion_timeout --- .../search/strategies/es_search/types.ts | 3 ++- .../ese_search/ese_search_strategy.test.ts | 24 +++++++++++++++++++ .../ese_search/ese_search_strategy.ts | 10 +++++--- .../search/strategies/ese_search/types.ts | 16 ++++++++++--- 4 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/plugins/data/common/search/strategies/es_search/types.ts b/src/plugins/data/common/search/strategies/es_search/types.ts index 73bf7961fea9b8..f8c3b73d995a9f 100644 --- a/src/plugins/data/common/search/strategies/es_search/types.ts +++ b/src/plugins/data/common/search/strategies/es_search/types.ts @@ -15,7 +15,8 @@ export type ISearchRequestParams = { trackTotalHits?: boolean; } & estypes.SearchRequest; -export interface IEsSearchRequest extends IKibanaSearchRequest { +export interface IEsSearchRequest + extends IKibanaSearchRequest { indexType?: string; } diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts index 6c1746984c86bc..33987c09d88ddf 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts @@ -136,6 +136,30 @@ describe('ES search strategy', () => { expect(request).toHaveProperty('keep_alive', '60000ms'); }); + it('allows overriding keep_alive and wait_for_completion_timeout', async () => { + mockGetCaller.mockResolvedValueOnce(mockAsyncResponse); + + const params = { + index: 'logstash-*', + body: { query: {} }, + wait_for_completion_timeout: '10s', + keep_alive: '5m', + }; + const esSearch = await enhancedEsSearchStrategyProvider( + mockLegacyConfig$, + mockSearchConfig, + mockLogger + ); + + await esSearch.search({ id: 'foo', params }, {}, mockDeps).toPromise(); + + expect(mockGetCaller).toBeCalled(); + const request = mockGetCaller.mock.calls[0][0]; + expect(request.id).toEqual('foo'); + expect(request).toHaveProperty('wait_for_completion_timeout', '10s'); + expect(request).toHaveProperty('keep_alive', '5m'); + }); + it('sets transport options on POST requests', async () => { const transportOptions = { maxRetries: 1 }; mockSubmitCaller.mockResolvedValueOnce(mockAsyncResponse); diff --git a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts index 4efd28b4c1c554..174f9924f1cc7b 100644 --- a/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts +++ b/src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts @@ -12,6 +12,7 @@ import { catchError, tap } from 'rxjs/operators'; import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; import { firstValueFrom, from } from 'rxjs'; import { getKbnServerError } from '@kbn/kibana-utils-plugin/server'; +import { IAsyncSearchRequestParams } from '../..'; import { getKbnSearchError, KbnSearchError } from '../../report_search_error'; import type { ISearchStrategy, SearchStrategyDependencies } from '../../types'; import type { @@ -43,14 +44,14 @@ export const enhancedEsSearchStrategyProvider = ( logger: Logger, usage?: SearchUsage, useInternalUser: boolean = false -): ISearchStrategy => { +): ISearchStrategy> => { function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) { const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser; return client.asyncSearch.delete({ id }); } function asyncSearch( - { id, ...request }: IEsSearchRequest, + { id, ...request }: IEsSearchRequest, options: IAsyncSearchOptions, { esClient, uiSettingsClient }: SearchStrategyDependencies ) { @@ -60,7 +61,10 @@ export const enhancedEsSearchStrategyProvider = ( const params = id ? { ...getDefaultAsyncGetParams(searchConfig, options), - ...request.params, + ...(request.params?.keep_alive ? { keep_alive: request.params.keep_alive } : {}), + ...(request.params?.wait_for_completion_timeout + ? { wait_for_completion_timeout: request.params.wait_for_completion_timeout } + : {}), } : { ...(await getDefaultAsyncSubmitParams(uiSettingsClient, searchConfig, options)), diff --git a/src/plugins/data/server/search/strategies/ese_search/types.ts b/src/plugins/data/server/search/strategies/ese_search/types.ts index 4116aa43803397..5ff324e1c2e4f0 100644 --- a/src/plugins/data/server/search/strategies/ese_search/types.ts +++ b/src/plugins/data/server/search/strategies/ese_search/types.ts @@ -6,11 +6,21 @@ * Side Public License, v 1. */ -import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; +import type { + AsyncSearchGetRequest, + SearchResponse, + ShardStatistics, +} from '@elastic/elasticsearch/lib/api/types'; +import { ISearchRequestParams } from '../../../../common'; + +export interface IAsyncSearchRequestParams extends ISearchRequestParams { + keep_alive?: AsyncSearchGetRequest['keep_alive']; + wait_for_completion_timeout?: AsyncSearchGetRequest['wait_for_completion_timeout']; +} export interface AsyncSearchResponse { id?: string; - response: estypes.SearchResponse; + response: SearchResponse; start_time_in_millis: number; expiration_time_in_millis: number; is_partial: boolean; @@ -18,5 +28,5 @@ export interface AsyncSearchResponse { } export interface AsyncSearchStatusResponse extends Omit { completion_status: number; - _shards: estypes.ShardStatistics; + _shards: ShardStatistics; }