fix(tracing): serialize retention updates - #108170
pauldambra wants to merge 2 commits into
Conversation
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review 🦔 PostHog Review reviewed this pull requestNothing worth raising this time. Enjoy the moment:
|
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
posthog/temporal/ai_observability/run_aggregate_evaluation.py:692 |
posthog/temporal/ai_observability/run_trace_evaluation.py:928 |
38 | 203 |
products/warehouse_sources/backend/temporal/data_imports/sources/companycam/source.py:1 |
products/warehouse_sources/backend/temporal/data_imports/sources/wix/source.py:1 |
21 | 164 |
posthog/temporal/ai_observability/run_aggregate_evaluation.py:584 |
posthog/temporal/ai_observability/run_trace_evaluation.py:855 |
24 | 135 |
products/signals/backend/scout_harness/tools/report.py:1475 |
products/signals/backend/scout_harness/tools/report.py:1647 |
21 | 133 |
products/signals/backend/scout_harness/tools/report.py:1604 |
products/signals/backend/scout_harness/tools/report.py:1754 |
30 | 118 |
posthog/temporal/ai_observability/run_session_evaluation.py:466 |
posthog/temporal/ai_observability/run_trace_evaluation.py:664 |
24 | 115 |
posthog/management/commands/backfill_hogflow_billable_action_types.py:49 |
posthog/management/commands/refresh_hog_flows.py:69 |
23 | 112 |
products/batch_exports/backend/api/batch_export.py:2092 |
products/batch_exports/backend/api/batch_export.py:2131 |
24 | 112 |
products/warehouse_sources/backend/temporal/data_imports/sources/pardot/pardot.py:2 |
products/warehouse_sources/backend/temporal/data_imports/sources/wix/wix.py:1 |
12 | 97 |
products/signals/backend/scout_harness/tools/report.py:1516 |
products/signals/backend/scout_harness/tools/report.py:1675 |
12 | 71 |
products/signals/backend/scout_harness/tools/report.py:1531 |
products/signals/backend/scout_harness/tools/report.py:1690 |
16 | 70 |
⚠️ Duplication (TypeScript) — 2 new duplicated blocks (worst 158 tokens)
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/business_knowledge/frontend/scenes/settings/businessKnowledgeSettingsLogic.test.ts:10 |
products/data_catalog/frontend/certificationsLogic.test.ts:17 |
23 | 158 |
products/business_knowledge/frontend/scenes/settings/businessKnowledgeSettingsLogic.test.ts:10 |
products/data_quality/frontend/dataQualityGateLogic.test.ts:14 |
23 | 158 |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe PATCH handler locks the current tracing config row inside a transaction. For retention changes, it checks throttling against the locked row and rejects stale configuration with a validation error. Other PATCH fields also save against the locked row. Tests cover stale and refreshed configurations and check the savepoint depth observed by the feature-flag callback. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The handler currently serializes these updates, but a lock regression could go undetected by the sequential tests. This is a bounded test-coverage risk, not an observed current failure. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
posthog/api/test/test_team_tracing_config.py (1)
272-273: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd a separate-transaction concurrency regression test.
test_second_change_within_24_hours_is_refusedsends both PATCH requests sequentially. Thestale_configcase checks the locked-row recheck, but it would still pass ifselect_for_update()were removed while that recheck remained. Add aTransactionTestCasetest with two database connections. Hold the first update before commit, then assert that the second update waits for the row lock and rejects the retention change after the first transaction commits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 6ac46be6-c028-4398-b5db-39dfa7ab35bf
📒 Files selected for processing (2)
posthog/api/team.pyposthog/api/test/test_team_tracing_config.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review.
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| with transaction.atomic(): | ||
| locked_config = TeamTracingConfig.objects.select_for_update().get(team=team) | ||
| retention = serializer.validated_data.get("retention_days") | ||
| if retention is not None and retention != locked_config.retention_days: | ||
| throttle_error = retention_update_throttle_error(locked_config.retention_last_updated) |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Throttle validation still runs before the row lock
Issue description
serializer.is_valid() runs before this lock, and validate_retention_days() checks the throttle using the earlier config read. If validation runs just before the 24-hour limit expires, it can reject the request even if the limit has expired by the time the locked config is read. The fresh check inside the transaction cannot correct that rejection.
Why we think it's a valid issue
- Checked: Traced
handle_tracing_configfrom serializer validation through the locked config check, and reviewedretention_update_throttle_error. - Found:
TeamTracingConfigSerializer.validate_retention_dayschecks the throttle atposthog/api/team.py:326-330.handle_tracing_configcallsserializer.is_valid()atposthog/api/team.py:352, before acquiring the lock and checking the locked timestamp atposthog/api/team.py:353-359. - Found:
retention_update_throttle_errorcompares the current time with the timestamp atposthog/models/team/logs_retention.py:63-69. If validation rejects the request, execution never reaches the fresh check. The handler’sretention is not None and retention != locked_config.retention_daysguard atposthog/api/team.py:356already limits the locked check to changed values. - Impact: A request validated just before the 24-hour boundary can be rejected even though the throttle has expired by the locked check. This is a narrow but reachable correctness issue, so keeping the finding at
consideris appropriate.
Suggested fix
Keep flag and entitlement checks in serializer validation, but perform the throttle check only against locked_config inside the transaction. Preserve the unchanged-retention behavior so unrelated updates are not throttled.
Prompt to fix with AI (copy-paste)
## Context
@posthog/api/team.py#L353-357
<issue_description>
`serializer.is_valid()` runs before this lock, and `validate_retention_days()` checks the throttle using the earlier config read. If validation runs just before the 24-hour limit expires, it can reject the request even if the limit has expired by the time the locked config is read. The fresh check inside the transaction cannot correct that rejection.
</issue_description>
<issue_validation>
- **Checked:** Traced `handle_tracing_config` from serializer validation through the locked config check, and reviewed `retention_update_throttle_error`.
- **Found:** `TeamTracingConfigSerializer.validate_retention_days` checks the throttle at `posthog/api/team.py:326-330`. `handle_tracing_config` calls `serializer.is_valid()` at `posthog/api/team.py:352`, before acquiring the lock and checking the locked timestamp at `posthog/api/team.py:353-359`.
- **Found:** `retention_update_throttle_error` compares the current time with the timestamp at `posthog/models/team/logs_retention.py:63-69`. If validation rejects the request, execution never reaches the fresh check. The handler’s `retention is not None and retention != locked_config.retention_days` guard at `posthog/api/team.py:356` already limits the locked check to changed values.
- **Impact:** A request validated just before the 24-hour boundary can be rejected even though the throttle has expired by the locked check. This is a narrow but reachable correctness issue, so keeping the finding at `consider` is appropriate.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Keep flag and entitlement checks in serializer validation, but perform the throttle check only against `locked_config` inside the transaction. Preserve the unchanged-retention behavior so unrelated updates are not throttled.
</potential_solution>
4c3bd30 to
688f743
Compare
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
688f743 to
59f465a
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This branch has not been deployed
1 participant
[Robot] Prepared by PostHog Desktop.
Problem
Concurrent tracing retention updates can pass the same update limit check.
Changes
Lock and re-read the config before checking the update limit and saving. Run flag checks before the transaction.
This fixes existing behavior before the module split in #99248.
How did you test this code?
API tests cover stale config reads, unchanged settings, and flag checks outside the update transaction. The focused tests ran against both module layouts. The full type check covers the combined fixes.
Release status
Retention changes use the existing
tracing-settings-retentionflag. This PR does not change flag access.Automatic notifications
Docs update
No existing document under
docs/covers this validation detail.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Desktop / Codex, GPT-6
Tools: Git, GitHub CLI, hogli, pytest, Ruff, mypy, and CodeRabbit.
Skills: stacking-prs, improving-drf-endpoints, writing-tests, writing-code-comments, writing-user-facing-copy, setting-up-devbox, running-ci-preflight, reviewing-with-coderabbit, writing-pr-descriptions.
Each existing bug has its own layer below the refactor. No matching open fix appeared in the PR search.
The combined CodeRabbit review found two deployment tradeoffs in the membership layer. That PR records the decisions.
Created with PostHog Desktop