Address Copilot feedback

Author: JimBobSquarePants

src/ImageSharp/Memory/AllocationTrackedMemoryManager{T}.cs

@@ -35,8 +35,14 @@ public abstract class AllocationTrackedMemoryManager<T> : MemoryManager<T>
     /// <inheritdoc />
     protected sealed override void Dispose(bool disposing)
     {
-        this.DisposeCore(disposing);
-        this.ReleaseAllocationTracking();
+        try
+        {
+            this.DisposeCore(disposing);
+        }
+        finally
+        {
+            this.ReleaseAllocationTracking();
+        }
     }
 
     /// <summary>

src/ImageSharp/Memory/Allocators/MemoryAllocator.cs

@@ -102,35 +102,23 @@ private protected void ApplyOptions(MemoryAllocatorOptions options)
     /// <param name="length">Size of the buffer to allocate.</param>
     /// <param name="options">The allocation options.</param>
     /// <returns>A buffer of values of type <typeparamref name="T"/>.</returns>
-    /// <exception cref="ArgumentOutOfRangeException">When length is negative.</exception>
-    /// <exception cref="InvalidMemoryOperationException">When length is over the capacity of the allocator.</exception>
+    /// <exception cref="InvalidMemoryOperationException">When length is negative or over the capacity of the allocator.</exception>
     public IMemoryOwner<T> Allocate<T>(int length, AllocationOptions options = AllocationOptions.None)
         where T : struct
     {
-        if (length < 0)
-        {
-            InvalidMemoryOperationException.ThrowNegativeAllocationException(length);
-        }
-
-        ulong lengthInBytes = (ulong)length * (ulong)Unsafe.SizeOf<T>();
-        if (lengthInBytes > (ulong)this.SingleBufferAllocationLimitBytes)
-        {
-            InvalidMemoryOperationException.ThrowAllocationOverLimitException(lengthInBytes, this.SingleBufferAllocationLimitBytes);
-        }
-
-        long lengthInBytesLong = (long)lengthInBytes;
-        bool shouldTrack = lengthInBytesLong != 0;
+        long lengthInBytes = this.GetValidatedAllocationLengthInBytes<T>(length);
+        bool shouldTrack = this.AccumulativeAllocationLimitBytes != long.MaxValue && lengthInBytes != 0;
         if (shouldTrack)
         {
-            this.ReserveAllocation(lengthInBytesLong);
+            this.ReserveAllocation(lengthInBytes);
         }
 
         try
         {
             AllocationTrackedMemoryManager<T> owner = this.AllocateCore<T>(length, options);
             if (shouldTrack)
             {
-                owner.AttachAllocationTracking(this, lengthInBytesLong);
+                owner.AttachAllocationTracking(this, lengthInBytes);
             }
 
             return owner;
@@ -139,7 +127,7 @@ public IMemoryOwner<T> Allocate<T>(int length, AllocationOptions options = Alloc
         {
             if (shouldTrack)
             {
-                this.ReleaseAccumulatedBytes(lengthInBytesLong);
+                this.ReleaseAccumulatedBytes(lengthInBytes);
             }
 
             throw;
@@ -199,7 +187,7 @@ internal MemoryGroup<T> AllocateGroup<T>(
         }
 
         long totalLengthInBytesLong = (long)totalLengthInBytes;
-        bool shouldTrack = totalLengthInBytesLong != 0;
+        bool shouldTrack = this.AccumulativeAllocationLimitBytes != long.MaxValue && totalLengthInBytesLong != 0;
         if (shouldTrack)
         {
             this.ReserveAllocation(totalLengthInBytesLong);
@@ -238,12 +226,38 @@ internal virtual MemoryGroup<T> AllocateGroupCore<T>(long totalLengthInElements,
     /// <param name="options">The allocation options.</param>
     /// <returns>A segment owner for the requested buffer length.</returns>
     /// <remarks>
-    /// The default implementation uses <see cref="Allocate{T}(int, AllocationOptions)"/>. Built-in allocators
-    /// can override this to supply raw segment owners when group construction must bypass nested tracking.
+    /// The default implementation validates the segment size then calls <see cref="AllocateCore{T}(int, AllocationOptions)"/>
+    /// directly so group construction can reserve and release the total allocation once.
     /// </remarks>
     internal virtual IMemoryOwner<T> AllocateGroupBuffer<T>(int length, AllocationOptions options = AllocationOptions.None)
         where T : struct
-        => this.AllocateCore<T>(length, options);
+    {
+        _ = this.GetValidatedAllocationLengthInBytes<T>(length);
+        return this.AllocateCore<T>(length, options);
+    }
+
+    /// <summary>
+    /// Returns the validated allocation length in bytes.
+    /// </summary>
+    /// <typeparam name="T">Type of the data stored in the buffer.</typeparam>
+    /// <param name="length">Size of the buffer to allocate.</param>
+    /// <returns>The allocation length in bytes.</returns>
+    private long GetValidatedAllocationLengthInBytes<T>(int length)
+        where T : struct
+    {
+        if (length < 0)
+        {
+            InvalidMemoryOperationException.ThrowNegativeAllocationException(length);
+        }
+
+        ulong lengthInBytes = (ulong)length * (ulong)Unsafe.SizeOf<T>();
+        if (lengthInBytes > (ulong)this.SingleBufferAllocationLimitBytes)
+        {
+            InvalidMemoryOperationException.ThrowAllocationOverLimitException(lengthInBytes, this.SingleBufferAllocationLimitBytes);
+        }
+
+        return (long)lengthInBytes;
+    }
 
     /// <summary>
     /// Reserves accumulative allocation bytes before creating the underlying buffer.
@@ -260,7 +274,7 @@ private void ReserveAllocation(long lengthInBytes)
         if (total > this.AccumulativeAllocationLimitBytes)
         {
             _ = Interlocked.Add(ref this.accumulativeAllocatedBytes, -lengthInBytes);
-            InvalidMemoryOperationException.ThrowAllocationOverLimitException((ulong)lengthInBytes, this.AccumulativeAllocationLimitBytes);
+            InvalidMemoryOperationException.ThrowAccumulativeAllocationOverLimitException(lengthInBytes, total, this.AccumulativeAllocationLimitBytes);
         }
     }
 

src/ImageSharp/Memory/Allocators/UnmanagedMemoryAllocator.cs

@@ -22,10 +22,6 @@ protected override AllocationTrackedMemoryManager<T> AllocateCore<T>(int length,
         where T : struct
         => AllocateBuffer<T>(length, options);
 
-    internal override IMemoryOwner<T> AllocateGroupBuffer<T>(int length, AllocationOptions options = AllocationOptions.None)
-        where T : struct
-        => AllocateBuffer<T>(length, options);
-
     // The pooled allocator uses this internal entry point when it needs a raw unmanaged owner without
     // nesting another allocator-level reservation cycle around the fallback allocation.
     internal static UnmanagedBuffer<T> AllocateBuffer<T>(int length, AllocationOptions options = AllocationOptions.None)

src/ImageSharp/Memory/InvalidMemoryOperationException.cs

@@ -39,4 +39,9 @@ internal static void ThrowInvalidAlignmentException(long alignment) =>
     [DoesNotReturn]
     internal static void ThrowAllocationOverLimitException(ulong length, long limit) =>
             throw new InvalidMemoryOperationException($"Attempted to allocate a buffer of length={length} that exceeded the limit {limit}.");
+
+    [DoesNotReturn]
+    internal static void ThrowAccumulativeAllocationOverLimitException(long requestedLength, long totalLength, long limit) =>
+            throw new InvalidMemoryOperationException(
+                $"Attempted to allocate a buffer of length={requestedLength} that would increase the cumulative allocation size to {totalLength}, exceeding the limit {limit}.");
 }

tests/ImageSharp.Tests/Image/ProcessPixelRowsTestBase.cs

@@ -313,7 +313,15 @@ public MockUnmanagedMemoryAllocator(params UnmanagedBuffer<T1>[] buffers)
 
         protected internal override int GetBufferCapacityInBytes() => int.MaxValue;
 
-        protected override AllocationTrackedMemoryManager<T> AllocateCore<T>(int length, AllocationOptions options = AllocationOptions.None) =>
-            this.buffers.Pop() as AllocationTrackedMemoryManager<T>;
+        protected override AllocationTrackedMemoryManager<T> AllocateCore<T>(int length, AllocationOptions options = AllocationOptions.None)
+        {
+            object buffer = this.buffers.Pop();
+            if (buffer is AllocationTrackedMemoryManager<T> trackedBuffer)
+            {
+                return trackedBuffer;
+            }
+
+            throw new InvalidMemoryOperationException("The requested buffer type does not match the mock allocator buffer type.");
+        }
     }
 }