From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 361961FF140 for ; Fri, 24 Apr 2026 10:05:32 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3A9A0C5D2; Fri, 24 Apr 2026 10:05:29 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 24 Apr 2026 10:04:49 +0200 Message-Id: Subject: Re: [PATCH proxmox v4 03/24] notify: smtp: Introduce state management To: "Arthur Bied-Charreton" X-Mailer: aerc 0.20.0 References: <20260421115957.402589-1-a.bied-charreton@proxmox.com> <20260421115957.402589-4-a.bied-charreton@proxmox.com> In-Reply-To: From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777017799122 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.120 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: XCJYLJFCP6SF722NZK2X5UROSLNRWDCP X-Message-ID-Hash: XCJYLJFCP6SF722NZK2X5UROSLNRWDCP X-MailFrom: s.sterz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com, pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 =3D "smtp")] >> > + fn save_oauth_state(&self, endpoint_name: &str, state: Option) -> 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<--