feat(prompts): hold a refetch cooldown after a failed prompt fetch - #968
Merged
Merged
Conversation
Contributor
 The PR is not safe to merge until cooldown handling preserves zero-TTL refetch semantics and honors HTTP-date Retry-After values, and the explicit test requirements are satisfied. Reviews (1) · Last reviewed commit: "feat(prompts): hold a refetch cooldown a..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+558
to
+566
| if cached.retry_not_before is not None and now < cached.retry_not_before: | ||
| return PromptResult( | ||
| source="stale_cache", | ||
| prompt=cached.prompt, | ||
| name=cached.name, | ||
| version=cached.version, | ||
| label=cached.label, | ||
| config=copy.deepcopy(cached.config), | ||
| ) |
Contributor
There was a problem hiding this comment.
A caller using cache_ttl_seconds=0 explicitly requests a refetch on every read, but this cooldown branch returns stale data without making a network request after a failure, potentially for up to an hour. Skip the cooldown when caching is disabled so the established zero-TTL behavior remains intact.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/ai/prompts.py
Line: 558-566
Comment:
**Cooldown overrides zero TTL**
A caller using `cache_ttl_seconds=0` explicitly requests a refetch on every read, but this cooldown branch returns stale data without making a network request after a failure, potentially for up to an hour. Skip the cooldown when caching is disabled so the established zero-TTL behavior remains intact.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+166
to
+169
| try: | ||
| seconds = float(value.strip()) | ||
| except ValueError: | ||
| return None |
Contributor
There was a problem hiding this comment.
Retry-After permits an HTTP-date, but this parser rejects that form and silently applies the 60-second default. The client can therefore retry before the server's stated deadline. Parse both delta-seconds and HTTP-date, as the repository's existing retry parser already does.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/ai/prompts.py
Line: 166-169
Comment:
**HTTP-date cooldowns are ignored**
`Retry-After` permits an HTTP-date, but this parser rejects that form and silently applies the 60-second default. The client can therefore retry before the server's stated deadline. Parse both delta-seconds and HTTP-date, as the repository's existing retry parser already does.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+330
to
+366
| def test_hold_a_cooldown_after_a_failed_refetch_then_retry( | ||
| self, mock_time, mock_get_session | ||
| ): | ||
| # Without the cooldown, one throttled client turns every later get() | ||
| # into another network request until one succeeds. | ||
| mock_get = mock_get_session.return_value.get | ||
| mock_get.side_effect = [ | ||
| MockResponse(json_data=self.mock_prompt_response), | ||
| MockResponse(status_code=500, ok=False), | ||
| MockResponse(json_data=self.mock_prompt_response), | ||
| ] | ||
| mock_time.return_value = 1000.0 | ||
|
|
||
| prompts = Prompts(self.create_mock_posthog()) | ||
| prompts.get("test-prompt", cache_ttl_seconds=300, with_metadata=False) | ||
|
|
||
| # Past TTL: the refetch fails, stale cache is served, cooldown starts. | ||
| mock_time.return_value = 1400.0 | ||
| result = prompts.get("test-prompt", cache_ttl_seconds=300, with_metadata=True) | ||
| self.assertEqual(result.source, "stale_cache") | ||
| self.assertEqual(mock_get.call_count, 2) | ||
|
|
||
| # Within the 60s cooldown: stale cache again, no network attempt. | ||
| mock_time.return_value = 1430.0 | ||
| result = prompts.get("test-prompt", cache_ttl_seconds=300, with_metadata=True) | ||
| self.assertEqual(result.source, "stale_cache") | ||
| self.assertEqual(mock_get.call_count, 2) | ||
|
|
||
| # Past the cooldown: the network is retried and the cache refreshed. | ||
| mock_time.return_value = 1470.0 | ||
| result = prompts.get("test-prompt", cache_ttl_seconds=300, with_metadata=True) | ||
| self.assertEqual(result.source, "api") | ||
| self.assertEqual(mock_get.call_count, 3) | ||
|
|
||
| @patch("posthog.ai.prompts._get_session") | ||
| @patch("posthog.ai.prompts.time.time") | ||
| def test_hold_the_cooldown_for_the_retry_after_a_429_sends( |
Contributor
There was a problem hiding this comment.
These adjacent tests repeat the same setup and cooldown lifecycle, varying only the response and duration. The repository directive says to prefer parameterized tests over near-duplicate test functions, so these cases must be parameterized before merging.
Context Used: Be direct and concise: state the issue, its impact, and the fix, with no preamble or praise. Do not comment on alphabetical sorting, trailing commas, or formatting. Linters catch these. Judge code by four simplicity rules: it passes all the tests, ex... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/ai/test_prompts.py
Line: 330-366
Comment:
**Cooldown tests duplicate scenarios**
These adjacent tests repeat the same setup and cooldown lifecycle, varying only the response and duration. The repository directive says to prefer parameterized tests over near-duplicate test functions, so these cases must be parameterized before merging.
**Context Used:** Be direct and concise: state the issue, its impact, and the fix, with no preamble or praise. Do not comment on alphabetical sorting, trailing commas, or formatting. Linters catch these. Judge code by four simplicity rules: it passes all the tests, ex... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+336
to
+362
| mock_get.side_effect = [ | ||
| MockResponse(json_data=self.mock_prompt_response), | ||
| MockResponse(status_code=500, ok=False), | ||
| MockResponse(json_data=self.mock_prompt_response), | ||
| ] | ||
| mock_time.return_value = 1000.0 | ||
|
|
||
| prompts = Prompts(self.create_mock_posthog()) | ||
| prompts.get("test-prompt", cache_ttl_seconds=300, with_metadata=False) | ||
|
|
||
| # Past TTL: the refetch fails, stale cache is served, cooldown starts. | ||
| mock_time.return_value = 1400.0 | ||
| result = prompts.get("test-prompt", cache_ttl_seconds=300, with_metadata=True) | ||
| self.assertEqual(result.source, "stale_cache") | ||
| self.assertEqual(mock_get.call_count, 2) | ||
|
|
||
| # Within the 60s cooldown: stale cache again, no network attempt. | ||
| mock_time.return_value = 1430.0 | ||
| result = prompts.get("test-prompt", cache_ttl_seconds=300, with_metadata=True) | ||
| self.assertEqual(result.source, "stale_cache") | ||
| self.assertEqual(mock_get.call_count, 2) | ||
|
|
||
| # Past the cooldown: the network is retried and the cache refreshed. | ||
| mock_time.return_value = 1470.0 | ||
| result = prompts.get("test-prompt", cache_ttl_seconds=300, with_metadata=True) | ||
| self.assertEqual(result.source, "api") | ||
| self.assertEqual(mock_get.call_count, 3) |
Contributor
There was a problem hiding this comment.
The tests cover only one failed fetch followed by success, so they do not verify that another failure starts a new cooldown and later permits another retry. The repository requires retry logic to test the complete flow with multiple retry attempts; add a second failed retry and a subsequent cooldown and success cycle before merging.
Rule Used: When implementing retry logic, include comprehensive tests that verify the complete retry flow with multiple retry attempts, similar to existing patterns in the codebase. (source)
Learned From
PostHog/posthog#32651
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/ai/test_prompts.py
Line: 336-362
Comment:
**Retry flow coverage is incomplete**
The tests cover only one failed fetch followed by success, so they do not verify that another failure starts a new cooldown and later permits another retry. The repository requires retry logic to test the complete flow with multiple retry attempts; add a second failed retry and a subsequent cooldown and success cycle before merging.
**Rule Used:** When implementing retry logic, include comprehensive tests that verify the complete retry flow with multiple retry attempts, similar to existing patterns in the codebase. ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=24d1be3f-07fb-465d-b1a7-60dff301aed8))
**Learned From**
[PostHog/posthog#32651](https://github.com/PostHog/posthog/pull/32651)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
posthog-python Compliance ReportDate: 2026-09-17 16:04:18 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
marandaneto
approved these changes
Sep 18, 2026
marandaneto
left a comment
marandaneto
left a comment
Member
There was a problem hiding this comment.
The code looks good; no qualifying findings. Verdict: correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After a failed prompt refetch the SDK serves the stale cached prompt but sets no cooldown, so once the TTL expires every
prompts.get()call retries the network until one succeeds. A rate-limited client holds itself against the limit. The JavaScript SDK already has a cooldown for this; this ports it.Changes
source="stale_cache").Retry-Afterheader overrides the default, capped at one hour, carried by a newPromptFetchErrorraised on HTTP failures in both fetch paths.PromptFetchErroris new public API).How did you test this code?
Two tests added: the cooldown lifecycle (fail, serve stale with no network call, retry after expiry; without the cooldown the middle call would hit the network) and the 429
Retry-Afteroverride. All 80 prompt tests pass locally.