Skip to content

feat(flags): detect stale flag health issues - #95189

Merged
trunk-io[bot] merged 18 commits into
masterfrom
feat/flags-stale-health-check
Sep 10, 2026
Merged

trunk-io[bot] merged 18 commits into
masterfrom
feat/flags-stale-health-check

Conversation

@JamesPatrickGill

@JamesPatrickGill JamesPatrickGill commented Sep 4, 2026 •

Copy link
Copy Markdown
Member

Problem

Teams accumulate feature flags that finished their job, and nothing tells them which flags are safe to clean up. The evidence sits in usage data, rollout shape, and five linked systems. Each flag is a manual investigation.

The feature-flags scout needs a durable evidence trail to author cleanup reports from. Nothing produces one today.

Changes

  • A weekly health check files one INFO health issue per flag that looks finished: not evaluated in 30 days, or old with a configuration that serves a fixed result.
  • Nothing is user-visible yet. The check ships with dry_run = True, so runs write no rows and send no alerts. Note that dry_run gates the writes and not the detection queries, so rollout_percentage = 0.01 also samples teams until one full batch has a measured cost. Flipping both is a later operational step.
  • The check skips a flag when a known blocker still references it. Blockers: survey and product tour internal flags, experiments, early access features, replay links, and dependent flags. This set covers every blocker the bulk-delete guard checks, so the check never reports a flag the API refuses to delete.
  • A disabled dependent flag still protects its dependency. It ships in the local-evaluation payload and can be re-enabled, so its references stay live.
  • Issue identity is the flag id, so weekly runs update the same row in place as evidence moves. The issue resolves when the flag gets evaluated again.
  • The Refresh button on the Health page now applies each check's dry_run. The Celery task behind it defaulted dry_run to False. This is the first check that sets it, so it is the first one affected.
  • Mechanical: the rollout-state mapping moved to flag_status.rollout_state_and_variant and the bulk-delete response now reads it (response unchanged). direct_flag_dependency_ids and the flag-dependency queryset became shared helpers in flag_version_sync. Survey.get_internal_flag_ids and replay_linked_flag_ids gained batch forms for the set-wise exclusion queries.
flowchart LR
  Cron{{Temporal schedule<br>Mon 06:00 UTC}} --> Batch[250-team batches]
  Refresh{{Health page Refresh}} --> Task[Celery task]
  Batch --> Detect[stale candidates minus referenced flags]
  Task --> Detect
  Detect --> Gate{dry_run?}
  Gate -->|today| Log[(log counts)]
  Gate -->|later| Rows[(HealthIssue rows + alert events)]
  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 Cron,Refresh phYellow;
  class Batch,Task,Detect,Gate phBlue;
  class Log,Rows phGray;
Loading

Follow-ups, before dry_run flips: chunk the flag-id lists in the exclusion queries. Django binds client-side here, so a long list does not raise an error. It builds one very large statement to interpolate and parse. Sample teams before the first full run. rollout_percentage now caps this check at 1 percent, because no full batch of filter_stale_flags has a measured cost. Later, if dry-run volumes warrant it: stream _depended_on_flag_ids rather than build the whole list, and move the survey and product tour exclusion pair (now in five places) into one helper as its own PR.

How did you test this code?

New suite products/feature_flags/backend/temporal/health_checks/tests/test_stale_flags.py. Each group guards a distinct regression:

  • The inclusion and exclusion matrix fails when any blocker query loses its predicate, including archived product tours and disabled dependent flags.
  • The query-count test fails when a per-candidate or per-team query appears in detect.
  • The lifecycle test fails when issue identity stops being the flag id, or resolution stops working.
  • The truncation tests fail when either cap on customer-supplied text is deleted.
  • The Celery test fails when the Health page Refresh path stops applying dry_run.

Also ran locally after the shared-helper consolidation: test_flag_version_sync.py, test_flag_status.py, and TestFeatureFlagBulkDelete (covers the rewired rollout-state response). Not run locally: the full backend matrix. CI covers it.

Automatic notifications

  • Publish to changelog?

Docs update

None. The check is internal and dark until dry_run flips.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

  • Built and reviewed with Claude Code. Skills invoked: /review-code --fix (seven reviewer agents plus an adversarial validator), /writing-tests, /writing-code-comments, /writing-pr-descriptions, /ste-writing.
  • The review found the Refresh-path dry_run bypass (blocking, fixed in this PR), shrank the batch policy from the 1000-team default to 250 teams with 2 in flight, and consolidated the rollout-state mapping.
  • The tests invent all fixture data. The diff and this description carry no session material from outside this repository.

