Skip to content

perf(onepassword): cache auth and batch fetch secrets#56

Merged
domenkozar merged 1 commit intomainfrom
perf/onepassword-batch-fetch
Jan 30, 2026
Merged

perf(onepassword): cache auth and batch fetch secrets#56
domenkozar merged 1 commit intomainfrom
perf/onepassword-batch-fetch

Conversation

@domenkozar
Copy link
Member

  • Cache whoami result using OnceCell to avoid repeated auth checks
  • Add get_batch() method to Provider trait for batch fetching
  • Implement parallel batch fetching for OnePassword provider
  • Update validate() to group secrets by provider and use batch ops

This reduces CLI calls from 2N sequential to ~2 + N parallel for N secrets.

- Cache whoami result using OnceCell to avoid repeated auth checks
- Add get_batch() method to Provider trait for batch fetching
- Implement parallel batch fetching for OnePassword provider
- Update validate() to group secrets by provider and use batch ops

This reduces CLI calls from 2N sequential to ~2 + N parallel for N secrets.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
secretspec ee0a592 Commit Preview URL

Branch Preview URL
Jan 29 2026, 10:13 PM

@domenkozar domenkozar merged commit de55b8a into main Jan 30, 2026
5 checks passed
@claude
Copy link

claude bot commented Jan 30, 2026

Claude finished @domenkozar's task —— View job


PR Review: OnePassword Batch Fetch Performance Optimization

This PR introduces significant performance improvements to the OnePassword provider through authentication caching and batch fetching. Here's my comprehensive review:

Review Checklist

  • Analyze code quality and best practices
  • Identify potential bugs or issues
  • Assess performance considerations
  • Check for security concerns
  • Evaluate test coverage

Code Quality & Best Practices ✅

Excellent architecture decisions:

  • Clean separation of concerns with batch fetching as a trait method
  • Proper use of OnceCell for thread-safe lazy initialization of auth status
  • Well-structured parallel execution using threads with proper resource management
  • Good error handling throughout the implementation

Minor suggestions:

  • Consider adding documentation to explain the performance tradeoff between batch vs individual fetching
  • The hardcoded thread pool approach works well but could benefit from a comment explaining why std::thread is chosen over a thread pool library

Performance Improvements 🚀

Significant gains achieved:

  • Authentication caching: Eliminates repeated op whoami calls using OnceCell - excellent optimization
  • Batch fetching: Reduces OnePassword CLI calls from 2N sequential to ~2 + N parallel
  • Smart grouping: Groups secrets by provider for optimal batch operations in secretspec/src/secrets.rs:1184-1227

Implementation strengths:

  • Parallel thread execution for fetching individual items after bulk listing
  • Proper fallback handling for providers with multiple aliases
  • Maintains backward compatibility with existing single-secret fetching

Potential Issues & Improvements 🔍

Thread safety & resource management:

// In onepassword.rs:793-850
let handles: Vec<_> = keys_to_fetch
    .into_iter()
    .map(|(key, item_id)| {
        // Creates one thread per secret - could be resource intensive for many secrets
  • Concern: Creating one thread per secret could be problematic with many secrets (100+ secrets = 100+ threads)
  • Suggestion: Consider using a thread pool with limited concurrency or rayon for better resource management

Error handling in batch operations:

// In onepassword.rs:855
if let Ok(Some((key, value))) = handle.join() {
    results.insert(key, value);
}
  • Issue: Silent failure handling - errors from individual threads are ignored
  • Recommendation: Consider logging failed fetches or collecting errors for debugging

Memory usage:

  • Cloning configuration data for each thread could be optimized
  • Consider using Arc for shared read-only data like op_command and config

Security Considerations 🔒

Good security practices:

  • Proper use of SecretString throughout the implementation
  • No secrets logged or exposed in error messages
  • Service account tokens properly handled via environment variables
  • Authentication state cached securely without exposing credentials

No security concerns identified - the implementation maintains the existing security model while improving performance.

Test Coverage 📋

Current testing:

  • Generic provider tests in secretspec/src/provider/tests.rs cover basic functionality
  • Integration tests exist for provider implementations
  • Batch fetching would be tested through the generic provider test suite

Recommendations:

  • Add specific tests for batch fetching with multiple secrets
  • Test thread safety with concurrent batch operations
  • Add performance benchmarks to validate the claimed improvements
  • Test error scenarios in batch operations (partial failures, thread panics)

Overall Assessment ⭐

This is a well-implemented performance optimization that addresses a real pain point with OnePassword CLI latency. The code quality is high, architecture is sound, and the performance gains are substantial.

Key strengths:

  • Solves a genuine performance problem
  • Maintains backward compatibility
  • Clean, readable implementation
  • Proper error handling and resource management

Minor areas for improvement:

  • Thread pool management for high secret counts
  • Enhanced error reporting in batch operations
  • Additional test coverage for edge cases

The PR successfully achieves its goal of reducing OnePassword CLI overhead and should provide significant real-world performance improvements for users with multiple secrets.

Recommendation: Approve with minor follow-up considerations for thread pool optimization in future iterations.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant