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

["Shannon Sterz" <s.sterz@proxmox.com>]

On Fri Apr 24, 2026 at 10:54 AM CEST, Arthur Bied-Charreton wrote:
> 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?

yes, there you need them on `/etc/pve` so locks work across the cluster.

>> -->8 snip 8<--