diff --git a/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/controller/SopController.java b/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/controller/SopController.java index d2b0f31029b..92491e5f0c5 100644 --- a/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/controller/SopController.java +++ b/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/controller/SopController.java @@ -30,6 +30,7 @@ import org.apache.hertzbeat.ai.sop.model.SopResult; import org.apache.hertzbeat.ai.sop.registry.SkillRegistry; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.GetMapping; @@ -143,7 +144,7 @@ public ResponseEntity executeSopSync( .status("FAILED") .error("SOP skill not found: " + skillName) .build(); - return ResponseEntity.notFound().build(); + return ResponseEntity.status(HttpStatus.NOT_FOUND).body(errorResult); } Map inputParams = params != null ? params : new HashMap<>(); diff --git a/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutor.java b/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutor.java index be861e3d45e..2466b846254 100644 --- a/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutor.java +++ b/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutor.java @@ -105,9 +105,8 @@ private void executeSchedule(SopSchedule schedule) { // Check if skill exists var definition = skillRegistry.getSkill(schedule.getSopName()); if (definition == null) { - log.warn("Skill {} not found, skipping schedule {}", - schedule.getSopName(), schedule.getId()); - return; + // Do not silently skip an invalid schedule because its execution time will still be advanced. + throw new IllegalStateException("SOP skill not found: " + schedule.getSopName()); } // Parse parameters diff --git a/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/service/impl/SopScheduleServiceImpl.java b/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/service/impl/SopScheduleServiceImpl.java index 88c8d0a23bd..7e6f0f08501 100644 --- a/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/service/impl/SopScheduleServiceImpl.java +++ b/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/service/impl/SopScheduleServiceImpl.java @@ -148,10 +148,17 @@ private void validateCronExpression(String cronExpression) { private LocalDateTime calculateNextRunTime(String cronExpression) { try { CronExpression cron = CronExpression.parse(cronExpression); - return cron.next(LocalDateTime.now()); - } catch (Exception e) { - log.error("Failed to calculate next run time for cron: {}", cronExpression, e); - return null; + LocalDateTime nextRunTime = cron.next(LocalDateTime.now()); + if (nextRunTime == null) { + // Expressions such as February 31 are syntactically valid but can never be triggered. + throw new IllegalArgumentException( + "Cron expression has no future execution time: " + cronExpression); + } + return nextRunTime; + } catch (IllegalArgumentException e) { + throw e; + } catch (RuntimeException e) { + throw new IllegalArgumentException("Failed to calculate next run time: " + cronExpression, e); } } } diff --git a/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/controller/SopControllerTest.java b/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/controller/SopControllerTest.java new file mode 100644 index 00000000000..00081b64e02 --- /dev/null +++ b/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/controller/SopControllerTest.java @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.ai.controller; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.mockito.Mockito.when; + +import org.apache.hertzbeat.ai.sop.engine.SopEngine; +import org.apache.hertzbeat.ai.sop.model.SopResult; +import org.apache.hertzbeat.ai.sop.registry.SkillRegistry; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; + +/** + * Verifies that synchronous SOP failures include a diagnostic response body. + */ +@ExtendWith(MockitoExtension.class) +class SopControllerTest { + + @Mock + private SkillRegistry skillRegistry; + + @Mock + private SopEngine sopEngine; + + @InjectMocks + private SopController controller; + + @Test + void executeSopSyncShouldReturnFailureBodyWhenSkillDoesNotExist() { + when(skillRegistry.getSkill("missing")).thenReturn(null); + + ResponseEntity response = controller.executeSopSync("missing", null); + + assertEquals(HttpStatus.NOT_FOUND, response.getStatusCode()); + assertNotNull(response.getBody()); + assertEquals("FAILED", response.getBody().getStatus()); + assertEquals("SOP skill not found: missing", response.getBody().getError()); + } +} diff --git a/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutorTest.java b/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutorTest.java index e4f6bcfe3cc..77c7d188434 100644 --- a/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutorTest.java +++ b/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/schedule/SopScheduleExecutorTest.java @@ -17,6 +17,7 @@ package org.apache.hertzbeat.ai.schedule; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.Mockito.doThrow; @@ -37,6 +38,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -100,6 +102,21 @@ void checkShouldRejectInvalidScheduleParameters() { verify(scheduleService).updateAfterExecution(1L); } + @Test + void checkShouldPushErrorWhenScheduledSkillNoLongerExists() { + SopSchedule schedule = schedule(1L, null); + when(scheduleService.getDueSchedules()).thenReturn(List.of(schedule)); + when(skillRegistry.getSkill("daily_inspection")).thenReturn(null); + + executor.checkAndExecuteDueSchedules(); + + ArgumentCaptor messageCaptor = ArgumentCaptor.forClass(ChatMessage.class); + verify(chatMessageDao).save(messageCaptor.capture()); + assertTrue(messageCaptor.getValue().getContent().contains("SOP skill not found: daily_inspection")); + verifyNoInteractions(sopEngine); + verify(scheduleService).updateAfterExecution(1L); + } + private SopSchedule schedule(Long id, String params) { return SopSchedule.builder() .id(id) diff --git a/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/service/impl/SopScheduleServiceImplTest.java b/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/service/impl/SopScheduleServiceImplTest.java new file mode 100644 index 00000000000..5eb588b872f --- /dev/null +++ b/hertzbeat-ai/src/test/java/org/apache/hertzbeat/ai/service/impl/SopScheduleServiceImplTest.java @@ -0,0 +1,58 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.ai.service.impl; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.verifyNoInteractions; + +import org.apache.hertzbeat.ai.dao.SopScheduleDao; +import org.apache.hertzbeat.common.entity.ai.SopSchedule; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +/** + * Verifies that SOP schedules with no future execution time are not persisted. + */ +@ExtendWith(MockitoExtension.class) +class SopScheduleServiceImplTest { + + @Mock + private SopScheduleDao sopScheduleDao; + + @InjectMocks + private SopScheduleServiceImpl scheduleService; + + @Test + void createScheduleShouldRejectCronWithoutFutureExecutionTime() { + SopSchedule schedule = SopSchedule.builder() + .conversationId(1L) + .sopName("daily_inspection") + .cronExpression("0 0 0 31 2 *") + .build(); + + IllegalArgumentException exception = assertThrows( + IllegalArgumentException.class, () -> scheduleService.createSchedule(schedule)); + + assertTrue(exception.getMessage().contains("no future execution time")); + verifyNoInteractions(sopScheduleDao); + } +}