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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.

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.

[...]
>  /// 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).

> +            },
> +        )
> +        .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.