Skip to content

Fix async-execution cache keys leaking with no TTL#7631

Merged
Linker44 merged 3 commits intomainfrom
fix/async-execution-key-ttl
Mar 16, 2026
Merged

Fix async-execution cache keys leaking with no TTL#7631
Linker44 merged 3 commits intomainfrom
fix/async-execution-key-ttl

Conversation

@Linker44
Copy link
Copy Markdown
Contributor

@Linker44 Linker44 commented Mar 11, 2026

Ticket [ENG-N/A]

Description Of Changes

cache_task_tracking_key() used cache.set() to store id-{request_id}-async-execution keys with no TTL, causing them to accumulate indefinitely — one per privacy request and request task queued. On a customer instance this resulted in 3.7M leaked keys, inflating the Redis keyspace and increasing the cost of every SCAN/KEYS operation.

Switched to cache.set_with_autoexpire() so these keys expire with the default 7-day TTL, matching the behavior of other cache keys (e.g. identity keys, encoded objects).

Code Changes

  • Changed cache.set() to cache.set_with_autoexpire() in cache_task_tracking_key() (src/fides/api/util/cache.py)

Steps to Confirm

  1. Queue a privacy request
  2. Check the TTL of the async-execution key: redis-cli TTL id-{request_id}-async-execution — should return ~604800 (7 days)
  3. Verify privacy request cancellation still works (reads the cached task ID via get_cached_task_id)
  4. Verify requeuing of in-progress privacy requests still works (reads the cached task ID to check if tasks are in-flight)

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:
    • No UX review needed
  • Followup issues:
    • No followup issues
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

`cache_task_tracking_key()` used `cache.set()` which stores
`id-{request_id}-async-execution` keys with no expiry. These keys
accumulate indefinitely — one per privacy request and request task
queued. On a customer instance this resulted in 3.7M leaked keys
inflating the Redis keyspace and increasing the cost of every
SCAN/KEYS operation.

Switch to `cache.set_with_autoexpire()` so these keys expire with
the default 7-day TTL, matching the behavior of other cache keys.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Linker44 Linker44 requested a review from a team as a code owner March 11, 2026 20:57
@Linker44 Linker44 requested review from thabofletcher and removed request for a team March 11, 2026 20:57
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Mar 11, 2026

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

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Mar 11, 2026 9:02pm
fides-privacy-center Ignored Ignored Mar 11, 2026 9:02pm

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR fixes a Redis key leak where cache_task_tracking_key() was storing id-{request_id}-async-execution keys via cache.set() with no TTL, causing them to accumulate indefinitely (manifesting as 3.7 M leaked keys on a customer instance). The fix is a one-line change to use cache.set_with_autoexpire(), which applies the configured default 7-day TTL — consistent with all other Fides cache keys (identity keys, encoded objects, etc.).

  • The change is minimal, targeted, and correct; set_with_autoexpire already handles the expire_time fallback internally.
  • Existing TestCacheTaskTrackingKey tests validate the stored value and will continue to pass unchanged.
  • No automated test asserting that the newly-stored key carries a positive TTL (e.g. cache.ttl(get_async_task_tracking_cache_key(...)) > 0) is included; the test plan is currently limited to manual redis-cli TTL verification. Adding a unit test would lock in this invariant and prevent accidental regression.

Confidence Score: 5/5

  • This PR is safe to merge; it is a one-line correctness fix with no behavioural side-effects beyond adding a TTL.
  • The change is minimal and well-understood: set_with_autoexpire is already used throughout the codebase for every other cache key, so this simply closes a gap. set_with_autoexpire handles the default TTL fallback internally, so the call-site change is complete. Existing tests still pass. The only gap is the absence of an automated TTL assertion test, which is a style/best-practice concern rather than a blocking issue.
  • No files require special attention.

Important Files Changed

Filename Overview
src/fides/api/util/cache.py Single-line fix swapping cache.set() for cache.set_with_autoexpire() in cache_task_tracking_key, ensuring async-execution keys now receive the default 7-day TTL instead of persisting indefinitely.

Last reviewed commit: 206a589

Linker44 and others added 2 commits March 11, 2026 18:01
Ensures a future refactor can't silently reintroduce the no-TTL
regression by verifying that cache_task_tracking_key stores keys
with a positive TTL.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Linker44 Linker44 added this pull request to the merge queue Mar 16, 2026
Merged via the queue into main with commit 105d324 Mar 16, 2026
56 of 57 checks passed
@Linker44 Linker44 deleted the fix/async-execution-key-ttl branch March 16, 2026 19:00
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