Re: [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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 <a.bied-charreton@proxmox.com>
> > ---
> >  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<String>,
> > +    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::<SmtpState>(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<String, Box<dyn
> Endpoint>. 
> 
> This could probably be solved with some internal architectural changes,
> removing the Box<dyn ...> 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).