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

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

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?