DYN-5128: Fix nodes executing twice after wire reconnect

src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs

@@ -238,7 +238,7 @@
 #endif
 
             var scopedNode = node as ScopedNodeModel;
-            IEnumerable<AssociativeNode> astNodes = 
+            IEnumerable<AssociativeNode> astNodes =
                 scopedNode != null
                     ? scopedNode.BuildAstInScope(inputAstNodes, verboseLogging, this)
                     : node.BuildAst(inputAstNodes, context);
@@ -335,7 +335,24 @@
 
             if (context == CompilationContext.DeltaExecution)
             {
-                sortedNodes = sortedNodes.Where(n => n.IsModified);
+                // When any non-input node in sortedNodes is marked IsModified, the DesignScript
+                // compiler will assign it a new, higher PC. Other non-input nodes in sortedNodes
+                // (which are already limited to downstream nodes of the modified set by
+                // ComputeModifiedNodes) keep their old, lower PCs. Because ApplyUpdate iterates
+                // graph nodes in ascending PC order, those nodes execute before the modified
+                // upstream node, consuming stale values. The upstream then runs, changes its
+                // output, and marks them dirty again — causing every affected node to execute
+                // twice. To prevent this, when any non-input node is IsModified, include all
+                // non-input nodes from sortedNodes in the compilation pass. The LiveRunner
+                // force-recompile path injects their ASTs into the delta with new, higher PCs,
+                // restoring topological execution order.
+                // Note: IsModified covers any pending re-evaluation, not only structural changes
+                // (e.g. reconnects). Non-structural triggers cause harmless extra compilation;
+                // the LiveRunner skips the force-recompile for subtrees whose AST is unchanged.
+                bool hasModifiedNonInputNode = sortedNodes.Any(n => n.IsModified && !n.IsInputNode);
+                sortedNodes = hasModifiedNonInputNode
+                    ? sortedNodes.Where(n => n.IsModified || !n.IsInputNode)
+                    : sortedNodes.Where(n => n.IsModified);
             }
 
             var result = new List<List<AssociativeNode>>();

src/Engine/ProtoCore/RuntimeData.cs

@@ -138,7 +138,18 @@ public CallSite GetCallSite(int classScope, string methodName, Executable execut
                 csInstance.UpdateCallSite(classScope, methodName);
                 if (runtimeCore.Options.IsDeltaExecution)
                 {
-                    runtimeCore.RuntimeStatus.ClearWarningForExpression(graphNode.exprUID);
+                    // Clear warnings for the specific Dynamo node being executed (by GUID + exprUID).
+                    // Using exprUID alone is unsafe because unrelated nodes can share the same exprUID
+                    // in delta compilation (e.g. force-recompiled downstream nodes), causing one
+                    // node's ClearWarningForExpression call to accidentally wipe another node's
+                    // runtime warnings. Using GUID alone is unsafe when a single Dynamo node compiles
+                    // to multiple AST expressions with distinct exprUIDs — clearing all warnings for
+                    // the GUID would remove a sibling expression's warning before it re-executes.
+                    // Fall back to exprUID for internal graph nodes (guid=Empty).
+                    if (!graphNode.guid.Equals(System.Guid.Empty))
+                        runtimeCore.RuntimeStatus.ClearWarningsForGraphNode(graphNode.guid, graphNode.exprUID);
+                    else
+                        runtimeCore.RuntimeStatus.ClearWarningForExpression(graphNode.exprUID);
                 }
             }
 

src/Engine/ProtoCore/RuntimeStatus.cs

@@ -150,6 +150,24 @@ public void ClearWarningsForGraph(Guid guid)
             }
         }
 
