Re: [PATCH proxmox 7/7] notify (smtp): Add logging and state-related error types

["Lukas Wagner" <l.wagner@proxmox.com>]

On Fri Feb 13, 2026 at 5:04 PM CET, Arthur Bied-Charreton wrote:
> Log load/store events in SMTP state management, and add error types to make errors clearer (previously used Config(De)?Serialization errors, which led to confusing error messages for state-related errors)

Commit message should be properly wrapped :)

Also, I think this commit should be split, the error-part and the
logging part rather seem to be separate things. Both could also be
probably folded into previous commits (e.g. add the persistence errors
when you introduce the state handling), but no hard feelings about that,
I'm also fine with having this in a separate commit.

>
> Also add logs for OAuth2 token exchange events.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  proxmox-notify/src/endpoints/smtp.rs       | 12 +++++++++++
>  proxmox-notify/src/endpoints/smtp/state.rs | 25 +++++++++++++++-------
>  proxmox-notify/src/lib.rs                  | 12 +++++++++++
>  3 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
> index 4364bd11..0ae1ac9f 100644
> --- a/proxmox-notify/src/endpoints/smtp.rs
> +++ b/proxmox-notify/src/endpoints/smtp.rs
> @@ -12,6 +12,7 @@ use oauth2::{ClientId, ClientSecret, RefreshToken};
>  
>  use proxmox_schema::api_types::COMMENT_SCHEMA;
>  use proxmox_schema::{api, Updater};
> +use tracing::info;

nit: usually we try to group imports like:

 - std
 - 3rd party crates
 - our on crates
 - crate-level imports

>  
>  use crate::context::context;
>  use crate::endpoints::common::mail;
> @@ -301,6 +302,12 @@ impl SmtpEndpoint {
>                              self.name().into(),
>                              Box::new(Error::Generic("no refresh token found".into())),
>                          ))?;
> +
> +                info!(
> +                    "requesting OAuth2 access token for endpoint '{}'",
> +                    self.config.name
> +                );
> +

Seeing this in practise, I think I'd rather use debug! here... Could be
considered a bit noisy otherwise, as this line would appear e.g. in task
logs for backup jobs.

>                  let token_exchange_result = self.get_access_token(&refresh_token, method)?;
>  
>                  state
> @@ -313,6 +320,11 @@ impl SmtpEndpoint {
>                      .set_last_refreshed(proxmox_time::epoch_i64())
>                      .store(self.name())?;
>  
> +                info!(
> +                    "OAuth2 token exchange successful for endpoint '{}'",
> +                    self.config.name
> +                );
> +

Same here

>                  transport_builder
>                      .credentials(Credentials::new(
>                          self.config.from_address.to_owned(),
> diff --git a/proxmox-notify/src/endpoints/smtp/state.rs b/proxmox-notify/src/endpoints/smtp/state.rs
> index 60bef590..45d0db93 100644
> --- a/proxmox-notify/src/endpoints/smtp/state.rs
> +++ b/proxmox-notify/src/endpoints/smtp/state.rs
> @@ -1,4 +1,5 @@
>  use serde::{Deserialize, Serialize};
> +use tracing::debug;
>  
>  use crate::{context::context, Error};
>  
> @@ -25,13 +26,19 @@ impl State {
>      /// # Errors
>      /// An [`Error`] is returned if deserialization of the state object fails.
>      pub(crate) fn load(name: &str) -> Result<State, Error> {
> -        match proxmox_sys::fs::file_get_optional_contents(context().state_file_path(name))
> -            .map_err(|e| Error::ConfigDeserialization(e.into()))?
> +        let path = context().state_file_path(name);
> +
> +        match proxmox_sys::fs::file_get_optional_contents(&path)
> +            .map_err(|e| Error::StateRetrieval(path.to_owned(), e.into()))?
>          {
>              Some(bytes) => {
> -                serde_json::from_slice(&bytes).map_err(|e| Error::ConfigDeserialization(e.into()))
> +                debug!("loaded state file for endpoint '{name}' from {path}");
> +                serde_json::from_slice(&bytes).map_err(|e| Error::StateRetrieval(path, e.into()))
> +            }
> +            None => {
> +                debug!("no existing state file found for endpoint '{name}' at {path}, creating empty state");
> +                Ok(State::default())
>              }
> -            None => Ok(State::default()),
>          }
>      }
>  
> @@ -43,14 +50,16 @@ impl State {
>          let path = context().state_file_path(name);
>          let parent = std::path::Path::new(&path).parent().unwrap();
>  
> +        debug!("attempting to persist state for endpoint '{name}' at {path}");
> +
>          proxmox_sys::fs::ensure_dir_exists(parent, &context().secret_create_options(), false)
> -            .map_err(|e| Error::ConfigSerialization(e.into()))?;
> +            .map_err(|e| Error::StatePersistence(path.to_owned(), e.into()))?;
>  
>          let s = serde_json::to_string_pretty(&self)
> -            .map_err(|e| Error::ConfigSerialization(e.into()))?;
> +            .map_err(|e| Error::StatePersistence(path.to_owned(), e.into()))?;
>  
> -        proxmox_sys::fs::replace_file(path, s.as_bytes(), context().secret_create_options(), true)
> -            .map_err(|e| Error::ConfigSerialization(e.into()))
> +        proxmox_sys::fs::replace_file(&path, s.as_bytes(), context().secret_create_options(), true)
> +            .map_err(|e| Error::StatePersistence(path, e.into()))
>      }
>  
>      /// Set `last_refreshed`.
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 996393c2..37237fc0 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -41,6 +41,10 @@ pub enum Error {
>      FilterFailed(String),
>      /// The notification's template string could not be rendered
>      RenderError(Box<dyn StdError + Send + Sync>),
> +    /// The state for an endpoint could not be persisted
> +    StatePersistence(String, Box<dyn StdError + Send + Sync>),
> +    /// The state for an endpoint could not be retrieved
> +    StateRetrieval(String, Box<dyn StdError + Send + Sync>),
>      /// Generic error for anything else
>      Generic(String),
>  }
> @@ -70,6 +74,12 @@ impl Display for Error {
>              Error::FilterFailed(message) => {
>                  write!(f, "could not apply filter: {message}")
>              }
> +            Error::StatePersistence(path, err) => {
> +                write!(f, "could not persist state at {path}: {err}")
> +            }
> +            Error::StateRetrieval(path, err) => {
> +                write!(f, "could not retrieve state from {path}: {err}")
> +            }
>              Error::RenderError(err) => write!(f, "could not render notification template: {err}"),
>              Error::Generic(message) => f.write_str(message),
>          }
> @@ -86,6 +96,8 @@ impl StdError for Error {
>              Error::TargetTestFailed(errs) => Some(&*errs[0]),
>              Error::FilterFailed(_) => None,
>              Error::RenderError(err) => Some(&**err),
> +            Error::StatePersistence(_, err) => Some(&**err),
> +            Error::StateRetrieval(_, err) => Some(&**err),
>              Error::Generic(_) => None,
>          }
>      }