RavenDB-22972 Fixing NREs on getting LowLevelTransaction.Id from Scan…

…Oldest() and LowLevelTransaction.TryGetClientState() called by LuceneIndexPersistence.Clean() after we started to null _envRecord on tx dispoe. The issue was that we could dispose a transaction while we iterated over active ones.

RavenDB-22995 Fixing missing disposal of IndexSearcher after recreating it to cleanup Lucene readers. Also changing the cleanup implementation so we won't create a new index searcher instance per each active transaction (it's likely that all active transaction use the same LuceneIndexState instance anyway, hence also same IndexSearcher).

src/Raven.Server/Documents/Indexes/IndexStateRecord.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Immutable;
 using System.Diagnostics;
+using System.Runtime.CompilerServices;
 using Corax;
 using Lucene.Net.Search;
 using Raven.Client.Documents.Linq;
@@ -47,6 +48,8 @@ public sealed record CollectionEtags(
 
 public class LuceneIndexState
 {
+    private static readonly ConditionalWeakTable<IndexSearcher, IndexSearcherDisposer> InstancesThatGotRecreated = new();
+
     public Lazy<IndexSearcher> IndexSearcher;
 
     public LuceneIndexState(Func<IndexSearcher> func)
@@ -80,7 +83,10 @@ public LuceneIndexState()
                     logger.Error("Failed to finalize index searcher", e);
                 }
             }
-            catch { }
+            catch
+            {
+                // ignored
+            }
             Debug.Assert(false, e.ToString());
         }
     }
@@ -89,6 +95,43 @@ public void Recreate(Func<IndexSearcher> createIndexSearcher)
     {
         if (IndexSearcher.IsValueCreated == false)
             return;
+
+        var old = IndexSearcher.Value;
+
+        InstancesThatGotRecreated.Add(old, new IndexSearcherDisposer(old)); // this way we ensure the old searcher will get disposed - https://www.ayende.com/blog/199169-A/externalfinalizer-adding-a-finalizer-to-3rd-party-objects
+
         IndexSearcher = new Lazy<IndexSearcher>(createIndexSearcher);
     }
+
+    private class IndexSearcherDisposer(IndexSearcher searcher)
+    {
+        ~IndexSearcherDisposer()
+        {
+            try
+            {
+                using (searcher)
+                using (searcher.IndexReader)
+                {
+
+                }
+            }
+            catch (Exception e)
+            {
+                try
+                {
+                    RavenLogger logger = RavenLogManager.Instance.GetLoggerForServer<IndexStateRecord>();
+                    if (logger.IsErrorEnabled)
+                    {
+                        logger.Error($"Failed to finalize index searcher from {nameof(IndexSearcherDisposer)}", e);
+                    }
+                }
+                catch
+                {
+                    // ignored
+                }
+                Debug.Assert(false, e.ToString());
+            }
+
+        }
+    }
 }

src/Raven.Server/Documents/Indexes/Persistence/Lucene/LuceneIndexPersistence.cs

@@ -205,9 +205,9 @@ public override void Clean(IndexCleanup mode)
             _converter?.Clean();
             if (mode.HasFlag(IndexCleanup.Readers))
             {
-                foreach (LowLevelTransaction llt in _index._indexStorage.Environment().ActiveTransactions.Enumerate())
+                using (var tx = _index._indexStorage.Environment().ReadTransaction())
                 {
-                    if (llt.TryGetClientState(out IndexStateRecord stateRecord))
+                    if (tx.LowLevelTransaction.TryGetClientState(out IndexStateRecord stateRecord))
                     {
                         stateRecord.LuceneIndexState.Recreate(CreateIndexSearcher);
                     }

src/Voron/Impl/LowLevelTransaction.cs

@@ -50,6 +50,7 @@ public sealed unsafe class LowLevelTransaction : IDisposable, IDisposableQueryab
         private readonly ImmutableDictionary<long,PageFromScratchBuffer> _scratchPagesForReads;
         private List<long> _sparsePageRanges;
         private readonly GetPageMethod _getPageMethod;
+        private readonly long _id;
 
         internal long DecompressedBufferBytes;
         internal TestingStuff _forTestingPurposes;
@@ -149,7 +150,7 @@ public Size AdditionalMemoryUsageSize
 
         public StorageEnvironment Environment => _env;
 
-        public long Id => _envRecord.TransactionId;
+        public long Id => _id;
 
         public bool Committed { get; private set; }
 
@@ -181,6 +182,7 @@ public LowLevelTransaction(LowLevelTransaction previous, TransactionPersistentCo
             _txHeader = TxHeaderInitializerTemplate;
             _env = previous._env;
             _journal = previous._journal;
+            _id = previous._id;
             _envRecord = previous._envRecord;
             _freeSpaceHandling = previous._freeSpaceHandling;
             _allocator = allocator ?? new ByteStringContext(SharedMultipleUseFlag.None);
@@ -220,9 +222,10 @@ private LowLevelTransaction(LowLevelTransaction previous, TransactionPersistentC
             TxStartTime = DateTime.UtcNow;
             DataPager = previous.DataPager;
             DataPagerState = previous.DataPagerState;
+            _id = txId;
             _envRecord = previous._envRecord with
             {
-                TransactionId = txId,
+                TransactionId = _id,
                 Root = previous._root.ReadHeader()
             };
             _localTxNextPageNumber = previous._localTxNextPageNumber;
@@ -297,7 +300,8 @@ public LowLevelTransaction(StorageEnvironment env, TransactionPersistentContext
                 return;
             }
 
-            _envRecord = _envRecord with { TransactionId = _envRecord.TransactionId + 1 };
+            _id = _envRecord.TransactionId + 1;
+            _envRecord = _envRecord with { TransactionId = _id };
             _getPageMethod = GetPageMethod.WriteScratchFirst;
             _env.WriteTransactionPool.Reset();
             _dirtyPages = _env.WriteTransactionPool.DirtyPagesPool;
@@ -313,7 +317,12 @@ public LowLevelTransaction(StorageEnvironment env, TransactionPersistentContext
 
         public bool TryGetClientState<T>(out T value)
         {
-            if (_envRecord.ClientState is T t)
+            var envRecord = _envRecord;
+
+            if (envRecord is null) 
+                ThrowObjectDisposed();
+
+            if (envRecord.ClientState is T t)
             {
                 value = t;
                 return true;
@@ -807,7 +816,7 @@ public void Dispose()
                 PagerTransactionState.InvokeDispose(_env, ref DataPagerState, ref PagerTransactionState);
                 OnDispose?.Invoke(this);
 
-                _envRecord = null;
+                _envRecord = null; // RavenDB-22972 - it prevents from having a possible reference cycle via EnvironmentStateRecord.ClientState
             }
         }