Skip to content

[ENG-740] 1/6: Redis cache manager#7462

Merged
johnewart merged 12 commits intomainfrom
johnewart/ENG-740/0-redis-cache-manager
Feb 24, 2026
Merged

[ENG-740] 1/6: Redis cache manager#7462
johnewart merged 12 commits intomainfrom
johnewart/ENG-740/0-redis-cache-manager

Conversation

@johnewart
Copy link
Copy Markdown
Collaborator

@johnewart johnewart commented Feb 23, 2026

Ticket ENG-2757

Description Of Changes

TL;DR - redis cache manager with automatic set-based key indexing, lazy migration of old keys, is not used in any APIs or async functions in this PR, it is purely the storage logic.

This PR lays the groundwork for a Redis cache layer that avoids using KEYS (and eventually SCAN) by providing a convenience layer for secondary index storage (and also as a starting point for other Redis improvements like clustering, enhanced logging, monitoring, etc). This implementation provides the following (and nothing else) - a RedisCacheManager that provides a layer around the Redis client to automatically handle secondary indexing of keys so that they can be cleared and queried without synchronous (and slow) access like KEYS (or SCAN). It does this by allowing you to specify an index name and then transparently recording keys that are related to that index in a Redis set with that name (i.e dsr:1234 or something). Then you can quickly get all the keys for iteration or determine membership, etc.

The RedisCacheManager is not used in any active API/async worker code in this PR, it is purely a base for the follow ons - DSR storage layer, identity cache, encryption and custom fields, request body, etc. in subsequent PRs.

Code Changes

  • Initial RedisCacheManager with secondary indexing

Steps to Confirm

  1. No manual steps

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@johnewart johnewart requested a review from JadeCara February 23, 2026 23:19
@johnewart johnewart requested a review from a team as a code owner February 23, 2026 23:19
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Feb 23, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
fides-plus-nightly Ready Ready Preview, Comment Feb 24, 2026 10:38pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
fides-privacy-center Ignored Ignored Feb 24, 2026 10:38pm

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 23, 2026

Greptile Summary

This PR introduces a foundational RedisCacheManager that provides set-based key indexing to avoid expensive KEYS and SCAN operations in Redis. The implementation is clean, well-documented, and includes comprehensive test coverage.

Key Features:

  • Set-based secondary indexing using Redis sets (stored with __idx: prefix)
  • Pipeline-based atomic operations for setting/deleting keys with index updates
  • Optional TTL management for index sets (opt-in via index_ttl_enabled parameter)
  • Consistent string return types from get_keys_by_index (handles bytes conversion)

Test Coverage:

  • All public methods have dedicated tests including edge cases
  • MockRedis implementation provides realistic testing without external dependencies
  • TTL functionality comprehensively tested with multiple scenarios

The implementation properly addresses previous review feedback regarding bytes/string handling and index TTL management. No active API or async worker code uses this manager yet - it's purely foundational infrastructure for follow-on PRs.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • Clean implementation of foundational infrastructure with excellent test coverage. The RedisCacheManager provides a well-designed abstraction for Redis key indexing with proper pipeline usage, comprehensive edge case handling, and thorough documentation. Previous review feedback has been properly addressed. Since this is not yet used in active code paths, there is no risk of production impact.
  • No files require special attention

Important Files Changed

Filename Overview
src/fides/common/cache/manager.py New RedisCacheManager implementation with set-based indexing and optional TTL management. Clean implementation with good documentation.
tests/common/cache/test_manager.py Comprehensive test coverage with MockRedis implementation. Tests cover all public methods, edge cases, and TTL functionality.

Last reviewed commit: b1a43cc

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@johnewart johnewart changed the title Johnewart/eng 740/0 redis cache manager [ENG-740] 1/6: Redis cache manager Feb 24, 2026
Copy link
Copy Markdown
Contributor

@JadeCara JadeCara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid! I left a few smaller comments etc but approving so you can merge when ready :)

The only main question I have is - I see all the mechanics we need to handle the indexes well. If keys get expired via TTL the index set doesn't get automatically evicted. In implementation we should be more explicit about it, but it might be worth adding a prune function to clean up any any index members whose data keys no longer exist.

johnewart and others added 6 commits February 24, 2026 08:58
Deserialize keys to strings when in `get_keys_by_index` - preferring deliberate conversion over `decode_responses`

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@johnewart johnewart marked this pull request as draft February 24, 2026 22:23
@johnewart johnewart marked this pull request as ready for review February 24, 2026 22:23
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@johnewart johnewart enabled auto-merge February 24, 2026 22:30
@johnewart johnewart added this pull request to the merge queue Feb 24, 2026
Merged via the queue into main with commit 5a6b6bc Feb 24, 2026
53 of 54 checks passed
@johnewart johnewart deleted the johnewart/ENG-740/0-redis-cache-manager branch February 24, 2026 23:09
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.

2 participants