diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs index af682e06..5557b7b5 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs @@ -40,17 +40,18 @@ public static Command CreateCommand( cleanupCommand.AddOption(configOption); cleanupCommand.AddOption(verboseOption); - // Generate correlation ID at workflow entry point - var correlationId = HttpClientFactory.GenerateCorrelationId(); - // Set default handler for 'a365 cleanup' (without subcommand) - cleans up everything cleanupCommand.SetHandler(async (configFile, verbose) => { + // Generate correlation ID at workflow entry point + var correlationId = HttpClientFactory.GenerateCorrelationId(); + logger.LogInformation("Starting cleanup (CorrelationId: {CorrelationId})", correlationId); + await ExecuteAllCleanupAsync(logger, configService, botConfigurator, executor, agentBlueprintService, confirmationProvider, federatedCredentialService, configFile, correlationId: correlationId); }, configOption, verboseOption); // Add subcommands for granular control - cleanupCommand.AddCommand(CreateBlueprintCleanupCommand(logger, configService, botConfigurator, executor, agentBlueprintService, federatedCredentialService, correlationId: correlationId)); + cleanupCommand.AddCommand(CreateBlueprintCleanupCommand(logger, configService, botConfigurator, executor, agentBlueprintService, federatedCredentialService)); cleanupCommand.AddCommand(CreateAzureCleanupCommand(logger, configService, executor)); cleanupCommand.AddCommand(CreateInstanceCleanupCommand(logger, configService, executor)); @@ -63,8 +64,7 @@ private static Command CreateBlueprintCleanupCommand( IBotConfigurator botConfigurator, CommandExecutor executor, AgentBlueprintService agentBlueprintService, - FederatedCredentialService federatedCredentialService, - string? correlationId = null) + FederatedCredentialService federatedCredentialService) { var command = new Command("blueprint", "Remove Entra ID blueprint application and service principal"); @@ -91,6 +91,10 @@ private static Command CreateBlueprintCleanupCommand( { try { + // Generate correlation ID at workflow entry point + var correlationId = HttpClientFactory.GenerateCorrelationId(); + logger.LogInformation("Starting blueprint cleanup (CorrelationId: {CorrelationId})", correlationId); + var config = await LoadConfigAsync(configFile, logger, configService); if (config == null) return; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs index a254f617..a62f71a6 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -197,7 +197,8 @@ await UpdateEndpointAsync( logger: logger, configService: configService, botConfigurator: botConfigurator, - platformDetector: platformDetector); + platformDetector: platformDetector, + correlationId: correlationId); } catch (Agent365Exception ex) { @@ -1859,13 +1860,15 @@ await ProjectSettingsSyncHelper.ExecuteAsync( /// Configuration service /// Bot configurator service /// Platform detector service + /// Optional correlation ID for tracing public static async Task UpdateEndpointAsync( string configPath, string newEndpointUrl, ILogger logger, IConfigService configService, IBotConfigurator botConfigurator, - PlatformDetector platformDetector) + PlatformDetector platformDetector, + string? correlationId = null) { var setupConfig = await configService.LoadAsync(configPath); @@ -1887,6 +1890,12 @@ public static async Task UpdateEndpointAsync( logger.LogInformation("Updating messaging endpoint..."); logger.LogInformation(""); + // Normalize location once; used by both Step 1 and Step 1.5. + // Null-coalescing is intentional: Location is only validated inside the Step 1 block (not here), + // so it may still be null at this point. The empty-string fallback is never passed to any API — + // Step 1 throws before using it, and Step 1.5 guards on !IsNullOrWhiteSpace(Location). + var normalizedLocation = setupConfig.Location?.Replace(" ", "").ToLowerInvariant() ?? string.Empty; + // Step 1: Delete existing endpoint if it exists if (!string.IsNullOrWhiteSpace(setupConfig.MessagingEndpoint) || !string.IsNullOrWhiteSpace(setupConfig.BotName)) { @@ -1916,12 +1925,11 @@ public static async Task UpdateEndpointAsync( endpointName = Services.Helpers.EndpointHelper.GetEndpointName(setupConfig.BotName); } - var normalizedLocation = setupConfig.Location.Replace(" ", "").ToLowerInvariant(); - var deleted = await botConfigurator.DeleteEndpointWithAgentBlueprintAsync( endpointName, normalizedLocation, - setupConfig.AgentBlueprintId); + setupConfig.AgentBlueprintId, + correlationId: correlationId); if (!deleted) { @@ -1936,12 +1944,29 @@ public static async Task UpdateEndpointAsync( logger.LogInformation("No existing endpoint found. Proceeding with registration."); } + // Step 1.5: Pre-create cleanup of the target endpoint name. + // If a previous --update-endpoint failed during the create step, Azure may have + // partially provisioned the new endpoint and left it in a bad state that blocks + // subsequent creates with InternalServerError. Delete it now to ensure a clean slate. + if (!setupConfig.NeedDeployment && !string.IsNullOrWhiteSpace(setupConfig.Location)) + { + var targetEndpointName = Services.Helpers.EndpointHelper.GetEndpointNameFromUrl(newEndpointUrl, setupConfig.AgentBlueprintId); + logger.LogInformation("Removing target endpoint '{EndpointName}' (derived from {Url}) to ensure a clean state before registration.", targetEndpointName, newEndpointUrl); + var preCleanupDeleted = await botConfigurator.DeleteEndpointWithAgentBlueprintAsync(targetEndpointName, normalizedLocation, setupConfig.AgentBlueprintId, correlationId: correlationId); + if (!preCleanupDeleted) + { + // Not fatal — proceed and let Step 2 surface the error if the partially-provisioned + // endpoint is still blocking. The warning helps diagnose production issues. + logger.LogWarning("Pre-create cleanup for '{EndpointName}' did not confirm deletion. Proceeding anyway.", targetEndpointName); + } + } + // Step 2: Register new endpoint with the provided URL logger.LogInformation(""); logger.LogInformation("Registering new messaging endpoint..."); var (endpointRegistered, _) = await SetupHelpers.RegisterBlueprintMessagingEndpointAsync( - setupConfig, logger, botConfigurator, newEndpointUrl); + setupConfig, logger, botConfigurator, newEndpointUrl, correlationId: correlationId); if (!endpointRegistered) { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/BotConfigurator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/BotConfigurator.cs index 9564d112..d59f6829 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/BotConfigurator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/BotConfigurator.cs @@ -206,8 +206,8 @@ public async Task DeleteEndpointWithAgentBlueprintAsync( string? correlationId = null) { _logger.LogInformation("Deleting endpoint with Agent Blueprint Identity..."); - _logger.LogDebug(" Endpoint Name: {EndpointName}", endpointName); - _logger.LogDebug(" Agent Blueprint ID: {AgentBlueprintId}", agentBlueprintId); + _logger.LogInformation(" Endpoint Name: {EndpointName}", endpointName); + _logger.LogInformation(" Agent Blueprint ID: {AgentBlueprintId}", agentBlueprintId); if (string.IsNullOrWhiteSpace(location)) { @@ -288,6 +288,12 @@ public async Task DeleteEndpointWithAgentBlueprintAsync( // Call the endpoint _logger.LogInformation("Making request to delete endpoint (Location: {Location}).", normalizedLocation); + _logger.LogInformation("Delete request payload:"); + _logger.LogInformation(" AzureBotServiceInstanceName: {Name}", endpointName); + _logger.LogInformation(" AppId: {AppId}", agentBlueprintId); + _logger.LogInformation(" TenantId: {TenantId}", tenantId); + _logger.LogInformation(" Location: {Location}", normalizedLocation); + _logger.LogInformation(" Environment: {Environment}", EndpointHelper.GetDeploymentEnvironment(config.Environment)); using var request = new HttpRequestMessage(HttpMethod.Delete, deleteEndpointUrl); request.Content = new StringContent(deleteEndpointBody.ToJsonString(), System.Text.Encoding.UTF8, "application/json"); diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs index 3791711d..46af84e2 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs @@ -5,6 +5,7 @@ using Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; using Microsoft.Agents.A365.DevTools.Cli.Models; using Microsoft.Agents.A365.DevTools.Cli.Services; +using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; using Microsoft.Extensions.Logging; using NSubstitute; using System.CommandLine; @@ -1462,7 +1463,7 @@ await _mockBotConfigurator.DidNotReceive().CreateEndpointWithAgentBlueprintAsync } [Fact] - public async Task UpdateEndpointAsync_WithNoExistingEndpoint_ShouldSkipDeleteAndRegister() + public async Task UpdateEndpointAsync_WithNoExistingOldEndpoint_ShouldOnlyCallPreCreateCleanup() { // Arrange - Config without BotName (no existing endpoint) var config = new Agent365Config @@ -1490,6 +1491,10 @@ public async Task UpdateEndpointAsync_WithNoExistingEndpoint_ShouldSkipDeleteAnd _mockConfigService.SaveStateAsync(Arg.Any(), Arg.Any()) .Returns(Task.CompletedTask); + _mockBotConfigurator.DeleteEndpointWithAgentBlueprintAsync( + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); // NotFound = success for pre-create cleanup + _mockBotConfigurator.CreateEndpointWithAgentBlueprintAsync( Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(EndpointRegistrationResult.Created); @@ -1503,13 +1508,100 @@ await BlueprintSubcommand.UpdateEndpointAsync( _mockBotConfigurator, _mockPlatformDetector); - // Assert - Should NOT call delete (no existing endpoint) - await _mockBotConfigurator.DidNotReceive().DeleteEndpointWithAgentBlueprintAsync( + // Assert - Step 1 (delete old) is skipped — no existing endpoint to delete. + // Step 1.5 (pre-create cleanup) still calls delete exactly once with the TARGET endpoint name, + // so there is exactly one delete call total. + var expectedTargetName = EndpointHelper.GetEndpointNameFromUrl(newEndpointUrl, config.AgentBlueprintId); + await _mockBotConfigurator.Received(1).DeleteEndpointWithAgentBlueprintAsync( + expectedTargetName, + "eastus", + config.AgentBlueprintId); + + // Should still register the new endpoint + await _mockBotConfigurator.Received(1).CreateEndpointWithAgentBlueprintAsync( Arg.Any(), Arg.Any(), - Arg.Any()); + newEndpointUrl, + Arg.Any(), + config.AgentBlueprintId); + } + finally + { + if (File.Exists(generatedPath)) File.Delete(generatedPath); + if (File.Exists(configPath)) File.Delete(configPath); + } + } - // Should still register the new endpoint + [Fact] + public async Task UpdateEndpointAsync_WithExistingOldEndpointAndPartiallyProvisionedTarget_ShouldCallDeleteTwice() + { + // Regression: when a prior --update-endpoint failed during the create step, Azure may have + // partially provisioned the new endpoint. On the next run, BOTH Step 1 (delete old) and + // Step 1.5 (pre-create cleanup of target) must fire, targeting different endpoint names. + var currentlyRegisteredUrl = "https://currently-registered-3979.inc1.devtunnels.ms/api/messages"; + var newEndpointUrl = "https://newtunnel-3979.inc1.devtunnels.ms/api/messages"; + + var config = new Agent365Config + { + TenantId = "00000000-0000-0000-0000-000000000000", + AgentBlueprintId = "blueprint-123", + MessagingEndpoint = currentlyRegisteredUrl, // static config (original tunnel) + BotMessagingEndpoint = currentlyRegisteredUrl, // generated config (last successful registration) + Location = "eastus", + NeedDeployment = false, + DeploymentProjectPath = Path.GetTempPath() + }; + + var testId = Guid.NewGuid().ToString(); + var configPath = Path.Combine(Path.GetTempPath(), $"test-config-{testId}.json"); + var generatedPath = Path.Combine(Path.GetTempPath(), $"a365.generated.config-{testId}.json"); + + await File.WriteAllTextAsync(generatedPath, "{}"); + + try + { + _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(config)); + + _mockConfigService.SaveStateAsync(Arg.Any(), Arg.Any()) + .Returns(Task.CompletedTask); + + _mockBotConfigurator.DeleteEndpointWithAgentBlueprintAsync( + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + + _mockBotConfigurator.CreateEndpointWithAgentBlueprintAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(EndpointRegistrationResult.Created); + + // Act + await BlueprintSubcommand.UpdateEndpointAsync( + configPath, + newEndpointUrl, + _mockLogger, + _mockConfigService, + _mockBotConfigurator, + _mockPlatformDetector); + + // Assert — exactly two delete calls with distinct endpoint names + var oldEndpointName = EndpointHelper.GetEndpointNameFromUrl(currentlyRegisteredUrl, config.AgentBlueprintId); + var targetEndpointName = EndpointHelper.GetEndpointNameFromUrl(newEndpointUrl, config.AgentBlueprintId); + + oldEndpointName.Should().NotBe(targetEndpointName, "Step 1 and Step 1.5 must target different endpoints"); + + // Step 1: delete the currently-registered (old) endpoint + await _mockBotConfigurator.Received(1).DeleteEndpointWithAgentBlueprintAsync( + oldEndpointName, "eastus", config.AgentBlueprintId); + + // Step 1.5: pre-create cleanup of the partially-provisioned target endpoint + await _mockBotConfigurator.Received(1).DeleteEndpointWithAgentBlueprintAsync( + targetEndpointName, "eastus", config.AgentBlueprintId); + + // Total: exactly two delete calls + await _mockBotConfigurator.Received(2).DeleteEndpointWithAgentBlueprintAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + + // Step 2: register the new endpoint await _mockBotConfigurator.Received(1).CreateEndpointWithAgentBlueprintAsync( Arg.Any(), Arg.Any(), diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/EndpointHelperTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/EndpointHelperTests.cs index 23583b8f..7a3df494 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/EndpointHelperTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Helpers/EndpointHelperTests.cs @@ -165,6 +165,23 @@ public void GetEndpointNameFromUrl_WithNgrokUrl_ReturnsExpectedName() result.Should().Be(expected); } + [Fact] + public void GetEndpointNameFromUrl_DifferentDevTunnelUrls_ProduceDifferentNames() + { + // Regression: second --update-endpoint must delete the currently registered endpoint, + // not the original MessagingEndpoint. Different URLs must yield different names so + // the delete step targets the right Azure resource. + var originalUrl = "https://x23kz7ll-3979.inc1.devtunnels.ms/api/messages"; // MessagingEndpoint (static) + var updatedUrl = "https://w3jv62pr-3979.inc1.devtunnels.ms/api/messages"; // BotMessagingEndpoint (after first update) + var blueprintId = "e412c0fa-7127-4615-9bef-1a7ba99e76af"; + + var originalName = EndpointHelper.GetEndpointNameFromUrl(originalUrl, blueprintId); + var updatedName = EndpointHelper.GetEndpointNameFromUrl(updatedUrl, blueprintId); + + updatedName.Should().NotBe(originalName, "deletion must target the currently registered URL, not the static MessagingEndpoint"); + updatedName.Should().Be("w3jv62pr-3979-inc1-devtunnels-ms-e412c0fa"); + } + // GetEndpointNameFromHost tests [Fact]