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

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

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.