Migrations: Fix RetrustForeignKeyAndCheckConstraints failing when data violates a constraint

[AndyButland]

Description

This fixes the RetrustForeignKeyAndCheckConstraints migration step which has been shown to break the upgrade when existing data violates a foreign key or check constraint.

The migration was introduced in 17.3 to re-trust untrusted constraints (where is_not_trusted = 1) so the SQL Server query optimizer can use them for join elimination and cardinality estimation. When all constraints pass validation this works fine, but when a constraint fails validation (e.g. orphaned FK rows from a previous bug), the upgrade crashes with:

System.InvalidOperationException: This SqlTransaction has completed; it is no longer usable.

Root cause

The original code executed ALTER TABLE ... WITH CHECK CHECK CONSTRAINT on the migration scope's database connection, which has an active .NET SqlTransaction. Although a SQL level TRY...CATCH was in place, when validation fails, the constraint violation error zombies the SqlTransaction at the TDS (Tabular Data Stream) protocol layer — the transaction state change propagates to the .NET SqlClient before T-SQL error handling can contain it. Once the transaction is zombied:

1. context.Complete() (which writes the migration state via IKeyValueService) fails because it uses the same dead transaction.
2. scope.Complete() is never reached.
3. Scope disposal tries to roll back the already-dead transaction → "This SqlTransaction has completed" exception.
4. The migration state is not saved.

Fix

Each ALTER TABLE ... WITH CHECK CHECK CONSTRAINT now executes on a separate database connection created via IUmbracoDatabaseFactory. This connection operates in SQL Server's autocommit mode (no explicit SqlTransaction), which means:

• T-SQL TRY...CATCH fully contains the error — no SqlException is thrown to .NET.
• The migration scope's own transaction is never touched, so context.Complete() and scope.Complete() succeed.
• Constraints that pass validation are re-trusted; those that fail are logged as warnings and skipped.

Testing

Setup — create a constraint violation database before upgrading:

-- Disable the FK so we can insert orphaned data
ALTER TABLE [dbo].[umbracoRelation] NOCHECK CONSTRAINT [FK_umbracoRelation_umbracoNode];

-- Insert a relation pointing to a non-existent node
INSERT INTO [dbo].[umbracoRelation] (parentId, childId, relType, [datetime], comment)
VALUES (
    999999,
    (SELECT TOP 1 id FROM [dbo].[umbracoNode]),
    (SELECT TOP 1 id FROM [dbo].[umbracoRelationType]),
    GETDATE(),
    '')

-- Re-enable the FK without validating existing data (leaves it untrusted)
ALTER TABLE [dbo].[umbracoRelation] WITH NOCHECK CHECK CONSTRAINT [FK_umbracoRelation_umbracoNode];

-- Verify it shows as untrusted
SELECT name, is_not_trusted
FROM sys.foreign_keys
WHERE name = 'FK_umbracoRelation_umbracoNode';
-- Expected: is_not_trusted = 1

-- Try to check the constraint - this is expected to report an error of:
-- The ALTER TABLE statement conflicted with the FOREIGN KEY 
-- constraint "FK_umbracoRelation_umbracoNode".
ALTER TABLE [dbo].[umbracoRelation] WITH CHECK CHECK CONSTRAINT [FK_umbracoRelation_umbracoNode];

Test 1 — upgrade completes without crashing:

Revert the database to the migration step prior to the RetrustForeignKeyAndCheckConstraints step:

UPDATE umbracoKeyValue
SET value = '{A7B8C9D0-E1F2-4A5B-8C7D-9E0F1A2B3C4D}'
WHERE [key] = 'Umbraco.Core.Upgrader.State+Umbraco.Core'

1. Start up the site using a build containing this fix.
2. Verify the site starts successfully.
3. Check logs for "Constraint re-trust complete: X succeeded, Y failed out of Z total.". Expect at least one failure.
4. Verify a WARNING (not an exception stack trace) is logged for the violated constraint.

Test 2 — clean constraints are re-trusted:

Check which constraints are still untrusted after upgrade. Its expected that only the intentionally violated FK remains untrusted.

-- Check which constraints are still untrusted after upgrade
SELECT 'FK' AS Type, name, OBJECT_NAME(parent_object_id) AS TableName, is_not_trusted
FROM sys.foreign_keys
WHERE is_not_trusted = 1
  AND (OBJECT_NAME(parent_object_id) LIKE 'umbraco%' OR OBJECT_NAME(parent_object_id) LIKE 'cms%')
