From 26b845e1b1950636c435ccd471cb2c5ffd4becb8 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Wed, 15 Jul 2026 15:11:15 -0500 Subject: [PATCH 1/8] Gate destructive DB migrations behind a post-deploy removal phase Migrations run before the app rolls out, so a destructive change to boxel_index (a column/table drop or rename) takes effect while the previous realm-server revision is still serving. During the rolling deploy window that old code queries the now-missing column and every published-realm serve 500s until the old tasks drain. Split migrations into two phases: - migrations/ additive, backward-compatible; runs pre-deploy (migrate-db), as before - migrations-removal/ destructive; runs post-deploy (migrate-db-remove), gated on deploy-realm-server reaching stability so the previous realm-server and worker task sets have fully drained before any drop executes A failure in the removal phase is non-fatal to serving: the new code no longer reads the removed columns, so it is already live and healthy. Each phase has its own directory and node-pg-migrate tracking table. The gated one-shot-ECS-task orchestration is extracted to .github/scripts/run-gated-migration-task.sh so both jobs stay in lockstep. run-migrations.sh takes an optional (dir, table); the local/CI driver migrate-local.sh runs both phases together (removal last on up, first on down). Schema-filename derivation and validation, the migration linter, the changed-migration detector, and the schema dump's excluded bookkeeping tables all now account for both directories. The already-applied drop-boxel-index-html-columns migration moves into the removal phase; its up is idempotent, so re-running under the new tracking table is a no-op where the columns are already gone. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/run-gated-migration-task.sh | 91 +++++++++++ .github/workflows/manual-deploy.yml | 145 ++++++++---------- packages/host/config/environment.js | 14 +- .../postgres/migrations-removal/.eslintrc.js | 17 ++ ...724976754_drop-boxel-index-html-columns.js | 0 .../postgres/migrations-removal/package.json | 3 + packages/postgres/package.json | 2 +- .../postgres/scripts/convert-to-sqlite.ts | 19 ++- .../scripts/determine-changed-migrations.sh | 2 +- packages/postgres/scripts/lint-migrations.cjs | 17 +- packages/postgres/scripts/migrate-local.sh | 49 ++++++ packages/postgres/scripts/run-migrations.sh | 39 +++-- packages/postgres/scripts/schema-dump.sh | 1 + 13 files changed, 293 insertions(+), 106 deletions(-) create mode 100755 .github/scripts/run-gated-migration-task.sh create mode 100644 packages/postgres/migrations-removal/.eslintrc.js rename packages/postgres/{migrations => migrations-removal}/1783724976754_drop-boxel-index-html-columns.js (100%) create mode 100644 packages/postgres/migrations-removal/package.json create mode 100755 packages/postgres/scripts/migrate-local.sh diff --git a/.github/scripts/run-gated-migration-task.sh b/.github/scripts/run-gated-migration-task.sh new file mode 100755 index 00000000000..6a1d4d1f4b1 --- /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/manual-deploy.yml b/.github/workflows/manual-deploy.yml index eaecb2421e3..8b3c4718eae 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 4e7d56cf033..07b387ef5f7 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/migrations-removal/.eslintrc.js b/packages/postgres/migrations-removal/.eslintrc.js new file mode 100644 index 00000000000..9cd5fbd2a6d --- /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 00000000000..5bbefffbabe --- /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 d27148be11b..15f2d992abc 100644 --- a/packages/postgres/package.json +++ b/packages/postgres/package.json @@ -26,7 +26,7 @@ "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", "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/convert-to-sqlite.ts b/packages/postgres/scripts/convert-to-sqlite.ts index f5df2aaf21d..2b071923e4e 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 c895b4396a4..ed3d987d078 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 ' ')" diff --git a/packages/postgres/scripts/lint-migrations.cjs b/packages/postgres/scripts/lint-migrations.cjs index e64386706ee..128c5c69c3c 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 00000000000..cdc7c7a1163 --- /dev/null +++ b/packages/postgres/scripts/migrate-local.sh @@ -0,0 +1,49 @@ +#!/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] | redo ...) is forwarded from the caller, e.g. +# `pnpm migrate up`, `pnpm migrate down "$COUNT"`. Callers only ever roll the +# whole chain back (down count = every migration), so `down` rolls the removal +# phase back in full and forwards the caller's count to the additive phase. +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}" +} + +action="${1:-up}" +shift || true + +if [ "$action" = "down" ]; then + # Roll the removal phase back in full first (its own applied count), then hand + # the caller's count to the additive phase. + removal_count=$(find migrations-removal -maxdepth 1 -type f -name '[0-9]*.js' | wc -l | tr -d ' ') + if [ "$removal_count" -gt 0 ]; then + mig migrations-removal migrations_removal down "$removal_count" + fi + mig migrations migrations down "$@" +else + mig migrations migrations "$action" "$@" + mig migrations-removal migrations_removal "$action" "$@" +fi diff --git a/packages/postgres/scripts/run-migrations.sh b/packages/postgres/scripts/run-migrations.sh index f80b6fd267d..d0b5678b177 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 e947ea3e599..1f98c469b49 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 \ From fd577e7c8e48712d49502734bcb4c62942c60f8b Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 17 Jul 2026 11:00:53 -0500 Subject: [PATCH 2/8] Keep `pnpm migrate create` additive-only in the two-phase driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migrate-local.sh dispatch ran every non-`down` action against both the additive (migrations/) and removal (migrations-removal/) phases. For `create` that scaffolded two files per invocation — the intended migrations/ file plus a spurious migrations-removal/ one — so the documented `pnpm migrate create ` no longer had a single unambiguous output. Handle `create` explicitly: scaffold one file in migrations/. To author a removal migration, create it this way and `git mv` it into migrations-removal/ (tracked by filename, so moving a not-yet-applied file is clean). Apply actions (up, redo) still run against both phases; `down` still rolls the removal phase back first. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/postgres/scripts/migrate-local.sh | 47 ++++++++++++++-------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/packages/postgres/scripts/migrate-local.sh b/packages/postgres/scripts/migrate-local.sh index cdc7c7a1163..c0c615335d2 100755 --- a/packages/postgres/scripts/migrate-local.sh +++ b/packages/postgres/scripts/migrate-local.sh @@ -11,10 +11,12 @@ # 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] | redo ...) is forwarded from the caller, e.g. -# `pnpm migrate up`, `pnpm migrate down "$COUNT"`. Callers only ever roll the -# whole chain back (down count = every migration), so `down` rolls the removal -# phase back in full and forwards the caller's count to the additive phase. +# The action (up | down [count] | create | redo ...) is forwarded from +# the caller, e.g. `pnpm migrate up`, `pnpm migrate down "$COUNT"`. Callers only +# ever roll the whole chain back (down count = every migration), so `down` rolls +# the removal phase back in full and forwards the caller's count to the additive +# phase. `create` is scaffolding, not application, so it targets the additive +# phase only (see the create case below). set -euo pipefail cd "$(dirname "$0")/.." @@ -35,15 +37,28 @@ mig() { action="${1:-up}" shift || true -if [ "$action" = "down" ]; then - # Roll the removal phase back in full first (its own applied count), then hand - # the caller's count to the additive phase. - removal_count=$(find migrations-removal -maxdepth 1 -type f -name '[0-9]*.js' | wc -l | tr -d ' ') - if [ "$removal_count" -gt 0 ]; then - mig migrations-removal migrations_removal down "$removal_count" - fi - mig migrations migrations down "$@" -else - mig migrations migrations "$action" "$@" - mig migrations-removal migrations_removal "$action" "$@" -fi +case "$action" in + create) + # Scaffolding is additive by default: `pnpm migrate create ` writes a + # single file to migrations/. To make a removal migration, create it this + # way and then `git mv` it into migrations-removal/ (node-pg-migrate tracks + # by filename, so moving a not-yet-applied file is clean). Running `create` + # against both phases would emit a spurious second migration every time. + mig migrations migrations create "$@" + ;; + down) + # Roll the removal phase back in full first (its own applied count), then + # hand the caller's count to the additive phase. + removal_count=$(find migrations-removal -maxdepth 1 -type f -name '[0-9]*.js' | wc -l | tr -d ' ') + if [ "$removal_count" -gt 0 ]; then + mig migrations-removal migrations_removal down "$removal_count" + fi + mig migrations migrations down "$@" + ;; + *) + # Apply actions (up, redo, …) run against both phases: additive first, then + # removal. + mig migrations migrations "$action" "$@" + mig migrations-removal migrations_removal "$action" "$@" + ;; +esac From a4723aa358273c2af507491e56360a77e559c410 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 17 Jul 2026 11:18:07 -0500 Subject: [PATCH 3/8] Make two-phase `down` revert the requested count across both phases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `down` branch always rolled the entire removal phase back, then applied the caller's count to the additive phase — so a bare `pnpm migrate down` reverted two migrations (all removal + one additive) instead of the single latest, and it ignored the combined ordering (removal migrations aren't necessarily the most recent once an additive migration is added and applied after them). During local recovery that can silently revert unrelated destructive changes. Revert exactly the requested count (default 1) across the combined newest-first timeline of both phases, using the tracking tables as the source of truth (--no-check-order lets migrations apply out of filename order). Consecutive same-phase entries coalesce into one `down K`, so a full-chain rollback stays ~2 calls rather than one node-pg-migrate invocation per migration. `determine-changed-migrations.sh` now counts both phases so CI's full-chain down_count reverts the entire combined chain. Verified locally: full-chain up/down/up (112+1 → 0 → 112+1, 2 down calls), bare down reverts only the newest, and an interleave case (fresh additive applied after the removal) reverts the additive and leaves the removal intact. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../scripts/determine-changed-migrations.sh | 8 ++- packages/postgres/scripts/migrate-local.sh | 59 +++++++++++++++---- 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/packages/postgres/scripts/determine-changed-migrations.sh b/packages/postgres/scripts/determine-changed-migrations.sh index ed3d987d078..19bbfbee6ee 100755 --- a/packages/postgres/scripts/determine-changed-migrations.sh +++ b/packages/postgres/scripts/determine-changed-migrations.sh @@ -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/migrate-local.sh b/packages/postgres/scripts/migrate-local.sh index c0c615335d2..0a9b2a4d52c 100755 --- a/packages/postgres/scripts/migrate-local.sh +++ b/packages/postgres/scripts/migrate-local.sh @@ -12,11 +12,13 @@ # 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"`. Callers only -# ever roll the whole chain back (down count = every migration), so `down` rolls -# the removal phase back in full and forwards the caller's count to the additive -# phase. `create` is scaffolding, not application, so it targets the additive -# phase only (see the create case below). +# 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")/.." @@ -34,6 +36,16 @@ mig() { "${@: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 @@ -47,13 +59,36 @@ case "$action" in mig migrations migrations create "$@" ;; down) - # Roll the removal phase back in full first (its own applied count), then - # hand the caller's count to the additive phase. - removal_count=$(find migrations-removal -maxdepth 1 -type f -name '[0-9]*.js' | wc -l | tr -d ' ') - if [ "$removal_count" -gt 0 ]; then - mig migrations-removal migrations_removal down "$removal_count" - fi - mig migrations migrations 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 From 1a59c904e8fac0a2ee2af4490b1fbac1b551a25e Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 17 Jul 2026 12:48:23 -0500 Subject: [PATCH 4/8] Guard against destructive DDL in additive migrations + document the split MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the two-phase migration system discoverable and hard to misuse: - CI guard (scripts/check-removal-phase.cjs, "Guard removal-phase migrations" step): AST-parses newly added migrations/ files and fails if an up() drops or renames a column/table, pointing to migrations-removal/. Scoped to changed files (historical drops grandfathered) and to up() only, so an additive migration's down() reversing itself isn't flagged. Uses the TypeScript parser (added as a devDependency) rather than a regex so it can distinguish up() from down(). Heuristic — covers the rolling-deploy outage class (drop/rename), not NOT-NULL tightening or type narrowing. - `pnpm migrate:create-removal ` scaffolds a destructive migration straight into migrations-removal/ (via a create-removal case in migrate-local.sh), so authoring one no longer needs a manual git mv. - packages/postgres/README.md documents which directory a migration belongs in, how to create each kind, and the twin-table / already-applied-move gotchas. - .claude/skills/postgres-migrations surfaces the same guidance to agents working on migrations. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/postgres-migrations/SKILL.md | 90 ++++++++++ .github/workflows/ci.yaml | 8 + packages/postgres/README.md | 73 ++++++++ packages/postgres/package.json | 4 +- .../postgres/scripts/check-removal-phase.cjs | 163 ++++++++++++++++++ packages/postgres/scripts/migrate-local.sh | 13 +- pnpm-lock.yaml | 3 + 7 files changed, 349 insertions(+), 5 deletions(-) create mode 100644 .claude/skills/postgres-migrations/SKILL.md create mode 100644 packages/postgres/README.md create mode 100755 packages/postgres/scripts/check-removal-phase.cjs diff --git a/.claude/skills/postgres-migrations/SKILL.md b/.claude/skills/postgres-migrations/SKILL.md new file mode 100644 index 00000000000..dfd4830a53e --- /dev/null +++ b/.claude/skills/postgres-migrations/SKILL.md @@ -0,0 +1,90 @@ +--- +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). This caused three +published-realm outages (2026-07-10, 07-13, 07-15). + +| 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 the 12 +historical drops already 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/workflows/ci.yaml b/.github/workflows/ci.yaml index 6e7f0548465..90d8e21951f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -291,6 +291,14 @@ 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' + shell: bash + run: node packages/postgres/scripts/check-removal-phase.cjs ${{ steps.migrations.outputs.files }} + - name: Apply migrations if: steps.migrations.outputs.count && steps.migrations.outputs.count != '0' working-directory: packages/postgres diff --git a/packages/postgres/README.md b/packages/postgres/README.md new file mode 100644 index 00000000000..fe27e379939 --- /dev/null +++ b/packages/postgres/README.md @@ -0,0 +1,73 @@ +# @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. (This caused +published-realm outages on 2026-07-10, 07-13, and 07-15.) 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 (historical drops 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/package.json b/packages/postgres/package.json index 15f2d992abc..4aa3a3de727 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 ./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 00000000000..b77c1955da3 --- /dev/null +++ b/packages/postgres/scripts/check-removal-phase.cjs @@ -0,0 +1,163 @@ +#!/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()`. Those are destructive changes that +// break the previous code revision mid-rollout (old tasks query a column the +// migration just removed) — the 07-10 / 07-13 / 07-15 published-realm outages. +// They 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 never flags the +// already-deployed historical drops in `migrations/` — only newly added ones. +// +// 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 (the +// rolling-deploy outage class), 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]; +} + +const files = process.argv + .slice(2) + .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/migrate-local.sh b/packages/postgres/scripts/migrate-local.sh index 0a9b2a4d52c..171b15fece8 100755 --- a/packages/postgres/scripts/migrate-local.sh +++ b/packages/postgres/scripts/migrate-local.sh @@ -52,12 +52,17 @@ shift || true case "$action" in create) # Scaffolding is additive by default: `pnpm migrate create ` writes a - # single file to migrations/. To make a removal migration, create it this - # way and then `git mv` it into migrations-removal/ (node-pg-migrate tracks - # by filename, so moving a not-yet-applied file is clean). Running `create` - # against both phases would emit a spurious second migration every time. + # 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 diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index e9934eb5e3f..adf5dd9e317 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: From 1ad742e22dede5a41c89d715df864781b9e1a917 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 17 Jul 2026 12:59:30 -0500 Subject: [PATCH 5/8] Drop dated incident references from migration docs/comments Describe the failure mode timelessly (a destructive change breaks the previous code revision during a rolling deploy) rather than citing specific past dates or counts, which don't help a future reader and rot. Affects the check-removal-phase guard comment, the postgres README, and the postgres-migrations skill. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/postgres-migrations/SKILL.md | 11 +++++----- packages/postgres/README.md | 13 +++++------ .../postgres/scripts/check-removal-phase.cjs | 22 +++++++++---------- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/.claude/skills/postgres-migrations/SKILL.md b/.claude/skills/postgres-migrations/SKILL.md index dfd4830a53e..be1489db370 100644 --- a/.claude/skills/postgres-migrations/SKILL.md +++ b/.claude/skills/postgres-migrations/SKILL.md @@ -9,8 +9,7 @@ description: How to author Postgres migrations in packages/postgres under the tw `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). This caused three -published-realm outages (2026-07-10, 07-13, 07-15). +serving (old tasks query a column the migration just dropped). | Directory | Tracking table | When it runs in a deploy | For | | --------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------- | ------------------------------------- | @@ -57,10 +56,10 @@ carry a `package.json` pinning `{ "type": "commonjs" }`. `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 the 12 -historical drops already 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 +`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. diff --git a/packages/postgres/README.md b/packages/postgres/README.md index fe27e379939..43eced40223 100644 --- a/packages/postgres/README.md +++ b/packages/postgres/README.md @@ -15,10 +15,9 @@ Migrations live in two directories, each with its own **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. (This caused -published-realm outages on 2026-07-10, 07-13, and 07-15.) 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. +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? @@ -48,9 +47,9 @@ 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 (historical drops are -grandfathered) and inspects only `up()` — an additive migration's `down()` may -call `dropColumn` to reverse itself. +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 diff --git a/packages/postgres/scripts/check-removal-phase.cjs b/packages/postgres/scripts/check-removal-phase.cjs index b77c1955da3..de67641a12c 100755 --- a/packages/postgres/scripts/check-removal-phase.cjs +++ b/packages/postgres/scripts/check-removal-phase.cjs @@ -4,23 +4,23 @@ 'use strict'; // Guard: a migration added to `migrations/` (the additive phase) must not drop -// or rename a column/table in its `up()`. Those are destructive changes that -// break the previous code revision mid-rollout (old tasks query a column the -// migration just removed) — the 07-10 / 07-13 / 07-15 published-realm outages. -// They 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. +// 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 never flags the -// already-deployed historical drops in `migrations/` — only newly added ones. +// 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 (the -// rolling-deploy outage class), not NOT-NULL tightening, type narrowing, or -// destructive SQL assembled from non-literal strings. +// 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'); From ad7c11db0eb982e8cf24dcfd5050c480f74715dc Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 17 Jul 2026 14:19:17 -0500 Subject: [PATCH 6/8] DEMO: misplaced destructive migration to show the CI guard failing Intentionally adds a DROP-in-up() migration to migrations/ (should be in migrations-removal/) so the "Guard removal-phase migrations" CI check fails. Remove this commit/file. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../1784315870597_demo-wrong-directory-drop.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 packages/postgres/migrations/1784315870597_demo-wrong-directory-drop.js diff --git a/packages/postgres/migrations/1784315870597_demo-wrong-directory-drop.js b/packages/postgres/migrations/1784315870597_demo-wrong-directory-drop.js new file mode 100644 index 00000000000..6fdde3254e4 --- /dev/null +++ b/packages/postgres/migrations/1784315870597_demo-wrong-directory-drop.js @@ -0,0 +1,18 @@ +exports.shorthands = undefined; + +// DEMO ONLY — remove this file. Intentionally placed in migrations/ (the +// additive phase) with a DROP in up() to show the "Guard removal-phase +// migrations" CI check failing. A destructive migration belongs in +// migrations-removal/. + +exports.up = (pgm) => { + pgm.dropColumn('boxel_index', 'demo_wrong_directory', { ifExists: true }); +}; + +exports.down = (pgm) => { + pgm.addColumn( + 'boxel_index', + { demo_wrong_directory: { type: 'text' } }, + { ifNotExists: true }, + ); +}; From c39a2e5822db5ee6f2558ee454c8cb7d75bcadde Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 17 Jul 2026 14:27:54 -0500 Subject: [PATCH 7/8] Pass changed migrations to the guard via env, not inline args determine-changed-migrations.sh emits a newline-separated file list. Inlining it into the guard step's run script made each newline a command separator, so the guard only received the first file and the rest ran as bogus shell commands. Feed the list through the CHANGED_MIGRATIONS env var and have check-removal-phase.cjs read and split it, so every changed migration is actually checked without relying on shell word-splitting. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yaml | 7 +++++-- packages/postgres/scripts/check-removal-phase.cjs | 9 +++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 90d8e21951f..43a8cce7b69 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -296,8 +296,11 @@ jobs: # 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' - shell: bash - run: node packages/postgres/scripts/check-removal-phase.cjs ${{ steps.migrations.outputs.files }} + 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' diff --git a/packages/postgres/scripts/check-removal-phase.cjs b/packages/postgres/scripts/check-removal-phase.cjs index de67641a12c..bca61ff4c55 100755 --- a/packages/postgres/scripts/check-removal-phase.cjs +++ b/packages/postgres/scripts/check-removal-phase.cjs @@ -108,8 +108,13 @@ function destructiveOpsInUp(upFn, sourceFile) { return [...ops]; } -const files = process.argv - .slice(2) +// 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. From d064426877b5621e1a58e6227cc0863f9659b291 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 17 Jul 2026 14:44:19 -0500 Subject: [PATCH 8/8] Revert "DEMO: misplaced destructive migration to show the CI guard failing" This reverts commit ad7c11db0eb982e8cf24dcfd5050c480f74715dc. --- .../1784315870597_demo-wrong-directory-drop.js | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 packages/postgres/migrations/1784315870597_demo-wrong-directory-drop.js diff --git a/packages/postgres/migrations/1784315870597_demo-wrong-directory-drop.js b/packages/postgres/migrations/1784315870597_demo-wrong-directory-drop.js deleted file mode 100644 index 6fdde3254e4..00000000000 --- a/packages/postgres/migrations/1784315870597_demo-wrong-directory-drop.js +++ /dev/null @@ -1,18 +0,0 @@ -exports.shorthands = undefined; - -// DEMO ONLY — remove this file. Intentionally placed in migrations/ (the -// additive phase) with a DROP in up() to show the "Guard removal-phase -// migrations" CI check failing. A destructive migration belongs in -// migrations-removal/. - -exports.up = (pgm) => { - pgm.dropColumn('boxel_index', 'demo_wrong_directory', { ifExists: true }); -}; - -exports.down = (pgm) => { - pgm.addColumn( - 'boxel_index', - { demo_wrong_directory: { type: 'text' } }, - { ifNotExists: true }, - ); -};