Skip to content

fix: honor updated TTL when overwriting feature cache entries - #145

Open
fallintoplace wants to merge 2 commits into
growthbook:mainfrom
fallintoplace:fix/cache-overwrite-ttl
Open

fallintoplace wants to merge 2 commits into
growthbook:mainfrom
fallintoplace:fix/cache-overwrite-ttl

Conversation

@fallintoplace

Copy link
Copy Markdown

What

  • Honor the new TTL when overwriting a feature cache entry.

Why

  • The old TTL was reused. Shorter TTLs kept stale features, and longer TTLs expired fresh features too soon.

Implementation

  • Set the entry TTL before updating its value and expiry.
  • Keep method signatures unchanged. Add regression cases for changed, zero and unchanged TTLs, expired entries, and separate keys.

@greptile-apps

greptile-apps Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Fixes feature cache TTL refresh on overwrite.

The PR should not merge until concurrent refreshes preserve the TTL belonging to each cached value.

Reviews (1) · Last reviewed commit: "fix: honor updated TTL when overwriting ..."

Comment thread growthbook/growthbook.py Outdated

def set(self, key: str, value: Dict[str, Any], ttl: int) -> None:
if key in self.cache:
self.cache[key].ttl = ttl

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Concurrent refreshes mix TTLs

When two clients refresh the same cache key at the same time with different TTLs, one can change entry.ttl before the other calls update(). For example, a refresh requesting a five-second TTL can store its value with a 600-second expiry instead. Those features may remain cached long after the requested five seconds. Keep each refreshed value paired with its own TTL and expiry.

Knowledge Base Used: Client lifecycle and feature refresh

Prompt To Fix With AI
This is a comment left during a code review.
Path: growthbook/growthbook.py
Line: 120

Comment:
**Concurrent refreshes mix TTLs**

When two clients refresh the same cache key at the same time with different TTLs, one can change `entry.ttl` before the other calls `update()`. For example, a refresh requesting a five-second TTL can store its value with a 600-second expiry instead. Those features may remain cached long after the requested five seconds. Keep each refreshed value paired with its own TTL and expiry.

**Knowledge Base Used:** [Client lifecycle and feature refresh](https://app.greptile.com/growthbook/-/custom-context/knowledge-base/growthbook/growthbook-python/-/docs/client-lifecycle-and-feature-refresh.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@vazarkevych

Copy link
Copy Markdown
Collaborator

Heads-up: this overlaps with #114, which adds a threading.Lock around
InMemoryFeatureCache.get/set/clear. That lock makes the two-step
entry.ttl = ttl; entry.update(value) atomic, so it resolves the P1 greptile
raised here — but only if both land, and whichever merges second will conflict
in set().

An alternative that fixes it independently of #114, and keeps the signature
backwards compatible:

def update(self, value: Dict[str, Any], ttl: Optional[int] = None) -> None:
    if ttl is not None:
        self.ttl = ttl
    self.value = value
    self.expires = time() + self.ttl

Then set() stays a single call and each refreshed value is paired with its own
TTL by construction.

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