diff --git a/.claude/skills/postgres-migrations/SKILL.md b/.claude/skills/postgres-migrations/SKILL.md new file mode 100644 index 0000000000..be1489db37 --- /dev/null +++ b/.claude/skills/postgres-migrations/SKILL.md @@ -0,0 +1,89 @@ +--- +name: postgres-migrations +allowed-tools: Read, Grep, Bash +description: How to author Postgres migrations in packages/postgres under the two-phase (additive vs removal) system — additive changes go in migrations/ and run pre-deploy; destructive changes (DROP COLUMN/TABLE, RENAME) go in migrations-removal/ and run post-deploy so they never break the previous code revision mid-rollout. Use whenever creating, editing, moving, or reviewing a migration in packages/postgres, deciding which directory a migration belongs in, or touching the boxel_index / boxel_index_working index tables. Triggers on adding a DB migration, a DROP/RENAME in a migration, or a review of one. +--- + +# Postgres migrations — the two-phase system + +`packages/postgres/` migrations are split into two directories, each with its own +node-pg-migrate tracking table, because a destructive schema change applied +during a rolling deploy breaks the **previous** code revision while it is still +serving (old tasks query a column the migration just dropped). + +| Directory | Tracking table | When it runs in a deploy | For | +| --------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------- | ------------------------------------- | +| `migrations/` | `migrations` | **pre**-deploy (`migrate-db` job) | additive, backward-compatible changes | +| `migrations-removal/` | `migrations_removal` | **post**-deploy (`migrate-db-remove` job), gated on the realm-server rollout reaching stability so old tasks have drained | destructive changes | + +## Which directory? + +Ask: **does the migration's `up()` remove or rename something the currently +deployed code still reads?** If yes → `migrations-removal/`. + +- **Additive → `migrations/`**: `addColumn`, `createTable`, new index, backfill, + dual-write. Safe for old code to run against. +- **Destructive → `migrations-removal/`**: `dropColumn` / `dropColumns` / + `dropTable` / `renameColumn` / `renameTable`, and the raw-SQL equivalents + (`pgm.sql('ALTER TABLE ... DROP COLUMN ...')`, `RENAME`). Also anything that + tightens a contract the old code depends on (e.g. adding `NOT NULL` to a + column old code doesn't populate, narrowing a type). + +The standard expand/contract pattern spans **two** releases: release N adds the +new shape and stops reading the old one (additive migration + code); release +N+1 drops the old shape (removal migration). Never ship the drop in the same +release that stops reading it unless you are certain no old task will run against +the new schema. + +## Creating a migration + +```sh +# additive (the common case) — writes to migrations/ +pnpm --filter @cardstack/postgres migrate create + +# destructive — writes straight to migrations-removal/ +pnpm --filter @cardstack/postgres migrate:create-removal +``` + +Filenames need a real `Date.now()` millisecond timestamp; `pnpm lint:migrations` +rejects any prefix with 6+ consecutive zeros. `migrate create` stamps a valid +one. Never hand-pick a round number. + +Migration files are CommonJS (`exports.up` / `exports.down`); both directories +carry a `package.json` pinning `{ "type": "commonjs" }`. + +## Enforcement + +`packages/postgres/scripts/check-removal-phase.cjs` (CI: "Guard removal-phase +migrations") AST-parses each **newly added** `migrations/` file and fails if its +`up()` drops or renames a column/table. It is scoped to changed files (so drops +already present in `migrations/` are grandfathered) and looks only at `up()` (an +additive migration's `down()` legitimately calls `dropColumn` to reverse +itself). It is heuristic — it will not catch `NOT NULL` tightening, type +narrowing, or destructive SQL built from non-literal strings — so the decision +rule above still applies. + +## Applying migrations locally / in CI + +`pnpm migrate up|down [count]|create` is transparent — the driver +(`scripts/migrate-local.sh`) runs both phases as one combined sequence +(additive first on `up`; `down [count]` reverts the N most-recent across both +phases, default 1). You do **not** think about the split when applying; only +when authoring a destructive change. + +## Gotchas + +- **`boxel_index` and `boxel_index_working` are twin tables that must stay + schema-identical.** The indexer does `SELECT * FROM boxel_index` and mirrors + the row shape into `boxel_index_working`, so any column add/drop must touch + **both** (the removal migration uses `TABLES = ['boxel_index', +'boxel_index_working']`). Changing only one breaks index writes with + `column "..." of relation "boxel_index_working" does not exist`. +- **Moving an already-applied migration between directories re-runs it** under + the new tracking table. Only safe if its `up()` is idempotent (`IF EXISTS` / + `ifNotExists`). Moving a not-yet-applied file is always clean. +- **Schema snapshot**: after any migration change run `pnpm make-schema`; the + host validates the committed `packages/host/config/schema/_schema.sql` + against the newest migration across **both** directories. +- `packages/realm-server/migrations/` is a **separate** migration system (not + this one); don't conflate them. diff --git a/.github/scripts/run-gated-migration-task.sh b/.github/scripts/run-gated-migration-task.sh new file mode 100755 index 0000000000..6a1d4d1f4b --- /dev/null +++ b/.github/scripts/run-gated-migration-task.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +# Run one DB-migration phase as a one-shot ECS task and gate on its exit code, +# so the deploy never proceeds past a failed migration. Shared by the +# migrate-db (expand) and migrate-db-remove (removal) jobs in manual-deploy.yml +# so both phases use identical, in-lockstep orchestration. +# +# The in-container command to run is passed as this script's arguments, e.g. +# run-gated-migration-task.sh ./scripts/run-migrations.sh +# run-gated-migration-task.sh ./scripts/run-migrations.sh migrations-removal migrations_removal +# +# Everything else comes from the job environment: +# CLUSTER SERVICE CONTAINER IMAGE LOG_GROUP TIMEOUT_SECONDS GITHUB_SHA +set -euo pipefail + +region=us-east-1 +console_logs="https://${region}.console.aws.amazon.com/cloudwatch/home?region=${region}#logsV2:log-groups/log-group/${LOG_GROUP}" + +# Register a new revision of the pg-migration task definition pinned to the +# image just built. Strip the read-only fields describe returns that +# register-task-definition rejects. +aws ecs describe-task-definition --task-definition "$SERVICE" \ + --query 'taskDefinition' --output json > td.json +jq --arg IMAGE "$IMAGE" --arg CONTAINER "$CONTAINER" ' + .containerDefinitions |= map(if .name == $CONTAINER then .image = $IMAGE else . end) + | del(.taskDefinitionArn, .revision, .status, .requiresAttributes, + .compatibilities, .registeredAt, .registeredBy, .deregisteredAt) +' td.json > td-new.json +task_def=$(aws ecs register-task-definition --cli-input-json file://td-new.json \ + --query 'taskDefinition.taskDefinitionArn' --output text) +echo "Registered migration task definition: $task_def" + +# Place the one-shot task in the pg-migration service's own network config — +# its security group is the one allowed to reach the DB. +net=$(aws ecs describe-services --cluster "$CLUSTER" --services "$SERVICE" \ + --query 'services[0].networkConfiguration.awsvpcConfiguration' --output json) +subnets=$(echo "$net" | jq -r '.subnets | join(",")') +groups=$(echo "$net" | jq -r '.securityGroups | join(",")') +public=$(echo "$net" | jq -r '.assignPublicIp // "DISABLED"') + +# Override the command to run the migrations WITHOUT the service's trailing +# `sleep infinity`, so the task exits with the migration's status instead of +# staying up. The command comes from this script's arguments. +overrides=$(jq -cn --arg CONTAINER "$CONTAINER" \ + '{containerOverrides: [{name: $CONTAINER, command: $ARGS.positional}]}' \ + --args "$@") +run_out=$(aws ecs run-task \ + --cluster "$CLUSTER" \ + --task-definition "$task_def" \ + --launch-type FARGATE \ + --count 1 \ + --started-by "deploy-${GITHUB_SHA:0:7}" \ + --network-configuration "awsvpcConfiguration={subnets=[$subnets],securityGroups=[$groups],assignPublicIp=$public}" \ + --overrides "$overrides" \ + --output json) +task_arn=$(echo "$run_out" | jq -r '.tasks[0].taskArn // empty') +if [ -z "$task_arn" ]; then + echo "::error::Failed to start the migration task." + echo "$run_out" | jq '.failures' + exit 1 +fi +echo "Started migration task: $task_arn" + +# Wait for the task to stop, then gate on the container's exit code. +deadline=$(( $(date +%s) + TIMEOUT_SECONDS )) +while :; do + status=$(aws ecs describe-tasks --cluster "$CLUSTER" --tasks "$task_arn" \ + --query 'tasks[0].lastStatus' --output text) + echo "migration task status: $status" + [ "$status" = "STOPPED" ] && break + if [ "$(date +%s)" -ge "$deadline" ]; then + echo "::error::Timed out after ${TIMEOUT_SECONDS}s waiting for the migration task to finish." + echo "Migration logs: ${console_logs}" + aws ecs stop-task --cluster "$CLUSTER" --task "$task_arn" \ + --reason "migration gate timeout" >/dev/null 2>&1 || true + exit 1 + fi + sleep 10 +done + +exit_code=$(aws ecs describe-tasks --cluster "$CLUSTER" --tasks "$task_arn" \ + --query "tasks[0].containers[?name=='${CONTAINER}']|[0].exitCode" --output text) +echo "migration container exit code: ${exit_code}" +if [ "$exit_code" != "0" ]; then + echo "::error::DB migration failed (exit ${exit_code}) — deploy blocked so the app never runs ahead of its schema." + aws ecs describe-tasks --cluster "$CLUSTER" --tasks "$task_arn" \ + --query "tasks[0].{stopCode:stopCode,taskReason:stoppedReason,containerReason:containers[?name=='${CONTAINER}']|[0].reason}" \ + --output table || true + echo "Migration logs: ${console_logs}" + exit 1 +fi +echo "DB migration succeeded." diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6e7f054846..43a8cce7b6 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -291,6 +291,17 @@ jobs: shell: bash run: packages/postgres/scripts/determine-changed-migrations.sh + # Fail fast if a newly added additive migration drops/renames a column or + # table in its up() — those belong in migrations-removal/ (post-deploy) so + # they don't break the previous code revision mid-rollout. + - name: Guard removal-phase migrations + if: steps.migrations.outputs.count && steps.migrations.outputs.count != '0' + env: + # Newline-separated file list; the guard reads it from this env var + # (no shell word-splitting, no multiline value inlined into the script). + CHANGED_MIGRATIONS: ${{ steps.migrations.outputs.files }} + run: node packages/postgres/scripts/check-removal-phase.cjs + - name: Apply migrations if: steps.migrations.outputs.count && steps.migrations.outputs.count != '0' working-directory: packages/postgres diff --git a/.github/workflows/manual-deploy.yml b/.github/workflows/manual-deploy.yml index eaecb2421e..8b3c4718ea 100644 --- a/.github/workflows/manual-deploy.yml +++ b/.github/workflows/manual-deploy.yml @@ -237,6 +237,9 @@ jobs: # migration must finish under it. Genuine failures exit immediately. TIMEOUT_SECONDS: "1800" steps: + - name: Check out + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Set up env run: | if [ "${{ inputs.environment }}" = "production" ]; then @@ -255,84 +258,7 @@ jobs: aws-region: us-east-1 - name: Run migrations and gate on exit code - run: | - set -euo pipefail - region=us-east-1 - console_logs="https://${region}.console.aws.amazon.com/cloudwatch/home?region=${region}#logsV2:log-groups/log-group/${LOG_GROUP}" - - # Register a new revision of the pg-migration task definition pinned to - # the image just built. Strip the read-only fields describe returns - # that register-task-definition rejects. - aws ecs describe-task-definition --task-definition "$SERVICE" \ - --query 'taskDefinition' --output json > td.json - jq --arg IMAGE "$IMAGE" --arg CONTAINER "$CONTAINER" ' - .containerDefinitions |= map(if .name == $CONTAINER then .image = $IMAGE else . end) - | del(.taskDefinitionArn, .revision, .status, .requiresAttributes, - .compatibilities, .registeredAt, .registeredBy, .deregisteredAt) - ' td.json > td-new.json - task_def=$(aws ecs register-task-definition --cli-input-json file://td-new.json \ - --query 'taskDefinition.taskDefinitionArn' --output text) - echo "Registered migration task definition: $task_def" - - # Place the one-shot task in the pg-migration service's own network - # config — its security group is the one allowed to reach the DB. - net=$(aws ecs describe-services --cluster "$CLUSTER" --services "$SERVICE" \ - --query 'services[0].networkConfiguration.awsvpcConfiguration' --output json) - subnets=$(echo "$net" | jq -r '.subnets | join(",")') - groups=$(echo "$net" | jq -r '.securityGroups | join(",")') - public=$(echo "$net" | jq -r '.assignPublicIp // "DISABLED"') - - # Override the command to run the migrations WITHOUT the service's - # trailing `sleep infinity`, so the task exits with the migration's - # status instead of staying up. - overrides=$(jq -cn --arg CONTAINER "$CONTAINER" \ - '{containerOverrides: [{name: $CONTAINER, command: ["./scripts/run-migrations.sh"]}]}') - run_out=$(aws ecs run-task \ - --cluster "$CLUSTER" \ - --task-definition "$task_def" \ - --launch-type FARGATE \ - --count 1 \ - --started-by "deploy-${GITHUB_SHA:0:7}" \ - --network-configuration "awsvpcConfiguration={subnets=[$subnets],securityGroups=[$groups],assignPublicIp=$public}" \ - --overrides "$overrides" \ - --output json) - task_arn=$(echo "$run_out" | jq -r '.tasks[0].taskArn // empty') - if [ -z "$task_arn" ]; then - echo "::error::Failed to start the migration task." - echo "$run_out" | jq '.failures' - exit 1 - fi - echo "Started migration task: $task_arn" - - # Wait for the task to stop, then gate on the container's exit code. - deadline=$(( $(date +%s) + TIMEOUT_SECONDS )) - while :; do - status=$(aws ecs describe-tasks --cluster "$CLUSTER" --tasks "$task_arn" \ - --query 'tasks[0].lastStatus' --output text) - echo "migration task status: $status" - [ "$status" = "STOPPED" ] && break - if [ "$(date +%s)" -ge "$deadline" ]; then - echo "::error::Timed out after ${TIMEOUT_SECONDS}s waiting for the migration task to finish." - echo "Migration logs: ${console_logs}" - aws ecs stop-task --cluster "$CLUSTER" --task "$task_arn" \ - --reason "migration gate timeout" >/dev/null 2>&1 || true - exit 1 - fi - sleep 10 - done - - exit_code=$(aws ecs describe-tasks --cluster "$CLUSTER" --tasks "$task_arn" \ - --query "tasks[0].containers[?name=='${CONTAINER}']|[0].exitCode" --output text) - echo "migration container exit code: ${exit_code}" - if [ "$exit_code" != "0" ]; then - echo "::error::DB migration failed (exit ${exit_code}) — deploy blocked so the app never runs ahead of its schema." - aws ecs describe-tasks --cluster "$CLUSTER" --tasks "$task_arn" \ - --query "tasks[0].{stopCode:stopCode,taskReason:stoppedReason,containerReason:containers[?name=='${CONTAINER}']|[0].reason}" \ - --output table || true - echo "Migration logs: ${console_logs}" - exit 1 - fi - echo "DB migration succeeded." + run: .github/scripts/run-gated-migration-task.sh ./scripts/run-migrations.sh deploy-prerender: name: Deploy prerender @@ -364,8 +290,7 @@ jobs: deploy-worker: name: Deploy worker - needs: - [build-worker, deploy-host, migrate-db, deploy-prerender-manager] + needs: [build-worker, deploy-host, migrate-db, deploy-prerender-manager] uses: cardstack/gh-actions/.github/workflows/ecs-deploy.yml@main secrets: inherit with: @@ -388,8 +313,7 @@ jobs: deploy-realm-server: name: Deploy realm server - needs: - [post-deploy-worker, build-realm-server, deploy-host, migrate-db] + needs: [post-deploy-worker, build-realm-server, deploy-host, migrate-db] uses: cardstack/gh-actions/.github/workflows/ecs-deploy.yml@main secrets: inherit with: @@ -432,6 +356,62 @@ jobs: exit 1 fi + # Destructive ("removal") migrations — column/table drops and renames, held in + # packages/postgres/migrations-removal/ — run HERE, after deploy-realm-server + # has reached stability. `wait-for-service-stability: true` on that job means + # the previous realm-server task set has fully drained (and, via its + # post-deploy-worker dependency, the previous worker task set too), so no task + # on the old code revision is left to query a column this phase drops. That is + # the whole point: running drops before the old tasks drain is what takes the + # published realms down (old code 500s on `column i. does not exist`). + # + # A failure here is non-fatal to serving: the new code, by definition, no + # longer reads the removed columns, so it is already live and healthy — the + # drop simply hasn't happened yet and can be retried on the next deploy. + # + # Gate on deploy-realm-server, NOT post-deploy-realm-server: the latter is a + # separate /_post-deployment hook that can fail independently of the only real + # precondition here — the old tasks being gone. + migrate-db-remove: + needs: [build-pg-migration, deploy-realm-server] + name: Run DB removal migrations + runs-on: ubuntu-latest + permissions: + contents: read + id-token: write + env: + CLUSTER: ${{ inputs.environment }} + SERVICE: boxel-pg-migration-${{ inputs.environment }} + CONTAINER: boxel-pg-migration + IMAGE: ${{ needs.build-pg-migration.outputs.image }} + LOG_GROUP: ecs-boxel-pg-migration-${{ inputs.environment }} + # Bounds a migration that hangs (e.g. blocked on a lock); a long backfill + # migration must finish under it. Genuine failures exit immediately. + TIMEOUT_SECONDS: "1800" + steps: + - name: Check out + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Set up env + run: | + if [ "${{ inputs.environment }}" = "production" ]; then + echo "AWS_ROLE_ARN=arn:aws:iam::120317779495:role/github" >> "$GITHUB_ENV" + elif [ "${{ inputs.environment }}" = "staging" ]; then + echo "AWS_ROLE_ARN=arn:aws:iam::680542703984:role/github" >> "$GITHUB_ENV" + else + echo "unrecognized environment: ${{ inputs.environment }}" + exit 1 + fi + + - name: Configure AWS credentials + uses: aws-actions/configure-aws-credentials@v4 + with: + role-to-assume: ${{ env.AWS_ROLE_ARN }} + aws-region: us-east-1 + + - name: Run removal migrations and gate on exit code + run: .github/scripts/run-gated-migration-task.sh ./scripts/run-migrations.sh migrations-removal migrations_removal + recycle-prerender: name: Recycle prerender after host is live # The prerender fleet deploys before the realm server (the manager, @@ -500,6 +480,7 @@ jobs: post-deploy-worker, deploy-realm-server, post-deploy-realm-server, + migrate-db-remove, recycle-prerender, apply-observability, ] diff --git a/packages/host/config/environment.js b/packages/host/config/environment.js index 4e7d56cf03..07b387ef5f 100644 --- a/packages/host/config/environment.js +++ b/packages/host/config/environment.js @@ -247,9 +247,17 @@ function getLatestSchemaFile() { const migrationsDir = path.resolve( path.join(__dirname, '..', '..', 'postgres', 'migrations'), ); - let migrations = fs.readdirSync(migrationsDir); - // Only timestamped migration files — ignores non-migration entries in the dir - // such as `package.json` (pins the dir to type:commonjs) and `.eslintrc.js`. + const removalMigrationsDir = path.resolve( + path.join(__dirname, '..', '..', 'postgres', 'migrations-removal'), + ); + // Latest timestamped migration across both phases (additive in migrations/, + // destructive in migrations-removal/). Only timestamped migration files count + // — ignores non-migration entries such as `package.json` (pins the dir to + // type:commonjs) and `.eslintrc.js`. + let migrations = [ + ...fs.readdirSync(migrationsDir), + ...fs.readdirSync(removalMigrationsDir), + ]; let lastMigration = migrations .filter((f) => /^\d+_/.test(f)) .sort() diff --git a/packages/postgres/README.md b/packages/postgres/README.md new file mode 100644 index 0000000000..43eced4022 --- /dev/null +++ b/packages/postgres/README.md @@ -0,0 +1,72 @@ +# @cardstack/postgres + +Postgres schema and migrations for the boxel stack. + +## Migrations run in two phases + +Migrations live in two directories, each with its own +[node-pg-migrate](https://github.com/salsita/node-pg-migrate) tracking table: + +| Directory | Tracking table | Runs during a deploy | For | +| --------------------- | -------------------- | ---------------------------------------------------------------------------------- | ------------------------------------- | +| `migrations/` | `migrations` | **before** the app rolls out (`migrate-db`) | additive, backward-compatible changes | +| `migrations-removal/` | `migrations_removal` | **after** the app rolls out (`migrate-db-remove`), once the old tasks have drained | destructive changes | + +**Why:** a destructive change applied during a rolling deploy breaks the +_previous_ code revision while it's still serving — the old tasks query a column +the migration just dropped and every request 500s until they drain. Deferring +drops/renames until after the new code is fully live avoids this. The +`migrate-db-remove` job is gated on the realm-server rollout reaching stability, +so by the time a drop runs there is no task left on the old code. + +### Which directory does my migration go in? + +Ask: **does `up()` remove or rename something the deployed code still reads?** + +- **No → `migrations/`** — `addColumn`, `createTable`, new index, backfill, etc. +- **Yes → `migrations-removal/`** — `dropColumn`/`dropTable`/`renameColumn`/ + `renameTable` (and the raw-SQL equivalents), or anything that tightens a + contract old code depends on (adding `NOT NULL` to a column old code doesn't + populate, narrowing a type). + +The standard expand/contract pattern spans two releases: release N adds the new +shape and stops reading the old one; release N+1 drops the old shape. + +### Creating a migration + +```sh +# additive (the common case) → writes to migrations/ +pnpm --filter @cardstack/postgres migrate create + +# destructive → writes to migrations-removal/ +pnpm --filter @cardstack/postgres migrate:create-removal +``` + +Filenames must use a real `Date.now()` timestamp; `pnpm lint:migrations` rejects +prefixes with 6+ consecutive zeros. `migrate create` stamps a valid one. + +A CI check (`scripts/check-removal-phase.cjs`) fails the build if a newly added +`migrations/` migration drops or renames a column/table in its `up()`, pointing +you to `migrations-removal/`. It's scoped to changed files (drops already in +`migrations/` are grandfathered) and inspects only `up()` — an additive +migration's `down()` may call `dropColumn` to reverse itself. + +### Applying migrations + +`pnpm migrate up | down [count] | create` is transparent — the driver +(`scripts/migrate-local.sh`) runs both phases as one combined sequence. `up` +applies additive then removal; `down [count]` reverts the N most-recent +migrations across both phases (default 1). You only think about the split when +_authoring_ a destructive change, not when applying. + +### Gotchas + +- **`boxel_index` and `boxel_index_working` are twin tables and must stay + schema-identical** — the indexer does `SELECT * FROM boxel_index` and mirrors + the row shape into `boxel_index_working`. Any column change must touch both. +- **Moving an already-applied migration between directories re-runs it** under + the new tracking table; only safe if `up()` is idempotent (`IF EXISTS` / + `ifNotExists`). Moving a not-yet-applied file is always clean. +- After changing migrations, run `pnpm make-schema` to regenerate the SQLite + schema snapshot the host validates. +- `packages/realm-server/migrations/` is a separate migration system. diff --git a/packages/postgres/migrations-removal/.eslintrc.js b/packages/postgres/migrations-removal/.eslintrc.js new file mode 100644 index 0000000000..9cd5fbd2a6 --- /dev/null +++ b/packages/postgres/migrations-removal/.eslintrc.js @@ -0,0 +1,17 @@ +'use strict'; + +module.exports = { + root: true, + parser: '@typescript-eslint/parser', + parserOptions: { + sourceType: 'script', + }, + env: { + browser: false, + node: true, + }, + extends: ['plugin:n/recommended'], + rules: { + camelcase: 'off', + }, +}; diff --git a/packages/postgres/migrations/1783724976754_drop-boxel-index-html-columns.js b/packages/postgres/migrations-removal/1783724976754_drop-boxel-index-html-columns.js similarity index 100% rename from packages/postgres/migrations/1783724976754_drop-boxel-index-html-columns.js rename to packages/postgres/migrations-removal/1783724976754_drop-boxel-index-html-columns.js diff --git a/packages/postgres/migrations-removal/package.json b/packages/postgres/migrations-removal/package.json new file mode 100644 index 0000000000..5bbefffbab --- /dev/null +++ b/packages/postgres/migrations-removal/package.json @@ -0,0 +1,3 @@ +{ + "type": "commonjs" +} diff --git a/packages/postgres/package.json b/packages/postgres/package.json index d27148be11..4aa3a3de72 100644 --- a/packages/postgres/package.json +++ b/packages/postgres/package.json @@ -21,12 +21,14 @@ "@cardstack/local-types": "workspace:*", "@glint/ember-tsc": "catalog:", "concurrently": "catalog:", - "sql-parser-cst": "catalog:" + "sql-parser-cst": "catalog:", + "typescript": "catalog:" }, "scripts": { "start:pg": "./scripts/start-pg.sh", "stop:pg": "./scripts/stop-pg.sh", - "migrate": "PGDATABASE=boxel ./scripts/ensure-db-exists.sh && PGPORT=5435 PGDATABASE=boxel PGUSER=postgres node ./scripts/fix-migration-names.ts && PGPORT=5435 PGDATABASE=boxel PGUSER=postgres node-pg-migrate --migrations-table migrations --no-check-order --ignore-pattern '.*\\.eslintrc\\.js|package\\.json' --verbose=false", + "migrate": "PGDATABASE=boxel ./scripts/ensure-db-exists.sh && PGPORT=5435 PGDATABASE=boxel PGUSER=postgres node ./scripts/fix-migration-names.ts && PGPORT=5435 PGDATABASE=boxel PGUSER=postgres ./scripts/migrate-local.sh", + "migrate:create-removal": "./scripts/migrate-local.sh create-removal", "make-schema": "./scripts/schema-dump.sh", "drop-db": "docker exec boxel-pg dropdb --if-exists -U postgres -w", "lint": "concurrently \"pnpm:lint:*(!fix)\" --names \"lint:\"", diff --git a/packages/postgres/scripts/check-removal-phase.cjs b/packages/postgres/scripts/check-removal-phase.cjs new file mode 100755 index 0000000000..bca61ff4c5 --- /dev/null +++ b/packages/postgres/scripts/check-removal-phase.cjs @@ -0,0 +1,168 @@ +#!/usr/bin/env node +/* eslint-env node */ +/* eslint-disable @typescript-eslint/no-var-requires */ +'use strict'; + +// Guard: a migration added to `migrations/` (the additive phase) must not drop +// or rename a column/table in its `up()`. A destructive change applied during a +// rolling deploy breaks the previous code revision while it is still serving — +// old tasks query a column the migration just removed. Such changes belong in +// `migrations-removal/`, which runs post-deploy once the old tasks have drained. +// See scripts/migrate-local.sh and the migrate-db-remove job in +// .github/workflows/manual-deploy.yml. +// +// Scoped to the migration files passed as arguments (the CI step feeds the +// changed files from determine-changed-migrations.sh), so it only checks newly +// added migrations, not the drops already present in `migrations/`. +// +// AST-based (via the TypeScript parser) rather than grep so it can look ONLY at +// the `up` function: an additive migration's `down()` legitimately calls +// dropColumn/dropTable to reverse itself, and a text search can't tell the two +// apart. Heuristic by design: catches column/table DROP and RENAME (what breaks +// old code mid-rollout), not NOT-NULL tightening, type narrowing, or destructive +// SQL assembled from non-literal strings. + +const fs = require('fs'); +const path = require('path'); + +let ts; +try { + ts = require('typescript'); +} catch { + console.error( + 'check-removal-phase: the `typescript` package is required to parse migrations but could not be resolved.', + ); + process.exit(1); +} + +// pgm builder methods that drop or rename a column/table. +const DESTRUCTIVE_METHODS = new Set([ + 'dropColumn', + 'dropColumns', + 'dropTable', + 'renameColumn', + 'renameTable', +]); +// Raw-SQL escape hatch: pgm.sql('... DROP COLUMN ...'), etc. +const DESTRUCTIVE_SQL = /\bdrop\s+(column|table)\b|\brename\s+(column\b|to\b)/i; + +// Only guard the additive phase. migrations-removal/ is where drops belong. +const ADDITIVE_DIR = `${path.sep}migrations${path.sep}`; + +function findUpFunction(sourceFile) { + let upFn = null; + (function visit(node) { + if ( + ts.isBinaryExpression(node) && + node.operatorToken.kind === ts.SyntaxKind.EqualsToken && + ts.isPropertyAccessExpression(node.left) && + node.left.name.text === 'up' + ) { + const obj = node.left.expression; + const isExports = + (ts.isIdentifier(obj) && obj.text === 'exports') || + (ts.isPropertyAccessExpression(obj) && obj.name.text === 'exports'); + if ( + isExports && + (ts.isArrowFunction(node.right) || ts.isFunctionExpression(node.right)) + ) { + upFn = node.right; + } + } + ts.forEachChild(node, visit); + })(sourceFile); + return upFn; +} + +function destructiveOpsInUp(upFn, sourceFile) { + const ops = new Set(); + (function visit(node) { + if ( + ts.isCallExpression(node) && + ts.isPropertyAccessExpression(node.expression) + ) { + const recv = node.expression.expression; + const onPgm = ts.isIdentifier(recv) && recv.text === 'pgm'; + const method = node.expression.name.text; + if (onPgm && DESTRUCTIVE_METHODS.has(method)) { + ops.add(method); + } + if (onPgm && method === 'sql' && node.arguments.length > 0) { + const arg = node.arguments[0]; + let sqlText = ''; + if (ts.isStringLiteralLike(arg)) { + sqlText = arg.text; + } else if ( + ts.isTemplateExpression(arg) || + ts.isNoSubstitutionTemplateLiteral(arg) + ) { + sqlText = arg.getText(sourceFile); + } + if (DESTRUCTIVE_SQL.test(sqlText)) { + ops.add('sql (DROP/RENAME)'); + } + } + } + ts.forEachChild(node, visit); + })(upFn.body); + return [...ops]; +} + +// Files come from argv (local use) and/or the CHANGED_MIGRATIONS env var (the CI +// step feeds determine-changed-migrations.sh's newline-separated list this way, +// so the workflow doesn't rely on shell word-splitting). +const files = [ + ...process.argv.slice(2), + ...(process.env.CHANGED_MIGRATIONS || '').split(/\s+/), +] + .map((f) => f.trim()) + .filter((f) => f && /^\d+_.*\.(js|ts)$/.test(path.basename(f))) + // Only the additive phase; drops in migrations-removal/ are the whole point. + .filter((f) => f.includes(ADDITIVE_DIR) && !f.includes('migrations-removal')); + +const violations = []; +for (const file of files) { + if (!fs.existsSync(file)) { + continue; // renamed/deleted in the diff — nothing to check + } + const src = fs.readFileSync(file, 'utf8'); + const sourceFile = ts.createSourceFile( + file, + src, + ts.ScriptTarget.Latest, + /* setParentNodes */ true, + ts.ScriptKind.JS, + ); + const upFn = findUpFunction(sourceFile); + if (!upFn) { + continue; + } + const ops = destructiveOpsInUp(upFn, sourceFile); + if (ops.length) { + violations.push({ file, ops }); + } +} + +if (violations.length) { + console.error( + 'Destructive DDL found in the up() of an additive migration:\n', + ); + for (const { file, ops } of violations) { + console.error(` ${file}`); + console.error(` → ${ops.join(', ')}`); + } + console.error( + '\nColumn/table drops and renames break the previous code revision during a' + + '\nrolling deploy (old tasks query a column the migration just removed).' + + '\nMove this migration to packages/postgres/migrations-removal/, which runs' + + '\npost-deploy once the old tasks have drained:' + + '\n\n pnpm --filter @cardstack/postgres migrate:create-removal ' + + '\n\nor `git mv` the file there (it is tracked by filename, so moving a' + + '\nnot-yet-applied migration is clean).', + ); + process.exit(1); +} + +console.log( + `check-removal-phase: no destructive DDL in ${files.length} additive migration(s) checked.`, +); diff --git a/packages/postgres/scripts/convert-to-sqlite.ts b/packages/postgres/scripts/convert-to-sqlite.ts index f5df2aaf21..2b071923e4 100755 --- a/packages/postgres/scripts/convert-to-sqlite.ts +++ b/packages/postgres/scripts/convert-to-sqlite.ts @@ -17,6 +17,9 @@ import { const args = process.argv; const migrationsDir = resolve(join(import.meta.dirname, '..', 'migrations')); +const removalMigrationsDir = resolve( + join(import.meta.dirname, '..', 'migrations-removal'), +); const sqliteSchemaDir = resolve( join(import.meta.dirname, '..', '..', 'host', 'config', 'schema'), ); @@ -290,11 +293,17 @@ function prepareDump(sql: string): string { } function getSchemaFilename(): string { - let files = readdirSync(migrationsDir); - // Only timestamped migration files — ignores non-migration entries in the - // dir such as `package.json` (pins the dir to type:commonjs) and - // `.eslintrc.js`. Must stay in sync with getLatestSchemaFile in - // packages/host/config/environment.js, which validates the schema file name. + // Latest timestamped migration across BOTH phases: additive migrations in + // migrations/ and destructive ones in migrations-removal/ (applied + // post-deploy). Only timestamped migration files count — ignores + // non-migration entries such as `package.json` (pins the dir to + // type:commonjs) and `.eslintrc.js`. Must stay in sync with + // getLatestSchemaFile in packages/host/config/environment.js, which validates + // the schema file name. + let files = [ + ...readdirSync(migrationsDir), + ...readdirSync(removalMigrationsDir), + ]; let lastFile = files .filter((f) => /^\d+_/.test(f)) .sort() diff --git a/packages/postgres/scripts/determine-changed-migrations.sh b/packages/postgres/scripts/determine-changed-migrations.sh index c895b4396a..19bbfbee6e 100755 --- a/packages/postgres/scripts/determine-changed-migrations.sh +++ b/packages/postgres/scripts/determine-changed-migrations.sh @@ -35,7 +35,7 @@ fi echo "Using base SHA $BASE_SHA" -CHANGED="$(git diff --name-only --diff-filter=AM "$BASE_SHA" "$TARGET_SHA" -- 'packages/postgres/migrations/*.js' 'packages/postgres/migrations/*.ts')" +CHANGED="$(git diff --name-only --diff-filter=AM "$BASE_SHA" "$TARGET_SHA" -- 'packages/postgres/migrations/*.js' 'packages/postgres/migrations/*.ts' 'packages/postgres/migrations-removal/*.js' 'packages/postgres/migrations-removal/*.ts')" echo "$CHANGED" COUNT="$(printf '%s\n' "$CHANGED" | awk 'NF' | wc -l | tr -d ' ')" @@ -61,10 +61,12 @@ printf '%s\n' "$SORTED" # few extra seconds per CI run but exercises every DOWN/UP cycle on # every migration-touching PR. # Match node-pg-migrate's own discovery: only timestamp-prefixed files -# count as migrations (excludes .eslintrc.js, README, etc.). -TOTAL="$(find packages/postgres/migrations -maxdepth 1 -type f \( -name '[0-9]*.js' -o -name '[0-9]*.ts' \) | wc -l | tr -d ' ')" +# count as migrations (excludes .eslintrc.js, README, etc.). Count BOTH phases +# (migrations/ + migrations-removal/) so the count reverts the entire combined +# chain — `pnpm migrate down` reverts that many across both phases. +TOTAL="$(find packages/postgres/migrations packages/postgres/migrations-removal -maxdepth 1 -type f \( -name '[0-9]*.js' -o -name '[0-9]*.ts' \) | wc -l | tr -d ' ')" if [[ "$TOTAL" -eq 0 ]]; then - echo "No migrations found in packages/postgres/migrations" >&2 + echo "No migrations found in packages/postgres/migrations or migrations-removal" >&2 exit 1 fi diff --git a/packages/postgres/scripts/lint-migrations.cjs b/packages/postgres/scripts/lint-migrations.cjs index e64386706e..128c5c69c3 100755 --- a/packages/postgres/scripts/lint-migrations.cjs +++ b/packages/postgres/scripts/lint-migrations.cjs @@ -6,11 +6,18 @@ const fs = require('fs'); const path = require('path'); -const migrationsDir = path.join(__dirname, '..', 'migrations'); -const files = fs - .readdirSync(migrationsDir) - .filter((file) => file.endsWith('.js')) - .sort(); +// Both phases: additive migrations in migrations/ and destructive ones in +// migrations-removal/ (applied post-deploy). +const migrationDirs = [ + path.join(__dirname, '..', 'migrations'), + path.join(__dirname, '..', 'migrations-removal'), +]; +const files = migrationDirs.flatMap((dir) => + fs + .readdirSync(dir) + .filter((file) => file.endsWith('.js')) + .sort(), +); const suspicious = []; diff --git a/packages/postgres/scripts/migrate-local.sh b/packages/postgres/scripts/migrate-local.sh new file mode 100755 index 0000000000..171b15fece --- /dev/null +++ b/packages/postgres/scripts/migrate-local.sh @@ -0,0 +1,104 @@ +#!/usr/bin/env bash +# Local/CI migration driver. Applies (or rolls back) BOTH migration phases: +# migrations/ additive, backward-compatible changes +# migrations-removal/ destructive changes (column/table drops, renames) +# +# Deployed environments run these phases at different points in the rollout so a +# drop never executes while the previous code revision is still serving (see the +# migrate-db and migrate-db-remove jobs in .github/workflows/manual-deploy.yml). +# Locally and in CI there is no rolling deploy, so both phases run together — the +# removal phase LAST on `up` (drops happen after the additive changes they +# depend on) and FIRST on `down` (drops are undone before the tables they touch +# are removed), which keeps the combined sequence a valid linear order. +# +# The action (up | down [count] | create | redo ...) is forwarded from +# the caller, e.g. `pnpm migrate up`, `pnpm migrate down "$COUNT"`. `down [count]` +# reverts the `count` most-recently-applied migrations across the COMBINED +# newest-first timeline of both phases (count defaults to 1, matching +# node-pg-migrate) — so it never reverts more than asked, and a full-chain +# rollback (CI's reversibility test) still undoes everything. `create` is +# scaffolding, not application, so it targets the additive phase only (see the +# create case below). +set -euo pipefail + +cd "$(dirname "$0")/.." + +IGNORE='.*\.eslintrc\.js|package\.json' + +# node-pg-migrate for one phase: mig [extra-args...] +mig() { + ./node_modules/.bin/node-pg-migrate \ + --migrations-dir "$1" \ + --migrations-table "$2" \ + --no-check-order \ + --ignore-pattern "$IGNORE" \ + --verbose=false \ + "${@:3}" +} + +# Roll back the $2 most-recent migrations of phase $1 (additive|removal). +rollback_run() { + [ "$2" -eq 0 ] && return 0 + if [ "$1" = removal ]; then + mig migrations-removal migrations_removal down "$2" + else + mig migrations migrations down "$2" + fi +} + +action="${1:-up}" +shift || true + +case "$action" in + create) + # Scaffolding is additive by default: `pnpm migrate create ` writes a + # single file to migrations/. For a destructive migration use + # `create-removal` (below), which writes to migrations-removal/. Running + # `create` against both phases would emit a spurious second migration. + mig migrations migrations create "$@" + ;; + create-removal) + # Scaffold a destructive migration (DROP COLUMN/TABLE, RENAME) directly into + # migrations-removal/, which runs post-deploy so the drop never executes + # while the previous code revision is still serving. + mig migrations-removal migrations_removal create "$@" + ;; + down) + # Revert the requested count across the combined newest-first timeline of + # both phases, not the whole removal phase. Read both tracking tables, order + # by application time, take the top $count, and roll each back from its own + # phase — coalescing consecutive same-phase entries into one `down K` so a + # full-chain rollback (CI) stays ~2 calls, not one per migration. Uses the + # DB (not filenames) as the source of truth because --no-check-order lets + # migrations apply out of filename order. + count="${1:-1}" + applied_phases=$(psql -h "${PGHOST:-localhost}" -At -c " + SELECT phase FROM ( + SELECT 'additive' AS phase, name, run_on FROM migrations + UNION ALL + SELECT 'removal' AS phase, name, run_on FROM migrations_removal + ) applied + ORDER BY run_on DESC, name DESC + LIMIT ${count}") || { + echo "migrate-local.sh: failed to read migration tracking tables" >&2 + exit 1 + } + phase="" run=0 + while read -r p; do + [ -z "$p" ] && continue + if [ -n "$phase" ] && [ "$p" != "$phase" ]; then + rollback_run "$phase" "$run" + run=0 + fi + phase="$p" + run=$((run + 1)) + done <<< "$applied_phases" + rollback_run "$phase" "$run" + ;; + *) + # Apply actions (up, redo, …) run against both phases: additive first, then + # removal. + mig migrations migrations "$action" "$@" + mig migrations-removal migrations_removal "$action" "$@" + ;; +esac diff --git a/packages/postgres/scripts/run-migrations.sh b/packages/postgres/scripts/run-migrations.sh index f80b6fd267..d0b5678b17 100755 --- a/packages/postgres/scripts/run-migrations.sh +++ b/packages/postgres/scripts/run-migrations.sh @@ -1,15 +1,35 @@ #!/bin/sh -# Apply all pending DB migrations and exit with node-pg-migrate's status, so a -# failing migration surfaces as a non-zero exit. Run from the image's WORKDIR -# (/boxel/packages/postgres). Used two ways: -# - the pg-migration container's CMD wraps this in `... && sleep infinity` so -# the long-lived service stays up after a successful migrate -# - the deploy runs it as a one-shot task (command override, no sleep) and -# gates the rest of the rollout on its exit code +# Apply pending DB migrations for one phase and exit with node-pg-migrate's +# status, so a failing migration surfaces as a non-zero exit. Run from the +# image's WORKDIR (/boxel/packages/postgres). +# +# Two phases, each with its own migrations directory and tracking table: +# run-migrations.sh → migrations/ (table: migrations) +# run-migrations.sh migrations-removal migrations_removal +# +# The default phase (migrations/) holds additive, backward-compatible changes +# and runs BEFORE the app rolls out. The removal phase (migrations-removal/) +# holds destructive changes — column/table drops and renames — and runs only +# AFTER the new app is fully live (see the migrate-db-remove job in +# manual-deploy.yml), so a drop never executes while a task on the previous +# code revision is still querying the column. +# +# Used two ways in either phase: +# - the pg-migration container's CMD wraps the default phase in +# `... && sleep infinity` so the long-lived service stays up after a +# successful migrate +# - the deploy runs each phase as a one-shot task (command override, no +# sleep) and gates the rollout on its exit code set -e +DIR="${1:-migrations}" +TABLE="${2:-migrations}" + # Normalize legacy migration filenames before running (see fix-migration-names). -node ./scripts/fix-migration-names.ts +# Only the default migrations directory has legacy names to repair. +if [ "$DIR" = "migrations" ]; then + node ./scripts/fix-migration-names.ts +fi # --ignore-pattern keeps node-pg-migrate from treating non-migration files in # the migrations directory (the `{ "type": "commonjs" }` package.json that pins @@ -19,6 +39,7 @@ node ./scripts/fix-migration-names.ts # and ignored by default; package.json is not, so it must be listed explicitly.) exec ./node_modules/.bin/node-pg-migrate \ --check-order false \ - --migrations-table migrations \ + --migrations-dir "$DIR" \ + --migrations-table "$TABLE" \ --ignore-pattern '.*\.eslintrc\.js|package\.json' \ up diff --git a/packages/postgres/scripts/schema-dump.sh b/packages/postgres/scripts/schema-dump.sh index e947ea3e59..1f98c469b4 100755 --- a/packages/postgres/scripts/schema-dump.sh +++ b/packages/postgres/scripts/schema-dump.sh @@ -15,6 +15,7 @@ docker exec boxel-pg pg_dump \ -U postgres -w --schema-only \ --exclude-table-and-children=pgmigrations \ --exclude-table-and-children=migrations \ + --exclude-table-and-children=migrations_removal \ --exclude-table-and-children=job_statuses \ --exclude-table-and-children=jobs \ --exclude-table-and-children=queues \ diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index e9934eb5e3..adf5dd9e31 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2549,6 +2549,9 @@ importers: sql-parser-cst: specifier: 'catalog:' version: 0.28.1 + typescript: + specifier: 'catalog:' + version: 5.9.3 packages/realm-server: dependencies: