Skip to content

feat(migrations): no-issue: add --dry-run to migrate and upgrade#10144

Open
passcod wants to merge 1 commit into
mainfrom
feat/migrate-dry-run
Open

feat(migrations): no-issue: add --dry-run to migrate and upgrade#10144
passcod wants to merge 1 commit into
mainfrom
feat/migrate-dry-run

Conversation

@passcod

@passcod passcod commented Jun 25, 2026

Copy link
Copy Markdown
Member

Changes

🤖 Adds a --dry-run mode to the up/redoLatest migrate subcommands and to the upgrade command (alias migrate), on both central and facility servers. It runs the real migration/upgrade code path inside a single outer transaction that is always rolled back, then exits 0 with a summary — so operators can preview a migration or upgrade on a real database without committing anything.

Mechanism notes for reviewers:

  • Sequelize 6 does not nest a bare transaction() call as a savepoint under CLS (CLS only auto-attaches individual queries to the current transaction). So the outer transaction is threaded down and each migration is nested as an explicit SAVEPOINT; the pre/post-migration hooks and upgrade steps attach to the outer transaction automatically via CLS.
  • Changelog audit triggers are DEFERRABLE INITIALLY DEFERRED and only fire at COMMIT, not at RELEASE SAVEPOINT. Under one outer transaction they would accumulate, and a later DDL migration would trip "pending trigger events" on a table an earlier DML migration wrote to (the exact DDL/DML split the codebase mandates). They are flushed at each migration and step boundary to mimic the per-migration COMMIT of a real run.
  • --dry-run is only accepted for up, redoLatest, and upgrade; passing it to a down command errors.

Auto-Deploy

  • Deploy
Options
  • Artillery load test
  • Seed from closest snapshot
  • Generate fake data
  • More data (20Gi)
  • No facility servers (central-only)
  • No sync (facility tasks scaled to zero)
  • AMD64 architecture (default is arm64)
  • Skip mobile build
  • Always build mobile
  • Stay up for 8 hours
  • Stay up for 24 hours
  • Stay up (no TTL)
  • Build images only (don't deploy)
  • Pause this deploy

Tests

  • Run E2E tests
  • Run DAST scan

Review Hero

  • Run Review Hero
  • Auto-fix review suggestions Wait for Review Hero to finish, resolve any comments you disagree with or want to fix manually, then check this to auto-fix the rest.
  • Auto-fix CI failures Check this to auto-fix lint errors, test failures, and other CI issues.
  • Auto-merge upstream Check this to merge the base branch into this PR, with AI conflict resolution if needed.
  • Save suppressions Check this to capture 👎 reactions on Review Hero comments as suppression rules in .github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.

Remember to...

  • ...write or update tests
  • ...add UI screenshots and testing notes to the Linear issue
  • ...add any manual upgrade steps to the Linear issue
  • ...update the config reference, settings reference, or any relevant runbook(s)
  • ...call out additions or changes to config files for the deployment team to take note of

Comment thread packages/database/src/services/migrations/migrations.js Outdated
Comment thread packages/database/src/services/migrations/migrations.js Outdated
Comment thread packages/database/src/services/migrations/migrations.js Outdated
Comment thread packages/database/__tests__/services/migrations/dryRun.test.ts Outdated
@review-hero

review-hero Bot commented Jun 25, 2026

Copy link
Copy Markdown

🦸 Review Hero Summary
6 agents reviewed this PR | 0 critical | 4 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 5 below threshold

Below consensus threshold (5 unique issues not confirmed by majority)
Location Agent Severity Comment
packages/database/src/services/migrations/migrations.js:10 BES Requirements nitpick Several new multi-paragraph JSDoc block comments were added (lines 10-13, 16-23, 40-48). Project rules say no multi-line comment blocks — one short line max, only when the WHY is non-obvious. The m...
packages/database/src/services/migrations/migrations.js:130 Bugs & Correctness suggestion When dryRun: true but parentTransaction is null (its default), sequelize.transaction({ transaction: null }, callback) is called. Sequelize treats transaction: null as no parent and starts...
packages/database/src/services/migrations/migrations.js:246 BES Requirements nitpick The project coding conventions require boolean variables to use an is/has/can/does prefix (e.g. isDryRun). dryRun is used as a boolean flag throughout this change (dryRun = false, `if...
packages/database/src/services/migrations/migrations.js:421 Bugs & Correctness suggestion For direction === 'redoLatest' with --dry-run, run() returns the result of migrateUp (number of migrations re-applied after the down), so the log message correctly reports the count. Howeve...
packages/upgrade/src/index.ts:194 Bugs & Correctness suggestion step.run(args) is called without a parentTransaction field in args. The code comment on runInRollbackTransaction explicitly warns that Sequelize does not auto-nest sequelize.transaction()...
Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.


packages/database/src/services/migrations/migrations.js:24: The log parameter is accepted but never referenced anywhere in runInRollbackTransaction. Every call site passes a logger (e.g. runInRollbackTransaction(sequelize, log, run) in migrate()), creating a misleading API. Either add a log call (e.g. log the rollback or the caught DryRunRollback), or drop the parameter and update all call sites.


packages/database/src/services/migrations/migrations.js:37: return result; at line 37 is unreachable dead code. sequelize.transaction() always throws from this call site — either DryRunRollback (caught and returned at line 33) or a real error (rethrown at line 35). The statement after the try-catch can never execute. Remove it to avoid confusing future readers about the control flow.


packages/database/src/services/migrations/migrations.js:25: The let result pattern is a code smell per coding-rules.md. The variable is declared mutable, then assigned inside the try block and read from the catch block. Carrying the value on the sentinel error object eliminates the mutable binding entirely:

`js
class DryRunRollback extends Error {
constructor(result) { super(); this.result = result; }
}

export async function runInRollbackTransaction(sequelize, log, fn) {
try {
await sequelize.transaction(async (outerTransaction) => {
throw new DryRunRollback(await fn(outerTransaction));
});
} catch (error) {
if (error instanceof DryRunRollback) return error.result;
throw error;
}
}
`


packages/database/__tests__/services/migrations/dryRun.test.ts:35: The assertion expect(await migrations.pending()).toHaveLength(0) is vacuously true because the test DB is fully migrated before the test runs, leaving no pending migrations to begin with — so this assertion passes regardless of whether the dry-run rollback works. Worse, if the test DB did have pending migrations and the dry-run correctly rolled them back, this assertion would incorrectly FAIL (the migrations would still be pending). The real rollback guarantee is verified by the runInRollbackTransaction suite below; this test only exercises a no-op code path. Consider dropping the misleading pending() assertion, or, to actually prove rollback, insert a fake migration name into SequelizeMeta before the test and verify it is absent afterward.

@passcod passcod force-pushed the feat/migrate-dry-run branch 2 times, most recently from f87b29c to 47cd0e8 Compare June 25, 2026 16:14
@passcod passcod marked this pull request as ready for review June 25, 2026 20:08

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 47cd0e8. Configure here.

Comment thread packages/upgrade/src/index.ts
Adds a --dry-run mode to the up/redoLatest migrate subcommands and the
upgrade command (central and facility). It runs the real migration and
upgrade code path inside a single outer transaction that is always rolled
back, so operators can preview a migration or upgrade without committing
any changes, then exits 0 with a summary.

Sequelize 6 does not nest a bare transaction() call as a savepoint under
CLS (it only auto-attaches individual queries), so the outer transaction is
threaded down and each migration is nested as an explicit SAVEPOINT; the
pre/post-migration hooks and upgrade steps attach via CLS automatically.

Because DEFERRABLE INITIALLY DEFERRED changelog audit triggers only fire at
COMMIT (not at RELEASE SAVEPOINT), they are flushed at each migration and
step boundary to mimic the per-migration COMMIT of a real run; otherwise a
later DDL migration would trip "pending trigger events" on a table an
earlier DML migration wrote to.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@passcod passcod force-pushed the feat/migrate-dry-run branch from 47cd0e8 to 3429eea Compare June 26, 2026 00:40
@passcod passcod added this pull request to the merge queue Jun 26, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 26, 2026
@passcod passcod added this pull request to the merge queue Jun 27, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 27, 2026
@github-actions

Copy link
Copy Markdown

E2E failure report

E2E failed for run 21624.

The report artifact contains the merged Playwright HTML report with failure videos, screenshots, and traces where available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant