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

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

On 2/10/26 1:57 PM, Christian Ebner wrote:
> 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.

Makes sense, I’ll update it to match this. Thanks!

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