fix: macOS keychain infinite prompt loop

[sheikhlimon]

Summary

Problem: Each call to get_secret() hits keychain separately, causing multiple prompts when loading provider config with multiple secret parameters.
Solution: Add in-memory cache to all_secrets() so keychain is accessed once per config load.

• Before: N keychain prompts for N secret parameters when loading provider config
• After: 1 keychain prompt per config load, cached for subsequent reads in same session

Type of Change

• Feature
• Bug fix
• Refactor / Code quality
• Performance improvement
• Documentation
• Tests
• Security fix
• Build / Release
• Other (specify below)

AI Assistance

• This PR was created or reviewed with AI assistance

Related Issues

Fixes #6595

[michaelneale]

I think this would be very nice @codefromthecrypt looks ok to you (once it is a clean build)

[codefromthecrypt]

TL;DR; Thanks for doing this. Only main thing to pay attention to is a lazy loading race, and let me know if I'm off on it. Otherwise, looks good.

---

I think if we wanted to do a super clean job it would be refactoring to a real loading cache like moka, and/or concurrent tests like loom, but that's probably over solving and I like your stepping in cautiously.

For this code.. I think there's a potential lazy loading race (e.g., multiple threads could check None outside the lock and all try to load, causing redundant hits). Something like this might fix it (didn't try it, but should serialize the load):

pub fn all_secrets(&self) -> Result<HashMap<String, Value>, ConfigError> {
    let mut cache = self.secrets_cache.lock().unwrap();
    let values = if let Some(ref cached) = *cache {
        cached.clone()
    } else {
        let loaded = match &self.secrets {
            SecretStorage::Keyring { service } => { /* ... load ... */ },
            SecretStorage::File { path } => self.read_secrets_from_file(path)?,
        };
        *cache = Some(loaded.clone());
        loaded
    };
    Ok(values)
}

Also, if you wanted to test this you could add instrumentation like a test-only SecretStorage.load_count: Arc<Mutex<u32>> (increment on load), then in a unit test simulate multiple get_secret calls (maybe even in threads) and assert only 1 load happened. Again, that's not full parallel testing, but low hanging fruit.

[sheikhlimon]

For this code.. I think there's a potential lazy loading race (e.g., multiple threads could check None outside the lock and all try to load, causing redundant hits).

Thanks for reviewing this! I’ve implemented your suggested fix by holding the lock across the entire cache check and load operation.
This prevents multiple threads from observing None and loading simultaneously. The first thread to acquire the lock performs the load, and subsequent callers wait and then use the cached result.

[codefromthecrypt]

thanks again for the help!