Fix Copilot's PR feedback

Author: paulirwin

src/Lucene.Net.Tests/Store/TestMultiMMap.cs

@@ -547,7 +547,10 @@ public void TestOpenInputConcurrentFileExtension_Issue1090()
         // promptly after the root is disposed. A successful pass here is
         // therefore a *positive* result — not Inconclusive — because the
         // expected behavior is that the invariant holds throughout.
-        [Test, LuceneNetSpecific, Slow, NonParallelizable]
+        // [Nightly]: wall-clock stress loop (up to ~30s). Kept out of the
+        // default run so CI isn't lengthened, but exercised in nightly runs
+        // where catching regressions in the #1013 race path is worth the time.
+        [Test, LuceneNetSpecific, Slow, Nightly, NonParallelizable]
         public void TestConcurrentCloneReadVsDispose_Issue1013()
         {
             var dirPath = CreateTempDir("testIssue1013");
@@ -688,7 +691,8 @@ public void TestConcurrentCloneReadVsDispose_Issue1013()
         //   - After join, calling Clone() + read on the disposed master
         //     from the main thread throws AlreadyClosed — pinning that a
         //     disposed root cannot silently hand out a working clone.
-        [Test, LuceneNetSpecific, Slow, NonParallelizable]
+        // [Nightly]: wall-clock stress loop (~15s). See Issue1013 rationale.
+        [Test, LuceneNetSpecific, Slow, Nightly, NonParallelizable]
         public void TestConcurrentCloneVsDispose_RaceScenario()
         {
             var dirPath = CreateTempDir("testCloneVsDispose");
@@ -780,7 +784,8 @@ public void TestConcurrentCloneVsDispose_RaceScenario()
         // Concurrent read of the SAME instance during Dispose of that
         // instance. Drain-barrier must prevent the disposer from releasing
         // the pointer while a reader is mid-CopyBlockUnaligned.
-        [Test, LuceneNetSpecific, Slow, NonParallelizable]
+        // [Nightly]: wall-clock stress loop (~15s). See Issue1013 rationale.
+        [Test, LuceneNetSpecific, Slow, Nightly, NonParallelizable]
         public void TestConcurrentReadVsSelfDispose_RaceScenario()
         {
             var dirPath = CreateTempDir("testReadVsSelfDispose");
@@ -850,7 +855,8 @@ public void TestConcurrentReadVsSelfDispose_RaceScenario()
         // slicer cascades Dispose to all issued slices; every in-flight
         // reader must either finish its current CopyBlockUnaligned or
         // observe the closed state cleanly (no AVE).
-        [Test, LuceneNetSpecific, Slow, NonParallelizable]
+        // [Nightly]: wall-clock stress loop (~15s). See Issue1013 rationale.
+        [Test, LuceneNetSpecific, Slow, Nightly, NonParallelizable]
         public void TestConcurrentSliceReadVsSlicerDispose_RaceScenario()
         {
             var dirPath = CreateTempDir("testSliceVsSlicerDispose");
@@ -1211,6 +1217,78 @@ public void TestDisposingOneSliceDoesNotAffectSiblings()
             sliceB.Dispose();
         }
 
+        // Concurrent-clone read correctness. N workers each take a Clone()
+        // of the same root input, seek independently, and read the full
+        // file; every worker must observe the exact bytes that were
+        // written. Existing concurrent tests only check that reads don't
+        // throw — this test pins that readers also don't observe torn or
+        // stale bytes under contention over the shared View.basePtr.
+        [Test, LuceneNetSpecific, Slow]
+        public void TestConcurrentClonesReadIdenticalBytes()
+        {
+            var dirPath = CreateTempDir("testConcurrentClonesIntegrity");
+            using var mmapDir = new MMapDirectory(dirPath);
+            const string name = "bytes";
+            // 2 MiB — large enough that readers overlap in time, small
+            // enough that the test finishes in well under a second per
+            // iteration.
+            const int fileSize = 2 * 1024 * 1024;
+            var expected = new byte[fileSize];
+            new Random(0xC0FFEE).NextBytes(expected);
+
+            using (var io = mmapDir.CreateOutput(name, NewIOContext(Random)))
+            {
+                io.WriteBytes(expected, 0, expected.Length);
+            }
+
+            using var root = mmapDir.OpenInput(name, NewIOContext(Random));
+
+            const int numWorkers = 8;
+            const int passesPerWorker = 20;
+            var errors = new ConcurrentBag<string>();
+            var clones = new IndexInput[numWorkers];
+            for (int i = 0; i < numWorkers; i++)
+            {
+                clones[i] = (IndexInput)root.Clone();
+            }
+
+            var start = new ManualResetEventSlim(false);
+            var threads = new Thread[numWorkers];
+            for (int i = 0; i < numWorkers; i++)
+            {
+                int idx = i;
+                threads[i] = new Thread(() =>
+                {
+                    var buf = new byte[fileSize];
+                    start.Wait();
+                    for (int pass = 0; pass < passesPerWorker && errors.IsEmpty; pass++)
+                    {
+                        clones[idx].Seek(0);
+                        clones[idx].ReadBytes(buf, 0, buf.Length);
+                        // Byte-equal check: any deviation means the read
+                        // path observed torn or stale data.
+                        for (int j = 0; j < buf.Length; j++)
+                        {
+                            if (buf[j] != expected[j])
+                            {
+                                errors.Add(
+                                    $"worker {idx} pass {pass}: byte at {j} expected 0x{expected[j]:X2}, got 0x{buf[j]:X2}");
+                                return;
+                            }
+                        }
+                    }
+                }) { IsBackground = true, Name = "concurrent-integrity-" + i };
+                threads[i].Start();
+            }
+            start.Set();
+            foreach (var t in threads) t.Join(TimeSpan.FromSeconds(30));
+
+            if (!errors.IsEmpty)
+            {
+                Assert.Fail("Concurrent-clone read corruption detected:\n" + string.Join("\n", errors));
+            }
+        }
+
         // Open a 3.0 CFS index with MMapDirectory and read it end-to-end
         // through DirectoryReader. 3.0 (pre-3.1) CFS files are the only
         // remaining caller of IndexInputSlicer.OpenFullSlice, which in our

src/Lucene.Net/Store/MMapDirectory.cs

@@ -103,6 +103,13 @@ public MMapDirectory(DirectoryInfo path)
         /// <param name="maxChunkSize"> maximum chunk size (default is 1 GiBytes for
         /// 64 bit runtimes and 256 MiBytes for 32 bit runtimes) used for memory mapping.
         /// <para/>
+        /// <b>LUCENENET note:</b> this parameter is retained for API compatibility
+        /// but is effectively a no-op in the current implementation. Each file is
+        /// mapped as a single <see cref="System.IO.MemoryMappedFiles.MemoryMappedViewAccessor"/>
+        /// regardless of <paramref name="maxChunkSize"/>; see the class remarks for
+        /// the rationale (issues #1013 and #1151). The value is still validated and
+        /// retained so that <see cref="MaxChunkSize"/> reflects what the caller passed.
+        /// <para/>
         /// Especially on 32 bit platform, the address space can be very fragmented,
         /// so large index files cannot be mapped. Using a lower chunk size makes
         /// the directory implementation a little bit slower (as the correct chunk
@@ -162,6 +169,13 @@ public MMapDirectory(string path)
         /// <param name="maxChunkSize"> maximum chunk size (default is 1 GiBytes for
         /// 64 bit runtimes and 256 MiBytes for 32 bit runtimes) used for memory mapping.
         /// <para/>
+        /// <b>LUCENENET note:</b> this parameter is retained for API compatibility
+        /// but is effectively a no-op in the current implementation. Each file is
+        /// mapped as a single <see cref="System.IO.MemoryMappedFiles.MemoryMappedViewAccessor"/>
+        /// regardless of <paramref name="maxChunkSize"/>; see the class remarks for
+        /// the rationale (issues #1013 and #1151). The value is still validated and
+        /// retained so that <see cref="MaxChunkSize"/> reflects what the caller passed.
+        /// <para/>
         /// Especially on 32 bit platform, the address space can be very fragmented,
         /// so large index files cannot be mapped. Using a lower chunk size makes
         /// the directory implementation a little bit slower (as the correct chunk
@@ -182,7 +196,15 @@ public MMapDirectory(string path, LockFactory lockFactory, int maxChunkSize)
         // indeed "release all resources". Therefore unmap hack is not needed in .NET.
 
         /// <summary>
-        /// Returns the current mmap chunk size. </summary>
+        /// Returns the current mmap chunk size.
+        /// <para/>
+        /// <b>LUCENENET note:</b> this value is retained for API compatibility
+        /// but does not affect runtime behavior in the current implementation.
+        /// Each file is mapped as a single
+        /// <see cref="System.IO.MemoryMappedFiles.MemoryMappedViewAccessor"/>
+        /// regardless of this setting; see the class remarks (issues #1013 and
+        /// #1151) for the rationale.
+        /// </summary>
         /// <seealso cref="MMapDirectory(DirectoryInfo, LockFactory, int)"/>
         public int MaxChunkSize => 1 << chunkSizePower;
 
@@ -204,7 +226,10 @@ public override IndexInputSlicer CreateSlicer(string name, IOContext context)
             EnsureOpen();
             var file = Path.Combine(Directory.FullName, name);
             var descriptor = new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
-            return new IndexInputSlicerAnonymousClass(this, context, file, descriptor);
+            // Cache the length eagerly so OpenFullSlice never has to touch
+            // the FileStream — which would race with Dispose closing it.
+            long descriptorLength = descriptor.Length;
+            return new IndexInputSlicerAnonymousClass(this, context, file, descriptor, descriptorLength);
         }
 
         private sealed class IndexInputSlicerAnonymousClass : IndexInputSlicer
@@ -213,6 +238,7 @@ private sealed class IndexInputSlicerAnonymousClass : IndexInputSlicer
             private readonly IOContext context;
             private readonly string file;
             private readonly FileStream descriptor;
+            private readonly long descriptorLength;
             private int disposed = 0; // LUCENENET specific - allow double-dispose
             // Track issued slices so that Dispose cascades. Lucene's
             // contract is that after slicer.Dispose, reads from any slice
@@ -221,12 +247,13 @@ private readonly SCG.List<MemoryMappedViewAccessorIndexInput> issuedSlices
                 = new SCG.List<MemoryMappedViewAccessorIndexInput>();
             private readonly object issuedSlicesLock = new object();
 
-            public IndexInputSlicerAnonymousClass(MMapDirectory outerInstance, IOContext context, string file, FileStream descriptor)
+            public IndexInputSlicerAnonymousClass(MMapDirectory outerInstance, IOContext context, string file, FileStream descriptor, long descriptorLength)
             {
                 this.outerInstance = outerInstance;
                 this.context = context;
                 this.file = file;
                 this.descriptor = descriptor;
+                this.descriptorLength = descriptorLength;
             }
 
             public override IndexInput OpenSlice(string sliceDescription, long offset, long length)
@@ -257,16 +284,11 @@ public override IndexInput OpenSlice(string sliceDescription, long offset, long
             public override IndexInput OpenFullSlice()
             {
                 outerInstance.EnsureOpen();
-                // Check disposed before touching descriptor. Without this,
-                // a concurrent Dispose could close the FileStream between
-                // EnsureOpen and the descriptor.Length read, and the user
-                // would see an ObjectDisposedException from FileStream
-                // instead of the conventional AlreadyClosedException.
-                if (Volatile.Read(ref disposed) != 0)
-                {
-                    throw AlreadyClosedException.Create(nameof(IndexInputSlicer), "this IndexInputSlicer is closed");
-                }
-                return OpenSlice("full-slice", 0, descriptor.Length);
+                // File length is captured eagerly in the slicer ctor, so we
+                // never touch the descriptor FileStream here. OpenSlice does
+                // its own disposed-check under the lock, which will throw
+                // AlreadyClosed deterministically if we raced with Dispose.
+                return OpenSlice("full-slice", 0, descriptorLength);
             }
 
             protected override void Dispose(bool disposing)

src/Lucene.Net/Store/MemoryMappedViewAccessorIndexInput.cs

@@ -110,49 +110,76 @@ public View(FileStream fc, long offset, long length)
                 // another process/thread is appending to this file, the file
                 // can grow between when we capture fc.Length and when
                 // CreateFromFile reads the size.
-                long capacity = Math.Max(offset + length, fc.Length);
-                const int maxAttempts = 5;
-                int attempt = 0;
-                while (true)
+                MemoryMappedFile localMmf = null;
+                MemoryMappedViewAccessor localAccessor = null;
+                byte* ptr = null;
+                bool pointerAcquired = false;
+                try
                 {
-                    try
+                    long capacity = Math.Max(offset + length, fc.Length);
+                    const int maxAttempts = 5;
+                    int attempt = 0;
+                    while (true)
                     {
-                        this.memoryMappedFile = MemoryMappedFile.CreateFromFile(
-                            fileStream: fc,
-                            mapName: null,
-                            capacity: capacity,
-                            access: MemoryMappedFileAccess.Read,
+                        try
+                        {
+                            localMmf = MemoryMappedFile.CreateFromFile(
+                                fileStream: fc,
+                                mapName: null,
+                                capacity: capacity,
+                                access: MemoryMappedFileAccess.Read,
 #if FEATURE_MEMORYMAPPEDFILESECURITY
-                            memoryMappedFileSecurity: null,
+                                memoryMappedFileSecurity: null,
 #endif
-                            inheritability: HandleInheritability.Inheritable,
-                            leaveOpen: true); // We dispose the FileStream explicitly.
-                        break;
+                                inheritability: HandleInheritability.Inheritable,
+                                leaveOpen: true); // We dispose the FileStream explicitly.
+                            break;
+                        }
+                        catch (ArgumentOutOfRangeException e) when (e.ParamName == "capacity" && attempt < maxAttempts - 1)
+                        {
+                            Interlocked.Increment(ref MMapDirectory.s_capacityRetryCount);
+                            capacity = Math.Max(capacity, fc.Length);
+                            attempt++;
+                        }
                     }
-                    catch (ArgumentOutOfRangeException e) when (e.ParamName == "capacity" && attempt < maxAttempts - 1)
+                    int attemptsTaken = attempt + 1;
+                    int prior;
+                    do
                     {
-                        Interlocked.Increment(ref MMapDirectory.s_capacityRetryCount);
-                        capacity = Math.Max(capacity, fc.Length);
-                        attempt++;
-                    }
-                }
-                int attemptsTaken = attempt + 1;
-                int prior;
-                do
-                {
-                    prior = Volatile.Read(ref MMapDirectory.s_maxCapacityAttemptsObserved);
-                    if (attemptsTaken <= prior) break;
-                } while (Interlocked.CompareExchange(ref MMapDirectory.s_maxCapacityAttemptsObserved, attemptsTaken, prior) != prior);
-                // LUCENENET specific END
+                        prior = Volatile.Read(ref MMapDirectory.s_maxCapacityAttemptsObserved);
+                        if (attemptsTaken <= prior) break;
+                    } while (Interlocked.CompareExchange(ref MMapDirectory.s_maxCapacityAttemptsObserved, attemptsTaken, prior) != prior);
+                    // LUCENENET specific END
 
-                this.accessor = memoryMappedFile.CreateViewAccessor(offset, length, MemoryMappedFileAccess.Read);
+                    localAccessor = localMmf.CreateViewAccessor(offset, length, MemoryMappedFileAccess.Read);
 
-                byte* ptr = null;
-                accessor.SafeMemoryMappedViewHandle.AcquirePointer(ref ptr);
-                // The accessor may be mapped at an offset inside the OS page,
-                // in which case PointerOffset is the distance from the
-                // SafeBuffer's base to the first byte of the requested view.
-                this.basePtr = ptr + accessor.PointerOffset;
+                    localAccessor.SafeMemoryMappedViewHandle.AcquirePointer(ref ptr);
+                    pointerAcquired = true;
+
+                    this.memoryMappedFile = localMmf;
+                    this.accessor = localAccessor;
+                    // The accessor may be mapped at an offset inside the OS page,
+                    // in which case PointerOffset is the distance from the
+                    // SafeBuffer's base to the first byte of the requested view.
+                    this.basePtr = ptr + localAccessor.PointerOffset;
+                }
+                catch
+                {
+                    // The View ctor owns fc (the caller passes ownership). If
+                    // any step above fails we must release everything we
+                    // partially acquired — including fc — before rethrowing,
+                    // or we leak the FileStream/MMF/view handle.
+                    if (pointerAcquired && localAccessor != null)
+                    {
+                        try { localAccessor.SafeMemoryMappedViewHandle.ReleasePointer(); }
+                        catch { /* never propagate from cleanup */ }
+                    }
+                    localAccessor?.Dispose();
+                    localMmf?.Dispose();
+                    try { fc.Dispose(); } catch { /* never propagate from cleanup */ }
+                    this.fc = null;
+                    throw;
+                }
             }
 
             /// <summary>