+        /// <summary>
+        /// Clears runtime warnings and infos that belong to a specific graph node GUID
+        /// AND a specific expression ID. This is more precise than <see cref="ClearWarningsForGraph"/>
+        /// and is needed when a single Dynamo node compiles to multiple AST expressions that each
+        /// get a distinct exprUID — clearing by GUID alone would remove warnings from sibling
+        /// expressions that have not yet been re-executed.
+        /// </summary>
+        /// <param name="guid">The graph-node GUID of the Dynamo node being re-executed.</param>
+        /// <param name="exprUID">The expression UID of the specific AST expression being re-executed.</param>
+        public void ClearWarningsForGraphNode(Guid guid, int exprUID)
+        {
+            using (rwl.CreateWriteLock())
+            {
+                warnings.RemoveAll(w => w.GraphNodeGuid.Equals(guid) && w.ExpressionID == exprUID);
+                infos.RemoveAll(w => w.GraphNodeGuid.Equals(guid) && w.ExpressionID == exprUID);
+            }
+        }
+
         public void ClearWarningsForAst(int astID)
         {
             using (rwl.CreateWriteLock())

src/Engine/ProtoScript/Runners/LiveRunner.cs

@@ -690,6 +690,63 @@
             }
         }
 
+        /// <summary>
+        /// Compares two AST nodes by textual content rather than object identity.
+        /// BuildAst always creates fresh objects, so reference equality fails even for
+        /// semantically unchanged nodes. ToString() produces a stable, identity-independent
+        /// representation that reflects the actual code the node will generate.
+        ///
+        /// ToString() alone is not always sufficient: FunctionCallNode.ToString() returns the
+        /// literal "null" for internal methods (those starting with '%') whose unprefixed name
+        /// doesn't parse as a binary or unary operator (e.g. "%conditionalIf" used by the If
+        /// node), hiding the actual arguments and their replication guides. When the stringified
+        /// form contains "null" — the narrow case where the textual form may be lossy — fall
+        /// back to the structural Equals operator (overridden on each AssociativeNode subclass
+        /// to do recursive content comparison) to confirm the match. ASTs that don't stringify
+        /// to a form containing "null" use only the fast textual comparison.
+        /// </summary>
+        private static bool AreAstNodesContentEqual(AssociativeNode a, AssociativeNode b)
+        {
+            if (ReferenceEquals(a, b)) return true;
+            if (a == null || b == null) return false;
+            var sa = a.ToString();
+            var sb = b.ToString();
+            if (sa != sb) return false;
+            if (sa.Contains(ProtoCore.DSDefinitions.Keyword.Null))
+            {
+                return a.Equals(b);
+            }
+            return true;
+        }
+
+        /// <summary>
+        /// Returns true if the subtree's AST nodes differ from the cached version.
+        /// Used to distinguish structural changes (reconnect) from identity changes (slider value).
+        /// Detects both additions (new nodes not in cache) and removals (cached nodes no longer present).
+        /// Uses content-based comparison so that freshly compiled but semantically unchanged nodes
+        /// are correctly detected as unmodified.
+        /// </summary>
+        private bool SubtreeHasActualChanges(Subtree subtree)
+        {
+            Subtree cached;
+            if (!currentSubTreeList.TryGetValue(subtree.GUID, out cached) || cached.AstNodes == null)
+                return subtree.AstNodes != null && subtree.AstNodes.Any();
+
+            var newNodes = subtree.AstNodes;
+            var cachedNodes = cached.AstNodes;
+
+            // Null or empty new AST when cache had content means all nodes were removed.
+            if (newNodes == null || !newNodes.Any())
+                return cachedNodes.Any();
+
+            // Different count means nodes were added or removed.
+            if (newNodes.Count() != cachedNodes.Count())
+                return true;
+
+            // Same count: any new node absent from the cache indicates a structural change.
+            return newNodes.Any(node => !cachedNodes.Any(prev => AreAstNodesContentEqual(prev, node)));
+        }
+
         /// <summary>
         /// Update the cached ASTs in the subtree given the modified ASTs
         /// </summary>
@@ -816,6 +873,16 @@
                 }
             }
 
