diff --git a/astrbot/core/cron/manager.py b/astrbot/core/cron/manager.py index 15071257de..b359b4755e 100644 --- a/astrbot/core/cron/manager.py +++ b/astrbot/core/cron/manager.py @@ -204,6 +204,14 @@ async def add_active_job( 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 @@ def _remove_scheduled(self, job_id: str) -> None: 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 @@ def _schedule_job(self, job: CronJob) -> None: 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, diff --git a/tests/unit/test_cron_edit_validation.py b/tests/unit/test_cron_edit_validation.py new file mode 100644 index 0000000000..5e81b91375 --- /dev/null +++ b/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 diff --git a/tests/unit/test_cron_manager.py b/tests/unit/test_cron_manager.py index 0dcb480d77..9aa37c2722 100644 --- a/tests/unit/test_cron_manager.py +++ b/tests/unit/test_cron_manager.py @@ -284,6 +284,7 @@ async def test_update_job(self, cron_manager, mock_db, sample_cron_job): 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 @@ async def test_update_job(self, cron_manager, mock_db, sample_cron_job): @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")