UNION ALL
SELECT 'CK', name, OBJECT_NAME(parent_object_id), is_not_trusted
FROM sys.check_constraints
WHERE is_not_trusted = 1
  AND (OBJECT_NAME(parent_object_id) LIKE 'umbraco%' OR OBJECT_NAME(parent_object_id) LIKE 'cms%');
-- Expected: only the intentionally violated FK remains untrusted

Remove the orphaned data:

DELETE FROM [dbo].[umbracoRelation] WHERE [parentId] = 999999;

Revert the database again to the migration step prior to the RetrustForeignKeyAndCheckConstraints step:

UPDATE umbracoKeyValue
SET value = '{A7B8C9D0-E1F2-4A5B-8C7D-9E0F1A2B3C4D}'
WHERE [key] = 'Umbraco.Core.Upgrader.State+Umbraco.Core'

Start up the application again and confirm the migration succeeds.

You should see an information log message of:

Constraint re-trust complete: 1 succeeded, 0 failed out of 1 total.

Verify that the constraint is now trusted (expect is_not_trusted = 0)

SELECT name, is_not_trusted FROM sys.foreign_keys WHERE name = 'FK_umbracoRelation_umbracoNode';

[Copilot]

Pull request overview

This PR updates the RetrustForeignKeyAndCheckConstraints upgrade migration (introduced in 17.3) to avoid upgrade crashes when re-trusting constraints fails due to existing data violating a FK/CK constraint, by executing the validation on a separate database connection instead of within the migration scope’s transaction.

Changes:

• Inject IUmbracoDatabaseFactory into the migration to create isolated database connections for ALTER TABLE ... WITH CHECK CHECK CONSTRAINT.
• Execute each constraint re-trust attempt using a separate IUmbracoDatabase instance (no explicit transaction), logging warnings for failures instead of crashing the upgrade.

[claude]

PR Review

Target: origin/main · Based on commit: ed9d1d97 · Skipped: 0 files out of 1 total

Fixes RetrustForeignKeyAndCheckConstraints failing when existing data violates a constraint: moves each ALTER TABLE ... WITH CHECK CHECK CONSTRAINT onto a separate, transaction-free IUmbracoDatabase connection so T-SQL TRY...CATCH isolation works correctly (the migration transaction could otherwise zombie even when the error was caught in SQL).

• Modified public API: RetrustForeignKeyAndCheckConstraints(IMigrationContext) constructor → RetrustForeignKeyAndCheckConstraints(IMigrationContext, IUmbracoDatabaseFactory)
• Affected implementations (outside this PR): none (auto-discovered via DI; UmbracoPlan.cs uses To<T>() which resolves through MigrationBuilder.Build)
• Breaking changes: Constructor signature changed on a public class released in v17.3.0 — old one-arg constructor removed without applying the Obsolete + StaticServiceProvider pattern (CLAUDE.md §5.1)

---

Critical

• RetrustForeignKeyAndCheckConstraints.cs:22: Constructor changed from (IMigrationContext) to (IMigrationContext, IUmbracoDatabaseFactory) without preserving the old overload → breaks any code calling new RetrustForeignKeyAndCheckConstraints(context). Per CLAUDE.md §5.1, keep the old constructor with [Obsolete("... Scheduled for removal in Umbraco 19.")] and delegate to the new one via : this(context, StaticServiceProvider.Instance.GetRequiredService<IUmbracoDatabaseFactory>()).

Important

• RetrustForeignKeyAndCheckConstraints.cs:90: db.CommandTimeout = 300 hard-codes the timeout, overriding any user-configured value higher than 300 (e.g. Connect Timeout=600 in the connection string). The existing EnsureLongCommandTimeout(db) helper is a one-line drop-in that only raises the value when it is below 300, preserving any higher configured value.

Suggestions

• RetrustForeignKeyAndCheckConstraints.cs:89: IUmbracoDatabase is created inside the loop — one connection per constraint. Since TRY...CATCH works correctly on a transactionless connection (as the comment explains), a single db opened before the loop and reused across all iterations would be sufficient, avoiding redundant pool round-trips.

---

Request Changes

The breaking constructor change must be addressed with the Obsolete + StaticServiceProvider pattern before merge. The EnsureLongCommandTimeout regression is also worth fixing while you are in this file.

[Zeegaan]

Looks good from a code perspective, thank you for the detailed testing steps 💪