fix: preserve scheduled reminders when an edit is invalid (#10075)

Co-authored-by: beemines <182060364+beemines@users.noreply.github.com>

astrbot/core/cron/manager.py

@@ -204,6 +204,14 @@ class CronJobManager:
         return job
 
     async def update_job(self, job_id: str, **kwargs) -> CronJob | None:
+        current_job = await self.db.get_cron_job(job_id)
+        if not current_job:
+            return None
+        candidate = current_job.model_copy(update=kwargs)
+        if candidate.enabled:
+            # Invalid edits must not overwrite the durable job or remove its
+            # working schedule. Disabled legacy jobs can still be corrected.
+            self._build_trigger(candidate)
         job = await self.db.update_cron_job(job_id, **kwargs)
         if not job:
             return None
@@ -224,10 +232,18 @@ class CronJobManager:
         if self.scheduler.get_job(job_id):
             self.scheduler.remove_job(job_id)
 
-    def _schedule_job(self, job: CronJob) -> None:
-        if not self._started:
-            self.scheduler.start()
-            self._started = True
+    def _build_trigger(self, job: CronJob) -> CronTrigger | DateTrigger:
+        """Validate a job's timing without modifying stored or scheduled jobs.
+
+        Args:
+            job: Candidate job definition, including one-shot payload fields.
+
+        Returns:
+            A trigger using the same timezone and weekday rules as scheduling.
+
+        Raises:
+            CronJobSchedulingError: If the schedule cannot be parsed.
+        """
         try:
             tzinfo = None
             if job.timezone:
@@ -266,6 +282,17 @@ class CronJobManager:
                 trigger = CronTrigger.from_crontab(
                     normalized_cron_expression, timezone=tzinfo
                 )
+            return trigger
+        except (ValueError, TypeError) as e:
+            logger.exception("Failed to build trigger for cron job %s", job.job_id)
+            raise CronJobSchedulingError(str(e)) from e
+
+    def _schedule_job(self, job: CronJob) -> None:
+        if not self._started:
+            self.scheduler.start()
+            self._started = True
+        try:
+            trigger = self._build_trigger(job)
             self.scheduler.add_job(
                 self._run_job,
                 id=job.job_id,

tests/unit/test_cron_edit_validation.py

@@ -0,0 +1,133 @@
+"""Verify rejected cron edits preserve durable jobs and their live schedules."""
+
+from datetime import datetime, timezone
+from types import SimpleNamespace
+from unittest.mock import AsyncMock
+
+import pytest
+import pytest_asyncio
+
+from astrbot.core.cron.manager import CronJobManager, CronJobSchedulingError
+from astrbot.core.db.sqlite import SQLiteDatabase
+from astrbot.dashboard.services.cron_service import CronService, CronServiceError
+
+
+@pytest_asyncio.fixture
+async def cron_store(tmp_path):
+    """Provide a real SQLite store and scheduler without external agent calls."""
+    db = SQLiteDatabase(str(tmp_path / "cron.db"))
+    await db.initialize()
+    manager = CronJobManager(db)
+    await manager.start(SimpleNamespace())
+    manager.scheduler.pause()
+    try:
+        yield db, manager
+    finally:
+        await manager.shutdown()
+        await db.engine.dispose()
+
+
+@pytest.mark.parametrize("expression", ["not a cron", "60 9 * * *", "0 9 * * 8"])
+@pytest.mark.asyncio
+async def test_invalid_dashboard_edit_preserves_job_and_schedule(cron_store, expression):
+    """A rejected edit must not stop a previously working reminder.
+
+    Args:
+        cron_store: Real database and paused scheduler.
+        expression: Invalid field count, minute, or crontab weekday.
+    """
+    db, manager = cron_store
+    job = await manager.add_active_job(
+        name="Daily reminder",
+        cron_expression="0 9 * * 1",
+        timezone="UTC",
+        payload={"note": "Send the report", "session": "test:FriendMessage:user"},
+    )
+    scheduled = manager.scheduler.get_job(job.job_id)
+    next_run = scheduled.next_run_time
+    service = CronService(SimpleNamespace(cron_manager=manager))
+
+    with pytest.raises(CronServiceError):
+        await service.update_job(
+            job.job_id, {"name": "Rejected name", "cron_expression": expression}
+        )
+
+    stored = await db.get_cron_job(job.job_id)
+    assert stored.name == job.name
+    assert stored.cron_expression == job.cron_expression
+    assert stored.payload == job.payload
+    assert manager.scheduler.get_job(job.job_id) is scheduled
+    assert scheduled.next_run_time == next_run
+
+    # The original scheduled callback still dispatches the original payload.
+    manager._run_active_agent_job = AsyncMock()
+    await scheduled.func(*scheduled.args)
+    executed = manager._run_active_agent_job.await_args.args[0]
+    assert executed.name == job.name
+    assert executed.payload == job.payload
+
+    # Reload from SQLite, as on restart; the failed edit must not poison startup.
+    await manager.shutdown()
+    await manager.start(SimpleNamespace())
+    manager.scheduler.pause()
+    restored = manager.scheduler.get_job(job.job_id)
+    assert restored is not None
+    assert str(restored.trigger) == str(scheduled.trigger)
+
+
+@pytest.mark.asyncio
+async def test_invalid_one_shot_edit_preserves_original_deadline(cron_store):
+    """Reject malformed one-shot timestamps before mutating the saved task."""
+    db, manager = cron_store
+    deadline = datetime(2099, 1, 1, 9, tzinfo=timezone.utc)
+    job = await manager.add_active_job(
+        name="One-shot reminder",
+        cron_expression=None,
+        payload={"note": "Send the report"},
+        run_once=True,
+        run_at=deadline,
+    )
+    scheduled = manager.scheduler.get_job(job.job_id)
+    with pytest.raises(CronJobSchedulingError):
+        await manager.update_job(job.job_id, payload={"run_at": "not-a-date"})
+
+    assert (await db.get_cron_job(job.job_id)).payload == job.payload
+    assert manager.scheduler.get_job(job.job_id) is scheduled
+    assert scheduled.next_run_time == deadline
+
+
+@pytest.mark.asyncio
+async def test_invalid_enable_keeps_legacy_job_disabled(cron_store):
+    """An invalid saved task must remain disabled when activation fails."""
+    db, manager = cron_store
+    job = await db.create_cron_job(
+        name="Legacy invalid task",
+        job_type="active_agent",
+        cron_expression="invalid",
+        enabled=False,
+    )
+    with pytest.raises(CronJobSchedulingError):
+        await manager.update_job(job.job_id, enabled=True)
+
+    assert (await db.get_cron_job(job.job_id)).enabled is False
+    assert manager.scheduler.get_job(job.job_id) is None
+
+
+@pytest.mark.asyncio
+async def test_valid_edit_and_disable_remain_supported(cron_store):
+    """Valid edits take effect, and broken legacy jobs can still be disabled."""
+    db, manager = cron_store
+    job = await manager.add_active_job(
+        name="Reminder", cron_expression="0 9 * * *", payload={}, timezone="UTC"
+    )
+    updated = await manager.update_job(job.job_id, cron_expression="0 10 * * 0")
+    assert updated.cron_expression == "0 10 * * 0"
+    scheduled = manager.scheduler.get_job(job.job_id)
+    assert scheduled.next_run_time.hour == 10
+    assert scheduled.next_run_time.weekday() == 6
+
+    # A pre-existing malformed row must not prevent the user from disabling it.
+    await db.update_cron_job(job.job_id, cron_expression="invalid")
+    disabled = await manager.update_job(job.job_id, enabled=False)
+    assert disabled.enabled is False
+    assert manager.scheduler.get_job(job.job_id) is None

tests/unit/test_cron_manager.py

@@ -284,6 +284,7 @@ class TestUpdateJob:
             enabled=False,  # Disabled to avoid scheduling
         )
         mock_db.update_cron_job.return_value = updated_job
+        mock_db.get_cron_job.return_value = sample_cron_job
 
         result = await cron_manager.update_job("test-job-id", name="Updated Job")
 
@@ -293,6 +294,7 @@ class TestUpdateJob:
     @pytest.mark.asyncio
     async def test_update_job_not_found(self, cron_manager, mock_db):
         """Test updating a non-existent job."""
+        mock_db.get_cron_job.return_value = None
         mock_db.update_cron_job.return_value = None
 
         result = await cron_manager.update_job("non-existent", name="Updated")