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

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

On Mon Mar 23, 2026 at 1:55 PM CET, Fabian Grünbichler wrote:

[snip]

>>>> This alters behavior as now it is never tried to load the state file if
>>>> status_in_memory.upid is None, so there is no error logged.
>>>>
>>>> So this must be expanded by an else branch where the loading is
>>>> attempted also for that case and the potential error logged.
>>> 
>>> Missed that while refactoring, sorry for the oversight. Also noticed
>>> that there is an additional change in behavior regarding the handling of
>>> any *other* error that might occur in compute_schedule_status because
>>> previously, we would use basically ignore any error and return the
>>> default status here, e.g. if the UPID could not be parsed for a started
>>> job. To match this behavior, I could just do
>>> 
>>>      let computed_schedule: JobScheduleStatus =
>>>          compute_schedule_status("garbage_collection", &store, Some(upid))
>>>              .unwrap_or_else(|_| JobScheduleStatus::default());
>>> 
>>> But the question is if the behavior here *should* differ from all other
>>> endpoints if the UPID could not be parsed? Because everywhere else we'd
>>> still return an error in that case.
>> 
>> True: this was introduced with commit fe1d34d2e ("api: garbage collect 
>> job status") and then adapted with commit 3ae21d87c ("GC: flatten 
>> existing status into job status"). So I guess this is related to the 
>> mentioned renaming.
>> 
>> Maybe Fabian can give us a clue?
>
> If there is a sensible way to handle compute_schedule_status failing
> (AFAICT, currently that only triggers if the UPID can't be parsed), we
> should do it consistently across the board - the same is true if that is
> not possible ;)

Thanks for your feedback and input!

So I would keep the behavior implemented in the v2 version of this patch
and return an error to the user if parsing the UPID fails, matching the
behavior of all other endpoints.

>
> maybe we should introduce another JobState variant that is not persisted
> ever, but used as a placeholder for partially-invalid entries? we
> already do quite a bit of mangling of other invalid data there anyway..

That's actually how I initially planned to implement it. I introduced an
`Unknown` JobState and returned that if parsing the statefile failed
(again, could also do this if parsing the UPID failed). Not quite sure
about the name, though. Open to suggestions.