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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 19, 2026 4:27 pm, Christian Ebner wrote:
> On 3/19/26 3:47 PM, Michael Köppl wrote:
>> On Thu Mar 19, 2026 at 12:24 PM CET, Christian Ebner wrote:
>>> one comment inline.
>>>
>>> On 3/19/26 12:03 PM, Michael Köppl wrote:
>>>> Centralize loading of the job statefiles in compute_schedule_status,
>>>> reducing code duplication across the job management API endpoints.
>>>>
>>>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>>>> ---
>>>>    src/api2/admin/datastore.rs | 13 +++----------
>>>>    src/api2/admin/prune.rs     |  9 +++------
>>>>    src/api2/admin/sync.rs      |  9 +++------
>>>>    src/api2/admin/verify.rs    |  9 +++------
>>>>    src/api2/tape/backup.rs     |  9 +++------
>>>>    src/server/jobstate.rs      |  8 ++++++--
>>>>    6 files changed, 21 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>>>> index cca340553..4018e0301 100644
>>>> --- a/src/api2/admin/datastore.rs
>>>> +++ b/src/api2/admin/datastore.rs
>>>> @@ -70,7 +70,7 @@ use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
>>>>    use crate::api2::backup::optional_ns_param;
>>>>    use crate::api2::node::rrd::create_value_from_rrd;
>>>>    use crate::backup::{check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK};
>>>> -use crate::server::jobstate::{compute_schedule_status, Job, JobState};
>>>> +use crate::server::jobstate::{compute_schedule_status, Job};
>>>>    use crate::tools::{backup_info_to_snapshot_list_item, get_all_snapshot_files, read_backup_index};
>>>>    
>>>>    // helper to unify common sequence of checks:
>>>> @@ -1167,19 +1167,12 @@ pub fn garbage_collection_status(
>>>>    
>>>>        let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
>>>>        let status_in_memory = datastore.last_gc_status();
>>>> -    let state_file = JobState::load("garbage_collection", &store)
>>>> -        .map_err(|err| log::error!("could not open GC statefile for {store}: {err}"))
>>>> -        .ok();
>>>>    
>>>>        let mut last = proxmox_time::epoch_i64();
>>>>    
>>>>        if let Some(ref upid) = status_in_memory.upid {
>>>> -        let mut computed_schedule: JobScheduleStatus = JobScheduleStatus::default();
>>>> -        if let Some(state) = state_file {
>>>> -            if let Ok(cs) = compute_schedule_status(&state, Some(upid)) {
>>>> -                computed_schedule = cs;
>>>> -            }
>>>> -        }
>>>> +        let computed_schedule: JobScheduleStatus =
>>>> +            compute_schedule_status("garbage_collection", &store, Some(upid))?;
>>>
>>> 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 ;)

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..