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

["Michael Köppl" <m.koeppl@proxmox.com>]

On Thu Apr 9, 2026 at 12:09 PM CEST, Shannon Sterz wrote:
> On Fri Apr 3, 2026 at 3:26 PM CEST, 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 | 61 +++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 54 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/server/jobstate.rs b/src/server/jobstate.rs
>> index ceac8dde8..42660aa7a 100644
>> --- a/src/server/jobstate.rs
>> +++ b/src/server/jobstate.rs
>> @@ -66,6 +66,7 @@ pub enum JobState {
>>          state: TaskState,
>>          updated: Option<i64>,
>>      },
>> +    Unknown,
>
> nit, this is a public enum so this variant should have a doc string

Thanks for having a look! Yes, that's an oversight from my side. I'll
add "The job's state could not be determined (e.g. because the state
file was corrupted or does not exist)".

>
>>  }
>>
>>  /// Represents a Job and holds the correct lock
>> @@ -77,6 +78,9 @@ pub struct Job {
>>      _lock: BackupLockGuard,
>>  }
>>
>> +/// Fallback offset (in seconds) used for job schedules when the last run time is unknown
>> +pub const SCHEDULE_FALLBACK_OFFSET: i64 = 30;
>> +
>>  const JOB_STATE_BASEDIR: &str = concat!(PROXMOX_BACKUP_STATE_DIR_M!(), "/jobstates");
>>
>>  /// Create jobstate stat dir with correct permission
>> @@ -155,6 +159,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 +184,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 +197,23 @@ 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 = match serde_json::from_str(&state) {
>> +                Ok(parsed_state) => parsed_state,
>> +                Err(err) => {
>> +                    log::error!("could not parse statefile for {jobname}: {err}");
>> +                    return Ok(JobState::Unknown);
>> +                }
>
> kind of a stylistic choice, but i think the following might be slightly
> neater here:
>
> ```
> let Ok(job_sate) = serde_json::from_str(&state) else {
>     log::error!("could not parse statefile for {jobname}: {err}");
>     return Ok(JobState::Unknown);
> }
> ```

AFAIK there's no way to get the error with let-else? It's why I chose
the regular approach with match.

>
>> +            };
>> +
>> +            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,11 +229,26 @@ 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 {
>>              Ok(JobState::Created {
>> -                time: proxmox_time::epoch_i64() - 30,
>> +                time: proxmox_time::epoch_i64() - SCHEDULE_FALLBACK_OFFSET,
>>              })
>>          }
>>      }
>> @@ -263,6 +296,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 +339,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());
>> +        }
>> +    };
>
> same as above, but again only a stylistic question.
>
>>
>>      let (upid, endtime, state, last) = match job_state {
>>          JobState::Created { time } => (None, None, None, time),
>> @@ -327,6 +368,12 @@ pub fn compute_schedule_status(
>>                  last,
>>              )
>>          }
>> +        JobState::Unknown => (
>> +            None,
>> +            None,
>> +            None,
>> +            proxmox_time::epoch_i64() - SCHEDULE_FALLBACK_OFFSET,
>> +        ),
>>      };
>>
>>      let mut status = JobScheduleStatus {