Re: [PATCH proxmox v4 03/24] notify: smtp: Introduce state management

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

On Thu, Apr 23, 2026 at 02:24:37PM +0200, Shannon Sterz wrote:
> On Tue Apr 21, 2026 at 1:59 PM CEST, Arthur Bied-Charreton wrote:
> > Export a new State struct in the xoauth2 module with associated
> > functionality for loading, updating, and persisting the OAuth2 state
> > for SMTP endpoints.
> >
> > The API for loading and saving the state is exposed through the
> > Context trait, in order to make migration as easy as possible in
> > a future where we might want to move towards KV storage instead
> > of files for secret management. It is made specific to oauth state,
> > because this implementation assumes invariants that hold for oauth2
> > refresh tokens (documented in the smtp::xoauth2 module's doc comments),
> > but are likely to be incorrect for other kinds of state that may be added
> > in the future.
> >
> > The State struct is public in order to support the long-term goal for
> > the Context trait to be implemented by the products themselves.
> >
> > The nix crate is added for the sys::stat::Mode struct, and
> > proxmox-sys is now pulled in unconditionally since it is used in the
> > Context implementations.
> >
> >
[...]
> > @@ -32,6 +34,21 @@ pub trait Context: Send + Sync + Debug {
> >          namespace: Option<&str>,
> >          source: TemplateSource,
> >      ) -> Result<Option<String>, Error>;
> > +    /// Load OAuth state for `endpoint_name`.
> > +    ///
> > +    /// The state file does not need to be locked, it is okay to just let the faster node "win"
> > +    /// as long as the invariants documented by [`smtp::xoauth2::get_microsoft_token`] and
> > +    /// [`smtp::xoauth2::get_google_token`] hold, see those functions' doc comments for details.
> > +    #[cfg(feature = "smtp")]
> > +    fn load_oauth_state(&self, endpoint_name: &str) -> Result<State, Error>;
> > +    /// Save OAuth state `state` for `endpoint_name`. Passing `None` deletes
> > +    /// the state file for `endpoint_name`.
> > +    ///
> > +    /// The state file does not need to be locked, it is okay to just let the faster node "win"
> > +    /// as long as the invariants documented by [`smtp::xoauth2::get_microsoft_token`] and
> > +    /// [`smtp::xoauth2::get_google_token`] hold, see those functions' doc comments for details.
> > +    #[cfg(feature = "smtp")]
> > +    fn save_oauth_state(&self, endpoint_name: &str, state: Option<State>) -> Result<(), Error>;
> 
> i'm not sure the no-locking approach here works. if i understand
> correctly, the following example is possible:
> 
> A: calls `trigger_state_refresh`, does not lock the notification config
> B: calls `delete_smtp_endpoint`, locks the notification config
> A: reads the refresh token
> B: removes the endpoint and state file
> A: finishes refreshing the token and safes it out
> 
> if we extend that example with a C that adds an endpoint with the same
> name after B finishes, then it would suddenly be left with a state file
> from the previous endpoint. similar examples can also happen when
> calling `build_transport` since that also includes a read & write cycle
> of the state file.
> 
Yes you are right, I may have been too focused on refresh token validity
and overlooked this - thanks a lot for catching it!

> if the state file were locked properly, B would fail to acquire the lock
> when trying to delete it (or have to wait until A finishes).
> 
> if this behaviour is fine, maybe this would benefit from some
> documentation.
> 
I will add locks in v5, I don't think we are gaining that much from a
lock-free approach here, per-endpoint locking will be pretty cheap
anyway.

[...]
> > +
> > +    /// Persist the state at `path` with `options`.
> > +    ///
> > +    /// # Errors
> > +    /// An [`Error`] is returned if serialization of the state object, or the final write, fail.
> > +    pub fn save<P: AsRef<Path>>(
> > +        self,
> > +        path: P,
> > +        options: proxmox_sys::fs::CreateOptions,
> > +    ) -> Result<(), Error> {
> > +        let path_str = path.as_ref().to_string_lossy();
> > +        let parent = path.as_ref().parent().unwrap();
> > +
> 
> imo returning an error here might be nicer. while this isn't likely to
> happen, this would panic if called with `path` set to an empty string or
> similar. if you want to not error out here, at least turn that into an
> `expect` and document the panic in the comment above.
> 
That unwrap is uncalled for yes, looking at the code again I realized
this could just be 

if let Some(parent) = path.as_ref().parent() {
    create_path(parent, ...);
}

- thanks!

> side note: thanks for the extensive documentation here in general!
> 
:)
> > +        debug!("attempting to persist state at {path_str}");
> > +
> > +        proxmox_sys::fs::create_path(parent, Some(options), Some(options))
> > +            .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))?;
> > +
> > +        let s = serde_json::to_string_pretty(&self)
> > +            .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))?;
> > +
> > +        proxmox_sys::fs::replace_file(&path, s.as_bytes(), options, false)
> > +            .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))
> > +    }
[...]