Merged PR 700546: Consider rewrites for creation date check

Rewrites were excluded from the just-produced outputs we pass to the fingerprint computation under OIP. But we need to know about them because otherwise they get excluded by the creation timestamp check

Author: smera

Public/Src/Engine/Scheduler/Fingerprints/ObservedInputProcessingState.cs

@@ -26,7 +26,10 @@ internal class ObservedInputProcessingState : IDisposable
         public readonly Dictionary<AbsolutePath, (DirectoryMembershipFilter, DirectoryEnumerationMode)> EnumeratedDirectories = new Dictionary<AbsolutePath, (DirectoryMembershipFilter, DirectoryEnumerationMode)>();
         public readonly HashSet<HierarchicalNameId> AllDependencyPathIds = new HashSet<HierarchicalNameId>();
         public readonly HashSet<AbsolutePath> SearchPaths = new HashSet<AbsolutePath>();
-        public readonly HashSet<AbsolutePath> SharedOpaqueOutputs = new HashSet<AbsolutePath>();
+        /// <summary>
+        /// Shared opaque outputs paths produced by the pip to whether they are allowed undeclared rewrites
+        /// </summary>
+        public readonly Dictionary<AbsolutePath, bool> SharedOpaqueOutputs = new Dictionary<AbsolutePath, bool>();
 
         /// <summary>
         /// Gets a pooled instance instance of <see cref="ObservedInputProcessingState"/>.
@@ -48,12 +51,12 @@ private void Clear()
             AllDependencyPathIds.Clear();
             SearchPaths.Clear();
             SharedOpaqueOutputs.Clear();
-        }
+    }
 
-        /// <summary>
-        /// Returns the instance to the pool
-        /// </summary>
-        public void Dispose()
+    /// <summary>
+    /// Returns the instance to the pool
+    /// </summary>
+    public void Dispose()
         {
             s_pool.PutInstance(this);
         }

Public/Src/Engine/Scheduler/Fingerprints/ObservedInputProcessor.cs

@@ -182,15 +182,16 @@ internal static async Task<ObservedInputProcessingResult> ProcessInternalAsync<T
 
                 // Compute the set of all shared dynamic outputs. This is only done if there is a chance we end up using MinimalGraphWithAlienFiles 
                 // for this pip, otherwise we keep the set empty to avoid unnecessary computations
-                HashSet<AbsolutePath> sharedOpaqueOutputs = processingState.SharedOpaqueOutputs;
+                Dictionary<AbsolutePath, bool> sharedOpaqueOutputs = processingState.SharedOpaqueOutputs;
+
                 if (unPopulatedSharedOpaqueOutputs != null && envAdapter.MayDetermineMinimalGraphWithAlienFiles(allowUndeclaredSourceReads))
                 {
-                    // We filter out artifacts that are allowed file rewrites since that information is not available
-                    // when processing a prior path set. The final result will be that allowed rewrites, even though outputs,
-                    // will be part of the directory fingerprint when using minimal with alien files. This is the desired outcome
-                    // since those files existed before the build begun.
+                    // Let's distinguish rewrites vs not. We want rewritten files to be part of the fingerprint (those files
+                    // existed before the build begun) so we could exclude them from the just-produced outputs we pass to the 
+                    // fingerprint enumeration computation, but on the other hand we need to know about them for the creation time
+                    // check
                     sharedOpaqueOutputs.AddRange(unPopulatedSharedOpaqueOutputs.Values.SelectMany(fileArtifacts => 
-                        fileArtifacts.Where(fa => !fa.IsUndeclaredFileRewrite).Select(fa => fa.Path)));
+                        fileArtifacts.Select(fa => new KeyValuePair<AbsolutePath, bool>(fa.Path, fa.IsUndeclaredFileRewrite))));
                 }
 
                 using (operationContext.StartOperation(PipExecutorCounter.ObservedInputProcessorPreProcessDuration))
@@ -1264,7 +1265,7 @@ internal interface IObservedInputProcessingEnvironment
             DirectoryMembershipFilter filter,
             bool isReadOnlyDirectory,
             DirectoryMembershipHashedEventData eventData,
-            IReadOnlyCollection<AbsolutePath> sharedOpaqueOutputs,
+            IReadOnlyDictionary<AbsolutePath, bool> sharedOpaqueOutputs,
             IReadOnlyCollection<AbsolutePath> createdDirectories,
             ConcurrentBigMap<AbsolutePath, IReadOnlyList<DirectoryMemberEntry>> alienFileEnumerationCache,
             out DirectoryEnumerationMode enumerationMode,
