Re: [PATCH proxmox 2/5] notify: Add state file handling

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

Hey Arthur,

great work so far. Some comments inline.

On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> Add State struct abstracting state file deserialization, updates and
> persistence, as well as an EndpointState marker trait stateful endpoints
> may implement.
>
> Also add a state_file_path method to the crate's Context trait, which
> allows
> tests to build their own context instead of depending on statics.

The context trait is more about being able to use proxmox-notify in
different products than making it testable, the latter is rather a
(great) side-effect.

>
> As far as SMTP endpoints are concerned, file locks are not necessary.
> Old Microsoft tokens stay valid for 90 days after refreshes [1], and
> Google
> tokens' lifetime is just extended at every use [2], so concurrent reads
> should not
> be an issue here.

Since this state is now pretty general purpose and *could* be used by
other endpoints for something different than OAuth tokens, it still
would make sense to think about potential race conditions and potential
solutions for them.

>
> [1]
> https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
> [2]
> https://stackoverflow.com/questions/8953983/do-google-refresh-tokens-expire
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  proxmox-notify/Cargo.toml          |  1 +
>  proxmox-notify/debian/control      |  2 +
>  proxmox-notify/src/context/mod.rs  |  2 +
>  proxmox-notify/src/context/pbs.rs  |  4 ++
>  proxmox-notify/src/context/pve.rs  |  4 ++
>  proxmox-notify/src/context/test.rs |  4 ++
>  proxmox-notify/src/lib.rs          | 60 ++++++++++++++++++++++++++++++
>  7 files changed, 77 insertions(+)
>
> diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
> index 52493ef7..daa10296 100644
> --- a/proxmox-notify/Cargo.toml
> +++ b/proxmox-notify/Cargo.toml
> @@ -40,6 +40,7 @@ proxmox-sendmail = { workspace = true, optional = true }
>  proxmox-sys = { workspace = true, optional = true }
>  proxmox-time.workspace = true
>  proxmox-uuid = { workspace = true, features = ["serde"] }
> +nix.workspace = true
>  
>  [features]
>  default = ["sendmail", "gotify", "smtp", "webhook"]
> diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/control
> index 7770f5ee..76b8a1fa 100644
> --- a/proxmox-notify/debian/control
> +++ b/proxmox-notify/debian/control
> @@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
>   librust-handlebars-5+default-dev <!nocheck>,
>   librust-http-1+default-dev <!nocheck>,
>   librust-lettre-0.11+default-dev (>= 0.11.1-~~) <!nocheck>,
> + librust-nix-0.29+default-dev <!nocheck>,
>   librust-oauth2-5+default-dev <!nocheck>,
>   librust-openssl-0.10+default-dev <!nocheck>,
>   librust-percent-encoding-2+default-dev (>= 2.1-~~) <!nocheck>,
> @@ -52,6 +53,7 @@ Depends:
>   librust-anyhow-1+default-dev,
>   librust-const-format-0.2+default-dev,
>   librust-handlebars-5+default-dev,
> + librust-nix-0.29+default-dev,
>   librust-oauth2-5+default-dev,
>   librust-openssl-0.10+default-dev,
>   librust-proxmox-http-error-1+default-dev,
> diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs
> index 8b6e2c43..86130409 100644
> --- a/proxmox-notify/src/context/mod.rs
> +++ b/proxmox-notify/src/context/mod.rs
> @@ -32,6 +32,8 @@ pub trait Context: Send + Sync + Debug {
>          namespace: Option<&str>,
>          source: TemplateSource,
>      ) -> Result<Option<String>, Error>;
> +    /// Return the state file, or None if no state file exists for this context.

This does not return an Option, so I guess the doc comment is not
correct here?

> +    fn state_file_path(&self) -> &'static str;
>  }
>  
>  #[cfg(not(test))]
> diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
> index 3e5da59c..67010060 100644
> --- a/proxmox-notify/src/context/pbs.rs
> +++ b/proxmox-notify/src/context/pbs.rs
> @@ -125,6 +125,10 @@ impl Context for PBSContext {
>              .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
>          Ok(template_string)
>      }
> +
> +    fn state_file_path(&self) -> &'static str {
> +        "/etc/proxmox-backup/notifications.state.json"
> +    }

Since we don't have a cluster filesystem in PBS, it probably would
be good to place this file in an actual state directory, such as
/var/lib/proxmox-backup/.

