Merge pull request #7233 from umbraco/v8/bugfix/6758-nucache-fix

When nucache updates are made the previous Gen root node is cleared

src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs

@@ -918,6 +918,14 @@ namespace Umbraco.Tests.PublishedContent
             var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null);
             _snapshotAccessor.PublishedSnapshot = snapshot;
 
+            var snapshotService = (PublishedSnapshotService)_snapshotService;
+            var contentStore = snapshotService.GetContentStore();
+
+            var parentNodes = contentStore.Test.GetValues(1);
+            var parentNode = parentNodes[0];
+            AssertLinkedNode(parentNode.contentNode, -1, -1, 2, 4, 6);
+            Assert.AreEqual(1, parentNode.gen);
+
             var documents = snapshot.Content.GetAtRoot().ToArray();
             AssertDocuments(documents, "N1", "N2", "N3");
 
@@ -934,6 +942,15 @@ namespace Umbraco.Tests.PublishedContent
                 new ContentCacheRefresher.JsonPayload(2, Guid.Empty, TreeChangeTypes.RefreshNode),
             }, out _, out _);
 
+            parentNodes = contentStore.Test.GetValues(1);
+            Assert.AreEqual(2, parentNodes.Length);
+            parentNode = parentNodes[1]; // get the first gen
+            AssertLinkedNode(parentNode.contentNode, -1, -1, 2, 4, 6); // the structure should have remained the same
+            Assert.AreEqual(1, parentNode.gen);
+            parentNode = parentNodes[0]; // get the latest gen
+            AssertLinkedNode(parentNode.contentNode, -1, -1, 2, 4, 6); // the structure should have remained the same
+            Assert.AreEqual(2, parentNode.gen);
+
             documents = snapshot.Content.GetAtRoot().ToArray();
             AssertDocuments(documents, "N1", "N2", "N3");
 
@@ -942,6 +959,8 @@ namespace Umbraco.Tests.PublishedContent
 
             documents = snapshot.Content.GetById(2).Children().ToArray();
             AssertDocuments(documents, "N9", "N8", "N7");
+
+            
         }
 
         [Test]

src/Umbraco.Web/PublishedCache/NuCache/ContentNode.cs

@@ -109,6 +109,8 @@ namespace Umbraco.Web.PublishedCache.NuCache
 
         // everything that is common to both draft and published versions
         // keep this as small as possible
+
+        
         public readonly int Id;
         public readonly Guid Uid;
         public IPublishedContentType ContentType;
@@ -116,10 +118,14 @@ namespace Umbraco.Web.PublishedCache.NuCache
         public readonly string Path;
         public readonly int SortOrder;
         public readonly int ParentContentId;
+
+        // TODO: Can we make everything readonly?? This would make it easier to debug and be less error prone especially for new developers.
+        // Once a Node is created and exists in the cache it is readonly so we should be able to make that happen at the API level too.
         public int FirstChildContentId;
         public int LastChildContentId;
         public int NextSiblingContentId;
         public int PreviousSiblingContentId;
+
         public readonly DateTime CreateDate;
         public readonly int CreatorId;
 

src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs

@@ -602,7 +602,7 @@ namespace Umbraco.Web.PublishedCache.NuCache
 
         private void ClearRootLocked()
         {
-            if (_root.Gen < _liveGen)
+            if (_root.Gen != _liveGen)
                 _root = new LinkedNode<ContentNode>(new ContentNode(), _liveGen, _root);
             else
                 _root.Value.FirstChildContentId = -1;
@@ -899,8 +899,18 @@ namespace Umbraco.Web.PublishedCache.NuCache
             return link;
         }
 
+        /// <summary>
+        /// This removes this current node from the tree hiearchy by removing it from it's parent's linked list
+        /// </summary>
+        /// <param name="content"></param>
+        /// <remarks>
+        /// This is called within a lock which means a new Gen is being created therefore this will not modify any existing content in a Gen.
+        /// </remarks>
         private void RemoveTreeNodeLocked(ContentNode content)
         {
+            // NOTE: DO NOT modify `content` here, this would modify data for an existing Gen, all modifications are done to clones
+            // which would be targeting the new Gen.
+
             var parentLink = content.ParentContentId < 0
                 ? _root
                 : GetRequiredLinkedNode(content.ParentContentId, "parent", null);
@@ -937,9 +947,6 @@ namespace Umbraco.Web.PublishedCache.NuCache
                 var prev = GenCloneLocked(prevLink);
                 prev.NextSiblingContentId = content.NextSiblingContentId;
             }
-
-            content.NextSiblingContentId = -1;
-            content.PreviousSiblingContentId = -1;
         }
 
         private bool ParentPublishedLocked(ContentNodeKit kit)
@@ -955,7 +962,7 @@ namespace Umbraco.Web.PublishedCache.NuCache
         {
             var node = link.Value;
 
-            if (node != null && link.Gen < _liveGen)
+            if (node != null && link.Gen != _liveGen)
             {
                 node = new ContentNode(link.Value);
                 if (link == _root)
@@ -1109,7 +1116,7 @@ namespace Umbraco.Web.PublishedCache.NuCache
             // this is safe only because we're write-locked
             foreach (var kvp in dict.Where(x => x.Value != null))
             {
-                if (kvp.Value.Gen < _liveGen)
+                if (kvp.Value.Gen != _liveGen)
                 {
                     var link = new LinkedNode<TValue>(null, _liveGen, kvp.Value);
                     dict.TryUpdate(kvp.Key, link, kvp.Value);