chore(warehouse): remove the always-on auto-coarsen rollout flag - #95006
chore(warehouse): remove the always-on auto-coarsen rollout flag#95006posthog[bot] wants to merge 2 commits into
Conversation
The `data-warehouse-auto-coarsen` flag is at 100% for every team, so it always evaluates to true. Every coarsening decision still paid for a Team fetch and a PostHog API call to learn that. Drop `WAREHOUSE_AUTO_COARSEN_FLAG`, `is_auto_coarsen_enabled`, and the two gates that read it: the detection gate in `maybe_flag_for_coarsening` and the queued rewrite release in the repartition activity. The `flag_disabled` decline reason goes with them. Generated-By: PostHog Desktop Task-Id: 428318a9-ebb2-4839-ae9a-5a28acfe33e3
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
posthog
Bot
added
the
skip-agent-review
🤖 CI report
|
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — escalated to a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
This removes the always-on auto-coarsen feature flag (and the rollback lever it provided) from the warehouse repartitioning/coarsening controller that actively rewrites customer data table layouts in production; the diff logic checks out against the description, but this is production data-pipeline behavior with reduced reversibility and it has zero human or agent review, and the bot author isn't on the owning team.
- No independent review (0 top-level/inline reviews, only an unrelated trunk-merge bot comment)
- Removes the feature flag that previously let ops disable auto-coarsening if it misbehaves, reducing the rollback path for a change that governs live data rewrites
- Author (posthog[bot]) is not on the owning team @PostHog/team-warehouse-sources and familiarity signals don't apply to this machine author
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 44L, 2F substantive, 106L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (106L, 6F, single-area, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 1b0adb4 · reviewed head 1b0adb4 |
🦔 PostHog Review reviewed this pull requestNothing worth raising this time, so here's a calming picture instead:
|
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| # such a table, and it does nothing here if it only gates detection. Only the triggers this flag | ||
| # stages answer to it; every other reason fails open, because that work was queued knowing that | ||
| # syncing on the old layout is the worse option, so it must never dead-end on a rollout flag. A | ||
| # staged swap is always driven to completion because temp is the source of truth in that window | ||
| # and live may already be deleted. | ||
| if pending is not None and swap is None and not enabled: |
There was a problem hiding this comment.
Queued coarsening loses its release control
Why we think it's a valid issue
- Checked: the pre-PR and post-PR forms of the release block, every caller of
maybe_flag_for_coarsening, the trigger_reason values the controller writes, the attempt cap, the activity timeout, and the admin views for an alternative operator lever. - Found: the new gate at
products/warehouse_sources/backend/temporal/data_imports/workflow_activities/repartition_table.py:327bindsenabledtois_auto_repartition_enabled, and the inner test only acceptsproactive_thresholdandoom_history. The controller writes"coarsening"for an automatic stage atproducts/warehouse_sources/backend/temporal/data_imports/pipelines/core/repartition_controller.py:250, so that reason now reaches no release branch. - Found: the repartition flag is not a substitute switch for coarsening.
maybe_flag_for_coarseningruns atrepartition_controller.py:356and:386, both before the flag read at:406. Coarsening detection therefore continues with the repartition flag off, and a coarsening rewrite already queued still runs. - Found: the queued rewrite runs ahead of extraction with
start_to_close_timeout=dt.timedelta(hours=6)(products/warehouse_sources/backend/temporal/data_imports/external_data_job.py:680), on every sync while the marker is set. - Found:
MAX_REPARTITION_ATTEMPTS = 3does not bound a rewrite that advances.repartition_table.py:672writesattempts: 0after any forward progress, so a rewrite that advances a little each run never reaches_give_up. - Found: no other supported release exists. The admin only stages a target (
products/warehouse_sources/backend/admin/external_data_schema_admin.py:265); no admin action or command clearsrepartition_pending. - Found: the comment kept at
repartition_table.py:319-326still states that the flag is the only lever support has, and it justifies the fail-open path with work "queued knowing that syncing on the old layout is the worse option". That reason describes operator-staged work. The deleted comment named exactly that set (admin, coarsening nominations), andcoarsening_requestedalready covers the nomination. The automaticcoarseningfamily now fails open under a reason that does not apply to it. - Impact: a large or pathological automatic coarsening rewrite can hold that schema's pre-extraction slot for up to six hours per sync, for an unbounded number of syncs, and an operator has no supported way to release it short of a code change or a manual database edit. The change is dead today because the flag sits at 100%, but it removes the documented stop for a state the surrounding code was built to escape.
Issue description
This condition never stops a queued coarsening rewrite. The rewrite runs before extraction and can consume the six-hour activity budget. Progressing rewrites reset the failure counter, so they can delay many syncs. The removed flag was the supported release control for this state.
Suggested fix
Retain an emergency stop for queued automatic coarsening. Evaluate it only when trigger_reason is coarsening and no swap exists. This preserves the release path without adding a flag call to every sync. Add a test for an existing queued coarsen when this control is disabled.
Prompt to fix with AI (copy-paste)
## Context
@products/warehouse_sources/backend/temporal/data_imports/workflow_activities/repartition_table.py#L322-327
<issue_description>
This condition never stops a queued coarsening rewrite. The rewrite runs before extraction and can consume the six-hour activity budget. Progressing rewrites reset the failure counter, so they can delay many syncs. The removed flag was the supported release control for this state.
</issue_description>
<issue_validation>
- **Checked:** the pre-PR and post-PR forms of the release block, every caller of `maybe_flag_for_coarsening`, the trigger_reason values the controller writes, the attempt cap, the activity timeout, and the admin views for an alternative operator lever.
- **Found:** the new gate at `products/warehouse_sources/backend/temporal/data_imports/workflow_activities/repartition_table.py:327` binds `enabled` to `is_auto_repartition_enabled`, and the inner test only accepts `proactive_threshold` and `oom_history`. The controller writes `"coarsening"` for an automatic stage at `products/warehouse_sources/backend/temporal/data_imports/pipelines/core/repartition_controller.py:250`, so that reason now reaches no release branch.
- **Found:** the repartition flag is not a substitute switch for coarsening. `maybe_flag_for_coarsening` runs at `repartition_controller.py:356` and `:386`, both before the flag read at `:406`. Coarsening detection therefore continues with the repartition flag off, and a coarsening rewrite already queued still runs.
- **Found:** the queued rewrite runs ahead of extraction with `start_to_close_timeout=dt.timedelta(hours=6)` (`products/warehouse_sources/backend/temporal/data_imports/external_data_job.py:680`), on every sync while the marker is set.
- **Found:** `MAX_REPARTITION_ATTEMPTS = 3` does not bound a rewrite that advances. `repartition_table.py:672` writes `attempts: 0` after any forward progress, so a rewrite that advances a little each run never reaches `_give_up`.
- **Found:** no other supported release exists. The admin only stages a target (`products/warehouse_sources/backend/admin/external_data_schema_admin.py:265`); no admin action or command clears `repartition_pending`.
- **Found:** the comment kept at `repartition_table.py:319-326` still states that the flag is the only lever support has, and it justifies the fail-open path with work "queued knowing that syncing on the old layout is the worse option". That reason describes operator-staged work. The deleted comment named exactly that set (admin, coarsening nominations), and `coarsening_requested` already covers the nomination. The automatic `coarsening` family now fails open under a reason that does not apply to it.
- **Impact:** a large or pathological automatic coarsening rewrite can hold that schema's pre-extraction slot for up to six hours per sync, for an unbounded number of syncs, and an operator has no supported way to release it short of a code change or a manual database edit. The change is dead today because the flag sits at 100%, but it removes the documented stop for a state the surrounding code was built to escape.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Retain an emergency stop for queued automatic coarsening. Evaluate it only when trigger_reason is coarsening and no swap exists. This preserves the release path without adding a flag call to every sync. Add a test for an existing queued coarsen when this control is disabled.
</potential_solution>
There was a problem hiding this comment.
Checked this against the current head and the finding holds — I'm escalating rather than fixing, because every way to address it is a call the flag's owner should make, not something I should decide unattended.
What I confirmed:
- The release check now reads only the repartition flag, and only lets
proactive_thresholdandoom_historythrough. An automatic coarsening rewrite writes the trigger reasoncoarsening, so it matches no release branch. - The repartition flag is not a stand-in switch. Coarsening detection runs before that flag is ever read, so turning the repartition flag off neither stops coarsening from being staged nor releases one already queued.
- The queued rewrite does run ahead of extraction on every sync, with a six-hour activity budget, and the attempt cap does not bound it: any forward progress resets the attempt counter to zero, so a rewrite that advances a little each run never gives up.
- There is no other supported release. The admin can only stage a target, and no admin action or management command clears the pending marker — clearing it happens only from inside the activity itself.
So the concern is real: after this PR, a large or pathological automatic coarsening rewrite has no supported off switch short of a code change or a manual database edit.
Why I'm not fixing it here. Keeping a flag read just for the queued-release path is the obvious minimal patch, but it conflicts with this PR's own release note — the flag is about to be turned off in PostHog, and a retained read would then release every queued coarsening rewrite the moment that happens. It would also leave a flag that must never be disabled, which is the stale-flag problem this PR exists to remove. The alternatives — a new settings kill switch, or an admin action / management command that clears the pending marker — are new operator surface area beyond this PR's scope.
What a human needs to decide, one of:
- Accept the trade-off. Automatic coarsening rewrites join
adminandcoarsening_requestedas work that always runs to completion — those already had no release lever before this PR. If that's the call, the comment above the release check should be corrected too: its fail-open justification says the work "was queued knowing that syncing on the old layout is the worse option", which describes operator-staged work and no longer covers the automatic coarsening family. - Keep a coarsening switch, scoped to the queued-release path only, and leave the flag enabled in PostHog rather than turning it off.
- Land the removal and follow up with a general release lever — an admin action or management command that clears a pending repartition — which covers
admin,coarsening_requested, andcoarseningin one place instead of one flag per family.
Happy to implement whichever is chosen.
…-the-always-on-96e63a Generated-By: PostHog Desktop Task-Id: 428318a9-ebb2-4839-ae9a-5a28acfe33e3
|
On the escalation, taking the three points in turn. Rollback lever. The point is correct, and it is now stated in the description under "Reversibility" rather than left for a reviewer to find. Checked rather than assumed: No code changed in response. Restoring a lever under a new mechanism is a wider change than a flag cleanup, and which lever is worth carrying is a call for the owning team. No independent review and author not on the owning team. Neither is something this PR can resolve, which is why the The PR is out of draft so the full backend matrix runs. Under draft the Django and product test matrices were skipped while their aggregate checks still reported green, so the change had no backend test behind it in CI. |
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
The PR's own agent reviewer flagged a substantive, unresolved concern — removing the coarsen flag also removes the only remaining kill switch for a queued automatic coarsening rewrite (a six-hour, self-resetting activity) — and explicitly declined to fix it, deferring to a human. That's an unaddressed substantive review finding on a data-rewrite path, which blocks auto-approval regardless of size tier.
- Unresolved inline review thread on repartition_table.py: after this change, a queued 'coarsening' rewrite has no supported way to be released/stopped (previously the coarsen flag could be disabled to release it); the reviewer confirmed the issue and chose to escalate rather than patch it.
- Author is posthog[bot] (machine user) with no owning-team membership signal usable for assurance, and files fall under @PostHog/team-warehouse-sources — the change needs a human from that team to decide whether losing this operational lever is acceptable.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 44L, 2F substantive, 106L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (106L, 6F, single-area, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ bacd9d5 · reviewed head bacd9d5 |
|
|
Problem
data-warehouse-auto-coarsenflag sits at 100% for the catch-all group and 100% for the team group, and PostHog reports it as stale with a full rollout.flag_disabled) and a release path in the repartition activity that could never fire.Reported by a PostHog inbox scout. Flag definition: 798825.
Warning
This removes the only runtime kill switch on coarsening. Read "Reversibility" below before approving.
Changes
WAREHOUSE_AUTO_COARSEN_FLAGandis_auto_coarsen_enabledare gone, with the two call sites that read them.maybe_flag_for_coarseningdrops its last gate, so the remaining gates end at the database and no longer reach a feature flag.coarseningrewrite always proceeds, which is what the always-on flag already produced.DELTA_COARSEN_DECLINE_TOTALloses theflag_disabledlabel. The other reasons are unchanged.Nothing user-visible changes. The controller made the same decision before this PR.
Reversibility
A reviewer should weigh this before approving. The trade-off is real and this PR does not remove it.
is_auto_repartition_enableddoes not cover the gap. Coarsening is evaluated on the healthy branch ofmaybe_flag_for_repartition, which is reached beforeenabledis resolved, so the repartition flag never gated coarsening.select_coarsen_targetstill refuses any target that would not fit the budget.Keeping a kill switch is a defensible alternative to this PR. It is a call for the owning team, not for the agent that opened it.
How did you test this code?
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/test_repartition_controller.pyand.../workflow_activities/tests/test_repartition_table.pyboth pass against a local dev stack.flag_disabledvariant, and one activity case (coarsen_flag_off_releases_a_queued_coarsen) asserted behavior that no longer exists.tests/e2e/test_end_to_end.py. It needs a live Postgres source and object storage that this sandbox does not provide. Its coarsening tests were edited, so CI is the check on them.Automatic notifications
Docs update
products/warehouse_sources/backend/temporal/data_imports/README.mdis updated in this PR. No external docs mention the flag.🤖 Agent context
Autonomy: Fully autonomous
Written by Claude Opus 5 through PostHog Desktop, from an inbox report on stale feature flags.
/writing-pr-descriptions,/writing-simplified-technical-english.gh pr list --state open --search "repartition"and a search on the flag name found no PR covering this cleanup. One open PR touches this area (#92380) on an unrelated corrupt-table path.releasevariable withcoarseningalways resolving to false. Collapsing the whole block behindnot enabledsays the same thing in fewer branches.Created with PostHog Desktop from this inbox report.