Add a weekly stale_feature_flags health check that persists one INFO
HealthIssue per qualifying user-managed flag. dry_run is True, so the
check detects but writes nothing until the feature-flags scout can
consume the issues.

The detector starts from the shared filter_stale_flags selector. It
excludes survey and product-tour internal flags, remote-config flags,
experiment-linked flags, early-access-feature flags, flags that active
flags depend on, and replay-linked flags. Every exclusion is one
set-wise query, so the query count does not grow with candidate volume.

render_signal stays the framework default (None). The feature-flags
scout is the only stale-report author, so this kind must not enter
generic Signals grouping.

Also:
- Add JobOwners.TEAM_FEATURE_FLAGS for alert routing.
- Survey.get_internal_flag_ids accepts a team_ids batch.
- Add replay_linked_flag_ids_for_projects, the multi-project form of
  replay_linked_flag_ids.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@JamesPatrickGill JamesPatrickGill self-assigned this Sep 4, 2026
@trunk-io

trunk-io Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@posthog

posthog Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

🦔 PostHog Review reviewed this pull request

Found 0 must fix, 0 should fix, 1 consider.

Published 1 finding (view the review).

Resolved comments: 1 fixed, 1 left for you

@github-actions

github-actions Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Complexity (TypeScript) — clean

Cyclomatic complexity above the limit in changed typescript files (10 for production files, 15 for test files). Warn only: worth simplifying when you next touch these functions.

🚨 Comment density — 6% of added code lines are comments (45 of 708)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/feature_flags/backend/temporal/health_checks/stale_flags.py 20 222
products/feature_flags/backend/temporal/health_checks/tests/test_stale_flags.py 15 370
products/feature_flags/backend/flag_status.py 6 22
products/feature_flags/backend/api/feature_flag.py 2 7
frontend/src/scenes/health/healthCategories.tsx 1 23
products/feature_flags/backend/flag_version_sync.py 1 17

This check does not block merging. It updates on every push and clears when the share drops.

✅ Bundle size — no change

Uncompressed size of every built .js bundle, compared against the base branch.

Total: 68.63 MiB · no change

No file changed by more than 1000 B.

Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report

⚠️ Eager graph — 1 over budget

How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.

Root Eager (shipped) Δ vs base Budget
entry (logged-out pages, app bootstrap)
src/index.tsx
1.54 MiB · 22 files no change ███░░░░░░░ 34.1% of 4.51 MiB
🟡 logged-out boot: index + App + bootApp (preloaded by every page, including /login)
src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
6.66 MiB · 1,552 files no change ██████████ 118.3% of 5.63 MiB
authenticated shell (every logged-in page)
src/scenes/AuthenticatedShell.tsx
7.70 MiB · 2,683 files no change █████████░ 87.8% of 8.77 MiB

🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx

Largest files eagerly shipped from src/index.tsx
Size File
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
24.6 KiB ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js
6.3 KiB ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js
4.5 KiB ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js
3.9 KiB ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js
1.4 KiB ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js
1.3 KiB src/index.tsx
1.3 KiB src/RootErrorBoundary.tsx
912 B ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js
854 B src/scenes/ChunkLoadErrorBoundary.tsx
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
Size File
291.3 KiB ../node_modules/.pnpm/posthog-js@1.429.2_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs
267.7 KiB ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
261.1 KiB src/taxonomy/core-filter-definitions-by-group.json
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
103.5 KiB src/lib/api.ts
93.3 KiB ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js
90.6 KiB ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js
79.4 KiB src/products.tsx
68.9 KiB src/lib/lemon-ui/icons/icons.tsx
62.6 KiB src/lib/utils/eventUsageLogic.ts
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
Size File
291.3 KiB ../node_modules/.pnpm/posthog-js@1.429.2_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs
267.7 KiB ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
261.1 KiB src/taxonomy/core-filter-definitions-by-group.json
153.8 KiB ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
103.5 KiB src/lib/api.ts
95.9 KiB ../packages/quill/packages/quill/dist/index.js
93.3 KiB ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js
90.6 KiB ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js
79.4 KiB src/products.tsx

Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479

✅ Toolbar bundle — eager 2.31 MiB within budget

What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.

