From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id F00171FF13F for ; Thu, 12 Feb 2026 09:26:18 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9B4718365; Thu, 12 Feb 2026 09:27:04 +0100 (CET) Date: Thu, 12 Feb 2026 09:26:56 +0100 From: Arthur Bied-Charreton To: Lukas Wagner Subject: Re: [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State Message-ID: <2ht7zyrnl6w2ufveidy7wzdxlaqj6jgei3ao2m7rhrxioqavfk@yvochtl365lg> References: <20260204161354.458814-1-a.bied-charreton@proxmox.com> <20260204161354.458814-4-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770884731337 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.817 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: NXISNKHVTGUWICCNO5IYQJSIHETQCT5G X-Message-ID-Hash: NXISNKHVTGUWICCNO5IYQJSIHETQCT5G X-MailFrom: a.bied-charreton@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: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, Feb 10, 2026 at 04:52:14PM +0100, Lukas Wagner wrote: > Hey Arthur, > > some comments inline. > > On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote: > > This lays the groundwork for handling OAuth2 state in proxmox-notify by > > updating the crate's internal API to pass around a mutable State struct. > > > > The state can be updated by its consumers and will be persisted > > afterwards, much like the Config struct, with the difference that it is > > fully internal to the crate. External consumers of the API do not need > > to know/care about it. > > > > Signed-off-by: Arthur Bied-Charreton > > --- > > proxmox-notify/src/api/common.rs | 70 +++++++++++++++++++++--- > > proxmox-notify/src/endpoints/gotify.rs | 4 +- > > proxmox-notify/src/endpoints/sendmail.rs | 4 +- > > proxmox-notify/src/endpoints/smtp.rs | 17 +++++- > > proxmox-notify/src/endpoints/webhook.rs | 4 +- > > proxmox-notify/src/lib.rs | 42 +++++++++----- > > 6 files changed, 113 insertions(+), 28 deletions(-) > > > > diff --git a/proxmox-notify/src/api/common.rs b/proxmox-notify/src/api/common.rs > > index fa2356e2..3483be9a 100644 > > --- a/proxmox-notify/src/api/common.rs > > +++ b/proxmox-notify/src/api/common.rs > > @@ -1,7 +1,52 @@ > > use proxmox_http_error::HttpError; > > > > use super::http_err; > > -use crate::{Bus, Config, Notification}; > > +use crate::{context::context, Bus, Config, Notification, State}; > > + > > +use tracing::error; > > + > > +fn get_state() -> State { > > + let path_str = context().state_file_path(); > > + let path = std::path::Path::new(path_str); > > + > > + match path.exists() { > > + true => match State::from_path(path) { > > + Ok(s) => s, > > + Err(e) => { > > + // We don't want to block notifications on all endpoints only > > + // because the stateful ones are broken. > > + error!("could not instantiate state from {path_str}: {e}",); > > + State::default() > > + } > > + }, > > + false => State::default(), > > + } > > +} > > This is a good example of TOCTOU (time of check, time of use). First you > check if something exists in the filesystem, and if it does, you read > the file. In theory, it could happen that the file is removed in between > these two lines, leading to an error when reading the file. Now, in > this concrete case here this is not a huge issue, but nevertheless its > better to avoid it if you can. In this case here you can just read the > file and then in case of an error check for ENOENT. See [1] > for a very similar example. > > [1] https://git.proxmox.com/?p=proxmox-datacenter-manager.git;a=blob;f=server/src/remote_updates.rs;h=e772eef510218d8da41fe4a88ee47847b2598045;hb=HEAD#l159 > > In proxmox-sys there are also the file_read_optional_string or > file_get_optional_contents helpers which should do just that. > Makes sense, thanks! > > + > > +fn persist_state(state: &State) { > > + let path_str = context().state_file_path(); > > + > > + if let Err(e) = state.persist(path_str) { > > + error!("could not persist state at '{path_str}': {e}"); > > + } > > +} > > + > > `refresh_targets` sounds very generic. How about > `periodic_maintenance_task` or something alike? > See https://lore.proxmox.com/pve-devel/3kqo4fxy4y3lrkhv7exd57ap6llkds2sxrn7gqj6wfxbo5zrvc@pvacwvkdp3zi/ > > +pub fn refresh_targets(config: &Config) -> Result<(), HttpError> { > > + let bus = Bus::from_config(config).map_err(|err| { > > + http_err!( > > + INTERNAL_SERVER_ERROR, > > + "Could not instantiate notification bus: {err}" > > + ) > > + })?; > > + > > + let mut state = get_state(); > > + > > + bus.refresh_targets(&mut state); > > + > > + persist_state(&state); > > + > > + Ok(()) > > +} > > I wonder, should we attempt to persist the state in case of an error > here? Yes we should, I overlooked that, thanks for catching it > > +#[derive(Serialize, Deserialize, Clone, Debug, Default)] > > +#[serde(rename_all = "kebab-case")] > > +pub struct SmtpState { > > + #[serde(skip_serializing_if = "Option::is_none")] > > + pub oauth2_refresh_token: Option, > > + pub last_refreshed: u64, > > +} > > + > > +impl EndpointState for SmtpState {} > > + > > #[api] > > #[derive(Serialize, Deserialize, Clone, Updater, Debug)] > > #[serde(rename_all = "kebab-case")] > > @@ -158,7 +169,9 @@ pub struct SmtpEndpoint { > > } > > > > impl Endpoint for SmtpEndpoint { > > - fn send(&self, notification: &Notification) -> Result<(), Error> { > > + fn send(&self, notification: &Notification, state: &mut State) -> Result<(), Error> { > > + let mut endpoint_state = state.get_or_default::(self.name())?; > > We talked about this off-list already, but it think would be cool if an > endpoint would not get the entire state container, but only it's *own* > state. I quickly tried this using an associated type in the trait for > the state type and making `send` generic, but unfortunately this did not > really work out - we store all endpoints in a HashMap Endpoint>. > > This could probably be solved with some internal architectural changes, > removing the Box and replace it with 'manual' dispatching > using an enum wrapper and `match`. > > I am working on that, using an enum seems like the right choice here, it feels a lot better (modulo the extra dispatching/variant unwrapping boilerplate, but each concrete endpoint having to get its own state from a generic pool is not much better).