From 0df94787e60cd6cd83d5fe3c737a73b976c68846 Mon Sep 17 00:00:00 2001 From: Roberto T <61755417+RobertGlobant20@users.noreply.github.com> Date: Tue, 5 May 2026 09:25:03 -0600 Subject: [PATCH 1/8] DYN-5128: Fix nodes executing twice after wire reconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DYN-5128: Fix nodes executing twice after wire reconnect When a node was reconnected, only that node was recompiled and received a new, higher PC in the instruction stream. Its downstream nodes kept their original lower PCs. Because ApplyUpdate scans graph nodes in ascending PC order, downstream nodes executed before their upstream dependency, consuming stale values. The upstream then ran, changed its output, and marked them dirty again — causing each downstream node to execute twice. Fix: when any non-input node is structurally modified, also compile all downstream non-input nodes (AstBuilder.cs) so they appear in the delta in topological order. The LiveRunner force-recompile path (LiveRunner.cs) then injects their cached ASTs into the delta after the modified node, giving them new higher PCs that respect the correct execution order. --- .../Engine/CodeGeneration/AstBuilder.cs | 15 ++++- src/Engine/ProtoScript/Runners/LiveRunner.cs | 61 +++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs b/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs index b0cd75103e5..7e36a092698 100644 --- a/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs +++ b/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs @@ -335,7 +335,20 @@ public IEnumerable>> CompileToAstN if (context == CompilationContext.DeltaExecution) { - sortedNodes = sortedNodes.Where(n => n.IsModified); + // If any non-input node has been structurally modified (e.g. a wire reconnect), + // the DesignScript compiler will assign it a new, higher PC. Downstream nodes + // that are not recompiled keep their old, lower PCs. Because ApplyUpdate iterates + // graph nodes in ascending PC order, those downstream nodes execute before the + // upstream node that just changed, 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, compile all non-input downstream nodes whenever a + // non-input node is structurally modified. The LiveRunner force-recompile path then + // injects their (structurally unchanged) ASTs into the delta with new, higher PCs + // that respect topological execution order. + 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>(); diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index 41b391401de..69763c31145 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -690,6 +690,29 @@ private void SetNestedLanguageBlockASTGuids(Guid guid, List } } + /// + /// Returns true if the subtree's AST nodes differ from the cached version. + /// Used to distinguish structural changes (reconnect) from identity changes (slider value). + /// + private bool SubtreeHasActualChanges(Subtree subtree) + { + Subtree cached; + if (!currentSubTreeList.TryGetValue(subtree.GUID, out cached) || cached.AstNodes == null) + return subtree.AstNodes != null && subtree.AstNodes.Any(); + if (subtree.AstNodes == null) + return false; + foreach (var node in subtree.AstNodes) + { + bool found = false; + foreach (var prevNode in cached.AstNodes) + { + if (prevNode.Equals(node)) { found = true; break; } + } + if (!found) return true; + } + return false; + } + /// /// Update the cached ASTs in the subtree given the modified ASTs /// @@ -816,6 +839,16 @@ private IEnumerable GetDeltaAstListModified(List modif } } + // 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 && st.AstNodes != null && SubtreeHasActualChanges(st)); + for (int n = 0; n < modifiedSubTrees.Count(); ++n) { var modifiedSubTree = modifiedSubTrees[n]; @@ -832,6 +865,34 @@ private IEnumerable GetDeltaAstListModified(List modif 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()) + { + Subtree cachedSubTree; + if (currentSubTreeList.TryGetValue(modifiedSubTree.GUID, out cachedSubTree) + && cachedSubTree.AstNodes != null) + { + var cachedBinaryNodes = cachedSubTree.AstNodes.OfType().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() { bnode }); + } + deltaAstList.AddRange(cachedBinaryNodes); + } + } + modifiedSubTree.ModifiedAstNodes.Clear(); modifiedSubTree.ModifiedAstNodes.AddRange(modifiedASTList); modifiedSubTree.ModifiedAstNodes.AddRange(modifiedInputAST); From 0b1b423b23bdba250292c5b849826d34c46378e6 Mon Sep 17 00:00:00 2001 From: Roberto T <61755417+RobertGlobant20@users.noreply.github.com> Date: Tue, 5 May 2026 16:21:53 -0600 Subject: [PATCH 2/8] Potential fix for pull request finding 'Missed opportunity to use Where' refactor Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> --- src/Engine/ProtoScript/Runners/LiveRunner.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index 69763c31145..bde034cab92 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -703,12 +703,7 @@ private bool SubtreeHasActualChanges(Subtree subtree) return false; foreach (var node in subtree.AstNodes) { - bool found = false; - foreach (var prevNode in cached.AstNodes) - { - if (prevNode.Equals(node)) { found = true; break; } - } - if (!found) return true; + if (!cached.AstNodes.Any(prevNode => prevNode.Equals(node))) return true; } return false; } From 9c8781ee49bc36fafa37922456837f75dd2ca9e2 Mon Sep 17 00:00:00 2001 From: Roberto T <61755417+RobertGlobant20@users.noreply.github.com> Date: Tue, 5 May 2026 16:49:52 -0600 Subject: [PATCH 3/8] DYN-5128: Address Copilot review feedback DYN-5128: Address Copilot review feedback Fix SubtreeHasActualChanges to also detect removed nodes: the previous implementation only checked whether new nodes existed in the cache, so a subtree that lost nodes was misclassified as unchanged. The method now handles null/empty new ASTs and short-circuits on count differences. Remove the st.AstNodes != null guard from anyNonInputActuallyModified: a non-input subtree going from having ASTs to null is a structural change, but the guard prevented SubtreeHasActualChanges from being called. Add regression test WhenUpstreamNodeReconnectedThenDownstreamNodesReceiveUpdatedValues in MicroFeatureTests to cover the reconnect scenario and exercise the LiveRunner force-recompile path. --- src/Engine/ProtoScript/Runners/LiveRunner.cs | 24 ++++++--- .../LiveRunnerTests/MicroFeatureTests.cs | 52 ++++++++++++++++++- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index bde034cab92..d7ace6be0e8 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -693,19 +693,27 @@ private void SetNestedLanguageBlockASTGuids(Guid guid, List /// /// 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). /// private bool SubtreeHasActualChanges(Subtree subtree) { Subtree cached; if (!currentSubTreeList.TryGetValue(subtree.GUID, out cached) || cached.AstNodes == null) return subtree.AstNodes != null && subtree.AstNodes.Any(); - if (subtree.AstNodes == null) - return false; - foreach (var node in subtree.AstNodes) - { - if (!cached.AstNodes.Any(prevNode => prevNode.Equals(node))) return true; - } - return false; + + 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 => prev.Equals(node))); } /// @@ -842,7 +850,7 @@ private IEnumerable GetDeltaAstListModified(List modif // 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 && st.AstNodes != null && SubtreeHasActualChanges(st)); + st => !st.IsInput && SubtreeHasActualChanges(st)); for (int n = 0; n < modifiedSubTrees.Count(); ++n) { diff --git a/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs b/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs index 1f859d07107..9c55e13884c 100644 --- a/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs +++ b/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs @@ -6095,7 +6095,57 @@ public void RegressMAGN7759() // There should be no warnings Assert.AreEqual(0, liveRunner.RuntimeCore.RuntimeStatus.WarningCount); } - + + /// + /// 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. + /// + [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 { 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. + // The LiveRunner force-recompile path gives B and C new PCs higher than A's new PC + // so the runtime executes them in the correct order. + var modA = TestFrameWork.CreateSubTreeFromCode(guidA, "a = 2;"); + modA.DeltaComputation = true; + var modB = TestFrameWork.CreateSubTreeFromCode(guidB, "b = a + 10;"); + modB.DeltaComputation = true; + var modC = TestFrameWork.CreateSubTreeFromCode(guidC, "c = b + 100;"); + modC.DeltaComputation = true; + + liveRunner.UpdateGraph(new GraphSyncData(null, null, + new List { modA, modB, modC })); + + // Assert: B and C must reflect A's new value, not the stale one + AssertValue("a", 2); + AssertValue("b", 12); + AssertValue("c", 112); + } + } } From d5c5c960cd38f0469876b14cc3a1cbf4c70662a3 Mon Sep 17 00:00:00 2001 From: Roberto T <61755417+RobertGlobant20@users.noreply.github.com> Date: Wed, 6 May 2026 09:23:07 -0600 Subject: [PATCH 4/8] DYN-5128 Code Review Fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AstBuilder.cs — Include all non-input nodes when any non-input is modified LiveRunner.cs — SubtreeHasActualChanges rewrite The method was rewritten to detect both additions and removals when comparing a subtree's current AST against its cached version. LiveRunner.cs — Force-recompile path for unchanged downstream nodes Updated test --- .../Engine/CodeGeneration/AstBuilder.cs | 24 ++++++++------ src/Engine/ProtoScript/Runners/LiveRunner.cs | 3 +- .../LiveRunnerTests/MicroFeatureTests.cs | 32 +++++++++++++++---- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs b/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs index 7e36a092698..87248fd14de 100644 --- a/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs +++ b/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs @@ -335,16 +335,20 @@ public IEnumerable>> CompileToAstN if (context == CompilationContext.DeltaExecution) { - // If any non-input node has been structurally modified (e.g. a wire reconnect), - // the DesignScript compiler will assign it a new, higher PC. Downstream nodes - // that are not recompiled keep their old, lower PCs. Because ApplyUpdate iterates - // graph nodes in ascending PC order, those downstream nodes execute before the - // upstream node that just changed, 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, compile all non-input downstream nodes whenever a - // non-input node is structurally modified. The LiveRunner force-recompile path then - // injects their (structurally unchanged) ASTs into the delta with new, higher PCs - // that respect topological execution order. + // 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) diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index d7ace6be0e8..22ab0b470bf 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -873,7 +873,8 @@ private IEnumerable GetDeltaAstListModified(List modif // 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()) + && !modifiedASTList.Any() && !modifiedInputAST.Any() + && !SubtreeHasActualChanges(modifiedSubTree)) { Subtree cachedSubTree; if (currentSubTreeList.TryGetValue(modifiedSubTree.GUID, out cachedSubTree) diff --git a/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs b/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs index 9c55e13884c..e71b58722c6 100644 --- a/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs +++ b/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs @@ -6128,22 +6128,40 @@ public void WhenUpstreamNodeReconnectedThenDownstreamNodesReceiveUpdatedValues() // 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. - // The LiveRunner force-recompile path gives B and C new PCs higher than A's new PC - // so the runtime executes them in the correct order. + // 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 = TestFrameWork.CreateSubTreeFromCode(guidB, "b = a + 10;"); - modB.DeltaComputation = true; - var modC = TestFrameWork.CreateSubTreeFromCode(guidC, "c = b + 100;"); - modC.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 { modA, modB, modC })); - // Assert: B and C must reflect A's new value, not the stale one + // 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"); } } From 0e35e9ed1e33a743f99c5fb661ece0be191022a8 Mon Sep 17 00:00:00 2001 From: Roberto T <61755417+RobertGlobant20@users.noreply.github.com> Date: Thu, 7 May 2026 20:33:28 -0600 Subject: [PATCH 5/8] DYN-5128 Fix wrong execution order and MAGN-7348 regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a double-execution ordering bug where reconnecting a non-input node (e.g. a CBN) caused unchanged downstream non-input nodes to execute before the upstream with stale values, then again after — producing incorrect results. LiveRunner.cs: Replace reference equality (Equals) with content-based AST comparison (ToString()) in SubtreeHasActualChanges, GetModifiedNodes, and GetUnmodifiedASTList. BuildAst always produces fresh object instances, so reference equality falsely treats semantically unchanged nodes as modified. RuntimeData.cs: Fix a regression in GetCallSite where two nodes in the same delta execution task could share the same exprUID, causing ClearWarningForExpression on one node to silently erase the runtime warning of the other. Switch to GUID-based clearing (ClearWarningsForGraph) for Dynamo nodes so each node only clears its own warnings. Fall back to exprUID-based clearing for internal graph nodes (guid == Empty). --- src/Engine/ProtoCore/RuntimeData.cs | 10 +++++++++- src/Engine/ProtoScript/Runners/LiveRunner.cs | 21 +++++++++++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/Engine/ProtoCore/RuntimeData.cs b/src/Engine/ProtoCore/RuntimeData.cs index df67c8a0555..7b7a28e4523 100644 --- a/src/Engine/ProtoCore/RuntimeData.cs +++ b/src/Engine/ProtoCore/RuntimeData.cs @@ -138,7 +138,15 @@ 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). + // Using exprUID 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. Fall back to exprUID for internal graph nodes (guid=Empty). + if (!graphNode.guid.Equals(System.Guid.Empty)) + runtimeCore.RuntimeStatus.ClearWarningsForGraph(graphNode.guid); + else + runtimeCore.RuntimeStatus.ClearWarningForExpression(graphNode.exprUID); } } diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index 22ab0b470bf..a9641e9096f 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -690,10 +690,25 @@ private void SetNestedLanguageBlockASTGuids(Guid guid, List } } + /// + /// 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. + /// + private static bool AreAstNodesContentEqual(AssociativeNode a, AssociativeNode b) + { + if (ReferenceEquals(a, b)) return true; + if (a == null || b == null) return false; + return a.ToString() == b.ToString(); + } + /// /// 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. /// private bool SubtreeHasActualChanges(Subtree subtree) { @@ -713,7 +728,7 @@ private bool SubtreeHasActualChanges(Subtree subtree) return true; // Same count: any new node absent from the cache indicates a structural change. - return newNodes.Any(node => !cachedNodes.Any(prev => prev.Equals(node))); + return newNodes.Any(node => !cachedNodes.Any(prev => AreAstNodesContentEqual(prev, node))); } /// @@ -1062,7 +1077,7 @@ private List GetModifiedNodes(Subtree subtree, bool redefinitio bool nodeFound = false; foreach (AssociativeNode prevNode in st.AstNodes) { - if (prevNode.Equals(node)) + if (AreAstNodesContentEqual(prevNode, node)) { nodeFound = true; break; @@ -1192,7 +1207,7 @@ private List GetUnmodifiedASTList(List prevAST { foreach (AssociativeNode newNode in newASTList) { - if (prevNode.Equals(newNode)) + if (AreAstNodesContentEqual(prevNode, newNode)) { existingList.Add(prevNode); break; From 718a549792ca1e348a14661f41868904f53f293c Mon Sep 17 00:00:00 2001 From: Roberto T <61755417+RobertGlobant20@users.noreply.github.com> Date: Fri, 8 May 2026 09:23:22 -0600 Subject: [PATCH 6/8] DYN-5128: Fix runtime warning clearing to match both GUID and exprUID Replaced ClearWarningsForGraph(guid) with a new ClearWarningsForGraphNode(guid, exprUID) method in RuntimeStatus. The previous GUID-only clear was too broad: nodes that compile to multiple AST expressions (e.g. BasicLineChartNodeModel) share the same GUID but have distinct exprUIDs, causing sibling expression warnings to be wiped prematurely. The previous exprUID-only clear (ClearWarningForExpression) was also unsafe: force-recompiled downstream nodes can share an exprUID with unrelated nodes in delta execution, causing one node's clear to accidentally remove another node's runtime warnings. Fixes MAGN_7348_Core_Python and LiveChartsBasicLineChartCreationTest regressions. --- src/Engine/ProtoCore/RuntimeData.cs | 11 +++++++---- src/Engine/ProtoCore/RuntimeStatus.cs | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/Engine/ProtoCore/RuntimeData.cs b/src/Engine/ProtoCore/RuntimeData.cs index 7b7a28e4523..52b25665024 100644 --- a/src/Engine/ProtoCore/RuntimeData.cs +++ b/src/Engine/ProtoCore/RuntimeData.cs @@ -138,13 +138,16 @@ public CallSite GetCallSite(int classScope, string methodName, Executable execut csInstance.UpdateCallSite(classScope, methodName); if (runtimeCore.Options.IsDeltaExecution) { - // Clear warnings for the specific Dynamo node being executed (by GUID). - // Using exprUID is unsafe because unrelated nodes can share the same 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. Fall back to exprUID for internal graph nodes (guid=Empty). + // 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.ClearWarningsForGraph(graphNode.guid); + runtimeCore.RuntimeStatus.ClearWarningsForGraphNode(graphNode.guid, graphNode.exprUID); else runtimeCore.RuntimeStatus.ClearWarningForExpression(graphNode.exprUID); } diff --git a/src/Engine/ProtoCore/RuntimeStatus.cs b/src/Engine/ProtoCore/RuntimeStatus.cs index 5cbc5792d75..f36fc179f5b 100644 --- a/src/Engine/ProtoCore/RuntimeStatus.cs +++ b/src/Engine/ProtoCore/RuntimeStatus.cs @@ -150,6 +150,24 @@ public void ClearWarningsForGraph(Guid guid) } } + /// + /// Clears runtime warnings and infos that belong to a specific graph node GUID + /// AND a specific expression ID. This is more precise than + /// 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. + /// + /// The graph-node GUID of the Dynamo node being re-executed. + /// The expression UID of the specific AST expression being re-executed. + 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()) From 1f7481f126f53813780b12f20f9311df45a87a1d Mon Sep 17 00:00:00 2001 From: Roberto T <61755417+RobertGlobant20@users.noreply.github.com> Date: Fri, 8 May 2026 15:11:22 -0600 Subject: [PATCH 7/8] DYN-5128: Use structural Equals for AST comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaced ToString()-based comparison in AreAstNodesContentEqual with the structural Equals() method already overridden on AssociativeNode subclasses (BinaryExpressionNode, IdentifierNode, FunctionCallNode, ArrayNameNode). ToString() was unsafe: FunctionCallNode.ToString() returns the literal string "null" for internal methods (those prefixed with '%') whose unprefixed name doesn't parse as a binary or unary operator — e.g. "%conditionalIf" used by the If node. This caused every IfNode AST to stringify identically regardless of its actual inputs or replication guides, so SubtreeHasActualChanges returned false even when lacing changed from Auto to Longest, and the runtime never received the new AST. Also updated GetInactiveASTList to use AreAstNodesContentEqual for symmetry with GetModifiedNodes and GetUnmodifiedASTList. Without this, an unchanged- content subtree included in modifiedSubTrees (e.g. via downstream-dirty propagation) would have all its old AST nodes queued for deactivation while no new ones were compiled to replace them, leaving the node with no active graph nodes at runtime. Fixes TestIfNodeLacingOptions regression while preserving the original DYN-5128 wire-reconnect fix and the MAGN_7348_Core_Python and LiveChartsBasicLineChartCreationTest fixes. --- .../Engine/CodeGeneration/AstBuilder.cs | 2 +- src/Engine/ProtoScript/Runners/LiveRunner.cs | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs b/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs index 87248fd14de..6c12a3d7f67 100644 --- a/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs +++ b/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs @@ -238,7 +238,7 @@ private void CompileToAstNodes( #endif var scopedNode = node as ScopedNodeModel; - IEnumerable astNodes = + IEnumerable astNodes = scopedNode != null ? scopedNode.BuildAstInScope(inputAstNodes, verboseLogging, this) : node.BuildAst(inputAstNodes, context); diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index a9641e9096f..344dea38e2a 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -691,16 +691,23 @@ private void SetNestedLanguageBlockASTGuids(Guid guid, List } /// - /// Compares two AST nodes by textual content rather than object identity. + /// Compares two AST nodes by structural 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. + /// semantically unchanged nodes. The AssociativeNode subclasses (BinaryExpressionNode, + /// IdentifierNode, FunctionCallNode, ArrayNameNode, ...) override Equals() to do + /// recursive structural comparison — including replication guides on identifiers, + /// which is what distinguishes an IfNode with Auto lacing from one with Longest lacing. + /// + /// Note: do not use ToString() for this comparison. 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"), which would + /// make all IfNode ASTs compare equal regardless of their actual contents. /// private static bool AreAstNodesContentEqual(AssociativeNode a, AssociativeNode b) { if (ReferenceEquals(a, b)) return true; if (a == null || b == null) return false; - return a.ToString() == b.ToString(); + return a.Equals(b); } /// @@ -940,7 +947,7 @@ private IEnumerable GetDeltaAstListModified(List modif } 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 @@ -1178,7 +1185,7 @@ private List GetInactiveASTList(List prevASTLi 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; From 8cf1f9840a0f5cdca1207a05d5ed0695b2c145fe Mon Sep 17 00:00:00 2001 From: Roberto T <61755417+RobertGlobant20@users.noreply.github.com> Date: Tue, 12 May 2026 08:00:39 -0600 Subject: [PATCH 8/8] DYN-5128: Hybrid ToString/Equals AST comparison Reverted AreAstNodesContentEqual to use ToString as the primary comparison and fall back to structural Equals only when the stringified form contains "null". The previous structural-Equals-only version caused ValueSchemaProviderReturnsTypeId (DefineData) to hang or crash. The "null" check targets the narrow case where FunctionCallNode.ToString collapses an internal-method call (e.g. "%conditionalIf" used by the If node) to the literal string "null", hiding the actual arguments and their replication guides. ASTs whose textual form doesn't contain "null" use only the cheap string comparison and never invoke Equals. Why: Equals delegates through every overridden AssociativeNode subclass and the cost or behavior on certain DefineData AST shapes was not safe in practice. ToString is fast and accurate for the common case; falling back to Equals only when the textual form is known to be lossy keeps the IfNode lacing fix intact without affecting other node types. Verified passing: TestIfNodeLacingOptions, ValueSchemaProviderReturnsTypeId, WhenUpstreamNodeReconnectedThenDownstreamNodesReceiveUpdatedValues, MAGN_7348_Core_Python, and LiveChartsBasicLineChartCreationTest. --- src/Engine/ProtoScript/Runners/LiveRunner.cs | 29 +++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index 344dea38e2a..8bb9217a2e3 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -691,23 +691,32 @@ private void SetNestedLanguageBlockASTGuids(Guid guid, List } /// - /// Compares two AST nodes by structural content rather than object identity. + /// 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. The AssociativeNode subclasses (BinaryExpressionNode, - /// IdentifierNode, FunctionCallNode, ArrayNameNode, ...) override Equals() to do - /// recursive structural comparison — including replication guides on identifiers, - /// which is what distinguishes an IfNode with Auto lacing from one with Longest lacing. + /// semantically unchanged nodes. ToString() produces a stable, identity-independent + /// representation that reflects the actual code the node will generate. /// - /// Note: do not use ToString() for this comparison. 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"), which would - /// make all IfNode ASTs compare equal regardless of their actual contents. + /// 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. /// private static bool AreAstNodesContentEqual(AssociativeNode a, AssociativeNode b) { if (ReferenceEquals(a, b)) return true; if (a == null || b == null) return false; - return a.Equals(b); + 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; } ///