Metric Size Δ vs base Budget
Eager (shipped)
entry + static imports
2.31 MiB · 18 files no change ████░░░░░░ 40.3% of 5.72 MiB
Deferred (lazy) 2.12 MiB · 45 files no change n/a — loads on demand
Loader dist/toolbar.js 1.1 KiB no change █░░░░░░░░░ 5.8% of 19.5 KiB
Largest eagerly-shipped chunks
Size File
762.8 KiB dist/toolbar/toolbar-app-EB7CUUF3.css
619.9 KiB dist/toolbar/chunk-chunk-AYNWFYAV.js
484.8 KiB dist/toolbar/chunk-chunk-JY7YDEEM.js
136.2 KiB dist/toolbar/chunk-chunk-OZ4GGLE4.js
131.8 KiB dist/toolbar/chunk-chunk-FDH2IBXT.js
72.2 KiB dist/toolbar/toolbar-app-4NX77REV.js
69.0 KiB dist/toolbar/chunk-chunk-TSAL54PB.js
35.6 KiB dist/toolbar/chunk-chunk-VIRNUZBP.js
21.0 KiB dist/toolbar/chunk-chunk-6O6F3F7E.js
6.8 KiB dist/toolbar/chunk-chunk-DV7IWQNF.js

Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile

✅ Dist folder size — 🔺 +1.5 KiB (+0.0%)

Total size of the built frontend/dist folder (all assets), compared against the base branch.

Total: 1467.77 MiB · 🔺 +1.5 KiB (+0.0%)

✅ Playwright — all passed

All tests passed.

View test results →

⚠️ Backend snapshots — 1 updated (1 modified, 0 added, 0 deleted)

Query snapshots: Backend query snapshots updated

Changes: 1 snapshots (1 modified, 0 added, 0 deleted)

What this means:

  • Query snapshots have been automatically updated to match current output
  • These changes reflect modifications to database queries or schema

Next steps:

  • Review the query changes to ensure they're intentional
  • If unexpected, investigate what caused the query to change

Review snapshot changes →

✅ ClickHouse migration SQL — none

No ClickHouse migrations in the latest push.

@trunk-io

trunk-io Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@posthog

posthog Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PostHog Review

Found 1 must fix, 1 should fix.

@posthog posthog Bot removed the reviewhog label Sep 4, 2026
posthog Bot and others added 5 commits September 4, 2026 13:46
`last_called_at` is populated only from received `$feature_flag_called`
events, so an old timestamp does not prove the flag stopped being
evaluated. Local evaluation and SDKs configured without flag-call events
leave the column stale while the flag stays in use.

Reword the health alert to state what the data supports, and rename the
evidence class to match. The alert now reads "PostHog has not received a
call for this flag in N days", which lines up with the existing stale
banner copy ("Flag has not been called in N days").

Generated-By: PostHog Desktop
Task-Id: 02112534-d34f-4c62-b9fc-6e5242d0f90e
dry_run stops the writes, not the detection queries. The check sweeps every
team active in the last 90 days on its first run.

Today filter_stale_flags runs paginated, for one team at a time. No full batch
has a measured cost. Sample 1 percent of teams until one run gives that
measurement.

Generated-By: PostHog Desktop
Task-Id: f87cfffb-b199-459d-b5d3-7fc6647b17fe
The Celery task used False for dry_run when a registration was absent. A kind
cannot register a detect function without a registration, so that branch never
ran.

Survey.get_internal_flag_ids kept a team_id parameter. No caller passes it.

The _excluded_flag_ids docstring called its result a superset of the candidate
ids. Read literally, that means the check reports nothing.

Three tests asserted less than their names claim. The remediation-ordering test
matched an earlier sentence, so a regression that deletes the guarded step left
the test green.

Generated-By: PostHog Desktop
Task-Id: f87cfffb-b199-459d-b5d3-7fc6647b17fe
Every other in-product reference to a flag is excluded from the stale check.
A survey's user-created linked_flag is not, while the survey flags PostHog
generates itself are. The code did not say why, so a reader reads it as an
oversight.

The link is user-managed, bulk delete permits it, and the remediation tells the
investigator to check surveys. Excluding it would turn the check into a safety
gate, and brief 06 scopes it as evidence to investigate instead.

