From d46509612f4579c6a2a3b52bb9f4d7dd399ddc04 Mon Sep 17 00:00:00 2001 From: mysma-9403 <64923976+mysma-9403@users.noreply.github.com> Date: Thu, 23 Jul 2026 15:54:25 +0200 Subject: [PATCH] fix(cron): stop one-shot At jobs from re-firing every poll (#5039) --- src/openhuman/cron/scheduler.rs | 16 ++++++++--- src/openhuman/cron/scheduler_tests.rs | 40 +++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/openhuman/cron/scheduler.rs b/src/openhuman/cron/scheduler.rs index a6ab51224..a44b3b3d9 100644 --- a/src/openhuman/cron/scheduler.rs +++ b/src/openhuman/cron/scheduler.rs @@ -1186,13 +1186,21 @@ async fn persist_job_result( duration_ms, ); - if is_one_shot_auto_delete(job) { - if success { + // A fixed-instant (`Schedule::At`) job is inherently one-shot: its `at` is in + // the past the moment it runs, so `reschedule_after_run` (which writes + // next_run = at for an `At` schedule) leaves next_run <= now and the job is + // re-selected by `due_jobs` on every poll, re-executing forever. Terminate + // every `At` job after a single run, regardless of `delete_after_run`. Only an + // auto-delete job that succeeded is removed; everything else is kept disabled + // so its run history stays inspectable. (Inside this `At` branch + // `is_one_shot_auto_delete` reduces to `job.delete_after_run`.) + if matches!(job.schedule, Schedule::At { .. }) { + if is_one_shot_auto_delete(job) && success { if let Err(e) = remove_job(config, &job.id) { tracing::warn!("Failed to remove one-shot cron job after success: {e}"); } } else { - let _ = record_last_run(config, &job.id, finished_at, false, output); + let _ = record_last_run(config, &job.id, finished_at, success, output); if let Err(e) = update_job( config, &job.id, @@ -1201,7 +1209,7 @@ async fn persist_job_result( ..CronJobPatch::default() }, ) { - tracing::warn!("Failed to disable failed one-shot cron job: {e}"); + tracing::warn!("Failed to disable one-shot cron job: {e}"); } } return success; diff --git a/src/openhuman/cron/scheduler_tests.rs b/src/openhuman/cron/scheduler_tests.rs index add63a25f..ea1cf6c82 100644 --- a/src/openhuman/cron/scheduler_tests.rs +++ b/src/openhuman/cron/scheduler_tests.rs @@ -1143,6 +1143,46 @@ async fn persist_job_result_failure_disables_one_shot() { assert_eq!(updated.last_status.as_deref(), Some("error")); } +#[tokio::test] +async fn persist_job_result_disables_at_job_without_delete_flag() { + // Regression: an `At` job created without delete_after_run (the RPC default, + // and every shell `At` job) must not be rescheduled after it runs. Its `at` + // is a fixed instant, so reschedule_after_run would write next_run = at + // (now in the past) and due_jobs would re-select it on every poll, re-firing + // the job forever. + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp).await; + let at = Utc::now() + ChronoDuration::minutes(10); + let job = cron::add_agent_job( + &config, + Some("at-no-delete".into()), + crate::openhuman::cron::Schedule::At { at }, + "Hello", + SessionTarget::Isolated, + None, + None, + false, // delete_after_run = false — the previously-buggy case + ) + .unwrap(); + let started = Utc::now(); + let finished = started + ChronoDuration::milliseconds(10); + + let success = persist_job_result(&config, &job, true, "ok", started, finished).await; + assert!(success); + + // The row is kept (not auto-deleted) but disabled, and its run is recorded. + let updated = cron::get_job(&config, &job.id).unwrap(); + assert!(!updated.enabled, "At job must be disabled after one run"); + assert_eq!(updated.last_status.as_deref(), Some("ok")); + + // It is never due again — even at a time past its `at` instant. + let due = cron::due_jobs(&config, at + ChronoDuration::minutes(1)).unwrap(); + assert!( + !due.iter().any(|j| j.id == job.id), + "disabled At job must not be re-selected by due_jobs" + ); +} + #[tokio::test] async fn deliver_if_configured_skips_non_announce_mode() { let tmp = TempDir::new().unwrap();