Scheduler cleanup #5571

DOsinga · 2025-11-04T23:58:33Z

Cleaning up the scheduler and verify that it actually works

DOsinga · 2025-11-04T23:59:09Z

fixes #5491 (hopefully)

Copilot

Pull Request Overview

This PR refactors the scheduler module to improve code maintainability and reduce duplication. The changes extract duplicated cron task creation logic into a reusable function, simplify error handling, reduce lock contention, and streamline the test suite.

Extracts cron task creation into a reusable create_cron_task() method
Simplifies the persist_jobs() function by removing the guard-specific variant
Replaces complex test mock provider setup with a simple cfg-gated execute_job() function
Reduces lock holding duration by restructuring critical sections

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-05T00:01:02Z

crates/goose/src/scheduler.rs

+            let mut jobs_guard = self.jobs.lock().await;
+            jobs_guard.insert(job_to_load.id.clone(), (job_uuid, job_to_load));


Lock is acquired inside the loop for each job during load. This could cause excessive lock contention if many jobs are being loaded. Consider collecting all job insertions and performing them with a single lock acquisition after the loop completes.

Copilot · 2025-11-05T00:01:03Z

crates/goose/src/scheduler.rs

+        if !original_recipe_path.is_file() {
+            return Err(SchedulerError::RecipeLoadError(format!(
+                "Recipe file not found: {}",


The error message 'Recipe file not found' is misleading when is_file() returns false. This could mean the path exists but is a directory, or doesn't exist at all. Consider checking exists() first and providing distinct error messages: 'Recipe path does not exist' vs 'Recipe path is not a file'.

Suggested change

if !original_recipe_path.is_file() {

return Err(SchedulerError::RecipeLoadError(format!(

"Recipe file not found: {}",

if !original_recipe_path.exists() {

return Err(SchedulerError::RecipeLoadError(format!(

"Recipe path does not exist: {}",

original_job_spec.source

)));

}

if !original_recipe_path.is_file() {

return Err(SchedulerError::RecipeLoadError(format!(

"Recipe path is not a file: {}",

Copilot · 2025-11-05T00:01:03Z

crates/goose/src/scheduler.rs

-    let jobs_guard = jobs_arc.lock().await;
+    let jobs_guard = jobs.lock().await;
    let list: Vec<ScheduledJob> = jobs_guard.values().map(|(_, j)| j.clone()).collect();
+    drop(jobs_guard);


Explicit drop(jobs_guard) is unnecessary here as the guard will be automatically dropped at the end of the block on line 159. The explicit drop doesn't provide any benefit and can be removed for cleaner code.

Suggested change

drop(jobs_guard);

Copilot

Pull Request Overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-06T03:04:00Z

ui/desktop/src/schedule.ts

-    console.error(`Error fetching sessions for schedule ${scheduleId}:`, error);
-    throw error;
-  }
+  limit: number


Making limit a required parameter without a default value forces all callers to provide it explicitly. Consider providing a default value (e.g., limit: number = 50) to maintain backward compatibility and provide a sensible fallback.

Suggested change

limit: number

limit: number = 50

Copilot · 2025-11-06T03:04:01Z

ui/desktop/src/components/schedule/ScheduleModal.tsx

+            const cleanId = recipe.title
+              .toLowerCase()
+              .replace(/[^a-z0-9-]/g, '-')
+              .replace(/-+/g, '-');


The auto-generated schedule ID could have leading/trailing hyphens (e.g., if title is '---Test---'). Consider trimming hyphens with .replace(/^-+|-+$/g, '') after the replacements.

Suggested change

.replace(/-+/g, '-');

.replace(/-+/g, '-')

.replace(/^-+|-+$/g, '');

Copilot · 2025-11-06T03:04:01Z

crates/goose/src/scheduler.rs

+            5 => {
+                tracing::warn!(
+                    "Job '{}' has legacy 5-field cron '{}', converting to 6-field",
+                    job.id,
+                    job.cron
+                );
+                format!("0 {}", job.cron)
+            }


The warning about legacy 5-field cron format will be logged every time the task is created (including at startup for all jobs). Consider logging this only once per job, perhaps during add_scheduled_job or storing a flag to avoid repeated warnings.

yeah, I don't think this needs a warning at all

I'm leaving it in as this was a cause of errors. it should all be rational now

Copilot · 2025-11-06T03:04:01Z

crates/goose/src/scheduler.rs

+    if job.source.is_empty() {
+        // Shortcut for testing
+        return Ok(job.id.to_string());
+    }


This testing shortcut in production code creates a hidden code path that bypasses validation. Consider using conditional compilation (#[cfg(test)]) or a more explicit testing mechanism to avoid potential confusion or misuse in production.

Suggested change

if job.source.is_empty() {

// Shortcut for testing

return Ok(job.id.to_string());

}

#[cfg(test)]

{

if job.source.is_empty() {

// Shortcut for testing

return Ok(job.id.to_string());

}

}

#[cfg(not(test))]

{

if job.source.is_empty() {

return Err(JobExecutionError {

job_id: job.id.clone(),

error: "Job source is empty".to_string(),

});

}

}

is this actually here for testing? it does seem weird to just do nothing if the job.source is empty

hmm, yeah

so I wanted to have a test that tests the actual scheduler. but I don't want to do anything. I experimented with various test only code, but that got my warnings of not used code, so if you have a better suggestion

Copilot

Pull Request Overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jamadeo

nice

the nits mostly refer to old code, feel free to ignore them

jamadeo · 2025-11-07T18:26:49Z

crates/goose/src/scheduler.rs

+            5 => {
+                tracing::warn!(
+                    "Job '{}' has legacy 5-field cron '{}', converting to 6-field",
+                    job.id,
+                    job.cron
+                );
+                format!("0 {}", job.cron)
+            }


yeah, I don't think this needs a warning at all

jamadeo · 2025-11-07T18:30:28Z

crates/goose/src/scheduler.rs

-            )));
-        }
+    fn create_cron_task(&self, job: ScheduledJob) -> Result<Job, SchedulerError> {
+        let job_for_task = job.clone();


I don't think this needs to own job

create_cron_task(&self, job: &ScheduledJob)

jamadeo · 2025-11-07T18:38:28Z

crates/goose/src/scheduler.rs

+        fs::copy(original_recipe_path, &destination_recipe_path)?;
+
+        let mut stored_job = original_job_spec;
+        stored_job.source = destination_recipe_path.to_string_lossy().into_owned();


seems like ScheduledJob.source should be a PathBuf not a String

jamadeo · 2025-11-07T18:44:49Z

crates/goose/src/scheduler.rs

+            Ok(jobs) => jobs,
+            Err(e) => {
+                tracing::error!(
+                    "Failed to parse schedules.json: {}. Starting with empty schedule list.",


I guess this should say the actual path

jamadeo · 2025-11-07T18:50:31Z

crates/goose/src/scheduler.rs

        }
+
+        persist_jobs(&self.storage_path, &self.jobs).await?;
+        Ok(())


so we have to remove it from three or four places but each might fail, do we want to not abort on the first fail?

jamadeo · 2025-11-07T19:00:33Z

crates/goose/src/scheduler.rs

 }

-async fn persist_jobs_from_arc(
+async fn persist_jobs(


I was going to say why isn't this a method on the scheduler, but I see it's because we are calling it in the job tasks where we only give it a pointer to the internal map

I probably would have one this where the job tasks write to a channel and the scheduler just has one task that saves whenever updates come through, that feels a bit safer and cleaner, but this is already a big refactor. Maybe for another time.

jamadeo · 2025-11-07T19:07:27Z

crates/goose/src/scheduler.rs

+    if job.source.is_empty() {
+        // Shortcut for testing
+        return Ok(job.id.to_string());
+    }


is this actually here for testing? it does seem weird to just do nothing if the job.source is empty

jamadeo · 2025-11-07T19:08:26Z

crates/goose/src/scheduler.rs

-                    extension, job.source
-                ),
-            }),
+            "json" | "jsonl" => serde_json::from_str(&recipe_content)?,


can recipes be .jsonl files? also do we need this check, can we not always call serde_yaml?

jamadeo · 2025-11-07T19:10:30Z

crates/goose/src/scheduler.rs

+        tracing::error!("Failed to update session: {}", e);
    }

-    tracing::info!("Finished job: {}", job.id);


I know we're all about reducing the logging but it does seem like logging the start/stop of each job execution would not be a bad idea.

Or is it that we log it outside of this function?

DOsinga · 2025-11-08T16:41:10Z

thanks @jamadeo - what are you saying is all true. but I want to get this in, so I am mostly going to leave it for another time. if that :)

Copilot

Pull Request Overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-08T18:05:45Z

ui/desktop/src/schedule.ts

+    query: { limit },
+    throwOnError: true,
+  });
+


The function removes all error handling that was previously present. Since throwOnError: true is set, the API will throw on errors, but the function should handle potential cases where response.data is undefined. Consider adding a check: if (!response.data) throw new Error('No data returned'); before the return statement.

Suggested change

if (!response.data) {

throw new Error('No data returned from apiGetScheduleSessions');

}

Copilot · 2025-11-08T18:05:46Z

ui/desktop/src/components/schedule/ScheduleModal.tsx

+            const cleanId = recipe.title
+              .toLowerCase()
+              .replace(/[^a-z0-9-]/g, '-')
+              .replace(/-+/g, '-');


The generated schedule ID doesn't trim leading/trailing hyphens that could result from the replacements. For example, a title like '!Test Recipe!' would become '-test-recipe-'. Consider adding .replace(/^-+|-+$/g, '') to trim hyphens from both ends.

Suggested change

.replace(/-+/g, '-');

.replace(/-+/g, '-')

.replace(/^-+|-+$/g, '');

Copilot · 2025-11-08T18:05:46Z

crates/goose/src/scheduler.rs

+        let cron_parts: Vec<&str> = job.cron.split_whitespace().collect();
+        let cron = match cron_parts.len() {
+            5 => {
+                tracing::warn!(
+                    "Job '{}' has legacy 5-field cron '{}', converting to 6-field",
+                    job.id,
+                    job.cron
+                );
+                format!("0 {}", job.cron)
+            }
+            6 => job.cron.clone(),
+            _ => {
+                return Err(SchedulerError::CronParseError(format!(
+                    "Invalid cron expression '{}': expected 5 or 6 fields, got {}",
+                    job.cron,
+                    cron_parts.len()
+                )))
+            }
+        };


The cron validation logic is duplicated in both create_cron_task and the removed normalize_cron_expression function. Consider extracting this into a helper function validate_and_normalize_cron(cron: &str) -> Result<String, SchedulerError> to avoid duplication and improve maintainability.

Copilot · 2025-11-08T18:05:46Z

crates/goose/src/scheduler.rs

+    async fn load_jobs_from_storage(self: &Arc<Self>) {
        if !self.storage_path.exists() {
-            return Ok(());
+            return;
        }
-        let data = fs::read_to_string(&self.storage_path)?;
+        let data = match fs::read_to_string(&self.storage_path) {
+            Ok(data) => data,
+            Err(e) => {
+                tracing::error!(
+                    "Failed to read schedules.json: {}. Starting with empty schedule list.",
+                    e
+                );
+                return;
+            }
+        };


The function signature changed from returning Result<(), SchedulerError> to returning nothing (unit), but errors are now silently logged and ignored. The caller in new() at line 155 doesn't check for errors anymore. Consider whether critical initialization errors should prevent scheduler creation or if silent failure is acceptable for your use case.

* main: (33 commits) Fix Claude Code provider to default to Auto mode (#5638) (#5642) Scheduler cleanup (#5571) Better search paths and handling of CLI providers (#5554) docs: description required for "Add Extension" in cli - phase 2 (#5635) Remove some logging (#5631) Use session IDs as task IDs for subagents instead of UUIDs (#5398) Fix the naming (#5628) fix: default tetrate model is broken, replace with haiku-4.5 (#5535) (#5587) Fetch less and use the right SHA (#5621) feat(ui): add custom macOS dock menu with New Window option (#5099) feat: remove hints from recipe prompts (#5622) docs: October 2025 Community All-Stars spotlight, Hacktoberfest edition (#5625) differentiate debug/release in cache key (#5613) Unify subrecipe and subagent execution through shared recipe pipeline (#5082) Standardize CLI argument flags and update documentation (#5516) Release 1.13.0 fix: move goosehints/AGENTS.md handling to goose, and out of developer extension (#5575) fix: add standard context menu items to prevent empty right-click menu (#5616) Bump openapi in prepare-release (#5611) docs: add access control section to Developer tutorial (#5615) ...

* main: (83 commits) silence copilot on minor text issues (block#5665) fix: disallow runaway subagent chains (block#5659) chore: remove usage of non-existent env var for log dir (block#5658) clarify agent instructions (block#5655) feat: add check-everything for unified style checks (block#5650) Show errors on failure (block#5643) custom instructions for copilot reviews (block#5646) fix: prevent repeated 404 errors when accessing deleted sessions (block#5644) Flake.nix corrected main (block#5600) fix: goose recipe list can return duplicated entries (block#5645) fix: bedrock creds refresh (block#5599) Fix Claude Code provider to default to Auto mode (block#5638) (block#5642) Scheduler cleanup (block#5571) Better search paths and handling of CLI providers (block#5554) docs: description required for "Add Extension" in cli - phase 2 (block#5635) Remove some logging (block#5631) Use session IDs as task IDs for subagents instead of UUIDs (block#5398) Fix the naming (block#5628) fix: default tetrate model is broken, replace with haiku-4.5 (block#5535) (block#5587) Fetch less and use the right SHA (block#5621) ...

Co-authored-by: Douwe Osinga <[email protected]>

Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: Blair Allan <[email protected]>

Scheduler cleanup

6025946

Copilot AI review requested due to automatic review settings November 4, 2025 23:58

Copilot AI reviewed Nov 5, 2025

View reviewed changes

Douwe Osinga added 9 commits November 4, 2025 19:09

Check point

35330a6

Add some debugging

072dc30

remove

822b542

Remove more

07a26ec

Hmm

d8ef826

Hmm

a90ac46

Remove

1cc2fca

More clean up

8ba9cc5

Who did this

2e6bee1

Copilot AI review requested due to automatic review settings November 6, 2025 03:01

Copilot AI reviewed Nov 6, 2025

View reviewed changes

More unused

6117275

DOsinga requested review from Kvadratni and jamadeo November 6, 2025 03:14

Even more cleanup

3b67bb0

Copilot AI review requested due to automatic review settings November 6, 2025 13:26

Copilot AI reviewed Nov 6, 2025

View reviewed changes

tlongwell-block mentioned this pull request Nov 7, 2025

Scheduler recipe execution unification #5634

Closed

jamadeo approved these changes Nov 7, 2025

View reviewed changes

Douwe Osinga added 2 commits November 8, 2025 12:09

Merge branch 'main' into scheduler-cleanup

36fcdc4

Idk

5e653ad

Copilot AI review requested due to automatic review settings November 8, 2025 18:03

Copilot AI reviewed Nov 8, 2025

View reviewed changes

DOsinga merged commit e5a1474 into main Nov 8, 2025
22 checks passed

DOsinga mentioned this pull request Nov 9, 2025

Scheduler does not run #5491

Open

github-actions bot mentioned this pull request Nov 11, 2025

chore(release): release version 1.14.0 (minor) #5676

Merged

Surendhar-N-D pushed a commit to Surendhar-N-D/goose that referenced this pull request Nov 17, 2025

Scheduler cleanup (block#5571)

680d961

Co-authored-by: Douwe Osinga <[email protected]>

arul-cc pushed a commit to arul-cc/goose that referenced this pull request Nov 17, 2025

Scheduler cleanup (block#5571)

830e7f7

Co-authored-by: Douwe Osinga <[email protected]>

BlairAllan pushed a commit to BlairAllan/goose that referenced this pull request Nov 29, 2025

Scheduler cleanup (block#5571)

7f61ecf

Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: Blair Allan <[email protected]>

tlongwell-block mentioned this pull request Dec 14, 2025

refactor: move scheduler to shared execution path #6108

Closed

		let mut jobs_guard = self.jobs.lock().await;
		jobs_guard.insert(job_to_load.id.clone(), (job_uuid, job_to_load));

	.replace(/-+/g, '-');
	.replace(/-+/g, '-')
	.replace(/^-+\|-+$/g, '');

+  if (!response.data) {
+    throw new Error('No data returned from apiGetScheduleSessions');
+  }

Scheduler cleanup #5571

Scheduler cleanup #5571

Uh oh!

Conversation

DOsinga commented Nov 4, 2025

Uh oh!

DOsinga commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

jamadeo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DOsinga commented Nov 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 8, 2025