Generated-By: PostHog Desktop
Task-Id: f87cfffb-b199-459d-b5d3-7fc6647b17fe
@JamesPatrickGill
JamesPatrickGill marked this pull request as ready for review September 7, 2026 11:45
@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested review from a team September 7, 2026 11:46
@posthog-project-board-bot posthog-project-board-bot Bot moved this to In Review in Feature Flags Sep 7, 2026
@greptile-apps

greptile-apps Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
### Issue 1
products/feature_flags/backend/temporal/health_checks/stale_flags.py:121-128
**Default flags are missed**

This detector relies on `filter_stale_flags`, which does not classify the model-default `{"groups": []}` configuration the same way as `FeatureFlagStatusChecker`. An active flag with that configuration, no usage data, and a creation date older than 30 days is considered fully rolled out by the checker but is omitted from this health check. As a result, the check silently misses a common class of old fixed-result flags that it is intended to report. Please include this shape in the set-based filter and detection tests.

---

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

Reviews (1): Last reviewed commit: "docs(feature-flags): say why a survey's ..." | Re-trigger Greptile

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PostHog Review

Found 1 consider.

@posthog posthog Bot removed the reviewhog label Sep 7, 2026
posthog Bot and others added 2 commits September 7, 2026 12:24
A product tour row stays on the environment that created it, while a feature flag
moves to the project root team. The stale-flag check narrowed the product tour
lookup to the candidate flags' own team ids, so a tour on a child environment
never matched and its internal targeting flag could be reported as a cleanup
candidate. The bulk-delete guard scopes the same lookup by project, so the API
would still refuse to delete that flag.

Scope the lookup by project to match the guard and the four sibling call sites in
the feature flag API. The lookup stays one set-wise query.

Generated-By: PostHog Desktop
Task-Id: 701e4c7b-1bdf-472b-a56d-9bb7e2ba78f4
JamesPatrickGill and others added 2 commits September 7, 2026 13:33
Declare access_controlled_resource = "feature_flag" on the stale-flags
health check, so the Health API hides its issues from members whose
feature-flag access is restricted. The issue payloads carry flag keys
and names.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review fixes from the local review pass:

- Assert the weekly schedule and 1% rollout in the contract test, next
  to the dry_run assertion they share an operational purpose with.
- Cover a NULL is_remote_configuration flag staying in detection.
- Import the ROLLOUT_* constants from flag_status, their home module.
- Say why the survey exclusion is team-scoped while its siblings are
  project-scoped, and cross-link _excluded_flag_ids and the bulk-delete
  guard in both directions so the two lists do not drift silently.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@JamesPatrickGill JamesPatrickGill changed the title feat(feature-flags): detect stale flag health issues feat(flags): detect stale flag health issues Sep 8, 2026

@haacked haacked left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work! A few non-blocking suggestions inline, nothing blocking.

