Switch to new async execution model - #4391
Open
TrueDoctor wants to merge 1 commit into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
12 issues found across 71 files
Confidence score: 2/5
- The highest risk is in async execution plumbing across
editor/src/node_graph_executor/runtime.rsandnode-graph/graphene-cli/src/export.rs:NoopSpawnerdrops source futures and export stops on the firstPending, so async graphs can stall indefinitely or fail instead of completing — wire a real runtime spawner plus completion/retry signaling until terminalGPollresults. - GPU adjustment graph construction looks brittle in
node-graph/node-macro/src/shader_nodes/per_pixel_adjust.rsbecause generated nodes reference an unregistered scope, which can leave executor inputs unresolved and break graph builds — target the existingwgpu_executorscope node and its expected input type. - Compatibility paths have concrete regression risk in
editor/src/messages/portfolio/document/utility_types/network_interface.rsandnode-graph/graph-craft/src/document/value.rs: legacyload_resourcedocs can migrate with missing inputs, and several list/context outputs cannot round-trip from edges back toTaggedValue, causing load/evaluation failures — run migration normalization aftermigrate_nodeand add missingfrom_edgeconversions. node-graph/libraries/wgpu-executor/src/shader_runtime/per_pixel_adjust_runtime.rsnow usesstd::sync::Mutexwithlock().unwrap(), so a poisoned lock can panic the whole app under thread failure — switch to non-panicking poison handling (for exampleunwrap_or_else) to contain runtime faults.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/node-macro/src/shader_nodes/per_pixel_adjust.rs">
<violation number="1" location="node-graph/node-macro/src/shader_nodes/per_pixel_adjust.rs:236">
P1: Generated GPU adjustment nodes now request a scope node that is not registered, so their executor input cannot be resolved when graphs are built. Point this field at the existing `wgpu_executor` scope node and use its `WgpuExecutorHandle` wire type instead of inventing an Arc-specific node ID.</violation>
</file>
<file name="node-graph/libraries/wgpu-executor/src/shader_runtime/per_pixel_adjust_runtime.rs">
<violation number="1" location="node-graph/libraries/wgpu-executor/src/shader_runtime/per_pixel_adjust_runtime.rs:37">
P1: Switching from `futures::lock::Mutex` to `std::sync::Mutex` introduces a poison panic risk: `lock().unwrap()` will crash the entire application if another thread panicked while holding the mutex. Use `.lock().unwrap_or_else(|e| e.into_inner())` to recover the inner pipeline cache from the poison error instead of panicking.</violation>
</file>
<file name="node-graph/graphene-cli/src/export.rs">
<violation number="1" location="node-graph/graphene-cli/src/export.rs:20">
P3: Successful fallback exports are recorded as errors, adding expected recovery events to error logs. Use `warn!` (or lower) unless this path is meant to fail the export.
(Based on your team's feedback about error-level logging for expected fallbacks.) [FEEDBACK_USED]</violation>
<violation number="2" location="node-graph/graphene-cli/src/export.rs:23">
P1: Exports containing async nodes now fail on their initial `Pending` result instead of waiting for graph work to complete. Wire a real runtime spawner and drive/retry evaluation until a terminal `GPoll` result; the current `NoopSpawner` drops the scheduled task entirely.</violation>
</file>
<file name="node-graph/node-macro/src/parsing.rs">
<violation number="1" location="node-graph/node-macro/src/parsing.rs:474">
P3: A `placeholder(...)` on a synchronous non-source node is silently ignored, making a typo/configuration mistake look valid. Validate that this attribute is used only by async/source-kernel nodes.</violation>
</file>
<file name="editor/src/messages/portfolio/document/utility_types/network_interface.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/network_interface.rs:118">
P1: Opening documents with the older 3-input `load_resource` layout leaves the node at two inputs after its legacy migration, without the required runtime and source-ID inputs. Run this pass after `migrate_node` normalizes legacy node arities (or invoke it again then) so those documents enter the new async model too.</violation>
</file>
<file name="node-graph/nodes/repeat/src/repeat_nodes.rs">
<violation number="1" location="node-graph/nodes/repeat/src/repeat_nodes.rs:13">
P3: Repeat nodes no longer have regression coverage for count/reverse ordering, transforms, or position-context propagation after this execution-model migration. Retain or rewrite the removed tests for synchronous `Node::eval`/`GPoll` behavior so these paths remain verified.</violation>
</file>
<file name="node-graph/libraries/core-types/src/context.rs">
<violation number="1" location="node-graph/libraries/core-types/src/context.rs:455">
P3: Node type displays will regress from `Context` to `ContextImpl`, since aliases are transparent to `type_name` and the user-readable type mapping still only handles `OwnedContextImpl`. Update that mapping (or register a descriptor alias) for the new representation.
(Based on your team's feedback about names reflecting current roles.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/libraries/core-types/src/registry.rs">
<violation number="1" location="node-graph/libraries/core-types/src/registry.rs:106">
P3: Every edge evaluation now depends on a manually maintained raw pointer although `Arc<N>` already forwards `Node`; retain only the `Arc` and delegate through it to remove unchecked dereferences and manual auto-trait invariants.
(Based on your team's feedback about using safe code before unsafe.) [FEEDBACK_USED]</violation>
<violation number="2" location="node-graph/libraries/core-types/src/registry.rs:244">
P3: Registry-level arity/type validation is test-only because production invokes `NodeConstructor` directly. Store/pass the `RegistryEntry` or route execution through `construct` so validation is centralized instead of duplicated in every constructor.</violation>
</file>
<file name="editor/src/node_graph_executor/runtime.rs">
<violation number="1" location="editor/src/node_graph_executor/runtime.rs:126">
P0: Async graph nodes never finish because `NoopSpawner` drops every source future. Wire a real platform spawner and completion notification into the editor so pending/partial graphs are re-executed when their sources complete.</violation>
</file>
<file name="node-graph/graph-craft/src/document/value.rs">
<violation number="1" location="node-graph/graph-craft/src/document/value.rs:339">
P1: Graph outputs such as `List<Graphic>`, `List<Color>`, `List<f64>`, `List<NodeId>`, or `ContextModification` cannot be converted back to `TaggedValue`: `to_edge` supports creating these edges, but `from_edge` has no corresponding cases and returns an error. This makes valid non-`RenderOutput` graph outputs fail in `DynamicExecutor::execute`; the conversion should cover every type materialized by `to_edge` (including the `TypeDefault` list types).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| impl NodeRuntime { | ||
| pub fn new(receiver: Receiver<GraphRuntimeRequest>, sender: Sender<NodeGraphUpdate>) -> Self { | ||
| let spawner: Box<DynSpawner> = Box::new(NoopSpawner); |
Contributor
There was a problem hiding this comment.
P0: Async graph nodes never finish because NoopSpawner drops every source future. Wire a real platform spawner and completion notification into the editor so pending/partial graphs are re-executed when their sources complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/node_graph_executor/runtime.rs, line 126:
<comment>Async graph nodes never finish because `NoopSpawner` drops every source future. Wire a real platform spawner and completion notification into the editor so pending/partial graphs are re-executed when their sources complete.</comment>
<file context>
@@ -121,18 +123,25 @@ pub static NODE_RUNTIME: once_cell::sync::Lazy<Mutex<Option<NodeRuntime>>> = onc
impl NodeRuntime {
pub fn new(receiver: Receiver<GraphRuntimeRequest>, sender: Sender<NodeGraphUpdate>) -> Self {
+ let spawner: Box<DynSpawner> = Box::new(NoopSpawner);
+ let graph_runtime: Arc<DynGraphRuntime> = Arc::new(GraphRuntime::new(spawner));
+ let mut executor = DynamicExecutor::default();
</file context>
| ty: parse_quote!(std::sync::Arc<WgpuExecutor>), | ||
| exposed: true, | ||
| value_source: ParsedValueSource::Scope(parse_quote!("graphene_std::platform_application_io::WgpuExecutorNode")), | ||
| value_source: ParsedValueSource::Scope(Box::new(parse_quote!("graphene_std::platform_application_io::WgpuExecutorArcNode"))), |
Contributor
There was a problem hiding this comment.
P1: Generated GPU adjustment nodes now request a scope node that is not registered, so their executor input cannot be resolved when graphs are built. Point this field at the existing wgpu_executor scope node and use its WgpuExecutorHandle wire type instead of inventing an Arc-specific node ID.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/node-macro/src/shader_nodes/per_pixel_adjust.rs, line 236:
<comment>Generated GPU adjustment nodes now request a scope node that is not registered, so their executor input cannot be resolved when graphs are built. Point this field at the existing `wgpu_executor` scope node and use its `WgpuExecutorHandle` wire type instead of inventing an Arc-specific node ID.</comment>
<file context>
@@ -231,9 +231,9 @@ impl PerPixelAdjustCodegen<'_> {
+ ty: parse_quote!(std::sync::Arc<WgpuExecutor>),
exposed: true,
- value_source: ParsedValueSource::Scope(parse_quote!("graphene_std::platform_application_io::WgpuExecutorNode")),
+ value_source: ParsedValueSource::Scope(Box::new(parse_quote!("graphene_std::platform_application_io::WgpuExecutorArcNode"))),
number_soft_min: None,
number_soft_max: None,
</file context>
| pub async fn run_per_pixel_adjust<T: BufferStruct>(&self, shaders: &Shaders<'_>, textures: List<Raster<GPU>>, args: Option<&T>) -> List<Raster<GPU>> { | ||
| let mut cache = self.per_pixel_adjust.pipeline_cache.lock().await; | ||
| pub fn run_per_pixel_adjust<T: BufferStruct>(&self, shaders: &Shaders<'_>, textures: List<Raster<GPU>>, args: Option<&T>) -> List<Raster<GPU>> { | ||
| let mut cache = self.per_pixel_adjust.pipeline_cache.lock().unwrap(); |
Contributor
There was a problem hiding this comment.
P1: Switching from futures::lock::Mutex to std::sync::Mutex introduces a poison panic risk: lock().unwrap() will crash the entire application if another thread panicked while holding the mutex. Use .lock().unwrap_or_else(|e| e.into_inner()) to recover the inner pipeline cache from the poison error instead of panicking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/shader_runtime/per_pixel_adjust_runtime.rs, line 37:
<comment>Switching from `futures::lock::Mutex` to `std::sync::Mutex` introduces a poison panic risk: `lock().unwrap()` will crash the entire application if another thread panicked while holding the mutex. Use `.lock().unwrap_or_else(|e| e.into_inner())` to recover the inner pipeline cache from the poison error instead of panicking.</comment>
<file context>
@@ -33,8 +33,8 @@ impl PerPixelAdjustShaderRuntime {
- pub async fn run_per_pixel_adjust<T: BufferStruct>(&self, shaders: &Shaders<'_>, textures: List<Raster<GPU>>, args: Option<&T>) -> List<Raster<GPU>> {
- let mut cache = self.per_pixel_adjust.pipeline_cache.lock().await;
+ pub fn run_per_pixel_adjust<T: BufferStruct>(&self, shaders: &Shaders<'_>, textures: List<Raster<GPU>>, args: Option<&T>) -> List<Raster<GPU>> {
+ let mut cache = self.per_pixel_adjust.pipeline_cache.lock().unwrap();
let pipeline = cache
.entry(shaders.fragment_shader_name.to_owned())
</file context>
| log::error!("Node graph evaluation reported an error alongside its fallback output: {error:?}"); | ||
| Ok(value) | ||
| } | ||
| GPoll::Partial(_) | GPoll::Pending => Err("Node graph evaluation did not complete".into()), |
Contributor
There was a problem hiding this comment.
P1: Exports containing async nodes now fail on their initial Pending result instead of waiting for graph work to complete. Wire a real runtime spawner and drive/retry evaluation until a terminal GPoll result; the current NoopSpawner drops the scheduled task entirely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/graphene-cli/src/export.rs, line 23:
<comment>Exports containing async nodes now fail on their initial `Pending` result instead of waiting for graph work to complete. Wire a real runtime spawner and drive/retry evaluation until a terminal `GPoll` result; the current `NoopSpawner` drops the scheduled task entirely.</comment>
<file context>
@@ -10,6 +12,19 @@ use std::io::Cursor;
+ log::error!("Node graph evaluation reported an error alongside its fallback output: {error:?}");
+ Ok(value)
+ }
+ GPoll::Partial(_) | GPoll::Pending => Err("Node graph evaluation did not complete".into()),
+ GPoll::Error(error) => Err(format!("Node graph evaluation failed: {error:?}").into()),
+ }
</file context>
| if let DocumentNodeImplementation::ProtoNode(protonode) = &node.implementation | ||
| && let Some(base) = protonode.as_str().split('<').next() | ||
| && let Some((_, arity)) = PRE_INJECTION_ARITIES.iter().find(|(identifier, _)| *identifier == base) | ||
| && node.inputs.len() == *arity |
Contributor
There was a problem hiding this comment.
P1: Opening documents with the older 3-input load_resource layout leaves the node at two inputs after its legacy migration, without the required runtime and source-ID inputs. Run this pass after migrate_node normalizes legacy node arities (or invoke it again then) so those documents enter the new async model too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface.rs, line 118:
<comment>Opening documents with the older 3-input `load_resource` layout leaves the node at two inputs after its legacy migration, without the required runtime and source-ID inputs. Run this pass after `migrate_node` normalizes legacy node arities (or invoke it again then) so those documents enter the new async model too.</comment>
<file context>
@@ -95,6 +95,34 @@ impl NodeNetworkInterface {
+ if let DocumentNodeImplementation::ProtoNode(protonode) = &node.implementation
+ && let Some(base) = protonode.as_str().split('<').next()
+ && let Some((_, arity)) = PRE_INJECTION_ARITIES.iter().find(|(identifier, _)| *identifier == base)
+ && node.inputs.len() == *arity
+ {
+ node.inputs.push(NodeInput::scope("graphene_std::runtime::RuntimeNode"));
</file context>
| // | ||
| // Example usage: | ||
| // #[node_macro::node(..., placeholder(empty_image), ...)] | ||
| "placeholder" => { |
Contributor
There was a problem hiding this comment.
P3: A placeholder(...) on a synchronous non-source node is silently ignored, making a typo/configuration mistake look valid. Validate that this attribute is used only by async/source-kernel nodes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/node-macro/src/parsing.rs, line 474:
<comment>A `placeholder(...)` on a synchronous non-source node is silently ignored, making a typo/configuration mistake look valid. Validate that this attribute is used only by async/source-kernel nodes.</comment>
<file context>
@@ -453,13 +466,63 @@ impl Parse for NodeFnAttributes {
+ //
+ // Example usage:
+ // #[node_macro::node(..., placeholder(empty_image), ...)]
+ "placeholder" => {
+ let meta = meta.require_list()?;
+ if placeholder.is_some() {
</file context>
| #[node_macro::node(category("Repeat"))] | ||
| async fn repeat<T: Into<Graphic> + Default + Send + Clone + 'static>( | ||
| ctx: impl ExtractAll + CloneVarArgs + Ctx, | ||
| fn repeat<T: Into<Graphic> + Default + Send + Clone + 'static>( |
Contributor
There was a problem hiding this comment.
P3: Repeat nodes no longer have regression coverage for count/reverse ordering, transforms, or position-context propagation after this execution-model migration. Retain or rewrite the removed tests for synchronous Node::eval/GPoll behavior so these paths remain verified.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/repeat/src/repeat_nodes.rs, line 13:
<comment>Repeat nodes no longer have regression coverage for count/reverse ordering, transforms, or position-context propagation after this execution-model migration. Retain or rewrite the removed tests for synchronous `Node::eval`/`GPoll` behavior so these paths remain verified.</comment>
<file context>
@@ -1,71 +1,73 @@
#[node_macro::node(category("Repeat"))]
-async fn repeat<T: Into<Graphic> + Default + Send + Clone + 'static>(
- ctx: impl ExtractAll + CloneVarArgs + Ctx,
+fn repeat<T: Into<Graphic> + Default + Send + Clone + 'static>(
+ ctx: impl Ctx + DeriveCtx,
#[implementations(
</file context>
| // ============== | ||
|
|
||
| pub type Context<'a> = Option<Arc<OwnedContextImpl>>; | ||
| pub type Context<'a> = ContextImpl<'a>; |
Contributor
There was a problem hiding this comment.
P3: Node type displays will regress from Context to ContextImpl, since aliases are transparent to type_name and the user-readable type mapping still only handles OwnedContextImpl. Update that mapping (or register a descriptor alias) for the new representation.
(Based on your team's feedback about names reflecting current roles.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/context.rs, line 455:
<comment>Node type displays will regress from `Context` to `ContextImpl`, since aliases are transparent to `type_name` and the user-readable type mapping still only handles `OwnedContextImpl`. Update that mapping (or register a descriptor alias) for the new representation.
(Based on your team's feedback about names reflecting current roles.) </comment>
<file context>
@@ -448,185 +448,12 @@ impl ExtractFootprint for () {
+// ==============
-pub type Context<'a> = Option<Arc<OwnedContextImpl>>;
+pub type Context<'a> = ContextImpl<'a>;
type DynRef<'a> = &'a (dyn Any + Send + Sync);
-type DynBox = Box<dyn AnyHash + Send + Sync>;
</file context>
| match dyn_any::downcast(input) { | ||
| Ok(input) => Box::pin(output(*input)), | ||
| Err(e) => panic!("DynAnyNode Input, {e} in:\n{node_name}"), | ||
| pub fn construct(entry: &RegistryEntry, inputs: Vec<EdgeHandle>) -> Result<EdgeHandle, ConstructionError> { |
Contributor
There was a problem hiding this comment.
P3: Registry-level arity/type validation is test-only because production invokes NodeConstructor directly. Store/pass the RegistryEntry or route execution through construct so validation is centralized instead of duplicated in every constructor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/registry.rs, line 244:
<comment>Registry-level arity/type validation is test-only because production invokes `NodeConstructor` directly. Store/pass the `RegistryEntry` or route execution through `construct` so validation is centralized instead of duplicated in every constructor.</comment>
<file context>
@@ -55,239 +57,457 @@ pub enum RegistryValueSource {
- match dyn_any::downcast(input) {
- Ok(input) => Box::pin(output(*input)),
- Err(e) => panic!("DynAnyNode Input, {e} in:\n{node_name}"),
+pub fn construct(entry: &RegistryEntry, inputs: Vec<EdgeHandle>) -> Result<EdgeHandle, ConstructionError> {
+ if inputs.len() != entry.io.inputs.len() {
+ return Err(ConstructionError::Arity {
</file context>
| #[cfg(not(feature = "dealloc_nodes"))] | ||
| self.node | ||
| pub struct SharedEdge<N: ?Sized> { | ||
| ptr: std::ptr::NonNull<N>, |
Contributor
There was a problem hiding this comment.
P3: Every edge evaluation now depends on a manually maintained raw pointer although Arc<N> already forwards Node; retain only the Arc and delegate through it to remove unchecked dereferences and manual auto-trait invariants.
(Based on your team's feedback about using safe code before unsafe.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/registry.rs, line 106:
<comment>Every edge evaluation now depends on a manually maintained raw pointer although `Arc<N>` already forwards `Node`; retain only the `Arc` and delegate through it to remove unchecked dereferences and manual auto-trait invariants.
(Based on your team's feedback about using safe code before unsafe.) </comment>
<file context>
@@ -55,239 +57,457 @@ pub enum RegistryValueSource {
- #[cfg(not(feature = "dealloc_nodes"))]
- self.node
+pub struct SharedEdge<N: ?Sized> {
+ ptr: std::ptr::NonNull<N>,
+ own: std::sync::Arc<N>,
+}
</file context>
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
1 participant
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.
No description provided.