Re: [pbs-devel] [PATCH proxmox-backup v4 4/4] pbs-config: add TTL window to token secret cache

[Christian Ebner <c.ebner@proxmox.com>]

one suggestion inline

On 1/21/26 4:13 PM, Samuel Rufinatscha wrote:
> Verify_secret() currently calls refresh_cache_if_file_changed() on every
> request, which performs a metadata() call on token.shadow each time.
> Under load this adds unnecessary overhead, considering also the file
> usually should rarely change.
> 
> This patch introduces a TTL boundary, controlled by
> TOKEN_SECRET_CACHE_TTL_SECS. File metadata is only re-loaded once the
> TTL has expired; documents TTL effects.
> 
> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
> ---
> Changes from v3 to 4:
> * Adjusted commit message
> 
> Changes from v2 to v3:
> * Refactored refresh_cache_if_file_changed TTL logic.
> * Remove had_prior_state check (replaced by last_checked logic).
> * Improve TTL bound checks.
> * Reword documentation warning for clarity.
> 
> Changes from v1 to v2:
> * Add TOKEN_SECRET_CACHE_TTL_SECS and last_checked.
> * Implement double-checked TTL: check with try_read first; only attempt
>    refresh with try_write if expired/unknown.
> * Fix TTL bookkeeping: update last_checked on the “file unchanged” path
>    and after API mutations.
> * Add documentation warning about TTL-delayed effect of manual
>    token.shadow edits.
> 
>   docs/user-management.rst       |  4 ++++
>   pbs-config/src/token_shadow.rs | 29 ++++++++++++++++++++++++++++-
>   2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/user-management.rst b/docs/user-management.rst
> index 41b43d60..8dfae528 100644
> --- a/docs/user-management.rst
> +++ b/docs/user-management.rst
> @@ -156,6 +156,10 @@ metadata:
>   Similarly, the ``user delete-token`` subcommand can be used to delete a token
>   again.
>   
> +.. WARNING:: Direct/manual edits to ``token.shadow`` may take up to 60 seconds (or
> +   longer in edge cases) to take effect due to caching. Restart services for
> +   immediate effect of manual edits.
> +
>   Newly generated API tokens don't have any permissions. Please read the next
>   section to learn how to set access permissions.
>   
> diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
> index a5bd1525..24633f6e 100644
> --- a/pbs-config/src/token_shadow.rs
> +++ b/pbs-config/src/token_shadow.rs
> @@ -31,6 +31,8 @@ static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new
>           shadow: None,
>       })
>   });
> +/// Max age in seconds of the token secret cache before checking for file changes.
> +const TOKEN_SECRET_CACHE_TTL_SECS: i64 = 60;
>   
>   #[derive(Serialize, Deserialize)]
>   #[serde(rename_all = "kebab-case")]
> @@ -72,11 +74,29 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
>   fn refresh_cache_if_file_changed() -> bool {
>       let now = epoch_i64();
>   
> -    // Best-effort refresh under write lock.
> +    // Fast path: cache is fresh if shared-gen matches and TTL not expired.
> +    if let (Some(cache), Some(shared_gen_read)) =
> +        (TOKEN_SECRET_CACHE.try_read(), token_shadow_shared_gen())
> +    {
> +        if cache.shared_gen == shared_gen_read

starting here ..

> +            && cache.shadow.as_ref().is_some_and(|cached| {
> +                now >= cached.last_checked
> +                    && (now - cached.last_checked) < TOKEN_SECRET_CACHE_TTL_SECS

... to here is the same as ...

> +            })
> +        {
> +            return true;
> +        }
> +        // read lock drops here
> +    } else {
> +        return false;
> +    }
> +
> +    // Slow path: best-effort refresh under write lock.
>       let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
>           return false;
>       };
>   
> +    // Re-read generation after acquiring the lock (may have changed meanwhile).
>       let Some(shared_gen_now) = token_shadow_shared_gen() else {
>           return false;
>       };
> @@ -86,6 +106,13 @@ fn refresh_cache_if_file_changed() -> bool {
>           invalidate_cache_state_and_set_gen(&mut cache, shared_gen_now);
>       }
>   
> +    // TTL check again after acquiring the lock
> +    if cache.shadow.as_ref().is_some_and(|cached| {
> +        now >= cached.last_checked && (now - cached.last_checked) < TOKEN_SECRET_CACHE_TTL_SECS

... this check above. I think this could be defined as method on the 
cache object so it can be easily reused and the code is more readable.

> +    }) {
> +        return true;
> +    }
> +
>       // Stat the file to detect manual edits.
>       let Ok((new_mtime, new_len)) = shadow_mtime_len() else {
>           return false;