>  }
>  
>  #[cfg(test)]
> diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs
> index a97cce26..0dffbb11 100644
> --- a/proxmox-notify/src/context/pve.rs
> +++ b/proxmox-notify/src/context/pve.rs
> @@ -74,6 +74,10 @@ impl Context for PVEContext {
>              .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
>          Ok(template_string)
>      }
> +
> +    fn state_file_path(&self) -> &'static str {
> +        "/etc/pve/priv/notifications.state.json"
> +    }
>  }
>  
>  pub static PVE_CONTEXT: PVEContext = PVEContext;
> diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/context/test.rs
> index 2c236b4c..e0236b9c 100644
> --- a/proxmox-notify/src/context/test.rs
> +++ b/proxmox-notify/src/context/test.rs
> @@ -40,4 +40,8 @@ impl Context for TestContext {
>      ) -> Result<Option<String>, Error> {
>          Ok(Some(String::new()))
>      }
> +
> +    fn state_file_path(&self) -> &'static str {
> +        "/tmp/notifications.state.json"
> +    }
>  }
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 1134027c..a40342cc 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -6,6 +6,7 @@ use std::fmt::Display;
>  use std::str::FromStr;
>  
>  use context::context;
> +use serde::de::DeserializeOwned;
>  use serde::{Deserialize, Serialize};
>  use serde_json::json;
>  use serde_json::Value;
> @@ -272,6 +273,65 @@ impl Notification {
>      }
>  }
>  
> +#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq, Eq)]
> +pub struct State {
> +    #[serde(flatten)]
> +    pub sections: HashMap<String, Value>,
> +}
> +
> +impl FromStr for State {
> +    type Err = Error;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        serde_json::from_str(s).map_err(|e| Error::ConfigDeserialization(e.into()))
> +    }
> +}
> +
> +/// Marker trait to be implemented by the state structs for stateful endpoints.
> +pub trait EndpointState: Serialize + DeserializeOwned + Default {}
> +
> +impl State {
> +    pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Result<Self, Error> {
> +        let contents = proxmox_sys::fs::file_read_string(path)
> +            .map_err(|e| Error::ConfigDeserialization(e.into()))?;
> +        Self::from_str(&contents)

Btw, you could use proxmox_sys::file_get_contents and then deserialize
using serde_json::from_slice; this should be a bit more efficient since
it does not need to check if it is valid utf-8 at all.

There is also promxox_sys::file_get_optional_contents -- see the next
patch for the context why I mention this.

> +    }
> +
> +    pub fn persist<P: AsRef<std::path::Path>>(&self, path: P) -> Result<(), Error> {
> +        let state_str =
> +            serde_json::to_string_pretty(self).map_err(|e| Error::ConfigSerialization(e.into()))?;
> +
> +        let mode = nix::sys::stat::Mode::from_bits_truncate(0o600);
> +        let options = proxmox_sys::fs::CreateOptions::new().perm(mode);

I think it might be better to provide the CreateOptions also via some
method in Context - then the application has full control over the
permissions, user and group for the file.
Since you also provide the path via a parameter, I guess the caller of
this function would retrieve it from the context and then pass it along
with the path.

I think I'd use something generic like "secret_create_options" (name
taken from `proxmox-product-config`), indicating that we want the
CreateOptions for *something* secret/sensitive.

Also you don't need the `nix` crate then, I think.

> +
> +        proxmox_sys::fs::replace_file(path, state_str.as_bytes(), options, true)
> +            .map_err(|e| Error::ConfigSerialization(e.into()))
> +    }
> +
> +    pub fn get<S: EndpointState>(&self, name: &str) -> Result<Option<S>, Error> {
> +        match self.sections.get(name) {
> +            Some(v) => Ok(Some(
> +                S::deserialize(v).map_err(|e| Error::ConfigDeserialization(e.into()))?,
> +            )),
> +            None => Ok(None),
> +        }
> +    }
> +
> +    pub fn get_or_default<S: EndpointState>(&self, name: &str) -> Result<S, Error> {
> +        Ok(self.get(name)?.unwrap_or_default())
> +    }
> +
> +    pub fn set<S: EndpointState>(&mut self, name: &str, state: &S) -> Result<(), Error> {
> +        let v = serde_json::to_value(state).map_err(|e| Error::ConfigSerialization(e.into()))?;
> +        self.sections.insert(name.to_string(), v);
> +        Ok(())
> +    }
> +
> +    pub fn remove(&mut self, name: &str) {
> +        self.sections.remove(name);
> +    }
> +}

Also here, same as for the other patch, consider adding doc-comments -
and `pub(crate)` might also be a good fit here.

> +
>  /// Notification configuration
>  #[derive(Debug, Clone)]
>  pub struct Config {