diff --git a/GitLfsCache.Tests/Storage/ObjectStoreTests.cs b/GitLfsCache.Tests/Storage/ObjectStoreTests.cs index abc8598..8d8e94f 100644 --- a/GitLfsCache.Tests/Storage/ObjectStoreTests.cs +++ b/GitLfsCache.Tests/Storage/ObjectStoreTests.cs @@ -6,6 +6,7 @@ namespace ktsu.GitLfsCache.Tests.Storage; using System.Text; using ktsu.GitLfsCache.Configuration; using ktsu.GitLfsCache.Storage; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using Microsoft.Extensions.Time.Testing; @@ -17,6 +18,9 @@ public class ObjectStoreTests { private static readonly DateTimeOffset Now = new(2026, 8, 18, 12, 0, 0, TimeSpan.Zero); + /// The event id StoreLog.CouldNotPublishObject logs under. + private const int CouldNotPublishObject = 1002; + /// /// An absolute root for whichever platform the suite runs on. AbsoluteDirectoryPath requires a /// fully qualified path, so a hard-coded POSIX root would be rejected on Windows. @@ -25,7 +29,8 @@ public class ObjectStoreTests Path.GetPathRoot(Path.GetTempPath()) ?? Path.DirectorySeparatorChar.ToString(), "gitlfscache-tests"); - private static (ObjectStore Store, MockFileSystem FileSystem, FakeTimeProvider Time) Create() + private static (ObjectStore Store, MockFileSystem FileSystem, FakeTimeProvider Time) Create( + ILogger? logger = null) { MockFileSystem fileSystem = new(); fileSystem.Directory.CreateDirectory(Root); @@ -37,7 +42,11 @@ private static (ObjectStore Store, MockFileSystem FileSystem, FakeTimeProvider T FakeTimeProvider time = new(Now); - ObjectStore store = new(fileSystem, Options.Create(options), time, NullLogger.Instance); + ObjectStore store = new( + fileSystem, + Options.Create(options), + time, + logger ?? NullLogger.Instance); return (store, fileSystem, time); } @@ -127,6 +136,44 @@ public async Task PublishAsync_ObjectAlreadyPresent_SucceedsAndRemovesStaging() Assert.IsFalse(fileSystem.File.Exists(stagingPath)); } + [TestMethod] + public async Task PublishAsync_ObjectArrivesBetweenTheCheckAndTheMove_SucceedsWithoutReportingAFailure() + { + RecordingLogger logger = new(); + (ObjectStore store, MockFileSystem fileSystem, _) = Create(logger); + (byte[] content, string oid) = Content("published twice at once"); + string destination = fileSystem.Path.Combine(Root, "github", "objects", oid[..2], oid[2..4], oid); + + StagingHandle handle = store.OpenStaging("github"); + string stagingPath = handle.Path; + await handle.Stream.WriteAsync(content, CancellationToken.None); + + // Two uploads of one blob are not coordinated, so both can pass the exists check and then both + // rename. Publishing the winner's copy from inside this one's rename puts the object in place + // in exactly that window, which is what the loser of the real race finds. + bool raced = false; + fileSystem.Intercept.Event( + _ => + { + raced = true; + fileSystem.File.WriteAllBytes(destination, content); + }, + change => !raced + && change.ChangeType == WatcherChangeTypes.Renamed + && string.Equals(change.Path, fileSystem.Path.GetFullPath(destination), StringComparison.Ordinal)); + + bool published = await store.PublishAsync(handle, "github", oid, CancellationToken.None); + + Assert.IsTrue(raced, "The other publisher never ran, so the race was not reproduced."); + Assert.IsTrue(published, "Losing the race to a byte-identical object is not a publish failure."); + Assert.IsTrue(store.Exists("github", oid)); + Assert.IsFalse(fileSystem.File.Exists(stagingPath), "Staging must not survive the race."); + CollectionAssert.DoesNotContain( + logger.Events, + CouldNotPublishObject, + "A duplicate publish must not be reported as a publish failure."); + } + [TestMethod] public async Task StagingHandle_DisposedWithoutPublishing_DeletesTheFile() { @@ -329,4 +376,24 @@ public void EnumerateStaging_NoStoreYet_ReturnsNothing() Assert.HasCount(0, store.Enumerate().ToList()); Assert.HasCount(0, store.EnumerateStaging().ToList()); } + + /// + /// Records the event ids the store logs, so a test can assert on what it did not report as well + /// as on what it did. + /// + private sealed class RecordingLogger : ILogger + { + public List Events { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) => Events.Add(eventId.Id); + } } diff --git a/GitLfsCache/Storage/ObjectStore.cs b/GitLfsCache/Storage/ObjectStore.cs index b4ab6da..a892d41 100644 --- a/GitLfsCache/Storage/ObjectStore.cs +++ b/GitLfsCache/Storage/ObjectStore.cs @@ -157,6 +157,19 @@ public async Task PublishAsync( } catch (Exception failure) when (failure is IOException or UnauthorizedAccessException) { + // The check above and the rename are not one atomic step, so another request publishing + // the same object can land between them and the rename then fails on an occupied + // destination. Uploads never go through the fetch coalescer, so two clients pushing one + // blob reach here with nothing coordinating them. Content addressing makes the file that + // arrived byte-identical to this one, which is the same outcome the check reports: the + // winner stands. Reporting it as a failure would have the caller record a verification + // failure, and a benign race is not something to alert on. + if (fileSystem.File.Exists(destination)) + { + await handle.DisposeAsync().ConfigureAwait(false); + return true; + } + StoreLog.CouldNotPublishObject(logger, failure, oid, upstream); await handle.DisposeAsync().ConfigureAwait(false); return false;