From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 39D711FF0E7 for ; Fri, 10 Jul 2026 13:36:34 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 8A0C42145E; Fri, 10 Jul 2026 13:36:33 +0200 (CEST) Message-ID: <3210199f-485c-4e99-b0ec-67c28fd7e716@proxmox.com> Date: Fri, 10 Jul 2026 13:36:00 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH datacenter-manager 08/15] task cache: archive iterator: don't yield more items if reading from file failed To: Lukas Wagner , pdm-devel@lists.proxmox.com References: <20260702092258.174740-1-l.wagner@proxmox.com> <20260702092258.174740-9-l.wagner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260702092258.174740-9-l.wagner@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783683349200 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.059 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) 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: 5EQNHTLH4TN3SCCPMPTW5SZISGZHMJSA X-Message-ID-Hash: 5EQNHTLH4TN3SCCPMPTW5SZISGZHMJSA X-MailFrom: d.csapak@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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: sorry maybe i'm not deep enough into the code here but how would a single err result produce an endless loop? the underlying lines iterator should not infinitely return the error? maybe a bit more context for that issue in the commit message would be nice.... (or maybe it's obvious enough for other, just not me ;) ) also should we handle partial corruption, e.g. when only a single line can't be read? (this skips the rest IIUC) On 7/2/26 11:23 AM, Lukas Wagner wrote: > Otherwise `get_tasks` runs into an endless loop if any of the archive > files is corrupted. > > Also add a test case that verifies the absence of the issue. > > Signed-off-by: Lukas Wagner > --- > server/src/remote_tasks/task_cache.rs | 48 +++++++++++++++++++++++---- > 1 file changed, 42 insertions(+), 6 deletions(-) > > diff --git a/server/src/remote_tasks/task_cache.rs b/server/src/remote_tasks/task_cache.rs > index f0e3533a..02be9ca1 100644 > --- a/server/src/remote_tasks/task_cache.rs > +++ b/server/src/remote_tasks/task_cache.rs > @@ -1244,6 +1244,7 @@ where > /// tasks are read line by line, without leading the entire archive file into memory. > struct ArchiveIterator { > iter: Lines>, > + failed: bool, > } > > impl ArchiveIterator { > @@ -1251,7 +1252,10 @@ impl ArchiveIterator { > pub fn new(reader: Box) -> Self { > let lines = reader.lines(); > > - Self { iter: lines } > + Self { > + iter: lines, > + failed: false, > + } > } > } > > @@ -1259,11 +1263,18 @@ impl Iterator for ArchiveIterator { > type Item = Result; > > fn next(&mut self) -> Option { > - self.iter.next().map(|result| { > - result > - .and_then(|line| Ok(serde_json::from_str(&line)?)) > - .map_err(Into::into) > - }) > + if self.failed { > + // Don't return any more items if we have failed reading a line once > + return None; > + } > + > + self.iter.next().map(|line| match line { > + Ok(line) => Some(serde_json::from_str(&line).context("failed to decode JSON")), > + Err(err) => { > + self.failed = true; > + Some(Err(err).context("failed to read line")) > + } > + })? > } > } > > @@ -1661,4 +1672,29 @@ mod tests { > assert_eq!(first.iter().unwrap().unwrap().count(), 0); > assert_eq!(second.iter().unwrap().unwrap().count(), 1); > } > + > + #[test] > + fn corrupted_archive_file_does_not_lead_to_endless_loop() { > + let (_tmp_dir, cache) = make_cache().unwrap(); > + let cache = cache.write().unwrap(); > + > + // Create compressed file > + cache.new_file(1000, true).unwrap(); > + add_tasks(&cache, vec![task(1100, Some(1110))]).unwrap(); > + cache.apply_journal().unwrap(); > + > + assert_eq!(cache.get_tasks(GetTasks::Archived).unwrap().count(), 1); > + > + let files = cache.cache.archive_files(&cache.lock).unwrap(); > + let file = files.get(0).expect("there is one archive file"); > + > + // truncate existing compressed file, corrupting the zstd file header > + let _file = OpenOptions::new() > + .write(true) > + .truncate(true) > + .open(&file.path) > + .expect("file truncated"); > + > + assert_eq!(cache.get_tasks(GetTasks::Archived).unwrap().count(), 0); > + } > }