Re: [PATCH proxmox-backup v3 25/30] sync: pull: extend encountered chunk by optional decrypted digest

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 14.04.26 um 14:59 schrieb Christian Ebner:
> diff --git a/src/server/pull.rs b/src/server/pull.rs
>
> +    fn mark_reusable(&mut self, digest: &[u8; 32], decrypted_digest: Option<[u8; 32]>) {
>          match self.chunk_set.entry(*digest) {
>              Entry::Occupied(mut occupied) => {
> -                let (reusable, _touched) = occupied.get_mut();
> -                *reusable = true;
> +                let chunk_info = occupied.get_mut();
> +                chunk_info.reusable = true;
>              }
>              Entry::Vacant(vacant) => {
> -                vacant.insert((true, false));
> +                vacant.insert(EncounteredChunkInfo {
> +                    reusable: true,
> +                    touched: false,
> +                    decrypted_digest,
> +                });
>              }
>          }
>      }
> [...]
> +    fn mark_touched(&mut self, digest: &[u8; 32], decrypted_digest: Option<[u8; 32]>) {
>          match self.chunk_set.entry(*digest) {
>              Entry::Occupied(mut occupied) => {
> -                let (_reusable, touched) = occupied.get_mut();
> -                *touched = true;
> +                let chunk_info = occupied.get_mut();
> +                chunk_info.touched = true;
>              }
>              Entry::Vacant(vacant) => {
> -                vacant.insert((false, true));
> +                vacant.insert(EncounteredChunkInfo {
> +                    reusable: false,
> +                    touched: true,
> +                    decrypted_digest,
> +                });
>              }
>          }
>      }

Both functions silently drop the passed `decrypted_digest` in the Occupied
arm - only the Vacant branch uses it. So the first caller to touch a given
digest wins, and every subsequent call with a different (or
present-vs-absent) decrypted_digest is discarded.

Today all callers in the series pass None except the decrypt path in patch
27, so there's no observed misbehavior, but this is somewhat of a latent
footgun: any future code path that calls `mark_reusable(digest, Some(d1))`
*before* the decrypt code reaches `mark_reusable(digest, Some(d2))` (or even
just `None`) will silently lose the mapping.

Either:
 - Document the "first insert wins for decrypted_digest" semantics
   explicitly (doc comment + debug_assert on mismatch), or
 - Also update the field in the Occupied arm (probably only when the stored
   value is currently None, to preserve a decrypted_digest once it's been
   learned).

Minor: the parameter name `decrypted_digest` reads naturally in the mark
functions, but check_reusable returns it with just the cryptic `(touched,
Some(&[u8; 32]))` tuple - maybe give it a name via a small struct return or
at least a doc comment line on what the second element means.