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 <[email protected]> > --- > 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 {
