From 9012a7b49f97535da0894ce2f08aefe2a2f89927 Mon Sep 17 00:00:00 2001 From: Pavlo Tubolets Date: Wed, 26 Aug 2026 12:48:53 +1000 Subject: [PATCH 1/3] Skip remote content upload when the remote cache is read-only CasCacheClient holds two sessions: the raw _remoteCacheSession and the _twoLevelCacheSession built from TwoLevelCacheConfiguration. When adding a node, content was uploaded directly through _remoteCacheSession without consulting RemoteCacheIsReadOnly, while the content hash list that would reference that content is published through _twoLevelCacheSession, which does honor it. The result is that a read-only client uploads content that can never be retrieved: no content hash list ever points at it, so no subsequent build can hit on it. The upload only costs egress, storage and build time, and leaves unreferenced blobs behind for lifecycle policies to collect. Guard the upload block on the read-only flag, which also elides the remote pin calls that were only made to decide what to upload. Co-Authored-By: Claude Opus 5 (1M context) --- src/Common/Caching/CasCacheClient.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Common/Caching/CasCacheClient.cs b/src/Common/Caching/CasCacheClient.cs index f52f6de..b9e30ab 100644 --- a/src/Common/Caching/CasCacheClient.cs +++ b/src/Common/Caching/CasCacheClient.cs @@ -38,6 +38,7 @@ public sealed class CasCacheClient : CacheClient private readonly ICacheSession? _remoteCacheSession; private readonly ICacheSession _twoLevelCacheSession; + private readonly bool _remoteCacheIsReadOnly; public CasCacheClient( Context rootContext, @@ -65,6 +66,7 @@ public CasCacheClient( else { _remoteCacheSession = remoteCache.Value.session; + _remoteCacheIsReadOnly = remoteCache.Value.config.RemoteCacheIsReadOnly; cacheSession = new TwoLevelCacheSession( nameof(TwoLevelCacheSession), localCacheSession, @@ -202,7 +204,11 @@ static async Task checkUploadResultsAsync(List> uploadTas } // Now that we've ensured everything is in the local cache, we can upload to the remote cache. - if (_remoteCacheSession != null) + // When the remote cache is read-only, skip this entirely: the content hash list which would reference + // this content is not published either (TwoLevelCacheSession honors RemoteCacheIsReadOnly below), so any + // content uploaded here could never be retrieved by a subsequent build. Uploading it would only cost + // egress, storage and time. + if (_remoteCacheSession != null && !_remoteCacheIsReadOnly) { // determine what needs to be uploaded PutFileOperation[] pinResults = await PinBulkAsync(context, _remoteCacheSession, _putRemoteTaskCache, pinContentHashes, cancellationToken); From 9555de7a9b9220e1462dca32110985677e3c6612 Mon Sep 17 00:00:00 2001 From: Pavlo Tubolets Date: Thu, 27 Aug 2026 09:15:08 +1000 Subject: [PATCH 2/3] Add regression tests for read-only remote content upload Adds RecordingCacheSession / RecordingCache, minimal ICacheSession and ICache implementations that count the calls made against them, and two tests driving CasCacheClient.AddNodeInternalAsync: - AddNodeDoesNotUploadContentWhenRemoteCacheIsReadOnly asserts the remote session sees no PinAsync, PutStreamAsync or PutFileAsync calls, while the node metadata and content hash list are still written to the local cache. - AddNodeUploadsContentWhenRemoteCacheIsWritable asserts the remote session is still pinned and uploaded to when the remote cache is writable, so the guard is conditional rather than unconditional. The first test fails without the accompanying fix: Expected:<0>. Actual:<1>. 'actual' expression: 'remoteSession.PinCallCount'. Co-Authored-By: Claude Opus 5 (1M context) --- src/Common.Tests/CasCacheClientTests.cs | 129 +++++++++++++++- src/Common.Tests/Mocks/RecordingCache.cs | 55 +++++++ .../Mocks/RecordingCacheSession.cs | 141 ++++++++++++++++++ 3 files changed, 324 insertions(+), 1 deletion(-) create mode 100644 src/Common.Tests/Mocks/RecordingCache.cs create mode 100644 src/Common.Tests/Mocks/RecordingCacheSession.cs diff --git a/src/Common.Tests/CasCacheClientTests.cs b/src/Common.Tests/CasCacheClientTests.cs index 9a4fff5..eeda5ee 100644 --- a/src/Common.Tests/CasCacheClientTests.cs +++ b/src/Common.Tests/CasCacheClientTests.cs @@ -1,18 +1,30 @@ -// Copyright (c) Microsoft. All rights reserved. +// Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +using System.Collections.Generic; +using System.IO; +using System.Threading; +using System.Threading.Tasks; using BuildXL.Cache.ContentStore.Hashing; +using BuildXL.Cache.ContentStore.Interfaces.Sessions; +using BuildXL.Cache.ContentStore.Interfaces.Tracing; +using BuildXL.Cache.ContentStore.Logging; using BuildXL.Cache.MemoizationStore.Interfaces.Results; using BuildXL.Cache.MemoizationStore.Interfaces.Sessions; using Microsoft.MSBuildCache.Caching; +using Microsoft.MSBuildCache.FileAccess; +using Microsoft.MSBuildCache.Fingerprinting; using Microsoft.VisualStudio.TestTools.UnitTesting; +using Fingerprint = Microsoft.MSBuildCache.Fingerprinting.Fingerprint; namespace Microsoft.MSBuildCache.Tests; [TestClass] public class CasCacheClientTests { + private static readonly string RepoRoot = Path.Combine(Path.GetTempPath(), "MSBuildCacheTests", "repo"); + [TestMethod] public void NullContentHashListMeansSubmittedValueWasAccepted() { @@ -29,4 +41,119 @@ public void ReturnedContentHashListMeansAnotherValueWon() Assert.AreEqual(AddNodeResult.AlreadyExists, CasCacheClient.GetAddNodeResult(result)); } + + [TestMethod] + public async Task AddNodeDoesNotUploadContentWhenRemoteCacheIsReadOnly() + { + (CasCacheClient cacheClient, RecordingCacheSession localSession, RecordingCacheSession remoteSession) = CreateCacheClient(remoteCacheIsReadOnly: true); + + await using (cacheClient) + { + await cacheClient.AddNodeInternalAsync(CreateNodeContext(), pathSet: null, CreateNodeBuildResult(), CancellationToken.None); + } + + Assert.AreEqual(0, remoteSession.PinCallCount, "The remote session must not be pinned when the remote cache is read-only."); + Assert.AreEqual(0, remoteSession.PutStreamCallCount, "Content must not be uploaded when the remote cache is read-only."); + Assert.AreEqual(0, remoteSession.PutFileCallCount, "Content must not be uploaded when the remote cache is read-only."); + + // The node still needs to be written to the local cache. + Assert.IsTrue(localSession.PutStreamCallCount > 0, "The node metadata must still be written to the local cache."); + Assert.AreEqual(1, localSession.AddOrGetContentHashListCallCount, "The content hash list must still be added to the local cache."); + } + + [TestMethod] + public async Task AddNodeUploadsContentWhenRemoteCacheIsWritable() + { + (CasCacheClient cacheClient, RecordingCacheSession localSession, RecordingCacheSession remoteSession) = CreateCacheClient(remoteCacheIsReadOnly: false); + + await using (cacheClient) + { + await cacheClient.AddNodeInternalAsync(CreateNodeContext(), pathSet: null, CreateNodeBuildResult(), CancellationToken.None); + } + + Assert.IsTrue(remoteSession.PinCallCount > 0, "The remote session should be pinned to determine what to upload."); + Assert.IsTrue(remoteSession.PutStreamCallCount > 0, "Content should be uploaded when the remote cache is writable."); + Assert.IsTrue(localSession.PutStreamCallCount > 0, "The node metadata should be written to the local cache."); + } + + private static (CasCacheClient CacheClient, RecordingCacheSession LocalSession, RecordingCacheSession RemoteSession) CreateCacheClient(bool remoteCacheIsReadOnly) + { + IContentHasher hasher = HashInfoLookup.GetContentHasher(HashType.Vso0); + + // The local session reports content as present so nothing needs to be ingested from disk. + RecordingCacheSession localSession = new("local", pinSucceeds: true); + + // The remote session reports content as missing, so a writable remote would upload it. + RecordingCacheSession remoteSession = new("remote", pinSucceeds: false); + +#pragma warning disable CA2000 // Ownership of the caches, sessions and logger is transferred to the cache client, which disposes them. + RecordingCache localCache = new(localSession); + RecordingCache remoteCache = new(remoteSession); + + TwoLevelCacheConfiguration twoLevelCacheConfiguration = new() + { + RemoteCacheIsReadOnly = remoteCacheIsReadOnly, + AlwaysUpdateFromRemote = true, + }; + + CasCacheClient cacheClient = new( + new Context(new Logger()), + new FixedFingerprintFactory(hasher), + localCache, + localSession, + (remoteCache, remoteSession, twoLevelCacheConfiguration), + hasher, + RepoRoot, + nugetPackageRoot: Path.Combine(RepoRoot, "packages"), + getFileRealizationMode: _ => FileRealizationMode.Copy, + maxConcurrentCacheContentOperations: 1, + enableAsyncPublishing: false, + enableAsyncMaterialization: false, + skipUnchangedOutputFiles: false, + touchOutputFiles: false); +#pragma warning restore CA2000 + + return (cacheClient, localSession, remoteSession); + } + + // ProjectInstance is only stored by NodeContext and is not used by the code under test, so this avoids + // depending on MSBuild toolset resolution. + private static NodeContext CreateNodeContext() + => new( + RepoRoot, + projectInstance: null!, + Array.Empty(), + "project.csproj", + new SortedDictionary(StringComparer.OrdinalIgnoreCase), + Array.Empty(), + null, + new HashSet(StringComparer.OrdinalIgnoreCase)); + + private static NodeBuildResult CreateNodeBuildResult() + => new( + new SortedDictionary(StringComparer.OrdinalIgnoreCase) + { + ["output.dll"] = ContentHash.Random(HashType.Vso0), + }, + new SortedDictionary(StringComparer.OrdinalIgnoreCase), + Array.Empty(), + DateTime.UtcNow, + DateTime.UtcNow, + buildId: null); + + private sealed class FixedFingerprintFactory : IFingerprintFactory + { + private readonly Fingerprint _fingerprint; + + public FixedFingerprintFactory(IContentHasher hasher) + => _fingerprint = new Fingerprint(hasher.Info.EmptyHash.ToHashByteArray(), Array.Empty()); + + public Task GetWeakFingerprintAsync(NodeContext nodeContext) => Task.FromResult(_fingerprint); + + public PathSet? GetPathSet(NodeContext nodeContext, IReadOnlyCollection observations) => null; + + public Task GetStrongFingerprintAsync(PathSet? pathSet) => Task.FromResult(_fingerprint); + + public bool MatchesCurrentState(PathSet? cachedPathSet) => true; + } } diff --git a/src/Common.Tests/Mocks/RecordingCache.cs b/src/Common.Tests/Mocks/RecordingCache.cs new file mode 100644 index 0000000..28fea85 --- /dev/null +++ b/src/Common.Tests/Mocks/RecordingCache.cs @@ -0,0 +1,55 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using BuildXL.Cache.ContentStore.Interfaces.Results; +using BuildXL.Cache.ContentStore.Interfaces.Stores; +using BuildXL.Cache.ContentStore.UtilitiesCore; +using BuildXL.Cache.ContentStore.Interfaces.Tracing; +using BuildXL.Cache.MemoizationStore.Interfaces.Caches; +using BuildXL.Cache.MemoizationStore.Interfaces.Sessions; + +namespace Microsoft.MSBuildCache.Tests; + +/// +/// A minimal which hands out a fixed . +/// Members which the tests do not exercise throw. +/// +internal sealed class RecordingCache : ICache +{ + private readonly RecordingCacheSession _session; + + public RecordingCache(RecordingCacheSession session) => _session = session; + + public Guid Id { get; } = Guid.NewGuid(); + + public bool StartupCompleted => true; + + public bool StartupStarted => true; + + public bool ShutdownCompleted { get; private set; } + + public bool ShutdownStarted { get; private set; } + + public Task StartupAsync(Context context) => Task.FromResult(BoolResult.Success); + + public Task ShutdownAsync(Context context) + { + ShutdownStarted = true; + ShutdownCompleted = true; + return Task.FromResult(BoolResult.Success); + } + + public void Dispose() + { + } + + public CreateSessionResult CreateSession(Context context, string name, ImplicitPin implicitPin) + => new(_session); + + public Task GetStatsAsync(Context context) => Task.FromResult(new GetStatsResult(new CounterSet())); + + public IAsyncEnumerable> EnumerateStrongFingerprints(Context context) => throw new NotSupportedException(); +} diff --git a/src/Common.Tests/Mocks/RecordingCacheSession.cs b/src/Common.Tests/Mocks/RecordingCacheSession.cs new file mode 100644 index 0000000..b2e43ae --- /dev/null +++ b/src/Common.Tests/Mocks/RecordingCacheSession.cs @@ -0,0 +1,141 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; +using BuildXL.Cache.ContentStore.Hashing; +using BuildXL.Cache.ContentStore.Interfaces.FileSystem; +using BuildXL.Cache.ContentStore.Interfaces.Results; +using BuildXL.Cache.ContentStore.Interfaces.Sessions; +using BuildXL.Cache.ContentStore.Interfaces.Tracing; +using BuildXL.Cache.MemoizationStore.Interfaces.Results; +using BuildXL.Cache.MemoizationStore.Interfaces.Sessions; + +namespace Microsoft.MSBuildCache.Tests; + +/// +/// An which records the calls made against it so tests can assert which +/// operations a cache client did, or did not, perform. Members which the tests do not exercise throw. +/// +internal sealed class RecordingCacheSession : ICacheSession +{ + private readonly bool _pinSucceeds; + + /// The session name. + /// + /// When true, pins report content as already present, so a caller has nothing to upload. + /// When false, pins report content as missing, so a caller will try to upload it. + /// + public RecordingCacheSession(string name, bool pinSucceeds) + { + Name = name; + _pinSucceeds = pinSucceeds; + } + + public string Name { get; } + + public int PinCallCount { get; private set; } + + public int PutStreamCallCount { get; private set; } + + public int PutFileCallCount { get; private set; } + + public int AddOrGetContentHashListCallCount { get; private set; } + + public bool StartupCompleted => true; + + public bool StartupStarted => true; + + public bool ShutdownCompleted { get; private set; } + + public bool ShutdownStarted { get; private set; } + + public Task StartupAsync(Context context) => Task.FromResult(BoolResult.Success); + + public Task ShutdownAsync(Context context) + { + ShutdownStarted = true; + ShutdownCompleted = true; + return Task.FromResult(BoolResult.Success); + } + + public void Dispose() + { + } + + /* Content session */ + + public Task PinAsync(Context context, ContentHash contentHash, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + { + PinCallCount++; + return Task.FromResult(_pinSucceeds ? PinResult.Success : PinResult.ContentNotFound); + } + + public Task>>> PinAsync(Context context, IReadOnlyList contentHashes, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + { + PinCallCount++; + PinResult result = _pinSucceeds ? PinResult.Success : PinResult.ContentNotFound; + return Task.FromResult(contentHashes.Select((_, index) => Task.FromResult(new Indexed(result, index)))); + } + + public Task>>> PinAsync(Context context, IReadOnlyList contentHashes, PinOperationConfiguration config) + => PinAsync(context, contentHashes, CancellationToken.None); + + public Task OpenStreamAsync(Context context, ContentHash contentHash, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + => Task.FromResult(new OpenStreamResult(new MemoryStream(Array.Empty()))); + + public Task PutStreamAsync(Context context, HashType hashType, Stream stream, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + { + PutStreamCallCount++; + return Task.FromResult(new PutResult(ContentHash.Random(hashType), contentSize: 0)); + } + + public Task PutStreamAsync(Context context, ContentHash contentHash, Stream stream, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + { + PutStreamCallCount++; + return Task.FromResult(new PutResult(contentHash, contentSize: 0)); + } + + public Task PutFileAsync(Context context, HashType hashType, AbsolutePath path, FileRealizationMode realizationMode, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + { + PutFileCallCount++; + return Task.FromResult(new PutResult(ContentHash.Random(hashType), contentSize: 0)); + } + + public Task PutFileAsync(Context context, ContentHash contentHash, AbsolutePath path, FileRealizationMode realizationMode, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + { + PutFileCallCount++; + return Task.FromResult(new PutResult(contentHash, contentSize: 0)); + } + + public Task PlaceFileAsync(Context context, ContentHash contentHash, AbsolutePath path, FileAccessMode accessMode, FileReplacementMode replacementMode, FileRealizationMode realizationMode, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + => throw new NotSupportedException(); + + public Task>>> PlaceFileAsync(Context context, IReadOnlyList hashesWithPaths, FileAccessMode accessMode, FileReplacementMode replacementMode, FileRealizationMode realizationMode, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + => throw new NotSupportedException(); + + /* Memoization session */ + + public async IAsyncEnumerable GetSelectors(Context context, Fingerprint weakFingerprint, [EnumeratorCancellation] CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + { + await Task.CompletedTask; + yield break; + } + + public Task GetContentHashListAsync(Context context, StrongFingerprint strongFingerprint, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + => Task.FromResult(new GetContentHashListResult(default(ContentHashListWithDeterminism))); + + public Task AddOrGetContentHashListAsync(Context context, StrongFingerprint strongFingerprint, ContentHashListWithDeterminism contentHashListWithDeterminism, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + { + AddOrGetContentHashListCallCount++; + return Task.FromResult(new AddOrGetContentHashListResult(default(ContentHashListWithDeterminism))); + } + + public Task IncorporateStrongFingerprintsAsync(Context context, IEnumerable> strongFingerprints, CancellationToken cts, UrgencyHint urgencyHint = UrgencyHint.Nominal) + => Task.FromResult(BoolResult.Success); +} From a777c1bef46b33d809d3924019120ca4ffb40870 Mon Sep 17 00:00:00 2001 From: Pavlo Tubolets Date: Thu, 27 Aug 2026 09:41:45 +1000 Subject: [PATCH 3/3] use HashInfoLookup --- src/Common.Tests/CasCacheClientTests.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Common.Tests/CasCacheClientTests.cs b/src/Common.Tests/CasCacheClientTests.cs index eeda5ee..cf2cd0d 100644 --- a/src/Common.Tests/CasCacheClientTests.cs +++ b/src/Common.Tests/CasCacheClientTests.cs @@ -78,7 +78,10 @@ public async Task AddNodeUploadsContentWhenRemoteCacheIsWritable() private static (CasCacheClient CacheClient, RecordingCacheSession LocalSession, RecordingCacheSession RemoteSession) CreateCacheClient(bool remoteCacheIsReadOnly) { - IContentHasher hasher = HashInfoLookup.GetContentHasher(HashType.Vso0); +#pragma warning disable CA2000 // Ownership of the hasher, caches, sessions and logger is transferred to the cache client, which disposes them. + // Create a hasher rather than using HashInfoLookup.GetContentHasher, which returns a process-wide + // instance that must not be disposed. CacheClient disposes the hasher it is given. + IContentHasher hasher = HashInfoLookup.Find(HashType.Vso0).CreateContentHasher(); // The local session reports content as present so nothing needs to be ingested from disk. RecordingCacheSession localSession = new("local", pinSucceeds: true); @@ -86,7 +89,6 @@ private static (CasCacheClient CacheClient, RecordingCacheSession LocalSession, // The remote session reports content as missing, so a writable remote would upload it. RecordingCacheSession remoteSession = new("remote", pinSucceeds: false); -#pragma warning disable CA2000 // Ownership of the caches, sessions and logger is transferred to the cache client, which disposes them. RecordingCache localCache = new(localSession); RecordingCache remoteCache = new(remoteSession);