Updates including:

* Return IsModelPersistedState to Serval
* Check for modelRevision + 1 in clearnup and just delete without keeping internal states.

src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs

@@ -4,7 +4,7 @@ public interface ITranslationEngineService
 {
     TranslationEngineType Type { get; }
 
-    Task CreateAsync(
+    Task<TranslationEngine> CreateAsync(
         string engineId,
         string? engineName,
         string sourceLanguage,

src/SIL.Machine.AspNetCore/Services/ModelCleanupService.cs

@@ -28,13 +28,19 @@ private async Task CheckModelsAsync(CancellationToken cancellationToken)
         );
         // Get all engine ids from the database
         IReadOnlyList<TranslationEngine>? allEngines = await _engines.GetAllAsync(cancellationToken: cancellationToken);
-        HashSet<string> allValidFilenames = allEngines
-            .Select(e => NmtEngineService.GetModelPath(e.EngineId, e.BuildRevision))
-            .ToHashSet();
+        IEnumerable<string> validFilenames = allEngines.Select(e =>
+            NmtEngineService.GetModelPath(e.EngineId, e.BuildRevision)
+        );
+        // If there is a currently running build that creates and pushes a new file, but the database has not
+        // updated yet, don't delete the new file.
+        IEnumerable<string> validFilenamesForNextBuild = allEngines.Select(e =>
+            NmtEngineService.GetModelPath(e.EngineId, e.BuildRevision + 1)
+        );
+        HashSet<string> filenameFilter = validFilenames.Concat(validFilenamesForNextBuild).ToHashSet();
 
         foreach (string path in paths)
         {
-            if (!allValidFilenames.Contains(path))
+            if (!filenameFilter.Contains(path))
             {
                 await DeleteFileAsync(
                     path,
@@ -43,23 +49,13 @@ await DeleteFileAsync(
                 );
             }
         }
-        // roll over the list of files previously marked for deletion
-        _filesPreviouslyMarkedForDeletion = new List<string>(_filesNewlyMarkedForDeletion);
-        _filesNewlyMarkedForDeletion.Clear();
     }
 
     private async Task DeleteFileAsync(string path, string message, CancellationToken cancellationToken = default)
     {
-        // If a file has been requested to be deleted twice, delete it. Otherwise, mark it for deletion.
-        if (_filesPreviouslyMarkedForDeletion.Contains(path))
-        {
-            _logger.LogInformation("Deleting old model file {filename}: {message}", path, message);
-            await _sharedFileService.DeleteAsync(path, cancellationToken);
-        }
-        else
-        {
-            _logger.LogInformation("Marking old model file {filename} for deletion: {message}", path, message);
-            _filesNewlyMarkedForDeletion.Add(path);
-        }
+        // This may delete a file while it is being downloaded, but the chance is rare
+        // enough and the solution easy enough (just download again) to just live with it.
+        _logger.LogInformation("Deleting old model file {filename}: {message}", path, message);
+        await _sharedFileService.DeleteAsync(path, cancellationToken);
     }
 }

src/SIL.Machine.AspNetCore/Services/NmtEngineService.cs

@@ -38,7 +38,7 @@ public static string GetModelPath(string engineId, int buildRevision)
 
     private const int MinutesToExpire = 60;
 
-    public async Task CreateAsync(
+    public async Task<TranslationEngine> CreateAsync(
         string engineId,
         string? engineName,
         string sourceLanguage,
@@ -48,23 +48,22 @@ public async Task CreateAsync(
     )
     {
         await _dataAccessContext.BeginTransactionAsync(cancellationToken);
-        await _engines.InsertAsync(
-            new TranslationEngine
-            {
-                EngineId = engineId,
-                SourceLanguage = sourceLanguage,
-                TargetLanguage = targetLanguage,
-                IsModelPersisted = isModelPersisted ?? false // models are not persisted if not specified
-            },
-            cancellationToken
-        );
+        var translationEngine = new TranslationEngine
+        {
+            EngineId = engineId,
+            SourceLanguage = sourceLanguage,
+            TargetLanguage = targetLanguage,
+            IsModelPersisted = isModelPersisted ?? false // models are not persisted if not specified
+        };
+        await _engines.InsertAsync(translationEngine, cancellationToken);
         await _buildJobService.CreateEngineAsync(
             [BuildJobType.Cpu, BuildJobType.Gpu],
             engineId,
             engineName,
             cancellationToken
         );
         await _dataAccessContext.CommitTransactionAsync(CancellationToken.None);
+        return translationEngine;
     }
 
     public async Task DeleteAsync(string engineId, CancellationToken cancellationToken = default)

src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs

@@ -15,18 +15,18 @@ HealthCheckService healthCheckService
 
     private readonly HealthCheckService _healthCheckService = healthCheckService;
 
-    public override async Task<Empty> Create(CreateRequest request, ServerCallContext context)
+    public override async Task<CreateResponse> Create(CreateRequest request, ServerCallContext context)
     {
         ITranslationEngineService engineService = GetEngineService(request.EngineType);
-        await engineService.CreateAsync(
+        TranslationEngine translationEngine = await engineService.CreateAsync(
             request.EngineId,
             request.HasEngineName ? request.EngineName : null,
             request.SourceLanguage,
             request.TargetLanguage,
             request.HasIsModelPersisted ? request.IsModelPersisted : null,
             context.CancellationToken
         );
-        return Empty;
+        return new CreateResponse { IsModelPersisted = translationEngine.IsModelPersisted };
     }
 
     public override async Task<Empty> Delete(DeleteRequest request, ServerCallContext context)

src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs

@@ -27,7 +27,7 @@ JobStorage jobStorage
 
     public TranslationEngineType Type => TranslationEngineType.SmtTransfer;
 
-    public async Task CreateAsync(
+    public async Task<TranslationEngine> CreateAsync(
         string engineId,
         string? engineName,
         string sourceLanguage,
@@ -42,17 +42,15 @@ public async Task CreateAsync(
                     + "Please remove the isModelPersisted parameter or set it to true."
             );
         await _dataAccessContext.BeginTransactionAsync(cancellationToken);
-        await _engines.InsertAsync(
-            new TranslationEngine
-            {
-                EngineId = engineId,
-                SourceLanguage = sourceLanguage,
-                TargetLanguage = targetLanguage,
-                IsModelPersisted = isModelPersisted ?? true // models are persisted if not specified
-            },
-            cancellationToken
-        );
-        await _buildJobService.CreateEngineAsync(new[] { BuildJobType.Cpu }, engineId, engineName, cancellationToken);
+        var translationEngine = new TranslationEngine
+        {
+            EngineId = engineId,
+            SourceLanguage = sourceLanguage,
+            TargetLanguage = targetLanguage,
+            IsModelPersisted = isModelPersisted ?? true // models are persisted if not specified
+        };
+        await _engines.InsertAsync(translationEngine, cancellationToken);
+        await _buildJobService.CreateEngineAsync([BuildJobType.Cpu], engineId, engineName, cancellationToken);
         await _dataAccessContext.CommitTransactionAsync(CancellationToken.None);
 
         IDistributedReaderWriterLock @lock = await _lockFactory.CreateAsync(engineId, CancellationToken.None);
@@ -61,6 +59,7 @@ await _engines.InsertAsync(
             SmtTransferEngineState state = _stateService.Get(engineId);
             state.InitNew();
         }
+        return translationEngine;
     }
 
     public async Task DeleteAsync(string engineId, CancellationToken cancellationToken = default)

tests/SIL.Machine.AspNetCore.Tests/Services/ModelCleanupServiceTests.cs

@@ -5,10 +5,16 @@ public class ModelCleanupServiceTests
 {
     private readonly ISharedFileService _sharedFileService = new SharedFileService(Substitute.For<ILoggerFactory>());
     private readonly MemoryRepository<TranslationEngine> _engines = new MemoryRepository<TranslationEngine>();
-    private static readonly List<string> validFiles = ["models/engineId1_1.tar.gz", "models/engineId2_2.tar.gz"];
+    private static readonly List<string> validFiles =
+    [
+        "models/engineId1_1.tar.gz",
+        "models/engineId2_2.tar.gz",
+        "models/engineId2_3.tar.gz" // only one build ahead - keep
+    ];
     private static readonly List<string> invalidFiles =
     [
-        "models/engineId2_1.targ.gz", // old build number
+        "models/engineId2_1.targ.gz", // 1 build behind
+        "models/engineId2_4.tar.gz", // 2 builds ahead
         "models/worngId_1.tar.gz",
         "models/engineId1_badbuildnumber.tar.gz",
         "models/noBuildNumber.tar.gz",
@@ -77,14 +83,12 @@ public async Task DoWorkAsync_ValidFiles()
             _engines,
             Substitute.For<ILogger<ModelCleanupService>>()
         );
-        await cleanupJob.DoWorkAsync();
-        // both valid and invalid files still exist after running once
         Assert.That(
             _sharedFileService.ListFilesAsync("models").Result.ToHashSet(),
             Is.EquivalentTo(validFiles.Concat(invalidFiles).ToHashSet())
         );
         await cleanupJob.DoWorkAsync();
-        // only valid files exist after running twice
+        // only valid files exist after running service
         Assert.That(
             _sharedFileService.ListFilesAsync("models").Result.ToHashSet(),
             Is.EquivalentTo(validFiles.ToHashSet())

tests/SIL.Machine.AspNetCore.Tests/Services/NmtEngineServiceTests.cs

@@ -12,8 +12,12 @@ public async Task StartBuildAsync()
         await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty<Corpus>());
         await env.WaitForBuildToFinishAsync();
         engine = env.Engines.Get("engine1");
-        Assert.That(engine.CurrentBuild, Is.Null);
-        Assert.That(engine.BuildRevision, Is.EqualTo(2));
+        Assert.Multiple(() =>
+        {
+            Assert.That(engine.CurrentBuild, Is.Null);
+            Assert.That(engine.BuildRevision, Is.EqualTo(2));
+            Assert.That(engine.IsModelPersisted, Is.False);
+        });
     }
 
     [Test]

tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs

@@ -9,9 +9,13 @@ public async Task CreateAsync()
         using var env = new TestEnvironment();
         await env.Service.CreateAsync("engine2", "Engine 2", "es", "en");
         TranslationEngine? engine = await env.Engines.GetAsync(e => e.EngineId == "engine2");
-        Assert.That(engine, Is.Not.Null);
-        Assert.That(engine.EngineId, Is.EqualTo("engine2"));
-        Assert.That(engine.BuildRevision, Is.EqualTo(0));
+        Assert.Multiple(() =>
+        {
+            Assert.That(engine, Is.Not.Null);
+            Assert.That(engine?.EngineId, Is.EqualTo("engine2"));
+            Assert.That(engine?.BuildRevision, Is.EqualTo(0));
+            Assert.That(engine?.IsModelPersisted, Is.True);
+        });
         env.SmtModelFactory.Received().InitNew("engine2");
         env.TransferEngineFactory.Received().InitNew("engine2");
     }