Apply code review suggestions

documentation/wiki/Binary-Log.md

@@ -117,15 +117,6 @@ logReader.AnyEventRaised += (_, e) =>
     // ...
 };
 
-// Those can be raised only during forward compatibility reading mode.
-logReader.OnRecoverableReadError += (errorType, recordKind, message) =>
-{
-    // ...
-
-    // e.g. we can decide to ignore the error and continue reading or break reading
-    //  based on the type of the error or/and type of the record
-};
-
 // Starts the synchronous log reading loop.
 logReader.Replay(path_to_binlog_file);
 
@@ -140,9 +131,40 @@ The unknown events and event parts are regarded as recoverable errors, since the
 To allow the calling code to decide - based on the type of error, type of events getting the error, or the number of errors - the `OnRecoverableReadError` event is exposed (from both `BinaryLogReplayEventSource` and `BuildEventArgsReader`).
 
 ```csharp
+/// <summary>
+/// Materializes the error message.
+/// Until it's called the error message is not materialized and no string allocations are made.
+/// </summary>
+/// <returns>The error message.</returns>
+public delegate string FormatErrorMessage();
+
 /// <summary>
 /// Receives recoverable errors during reading.
 /// Communicates type of the error, kind of the record that encountered the error and the message detailing the error.
+/// The error message is returned as a function to avoid unnecessary string allocations in case the error is not logged.
 /// </summary>
-event Action<ReaderErrorType, BinaryLogRecordKind, Func<string>>? OnRecoverableReadError;
+event Action<ReaderErrorType, BinaryLogRecordKind, FormatErrorMessage>? OnRecoverableReadError;
 ```
+
+Our sample usage of the [Reading API](#reading-api) can be enhanced with recoverable errors handling e.g. as such:
+
+```csharp
+
+// Those can be raised only during forward compatibility reading mode.
+logReader.OnRecoverableReadError += (errorType, recordKind, messageFactory) =>
+{
+    // ...
+
+    // e.g. we can decide to ignore the error and continue reading or break reading
+    //  based on the type of the error or/and type of the record or/and the frequency of the error
+
+    // Would we decide to completely ignore some errors - we can aid better performance by not materializing the actual error message.
+    // Otherwise the error message can be materialized via the provided delegate:
+    Console.WriteLine($"Recoverable reader error: {messageFactory()}");
+};
+
+```
+
+When authoring changes to the specific BuildEventArg types - it is always strongly recommended to **prefer append-only changes**. 
+
+This prevents the possibility of collision where some fields are removed in one version and then different fields with same binary size are added in future version. Such a sequence of format changes might not be caught by the decoder and might lead to unnoticed corrupt interpretation of data. For this reason the author of specific OM changes should always check whether there is a possibility of unrecognizable format collision (same binary size, different representation) within binlog versions of a same [minimum reader version support](#forward-compatibility-reading). If this is possible, the [minimum reader version support](#forward-compatibility-reading) should be incremented.

src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs

@@ -849,15 +849,7 @@ public void ReadingCorruptedStreamThrows()
                 var binaryReader = new BinaryReader(memoryStream);
                 using var buildEventArgsReader = new BuildEventArgsReader(binaryReader, BinaryLogger.FileFormatVersion);
 
-                try
-                {
-                    buildEventArgsReader.Read();
-                }
-                catch (Exception e)
-                {
-                    // if the EndOfStreamException is received during EventArgs parsing - the parsing code will translate it to InvalidDataException
-                    Assert.True(e is InvalidDataException or EndOfStreamException, "Abruptly ended stream should lead to InvalidDataException or EndOfStreamException");
-                }
+                Assert.Throws<EndOfStreamException>(() => buildEventArgsReader.Read());
             }
         }
 
@@ -1079,9 +1071,7 @@ public void ForwardCompatibleRead_HandleRemovalOfDataFromEventDefinition()
             readerErrors.Count.ShouldBe(1);
             readerErrors[0].errorType.ShouldBe(ReaderErrorType.UnknownFormatOfEventData);
             readerErrors[0].recordKind.ShouldBe(BinaryLogRecordKind.Error);
-            // Expect StreamChunkOverReadException or EndOfStreamException - based on the implementation
-            //  of indicating attempt to overread in the TransparentReadStream.
-            readerErrors[0].error.ShouldContain("StreamChunkOverReadException");
+            readerErrors[0].error.ShouldContain("EndOfStreamException");
 
             deserializedEvent.Should().BeEquivalentTo(finished);
 

src/Build/Logging/BinaryLogger/BinaryLogReplayEventSource.cs

@@ -40,6 +40,7 @@ internal interface IBinaryLogReplaySource :
         IRawLogEventsSource,
         IBuildEventStringsReader,
         IEmbeddedContentSource,
+        IBuildFileReader,
         IBinlogReaderErrors
     { }
 
@@ -62,10 +63,10 @@ static BinaryLogReplayEventSource()
         /// <summary>
         /// Unknown build events or unknown parts of known build events will be ignored if this is set to true.
         /// </summary>