+            // When any non-input subtree has structural changes (e.g. a reconnect), delta
+            // compilation recompiles that node at a new, higher PC. Unchanged downstream nodes
+            // keep their old, lower PCs. ApplyUpdate then finds them first (lower PC = earlier
+            // in the graph list) and runs them with stale upstream values, then the upstream
+            // node executes and marks them dirty again — causing each downstream node to execute
+            // twice. To prevent this, force-recompile all unchanged non-input subtrees so they
+            // receive new, higher PCs that respect the topological execution order.
+            bool anyNonInputActuallyModified = modifiedSubTrees.Any(
+                st => !st.IsInput && SubtreeHasActualChanges(st));
+
             for (int n = 0; n < modifiedSubTrees.Count(); ++n)
             {
                 var modifiedSubTree = modifiedSubTrees[n];
@@ -832,6 +899,35 @@
                     var modifiedASTList = GetModifiedNodes(modifiedSubTree, redefinitionAllowed, out modifiedInputAST);
                     csData.ModifiedNodesForRuntimeSetValue.AddRange(modifiedInputAST);
 
+                    // If another non-input subtree was structurally modified and this non-input
+                    // subtree appears unchanged, force-recompile it so its new graph node lands
+                    // at a PC higher than the recompiled upstream node. Also deactivate the old
+                    // graph nodes so they do not co-execute with the new ones in ApplyUpdate.
+                    if (!modifiedSubTree.IsInput && anyNonInputActuallyModified
+                        && !modifiedASTList.Any() && !modifiedInputAST.Any()
+                        && !SubtreeHasActualChanges(modifiedSubTree))
+                    {
+                        Subtree cachedSubTree;
+                        if (currentSubTreeList.TryGetValue(modifiedSubTree.GUID, out cachedSubTree)
+                            && cachedSubTree.AstNodes != null)
+                        {
+                            var cachedBinaryNodes = cachedSubTree.AstNodes.OfType<BinaryExpressionNode>().ToList();
+                            // Deactivate the old graph nodes so they are replaced by freshly compiled ones.
+                            csData.RemovedBinaryNodesFromModification.AddRange(cachedBinaryNodes);
+                            // Stamp the GUID and inject directly into the delta list so the compiler
+                            // assigns new, higher PCs — keeping modifiedASTList empty so
+                            // UpdateCachedASTList does not see these as modifications and double
+                            // the cached AST list.
+                            foreach (var bnode in cachedBinaryNodes)
+                            {
+                                bnode.guid = modifiedSubTree.GUID;
+                                bnode.IsInputExpression = false;
+                                SetNestedLanguageBlockASTGuids(modifiedSubTree.GUID, new List<ProtoCore.AST.Node>() { bnode });
+                            }
+                            deltaAstList.AddRange(cachedBinaryNodes);
+                        }
+                    }
+
                     modifiedSubTree.ModifiedAstNodes.Clear();
                     modifiedSubTree.ModifiedAstNodes.AddRange(modifiedASTList);
                     modifiedSubTree.ModifiedAstNodes.AddRange(modifiedInputAST);
@@ -860,7 +956,7 @@
                 }
                 else
                 {
-                    // No delta computation. 
+                    // No delta computation.
                     // Right now it is only disabled for code block node, but we may
                     // completely disable it. Details about why doing so:
                     // https://github.com/DynamoDS/Dynamo/pull/7282
@@ -997,7 +1093,7 @@
                 bool nodeFound = false;
                 foreach (AssociativeNode prevNode in st.AstNodes)
                 {
-                    if (prevNode.Equals(node))
+                    if (AreAstNodesContentEqual(prevNode, node))
                     {
                         nodeFound = true;
                         break;
@@ -1098,7 +1194,7 @@
                 bool prevNodeFoundInNewList = false;
                 foreach (AssociativeNode newNode in newASTList)
                 {
-                    if (prevNode.Equals(newNode))
+                    if (AreAstNodesContentEqual(prevNode, newNode))
                     {
                         // prev node still exists in the new list
                         prevNodeFoundInNewList = true;
@@ -1127,7 +1223,7 @@
             {
                 foreach (AssociativeNode newNode in newASTList)
                 {
-                    if (prevNode.Equals(newNode))
+                    if (AreAstNodesContentEqual(prevNode, newNode))
                     {
                         existingList.Add(prevNode);
                         break;

test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs

@@ -6095,7 +6095,75 @@ public void RegressMAGN7759()
             // There should be no warnings
             Assert.AreEqual(0, liveRunner.RuntimeCore.RuntimeStatus.WarningCount);
         }
-
+
+        /// <summary>
+        /// Regression test for DYN-5128.
+        /// When an upstream node is structurally modified (e.g. a wire reconnect), downstream
+        /// nodes that are included in the update with unchanged ASTs must receive new, higher PCs
+        /// so that the runtime executes them after the upstream node. Without the fix, unchanged
+        /// downstream nodes kept their original lower PCs and ran before the upstream, consuming
+        /// stale output and executing a second time after the upstream completed.
+        /// </summary>
+        [Test]
+        [Category("UnitTests")]
+        public void WhenUpstreamNodeReconnectedThenDownstreamNodesReceiveUpdatedValues()
+        {
+            // Arrange: three nodes in a chain A -> B -> C
+            var guidA = Guid.NewGuid();
+            var guidB = Guid.NewGuid();
+            var guidC = Guid.NewGuid();
+
+            var addedA = TestFrameWork.CreateSubTreeFromCode(guidA, "a = 1;");
+            addedA.DeltaComputation = true;
+            var addedB = TestFrameWork.CreateSubTreeFromCode(guidB, "b = a + 10;");
+            addedB.DeltaComputation = true;
+            var addedC = TestFrameWork.CreateSubTreeFromCode(guidC, "c = b + 100;");
+            addedC.DeltaComputation = true;
+
+            liveRunner.UpdateGraph(new GraphSyncData(null, new List<Subtree> { addedA, addedB, addedC }, null));
+
+            AssertValue("a", 1);
+            AssertValue("b", 11);
+            AssertValue("c", 111);
+
+            // Act: simulate reconnect — A changes structurally; B and C are included with their
+            // unchanged ASTs in topological order, as AstBuilder now does after the DYN-5128 fix.
+            // Reuse the same AstNodes object references from the initial add so that
+            // BinaryExpressionNode.Equals returns true in GetModifiedNodes, leaving modifiedASTList
+            // empty for B and C. The LiveRunner force-recompile path then gives B and C new PCs
+            // higher than A's new PC so the runtime executes them in the correct topological order.
+            var modA = TestFrameWork.CreateSubTreeFromCode(guidA, "a = 2;");
+            modA.DeltaComputation = true;
+            var modB = new Subtree(addedB.AstNodes, guidB) { DeltaComputation = true };
+            var modC = new Subtree(addedC.AstNodes, guidC) { DeltaComputation = true };
+
+            liveRunner.UpdateGraph(new GraphSyncData(null, null,
+                new List<Subtree> { modA, modB, modC }));
+
+            // Assert values: B and C must reflect A's new value, not the stale one.
+            AssertValue("a", 2);
+            AssertValue("b", 12);
+            AssertValue("c", 112);
+
+            // Assert PC ordering: after the reconnect the active graph nodes for A, B, C must
+            // be ordered A.startpc < B.startpc < C.startpc.  If the fix regresses, B and C keep
+            // their original low PCs (below A's new PC) and the runtime executes them before A,
+            // causing double execution.
+            var graphNodes = runtimeCore.DSExecutable.instrStreamList[0].dependencyGraph.GraphList;
+
+            // OriginalAstID == 0 identifies auto-generated infrastructure nodes (null-assignments,
+            // dependency tracking stubs). All user-expression graph nodes have OriginalAstID > 0
+            // because BinaryExpressionNode constructors always set OriginalAstID = ID and
+            // Node.ID = ++sID (starts at 1). Exclude them so PC ordering reflects only the
+            // actual expression evaluation nodes for A, B, and C.
+            int pcA = graphNodes.Where(n => n.guid == guidA && n.isActive && n.OriginalAstID != 0).Min(n => n.updateBlock.startpc);
+            int pcB = graphNodes.Where(n => n.guid == guidB && n.isActive && n.OriginalAstID != 0).Min(n => n.updateBlock.startpc);
+            int pcC = graphNodes.Where(n => n.guid == guidC && n.isActive && n.OriginalAstID != 0).Min(n => n.updateBlock.startpc);
+
+            Assert.Less(pcA, pcB, "Node A must have a lower PC than node B");
+            Assert.Less(pcB, pcC, "Node B must have a lower PC than node C");
+        }
+
     }
 
 }