DYN-5128: Fix nodes executing twice after wire reconnect#17095
Open
RobertGlobant20 wants to merge 9 commits into
Open
DYN-5128: Fix nodes executing twice after wire reconnect#17095RobertGlobant20 wants to merge 9 commits into
RobertGlobant20 wants to merge 9 commits into
Conversation
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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-5128
Contributor
Author
|
GIF showing the expected behavior |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an ordering issue in Dynamo’s delta execution pipeline where reconnecting wires could cause downstream nodes to execute twice due to PC (program counter) re-assignment affecting execution order in ApplyUpdate.
Changes:
- Updates
AstBuilderdelta compilation filtering so downstream non-input nodes are included in delta compilation when a non-input node is modified, preserving topological execution order in the delta. - Adds LiveRunner logic to detect “actual” (structural) subtree changes and, when needed, force-recompile unchanged downstream non-input subtrees by injecting cached ASTs into the delta to obtain new higher PCs.
- Deactivates old graph nodes for those forced recompiles to avoid co-execution with newly compiled graph nodes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/Engine/ProtoScript/Runners/LiveRunner.cs |
Adds structural-change detection and a force-recompile/injection path to ensure downstream nodes receive higher PCs and don’t execute twice after reconnect-like edits. |
src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs |
Expands delta compilation set (under certain conditions) to include non-input nodes so downstream nodes appear in the delta in topological order. |
Comment on lines
+700
to
+712
| 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; | ||
| } |
Comment on lines
+702
to
+713
| 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; |
| // 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)); |
Comment on lines
+338
to
+351
| // 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); |
Contributor
Author
There was a problem hiding this comment.
even if that happens, anyNonInputActuallyModified would be false (since SubtreeHasActualChanges would return false for NodeA), so the force-recompile block never fires. The performance cost is real but contained — wasted compilation work, no correctness issue.
Comment on lines
+338
to
+351
| // 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); |
refactor Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
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.
Comment on lines
+871
to
+877
| // 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()) | ||
| { |
Comment on lines
+344
to
+351
| // 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); |
Comment on lines
+6143
to
+6146
| // Assert: B and C must reflect A's new value, not the stale one | ||
| AssertValue("a", 2); | ||
| AssertValue("b", 12); | ||
| AssertValue("c", 112); |
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
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).
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.
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.
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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
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.
Declarations
Check these if you believe they are true
Release Notes
DYN-5128: Fix nodes executing twice after wire reconnect
Reviewers
@jasonstratton @aparajit-pratap @eamiri
FYIs
@jnealb