diff --git a/src/openhuman/cron/seed.rs b/src/openhuman/cron/seed.rs index 852efb631..e80f6fc4d 100644 --- a/src/openhuman/cron/seed.rs +++ b/src/openhuman/cron/seed.rs @@ -1,7 +1,8 @@ //! Seed default proactive agent cron jobs. //! //! Called once after onboarding completes to create: -//! - A recurring daily morning briefing job (7 AM, user's local time or UTC) +//! - A recurring daily morning briefing job (7 AM, user's local time or UTC), +//! seeded disabled until the user opts in //! //! The morning briefing uses `mode: "proactive"` delivery so the //! channels module's @@ -73,7 +74,7 @@ pub fn seed_proactive_agents(config: &Config) -> Result<()> { prune_legacy_welcome(config, &existing); if !has(MORNING_BRIEFING_JOB_NAME) { - tracing::info!("[cron::seed] creating morning_briefing daily cron job"); + tracing::info!("[cron::seed] creating morning_briefing daily cron job (disabled — opt-in)"); seed_morning_briefing(config)?; } else { tracing::debug!("[cron::seed] morning_briefing job already exists — skipping"); @@ -164,7 +165,12 @@ fn prune_legacy_welcome(config: &Config, existing: &[crate::openhuman::cron::Cro /// (unless a timezone is later set explicitly). /// The cron expression `0 7 * * *` fires once per day. Users can later /// adjust the schedule or time zone via `cron.update_job`. +/// +/// Created disabled in a single insert. The briefing is a full proactive agent +/// turn, so it must not start billing inference until the user explicitly +/// enables it from Settings/Routines (`cron.update_job → enabled=true`). fn seed_morning_briefing(config: &Config) -> Result<()> { + tracing::debug!("[cron::seed] seed_morning_briefing start"); let schedule = Schedule::Cron { expr: "0 7 * * *".to_string(), tz: None, @@ -178,7 +184,7 @@ fn seed_morning_briefing(config: &Config) -> Result<()> { "efficient briefing they can scan in 30 seconds over coffee." ); - add_agent_job_with_definition( + let job = add_agent_job_with_definition( config, Some(MORNING_BRIEFING_JOB_NAME.to_string()), schedule, @@ -188,9 +194,14 @@ fn seed_morning_briefing(config: &Config) -> Result<()> { Some(proactive_delivery()), false, // recurring — do not delete after run Some(MORNING_BRIEFING_JOB_NAME.to_string()), - true, // enabled + false, // enabled=false — opt-in, created disabled atomically )?; + tracing::debug!( + job_id = %job.id, + enabled = job.enabled, + "[cron::seed] seed_morning_briefing done — created disabled (opt-in)" + ); Ok(()) } @@ -328,6 +339,54 @@ mod tests { ); } + #[test] + fn seeds_morning_briefing_disabled_and_idempotent() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + seed_proactive_agents(&config).expect("first seed"); + let jobs = list_jobs(&config).unwrap(); + assert!( + jobs.iter() + .filter(|j| matches!(j.job_type, crate::openhuman::cron::JobType::Agent)) + .all(|j| !j.enabled), + "fresh onboarding seed must not create enabled billable agent cron jobs: {jobs:?}" + ); + let briefings: Vec<_> = jobs + .iter() + .filter(|j| j.name.as_deref() == Some(MORNING_BRIEFING_JOB_NAME)) + .collect(); + assert_eq!( + briefings.len(), + 1, + "exactly one morning_briefing job, got {briefings:?}" + ); + let briefing = briefings[0]; + assert!( + !briefing.enabled, + "morning_briefing must be seeded disabled until explicit opt-in" + ); + assert_eq!( + briefing.agent_id.as_deref(), + Some(MORNING_BRIEFING_JOB_NAME) + ); + assert!(matches!( + briefing.schedule, + Schedule::Cron { ref expr, .. } if expr == "0 7 * * *" + )); + + seed_proactive_agents(&config).expect("second seed"); + let after = list_jobs(&config).unwrap(); + assert_eq!( + after + .iter() + .filter(|j| j.name.as_deref() == Some(MORNING_BRIEFING_JOB_NAME)) + .count(), + 1, + "second seed must not duplicate the morning_briefing job" + ); + } + #[test] fn boot_seed_is_noop_until_onboarded() { let tmp = TempDir::new().unwrap(); diff --git a/src/openhuman/cron/store.rs b/src/openhuman/cron/store.rs index 238925210..217efeea0 100644 --- a/src/openhuman/cron/store.rs +++ b/src/openhuman/cron/store.rs @@ -304,6 +304,7 @@ pub fn due_jobs(config: &Config, now: DateTime) -> Result> { pub fn update_job(config: &Config, job_id: &str, patch: CronJobPatch) -> Result { let mut job = get_job(config, job_id)?; + let was_enabled = job.enabled; let mut schedule_changed = false; if let Some(schedule) = patch.schedule { @@ -342,6 +343,24 @@ pub fn update_job(config: &Config, job_id: &str, patch: CronJobPatch) -> Result< if schedule_changed { job.next_run = next_run_for_schedule(&job.schedule, Utc::now())?; + } else if job.enabled && !was_enabled { + // Disabled→enabled transition (e.g. opting into a seeded morning + // briefing). A job that sat disabled past its originally computed + // next_run would otherwise fire immediately on opt-in, because the + // scheduler selects `enabled = 1 AND next_run <= now`. Refresh a stale + // next_run so the first run lands on the next scheduled occurrence + // rather than firing the instant the user flips the switch. + let now = Utc::now(); + if job.next_run <= now { + let refreshed = next_run_for_schedule(&job.schedule, now)?; + tracing::debug!( + job_id = %job.id, + stale_next_run = %job.next_run.to_rfc3339(), + next_run = %refreshed.to_rfc3339(), + "[cron::update_job] refreshed stale next_run on disabled→enabled transition" + ); + job.next_run = refreshed; + } } with_connection(config, |conn| { diff --git a/src/openhuman/cron/store_tests.rs b/src/openhuman/cron/store_tests.rs index 3eb413b97..3d42d13b9 100644 --- a/src/openhuman/cron/store_tests.rs +++ b/src/openhuman/cron/store_tests.rs @@ -103,6 +103,102 @@ fn due_jobs_filters_by_timestamp_and_enabled() { assert!(due_after_disable.is_empty()); } +#[test] +fn enabling_stale_disabled_job_refreshes_next_run() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + // Daily 7 AM job, then disable it (mimics a seeded opt-in morning briefing). + let job = add_job(&config, "0 7 * * *", "echo briefing").unwrap(); + update_job( + &config, + &job.id, + CronJobPatch { + enabled: Some(false), + ..CronJobPatch::default() + }, + ) + .unwrap(); + + // Force a stale next_run in the past, as if the user onboarded before the + // job's first scheduled fire and only opted in later (hours or days after). + let stale = Utc::now() - ChronoDuration::hours(2); + with_connection(&config, |conn| { + conn.execute( + "UPDATE cron_jobs SET next_run = ?1 WHERE id = ?2", + params![stale.to_rfc3339(), job.id], + )?; + Ok(()) + }) + .unwrap(); + + // Opt in: disabled -> enabled, with the schedule unchanged. + let enabled = update_job( + &config, + &job.id, + CronJobPatch { + enabled: Some(true), + ..CronJobPatch::default() + }, + ) + .unwrap(); + + assert!(enabled.enabled); + assert!( + enabled.next_run > Utc::now(), + "enabling a job with a stale next_run must refresh it to the future, got {}", + enabled.next_run + ); + assert!( + due_jobs(&config, Utc::now()).unwrap().is_empty(), + "freshly opted-in job must not fire immediately on enable" + ); +} + +#[test] +fn enabling_job_with_future_next_run_preserves_it() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + let job = add_job(&config, "0 7 * * *", "echo briefing").unwrap(); + update_job( + &config, + &job.id, + CronJobPatch { + enabled: Some(false), + ..CronJobPatch::default() + }, + ) + .unwrap(); + + // A future next_run is still valid and must be left untouched on enable. + let future = Utc::now() + ChronoDuration::hours(3); + with_connection(&config, |conn| { + conn.execute( + "UPDATE cron_jobs SET next_run = ?1 WHERE id = ?2", + params![future.to_rfc3339(), job.id], + )?; + Ok(()) + }) + .unwrap(); + + let enabled = update_job( + &config, + &job.id, + CronJobPatch { + enabled: Some(true), + ..CronJobPatch::default() + }, + ) + .unwrap(); + + assert_eq!( + enabled.next_run.to_rfc3339(), + future.to_rfc3339(), + "enabling a job whose next_run is in the future must not reschedule it" + ); +} + #[test] fn due_jobs_respects_scheduler_max_tasks_limit() { let tmp = TempDir::new().unwrap();