Repository navigation
fix(flags): keep the replay relink alive under activity-signal muting - #86720
Conversation
|
😎 Merged successfully - details. |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/feature_flags/backend/session_recording_links.py |
94.7% | 35 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 32892881832 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
warehouse_sources_queue |
██████░░░░░░░░░░░░░░ 29.4% |
91 / 309 |
demo |
███████████░░░░░░░░░ 56.9% |
1,516 / 2,666 |
data_tools |
█████████████░░░░░░░ 66.2% |
90 / 136 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
batch_exports |
████████████████░░░░ 79.8% |
19,261 / 24,143 |
apm |
█████████████████░░░ 83.3% |
1,234 / 1,481 |
signals |
█████████████████░░░ 83.5% |
29,804 / 35,674 |
cdp |
█████████████████░░░ 85.1% |
4,146 / 4,874 |
actions |
█████████████████░░░ 86.6% |
716 / 827 |
wizard |
█████████████████░░░ 86.7% |
1,090 / 1,257 |
data_modeling |
█████████████████░░░ 86.9% |
9,644 / 11,092 |
notebooks |
█████████████████░░░ 87.3% |
8,736 / 10,003 |
cohorts |
██████████████████░░ 87.9% |
6,768 / 7,704 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
data_warehouse |
██████████████████░░ 89.1% |
12,630 / 14,181 |
business_knowledge |
██████████████████░░ 89.1% |
4,431 / 4,975 |
dashboards |
██████████████████░░ 89.1% |
6,386 / 7,165 |
exports |
██████████████████░░ 89.3% |
7,887 / 8,830 |
engineering_analytics |
██████████████████░░ 89.7% |
7,009 / 7,815 |
visual_review |
██████████████████░░ 89.8% |
6,057 / 6,745 |
tasks |
██████████████████░░ 89.9% |
59,101 / 65,717 |
alerts |
██████████████████░░ 90.4% |
4,707 / 5,209 |
mcp_analytics |
██████████████████░░ 90.5% |
4,000 / 4,418 |
links |
██████████████████░░ 90.6% |
183 / 202 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
managed_warehouse |
██████████████████░░ 90.7% |
8,555 / 9,427 |
conversations |
██████████████████░░ 90.9% |
20,495 / 22,558 |
error_tracking |
██████████████████░░ 91.5% |
12,246 / 13,380 |
canvas |
██████████████████░░ 92.5% |
4,512 / 4,880 |
early_access_features |
███████████████████░ 92.5% |
1,323 / 1,430 |
managed_migrations |
███████████████████░ 92.6% |
1,556 / 1,681 |
slack_app |
███████████████████░ 92.6% |
12,123 / 13,096 |
stamphog |
███████████████████░ 92.6% |
5,510 / 5,948 |
context_layer |
███████████████████░ 92.7% |
2,285 / 2,464 |
notifications |
███████████████████░ 92.8% |
1,074 / 1,157 |
web_analytics |
███████████████████░ 93.0% |
17,575 / 18,908 |
mcp_store |
███████████████████░ 93.2% |
7,438 / 7,984 |
surveys |
███████████████████░ 93.3% |
5,942 / 6,372 |
marketing_analytics |
███████████████████░ 93.4% |
16,881 / 18,081 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
posthog_ai |
███████████████████░ 93.5% |
1,371 / 1,467 |
ai_observability |
███████████████████░ 93.6% |
17,489 / 18,684 |
product_analytics |
███████████████████░ 93.7% |
7,369 / 7,865 |
approvals |
███████████████████░ 93.7% |
3,485 / 3,718 |
billing_alerts |
███████████████████░ 93.9% |
2,047 / 2,181 |
legal_documents |
███████████████████░ 94.0% |
1,914 / 2,037 |
workflows |
███████████████████░ 94.3% |
9,781 / 10,374 |
endpoints |
███████████████████░ 94.3% |
8,813 / 9,346 |
review_hog |
███████████████████░ 94.3% |
9,975 / 10,576 |
tracing |
███████████████████░ 94.4% |
2,678 / 2,837 |
annotations |
███████████████████░ 95.1% |
734 / 772 |
customer_analytics |
███████████████████░ 95.2% |
16,556 / 17,398 |
skills |
███████████████████░ 95.3% |
4,794 / 5,031 |
experiments |
███████████████████░ 95.5% |
29,198 / 30,563 |
messaging |
███████████████████░ 95.8% |
3,586 / 3,745 |
logs |
███████████████████░ 95.8% |
13,255 / 13,836 |
replay_vision |
███████████████████░ 95.8% |
22,600 / 23,585 |
feature_flags |
███████████████████░ 96.1% |
19,239 / 20,018 |
access_control |
███████████████████░ 96.2% |
5,078 / 5,280 |
revenue_analytics |
███████████████████░ 96.3% |
1,841 / 1,911 |
user_interviews |
███████████████████░ 96.5% |
2,639 / 2,735 |
data_quality |
███████████████████░ 96.5% |
2,692 / 2,789 |
growth |
███████████████████░ 96.6% |
7,639 / 7,909 |
warehouse_sources |
███████████████████░ 97.2% |
400,006 / 411,637 |
analytics_platform |
████████████████████ 98.1% |
2,286 / 2,330 |
data_catalog |
████████████████████ 98.2% |
3,203 / 3,262 |
metrics |
████████████████████ 98.3% |
3,256 / 3,312 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). Resolved comments: 1 declined, 1 left for you |
There was a problem hiding this comment.
Pull request overview
This PR hardens session replay’s “linked feature flag” behavior so renaming (or tombstoning) a flag can’t silently disable replay by leaving Team.session_recording_linked_flag["key"] stale, even when activity logging signals are muted.
Changes:
- Move the replay relink trigger from
model_activity_signalto plainpre_save/post_savereceivers using a shared “field snapshot” helper to detect key changes. - Ensure tombstone key renames also relink teams, and ensure one team update failure doesn’t prevent relinking others.
- Add regression tests and regenerate receiver/query snapshot baselines to match the new signal wiring.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| products/feature_flags/backend/session_recording_links.py | Introduces pre_save/post_save relink path, adds linked_flag_id, and makes relinking resilient to per-team write failures. |
| products/feature_flags/backend/field_snapshots.py | New shared utilities for pre-save snapshots + post-save change detection. |
| products/feature_flags/backend/flag_version_sync.py | Switches definition-change detection to use the new shared snapshot utilities. |
| products/feature_flags/backend/api/test/test_feature_flag.py | Adds API-level regression tests covering muted-signal rename and tombstone-key freeing behavior. |
| products/feature_flags/backend/test/test_session_recording_links.py | Adds test for “one team write failure doesn’t strand siblings” (but currently has an import-order lint issue). |
| products/feature_flags/backend/management/commands/repair_replay_linked_flag_keys.py | Reuses linked_flag_id, adopts @frozen, and validates chunk size > 0. |
| products/feature_flags/backend/management/commands/test/test_repair_replay_linked_flag_keys.py | Adds coverage for multi-page chunk iteration and zero chunk-size rejection. |
| posthog/test/repo_invariants/setup_receivers_baseline.txt | Updates the receiver baseline to reflect the new pre/post-save receivers. |
| posthog/tasks/test/snapshots/test_process_scheduled_changes.ambr | Updates the scheduled-changes query snapshot for the new receiver set / queries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
4e95989 to
7e9bab8
Compare
|
Reviews (1): Last reviewed commit: "fix(flags): keep the replay relink alive..." | Re-trigger Greptile |
patricio-posthog
left a comment
There was a problem hiding this comment.
Approving. I think these race conditions are still possible so we may want to revise them in a follow-up, though we may just be OK with them:
- A team can begin linking to a tombstone between the link-existence check and hard deletion.
- A team can write a stale
{id, key}pair just after a relink scan has passed it. The current API validator does not verify or canonicalize the referenced flag. - Hard deletion through paths outside this serializer can still leave dangling replay links.
- QuerySet.update() bypasses model signals, so internal callers using it to change a flag key would bypass this protection.
The relink receiver rides plain pre_save/post_save instead of model_activity_signal, so mute_selected_signals() and activity-log signal_exclusions cannot silently disable it and stop linking teams from recording. A pre_save snapshot of the persisted key detects renames; relink_teams still re-reads the stored key on commit. field_snapshots holds the pre-save snapshot mechanism once, shared by this receiver and flag_version_sync's, each parameterized by its own attribute and field set so neither depends on what the other watches. Freeing a soft-deleted flag's key for reuse now renames the tombstone instead of hard-deleting it when a team still links it for replay, since a hard delete fires no save and would leave that team on the freed key. The existing key-reuse blocker check (an active experiment or early access feature still referencing the tombstone) runs on this path too, so reusing a blocked key still returns its "cannot reuse key" error rather than silently renaming a flag a dependent still resolves by key. The repair command shares the public linked_flag_id helper and uses @Frozen. Tests cover the muted rename, the tombstone relink on both the create and rename paths, per-team failure isolation, the repair command's chunk pagination, and the key-reuse blocker with a replay link attached. The receiver baseline and the scheduled-changes query snapshot are regenerated for the new receiver set and the added key SELECT. Generated-By: PostHog Desktop Task-Id: bbf891ce-405a-4670-8dc2-d5da318b17fe
55153f7 to
74c4748
Compare
…nce check
Introduce teams_linking_flag_in_project and use it from _free_key_held_by_soft_deleted_flags
where self.context["project_id"] is already in hand, removing the select_related("team")
workaround. Fold the one-use _KEY_FIELD constant and key_changed local into their call sites
to match the sibling idiom in flag_version_sync.
Generated-By: PostHog Desktop
Task-Id: 18221516-e2d3-4c7f-bd22-176e078a1151
Problem
Renaming a feature flag that gates session replay can silently turn replay off for every team linked to it.
Team.session_recording_linked_flagstores the flag key, and the SDK payload is built from that key alone. The browser and React Native SDKs both treat a linked flag they cannot resolve as "do not record", so a stale key stops recording without surfacing an error.model_activity_signal, which exists to feed activity logging.mute_selected_signals()silences every receiver on that signal. It sets a process-global flag rather than a thread-local one, so one request's mute window covers concurrent requests in the same process.signal_exclusionslist can suppress the same signal per model.Changes
pre_save/post_save, which neither mute mechanism gates.Mechanical, no behavior change:
field_snapshots.pyholds the pre-save snapshot helpers thatflag_version_sync.pykept privately, so both receivers detect changes the same way.snapshot_if_changedreplaceschanged_fields. It returns the whole snapshot rather than only the fields that moved, andget_changed_fields_localinposthog/models/activity_logging/utils.pyalready uses the other meaning.keySELECT.Nothing looks different. This is backend only.
Signal path
Before:
flowchart LR Save["FeatureFlag.save()"] --> Mixin["ModelActivityMixin.save()"] Mixin --> Sig["model_activity_signal"] Sig --> Gate{{"mute_selected_signals()<br/>signal_exclusions"}} Gate -->|"open"| Relink["relink_teams_on_key_change"] Gate -->|"muted"| Drop["relink skipped<br/>stored key goes stale"] Relink --> Teams["teams relinked"] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class Save phYellow; class Mixin,Sig phBlue; class Gate,Drop phRed; class Relink phBlue; class Teams phGray;After:
flowchart LR Save["FeatureFlag.save()"] --> Pre["pre_save<br/>capture_replay_link_key_before_save"] Pre --> Post["post_save<br/>relink_teams_on_key_change"] Post --> Teams["teams relinked"] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class Save phYellow; class Pre,Post phBlue; class Teams phGray;How did you test this code?
Automated tests only. Nothing was tested by hand, and nothing ran against a deployed environment.
Each new test and the regression it catches:
test_rename_still_relinks_teams_while_the_activity_signal_is_mutedcovers the bug itself. It fails on the old wiring and passes on the new.test_freeing_a_tombstoned_key_relinks_teams_to_the_tombstonecovers a team stranded on the freed key when its key is reused, over both the create and rename paths. Remove the new guard and it fails onFeatureFlag.DoesNotExist.test_reuse_key_with_inconsistent_soft_deleted_flag_referenced_by_active_experimentand its early-access-feature sibling now attach a replay link, so they cover the rename path. Without that link neither could catch a guard that skips the blocker check.test_one_teams_write_failure_does_not_strand_its_siblingscovers one unwritable team aborting the loop for the rest.test_repairs_every_team_across_chunk_boundariescovers the repair command dropping teams past the first page. No existing test passes a chunk size, so the multi-page path in_iter_team_chunkshad no coverage.test_rejects_a_zero_chunk_sizecovers a zero chunk size reporting a clean scan having looked at nothing.Two committed baselines fail CI without regeneration, and both are regenerated here:
setup_receivers_baseline.txtfor the changed receiver set, andtest_process_scheduled_changes.ambrfor the addedkeySELECT.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
None. No documented workflow, API, or setting changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written with Claude Code in PostHog Desktop. Skills invoked:
/review-code(multi-agent review with--fix, run twice),/simplify,/writing-tests,/writing-pr-descriptions,/explain-open,/address-pr-reviews,/followup,/squash.Three findings changed the code rather than the description. The first review caught two stale generated baselines; agents disagreed on whether the query snapshot was affected, so a validator settled it by running that module with and without the new receivers connected. ReviewHog then found that the tombstone case only held on the FK-blocked branch, which is where the delete-path guard comes from. Reviewing that guard in turn caught a regression in it: it renamed and returned before the key-reuse blocker check, so a linked tombstone with an active dependent would have been renamed silently instead of refused. Each fix was confirmed by disabling it and watching the new test go red.
One proposal was rejected: folding the inline id check in
update_linked_flag_keyintolinked_flag_id.teams_linking_flagmatches a stored float id through jsonb and the helper rejects floats, so unifying them would stop relinking those teams. A comment at the loose check records that.Two gaps found during review are deliberately left open, both pre-existing on master: no
post_deletereceiver clearsTeam.session_recording_linked_flag, so hard deletes outside this serializer still dangle; andvalidate_session_recording_linked_flagnever checks that the flag it stores exists or is live.No customer conversation, ticket, or log fed this work. Everything came from the repository, and the test fixtures are invented.
Created with PostHog Desktop