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

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

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.

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!

-->8 snip 8<--