Re: [PATCH proxmox-backup v3 3/3] fix #7400: proxy: self-heal corrupted job statefiles

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

On 4/3/26 10:43 AM, Michael Köppl wrote:
> On Thu Apr 2, 2026 at 6:50 PM CEST, Christian Ebner wrote:
>> 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:
>>> Update update_job_last_run_time to transition JobState::Unknown into
>>> JobState::Created so the corrupted statefile is overwritten. In
>>> addition, update the scheduling loops to actively overwrite corrupted
>>> statefiles and return the time for the next scheduled run of the
>>> affected job.
>>>
>>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>>> ---
>>>    src/bin/proxmox-backup-proxy.rs | 4 +++-
>>>    src/server/jobstate.rs          | 2 +-
>>>    2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
>>> index c1fe3ac15..71d8566f4 100644
>>> --- a/src/bin/proxmox-backup-proxy.rs
>>> +++ b/src/bin/proxmox-backup-proxy.rs
>>> @@ -564,6 +564,7 @@ async fn schedule_datastore_garbage_collection() {
>>>                Ok(time) => time,
>>>                Err(err) => {
>>>                    eprintln!("could not get last run time of {worker_type} {store}: {err}");
>>> +                let _ = jobstate::update_job_last_run_time(worker_type, &store);
>>>                    continue;
>>>                }
>>>            };
>>> @@ -936,7 +937,8 @@ fn check_schedule(worker_type: &str, event_str: &str, id: &str) -> bool {
>>>            Ok(time) => time,
>>>            Err(err) => {
>>>                eprintln!("could not get last run time of {worker_type} {id}: {err}");
>>> -            return false;
>>> +            let _ = jobstate::update_job_last_run_time(worker_type, id);
>>> +            proxmox_time::epoch_i64() - 30
>>
>> nit: Should we define the offset from epoch here as constant? This is
>> used in several places now and having one common constant with a
>> meaningful name would help with code quality IMHO.
> 
> Thanks for a having another look at this version! I agree, that's a good
> idea. What do you think of SCHEDULE_FALLBACK_OFFSET?

Fine by me, yes!

> 
>>
>>>            }
>>>        };
>>>    
>>> diff --git a/src/server/jobstate.rs b/src/server/jobstate.rs
>>> index 4163656e8..214691965 100644
>>> --- a/src/server/jobstate.rs
>>> +++ b/src/server/jobstate.rs
>>> @@ -156,7 +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"),
>>> +        JobState::Unknown => JobState::Created { time },
>>>        };
>>>        job.write_state()
>>>    }
>