Re: [PATCH proxmox-backup v3 2/3] fix #7400: api: gracefully handle corrupted job statefiles

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

One nit inline.

Other than that:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

On 3/25/26 5:06 PM, Michael Köppl wrote:
> Introduce Unknown JobState to more explicitly represent cases where the
> state could not be determined, e.g. if the statefile was corrupted or
> missing. Update JobState::load to handle parsing errors (both for
> statefiles themselves as well as UPIDs) and return an Unknown state if
> such an error occurred. Update compute_schedule_status to also handle
> the new Unknown status, returning a default JobScheduleStatus so API
> endpoints don't return an error to the user, stopping them from viewing
> their jobs.
> 
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>   src/server/jobstate.rs | 48 ++++++++++++++++++++++++++++++++++++------
>   1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/src/server/jobstate.rs b/src/server/jobstate.rs
> index ceac8dde8..4163656e8 100644
> --- a/src/server/jobstate.rs
> +++ b/src/server/jobstate.rs
> @@ -66,6 +66,7 @@ pub enum JobState {
>           state: TaskState,
>           updated: Option<i64>,
>       },
> +    Unknown,
>   }
>   
>   /// Represents a Job and holds the correct lock
> @@ -155,6 +156,7 @@ pub fn update_job_last_run_time(jobtype: &str, jobname: &str) -> Result<(), Erro
>               state,
>               updated: Some(time),
>           },
> +        JobState::Unknown => bail!("cannot update last run time for unknown job state"),
>       };
>       job.write_state()
>   }
> @@ -179,6 +181,7 @@ pub fn last_run_time(jobtype: &str, jobname: &str) -> Result<i64, Error> {
>                   .map_err(|err| format_err!("could not parse upid from state: {err}"))?;
>               Ok(upid.starttime)
>           }
> +        JobState::Unknown => bail!("statefile could not be parsed or was empty"),
>       }
>   }
>   
> @@ -191,11 +194,20 @@ impl JobState {
>       /// This does not update the state in the file.
>       pub fn load(jobtype: &str, jobname: &str) -> Result<Self, Error> {
>           if let Some(state) = file_read_optional_string(get_path(jobtype, jobname))? {
> -            match serde_json::from_str(&state)? {
> +            let job_state = serde_json::from_str(&state).unwrap_or_else(|err| {
> +                log::error!("could not parse statefile for {jobname}: {err}");
> +                JobState::Unknown

nit: This should early return IMHO, no need to fall trough to the match 
statement below.

> +            });
> +
> +            match job_state {
>                   JobState::Started { upid } => {
> -                    let parsed: UPID = upid
> -                        .parse()
> -                        .map_err(|err| format_err!("error parsing upid: {err}"))?;
> +                    let parsed: UPID = match upid.parse() {
> +                        Ok(parsed) => parsed,
> +                        Err(err) => {
> +                            log::error!("error parsing upid for {jobname}: {err}");
> +                            return Ok(JobState::Unknown);
> +                        }
> +                    };
>   
>                       if !worker_is_active_local(&parsed) {
>                           let state = upid_read_status(&parsed).unwrap_or(TaskState::Unknown {
> @@ -211,6 +223,21 @@ impl JobState {
>                           Ok(JobState::Started { upid })
>                       }
>                   }
> +                JobState::Finished {
> +                    upid,
> +                    state,
> +                    updated,
> +                } => {
> +                    if let Err(err) = upid.parse::<UPID>() {
> +                        log::error!("error parsing upid for {jobname}: {err}");
> +                        return Ok(JobState::Unknown);
> +                    }
> +                    Ok(JobState::Finished {
> +                        upid,
> +                        state,
> +                        updated,
> +                    })
> +                }
>                   other => Ok(other),
>               }
>           } else {
> @@ -263,6 +290,7 @@ impl Job {
>               JobState::Created { .. } => bail!("cannot finish when not started"),
>               JobState::Started { upid } => upid,
>               JobState::Finished { upid, .. } => upid,
> +            JobState::Unknown => bail!("cannot finish job with unknown status"),
>           }
>           .to_string();
>   
> @@ -305,8 +333,15 @@ pub fn compute_schedule_status(
>       jobname: &str,
>       schedule: Option<&str>,
>   ) -> Result<JobScheduleStatus, Error> {
> -    let job_state = JobState::load(jobtype, jobname)
> -        .map_err(|err| format_err!("could not open statefile for {jobname}: {err}"))?;
> +    let job_state = match JobState::load(jobtype, jobname) {
> +        Ok(job_state) => job_state,
> +        Err(err) => {
> +            log::error!(
> +                "could not open statefile for {jobname}: {err} - falling back to default job schedule status",
> +            );
> +            return Ok(JobScheduleStatus::default());
> +        }
> +    };
>   
>       let (upid, endtime, state, last) = match job_state {
>           JobState::Created { time } => (None, None, None, time),
> @@ -327,6 +362,7 @@ pub fn compute_schedule_status(
>                   last,
>               )
>           }
> +        JobState::Unknown => (None, None, None, proxmox_time::epoch_i64() - 30),
>       };
>   
>       let mut status = JobScheduleStatus {