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

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

On Fri, Apr 24, 2026 at 10:04:49AM +0200, Shannon Sterz wrote:
> On Fri Apr 24, 2026 at 9:47 AM CEST, Arthur Bied-Charreton wrote:
> > 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:
> 
> -->8 snip 8<--
> 
> >> > +    /// 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.
> >
> 
> just to be sure, since i realized this is misleadingly phrased in my
> initial message: make sure not to lock the state files themselves.
> rather keep around a lock file that won't be removed if the state file
> is removed. otherwise, you can run into a different kind of race between
> deleting and writing threads.
> 
That was clear, but thanks for the clarification still :)
> we do this already for almost all other config files. if you are worried
> about "polluting" the state directory here, you can consider putting
> these lock files in a sub-directory on `/run`. though, this would
> require extra care to integrate it with all products. hope that makes
> sense and thanks for tackling this!
> 
Yes, we already have the Context trait in proxmox-notify, will add a
lock_oauth_state method that allows to handle this on a per-product
basis. I was thinking of putting them into /run for PBS and PDM, but for
PVE they should be in /etc/pve right?
> -->8 snip 8<--