fix: Retry flags only on transport errors#707
Conversation
|
Reviews (1): Last reviewed commit: "fix: retry flags only on transport error..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| flag_request_options: Dict[str, Any] = {} | ||
| if ( | ||
| self.feature_flags_request_max_retries | ||
| != DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES | ||
| ): | ||
| flag_request_options["max_retries"] = self.feature_flags_request_max_retries | ||
|
|
||
| resp_data = flags( | ||
| self.api_key, | ||
| self.host, | ||
| timeout=self.feature_flags_request_timeout_seconds, | ||
| **flag_request_options, | ||
| **request_data, | ||
| ) |
There was a problem hiding this comment.
flag_request_options dict and its conditional guard are superfluous. flags() already defaults to DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES, so passing the stored value unconditionally produces identical behaviour for every configuration — but is simpler and more obviously correct. The extra dict and branch also make the default code path invisible to tests: patch_flags.call_args.kwargs["max_retries"] would raise KeyError when the default is in use, forcing the test to cover only the non-default path.
| flag_request_options: Dict[str, Any] = {} | |
| if ( | |
| self.feature_flags_request_max_retries | |
| != DEFAULT_FEATURE_FLAGS_REQUEST_MAX_RETRIES | |
| ): | |
| flag_request_options["max_retries"] = self.feature_flags_request_max_retries | |
| resp_data = flags( | |
| self.api_key, | |
| self.host, | |
| timeout=self.feature_flags_request_timeout_seconds, | |
| **flag_request_options, | |
| **request_data, | |
| ) | |
| resp_data = flags( | |
| self.api_key, | |
| self.host, | |
| timeout=self.feature_flags_request_timeout_seconds, | |
| max_retries=self.feature_flags_request_max_retries, | |
| **request_data, | |
| ) |
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!
| @mock.patch("posthog.client.flags") | ||
| def test_feature_flags_request_max_retries_is_forwarded_when_configured( | ||
| self, patch_flags | ||
| ): | ||
| patch_flags.return_value = {"featureFlags": {}, "featureFlagPayloads": {}} | ||
| client = Client( | ||
| FAKE_TEST_API_KEY, | ||
| feature_flags_request_max_retries=0, | ||
| personal_api_key=FAKE_TEST_API_KEY, | ||
| ) | ||
|
|
||
| client.get_all_flags("distinct_id") | ||
|
|
||
| self.assertEqual(patch_flags.call_args.kwargs["max_retries"], 0) |
There was a problem hiding this comment.
max_retries forwarding. The custom instructions prefer parameterised tests, and there are at least three distinct cases worth covering: the default value (1), zero (disable), and a non-default positive value (e.g. 3). A single case also doesn't verify that the default is forwarded at all, since the current conditional in client.py omits max_retries from the call when its value equals the default.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
|
Closing in favor of a fork-backed branch because repository rules require signed commits on origin feature branches and the follow-up public API snapshot update could not be pushed directly. Replacement PR coming next. |
posthog-python Compliance ReportDate: 2026-06-28 14:23:19 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
1 participant
💡 Motivation and Context
Align Python
/flags/?v=2retry behavior with the other SDK fixes: retry only bounded network/transport/timeout failures, and do not retry HTTP/API status responses like 408, 429, or 5xx.This keeps transient network handling while avoiding repeated requests when PostHog has already returned an API response.
💚 How did you test it?
uv run --extra test --extra dev ruff check posthog/request.py posthog/client.py posthog/__init__.py posthog/test/test_request.py posthog/test/test_client.pyuv run --extra test pytest posthog/test/test_request.py posthog/test/test_client.py📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with pi. The retry policy was changed from urllib3 status-code retries to explicit
/flagstransport retries, with default max retries set to 1,0disabling retries, and exponential backoff starting at 300ms.