From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 503AF1FF14C for ; Fri, 15 May 2026 11:49:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7F45E16B51; Fri, 15 May 2026 11:49:43 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 15 May 2026 11:49:07 +0200 Message-Id: Subject: Re: [PATCH datacenter-manager 3/4] api: resources: subscriptions: switch over to api_cache From: "Lukas Wagner" To: "Thomas Lamprecht" , "Lukas Wagner" X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260513135457.573414-1-l.wagner@proxmox.com> <20260513135457.573414-4-l.wagner@proxmox.com> <20260515090637.950992-3-t.lamprecht@proxmox.com> In-Reply-To: <20260515090637.950992-3-t.lamprecht@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778838541754 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: CNIB527TZ64EWHZSA7EM6PV7XG5T5SGL X-Message-ID-Hash: CNIB527TZ64EWHZSA7EM6PV7XG5T5SGL X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pdm-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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>, Error> { >> - if let Some(cached_subscription) =3D get_cached_subscription_info(&= remote.id, max_age) { >> + if let Some(cached_subscription) =3D >> + get_cached_subscription_info(remote.id.clone(), max_age).await? >> + { >> Ok(cached_subscription.node_info) >> } else { >> let node_info =3D fetch_remote_subscription_info(remote).await?= ; >> - let now =3D proxmox_time::epoch_i64(); >> - update_cached_subscription_info(&remote.id, &node_info, now); >> + update_cached_subscription_info(remote.id.clone(), node_info.cl= one()).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) =3D cache.get(remote) { > if cached_resource.timestamp >=3D 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>, >> +) -> Result<(), Error> { >> + let cache =3D 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_name= s). > 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.