Re: [PATCH proxmox-backup v5 1/3] api: move statefile loading into compute_schedule_status

[Christian Ebner <c.ebner@proxmox.com>]

tiny nit, but fixing this can be a followup when applying.

On 4/13/26 3:19 PM, Michael Köppl wrote:
> Centralize loading of the job statefiles in compute_schedule_status,
> reducing code duplication across the job management API endpoints.
> 
> This also changes the error handling for UPID parsing errors with
> garbage collection state files, aligning it to the rest of the API
> handler behavior.
> 
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>   src/api2/admin/datastore.rs | 15 ++++++---------
>   src/api2/admin/prune.rs     |  9 +++------
>   src/api2/admin/sync.rs      |  9 +++------
>   src/api2/admin/verify.rs    |  9 +++------
>   src/api2/tape/backup.rs     |  9 +++------
>   src/server/jobstate.rs      |  8 ++++++--
>   6 files changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index fcb81ec5b..757b31145 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1172,19 +1172,14 @@ pub fn garbage_collection_status(
>   
>       let datastore = DataStore::lookup_datastore(lookup_with(&store, Operation::Read))?;
>       let status_in_memory = datastore.last_gc_status();
> -    let state_file = JobState::load("garbage_collection", &store)
> -        .map_err(|err| log::error!("could not open GC statefile for {store}: {err}"))
> -        .ok();
>   
>       let mut last = proxmox_time::epoch_i64();
>   
> +    let jobtype = "garbage_collection";
> +
>       if let Some(ref upid) = status_in_memory.upid {
> -        let mut computed_schedule: JobScheduleStatus = JobScheduleStatus::default();
> -        if let Some(state) = state_file {
> -            if let Ok(cs) = compute_schedule_status(&state, Some(upid)) {
> -                computed_schedule = cs;
> -            }
> -        }
> +        let computed_schedule: JobScheduleStatus =
> +            compute_schedule_status(jobtype, &store, Some(upid))?;
>   
>           if let Some(endtime) = computed_schedule.last_run_endtime {
>               last = endtime;
> @@ -1196,6 +1191,8 @@ pub fn garbage_collection_status(
>           info.next_run = computed_schedule.next_run;
>           info.last_run_endtime = computed_schedule.last_run_endtime;
>           info.last_run_state = computed_schedule.last_run_state;
> +    } else if let Err(err) = JobState::load(jobtype, &store) {
> +        log::error!("could not open statefile for {store}: {err}");
>       }
>   
>       info.next_run = info
> diff --git a/src/api2/admin/prune.rs b/src/api2/admin/prune.rs
> index a5ebf2975..1b1d2f1ba 100644
> --- a/src/api2/admin/prune.rs
> +++ b/src/api2/admin/prune.rs
> @@ -1,6 +1,6 @@
>   //! Datastore Prune Job Management
>   
> -use anyhow::{format_err, Error};
> +use anyhow::Error;
>   use serde_json::Value;
>   
>   use proxmox_router::{
> @@ -18,7 +18,7 @@ use pbs_config::CachedUserInfo;
>   
>   use crate::server::{
>       do_prune_job,
> -    jobstate::{compute_schedule_status, Job, JobState},
> +    jobstate::{compute_schedule_status, Job},
>   };
>   
>   #[api(
> @@ -73,10 +73,7 @@ pub fn list_prune_jobs(
>       let mut list = Vec::new();
>   
>       for job in job_config_iter {
> -        let last_state = JobState::load("prunejob", &job.id)
> -            .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
> -
> -        let mut status = compute_schedule_status(&last_state, Some(&job.schedule))?;
> +        let mut status = compute_schedule_status("prunejob", &job.id, Some(&job.schedule))?;
>           if job.disable {
>               status.next_run = None;
>           }
> diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
> index 6722ebea0..2384ede75 100644
> --- a/src/api2/admin/sync.rs
> +++ b/src/api2/admin/sync.rs
> @@ -1,6 +1,6 @@
>   //! Datastore Synchronization Job Management
>   
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, Error};
>   use serde::{Deserialize, Serialize};
>   use serde_json::Value;
>   
> @@ -19,7 +19,7 @@ use pbs_config::CachedUserInfo;
>   
>   use crate::{
>       api2::config::sync::{check_sync_job_modify_access, check_sync_job_read_access},
> -    server::jobstate::{compute_schedule_status, Job, JobState},
> +    server::jobstate::{compute_schedule_status, Job},
>       server::sync::do_sync_job,
>   };
>   
> @@ -112,10 +112,7 @@ pub fn list_config_sync_jobs(
>               continue;
>           }
>   
> -        let last_state = JobState::load("syncjob", &job.id)
> -            .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
> -
> -        let status = compute_schedule_status(&last_state, job.schedule.as_deref())?;
> +        let status = compute_schedule_status("syncjob", &job.id, job.schedule.as_deref())?;
>   
>           list.push(SyncJobStatus {
>               config: job,
> diff --git a/src/api2/admin/verify.rs b/src/api2/admin/verify.rs
> index 66695236c..af5b7fff4 100644
> --- a/src/api2/admin/verify.rs
> +++ b/src/api2/admin/verify.rs
> @@ -1,6 +1,6 @@
>   //! Datastore Verify Job Management
>   
> -use anyhow::{format_err, Error};
> +use anyhow::Error;
>   use serde_json::Value;
>   
>   use proxmox_router::{
> @@ -19,7 +19,7 @@ use pbs_config::CachedUserInfo;
>   
>   use crate::server::{
>       do_verification_job,
> -    jobstate::{compute_schedule_status, Job, JobState},
> +    jobstate::{compute_schedule_status, Job},
>   };
>   
>   #[api(
> @@ -73,10 +73,7 @@ pub fn list_verification_jobs(
>       let mut list = Vec::new();
>   
>       for job in job_config_iter {
> -        let last_state = JobState::load("verificationjob", &job.id)
> -            .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
> -
> -        let status = compute_schedule_status(&last_state, job.schedule.as_deref())?;
> +        let status = compute_schedule_status("verificationjob", &job.id, job.schedule.as_deref())?;
>   
>           list.push(VerificationJobStatus {
>               config: job,
> diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
> index c254c6d8b..cd68fe279 100644
> --- a/src/api2/tape/backup.rs
> +++ b/src/api2/tape/backup.rs
> @@ -1,6 +1,6 @@
>   use std::sync::{Arc, Mutex};
>   
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, Error};
>   use serde_json::Value;
>   use tracing::{info, warn};
>   
> @@ -23,7 +23,7 @@ use pbs_datastore::{DataStore, StoreProgress};
>   use crate::tape::{assert_datastore_type, TapeNotificationMode};
>   use crate::{
>       server::{
> -        jobstate::{compute_schedule_status, Job, JobState},
> +        jobstate::{compute_schedule_status, Job},
>           TapeBackupJobSummary,
>       },
>       tape::{
> @@ -97,10 +97,7 @@ pub fn list_tape_backup_jobs(
>               continue;
>           }
>   
> -        let last_state = JobState::load("tape-backup-job", &job.id)
> -            .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
> -
> -        let status = compute_schedule_status(&last_state, job.schedule.as_deref())?;
> +        let status = compute_schedule_status("tape-backup-job", &job.id, job.schedule.as_deref())?;
>   
>           let next_run = status.next_run.unwrap_or(current_time);
>   
> diff --git a/src/server/jobstate.rs b/src/server/jobstate.rs
> index dc9f6c90d..ceac8dde8 100644
> --- a/src/server/jobstate.rs
> +++ b/src/server/jobstate.rs
> @@ -301,11 +301,15 @@ impl Job {
>   }
>   
>   pub fn compute_schedule_status(
> -    job_state: &JobState,
> +    jobtype: &str,
> +    jobname: &str,

since these parameters are now passed along instead of &JobState...

>       schedule: Option<&str>,
>   ) -> Result<JobScheduleStatus, Error> {
> +    let job_state = JobState::load(jobtype, jobname)

... and this binding therefore now is of type JobState ...

> +        .map_err(|err| format_err!("could not open statefile for {jobname}: {err}"))?;
> +
>       let (upid, endtime, state, last) = match job_state {
> -        JobState::Created { time } => (None, None, None, *time),
> +        JobState::Created { time } => (None, None, None, time),
>           JobState::Started { upid } => {
>               let parsed_upid: UPID = upid.parse()?;
>               (Some(upid), None, None, parsed_upid.starttime)

... this introduces a clippy warning "useless conversion to the same 
type: `std::string::String`" on line 333:

  last_run_upid: upid.map(String::from),

since that is now no longer a &String.