Repository navigation
feat(nodejs): add a build-time migration manifest and schema check - #128242
Conversation
…heck Node images deploy before the Django image that runs bin/migrate, so a Node release that reads a new column can start while the column is still missing. After #127579 that meant "column does not exist" errors until Django caught up. - Dockerfile.node writes nodejs/schema-requirements.json at build time. The generator scans nodejs/src for Django table names, maps each table to its owning app from the migration files (CreateModel, db_table, AlterModelTable, so moved models count for both apps) and records each scoped app's max_migration.txt leaf. Squash leaves also record their replaces list. ClickHouse and apps routed in products/db_routing.yaml are skipped. - Bind mounts feed the migration trees to the generator, so no Python, no Django and no extra layer in the Node build. The step lives in the Dockerfile so every build of Dockerfile.node gets the file. - nodejs/dist/schema-check/cli.js reads the manifest and checks django_migrations on the primary (DATABASE_URL or the POSTHOG_DB_* parts, never the replica). Exit 0 applied, 1 missing, 2 cannot check. --wait-seconds polls. statement_timeout goes through set_config inside a transaction because PgBouncer rejects it as a startup parameter. - The DATABASE_URL builder moves to common/config/database-url.ts so the CLI reuses it without loading the full service config. - safe-django-migrations.md: Node/Rust readers of a new column need the migration live first; drops stay code-first. The deploy hook that runs the CLI lives in the charts repo and ships disabled. Hook timing and which apps enable it stay a cloud-platform call.
|
😎 Merged successfully - details. |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
nodejs/src/schema-check/derive-requirements.ts |
12 | 228 |
nodejs/src/schema-check/postgres-migration-store.ts |
7 | 74 |
nodejs/src/schema-check/check.ts |
6 | 143 |
nodejs/src/schema-check/manifest.ts |
6 | 136 |
nodejs/src/schema-check/cli.ts |
3 | 202 |
nodejs/src/schema-check/derive-requirements.test.ts |
2 | 114 |
nodejs/src/schema-check/check.test.ts |
1 | 185 |
This check does not block merging. It updates on every push and clears when the share drops.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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. |
…mode
Node also reads tables in the persons, cyclotron-node and behavioral-cohorts
databases. sqlx-migrate deploys those migrations as its own app, so nothing
orders them against a Node rollout either. Recording them now settles the
manifest format before anything depends on it.
- Every requirement carries a kind ("django" or "sqlx"). The parser rejects an
unknown kind, because skipping it would pass a check nothing ran.
- The generator reads CREATE TABLE statements in the three sqlx migration
directories and adds the latest version of each database whose tables
nodejs/src names. Dockerfile.node bind-mounts those directories.
- The CLI checks sqlx entries once after the Django wait, in shadow mode: it
logs satisfied, missing, dirty (a success = false row), skipped (no URL in
this app's env) or error, and never changes the exit code. Enforcing comes
later, once the logs show the grants and timing work.
- The behavioral cohorts URL builder moves next to the main one so the CLI
reuses it. Behavior is unchanged.
- The digest stays an identifier for logs. Consumers do not recompute it.
|
Risk: Low · 1 low Adds a build-time manifest of the Django/sqlx migrations Node code depends on, plus a deploy-time CLI that checks them against Sentinel reviewed |
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| } | ||
|
|
||
| // A table belongs to every app whose migrations name it, so a model moved between apps puts both apps in scope. | ||
| export function collectTableOwners(apps: DjangoApp[]): Map<string, Set<string>> { |
There was a problem hiding this comment.
The deploy gate can report "all required migrations applied" while a migration Node needs is still unapplied, so the rollout proceeds into the exact column does not exist failure this PR exists to prevent. collectTableOwners (nodejs/src/schema-check/derive-requirements.ts:126-151) attributes a table only to apps whose migrations contain CreateModel, db_table, or table=; a migration that only alters another app's table records no ownership, so the altering app never gets a manifest requirement.
How:
- An app (e.g.
ee) ships a migration adding a column to a table owned by theposthogapp —ee/migrations/0006_event_definition_verification.pydoes exactly this shape (AddFieldonenterpriseeventdefinition, tableposthog_eventdefinition, nodb_table/CreateModel). - Node code starts reading that column;
posthog_eventdefinitionappears innodejs/src, which puts only theposthogapp in the manifest (its leaf migration), neveree. - Node deploys before the Django image runs
bin/migrate; theeemigration is unapplied. - The check queries only the
posthogrequirement, sees it applied, exits 0, and the rollout proceeds against the missing column.
The same heuristic also misses tables whose names Node builds dynamically (collectNodeIdentifiers at line ~166 matches literal identifiers only). No guard in derive-requirements.ts or manifest.ts captures cross-app alterations. Fix: also attribute a table to any app whose migrations reference its model (AddField/AlterField on model_name, and RunSQL bodies), or have the derivation take an explicit per-table ownership map instead of inferring it.
React with 👍 if useful or 👎 if not
There was a problem hiding this comment.
Does not apply. Django migration operations only touch models of their own app, so AddField(model_name="enterpriseeventdefinition") in ee changes ee_enterpriseeventdefinition (multi-table inheritance), not posthog_eventdefinition. A cross-app change would need RunSQL, and a scan of every default-database migration finds no ALTER TABLE on a table another app owns. Table names that Node builds at runtime are a known limit of the text match.
Problem
bin/migrate. After feat(workflows): hold capped messages until a frequency cap slot frees #127579 this causedcolumn does not existerrors.Changes
nodejs/schema-requirements.json: the Django migrations its code needs, derived at build time from the tablesnodejs/srcnames. No Python or Django in the build.node nodejs/dist/schema-check/cli.jschecks that list againstdjango_migrationson the primary. A missing manifest fails, never passes.kind, and an unknown kind fails the parse.DATABASE_URLand behavioral cohorts URL builders move tocommon/config/database-url.ts.How did you test this code?
Test rationale: fixtures catch scope regressions (label overrides, moved models, squashes, excluded databases), and a real-repo case keeps
workflows_teamworkflowsconfigin scope.replacesparsing matches Python'sastfor all 72 squash migrations in the repo.Release status
Docs update
safe-django-migrations.mdgains a rule: migrate before Node or Rust code reads the column.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Opus 5.5 (orchestrator and workflow subagents)
Skills:
/workflow-authoring,/writing-pr-descriptions,/writing-tests,/writing-code-commentsMore context for AI agents
statement_timeoutgoes throughset_configin a transaction, because PgBouncer rejects it as a startup parameter.Dockerfile.node,nodejs/src/schema-check/derive-requirements.ts, andpostgres-migration-store.ts.