fix: unscheduling a recipe should not delete them (#8978)

Signed-off-by: Angie Jones <jones.angie@gmail.com>
This commit is contained in:
Angie Jones 2026-05-03 20:26:54 -05:00 committed by GitHub
parent 45d8bf81d0
commit 17c12bfbd8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 60 additions and 14 deletions

View file

@ -180,10 +180,10 @@ pub async fn handle_schedule_remove(schedule_id: String) -> Result<()> {
.await
.context("Failed to initialize scheduler")?;
match scheduler.remove_scheduled_job(&schedule_id, true).await {
match scheduler.remove_scheduled_job(&schedule_id, false).await {
Ok(_) => {
println!(
"Scheduled job '{}' and its associated recipe removed.",
"Scheduled job '{}' removed. Associated recipe was kept.",
schedule_id
);
Ok(())

View file

@ -192,7 +192,7 @@ async fn list_schedules(
("id" = String, Path, description = "ID of the schedule to delete")
),
responses(
(status = 204, description = "Scheduled job deleted successfully"),
(status = 204, description = "Scheduled job removed successfully"),
(status = 404, description = "Scheduled job not found"),
(status = 500, description = "Internal server error")
),
@ -205,13 +205,13 @@ async fn delete_schedule(
) -> Result<StatusCode, ErrorResponse> {
let scheduler = state.scheduler();
scheduler
.remove_scheduled_job(&id, true)
.remove_scheduled_job(&id, false)
.await
.map_err(|e| match e {
goose::scheduler::SchedulerError::JobNotFound(msg) => {
ErrorResponse::not_found(format!("Schedule not found: {}", msg))
}
_ => ErrorResponse::internal(format!("Error deleting schedule: {}", e)),
_ => ErrorResponse::internal(format!("Error removing schedule: {}", e)),
})?;
Ok(StatusCode::NO_CONTENT)
}

View file

@ -281,14 +281,14 @@ impl Agent {
)
})?;
match scheduler.remove_scheduled_job(job_id, true).await {
match scheduler.remove_scheduled_job(job_id, false).await {
Ok(()) => Ok(vec![Content::text(format!(
"Successfully deleted job '{}'",
"Successfully removed schedule for job '{}'",
job_id
))]),
Err(e) => Err(ErrorData::new(
ErrorCode::INTERNAL_ERROR,
format!("Failed to delete job: {}", e),
format!("Failed to remove schedule: {}", e),
None,
)),
}

View file

@ -1148,6 +1148,49 @@ mod tests {
assert!(jobs[0].last_run.is_none(), "Paused job should not run");
}
#[tokio::test]
async fn test_remove_scheduled_job_respects_recipe_removal_flag() {
let temp_dir = tempdir().unwrap();
let storage_path = temp_dir.path().join("schedule.json");
let recipe_path = create_test_recipe(temp_dir.path(), "recipe_removal_flag_job");
let session_manager = Arc::new(SessionManager::new(temp_dir.path().to_path_buf()));
let scheduler = Scheduler::new(storage_path, session_manager).await.unwrap();
let job = ScheduledJob {
id: "recipe_removal_flag_job".to_string(),
source: recipe_path.to_string_lossy().to_string(),
cron: "0 0 0 1 1 *".to_string(),
last_run: None,
currently_running: false,
paused: false,
current_session_id: None,
process_start_time: None,
};
scheduler
.add_scheduled_job(job.clone(), false)
.await
.unwrap();
scheduler
.remove_scheduled_job("recipe_removal_flag_job", false)
.await
.unwrap();
assert!(
recipe_path.exists(),
"Recipe should be kept when remove_recipe is false"
);
scheduler.add_scheduled_job(job, false).await.unwrap();
scheduler
.remove_scheduled_job("recipe_removal_flag_job", true)
.await
.unwrap();
assert!(
!recipe_path.exists(),
"Recipe should be deleted when remove_recipe is true"
);
}
#[tokio::test]
async fn test_job_with_no_prompt_does_not_panic() {
let _guard = env_lock::lock_env([

View file

@ -2749,7 +2749,7 @@
],
"responses": {
"204": {
"description": "Scheduled job deleted successfully"
"description": "Scheduled job removed successfully"
},
"404": {
"description": "Scheduled job not found"

View file

@ -3767,7 +3767,7 @@ export type DeleteScheduleErrors = {
export type DeleteScheduleResponses = {
/**
* Scheduled job deleted successfully
* Scheduled job removed successfully
*/
204: void;
};

View file

@ -45,7 +45,10 @@ const i18n = defineMessages({
noSchedules: { id: 'schedulesView.noSchedules', defaultMessage: 'No schedules yet' },
scheduleUpdated: { id: 'schedulesView.scheduleUpdated', defaultMessage: 'Schedule Updated' },
scheduleUpdatedMsg: { id: 'schedulesView.scheduleUpdatedMsg', defaultMessage: 'Successfully updated schedule "{id}"' },
confirmDelete: { id: 'schedulesView.confirmDelete', defaultMessage: 'Are you sure you want to delete schedule "{id}"?' },
confirmDelete: {
id: 'schedulesView.confirmDelete',
defaultMessage: 'Remove schedule "{id}"? The recipe will be kept.',
},
schedulePaused: { id: 'schedulesView.schedulePaused', defaultMessage: 'Schedule Paused' },
schedulePausedMsg: { id: 'schedulesView.schedulePausedMsg', defaultMessage: 'Successfully paused schedule "{id}"' },
pauseError: { id: 'schedulesView.pauseError', defaultMessage: 'Pause Schedule Error' },
@ -326,8 +329,8 @@ const SchedulesView: React.FC<SchedulesViewProps> = ({ onClose: _onClose }) => {
trackScheduleDeleted(true);
await fetchSchedules();
} catch (error) {
console.error(`Failed to delete schedule "${id}":`, error);
const errorMsg = errorMessage(error, `Unknown error deleting "${id}".`);
console.error(`Failed to remove schedule "${id}":`, error);
const errorMsg = errorMessage(error, `Unknown error removing schedule "${id}".`);
setApiError(errorMsg);
trackScheduleDeleted(false, getErrorType(error));
} finally {

View file

@ -3534,7 +3534,7 @@
"defaultMessage": "YAML"
},
"schedulesView.confirmDelete": {
"defaultMessage": "Are you sure you want to delete schedule \"{id}\"?"
"defaultMessage": "Remove schedule \"{id}\"? The recipe will be kept."
},
"schedulesView.createSchedule": {
"defaultMessage": "Create Schedule"