Review feedback

elastic · Nov 3, 2023 · aa3817f · aa3817f
1 parent 8623348
commit aa3817f
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 5 deletions.
diff --git 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<ISearchSetup, ISearchStart> {
           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
@@ -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,13 +55,16 @@ describe('EQL search strategy', () => {
   describe('search()', () => {
     let mockEqlSearch: jest.Mock;
     let mockEqlGet: jest.Mock;
+    let mockEqlDelete: jest.Mock;
     let mockDeps: SearchStrategyDependencies;
     let params: Required<EqlSearchStrategyRequest>['params'];
     let options: Required<EqlSearchStrategyRequest>['options'];
 
     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
@@ -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
@@ -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