Merge branch 'main' into 149814

src/core/server/integration_tests/saved_objects/migrations/group2/check_registered_types.test.ts

@@ -57,7 +57,7 @@ describe('checking migration metadata changes on all registered SO types', () =>
       Object {
         "action": "6cfc277ed3211639e37546ac625f4a68f2494215",
         "action_task_params": "db2afea7d78e00e725486b791554d0d4e81956ef",
-        "alert": "2568bf6d8ba0876441c61c9e58e08016c1dc1617",
+        "alert": "785240e3137f5eb1a0f8986e5b8eff99780fc04f",
         "api_key_pending_invalidation": "16e7bcf8e78764102d7f525542d5b616809a21ee",
         "apm-indices": "d19dd7fb51f2d2cbc1f8769481721e0953f9a6d2",
         "apm-server-schema": "1d42f17eff9ec6c16d3a9324d9539e2d123d0a9a",

x-pack/plugins/actions/server/action_type_registry.test.ts

@@ -8,7 +8,7 @@
 import { taskManagerMock } from '@kbn/task-manager-plugin/server/mocks';
 import { ActionTypeRegistry, ActionTypeRegistryOpts } from './action_type_registry';
 import { ActionType, ExecutorType } from './types';
-import { ActionExecutor, ExecutorError, ILicenseState, TaskRunnerFactory } from './lib';
+import { ActionExecutor, ILicenseState, TaskRunnerFactory } from './lib';
 import { actionsConfigMock } from './actions_config.mock';
 import { licenseStateMock } from './lib/license_state.mock';
 import { ActionsConfigurationUtilities } from './actions_config';
@@ -70,7 +70,6 @@ describe('actionTypeRegistry', () => {
         Object {
           "actions:my-action-type": Object {
             "createTaskRunner": [Function],
-            "getRetry": [Function],
             "maxAttempts": 3,
             "title": "My action type",
           },
@@ -149,45 +148,6 @@ describe('actionTypeRegistry', () => {
       );
     });
 
-    test('provides a getRetry function that handles ExecutorError', () => {
-      const actionTypeRegistry = new ActionTypeRegistry(actionTypeRegistryParams);
-      actionTypeRegistry.register({
-        id: 'my-action-type',
-        name: 'My action type',
-        minimumLicenseRequired: 'basic',
-        supportedFeatureIds: ['alerting'],
-        executor,
-      });
-      expect(mockTaskManager.registerTaskDefinitions).toHaveBeenCalledTimes(1);
-      const registerTaskDefinitionsCall = mockTaskManager.registerTaskDefinitions.mock.calls[0][0];
-      const getRetry = registerTaskDefinitionsCall['actions:my-action-type'].getRetry!;
-
-      const retryTime = new Date();
-      expect(getRetry(0, new Error())).toEqual(true);
-      expect(getRetry(0, new ExecutorError('my message', {}, true))).toEqual(true);
-      expect(getRetry(0, new ExecutorError('my message', {}, false))).toEqual(false);
-      expect(getRetry(0, new ExecutorError('my message', {}, undefined))).toEqual(false);
-      expect(getRetry(0, new ExecutorError('my message', {}, retryTime))).toEqual(retryTime);
-    });
-
-    test('provides a getRetry function that handles errors based on maxAttempts', () => {
-      const actionTypeRegistry = new ActionTypeRegistry(actionTypeRegistryParams);
-      actionTypeRegistry.register({
-        id: 'my-action-type',
-        name: 'My action type',
-        minimumLicenseRequired: 'basic',
-        supportedFeatureIds: ['alerting'],
-        executor,
-        maxAttempts: 2,
-      });
-      expect(mockTaskManager.registerTaskDefinitions).toHaveBeenCalledTimes(1);
-      const registerTaskDefinitionsCall = mockTaskManager.registerTaskDefinitions.mock.calls[0][0];
-      const getRetry = registerTaskDefinitionsCall['actions:my-action-type'].getRetry!;
-
-      expect(getRetry(1, new Error())).toEqual(true);
-      expect(getRetry(3, new Error())).toEqual(false);
-    });
-
     test('registers gold+ action types to the licensing feature usage API', () => {
       const actionTypeRegistry = new ActionTypeRegistry(actionTypeRegistryParams);
       actionTypeRegistry.register({

x-pack/plugins/actions/server/action_type_registry.ts

@@ -11,12 +11,7 @@ import { RunContext, TaskManagerSetupContract } from '@kbn/task-manager-plugin/s
 import { LicensingPluginSetup } from '@kbn/licensing-plugin/server';
 import { ActionType as CommonActionType, areValidFeatures } from '../common';
 import { ActionsConfigurationUtilities } from './actions_config';
-import {
-  ExecutorError,
-  getActionTypeFeatureUsageName,
-  TaskRunnerFactory,
-  ILicenseState,
-} from './lib';
+import { getActionTypeFeatureUsageName, TaskRunnerFactory, ILicenseState } from './lib';
 import {
   ActionType,
   PreConfiguredAction,
@@ -157,13 +152,6 @@ export class ActionTypeRegistry {
       [`actions:${actionType.id}`]: {
         title: actionType.name,
         maxAttempts,
-        getRetry(attempts: number, error: unknown) {
-          if (error instanceof ExecutorError) {
-            return error.retry == null ? false : error.retry;
-          }
-          // Only retry other kinds of errors based on attempts
-          return attempts < maxAttempts;
-        },
         createTaskRunner: (context: RunContext) =>
           this.taskRunnerFactory.create(context, maxAttempts),
       },

x-pack/plugins/actions/server/lib/index.ts

@@ -5,7 +5,6 @@
  * 2.0.
  */
 
-export { ExecutorError } from './executor_error';
 export {
   validateParams,
   validateConfig,

x-pack/plugins/actions/server/lib/task_runner_factory.test.ts

@@ -6,7 +6,6 @@
  */
 
 import sinon from 'sinon';
-import { ExecutorError } from './executor_error';
 import { ActionExecutor } from './action_executor';
 import { ConcreteTaskInstance, TaskStatus } from '@kbn/task-manager-plugin/server';
 import { TaskRunnerFactory } from './task_runner_factory';
@@ -20,6 +19,7 @@ import { actionsClientMock } from '../mocks';
 import { inMemoryMetricsMock } from '../monitoring/in_memory_metrics.mock';
 import { IN_MEMORY_METRICS } from '../monitoring';
 import { pick } from 'lodash';
+import { isRetryableError } from '@kbn/task-manager-plugin/server/task_running';
 
 const executeParamsFields = [
   'actionId',
@@ -421,9 +421,7 @@ test('throws an error with suggested retry logic when return status is error', a
     await taskRunner.run();
     throw new Error('Should have thrown');
   } catch (e) {
-    expect(e instanceof ExecutorError).toEqual(true);
-    expect(e.data).toEqual({ foo: true });
-    expect(e.retry).toEqual(false);
+    expect(isRetryableError(e)).toEqual(false);
   }
 });
 
@@ -728,13 +726,11 @@ test(`throws an error when license doesn't support the action type`, async () =>
     await taskRunner.run();
     throw new Error('Should have thrown');
   } catch (e) {
-    expect(e instanceof ExecutorError).toEqual(true);
-    expect(e.data).toEqual({});
-    expect(e.retry).toEqual(true);
+    expect(isRetryableError(e)).toEqual(true);
   }
 });
 
-test(`treats errors as errors if the task is retryable`, async () => {
+test(`will throw an error with retry: false if the task is not retryable`, async () => {
   const taskRunner = taskRunnerFactory.create({
     taskInstance: {
       ...mockedTaskInstance,
@@ -774,9 +770,7 @@ test(`treats errors as errors if the task is retryable`, async () => {
     err = e;
   }
   expect(err).toBeDefined();
-  expect(err instanceof ExecutorError).toEqual(true);
-  expect(err.data).toEqual({ foo: true });
-  expect(err.retry).toEqual(false);
+  expect(isRetryableError(err)).toEqual(false);
   expect(taskRunnerFactoryInitializerParams.logger.error as jest.Mock).toHaveBeenCalledWith(
     `Action '2' failed and will not retry: Error message`
   );
@@ -827,7 +821,7 @@ test(`treats errors as successes if the task is not retryable`, async () => {
   );
 });
 
-test('treats errors as errors if the error is thrown instead of returned', async () => {
+test('will throw a retry error if the error is thrown instead of returned', async () => {
   const taskRunner = taskRunnerFactory.create({
     taskInstance: {
       ...mockedTaskInstance,
@@ -861,9 +855,7 @@ test('treats errors as errors if the error is thrown instead of returned', async
     err = e;
   }
   expect(err).toBeDefined();
-  expect(err instanceof ExecutorError).toEqual(true);
-  expect(err.data).toEqual({});
-  expect(err.retry).toEqual(true);
+  expect(isRetryableError(err)).toEqual(true);
   expect(taskRunnerFactoryInitializerParams.logger.error as jest.Mock).toHaveBeenCalledWith(
     `Action '2' failed and will retry: undefined`
   );

x-pack/plugins/actions/server/lib/task_runner_factory.ts

@@ -23,8 +23,8 @@ import {
 } from '@kbn/core/server';
 import { RunContext } from '@kbn/task-manager-plugin/server';
 import { EncryptedSavedObjectsClient } from '@kbn/encrypted-saved-objects-plugin/server';
+import { throwRetryableError } from '@kbn/task-manager-plugin/server/task_running';
 import { ActionExecutorContract } from './action_executor';
-import { ExecutorError } from './executor_error';
 import {
   ActionTaskParams,
   ActionTypeRegistryContract,
@@ -105,7 +105,6 @@ export class TaskRunnerFactory {
         const request = getFakeRequest(apiKey);
         basePathService.set(request, path);
 
-        // Throwing an executor error means we will attempt to retry the task
         // TM will treat a task as a failure if `attempts >= maxAttempts`
         // so we need to handle that here to avoid TM persisting the failed task
         const isRetryableBasedOnAttempts = taskInfo.attempts < maxAttempts;
@@ -133,9 +132,8 @@ export class TaskRunnerFactory {
             }: ${e.message}`
           );
           if (isRetryableBasedOnAttempts) {
-            // In order for retry to work, we need to indicate to task manager this task
-            // failed
-            throw new ExecutorError(e.message, {}, true);
+            // To retry, we will throw a Task Manager RetryableError
+            throw throwRetryableError(new Error(e.message), true);
           }
         }
 
@@ -152,11 +150,9 @@ export class TaskRunnerFactory {
               !!executorResult.retry ? willRetryMessage : willNotRetryMessage
             }: ${executorResult.message}`
           );
-          // Task manager error handler only kicks in when an error thrown (at this time)
-          // So what we have to do is throw when the return status is `error`.
-          throw new ExecutorError(
-            executorResult.message,
-            executorResult.data,
+          // When the return status is `error`, we will throw a Task Manager RetryableError
+          throw throwRetryableError(
+            new Error(executorResult.message),
             executorResult.retry as boolean | Date
           );
         } else if (executorResult && executorResult?.status === 'error') {

x-pack/plugins/alerting/server/alert/alert.test.ts

@@ -52,6 +52,25 @@ describe('isThrottled', () => {
     expect(alert.isThrottled({ throttle: '1m' })).toEqual(true);
   });
 
+  test(`should use actionHash if it was used in a legacy action`, () => {
+    const alert = new Alert<AlertInstanceState, AlertInstanceContext, DefaultActionGroupId>('1', {
+      meta: {
+        lastScheduledActions: {
+          date: new Date(),
+          group: 'default',
+          actions: {
+            'slack:alert:1h': { date: new Date() },
+          },
+        },
+      },
+    });
+    clock.tick(30000);
+    alert.scheduleActions('default');
+    expect(
+      alert.isThrottled({ throttle: '1m', actionHash: 'slack:alert:1h', uuid: '111-222' })
+    ).toEqual(true);
+  });
+
   test(`shouldn't throttle when group didn't change and throttle period expired`, () => {
     const alert = new Alert<AlertInstanceState, AlertInstanceContext, DefaultActionGroupId>('1', {
       meta: {
@@ -87,14 +106,14 @@ describe('isThrottled', () => {
           date: new Date(),
           group: 'default',
           actions: {
-            'slack:1h': { date: new Date() },
+            '111-111': { date: new Date() },
           },
         },
       },
     });
     clock.tick(5000);
     alert.scheduleActions('other-group');
-    expect(alert.isThrottled({ throttle: '1m', actionHash: 'slack:1h' })).toEqual(false);
+    expect(alert.isThrottled({ throttle: '1m', uuid: '111-111' })).toEqual(false);
   });
 
   test(`shouldn't throttle a specific action when group didn't change and throttle period expired`, () => {
@@ -104,14 +123,16 @@ describe('isThrottled', () => {
           date: new Date('2020-01-01'),
           group: 'default',
           actions: {
-            'slack:1h': { date: new Date() },
+            '111-111': { date: new Date() },
           },
         },
       },
     });
     clock.tick(30000);
     alert.scheduleActions('default');
-    expect(alert.isThrottled({ throttle: '15s', actionHash: 'slack:1h' })).toEqual(false);
+    expect(alert.isThrottled({ throttle: '15s', uuid: '111-111', actionHash: 'slack:1h' })).toEqual(
+      false
+    );
   });
 
   test(`shouldn't throttle a specific action when group changes`, () => {
@@ -121,14 +142,14 @@ describe('isThrottled', () => {
           date: new Date(),
           group: 'default',
           actions: {
-            'slack:1h': { date: new Date() },
+            '111-111': { date: new Date() },
           },
         },
       },
     });
     clock.tick(5000);
     alert.scheduleActions('other-group');
-    expect(alert.isThrottled({ throttle: '1m', actionHash: 'slack:1h' })).toEqual(false);
+    expect(alert.isThrottled({ throttle: '1m', uuid: '111-111' })).toEqual(false);
   });
 });
 
@@ -312,7 +333,36 @@ describe('updateLastScheduledActions()', () => {
     const alert = new Alert<AlertInstanceState, AlertInstanceContext, DefaultActionGroupId>('1', {
       meta: {},
     });
-    alert.updateLastScheduledActions('default', 'actionId1');
+    alert.updateLastScheduledActions('default', 'actionId1', '111-111');
+    expect(alert.toJSON()).toEqual({
+      state: {},
+      meta: {
+        flappingHistory: [],
+        lastScheduledActions: {
+          date: new Date().toISOString(),
+          group: 'default',
+          actions: {
+            '111-111': { date: new Date().toISOString() },
+          },
+        },
+      },
+    });
+  });
+
+  test('removes the objects with an old actionHash', () => {
+    const alert = new Alert<AlertInstanceState, AlertInstanceContext, DefaultActionGroupId>('1', {
+      meta: {
+        flappingHistory: [],
+        lastScheduledActions: {
+          date: new Date(),
+          group: 'default',
+          actions: {
+            'slack:alert:1h': { date: new Date() },
+          },
+        },
+      },
+    });
+    alert.updateLastScheduledActions('default', 'slack:alert:1h', '111-111');
     expect(alert.toJSON()).toEqual({
       state: {},
       meta: {
@@ -321,7 +371,7 @@ describe('updateLastScheduledActions()', () => {
           date: new Date().toISOString(),
           group: 'default',
           actions: {
-            actionId1: { date: new Date().toISOString() },
+            '111-111': { date: new Date().toISOString() },
           },
         },
       },