-        public bool AllowForwardCompatibility { private get; init; } = true;
+        public bool AllowForwardCompatibility { private get; init; }
 
         /// <inheritdoc cref="IBinlogReaderErrors.OnRecoverableReadError"/>
-        public event Action<ReaderErrorType, BinaryLogRecordKind, Func<string>>? OnRecoverableReadError;
+        public event Action<ReaderErrorType, BinaryLogRecordKind, FormatErrorMessage>? OnRecoverableReadError;
 
         /// <summary>
         /// WARNING: This event is under low support and low maintenance - please use events directly exposed by <see cref="BinaryLogReplayEventSource"/> instead. 
@@ -204,6 +205,7 @@ public void Replay(BuildEventArgsReader reader, CancellationToken cancellationTo
             }
 
             reader.EmbeddedContentRead += _embeddedContentRead;
+            reader.ArchiveFileEncountered += _archiveFileEncountered;
             reader.StringReadDone += _stringReadDone;
             reader.StringEncountered += _stringEncountered;
 
@@ -230,6 +232,7 @@ public void Replay(BuildEventArgsReader reader, CancellationToken cancellationTo
                 if (this._rawLogRecordReceived == null &&
                     this._embeddedContentRead == null &&
                     this._stringReadDone == null &&
+                    this._archiveFileEncountered == null &&
                     this._stringEncountered == null)
                 {
                     throw new NotSupportedException(
@@ -273,6 +276,14 @@ public void Replay(BuildEventArgsReader reader, CancellationToken cancellationTo
             remove => _stringReadDone -= value;
         }
 
+        private Action<ArchiveFileEventArgs>? _archiveFileEncountered;
+        /// <inheritdoc cref="IBuildFileReader.ArchiveFileEncountered"/>
+        event Action<ArchiveFileEventArgs>? IBuildFileReader.ArchiveFileEncountered
+        {
+            add => _archiveFileEncountered += value;
+            remove => _archiveFileEncountered -= value;
+        }
+
         private Action? _stringEncountered;
         /// <inheritdoc cref="IBuildEventStringsReader.StringEncountered"/>
         event Action? IBuildEventStringsReader.StringEncountered

src/Build/Logging/BinaryLogger/BinaryLogger.cs

@@ -204,7 +204,7 @@ public void Initialize(IEventSource eventSource)
                 if (CollectProjectImports == ProjectImportsCollectionMode.Embed)
                 {
                     replayEventsSource.EmbeddedContentRead += args =>
-                        eventArgsWriter.WriteBlob(args.ContentKind.ToBinaryLogRecordKind(), args.ContentStream);
+                        eventArgsWriter.WriteBlob(args.ContentKind, args.ContentStream);
                 }
                 else if (CollectProjectImports == ProjectImportsCollectionMode.ZipFile)
                 {

src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs

@@ -128,7 +128,7 @@ private void EnsureForwardCompatibleReadingSupported()
         /// Receives recoverable errors during reading. See <see cref="IBinlogReaderErrors.OnRecoverableReadError"/> for documentation on arguments.
         /// Applicable mainly when <see cref="SkipUnknownEvents"/> or <see cref="SkipUnknownEventParts"/> is set to true."/>
         /// </summary>
-        public event Action<ReaderErrorType, BinaryLogRecordKind, Func<string>>? OnRecoverableReadError;
+        public event Action<ReaderErrorType, BinaryLogRecordKind, FormatErrorMessage>? OnRecoverableReadError;
 
         public void Dispose()
         {
@@ -228,9 +228,8 @@ internal RawRecord ReadRaw()
                     e is InvalidDataException ||
                     // Thrown when BinaryReader is unable to deserialize binary data into expected type.
                     e is FormatException ||
-                    // Following 2 are thrown when we attempt to read more bytes than what is in the next event chunk.
-                    e is StreamChunkOverReadException ||
-                    e is EndOfStreamException)
+                    // Thrown when we attempt to read more bytes than what is in the next event chunk.
+                    (e is EndOfStreamException && _readStream.BytesCountAllowedToReadRemaining <= 0))
                 {
                     hasError = true;
 
@@ -268,7 +267,7 @@ internal RawRecord ReadRaw()
 
             return result;
 
-            void HandleError(Func<string> msgFactory, bool noThrow, ReaderErrorType readerErrorType, BinaryLogRecordKind recordKind, Exception? innerException = null)
+            void HandleError(FormatErrorMessage msgFactory, bool noThrow, ReaderErrorType readerErrorType, BinaryLogRecordKind recordKind, Exception? innerException = null)
             {
                 if (noThrow)
                 {
@@ -421,15 +420,15 @@ private void ReadEmbeddedContent(BinaryLogRecordKind recordKind)
                 if (EmbeddedContentRead != null)
                 {
                     projectImportsCollector!.ProcessResult(
-                        streamToEmbed => EmbeddedContentRead(new EmbeddedContentEventArgs(EmbeddedContentKind.ProjectImportArchive, streamToEmbed)),
+                        streamToEmbed => EmbeddedContentRead(new EmbeddedContentEventArgs(recordKind, streamToEmbed)),
                         error => throw new InvalidDataException(error));
                     projectImportsCollector.DeleteArchive();
                 }
             }
             else if (EmbeddedContentRead != null)
             {
                 EmbeddedContentRead(new EmbeddedContentEventArgs(
-                    recordKind.ToEmbeddedContentKind(),
+                    recordKind,
                     _binaryReader.BaseStream.Slice(length)));
             }
             else

src/Build/Logging/BinaryLogger/Postprocessing/EmbeddedContentEventArgs.cs

@@ -8,13 +8,13 @@ namespace Microsoft.Build.Logging
 {
     internal sealed class EmbeddedContentEventArgs : EventArgs
     {
-        public EmbeddedContentEventArgs(EmbeddedContentKind contentKind, Stream contentStream)
+        public EmbeddedContentEventArgs(BinaryLogRecordKind contentKind, Stream contentStream)
         {
             ContentKind = contentKind;
             ContentStream = contentStream;
         }
 
-        public EmbeddedContentKind ContentKind { get; }
+        public BinaryLogRecordKind ContentKind { get; }
         public Stream ContentStream { get; }
     }
 }

src/Build/Logging/BinaryLogger/Postprocessing/IBinlogReaderErrors.cs

@@ -10,14 +10,6 @@ namespace Microsoft.Build.Logging
     /// </summary>
     public enum ReaderErrorType
     {
-        /// <summary>
-        /// The file format of the binlog is not supported by the current reader.
-        /// Despite the logs should be supported by older readers - there might be certain format updates that prevent
-        ///  such forward compatibility. The binlog file contains the info about the minimum required reader version
-        ///  to detect this case.
-        /// </summary>
-        UnsupportedFileFormat,
-
         /// <summary>
         /// The encountered event is completely unknown to the reader. It cannot interpret neither a part of it.
         /// </summary>
@@ -38,13 +30,20 @@ public enum ReaderErrorType
         UnknownFormatOfEventData,
     }
 
+    /// <summary>
+    /// Materializes the error message.
+    /// Until it's called the error message is not materialized and no string allocations are made.
+    /// </summary>
+    /// <returns>The error message.</returns>
+    public delegate string FormatErrorMessage();
+
     public interface IBinlogReaderErrors
     {
         /// <summary>
         /// Receives recoverable errors during reading.
         /// Communicates type of the error, kind of the record that encountered the error and the message detailing the error.
         /// The error message is returned as a function to avoid unnecessary string allocations in case the error is not logged.
         /// </summary>
-        event Action<ReaderErrorType, BinaryLogRecordKind, Func<string>>? OnRecoverableReadError;
+        event Action<ReaderErrorType, BinaryLogRecordKind, FormatErrorMessage>? OnRecoverableReadError;
     }
 }

src/Build/Logging/BinaryLogger/Postprocessing/TransparentReadStream.cs

@@ -79,9 +79,7 @@ public override int Read(byte[] buffer, int offset, int count)
         {
             if (_position + count > _maxAllowedPosition)
             {
-                throw new StreamChunkOverReadException(
-                    ResourceUtilities.FormatResourceStringStripCodeAndKeyword("Binlog_StreamUtils_OverRead", count,
-                        _maxAllowedPosition - _position));
+                count = (int)(_maxAllowedPosition - _position);
             }
 
             int cnt = _stream.Read(buffer, offset, count);

src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs

@@ -69,7 +69,7 @@ private static string GetArchiveFilePath(string logFilePath, string sourcesArchi
                 // Archive file will be temporarily stored in MSBuild cache folder and deleted when no longer needed
                 _archiveFilePath = Path.Combine(
                     cacheDirectory,
-                    Path.ChangeExtension(
+                    GetArchiveFilePath(
                         Path.GetFileName(logFilePath),
                         sourcesArchiveExtension));
             }

src/Build/Resources/Strings.resx

@@ -2026,7 +2026,7 @@ Utilization:          {0} Average Utilization: {1:###.0}</value>
     <value>Skipping the record.</value>
   </data>
   <data name="Binlog_ReaderMismatchedRead" xml:space="preserve">
-    <value>BuildEvent record number {0} (serialized size: {1}) attempted to perform disallowed reads (details: {2}:{3}).</value>
+    <value>BuildEvent record number {0} (serialized size: {1}) attempted to perform disallowed reads (details: {2}: {3}).</value>
     <comment>
       LOCALIZATION: {0} is an integer number denoting order. {1} is an integer denoting size. {2} is an exception type and {3} the exception message string.
     </comment>
@@ -2055,12 +2055,6 @@ Utilization:          {0} Average Utilization: {1:###.0}</value>
   <data name="Binlog_StreamUtils_MustBeReadable" xml:space="preserve">
     <value>Stream must be readable.</value>
   </data>
-  <data name="Binlog_StreamUtils_OverRead" xml:space="preserve">
-    <value>Attempt to read {0} bytes, when only {1} are allowed to be read.</value>
-    <comment>
-      LOCALIZATION: {0} and {1} are integer numbers denoting number of bytes.
-    </comment>
-  </data>
   <data name="Binlog_StreamUtils_SeekNonOrigin" xml:space="preserve">
     <value>Only seeking from SeekOrigin.Current is supported.</value>
   </data>