[posthog]

Problem

• The ClickHouse migration step dies before it applies anything whenever the posthog_migrations cluster has more than one node. This hits our own migration and bootstrap tooling across dev, CI, and self-hosted installs, not end users. It has failed continuously since late February, still arriving today, across ClickHouse 23.9 through 25.6 (many separate installs, not one host retrying).
• migrate_clickhouse asks infi for applied migrations with replicated=True, so infi selects from the Distributed tracking table. A helper pre-creates that table ON CLUSTER so infi's auto-create never runs, but it was gated on the MULTINODE_CLICKHOUSE flag, which defaults to False everywhere except one CI workflow.
• The guard was keyed on the wrong thing. The hazard is the cluster topology, not the flag. With the flag off and a real multi-node cluster, infi falls into a CREATE TABLE with no ON CLUSTER and hangs past the 60 second default timeout the Database(...) call never overrode. A variant fails with CLUSTER_DOESNT_EXIST where the config has no such cluster.
• Retrying does not help. Each attempt re-hangs for the full timeout. The same call site also timed out against the US online cluster since late July, the transient version of the same weakness.

Changes

• Key the pre-create on the real cluster topology, read from system.clusters, instead of MULTINODE_CLICKHOUSE. The tracking-table pre-create now runs when the migrations cluster has more than one node, and stays off for a single-node cluster or a cluster the config does not define (which also avoids the CLUSTER_DOESNT_EXIST variant).
• Pass an explicit, generous timeout when constructing the migration Database, so a slow ON CLUSTER statement no longer fails at infi's 60 second default.
• Stacked on the open retry PR (fix(migrations): retry clickhouse migration step on startup races #66568), which touches only bin/migrate. That PR retries the step; this one removes the deterministic hang the retries were re-hitting.

-        if not settings.MULTINODE_CLICKHOUSE:
-            return
+        if not multi_node:  # multi_node comes from system.clusters
+            return

How did you test this code?

I am an agent. I did not manually run this against a multi-node cluster. I added posthog/management/commands/test/test_migrate_clickhouse.py with automated cases:

• The tracking-table ON CLUSTER pre-create follows the cluster node count, not the flag: it runs for a multi-node cluster and stays off for a single-node one, both with MULTINODE_CLICKHOUSE=False. This guards the exact regression fixed here — re-gating the pre-create on the flag would fail these cases.
• The migration Database is built with the generous timeout, guarding against a return to the 60 second default.

No existing test covered this command, so the tests are new rather than extended.

Automatic notifications

• Publish to changelog?
• Alert Sales and Marketing teams?

Docs update

No user-facing docs affected. This is internal migration tooling.

🤖 Agent context

Autonomy: Fully autonomous

• Authored by an agent in a PostHog Desktop cloud task, from a Signals inbox report that traced the mechanism.
• Skills invoked: /writing-tests (gated and shaped the new test), /writing-simplified-technical-english.
• Decision: keyed the guard on system.clusters row count so single-node and undefined-cluster installs skip the ON CLUSTER path, which also removes the CLUSTER_DOESNT_EXIST variant. Chose to stack on the retry PR rather than duplicate its bin/migrate changes.

---

Created with PostHog Desktop from this inbox report.

[posthog]

🦔 PostHog Review reviewed this pull request

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

Published 1 finding (view the review).

[stamphog]

This changes ClickHouse migration bootstrap logic (cluster-topology detection driving ON CLUSTER table pre-creation, plus a new connection timeout) — migration tooling that runs against production and self-hosted clusters is risky territory, and the only review activity so far is a bot review that has just started ("Step 1/6") with no findings or sign-off yet, so there's no independent assurance on the current head.

• No completed review (human or agent) — the only discussion activity is ReviewHog's in-progress status comment, not a validated finding set or approval.
• No ownership-source match and the author is a machine user, so familiarity/ownership provide no assurance signal for this risky-territory change.
• New unit tests only cover the author's own claims (mocked cluster topology) — they don't substitute for independent review of the actual multi-node behavior.

Gate mechanics and policy version

Gate		Result
prerequisites	✓	all clear
deny-list	✓	no deny categories matched
size	✓	63L, 1F substantive, 118L/2F incl. docs/generated/snapshots — within ceiling
tier	✓	T1-agent / T1c-medium (118L, 2F, single-area, fix)
stamphog 2.0.0b4		.stamphog/policy.yml @ 31370b2 · reviewed head 31370b2

[posthog]

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

[posthog]

ReviewHog Report

Found 1 consider.

[scheduled-actions-posthog]

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

[scheduled-actions-posthog]

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

[posthog]

PostHog Review

Found 1 consider.

[posthog]

Correct the auto-create explanation

Why we think it's a valid issue

• Checked: The infi.clickhouse_orm auto-create path in the installed venv, the two tracking models it creates, on_cluster(), the Database constructor, and the value of CLICKHOUSE_MIGRATIONS_CLUSTER.
• Found: The auto-create path calls create_table(MigrationHistoryReplicated) and create_table(MigrationHistoryDistributed) (database.py:364-378). Both models subclass plain Model (migrations.py:289, migrations.py:309), so both use Model.create_table_sql, which renders on_cluster(db) into the statement (models.py:355). on_cluster() returns ON CLUSTER '<cluster>' whenever db.cluster is not None (utils.py:158-162).
• Found: db.cluster is always set on this path. Database.__init__ stores it (database.py:109), migrate_clickhouse.py:71 passes cluster=CLICKHOUSE_MIGRATIONS_CLUSTER, and that setting defaults to the non-empty string "posthog_migrations" (posthog/settings/data_stores.py:342). It is never None. So infi's auto-create does emit ON CLUSTER, and the comment at posthog/management/commands/migrate_clickhouse.py:167-168 states the opposite.
• Found: The same file already contradicts itself. Line 143 says "infi still builds the tracking tables with ON CLUSTER <cluster>" — the correct behavior, added by commit feb766a, which fixed this exact misconception in the neighboring comment. Lines 167-171 still carry the old, wrong version, including the UNKNOWN_TABLE-on-satellites failure chain that cannot happen once the DDL runs ON CLUSTER.
• Impact: The only stated justification for _create_migration_tracking_tables_if_not_exist is false, and it disagrees with another comment 25 lines above it. A future maintainer reading the two comments cannot tell which one describes infi. That maintainer may delete the helper as unnecessary, or re-derive the guard from the wrong failure mode, in tooling that runs DDL against production clusters. The real reasons — the helper creates the tables through the native client before infi's HTTP request path reads the Distributed table, and it removes the failed-SELECT-then-create retry — are absent.
• Impact: No behavior changes, so this stays at the reviewer's lowest priority. It is not a wording preference, however: the claim is objectively wrong and self-contradictory inside the added code, so it does not fall under the style exclusion.

Issue description

The earlier review thread confirmed that infi adds ON CLUSTER when db.cluster is set. This comment still says that auto-create omits it. Both tracking models inherit Model, whose create_table_sql calls on_cluster(db). The comment therefore gives the wrong reason for this pre-create path.

Suggested fix

Explain that this helper creates the cluster tables before infi reads the Distributed table. It also moves creation to the native client. Do not state that infi omits ON CLUSTER.

Prompt to fix with AI (copy-paste)

## Context
@posthog/management/commands/migrate_clickhouse.py#L166-173

<issue_description>
The earlier review thread confirmed that infi adds `ON CLUSTER` when `db.cluster` is set. This comment still says that auto-create omits it. Both tracking models inherit `Model`, whose `create_table_sql` calls `on_cluster(db)`. The comment therefore gives the wrong reason for this pre-create path.
</issue_description>

<issue_validation>
- **Checked:** The `infi.clickhouse_orm` auto-create path in the installed venv, the two tracking models it creates, `on_cluster()`, the `Database` constructor, and the value of `CLICKHOUSE_MIGRATIONS_CLUSTER`.
- **Found:** The auto-create path calls `create_table(MigrationHistoryReplicated)` and `create_table(MigrationHistoryDistributed)` (`database.py:364-378`). Both models subclass plain `Model` (`migrations.py:289`, `migrations.py:309`), so both use `Model.create_table_sql`, which renders `on_cluster(db)` into the statement (`models.py:355`). `on_cluster()` returns `ON CLUSTER '<cluster>'` whenever `db.cluster is not None` (`utils.py:158-162`).
- **Found:** `db.cluster` is always set on this path. `Database.__init__` stores it (`database.py:109`), `migrate_clickhouse.py:71` passes `cluster=CLICKHOUSE_MIGRATIONS_CLUSTER`, and that setting defaults to the non-empty string `"posthog_migrations"` (`posthog/settings/data_stores.py:342`). It is never `None`. So infi's auto-create does emit `ON CLUSTER`, and the comment at `posthog/management/commands/migrate_clickhouse.py:167-168` states the opposite.
- **Found:** The same file already contradicts itself. Line 143 says "infi still builds the tracking tables with `ON CLUSTER <cluster>`" — the correct behavior, added by commit feb766abb, which fixed this exact misconception in the neighboring comment. Lines 167-171 still carry the old, wrong version, including the `UNKNOWN_TABLE`-on-satellites failure chain that cannot happen once the DDL runs `ON CLUSTER`.
- **Impact:** The only stated justification for `_create_migration_tracking_tables_if_not_exist` is false, and it disagrees with another comment 25 lines above it. A future maintainer reading the two comments cannot tell which one describes infi. That maintainer may delete the helper as unnecessary, or re-derive the guard from the wrong failure mode, in tooling that runs DDL against production clusters. The real reasons — the helper creates the tables through the native client before infi's HTTP request path reads the Distributed table, and it removes the failed-SELECT-then-create retry — are absent.
- **Impact:** No behavior changes, so this stays at the reviewer's lowest priority. It is not a wording preference, however: the claim is objectively wrong and self-contradictory inside the added code, so it does not fall under the style exclusion.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Explain that this helper creates the cluster tables before infi reads the Distributed table. It also moves creation to the native client. Do not state that infi omits `ON CLUSTER`.
</potential_solution>