Re: [PATCH datacenter-manager 4/4] remote-updates: switch over to new api_cache

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

On Wed, 13 May 2026 15:54:57 +0200, Lukas Wagner wrote:
> diff --git a/server/src/remote_updates.rs b/server/src/remote_updates.rs
> --- a/server/src/remote_updates.rs
> +++ b/server/src/remote_updates.rs
> @@ -179,10 +167,11 @@ async fn update_cached_summary_for_node(
>      node: String,
>      node_data: NodeUpdateSummary,
>  ) -> Result<(), Error> {
> -    let mut file = File::open(UPDATE_CACHE)?;
> -    let mut cache_content: UpdateSummary = serde_json::from_reader(&mut file)?;
> -    let remote_entry =
> -        cache_content
> +    let cache = api_cache::write_global().await?;
> +    let cache_content = cache.get::<UpdateSummary>(UPDATE_SUMMARY_CACHE_KEY).await?;
> +
> +    if let Some(mut entry) = cache_content {
> +        let remote_entry = entry
>              .remotes
>              .entry(remote.id)
>              .or_insert_with(|| RemoteUpdateSummary {
> @@ -191,15 +180,9 @@ async fn update_cached_summary_for_node(
>                  status: RemoteUpdateStatus::Success,
>              });
>
> -    remote_entry.nodes.insert(node, node_data);
[...]
> +        remote_entry.nodes.insert(node, node_data);
> +        cache.set(UPDATE_SUMMARY_CACHE_KEY, entry).await?;
> +    }
>
>      Ok(())
>  }

Small behaviour change worth a second look: the old code did
`File::open(UPDATE_CACHE)?`, which returned an error if the cache file
did not exist.
The new code uses `cache.get(..)`, which returns `Ok(None)` for that
case, and the `if let Some(..)` then silently skips the write.
So, if `list_available_updates` is called before any refresh has
populated the cache, its result is now thrown away silently instead of
surfaced as an error.
If you want this code path to be able to create the initial cache entry
as well, replacing the `if let Some(..)` with `let mut entry =
cache_content.unwrap_or_default();` would do it.

> @@ -212,7 +195,7 @@ pub async fn refresh_update_summary_cache(remotes: Vec<Remote>) -> Result<(), Er
>          .do_for_all_remote_nodes(remotes.clone().into_iter(), fetch_available_updates)
>          .await;
>
> -    let mut content = get_cached_summary_or_default()?;
> +    let mut content = get_cached_summary_or_default().await?;
>
>      // Clean out any remotes that might have been removed from the remote config in the meanwhile.
>      content
> @@ -275,8 +258,28 @@ pub async fn refresh_update_summary_cache(remotes: Vec<Remote>) -> Result<(), Er
>          }
>      }
>
> -    let options = proxmox_product_config::default_create_options();
> -    proxmox_sys::fs::replace_file(UPDATE_CACHE, &serde_json::to_vec(&content)?, options, true)?;
> +    cleanup_old_cachefile().await?;
> +
> +    let cache = api_cache::write_global().await?;
> +    cache.set(UPDATE_SUMMARY_CACHE_KEY, content).await?;
> +
> +    Ok(())
> +}

Two things on the final write:

 - `get_cached_summary_or_default()` above takes a read lock and drops
   it again before `write_global()` is called down here. If
   `update_cached_summary_for_node` runs in that gap, the entry it just
   wrote will be overwritten by the `cache.set` below. Holding a single
   write lock for the whole function would prevent that. Not sure if we
   strictly need that guarantee though.

 - `cleanup_old_cachefile` runs before the new `cache.set`. If the `set`
   ever fails, the old file is already gone, so the next refresh starts
   from an empty cache. Either move the cleanup after the successful
   write, or check whether the old file exists first so the cleanup only
   runs once.