From c4dcd9bc02b4f9485879c5cc6faadd2fd554a73e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 23 Jul 2026 13:55:09 -0400 Subject: [PATCH] fix(transport): run bundle import inside the execution strategy (Gitea #28) BundleImporter.ApplyAsync opened a user-initiated transaction directly, but the central ConfigurationDb context is configured with EnableRetryOnFailure. SqlServerRetryingExecutionStrategy rejects user-initiated transactions, so every bundle import threw on any real SQL Server ('...does not support user-initiated transactions.'). The whole Transport suite ran on the in-memory provider (no retrying strategy, BeginTransaction is a no-op) so it never surfaced. Extract the transactional apply into ApplyMergeAsync and drive it via _dbContext.Database.CreateExecutionStrategy().ExecuteAsync, so the strategy owns the BeginTransaction -> apply -> Commit unit as one retriable block. The delegate resets per-attempt state (change tracker + ApplyMergeAsync rebuilds its own summary/resolution accumulators) so a retried attempt cannot double-apply; a rollback failure is captured and surfaced on the BundleImportFailed audit row exactly as before. Post- commit side effects (ScriptArtifactsChanged publish, session zero/ remove) moved outside the retriable delegate so they run once. Regression test: BundleImporterRetryingStrategyTests runs the import on SQLite with a retrying execution strategy (RetriesOnFailure == true, arming the same guard as production). It fails with the exact production error on the pre-fix code and passes after. Existing rollback/apply contracts unchanged (104 integration + 154 unit tests green). --- .../Import/BundleImporter.cs | 551 ++++++++++-------- .../BundleImporterRetryingStrategyTests.cs | 219 +++++++ 2 files changed, 525 insertions(+), 245 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterRetryingStrategyTests.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index 8b8a880d..5d514d52 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -1272,265 +1272,96 @@ public sealed class BundleImporter : IBundleImporter } var bundleImportId = Guid.NewGuid(); - var resolutionMap = resolutions.ToDictionary( - r => (r.EntityType, r.Name), - r => r); - var summary = new ImportSummary(); - - // Inbound API keys are not transported between environments. - // A legacy bundle may still contain a keys section; we ignore those keys - // entirely (never re-create them) but count them so the result can tell - // the operator to re-issue keys on this environment. - var apiKeysIgnored = content.ApiKeys?.Count ?? 0; - // Set the correlation BEFORE the transaction so any audit writes // triggered during the apply pick up the BundleImportId — AuditService // reads the scoped context at the moment LogAsync is called. _correlationContext.BundleImportId = bundleImportId; - // BeginTransactionAsync is a no-op on the in-memory EF provider (which - // logs an InMemoryEventId.TransactionIgnoredWarning by default). To keep - // rollback semantics testable on in-memory AND correct on relational - // providers, we defer the SINGLE SaveChangesAsync call until just before - // CommitAsync — every Add*Async + LogAsync call only stages on the - // change tracker, so throwing before SaveChangesAsync naturally undoes - // the entire apply on both providers. - await using var tx = await _dbContext.Database.BeginTransactionAsync(ct).ConfigureAwait(false); + // The central ConfigurationDb context is configured with + // EnableRetryOnFailure (ServiceCollectionExtensions), and + // SqlServerRetryingExecutionStrategy REJECTS a user-initiated + // transaction opened outside the strategy ("The configured execution + // strategy 'SqlServerRetryingExecutionStrategy' does not support + // user-initiated transactions."), so opening BeginTransactionAsync + // directly here throws on any real SQL Server. Run the whole + // BeginTransaction -> apply -> Commit unit INSIDE the execution strategy + // so the strategy owns it and re-executes it as one retriable unit on a + // transient fault. On the in-memory provider CreateExecutionStrategy() + // returns a non-retrying strategy, so this stays a single straight- + // through call there (behaviour unchanged). + // + // Retry idempotency: the strategy re-runs the WHOLE delegate on a + // transient fault, so the change tracker is cleared at the top of the + // delegate (adds staged by the failed attempt, whose transaction has + // already rolled back, must not leak into the retry) and ApplyMergeAsync + // rebuilds its own summary/resolution accumulators per call — otherwise + // a retry would double-apply. A rollback failure captured inside the + // delegate is surfaced to the outer catch via rollbackFailureMessage so + // the BundleImportFailed row still records both faults. + var strategy = _dbContext.Database.CreateExecutionStrategy(); + string? rollbackFailureMessage = null; + ImportResult result; try { - // Run semantic validation FIRST — before any writes are staged. - // This is purely a name-resolution scan over the in-memory DTOs + - // pre-existing target reads, so it has no ordering dependency on - // the Apply* helpers. Failing here means the change tracker is - // still empty, which keeps the rollback contract simple on both - // the in-memory and relational providers (intermediate - // SaveChangesAsync between Apply* and the second-pass rewire - // would otherwise prevent the in-memory provider from undoing - // already-flushed template rows on validation failure — the - // in-memory transaction is a no-op, so the only safe pattern is - // ONE deferred SaveChangesAsync at the very end of try-block). - // - // Skip-resolved DTOs are excluded from the in-bundle name set so - // a Skip on a dependency surfaces as a missing-reference error - // rather than silently passing. - var (validationErrors, validationWarnings) = - await RunSemanticValidationAsync(content, resolutionMap, ct).ConfigureAwait(false); - // Validate every site / connection / template reference the - // site-instance payload depends on BEFORE any row is staged. Running - // this in the validation phase (not as an apply-pass guard) preserves - // the rollback contract: a structurally-unresolvable bundle fails with - // an empty change tracker, so nothing is half-written on the in-memory - // provider (where the intermediate site/connection flush can't be - // undone by ChangeTracker.Clear). The apply passes keep defensive - // guards, but in normal operation those never fire. - validationErrors = validationErrors.Count > 0 - ? validationErrors - : await ValidateSiteInstanceReferencesAsync(content, resolutionMap, nameMap, ct).ConfigureAwait(false); - if (validationErrors.Count > 0) + result = await strategy.ExecuteAsync(async () => { - throw new SemanticValidationException(validationErrors); - } + // Reset per-attempt state so a retried delegate re-runs clean. + _dbContext.ChangeTracker.Clear(); + rollbackFailureMessage = null; - // Advisory template-script reference findings never block the import; - // log them and surface them on the ImportResult so the operator sees - // the same advisory the preview showed. The deploy-time gate is the - // authoritative re-validation. - if (validationWarnings.Count > 0) - { - _logger?.LogWarning( - "Bundle import {BundleImportId}: {Count} advisory import warning(s): {Warnings}", - bundleImportId, validationWarnings.Count, string.Join("; ", validationWarnings)); - } - - // ---- Site/instance-scoped apply: sites + connections FIRST ---- - // Sites are the FK target for both data connections (SiteId) and - // instances (SiteId); data connections are the FK target for instance - // connection bindings (DataConnectionId) and the rewrite target for - // native-alarm-source ConnectionNameOverride. Resolve-or-create both - // BEFORE the central-config apply so the maps the instance pass needs - // are fully populated, then flush so newly-created site/connection ids - // materialise on the relational provider. (The instance pass itself - // runs AFTER the central-config flush so it can also resolve template - // ids by name.) - var siteBySourceIdentifier = await ApplySitesAsync( - content, nameMap, resolutionMap, user, summary, ct).ConfigureAwait(false); - var connectionMaps = await ApplyDataConnectionsAsync( - content, nameMap, siteBySourceIdentifier, resolutionMap, user, summary, ct).ConfigureAwait(false); - // Flush so site + connection surrogate ids are assigned (relational - // provider) before the instance pass wires its FKs. In-memory assigns - // ids on AddAsync, so this is mostly a no-op there, but it keeps the - // ordering correct on a real DB. Rides the same outer transaction. - await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); - - await ApplyTemplateFoldersAsync(content.TemplateFolders, resolutionMap, user, summary, ct).ConfigureAwait(false); - await ApplyTemplatesAsync(content.Templates, resolutionMap, user, summary, ct).ConfigureAwait(false); - await ApplySharedScriptsAsync(content.SharedScripts, resolutionMap, user, summary, ct).ConfigureAwait(false); - await ApplyExternalSystemsAsync(content.ExternalSystems, resolutionMap, user, summary, ct).ConfigureAwait(false); - await ApplyDatabaseConnectionsAsync(content.DatabaseConnections, resolutionMap, user, summary, ct).ConfigureAwait(false); - await ApplyNotificationListsAsync(content.NotificationLists, resolutionMap, user, summary, ct).ConfigureAwait(false); - await ApplySmtpConfigsAsync(content.SmtpConfigs, resolutionMap, user, summary, ct).ConfigureAwait(false); - await ApplySmsConfigsAsync(content.SmsConfigs, resolutionMap, user, summary, ct).ConfigureAwait(false); - // Inbound API keys are NOT applied from a bundle — any keys - // in a legacy bundle were counted above (apiKeysIgnored) and are skipped. - await ApplyApiMethodsAsync(content.ApiMethods, resolutionMap, user, summary, ct).ConfigureAwait(false); - - // Second-pass rewire of name-keyed - // FKs that can only be resolved AFTER every template's scripts and - // child rows have been staged. We flush here so Pass A can look up - // the just-persisted scripts by name and Pass B can look up the - // just-persisted templates by name; both passes stage further - // mutations that ride the SAME outer transaction (committed below). - // - // EF tracking note: AddAsync on the in-memory provider assigns - // synthetic ids eagerly, so this intermediate flush mostly - // materialises identity values on a relational provider. The - // rollback contract is preserved because semantic validation - // already ran above — any throw from this point onward represents - // a successful merge that the user wants to keep, and the only - // remaining failure surface is the final BundleImported audit - // write itself. - await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); - await ResolveAlarmScriptLinksAsync(content.Templates, resolutionMap, user, ct).ConfigureAwait(false); - await ResolveCompositionEdgesAsync(content.Templates, resolutionMap, user, ct).ConfigureAwait(false); - await ResolveInheritanceEdgesAsync(content.Templates, resolutionMap, user, ct).ConfigureAwait(false); - // Folder parent edges are name-keyed too — resolve them AFTER the flush - // above so every imported folder has a materialised id to point at - // (the Add pass created the folders parent-less). Mirrors the template - // inheritance/composition rewires. - await ResolveFolderParentEdgesAsync(content.TemplateFolders, resolutionMap, user, ct).ConfigureAwait(false); - // External-system methods have no navigation on the parent definition, - // so an Add can't cascade them — persist them here, once the parent - // system has a real id from the flush above. (Overwrite already syncs - // its methods inline against the existing parent id.) - await PersistAddedExternalSystemMethodsAsync(content.ExternalSystems, resolutionMap, user, ct).ConfigureAwait(false); - - // ---- Import-time acyclicity guard over the merged template graph ---- - // The two rewire passes above have STAGED (not yet saved) the bundle's - // inheritance (ParentTemplateId) and composition edges onto the tracked - // target templates. A bundle can still close a loop through a pre-existing - // target template — e.g. Overwrite a base so it now inherits from (or - // composes) one of its own descendants. The design-time save-gate enforces - // acyclicity on individual edits, but an import merges many edges at once, - // so re-check the whole merged graph here. On a cycle we throw and the - // catch below rolls the whole import back (ChangeTracker.Clear on the - // in-memory provider) BEFORE any of it is persisted; deliberately no - // SaveChanges precedes this check so nothing survives that rollback. - EnsureNoTemplateGraphCycles(); - - // ---- Site/instance-scoped apply: instances LAST ---- - // The instance pass needs every FK target materialised first: - // • template id — resolved by name against the just-flushed central - // config (Templates were flushed above for the alarm/composition - // rewire), plus pre-existing target templates; - // • site id — from the site map built by ApplySitesAsync; - // • connection id — from the connection map built by - // ApplyDataConnectionsAsync. - // It writes the instance row + its four child collections (attribute / - // alarm / native-alarm-source overrides + connection bindings) and - // rewires every name-keyed FK to the target environment's ids. - await ApplyInstancesAsync( - content, resolutionMap, siteBySourceIdentifier, connectionMaps, - user, summary, ct).ConfigureAwait(false); - - // ---- Pre-commit point: stale-instance computation ---- - // Everything the import will write is staged on the change tracker at - // this point but NOT yet committed. This computes StaleInstanceIds here - // — target instances whose template was overwritten by this import and - // therefore now carry a stale flattened-config revision — and threads - // the resulting list into the ImportResult below (replacing the - // Array.Empty() placeholder). The single deferred SaveChangesAsync - // is the next statement, so a read against the change tracker here sees - // the full post-apply graph before commit. - var staleInstanceIds = await ComputeStaleInstanceIdsAsync( - content, resolutionMap, ct).ConfigureAwait(false); - - await _auditService.LogAsync( - user: user, - action: "BundleImported", - entityType: "Bundle", - entityId: bundleImportId.ToString(), - entityName: session.Manifest.SourceEnvironment, - afterState: new + // BeginTransactionAsync is a no-op on the in-memory EF provider + // (which logs an InMemoryEventId.TransactionIgnoredWarning by + // default). To keep rollback semantics testable on in-memory AND + // correct on relational providers, ApplyMergeAsync defers the + // SINGLE final SaveChangesAsync until the very end — every + // Add*Async + LogAsync call only stages on the change tracker, + // so throwing before that naturally undoes the whole apply on + // both providers. + var tx = await _dbContext.Database.BeginTransactionAsync(ct).ConfigureAwait(false); + try { - BundleImportId = bundleImportId, - session.Manifest.SourceEnvironment, - session.Manifest.ContentHash, - Summary = new + // ApplyMergeAsync stages the whole merge and issues the + // single deferred final SaveChangesAsync itself; here we only + // commit the transaction it ran under. + var applied = await ApplyMergeAsync( + content, resolutions, nameMap, session, bundleImportId, user, ct).ConfigureAwait(false); + await tx.CommitAsync(ct).ConfigureAwait(false); + await tx.DisposeAsync().ConfigureAwait(false); + return applied; + } + catch + { + // Roll back explicitly (rather than leaning on Dispose) so a + // rollback that itself throws does not mask the ORIGINAL + // exception — capture it and let the original propagate to + // the strategy, which decides whether to retry. + try { - summary.Added, - summary.Overwritten, - summary.Skipped, - summary.Renamed, - }, - // Legacy inbound API keys present in the bundle that - // were ignored (not transported / not re-created here). - ApiKeysIgnored = apiKeysIgnored, - }, - cancellationToken: ct).ConfigureAwait(false); + await tx.RollbackAsync(ct).ConfigureAwait(false); + } + catch (Exception rbEx) + { + rollbackFailureMessage = rbEx.Message; + } + try { await tx.DisposeAsync().ConfigureAwait(false); } + catch { /* dispose-after-throw must not mask the original cause */ } - await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); - await tx.CommitAsync(ct).ConfigureAwait(false); - - PublishScriptArtifactChanges(resolutions); - - // T-007: zero out the decrypted plaintext BEFORE remove so any - // caller-held reference (e.g. the Razor page that built the - // ImportPreview) sees the cleared buffer too. Remove drops the - // dictionary entry; together they release the secrets immediately - // instead of leaving them in process memory for the full TTL. - ZeroDecryptedContent(session); - _sessionStore.Remove(sessionId); - - return new ImportResult( - BundleImportId: bundleImportId, - Added: summary.Added, - Overwritten: summary.Overwritten, - Skipped: summary.Skipped, - Renamed: summary.Renamed, - StaleInstanceIds: staleInstanceIds, - AuditEventCorrelation: bundleImportId.ToString(), - ApiKeysIgnored: apiKeysIgnored, - Warnings: validationWarnings); + // Clear the change tracker — on the in-memory provider the + // rollback is a no-op and the staged adds would otherwise + // persist on the next SaveChangesAsync (a retry or the + // failure-row write below). + _dbContext.ChangeTracker.Clear(); + throw; + } + }).ConfigureAwait(false); } catch (Exception ex) { - // Rollback can itself throw (connection drop mid-rollback, provider - // bug, etc). If it does, we must STILL write the BundleImportFailed - // audit row — otherwise a rollback-failure path silently swallows - // the import's audit trail. Capture the rollback exception (if any) - // and surface it on the failure row alongside the original cause. - Exception? rollbackFailure = null; - try - { - await tx.RollbackAsync(ct).ConfigureAwait(false); - } - catch (Exception rbEx) - { - rollbackFailure = rbEx; - } - - // If rollback threw the IDbContextTransaction is in an indeterminate - // state and still associated with the DbContext — a subsequent - // SaveChangesAsync would attempt to enlist in (or commit to) that - // broken transaction, and the failure-row would itself be rolled - // back when the transaction is finally disposed. Dispose it now so - // the audit-row write below uses a fresh implicit transaction. On - // the happy rollback path Dispose is a benign no-op (the using - // would call it on scope exit anyway). - if (rollbackFailure is not null) - { - try { await tx.DisposeAsync().ConfigureAwait(false); } - catch { /* dispose-after-throw must not mask the original cause */ } - } - - // Clear the change tracker before writing the failure row — on the - // in-memory provider the rollback is a no-op and the staged adds - // would otherwise persist when the next SaveChangesAsync runs. This - // also matters when rollback threw: the change tracker is in an - // ambiguous state and we don't want the failure-write to sweep up - // any of the staged apply mutations. - _dbContext.ChangeTracker.Clear(); - + // The strategy exhausted its retries (or threw a non-transient + // error). The failed attempt already rolled back and cleared the + // change tracker inside the delegate, so all that remains here is + // the top-level BundleImportFailed audit row. + // // Clear correlation FIRST so the failure row doesn't carry the now- // rolled-back BundleImportId. The contract is: BundleImportFailed // exists at top level (no correlation) so audit consumers can see @@ -1549,7 +1380,7 @@ public sealed class BundleImporter : IBundleImporter BundleImportId = bundleImportId, Reason = ex.Message, ExceptionType = ex.GetType().FullName, - RollbackException = rollbackFailure?.Message, + RollbackException = rollbackFailureMessage, }, cancellationToken: ct).ConfigureAwait(false); await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); @@ -1580,6 +1411,236 @@ public sealed class BundleImporter : IBundleImporter // not inherit the import id. _correlationContext.BundleImportId = null; } + + // Post-commit side effects — run ONCE, only after a durable commit, so a + // retried attempt neither publishes twice nor releases the session early. + PublishScriptArtifactChanges(resolutions); + + // T-007: zero out the decrypted plaintext BEFORE remove so any + // caller-held reference (e.g. the Razor page that built the + // ImportPreview) sees the cleared buffer too. Remove drops the + // dictionary entry; together they release the secrets immediately + // instead of leaving them in process memory for the full TTL. + ZeroDecryptedContent(session); + _sessionStore.Remove(sessionId); + + return result; + } + + /// + /// Applies one bundle's merge inside the caller's transaction: semantic + /// validation, every Apply* pass, the name-keyed FK rewires, the stale- + /// instance computation and the BundleImported audit row, ending with the + /// deferred final SaveChangesAsync staged by the caller. Does NOT begin or + /// commit the transaction — owns it so the whole + /// unit can run inside the context's execution strategy (EnableRetryOnFailure + /// rejects user-initiated transactions opened outside the strategy). Safe to + /// re-run: the caller clears the change tracker before each attempt, and + /// every accumulator here (summary counts, resolution map) is rebuilt per + /// call. + /// + private async Task ApplyMergeAsync( + BundleContentDto content, + IReadOnlyList resolutions, + BundleNameMap nameMap, + BundleSession session, + Guid bundleImportId, + string user, + CancellationToken ct) + { + var resolutionMap = resolutions.ToDictionary( + r => (r.EntityType, r.Name), + r => r); + var summary = new ImportSummary(); + + // Inbound API keys are not transported between environments. + // A legacy bundle may still contain a keys section; we ignore those keys + // entirely (never re-create them) but count them so the result can tell + // the operator to re-issue keys on this environment. + var apiKeysIgnored = content.ApiKeys?.Count ?? 0; + + // Run semantic validation FIRST — before any writes are staged. + // This is purely a name-resolution scan over the in-memory DTOs + + // pre-existing target reads, so it has no ordering dependency on + // the Apply* helpers. Failing here means the change tracker is + // still empty, which keeps the rollback contract simple on both + // the in-memory and relational providers (intermediate + // SaveChangesAsync between Apply* and the second-pass rewire + // would otherwise prevent the in-memory provider from undoing + // already-flushed template rows on validation failure — the + // in-memory transaction is a no-op, so the only safe pattern is + // ONE deferred SaveChangesAsync at the very end of try-block). + // + // Skip-resolved DTOs are excluded from the in-bundle name set so + // a Skip on a dependency surfaces as a missing-reference error + // rather than silently passing. + var (validationErrors, validationWarnings) = + await RunSemanticValidationAsync(content, resolutionMap, ct).ConfigureAwait(false); + // Validate every site / connection / template reference the + // site-instance payload depends on BEFORE any row is staged. Running + // this in the validation phase (not as an apply-pass guard) preserves + // the rollback contract: a structurally-unresolvable bundle fails with + // an empty change tracker, so nothing is half-written on the in-memory + // provider (where the intermediate site/connection flush can't be + // undone by ChangeTracker.Clear). The apply passes keep defensive + // guards, but in normal operation those never fire. + validationErrors = validationErrors.Count > 0 + ? validationErrors + : await ValidateSiteInstanceReferencesAsync(content, resolutionMap, nameMap, ct).ConfigureAwait(false); + if (validationErrors.Count > 0) + { + throw new SemanticValidationException(validationErrors); + } + + // Advisory template-script reference findings never block the import; + // log them and surface them on the ImportResult so the operator sees + // the same advisory the preview showed. The deploy-time gate is the + // authoritative re-validation. + if (validationWarnings.Count > 0) + { + _logger?.LogWarning( + "Bundle import {BundleImportId}: {Count} advisory import warning(s): {Warnings}", + bundleImportId, validationWarnings.Count, string.Join("; ", validationWarnings)); + } + + // ---- Site/instance-scoped apply: sites + connections FIRST ---- + // Sites are the FK target for both data connections (SiteId) and + // instances (SiteId); data connections are the FK target for instance + // connection bindings (DataConnectionId) and the rewrite target for + // native-alarm-source ConnectionNameOverride. Resolve-or-create both + // BEFORE the central-config apply so the maps the instance pass needs + // are fully populated, then flush so newly-created site/connection ids + // materialise on the relational provider. (The instance pass itself + // runs AFTER the central-config flush so it can also resolve template + // ids by name.) + var siteBySourceIdentifier = await ApplySitesAsync( + content, nameMap, resolutionMap, user, summary, ct).ConfigureAwait(false); + var connectionMaps = await ApplyDataConnectionsAsync( + content, nameMap, siteBySourceIdentifier, resolutionMap, user, summary, ct).ConfigureAwait(false); + // Flush so site + connection surrogate ids are assigned (relational + // provider) before the instance pass wires its FKs. In-memory assigns + // ids on AddAsync, so this is mostly a no-op there, but it keeps the + // ordering correct on a real DB. Rides the same outer transaction. + await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); + + await ApplyTemplateFoldersAsync(content.TemplateFolders, resolutionMap, user, summary, ct).ConfigureAwait(false); + await ApplyTemplatesAsync(content.Templates, resolutionMap, user, summary, ct).ConfigureAwait(false); + await ApplySharedScriptsAsync(content.SharedScripts, resolutionMap, user, summary, ct).ConfigureAwait(false); + await ApplyExternalSystemsAsync(content.ExternalSystems, resolutionMap, user, summary, ct).ConfigureAwait(false); + await ApplyDatabaseConnectionsAsync(content.DatabaseConnections, resolutionMap, user, summary, ct).ConfigureAwait(false); + await ApplyNotificationListsAsync(content.NotificationLists, resolutionMap, user, summary, ct).ConfigureAwait(false); + await ApplySmtpConfigsAsync(content.SmtpConfigs, resolutionMap, user, summary, ct).ConfigureAwait(false); + await ApplySmsConfigsAsync(content.SmsConfigs, resolutionMap, user, summary, ct).ConfigureAwait(false); + // Inbound API keys are NOT applied from a bundle — any keys + // in a legacy bundle were counted above (apiKeysIgnored) and are skipped. + await ApplyApiMethodsAsync(content.ApiMethods, resolutionMap, user, summary, ct).ConfigureAwait(false); + + // Second-pass rewire of name-keyed + // FKs that can only be resolved AFTER every template's scripts and + // child rows have been staged. We flush here so Pass A can look up + // the just-persisted scripts by name and Pass B can look up the + // just-persisted templates by name; both passes stage further + // mutations that ride the SAME outer transaction (committed below). + // + // EF tracking note: AddAsync on the in-memory provider assigns + // synthetic ids eagerly, so this intermediate flush mostly + // materialises identity values on a relational provider. The + // rollback contract is preserved because semantic validation + // already ran above — any throw from this point onward represents + // a successful merge that the user wants to keep, and the only + // remaining failure surface is the final BundleImported audit + // write itself. + await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); + await ResolveAlarmScriptLinksAsync(content.Templates, resolutionMap, user, ct).ConfigureAwait(false); + await ResolveCompositionEdgesAsync(content.Templates, resolutionMap, user, ct).ConfigureAwait(false); + await ResolveInheritanceEdgesAsync(content.Templates, resolutionMap, user, ct).ConfigureAwait(false); + // Folder parent edges are name-keyed too — resolve them AFTER the flush + // above so every imported folder has a materialised id to point at + // (the Add pass created the folders parent-less). Mirrors the template + // inheritance/composition rewires. + await ResolveFolderParentEdgesAsync(content.TemplateFolders, resolutionMap, user, ct).ConfigureAwait(false); + // External-system methods have no navigation on the parent definition, + // so an Add can't cascade them — persist them here, once the parent + // system has a real id from the flush above. (Overwrite already syncs + // its methods inline against the existing parent id.) + await PersistAddedExternalSystemMethodsAsync(content.ExternalSystems, resolutionMap, user, ct).ConfigureAwait(false); + + // ---- Import-time acyclicity guard over the merged template graph ---- + // The two rewire passes above have STAGED (not yet saved) the bundle's + // inheritance (ParentTemplateId) and composition edges onto the tracked + // target templates. A bundle can still close a loop through a pre-existing + // target template — e.g. Overwrite a base so it now inherits from (or + // composes) one of its own descendants. The design-time save-gate enforces + // acyclicity on individual edits, but an import merges many edges at once, + // so re-check the whole merged graph here. On a cycle we throw and the + // catch below rolls the whole import back (ChangeTracker.Clear on the + // in-memory provider) BEFORE any of it is persisted; deliberately no + // SaveChanges precedes this check so nothing survives that rollback. + EnsureNoTemplateGraphCycles(); + + // ---- Site/instance-scoped apply: instances LAST ---- + // The instance pass needs every FK target materialised first: + // • template id — resolved by name against the just-flushed central + // config (Templates were flushed above for the alarm/composition + // rewire), plus pre-existing target templates; + // • site id — from the site map built by ApplySitesAsync; + // • connection id — from the connection map built by + // ApplyDataConnectionsAsync. + // It writes the instance row + its four child collections (attribute / + // alarm / native-alarm-source overrides + connection bindings) and + // rewires every name-keyed FK to the target environment's ids. + await ApplyInstancesAsync( + content, resolutionMap, siteBySourceIdentifier, connectionMaps, + user, summary, ct).ConfigureAwait(false); + + // ---- Pre-commit point: stale-instance computation ---- + // Everything the import will write is staged on the change tracker at + // this point but NOT yet committed. This computes StaleInstanceIds here + // — target instances whose template was overwritten by this import and + // therefore now carry a stale flattened-config revision — and threads + // the resulting list into the ImportResult below (replacing the + // Array.Empty() placeholder). The single deferred SaveChangesAsync + // is the next statement, so a read against the change tracker here sees + // the full post-apply graph before commit. + var staleInstanceIds = await ComputeStaleInstanceIdsAsync( + content, resolutionMap, ct).ConfigureAwait(false); + + await _auditService.LogAsync( + user: user, + action: "BundleImported", + entityType: "Bundle", + entityId: bundleImportId.ToString(), + entityName: session.Manifest.SourceEnvironment, + afterState: new + { + BundleImportId = bundleImportId, + session.Manifest.SourceEnvironment, + session.Manifest.ContentHash, + Summary = new + { + summary.Added, + summary.Overwritten, + summary.Skipped, + summary.Renamed, + }, + // Legacy inbound API keys present in the bundle that + // were ignored (not transported / not re-created here). + ApiKeysIgnored = apiKeysIgnored, + }, + cancellationToken: ct).ConfigureAwait(false); + + await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); + + return new ImportResult( + BundleImportId: bundleImportId, + Added: summary.Added, + Overwritten: summary.Overwritten, + Skipped: summary.Skipped, + Renamed: summary.Renamed, + StaleInstanceIds: staleInstanceIds, + AuditEventCorrelation: bundleImportId.ToString(), + ApiKeysIgnored: apiKeysIgnored, + Warnings: validationWarnings); } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterRetryingStrategyTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterRetryingStrategyTests.cs new file mode 100644 index 00000000..8b8eeb4e --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterRetryingStrategyTests.cs @@ -0,0 +1,219 @@ +using System.Data.Common; +using Microsoft.AspNetCore.DataProtection; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Storage; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Scripts; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport; +using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase; +using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories; +using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Services; +using ZB.MOM.WW.ScadaBridge.DeploymentManager; +using ZB.MOM.WW.ScadaBridge.TemplateEngine; +using ZB.MOM.WW.ScadaBridge.Transport; +using ZB.MOM.WW.ScadaBridge.Transport.Import; + +namespace ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests.Import; + +/// +/// Regression guard for Gitea #28: must +/// run its multi-SaveChanges apply through the context's execution +/// strategy, because the production central ScadaBridgeDbContext is +/// configured with EnableRetryOnFailure and +/// SqlServerRetryingExecutionStrategy REJECTS a user-initiated +/// transaction opened outside the strategy +/// (CoreStrings.ExecutionStrategyExistingTransaction) — so before the +/// fix every bundle import threw on any real SQL Server. +/// +/// The rest of the Transport suite runs on the in-memory provider, which has NO +/// retrying strategy and where BeginTransactionAsync is a no-op, so it +/// could never surface this. Here we run on SQLite (real relational +/// transactions) AND configure a retrying execution strategy whose only job is +/// to have RetriesOnFailure == true, which arms the exact same guard as +/// the production SQL Server strategy. If ApplyAsync opened the +/// transaction directly, this test would throw before importing anything. +/// +/// +/// Reuses defined alongside +/// in this assembly. +/// +/// +public sealed class BundleImporterRetryingStrategyTests : IDisposable +{ + private readonly ServiceProvider _provider; + private readonly DbConnection _sharedConnection; + + public BundleImporterRetryingStrategyTests() + { + var services = new ServiceCollection(); + services.AddSingleton( + new ConfigurationBuilder().AddInMemoryCollection().Build()); + + // SQLite :memory: is per-connection, so pin one open connection for the + // whole fixture or the schema vanishes between DbContext instances. + _sharedConnection = new Microsoft.Data.Sqlite.SqliteConnection("DataSource=:memory:"); + _sharedConnection.Open(); + + services.AddSingleton(new EphemeralDataProtectionProvider()); + + services.AddSingleton(sp => + { + var builder = new DbContextOptionsBuilder(); + // Configure a RETRYING execution strategy — this is the crux of the + // regression. RetriesOnFailure == true (MaxRetryCount > 0) makes EF + // reject any user-initiated BeginTransaction opened outside + // strategy.ExecuteAsync, exactly like the production SQL Server + // EnableRetryOnFailure configuration. + builder.UseSqlite( + _sharedConnection, + sqlite => sqlite.ExecutionStrategy(deps => new TestRetryingExecutionStrategy(deps))); + builder.ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning)); + return builder.Options; + }); + services.AddScoped(sp => new SqliteCompatibleScadaBridgeDbContext( + sp.GetRequiredService>(), + sp.GetRequiredService())); + + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddTransport(); + // IStaleInstanceProbe (DeploymentManager) + flatten/hash (TemplateEngine) + // so ApplyAsync's StaleInstanceIds computation resolves a real probe. + services.AddTemplateEngine(); + services.AddDeploymentManager(); + + _provider = services.BuildServiceProvider(); + + using var scope = _provider.CreateScope(); + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.Database.EnsureCreated(); + } + + public void Dispose() + { + _provider.Dispose(); + _sharedConnection.Dispose(); + } + + [Fact] + public async Task ApplyAsync_succeeds_under_a_retrying_execution_strategy() + { + // Arrange: seed → export → wipe → apply(Add). Same shape as + // BundleImporterApplyTests.ApplyAsync_adds_new_artifacts_in_single_transaction, + // but the fixture's context carries a retrying execution strategy. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.SharedScripts.Add(new SharedScript("HelperFn", "return 1;")); + ctx.Templates.Add(new Template("Pump") { Description = "fresh" }); + await ctx.SaveChangesAsync(); + } + + var sessionId = await ExportAndLoadAsync(); + await WipeContentAsync(); + + // Act: before the fix this threw CoreStrings.ExecutionStrategyExistingTransaction + // ("... does not support user-initiated transactions.") from the direct + // BeginTransactionAsync. With the fix the apply runs inside the strategy. + ImportResult result; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + var resolutions = new List + { + new("Template", "Pump", ResolutionAction.Add, null), + new("SharedScript", "HelperFn", ResolutionAction.Add, null), + }; + result = await importer.ApplyAsync(sessionId, resolutions, user: "bob"); + } + + // Assert: the whole transactional apply committed. + Assert.Equal(2, result.Added); + Assert.Equal(0, result.Overwritten); + Assert.NotEqual(Guid.Empty, result.BundleImportId); + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + Assert.Equal(1, await ctx.Templates.CountAsync(t => t.Name == "Pump")); + Assert.Equal(1, await ctx.SharedScripts.CountAsync(s => s.Name == "HelperFn")); + // The BundleImported audit row landed inside the committed transaction. + Assert.Equal(1, await ctx.AuditLogEntries.CountAsync(a => a.Action == "BundleImported")); + } + } + + // ---- helpers (mirror BundleImporterApplyTests) ---- + + private async Task ExportAndLoadAsync() + { + Stream bundleStream; + await using (var scope = _provider.CreateAsyncScope()) + { + var exporter = scope.ServiceProvider.GetRequiredService(); + var ctx = scope.ServiceProvider.GetRequiredService(); + var templateIds = await ctx.Templates.Select(t => t.Id).ToListAsync(); + var sharedScriptIds = await ctx.SharedScripts.Select(s => s.Id).ToListAsync(); + var selection = new ExportSelection( + TemplateIds: templateIds, + SharedScriptIds: sharedScriptIds, + ExternalSystemIds: Array.Empty(), + DatabaseConnectionIds: Array.Empty(), + NotificationListIds: Array.Empty(), + SmtpConfigurationIds: Array.Empty(), + ApiMethodIds: Array.Empty(), + IncludeDependencies: false); + bundleStream = await exporter.ExportAsync(selection, user: "alice", sourceEnvironment: "dev", + passphrase: null, cancellationToken: CancellationToken.None); + } + + using var ms = new MemoryStream(); + await bundleStream.CopyToAsync(ms); + ms.Position = 0; + + await using var loadScope = _provider.CreateAsyncScope(); + var importer = loadScope.ServiceProvider.GetRequiredService(); + var session = await importer.LoadAsync(ms, passphrase: null); + return session.SessionId; + } + + private async Task WipeContentAsync() + { + await using var scope = _provider.CreateAsyncScope(); + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.Templates.RemoveRange(ctx.Templates); + ctx.SharedScripts.RemoveRange(ctx.SharedScripts); + ctx.TemplateFolders.RemoveRange(ctx.TemplateFolders); + await ctx.SaveChangesAsync(); + } + + /// + /// A minimal retrying execution strategy for the test. Its sole purpose is + /// to report RetriesOnFailure == true (inherited from + /// , since MaxRetryCount > 0), which + /// arms EF's user-initiated-transaction guard just like the production + /// SqlServerRetryingExecutionStrategy. It never actually retries — + /// returns false — so a genuine fault surfaces + /// immediately instead of looping. + /// + private sealed class TestRetryingExecutionStrategy : ExecutionStrategy + { + public TestRetryingExecutionStrategy(ExecutionStrategyDependencies dependencies) + : base(dependencies, maxRetryCount: 3, maxRetryDelay: TimeSpan.FromMilliseconds(1)) + { + } + + protected override bool ShouldRetryOn(Exception exception) => false; + } +}