Re: [PATCH datacenter-manager 3/4] api: resources: subscriptions: switch over to api_cache

["Lukas Wagner" <l.wagner@proxmox.com>]

On Fri May 15, 2026 at 11:06 AM CEST, Thomas Lamprecht wrote:
> On Wed, 13 May 2026 15:54:56 +0200, Lukas Wagner wrote:
>> diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
>> --- a/server/src/api/resources.rs
>> +++ b/server/src/api/resources.rs
>> @@ -815,66 +812,46 @@ pub async fn get_subscription_info_for_remote(
>>      remote: &Remote,
>>      max_age: u64,
>>  ) -> Result<HashMap<String, Option<NodeSubscriptionInfo>>, Error> {
>> -    if let Some(cached_subscription) = get_cached_subscription_info(&remote.id, max_age) {
>> +    if let Some(cached_subscription) =
>> +        get_cached_subscription_info(remote.id.clone(), max_age).await?
>> +    {
>>          Ok(cached_subscription.node_info)
>>      } else {
>>          let node_info = fetch_remote_subscription_info(remote).await?;
>> -        let now = proxmox_time::epoch_i64();
>> -        update_cached_subscription_info(&remote.id, &node_info, now);
>> +        update_cached_subscription_info(remote.id.clone(), node_info.clone()).await?;
>>          Ok(node_info)
>>      }
>>  }
>
> Both helpers below only borrow their `remote` parameter (they pass
> `&remote` into `api_cache::read_remote` / `write_remote`), so changing
> their parameter type from `String` to `&str` would let this call site
> stop cloning `remote.id` twice FWICT.

Argh, yeah, this was a leftover from the RFC, when I used spawn_blocking
at the callsite, which requires ownership of the values that are moved
into to the closure.

I should have reviewed the final diff more careful, then I would've
caught this myself.

>
> The old `update_cached_subscription_info` used to compare timestamps and
> skip the insert when the existing cache entry was already at least as
> new:
>
>     if let Some(cached_resource) = cache.get(remote) {
>         if cached_resource.timestamp >= now {
>             return;
>         }
>     }
>     cache.insert(...)
>
> The new code drops that check and just calls `set` unconditionally, so
> under two concurrent misses for the same remote the slower fetch result
> will overwrite the fresher one that arrived first. The fetch race itself
> existed before too, but the compare-before-insert mitigated the worst
> outcome (older data replacing newer). If you want to keep that property,
> the new function would have to `get` the existing entry under the held
> write lock and skip when its timestamp is already at least as new. See
> also the doc-comment point below.

I wonder if I should maybe fix this in the cache implementation itself,
e.g. by offering something like set_if_newer(val) and
set_if_newer_with_timestamp(val, ts)... I assume this is something that
we will need quite often.

FWIW, redis has something similar, there they have SET with the NX
parameter, which only sets if a key does not exist (or is expired).

Our key expiry works differently, by specifying a max-age on get,
instead of a TTL on set, but it roughly boils down to the same outcome
for scenarios like these.

>
> [...]
>>  /// Update cached subscription data.
>>  ///
>>  /// If the cache already contains more recent data we don't insert the passed resources.
> [...]
>> +async fn update_cached_subscription_info(
>> +    remote: String,
>> +    node_info: HashMap<String, Option<NodeSubscriptionInfo>>,
>> +) -> Result<(), Error> {
>> +    let cache = api_cache::write_remote(&remote).await?;
>>
>> +    Ok(cache
>> +        .set(
>> +            SUBSCRIPTION_STATE_CACHE_KEY,
>> +            CachedSubscriptionState {
>> +                node_info: node_info,
>
> nit: `node_info: node_info,` -> `node_info,` (clippy redundant_field_names).
>

Fixed, thanks!


>> +            },
>> +        )
>> +        .await?)
>> +}
>
> The doc comment above is the one that used to describe the
> compare-before-insert behaviour from the old code. Either drop the doc
> line (IMO not ideal), or restore the behaviour as discussed above.

Will attempt to restore the previous behavior.