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

Christian Ebner
2026-04-02T16:36:00Z

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 {