"""

name = "stale_feature_flags"
kind = "stale_feature_flags"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: Once dry_run flips, stale_feature_flags issues land in the "Other" bucket on the Health page. The page skips that category when building summary counts and shows zero issues on the detail page, so the issue count never shows.

The frontend lists all 13 other check kinds in HealthIssueKind, KIND_TO_CATEGORY, and KIND_LABELS, but not this one. Add a feature_flags category and register this kind in those three maps before flipping dry_run, or note next to dry_run = True (stale_flags.py:65) that the frontend work is deferred.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed. Added a feature_flags category to the Health page and registered the kind in HealthIssueKind, KIND_TO_CATEGORY, and KIND_LABELS. Issues now count in the summary, and the detail page labels them "Stale feature flag".

Comment thread posthog/tasks/health_checks.py Outdated
return

_process_batch_detection(team_ids=[team_id], kind=kind, detect_fn=detect_fn)
_process_batch_detection(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: _reevaluate_one_check builds the same _process_batch_detection call but doesn't forward dry_run. It's a no-op today since no web-analytics check sets dry_run, but the bug this PR just fixed is still live in that path.

A shared helper that looks up both the detect fn and dry_run from the registry would let both callers use it and remove the chance to forget again:

def run_check_for_team(kind: str, team_id: int) -> BatchResult:
    return _process_batch_detection(
        team_ids=[team_id],
        kind=kind,
        detect_fn=get_detect_fn(kind),
        dry_run=HEALTH_CHECKS[kind].dry_run,
    )

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Took the helper. run_check_for_team in processing.py looks up the detect fn and dry_run from the registry. The Celery task and _reevaluate_one_check both call it now. This touches web analytics, but only to route the same call through the helper. Behavior there is unchanged, because no web-analytics check sets dry_run.

candidate volume grows. These exclusions remove known blockers only. They do not prove
the remaining flags are free of repository references or product intent.

The bulk-delete guard in ``products/feature_flags/backend/api/feature_flag.py`` blocks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: The docstring mentions one deliberate divergence from bulk_delete's guard (bulk delete blocks only running experiments, this excludes every non-deleted one). There's a second: bulk delete's find_dependent_flags_batch only counts active=True dependent flags, while _depended_on_flag_ids counts disabled ones too. A flag whose only dependent is disabled can be deleted through the API but won't be reported stale here.

Add that divergence to the docstring alongside the experiments one, so someone reconciling the two guards doesn't have to work it out themselves.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Documented. The docstring now names both deliberate divergences: the guard blocks only running experiments where this excludes every non-deleted one, and the guard counts only active dependent flags where this also lets disabled dependents block.



def replay_linked_flag_ids_for_projects(project_ids: Collection[int], flag_ids: Collection[int]) -> set[int]:
"""Multi-project form of `replay_linked_flag_ids`, for batched jobs that span teams."""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: The docstring explaining jsonb id-matching semantics (why a malformed {"id": true} matches nothing) sits on the one-line wrapper replay_linked_flag_ids, not on replay_linked_flag_ids_for_projects which does the work. Move it to the function that does the work, or delete the wrapper and have bulk_delete call replay_linked_flag_ids_for_projects([self.project_id], ...) directly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved the jsonb semantics docstring to replay_linked_flag_ids_for_projects, which does the work. Kept the wrapper: bulk_delete has one project id in hand, and the wrapper keeps that call site plain.

ROLLOUT_PARTIAL = "partial"


def rollout_state_and_variant(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: rollout_state_and_variant is the only per-flag rollout derivation outside FeatureFlagStatusChecker and taking the checker as an argument; get_rollout_summary and is_multivariate_flag_fully_rolled_out are both methods on it. Making it a method drops both the "FeatureFlagStatusChecker" forward-reference string and the checker parameter at both call sites (api/feature_flag.py:582, stale_flags.py:210).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Made it a method on FeatureFlagStatusChecker. Both call sites drop the checker argument, and the forward-reference string is gone.

team__project_id__in=project_ids, internal_targeting_flag__isnull=False
).values_list("internal_targeting_flag_id", flat=True)
)
excluded |= set(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: The experiment and early-access lookups in _excluded_flag_ids interpolate every candidate flag id into the statement, while the other four exclusion queries scope by team/project. At a 250-team batch with an uncapped candidate list, each of those two statements carries one bind parameter per stale flag in the whole batch.

Experiment and EarlyAccessFeature both carry a team FK (RootTeamMixin), so you can scope them by team_id__in=team_ids the way the survey lookup already does. The caller still narrows with flag.id in excluded_ids.

Suggested change
excluded |= set(
excluded |= set(
Experiment.objects.filter(team_id__in=team_ids, deleted=False).values_list("feature_flag_id", flat=True)
)
excluded |= set(
EarlyAccessFeature.objects.filter(team_id__in=team_ids, feature_flag_id__isnull=False).values_list(
"feature_flag_id", flat=True
)
)

EarlyAccessFeature.feature_flag is nullable, so the isnull=False filter matters here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Applied as suggested. Both lookups scope by team_id__in=team_ids now, with feature_flag_id__isnull=False on the early-access lookup. test_batches_multiple_teams also gained a blocker on the second team, so the tests would catch a lookup that only covers the first team.

for flag in candidates:
if flag.id in excluded_ids:
continue
issues.setdefault(flag.team_id, []).append(_build_result(flag, now))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

question: This is the first health check that files more than one issue per team, every other builds a single result with empty hash_keys, while this appends one per stale flag. On the first run after dry_run flips, a team's entire backlog becomes newly-active at once, each firing its own $health_check_issue_firing event.

The dry-run summary logs batch-wide counts and teams-with-issues, but not the worst single team. Worth logging the per-team max in detect while the check is still dark, so the flip decision has that number?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One issue per flag is deliberate. The flag is the unit the cleanup loop operates on: the follow-up scout authors one report per flag, so each flag needs a stable issue identity (hash_keys=["flag_id"]) and independent resolution when that one flag gets evaluated again. A single per-team issue would either churn its hash on every membership change or carry an unbounded flag list in one payload.

The first-run fan-out is real, and it is part of why dry_run stays on. detect now logs teams_with_issues, issue_count, and max_issues_per_team per batch, so the flip decision has the worst-team number while the check is dark. At flip time the options are a per-team cap in detect or handling the volume at the destination. That choice, like the Health API access-control question, gates the flip rather than this PR.


assert not HealthIssue.objects.filter(team=self.team, kind="stale_feature_flags").exists()

def test_batches_multiple_teams(self) -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: The exclusion queries are only tested against a single team. test_detect_inclusion_and_exclusion puts every blocker on self.team, and test_batches_multiple_teams uses two teams but creates no blockers on the second, so those lookups could check only the first team and the tests would still pass.

Extend test_batches_multiple_teams to add a blocker (e.g. a session-recording-linked flag) on team_two and assert it's excluded:

Suggested change
def test_batches_multiple_teams(self) -> None:
FeatureFlag.objects.create(team=team_two, key="third", created_by=self.user, active=True, **stale_by_usage())
blocked = FeatureFlag.objects.create(
team=team_two, key="blocked", created_by=self.user, active=True, **stale_by_usage()
)
Team.objects.filter(pk=team_two.pk).update(
session_recording_linked_flag={"id": blocked.id, "key": blocked.key}
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Extended as suggested. test_batches_multiple_teams puts a replay-linked stale flag on the second team and asserts it never appears in the results.

@github-project-automation github-project-automation Bot moved this from In Review to Approved in Feature Flags Sep 8, 2026
JamesPatrickGill and others added 4 commits September 9, 2026 11:55
Register the stale_feature_flags kind in the Health page's category
maps under a new feature flags category. Without this the kind falls
into the Other bucket, which the summary counts skip, so the detector
would ship invisible when dry_run flips.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
run_check_for_team looks up the detect fn and dry_run from the
registry, so no manual path can forget to forward dry_run again. The
Health page refresh task and the web-analytics doctor tool both use it
now; the doctor tool's behavior is unchanged because no web-analytics
check sets dry_run.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Scope the experiment and early-access exclusion lookups by
  team_id__in instead of binding one parameter per candidate flag, and
  prove batch-wide exclusion with a blocker on a second team in
  test_batches_multiple_teams.
- Log teams_with_issues, issue_count, and max_issues_per_team from
  detect, so the dry_run flip decision has the worst-team alert volume.
- Document the second deliberate divergence from the bulk-delete guard:
  it counts only active dependent flags, this also lets disabled
  dependents block.
- Make rollout_state_and_variant a FeatureFlagStatusChecker method,
  dropping the checker argument and forward reference at both call
  sites.
- Move the jsonb id-matching docstring onto
  replay_linked_flag_ids_for_projects, which does the work.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

🦔 Hogbox preview · ✅ ready

▶ Open the preview

🔑 Login test@posthog.com / 12345678 (demo data)
🧩 Running this PR's backend and frontend, on the PostHog :master base
🔗 Link stable across rebuilds — a re-push swaps the box underneath, the URL stays
🔒 Access tailnet only (PostHog VPN)
🛠️ Admin inspect & debug state in hogland
💤 Idle sleeps after ~30 min idle (snapshot to S3, zero node cost) and wakes on your next visit in ~30s, behind a brief "waking up" screen

commit 1f3970c · box box-3eba09effab5 · ready in 1024s (push → usable) · build log · rebuilds on every push, torn down on close

The doctor tool now routes re-evaluation through run_check_for_team,
so the tests patch that instead of the removed _process_batch_detection
import, and the per-kind failure stub takes the new signature.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@JamesPatrickGill

Copy link
Copy Markdown
Member Author

/trunk merge

@trunk-io
trunk-io Bot merged commit c34d9b9 into master Sep 10, 2026
254 checks passed
@trunk-io
trunk-io Bot deleted the feat/flags-stale-health-check branch September 10, 2026 11:55
@github-project-automation github-project-automation Bot moved this from Approved to Done in Feature Flags Sep 10, 2026
@deployment-status-posthog

deployment-status-posthog Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-09-10 12:21 UTC Run
prod-us ✅ Deployed 2026-09-10 12:46 UTC Run
prod-eu ✅ Deployed 2026-09-10 12:49 UTC Run

This branch was successfully deployed

1 active deployment
preview-pr-95189 — 1f3970cd Deployed Sep 10, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants