Skip to content

[Migration Engine Part 2] Add a migration checkpoint table to SQL Server and PostgreSQL - #5897

Draft
warwickschroeder wants to merge 3 commits into
warwick/migration-engine-1from
warwick/migration-engine-2
Draft

warwickschroeder wants to merge 3 commits into
warwick/migration-engine-1from
warwick/migration-engine-2

Conversation

@warwickschroeder

Copy link
Copy Markdown
Contributor

Stacked on #5894 (warwick/migration-engine-1). Review against that branch, not master.

What this adds

Somewhere for the migration engine to keep its progress, so a copy that stops part way can be picked up rather than started again. Nothing writes to it yet: no persister implements IMigrationTarget, so the table is created empty and only the tests put rows in it.

  • A checkpoint table on both EF Core backends. MigrationCheckpointEntity holds one row per migration category, with the cursor to resume after, the copied, skipped and already-present counts, the state, the timestamps and the last error. One EF migration per provider (20260915083155_AddMigrationCheckpoints on SQL Server, 20260915083207_AddMigrationCheckpoints on PostgreSQL), each a bare CREATE TABLE for a table that did not exist, so it is applied by --setup and there are no existing rows to normalise first.
  • A checkpoint saves with the data it describes, or not at all. ServiceControlDbContext.UpsertCheckpoint runs on the caller's own context, so it commits inside the caller's transaction and the rows and the progress land together. EFMigrationCheckpointStore is the out-of-transaction way in, registered in BasePersistence for both backends; every one of its methods opens its own scope and context, so it cannot join a caller's transaction and the doc comment says to use the extension instead.
  • A stale save is refused rather than silently winning. MigrationCheckpoint gains a Version optimistic concurrency token, configured with IsConcurrencyToken so the expected value goes into the UPDATE's WHERE clause. A save built from an out-of-date copy matches no row and raises the new MigrationCheckpointConflictException, as does a save carrying a version for a category that has no row at all.
  • A shutdown no longer rolls the cursor back. MigrationEngine now tracks the version it expects after each save, and its cancellation path catches MigrationCheckpointConflictException and logs it instead of failing: the last batch had already committed its own checkpoint, so saving the in-memory one would move the cursor backwards. The shutdown still reports as a shutdown.
  • Skip reasons are typed instead of stringly typed. MigrationSkipReason replaces string as the dictionary key across MigrationCheckpoint, MigrationWriteResult and the engine's accounting. The database column stays JSON keyed by the reason's name, not its number, so a stored count keeps its meaning if the enum's members are ever reordered.
  • An embedded migration source waits for the server it started. EmbeddedDatabase.WaitUntilReady wraps EmbeddedServer.Instance.GetServerUriAsync, and RavenReadOnlySourceLifecycle.StartEmbedded awaits it instead of returning the configured URL. Start only queues the server up, so connecting by the configured URL raced it on a cold start and reached whatever already held the port when a ServiceControl instance was running. Failure now names the data directory, the setting it came from, and the instance to stop. No existing caller of EmbeddedDatabase changed.
  • Docs. docs/migration/ravendb-to-sql-migration-instructions.md gains the third report failure and what to do about it.

Tests

  • ServiceControl.Persistence.Tests/EFCore (new files): the table round-tripping every column through the DbContext, every timestamp coming back as UTC, the skip-reason JSON being keyed by name; the store's read, upsert, read-all, an abandoned checkpoint and a restart after a halt clearing what the previous save set; and five transaction cases, including a commit landing rows and checkpoint together, a rollback leaving the previous cursor in place, and a save from an out-of-date copy being refused. Runs on SQL Server and PostgreSQL.
  • ServiceControl.UnitTests/Migration: the existing engine fakes and tests move to the typed skip reason and assert the version the engine expects after each save.

@warwickschroeder warwickschroeder self-assigned this Sep 16, 2026
@warwickschroeder
warwickschroeder added this pull request to stack #5898 September 16, 2026 04:25
@warwickschroeder
warwickschroeder force-pushed the warwick/migration-engine-2 branch from bd65b15 to 4456c32 Compare September 16, 2026 13:20
services.AddSingleton<IExternalIntegrationRequestsDataStore>(p => p.GetRequiredService<ExternalIntegrationRequestsDataStore>());
services.AddHostedService(p => p.GetRequiredService<ExternalIntegrationRequestsDataStore>());

services.AddSingleton<IMigrationCheckpointStore, EFMigrationCheckpointStore>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to register if we are not in "migration" mode ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Ill confirm but I dont think so.

@warwickschroeder warwickschroeder Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes it should still be registered. SC still checks the checkpoint store on startup to ensure its not in a broken migration state, or all of the required data hasnt been migrated yet (i.e. partly due to a crash or something). We dont want SC to boot in SQL and start ingesting before these are brought across as its harder to rollback to raven once SQL is ingesting.

I've updated the overview doc to show the startup seq

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These changes in this file feel very migration related, should they really be extension methods given that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved the methods out

- Introduced a new table `MigrationCheckpoints` to track migration states and progress.
- Implemented `MigrationCheckpointEntity` to represent the checkpoint data structure.
- Added `EFMigrationCheckpointStore` for managing checkpoint data with methods for reading and upserting checkpoints.
- Created `MigrationCheckpointConfiguration` for configuring the entity in the DbContext.
- Enhanced `ServiceControlDbContext` to include the new `MigrationCheckpoints` DbSet.
- Updated `EfCoreExtensions` to provide an `UpsertCheckpoint` method for managing checkpoint persistence.
- Added unit tests for `EFMigrationCheckpointStore` and `MigrationCheckpointTable` to ensure correct functionality.
- Implemented transaction handling in `MigrationCheckpointTransactionTests` to verify checkpoint behavior during commits and rollbacks.
- Updated RavenDB integration to support embedded server readiness checks.
@warwickschroeder
warwickschroeder force-pushed the warwick/migration-engine-2 branch from 059e1b9 to 805d0d4 Compare September 17, 2026 04:17
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.

2 participants