@@ -1938,7 +1939,7 @@ internal DirectoryEnumerationMode DetermineEnumerationModeAndRule(AbsolutePath d
             DirectoryMembershipFilter filter,
             bool isReadOnlyDirectory,
             DirectoryMembershipHashedEventData eventData,
-            IReadOnlyCollection<AbsolutePath> sharedOpaqueOutputs,
+            IReadOnlyDictionary<AbsolutePath, bool> sharedOpaqueOutputs,
             IReadOnlyCollection<AbsolutePath> createdDirectories,
             ConcurrentBigMap<AbsolutePath, IReadOnlyList<DirectoryMemberEntry>> alienFileEnumerationCache,
             out DirectoryEnumerationMode enumerationMode,
@@ -2106,7 +2107,7 @@ private Action<AbsolutePath, string> FilteredHandledEntry(AbsolutePath directory
         }
 
         private Func<EnumerationRequest, PathExistence?> TryEnumerateDirectoryWithMinimalGraphWithAlienFiles(
-            IReadOnlyCollection<AbsolutePath> sharedDynamicOutputs,
+            IReadOnlyDictionary<AbsolutePath, bool> sharedDynamicOutputs,
             IReadOnlyCollection<AbsolutePath> createdDirectories,
             ConcurrentBigMap<AbsolutePath, IReadOnlyList<DirectoryMemberEntry>> alienFileEnumerationCache)
         {
@@ -2216,7 +2217,9 @@ private Action<AbsolutePath, string> FilteredHandledEntry(AbsolutePath directory
 
                                     // Rule out all shared opaques produced by this pip. These files may not be part yet of the output file system since those get registered
                                     // in a post-execution step which is not reached yet on cache miss, so the output file system may have not captured them yet
-                                    if (sharedDynamicOutputs.Contains(realFileEntryPath))
+                                    // Only exclude the ones that are not a file rewrite, in consonance with the check we did above against the output file system
+                                    var isOutputProducedByThisPip = sharedDynamicOutputs.TryGetValue(realFileEntryPath, out var isFileRewrite);
+                                    if (isOutputProducedByThisPip && !isFileRewrite)
                                     {
                                         continue;
                                     }
@@ -2241,7 +2244,10 @@ private Action<AbsolutePath, string> FilteredHandledEntry(AbsolutePath directory
                                     // Some considerations here: the correctness of this check relies on making sure outputs get to the output
                                     // file system before they are either hardlinked from the cache or (on Windows) made shared opaque by virtue of timestamp flagging.
                                     // Otherwise, we run the risk of altering the file creation original timestamp.
-                                    if (m_env.State.FileTimestampTracker.IsFileCreationTrackingSupported && m_env.State.FileTimestampTracker.PathCreatedAfterEngineStarted(realFileEntryPath))
+                                    // We could have here an output produced by this pip that is a file rewrite (and whose creation will therefore had occurred after the engine
+                                    // started. We do want those as part of the fingerprint, so don't rule it out here.
+                                    if (m_env.State.FileTimestampTracker.IsFileCreationTrackingSupported && !isOutputProducedByThisPip && 
+                                        m_env.State.FileTimestampTracker.PathCreatedAfterEngineStarted(realFileEntryPath))
                                     {
                                         continue;
                                     }

Public/Src/Engine/UnitTests/Scheduler.IntegrationTest/MinimalGraphWithAlienFileTests.cs

@@ -261,6 +261,53 @@ public void ExistingDirectoriesArePartOfTheFingerprint(bool createDirectoryInBui
             RunScheduler(fileTimestampTracker: tracker).AssertCacheMiss(pip.Process.PipId);
         }
 
+        [Fact]
+        public void FileRewritesArePartOfTheFingerprint()
+        {
+            // Make sure the source file we create before the build runs is considered a true source file by making
+            // it older than the engine start time
+            var timestampTracker = GetFileTimestampTrackerFromTomorrow();
+
+            AbsolutePath dirPath = AbsolutePath.Create(Context.PathTable, Path.Combine(SourceRoot, "dir"));
+            DirectoryArtifact dirToEnumerate = DirectoryArtifact.CreateWithZeroPartialSealId(dirPath);
+            
+            var sourceFile = CreateSourceFile(root: dirPath);
+            string sourceFileString = sourceFile.Path.ToString(Context.PathTable);
+
+            // Create a source file under the directory we will enumerate
+            File.WriteAllText(sourceFileString, "some text");
+
+            var builder = CreatePipBuilder(new Operation[]
+            {
+                // Rewrite the source file
+                Operation.WriteFile(sourceFile, doNotInfer: true),
+                // And enumerate it
+                Operation.EnumerateDir(dirToEnumerate, doNotInfer: true),
+                // Dummy output
+                Operation.WriteFile(CreateOutputFileArtifact())
+            });
+
+            // This makes sure we use the right file system, which is aware of alien files
+            builder.Options |= global::BuildXL.Pips.Operations.Process.Options.AllowUndeclaredSourceReads;
+            // Let source rewrites happen
+            builder.RewritePolicy = RewritePolicy.SafeSourceRewritesAreAllowed;
+
+            builder.AddOutputDirectory(dirPath, SealDirectoryKind.SharedOpaque);
+
+            var pip = SchedulePipBuilder(builder);
+
+            // Run once
+            RunScheduler(fileTimestampTracker: timestampTracker).AssertSuccess();
+
+            // Make sure fingerprints are stable
+            RunScheduler(fileTimestampTracker: timestampTracker).AssertSuccess().AssertCacheHit(pip.Process.PipId);
+
+            // Delete the source file and make sure we get a cache miss since the enumeration changed
+            File.Delete(sourceFileString);
+
+            RunScheduler(fileTimestampTracker: timestampTracker).AssertCacheMiss(pip.Process.PipId);
+        }
+
         /// <summary>
         /// Requiring Windows for this test is not actually a limitation of the enumeration mode (which should work fine in non-windows OSs) but
         /// a limitation of <see cref="IScheduleConfiguration.TreatAbsentDirectoryAsExistentUnderOpaque"/>, where directory probes cannot be simulated

Public/Src/Engine/UnitTests/Scheduler/ObservedInputProcessorTests.cs

@@ -771,7 +771,7 @@ public Environment(Harness harness, LoggingContext loggingContext)
                     DirectoryMembershipFilter filter,
                     bool isReadOnlyDirectory,
                     DirectoryMembershipHashedEventData eventData,
-                    IReadOnlyCollection<AbsolutePath> sharedOpaqueOutputs,
+                    IReadOnlyDictionary<AbsolutePath, bool> sharedOpaqueOutputs,
                     IReadOnlyCollection<AbsolutePath> createdDirectories,
                     ConcurrentBigMap<AbsolutePath, IReadOnlyList<(AbsolutePath, string)>> alienFileEnumerationCache,
                     out DirectoryEnumerationMode mode,