ENG-3375: Add TTL to RedisRepository keys

[galvana]

Ticket ENG-3375

Description Of Changes

RedisRepository.save() was writing entity keys, _all index sets, and secondary index sets to Redis with no expiration, causing unbounded memory growth across all PBAC subclasses (DataPurpose, DataConsumer). The existing TTL pattern in FidesopsRedis.set_with_autoexpire() using CONFIG.redis.default_ttl_seconds (7 days) was not being used by RedisRepository, which bypassed it with raw pipeline operations.

This PR applies CONFIG.redis.default_ttl_seconds to all keys written by RedisRepository.save():

• Entity keys via pipe.set(key, value, ex=ttl)
• _all index sets via pipe.expire()
• Secondary index sets via pipe.expire()

A ttl_seconds constructor parameter allows per-instance override for testing and future flexibility.

Note: The fidesplus companion PR (fidesplus#3391) applies the same fix to the fidesplus copy of RedisRepository and adds TTL to the ViolationRedisRepository and QueryAccessLogRedisRepository time-sorted index sets.

Code Changes

• src/fides/service/pbac/redis_repository.py — Added ttl_seconds constructor param (defaults to CONFIG.redis.default_ttl_seconds); applied TTL to entity key (pipe.set(ex=)), _all set, and all secondary index sets (pipe.expire())
• tests/service/pbac/test_redis_repository.py — New unit tests verifying TTL on entity keys, _all set, index sets, pipeline execution, stale index cleanup on update, CONFIG default fallback, and delete behavior

Steps to Confirm

1. Review that save() applies TTL to all three key types (entity, _all, index)
2. Verify tests pass: pytest tests/service/pbac/test_redis_repository.py -v
3. Confirm no behavioral regression in PBAC API endpoints (/v1/data-purpose, /v1/data-consumer)

Pre-Merge Checklist

• Issue requirements met
• All CI pipelines succeeded
• CHANGELOG.md updated
  • Add a db-migration label
  • Add a high-risk label
  • Updates unreleased work already in Changelog
• UX feedback:
  • No UX review needed
• Followup issues:
  • No followup issues
• Database migrations:
  • No migrations
• Documentation:
  • No documentation updates required

🤖 Generated with Claude Code

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ready	Preview, Comment	Apr 14, 2026 0:29am

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
fides-privacy-center	Ignored		Apr 14, 2026 0:29am

[claude]

Code Review: Add TTL to RedisRepository keys

The fix is correct in intent — applying ex=self._ttl_seconds to entity keys and EXPIRE to the shared set and index keys closes the unbounded memory growth path. A few issues worth addressing before merge.

---

Critical

DataConsumerRedisRepository silently swallows the ttl_seconds parameter

DataConsumerRedisRepository.__init__ calls super().__init__(cache) without forwarding ttl_seconds. This means callers cannot construct a DataConsumerRedisRepository with a custom TTL — the parameter added to the base class is inaccessible from the only concrete OSS subclass. If a companion PR in fidesplus introduces per-subclass TTL overrides, this is a blocker.

Fix in src/fides/service/pbac/consumers/repository.py:

def __init__(
    self,
    cache: FidesopsRedis,
    purpose_repo: DataPurposeRedisRepository,
    ttl_seconds: Optional[int] = None,
) -> None:
    super().__init__(cache, ttl_seconds=ttl_seconds)
    self._purpose_repo = purpose_repo

---

Inline findings

See individual comments for:

• Unused Params import in the test file (will fail linting)
• Sliding vs. fixed TTL behavior on shared set keys — undocumented, may be unintentional
• Missing input validation for ttl_seconds <= 0
• TTL magic constant in tests duplicating config default
• test_defaults_to_config_ttl verifies constructor assignment but not save-path behavior

---

Notes

• The pipe.set(..., ex=self._ttl_seconds) on the entity key correctly gives a fresh full TTL on every update — this is the right behavior, but a brief comment ("updates renew TTL") would help future readers.
• The pipe.execute() call is correctly placed after all pipeline commands; no atomicity concerns beyond the existing non-transactional pipeline design.

🔬 Codegraph: unavailable

---

💡 Write /code-review in a comment to re-run this review.

[claude]

src/fides/service/pbac/redis_repository.py:34

_ttl_seconds is evaluated once at construction time from CONFIG. If ttl_seconds=0 or a negative value is passed in, Redis will interpret EXPIRE key 0 as an immediate delete, silently discarding every entity immediately after writing. Consider adding a guard:

if ttl_seconds is not None and ttl_seconds <= 0:
    raise ValueError(f"ttl_seconds must be positive, got {ttl_seconds}")
self._ttl_seconds = ttl_seconds if ttl_seconds is not None else CONFIG.redis.default_ttl_seconds

[claude]

src/fides/service/pbac/redis_repository.py:85

The _all set (and index sets below) receive a sliding TTL — every save() on any entity of this type resets the expiry clock to now + TTL. In a system with frequent writes, these shared set keys can remain in Redis significantly longer than intended, partially defeating the purpose of the fix.

This may be intentional (sliding TTL for active collections is a reasonable design), but it's meaningfully different from the absolute TTL that the entity key gets. Worth a comment to make the intent explicit:

# Shared set keys get a sliding TTL — any write to the collection
# extends expiry, so active collections stay alive.
pipe.expire(self._all_key(), self._ttl_seconds)

[claude]

tests/service/pbac/test_redis_repository.py:9

Params is imported but never used in this file. This will be flagged by flake8/isort in CI. Please remove it.

[claude]

tests/service/pbac/test_redis_repository.py:13

This constant duplicates the config default value. If the default ever changes, this test silently breaks. Consider asserting against repo._ttl_seconds instead of the raw integer, or referencing the config value directly:

# Instead of:
assert repo._ttl_seconds == 86400

# Prefer:
assert repo._ttl_seconds == CONFIG.redis.default_ttl_seconds

[claude]

tests/service/pbac/test_redis_repository.py:149

test_defaults_to_config_ttl correctly verifies _ttl_seconds is set from CONFIG, but there is no test asserting that a save() call on a repo constructed without an explicit TTL actually uses CONFIG.redis.default_ttl_seconds in the pipe.set(..., ex=...) call. A future refactor that read _ttl_seconds lazily (or from a different path) could break the actual Redis behavior while this test still passes. Consider adding a corresponding save-path assertion.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.14%. Comparing base (ec18f3c) to head (e4b95ab).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7884      +/-   ##
==========================================
+ Coverage   85.06%   85.14%   +0.08%     
==========================================
  Files         629      629              
  Lines       40855    40859       +4     
  Branches     4748     4748              
==========================================
+ Hits        34753    34790      +37     
+ Misses       5029     4997      -32     
+ Partials     1073     1072       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[erosselli]

I'm a bit confused , don't we have this same code in fidesplus? it seems to me src/fidesplus/service/redis_repository.py and src/fides/service/pbac/redis_repository.py should share some of this code , maybe the base service should be in Fides instead of Fidesplus?