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 8EF971FF140 for ; Fri, 24 Apr 2026 10:59:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A1E3F1091F; Fri, 24 Apr 2026 10:59:48 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 24 Apr 2026 10:59:14 +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: 1777021064066 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.119 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: DYJIMUF2HQ5OU44R4XKPYUHWKLK5FYMH X-Message-ID-Hash: DYJIMUF2HQ5OU44R4XKPYUHWKLK5FYMH 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 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 j= ust let the faster node "win" >> >> > + /// as long as the invariants documented by [`smtp::xoauth2::g= et_microsoft_token`] and >> >> > + /// [`smtp::xoauth2::get_google_token`] hold, see those functi= ons' doc comments for details. >> >> > + #[cfg(feature =3D "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 conf= ig >> >> 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 sam= e >> >> name after B finishes, then it would suddenly be left with a state fi= le >> >> from the previous endpoint. similar examples can also happen when >> >> calling `build_transport` since that also includes a read & write cyc= le >> >> of the state file. >> >> >> > Yes you are right, I may have been too focused on refresh token validi= ty >> > and overlooked this - thanks a lot for catching it! >> > >> >> if the state file were locked properly, B would fail to acquire the l= ock >> >> 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<--