diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx index f4edd9a7d023e..040a6b1519813 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.test.tsx @@ -126,6 +126,24 @@ describe('RecoveryConditionStep', () => { expect(screen.queryByTestId('composeDiscoverEditRecovery')).not.toBeInTheDocument(); }); + it('renders the recovery delay field when recovery type is default', () => { + renderRecoveryStep('no_breach'); + + expect(screen.getByTestId('recoveryDelayFormRow')).toBeInTheDocument(); + }); + + it('renders the recovery delay field when recovery type is custom', () => { + renderRecoveryStep('query', {}, CUSTOM_RECOVERY_QUERY); + + expect(screen.getByTestId('recoveryDelayFormRow')).toBeInTheDocument(); + }); + + it('hides the recovery delay field when recovery type is none (delay is inert)', () => { + renderRecoveryStep('none'); + + expect(screen.queryByTestId('recoveryDelayFormRow')).not.toBeInTheDocument(); + }); + it('renders query summaries and edit button in custom mode', () => { renderRecoveryStep('query', {}, CUSTOM_RECOVERY_QUERY); diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx index f1f8614b9cfad..79645716cce9b 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_discover_form/recovery_condition_step.tsx @@ -151,8 +151,12 @@ export function RecoveryConditionStep({ )} - - + {recoveryStrategy !== 'none' && ( + <> + + + + )} ); } diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.test.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.test.ts index 55f0d862f1930..d5cb73ae216af 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.test.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.test.ts @@ -170,11 +170,28 @@ describe('composeFormToCreateRequest', () => { expect(result.state_transition).toBeUndefined(); }); - it('maps state_transition for immediate delay mode', () => { + it('maps state_transition for immediate delay mode (recovery disabled omits recovering_count)', () => { const result = composeFormToCreateRequest(baseFormValues); + expect(result.state_transition).toEqual({ pending_count: 0 }); + }); + + it('emits recovering_count: 0 for immediate delay mode when recovery is enabled', () => { + const values: FormValues = { ...baseFormValues, recoveryStrategy: 'no_breach' }; + const result = composeFormToCreateRequest(values); expect(result.state_transition).toEqual({ pending_count: 0, recovering_count: 0 }); }); + it('omits recovering fields when recovery_strategy is "none" even if recovering values are set', () => { + const values: FormValues = { + ...baseFormValues, + recoveryStrategy: 'none', + stateTransitionRecoveryDelayMode: 'recoveries', + stateTransition: { recoveringCount: 3 }, + }; + const result = composeFormToCreateRequest(values); + expect(result.state_transition).toEqual({ pending_count: 0 }); + }); + it('maps state_transition for breaches delay mode', () => { const values: FormValues = { ...baseFormValues, diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts index 304a334158c08..39c603457c0d4 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/flyout/compose_discover/compose_mappers.ts @@ -16,7 +16,7 @@ import { deriveAlertDelayModeFromStateTransition, deriveRecoveryDelayModeFromStateTransition, } from '../../form/utils/state_transition_helpers'; -import { resolveRecoveryStrategy } from '../../form/utils/rule_request_mappers'; +import { isRecoveryEnabled, resolveRecoveryStrategy } from '../../form/utils/rule_request_mappers'; import type { FormValues } from '../../form/types'; const DELAY_IMMEDIATE = 'immediate'; @@ -42,15 +42,17 @@ const mapStateTransition = (formValues: FormValues) => { if (stateTransition?.pendingCount != null) out.pending_count = stateTransition.pendingCount; } - if (recoveryMode === DELAY_IMMEDIATE) { - out.recovering_count = 0; - } else if (recoveryMode !== DELAY_DURATION && stateTransition?.recoveringCount != null) { - out.recovering_count = stateTransition.recoveringCount; - } else if (recoveryMode === DELAY_DURATION) { - if (stateTransition?.recoveringTimeframe != null) - out.recovering_timeframe = stateTransition.recoveringTimeframe; - if (stateTransition?.recoveringCount != null) + if (isRecoveryEnabled(formValues)) { + if (recoveryMode === DELAY_IMMEDIATE) { + out.recovering_count = 0; + } else if (recoveryMode !== DELAY_DURATION && stateTransition?.recoveringCount != null) { out.recovering_count = stateTransition.recoveringCount; + } else if (recoveryMode === DELAY_DURATION) { + if (stateTransition?.recoveringTimeframe != null) + out.recovering_timeframe = stateTransition.recoveringTimeframe; + if (stateTransition?.recoveringCount != null) + out.recovering_count = stateTransition.recoveringCount; + } } return Object.keys(out).length ? out : undefined; diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx index 61e7ea24b6597..f3f49a181dca6 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx @@ -160,6 +160,7 @@ describe('AlertDelayField', () => { { wrapper: createFormWrapper({ kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'breaches', stateTransitionRecoveryDelayMode: 'recoveries', stateTransition: { diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_delay_field.test.tsx b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_delay_field.test.tsx index b143d3256fc47..1c1367fa7e515 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_delay_field.test.tsx +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_delay_field.test.tsx @@ -160,6 +160,7 @@ describe('RecoveryDelayField', () => { { wrapper: createFormWrapper({ kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'breaches', stateTransitionRecoveryDelayMode: 'recoveries', stateTransition: { diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts index b61fb999db9e3..ff64055ebce71 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts @@ -91,7 +91,6 @@ describe('rule_request_mappers', () => { expect(result.state_transition).toEqual({ pending_count: 3, pending_timeframe: '10m', - recovering_count: 0, }); }); @@ -106,7 +105,7 @@ describe('rule_request_mappers', () => { const result = mapFormValuesToRuleRequest(formValues); - expect(result.state_transition).toEqual({ pending_count: 5, recovering_count: 0 }); + expect(result.state_transition).toEqual({ pending_count: 5 }); expect(result.state_transition).not.toHaveProperty('pending_timeframe'); }); @@ -122,10 +121,11 @@ describe('rule_request_mappers', () => { expect(result.state_transition).toBeUndefined(); }); - it('emits pending_count: 0 and recovering_count: 0 for alert kind when both modes are immediate', () => { + it('emits pending_count: 0 and recovering_count: 0 for an alert with recovery enabled when both modes are immediate', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransition: {}, }; @@ -134,21 +134,49 @@ describe('rule_request_mappers', () => { expect(result.state_transition).toEqual({ pending_count: 0, recovering_count: 0 }); }); - it('emits pending_count: 0 and recovering_count: 0 for alert kind when stateTransition is undefined', () => { + it('omits recovering_count for an alert when recovery is disabled and both modes are immediate', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + stateTransition: {}, }; const result = mapFormValuesToRuleRequest(formValues); - expect(result.state_transition).toEqual({ pending_count: 0, recovering_count: 0 }); + expect(result.state_transition).toEqual({ pending_count: 0 }); + }); + + it('omits recovering_count for an alert when recovery is disabled and stateTransition is undefined', () => { + const formValues: FormValues = { + ...baseFormValues, + kind: 'alert', + }; + + const result = mapFormValuesToRuleRequest(formValues); + + expect(result.state_transition).toEqual({ pending_count: 0 }); + }); + + it('omits recovering fields when recovery_strategy is "none" even if recovering values are set', () => { + const formValues: FormValues = { + ...baseFormValues, + kind: 'alert', + recoveryStrategy: 'none', + stateTransitionAlertDelayMode: 'immediate', + stateTransitionRecoveryDelayMode: 'duration', + stateTransition: { recoveringCount: 3, recoveringTimeframe: '5m' }, + }; + + const result = mapFormValuesToRuleRequest(formValues); + + expect(result.state_transition).toEqual({ pending_count: 0 }); }); it('emits pending_count: 0 when alert delay mode is immediate even if pendingCount is stale', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'immediate', stateTransitionRecoveryDelayMode: 'recoveries', stateTransition: { @@ -169,6 +197,7 @@ describe('rule_request_mappers', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'immediate', stateTransitionRecoveryDelayMode: 'duration', stateTransition: { recoveringCount: 4, recoveringTimeframe: '15m' }, @@ -187,6 +216,7 @@ describe('rule_request_mappers', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'immediate', stateTransitionRecoveryDelayMode: 'recoveries', stateTransition: { recoveringCount: 3 }, @@ -202,6 +232,7 @@ describe('rule_request_mappers', () => { const formValues: FormValues = { ...baseFormValues, kind: 'alert', + recoveryStrategy: 'no_breach', stateTransitionAlertDelayMode: 'breaches', stateTransitionRecoveryDelayMode: 'duration', stateTransition: { @@ -1006,10 +1037,11 @@ describe('rule_request_mappers', () => { breach: { query: 'FROM logs-* | STATS count() BY host' }, }); expect(createPayload.grouping).toEqual({ fields: ['host.name'] }); + // baseRuleResponse has no recovery_strategy, so recovery is disabled and the + // inert recovering_count is not emitted. expect(createPayload.state_transition).toEqual({ pending_count: 3, pending_timeframe: '10m', - recovering_count: 0, }); }); }); diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts index 8e9a276e9e54d..d1ca838f81e15 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.ts @@ -50,6 +50,13 @@ export const resolveRecoveryStrategy = ( return formValues.query.recovery != null ? ('query' as const) : undefined; }; +export const isRecoveryEnabled = ( + formValues: Pick +): boolean => { + const strategy = resolveRecoveryStrategy(formValues); + return strategy != null && strategy !== 'none'; +}; + // --------------------------------------------------------------------------- // FormValues → API request // --------------------------------------------------------------------------- @@ -95,16 +102,20 @@ const mapStateTransition = (formValues: FormValues) => { } } - if (recoveryMode === DELAY_MODE.immediate) { - out.recovering_count = 0; - } else if (recoveryMode !== DELAY_MODE.duration && stateTransition?.recoveringCount != null) { - out.recovering_count = stateTransition.recoveringCount; - } else if (recoveryMode === DELAY_MODE.duration) { - if (stateTransition?.recoveringTimeframe != null) { - out.recovering_timeframe = stateTransition.recoveringTimeframe; - } - if (stateTransition?.recoveringCount != null) { + // Recovering thresholds are only meaningful when recovery is enabled; emitting them + // while recovery is disabled is inert and rejected by the write API. + if (isRecoveryEnabled(formValues)) { + if (recoveryMode === DELAY_MODE.immediate) { + out.recovering_count = 0; + } else if (recoveryMode !== DELAY_MODE.duration && stateTransition?.recoveringCount != null) { out.recovering_count = stateTransition.recoveringCount; + } else if (recoveryMode === DELAY_MODE.duration) { + if (stateTransition?.recoveringTimeframe != null) { + out.recovering_timeframe = stateTransition.recoveringTimeframe; + } + if (stateTransition?.recoveringCount != null) { + out.recovering_count = stateTransition.recoveringCount; + } } } diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts index 6af06a51f4d75..7b22b4faec399 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.test.ts @@ -9,6 +9,7 @@ import { RUNBOOK_ARTIFACT_TYPE, RUNBOOK_CONTENT_LIMIT } from '@kbn/alerting-v2-c import { createRuleDataBaseSchema, createRuleDataSchema, + isRecoveryTransitionConsistentWithStrategy, updateRuleDataSchema, IMMUTABLE_RULE_FIELDS, getBreachEsqlQuery, @@ -55,6 +56,7 @@ describe('createRuleDataSchema', () => { metadata: { name: 'test rule', owner: 'team-a', tags: ['label-1', 'label-2'] }, time_field: 'event.created', schedule: { every: '5m', lookback: '10m' }, + recovery_strategy: 'no_breach', grouping: { fields: ['host.name'] }, state_transition: { pending_operator: 'AND', @@ -72,6 +74,7 @@ describe('createRuleDataSchema', () => { metadata: { name: 'test rule', owner: 'team-a', tags: ['label-1', 'label-2'] }, time_field: 'event.created', schedule: { every: '5m', lookback: '10m' }, + recovery_strategy: 'no_breach', grouping: { fields: ['host.name'] }, state_transition: { pending_operator: 'AND', @@ -702,6 +705,7 @@ describe('createRuleDataSchema', () => { it('accepts state_transition with only recovering fields', () => { const result = createRuleDataSchema.parse({ ...validCreateData, + recovery_strategy: 'no_breach', state_transition: { recovering_operator: 'OR', recovering_count: 5, @@ -728,6 +732,7 @@ describe('createRuleDataSchema', () => { it('accepts recovering_count of 0', () => { const result = createRuleDataSchema.parse({ ...validCreateData, + recovery_strategy: 'no_breach', state_transition: { recovering_count: 0 }, }); @@ -863,6 +868,83 @@ describe('createRuleDataSchema', () => { }); }); + describe('recovery delay allowed', () => { + it('rejects a recovering_count when recovery_strategy is unset', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + state_transition: { pending_count: 0, recovering_count: 2 }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects a recovering_count when recovery_strategy is "none"', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + state_transition: { recovering_count: 2 }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects a recovering_timeframe when recovery is disabled', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + state_transition: { recovering_timeframe: '5m' }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects an inert recovery delay even when no_data_strategy is "recover"', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + no_data_strategy: 'recover', + query: { + format: 'standalone', + breach: { query: 'FROM logs-* | LIMIT 1' }, + no_data: { query: 'FROM logs-* | STATS c = COUNT(*)' }, + }, + state_transition: { recovering_count: 2 }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects recovering_count of 0 when recovery is disabled', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + state_transition: { pending_count: 0, recovering_count: 0 }, + }); + + expect(result.success).toBe(false); + }); + + it('accepts pending-only state_transition when recovery is disabled', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'none', + state_transition: { pending_count: 3 }, + }); + + expect(result.success).toBe(true); + }); + + it('accepts a recovering delay when recovery_strategy is "no_breach"', () => { + const result = createRuleDataSchema.safeParse({ + ...validCreateData, + recovery_strategy: 'no_breach', + state_transition: { recovering_count: 2, recovering_timeframe: '5m' }, + }); + + expect(result.success).toBe(true); + }); + }); + describe('artifacts envelope', () => { const parseWithArtifact = (artifact: Record) => createRuleDataSchema.safeParse({ ...validCreateData, artifacts: [artifact] }); @@ -1677,3 +1759,59 @@ describe('tagsResponseSchema', () => { expect(() => tagsResponseSchema.parse({})).toThrow(); }); }); + +describe('isRecoveryTransitionConsistentWithStrategy', () => { + it('returns true when recovery is enabled, regardless of recovering delay', () => { + expect( + isRecoveryTransitionConsistentWithStrategy({ + recovery_strategy: 'no_breach', + state_transition: { recovering_count: 3, recovering_timeframe: '5m' }, + }) + ).toBe(true); + expect( + isRecoveryTransitionConsistentWithStrategy({ + recovery_strategy: 'query', + state_transition: { recovering_count: 3 }, + }) + ).toBe(true); + }); + + it('returns true when recovery is disabled but no recovering delay is set', () => { + expect(isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: 'none' })).toBe(true); + expect(isRecoveryTransitionConsistentWithStrategy({ recovery_strategy: null })).toBe(true); + expect(isRecoveryTransitionConsistentWithStrategy({})).toBe(true); + expect( + isRecoveryTransitionConsistentWithStrategy({ + recovery_strategy: 'none', + state_transition: {}, + }) + ).toBe(true); + }); + + it('rejects recovering_count 0 when recovery is disabled (immediate recovery is not a delay)', () => { + expect( + isRecoveryTransitionConsistentWithStrategy({ + recovery_strategy: 'none', + state_transition: { recovering_count: 0 }, + }) + ).toBe(false); + }); + + it('returns false for a positive recovering delay when recovery is disabled', () => { + expect( + isRecoveryTransitionConsistentWithStrategy({ + recovery_strategy: 'none', + state_transition: { recovering_count: 1 }, + }) + ).toBe(false); + expect( + isRecoveryTransitionConsistentWithStrategy({ + recovery_strategy: null, + state_transition: { recovering_timeframe: '5m' }, + }) + ).toBe(false); + expect( + isRecoveryTransitionConsistentWithStrategy({ state_transition: { recovering_count: 2 } }) + ).toBe(false); + }); +}); diff --git a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts index 5835c3b744f27..de12a096d0617 100644 --- a/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts +++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-schemas/src/rule_data_schema.ts @@ -541,6 +541,36 @@ export const isNoDataQueryProvidedForStrategy = (data: { export const isNoDataStrategyNotEmit = (data: { no_data_strategy?: NoDataStrategy | null; }): boolean => data.no_data_strategy !== noDataStrategy.emit; + +/** + * Recovery transition thresholds are inert when recovery is disabled + * (`recovery_strategy` is `none` or unset), so we reject any `recovering_count` + * (including `0`) or `recovering_timeframe`. `recovering_count: 0` is not a + * delay — the episode recovers immediately — so it must not be configured while + * recovery is off. + */ +export const isRecoveryTransitionConsistentWithStrategy = (data: { + recovery_strategy?: RecoveryStrategy | null; + state_transition?: { + recovering_count?: number | null; + recovering_timeframe?: string | null; + } | null; +}): boolean => { + const recoveryEnabled = + data.recovery_strategy != null && data.recovery_strategy !== recoveryStrategy.none; + if (recoveryEnabled) { + return true; + } + + const stateTransition = data.state_transition; + if (stateTransition == null) { + return true; + } + + const hasRecoveringConfig = + stateTransition.recovering_count != null || stateTransition.recovering_timeframe != null; + return !hasRecoveringConfig; +}; const rejectEmitNoDataStrategy = { message: 'no_data_strategy "emit" is not currently supported.', path: ['no_data_strategy'], @@ -577,6 +607,11 @@ export const createRuleDataSchema = createRuleDataBaseSchema path: ['query', 'no_data'], }) .refine(isNoDataStrategyNotEmit, rejectEmitNoDataStrategy) + .refine(isRecoveryTransitionConsistentWithStrategy, { + message: + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', + path: ['state_transition', 'recovering_count'], + }) .meta({ id: 'alerting_new_rule' }); export type CreateRuleData = z.infer; diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts index af0ebf97d5656..e33682c4c6feb 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.test.ts @@ -706,6 +706,42 @@ describe('executeRuleOperations', () => { ); }); + it('throws when a recovering delay is set while recovery is disabled', async () => { + const ops: RuleOperation[] = [ + { operation: 'set_kind', kind: 'alert' }, + { + operation: 'set_query', + query: { format: 'standalone', breach: { query: 'FROM metrics-* | STATS COUNT(*)' } }, + }, + { operation: 'set_state_transition', pending_count: 0, recovering_count: 2 }, + ]; + + await expect(executeRuleOperations({}, ops)).rejects.toThrow( + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled' + ); + await expect(executeRuleOperations({}, ops)).rejects.toBeInstanceOf( + RuleOperationValidationError + ); + }); + + it('throws when recovering_count 0 is set while recovery is disabled', async () => { + const ops: RuleOperation[] = [ + { operation: 'set_kind', kind: 'alert' }, + { + operation: 'set_query', + query: { format: 'standalone', breach: { query: 'FROM metrics-* | STATS COUNT(*)' } }, + }, + { operation: 'set_state_transition', pending_count: 0, recovering_count: 0 }, + ]; + + await expect(executeRuleOperations({}, ops)).rejects.toThrow( + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled' + ); + await expect(executeRuleOperations({}, ops)).rejects.toBeInstanceOf( + RuleOperationValidationError + ); + }); + it('throws when signal rule uses composed query format', async () => { const ops: RuleOperation[] = [ { operation: 'set_kind', kind: 'signal' }, diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts index 00332945f7559..e4774ce8bd4f2 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/agent_builder/tools/manage_rule/operations.ts @@ -32,6 +32,7 @@ import { isStateTransitionAllowed, isSignalUsingStandaloneFormat, isSignalQueryBreachOnly, + isRecoveryTransitionConsistentWithStrategy, isRecoveryQueryConsistentWithStrategy, isRecoveryQueryProvidedForStrategy, isNoDataQueryConsistentWithStrategy, @@ -570,6 +571,12 @@ export const executeRuleOperations = async ( ); } + if (!isRecoveryTransitionConsistentWithStrategy(next)) { + throw new RuleOperationValidationError( + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).' + ); + } + return { data: next, ...(lastQueryColumns ? { queryColumns: lastQueryColumns } : {}), diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/README.md b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/README.md index 8de1943b59a82..1c25b2856256f 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/README.md +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/README.md @@ -66,6 +66,7 @@ backwards compatible. Renaming or removing a code is a breaking change. | `INVALID_STATE_TRANSITION` | 400 | `state_transition` is incompatible with the rule's `kind` | `{ rule_id, kind, transition }` | | `INVALID_SIGNAL_RULE` | 400 | An update would leave a signal rule in an invalid shape | `{ rule_id, rule_kind }` | | `INVALID_RULE_QUERY_CONFIG` | 400 | An update would desynchronize a `recovery_strategy`/`no_data_strategy` from its `query.recovery`/`query.no_data` block | `{ rule_id }` | +| `INVALID_STATE_TRANSITION_CONFIG` | 400 | A recovery delay (`state_transition.recovering_count`/`recovering_timeframe`) is set while recovery is disabled, where it is inert | `{ rule_id }` | | `BULK_QUERY_MATCH_LIMIT_EXCEEDED` | 400 | By-query bulk operation with `force: true` matched more resources than the per-request cap; rejected before any resource is mutated | `{ match_count, limit }` | | `IMMUTABLE_FIELDS_CHANGED` | 400 | PUT (upsert) request changes a field flagged as immutable | `{ fields }` | | `INVALID_FILTER_FIELD` | 400 | The `filter` references a field that is not in the allow-list | `{ field, allowed_fields }` | diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts index c4efba697dba1..86aa507776f11 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/errors/error_codes.ts @@ -35,6 +35,7 @@ export const ALERTING_ERROR_CODES = { INVALID_ARTIFACT_DATA: 'INVALID_ARTIFACT_DATA', /** `state_transition` cannot be applied to the rule's `kind`. */ INVALID_STATE_TRANSITION: 'INVALID_STATE_TRANSITION', + INVALID_STATE_TRANSITION_CONFIG: 'INVALID_STATE_TRANSITION_CONFIG', /** A signal rule's merged shape violates signal constraints. */ INVALID_SIGNAL_RULE: 'INVALID_SIGNAL_RULE', /** diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/rules_client.test.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/rules_client.test.ts index aa4cb2e9c88f1..c4b1b1981e12a 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/rules_client.test.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/rules_client.test.ts @@ -582,6 +582,36 @@ describe('RulesClient', () => { ).resolves.not.toThrow(); }); + it('throws 400 when disabling recovery leaves a stored recovering delay inert', async () => { + const client = createClient(); + + const existingAttributes: RuleSavedObjectAttributes = { + ...baseSoAttrs, + kind: 'alert', + recovery_strategy: 'no_breach', + state_transition: { recovering_count: 3 }, + }; + + rulesSavedObjectService.get.mockResolvedValueOnce({ + id: 'rule-id-inert-recovery-delay', + attributes: existingAttributes, + version: 'WzEsMV0=', + }); + + await expect( + client.updateRule({ + id: 'rule-id-inert-recovery-delay', + data: { recovery_strategy: 'none' }, + }) + ).rejects.toMatchObject({ + output: { statusCode: 400 }, + message: + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', + }); + + expect(rulesSavedObjectService.update).not.toHaveBeenCalled(); + }); + it('throws 400 when updating a signal rule query to composed format', async () => { const client = createClient(); diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts index a61648314efae..de7b37cb701c4 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.test.ts @@ -870,6 +870,62 @@ describe('utils', () => { expect(() => validateMergedRuleAttributes('rule-1', attrs)).not.toThrow(); }); + + it('throws INVALID_STATE_TRANSITION_CONFIG (400) when a recovering delay is set while recovery is disabled', () => { + const attrs = createRuleSoAttributes({ + kind: 'alert', + recovery_strategy: 'none', + state_transition: { recovering_count: 3 }, + }); + + expect(() => validateMergedRuleAttributes('rule-1', attrs)).toThrow( + expect.objectContaining({ + isBoom: true, + output: expect.objectContaining({ statusCode: 400 }), + message: + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', + data: { code: 'INVALID_STATE_TRANSITION_CONFIG', details: { rule_id: 'rule-1' } }, + }) + ); + }); + + it('throws INVALID_STATE_TRANSITION_CONFIG when a recovering_timeframe is set while recovery is unset', () => { + const attrs = createRuleSoAttributes({ + kind: 'alert', + recovery_strategy: undefined, + state_transition: { recovering_timeframe: '5m' }, + }); + + expect(() => validateMergedRuleAttributes('rule-1', attrs)).toThrow( + expect.objectContaining({ + data: { code: 'INVALID_STATE_TRANSITION_CONFIG', details: { rule_id: 'rule-1' } }, + }) + ); + }); + + it('does not throw for a recovering delay when recovery is enabled', () => { + const attrs = createRuleSoAttributes({ + kind: 'alert', + recovery_strategy: 'no_breach', + state_transition: { recovering_count: 3, recovering_timeframe: '5m' }, + }); + + expect(() => validateMergedRuleAttributes('rule-1', attrs)).not.toThrow(); + }); + + it('throws INVALID_STATE_TRANSITION_CONFIG for recovering_count 0 when recovery is disabled', () => { + const attrs = createRuleSoAttributes({ + kind: 'alert', + recovery_strategy: 'none', + state_transition: { pending_count: 0, recovering_count: 0 }, + }); + + expect(() => validateMergedRuleAttributes('rule-1', attrs)).toThrow( + expect.objectContaining({ + data: { code: 'INVALID_STATE_TRANSITION_CONFIG', details: { rule_id: 'rule-1' } }, + }) + ); + }); }); describe('pickImmutable', () => { diff --git a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts index 3f2475fb90220..c1c4f20e74c53 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/server/lib/rules_client/utils.ts @@ -12,6 +12,7 @@ import { IMMUTABLE_RULE_FIELDS, isNoDataQueryConsistentWithStrategy, isNoDataQueryProvidedForStrategy, + isRecoveryTransitionConsistentWithStrategy, isRecoveryQueryConsistentWithStrategy, isRecoveryQueryProvidedForStrategy, isSignalQueryBreachOnly, @@ -413,6 +414,13 @@ export function validateMergedRuleAttributes( code: ALERTING_ERROR_CODES.INVALID_RULE_QUERY_CONFIG, details: { rule_id: ruleId }, }, + { + valid: isRecoveryTransitionConsistentWithStrategy(attrs), + message: + 'state_transition.recovering_count and recovering_timeframe have no effect when recovery is disabled (recovery_strategy is "none" or unset).', + code: ALERTING_ERROR_CODES.INVALID_STATE_TRANSITION_CONFIG, + details: { rule_id: ruleId }, + }, ]; for (const invariant of invariants) { diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts index bf4206e920d15..dc8304d2ed9cc 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/common/builders.ts @@ -33,6 +33,10 @@ import { LOOKBACK_WINDOW, SCHEDULE_INTERVAL } from './constants'; * recovery strategies on `kind: 'signal'`. Tests that override `query` * should include `recovery_strategy: 'no_breach'` (or another valid * strategy) if they want the executor to emit recovery events. + * - When a caller disables recovery (`recovery_strategy: 'none'` or `undefined`) + * without supplying its own `state_transition`, `buildCreateRuleData` strips + * the default `recovering_count`/`recovering_timeframe`, since the write API + * rejects an inert recovering delay when recovery is off. */ const DEFAULTS: CreateRuleData = { kind: 'alert', @@ -56,10 +60,17 @@ const ACTION_POLICY_DEFAULTS: CreateActionPolicyDataInput = { export type BuildCreateRuleDataInput = Partial; -export const buildCreateRuleData = (input: BuildCreateRuleDataInput = {}): CreateRuleData => ({ - ...DEFAULTS, - ...input, -}); +export const buildCreateRuleData = (input: BuildCreateRuleDataInput = {}): CreateRuleData => { + const merged: CreateRuleData = { ...DEFAULTS, ...input }; + + const recoveryEnabled = merged.recovery_strategy != null && merged.recovery_strategy !== 'none'; + if (!recoveryEnabled && input.state_transition === undefined && merged.state_transition != null) { + const { recovering_count, recovering_timeframe, ...rest } = merged.state_transition; + merged.state_transition = rest; + } + + return merged; +}; export const buildRuleTemplateData = (rule: BuildCreateRuleDataInput = {}): RuleTemplateData => ({ engine: 'v2', diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/engine_director/api/tests/director.spec.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/engine_director/api/tests/director.spec.ts index a66bf21c817cd..6685037e25431 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/engine_director/api/tests/director.spec.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/engine_director/api/tests/director.spec.ts @@ -1513,7 +1513,7 @@ apiTest.describe('Director', { tag: tags.stateful.classic }, () => { }, recovery_strategy: 'none', no_data_strategy: 'recover', - state_transition: { pending_count: 0, recovering_count: 1 }, + state_transition: { pending_count: 0 }, }) ); @@ -1535,7 +1535,8 @@ apiTest.describe('Director', { tag: tags.stateful.classic }, () => { }); expect(noDataEvents.length).toBeGreaterThanOrEqual(1); - // The director resolves the episode directly to inactive, ignoring recovering_count. + // The director resolves the episode directly to inactive on a no_data + // event when no_data_strategy is 'recover'. for (const event of noDataEvents) { expect(event.episode?.status).toBe('inactive'); } diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/create_rule.spec.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/create_rule.spec.ts index 7ff6488ffa7b3..37fe8f86b595e 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/create_rule.spec.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/create_rule.spec.ts @@ -309,6 +309,23 @@ apiTest.describe('Create rule API', { tag: '@local-stateful-classic' }, () => { } ); + apiTest( + 'validation: rejects a recovering delay when recovery is disabled', + async ({ apiClient }) => { + const body = buildCreateRuleData({ + metadata: { name: 'invalid-inert-recovery-delay' }, + recovery_strategy: 'none', + state_transition: { pending_count: 0, recovering_count: 2 }, + }); + const response = await apiClient.post(testData.RULE_API_PATH, { + headers: writerHeaders, + body, + }); + expect(response).toHaveStatusCode(400); + expect(response.body.code).toBe('BAD_REQUEST'); + } + ); + apiTest( 'create: returns 201 with the signal kind round-tripped to the response', async ({ apiClient, apiServices }) => { diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/update_rule.spec.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/update_rule.spec.ts index 3ad706f1d4d3d..6976287bf52b0 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/update_rule.spec.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/update_rule.spec.ts @@ -545,6 +545,30 @@ apiTest.describe('Update rule API', { tag: '@local-stateful-classic' }, () => { } ); + apiTest( + 'validation: should reject disabling recovery that would leave a stored recovering delay inert', + async ({ apiClient, apiServices }) => { + const created = await apiServices.alertingV2.rules.create( + buildCreateRuleData({ + metadata: { name: 'rule-inert-recovery-delay-on-update' }, + recovery_strategy: 'no_breach', + state_transition: { pending_count: 0, recovering_count: 3 }, + }) + ); + + const response = await apiClient.patch(getRuleUrl(created.id), { + headers: writerHeaders, + body: { recovery_strategy: 'none' }, + }); + + expect(response).toHaveStatusCode(400); + expect(response.body.code).toBe('INVALID_STATE_TRANSITION_CONFIG'); + + const stored = await apiServices.alertingV2.rules.get(created.id); + expect(stored.recovery_strategy).toBe('no_breach'); + } + ); + const buildSignalRuleData = (name: string) => buildCreateRuleData({ kind: 'signal', diff --git a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/upsert_rule.spec.ts b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/upsert_rule.spec.ts index a80896f10e604..2fa8bb8ce4963 100644 --- a/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/upsert_rule.spec.ts +++ b/x-pack/platform/plugins/shared/alerting_v2/test/scout_alerting_v2/rules/api/tests/upsert_rule.spec.ts @@ -287,6 +287,22 @@ apiTest.describe('Upsert rule API', { tag: '@local-stateful-classic' }, () => { expect(response.body.code).toBe('BAD_REQUEST'); }); + apiTest( + 'validation: should reject a recovering delay when recovery is disabled', + async ({ apiClient }) => { + const response = await apiClient.put(getRuleUrl('any-id'), { + headers: writerHeaders, + body: buildCreateRuleData({ + metadata: { name: 'upsert-inert-recovery-delay' }, + recovery_strategy: 'none', + state_transition: { pending_count: 0, recovering_count: 2 }, + }), + }); + expect(response).toHaveStatusCode(400); + expect(response.body.code).toBe('BAD_REQUEST'); + } + ); + apiTest( 'authorization: should return 201 for a user with full alerting_v2 privileges', async ({ apiClient }) => {