From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 103B71FF14F for ; Tue, 28 Apr 2026 03:46:46 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5DE191D03; Tue, 28 Apr 2026 03:46:45 +0200 (CEST) From: Thomas Lamprecht To: pbs-devel@lists.proxmox.com Subject: [PATCH 2/2] client: pxar: re-encode files with non-monotonic previous offsets Date: Tue, 28 Apr 2026 03:28:10 +0200 Message-ID: <20260428014635.2655155-3-t.lamprecht@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260428014635.2655155-1-t.lamprecht@proxmox.com> References: <20260428014635.2655155-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777340705358 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.003 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [create.rs] Message-ID-Hash: NQ5KEGDFTAM5MWMSKF2XE2B7JAQE5TJM X-Message-ID-Hash: NQ5KEGDFTAM5MWMSKF2XE2B7JAQE5TJM X-MailFrom: t.lamprecht@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 Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: The strict payload-offset check added in pxar 1.0.1 rejects any payload_offset not strictly greater than the encoder's previously recorded one. Older client versions could write metadata archives with duplicate or non-monotonic PXAR_PAYLOAD_REF offsets via the producer-side bug fixed in the previous commit. Without the encode- time guard such an archive looked fine on disk, but every later metadata-mode incremental backup now aborts on the strict check until the chain is cycled out via --change-detection-mode=data. Track the highest previously-recorded payload offset globally on the Archiver and treat any prev-archive entry with a smaller-or-equal offset as non-reusable, so it is re-encoded instead. The chain self-heals on the next run, and a single warning per affected file is emitted to make the fall-back visible. Tracking is intentionally global, not per cache range, so inversions that span ranges (because the next backup's cache flushes happen between the two inverted offsets) are caught too. Healthy archives written on or after pxar 1.0.1 cannot trigger this fall-back since the encoder already enforces strict monotonicity at write time. Signed-off-by: Thomas Lamprecht --- pbs-client/src/pxar/create.rs | 86 +++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs index 15e149a23..70b858092 100644 --- a/pbs-client/src/pxar/create.rs +++ b/pbs-client/src/pxar/create.rs @@ -188,6 +188,11 @@ struct Archiver { suggested_boundaries: Option>, previous_payload_index: Option, cache: PxarLookaheadCache, + // Highest payload offset accepted as reusable so far. Mirrors the encoder's accumulated + // strict-monotonic-offset invariant on the read side, so that a previous archive written by + // an older client without the encode-time check is detected here and the affected file is + // re-encoded instead of reused. + last_reusable_offset: Option, reuse_stats: ReuseStats, split_archive: bool, } @@ -298,6 +303,7 @@ where suggested_boundaries, previous_payload_index, cache: PxarLookaheadCache::new(options.max_cache_size), + last_reusable_offset: None, reuse_stats: ReuseStats::default(), split_archive, }; @@ -418,6 +424,16 @@ impl Archiver { .boxed() } + /// Record an accepted reusable payload offset from the previous metadata archive. + /// + /// Returns `false` if `offset` is not strictly greater than the last accepted offset, which + /// signals a duplicate or non-monotonic previous archive. The caller must then treat the file + /// as non-reusable so the encoder's strict offset check cannot reject the eventual + /// `add_payload_ref`. Tracking is global to catch inversions that span cache ranges. + fn try_record_reusable_offset(&mut self, offset: u64) -> bool { + try_record_strictly_greater(&mut self.last_reusable_offset, offset) + } + async fn is_reusable_entry( &mut self, previous_metadata_accessor: &Option>, @@ -437,6 +453,19 @@ impl Archiver { if file_size != *size { return Ok(None); } + // a previous archive written by an older client version may contain + // duplicate or non-monotonic PXAR_PAYLOAD_REF offsets for distinct files. + // Re-encoding the affected file lets the chain self-heal and avoids the + // pxar strict offset check rejecting the whole backup. + if !self.try_record_reusable_offset(*offset) { + let last = self.last_reusable_offset.unwrap_or(0); + warn!( + "re-encode: {file_name:?} previous archive payload offset \ + {offset} not strictly greater than last seen {last}, \ + treating as non-reusable" + ); + return Ok(None); + } let range = *offset..*offset + size + size_of::() as u64; debug!( @@ -1378,6 +1407,19 @@ impl ReusableDynamicEntry { } } +/// Updates `last` to `offset` and returns `true` if `offset` is strictly greater than the current +/// value (or `last` is `None`). Returns `false` without modifying `last` for duplicate or +/// backwards offsets. +fn try_record_strictly_greater(last: &mut Option, offset: u64) -> bool { + if let Some(prev) = *last { + if offset <= prev { + return false; + } + } + *last = Some(offset); + true +} + /// List of dynamic entries containing the data given by an offset range fn lookup_dynamic_entries( index: &DynamicIndexReader, @@ -2009,6 +2051,7 @@ mod tests { previous_payload_index, suggested_boundaries: Some(suggested_boundaries), cache: PxarLookaheadCache::new(None), + last_reusable_offset: None, reuse_stats: ReuseStats::default(), split_archive: true, }; @@ -2071,4 +2114,47 @@ mod tests { "take_last_chunk must consume the value" ); } + + /// Strictly-greater bookkeeping for [`Archiver::try_record_reusable_offset`]; mirrors the + /// encoder's accumulated strict-monotonic-offset invariant on the read side. + #[test] + fn try_record_strictly_greater_accepts_increasing() { + let mut last = None; + assert!(super::try_record_strictly_greater(&mut last, 100)); + assert_eq!(last, Some(100)); + assert!(super::try_record_strictly_greater(&mut last, 200)); + assert_eq!(last, Some(200)); + } + + #[test] + fn try_record_strictly_greater_rejects_equal_and_backwards() { + let mut last = Some(200); + // duplicate offset: previous archive recorded two distinct files at the same offset + assert!(!super::try_record_strictly_greater(&mut last, 200)); + assert_eq!(last, Some(200), "rejected offset must not update state"); + // backwards offset: previous archive recorded the next file at a lower offset + assert!(!super::try_record_strictly_greater(&mut last, 150)); + assert_eq!(last, Some(200)); + } + + #[test] + fn try_record_strictly_greater_first_call_accepts_any_value() { + let mut last = None; + assert!(super::try_record_strictly_greater(&mut last, 0)); + assert_eq!(last, Some(0)); + } + + /// Inversions that span cache ranges must be caught by the global tracking; a per-range + /// implementation would have reset on flush and accepted the second offset, after which + /// the encoder strict check would still abort the backup. + #[test] + fn try_record_strictly_greater_persists_across_resets() { + let mut last = None; + assert!(super::try_record_strictly_greater(&mut last, 1000)); + // a per-range implementation would have cleared `last` here on cache flush + assert!(!super::try_record_strictly_greater(&mut last, 500)); + assert_eq!(last, Some(1000)); + assert!(super::try_record_strictly_greater(&mut last, 1500)); + assert_eq!(last, Some(1500)); + } } -- 2.47.3