-
Notifications
You must be signed in to change notification settings - Fork 0
feat(engine): make AddNode non-blocking with async node creation #286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0df4ddc
399c7b5
38d6ef6
b429367
e02edd8
a17b6cb
164d97e
6055fc6
6493d4f
3c7312d
6f0ec93
e7b5ee9
561d27c
5f877f3
3dbe16b
7b91d3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -519,6 +519,11 @@ async fn handle_add_node( | |
|
|
||
| { | ||
| let mut pipeline = session.pipeline.lock().await; | ||
| if pipeline.nodes.contains_key(&node_id) { | ||
| return Some(ResponsePayload::Error { | ||
| message: format!("Node '{node_id}' already exists in the pipeline"), | ||
| }); | ||
| } | ||
|
staging-devin-ai-integration[bot] marked this conversation as resolved.
|
||
| pipeline.nodes.insert( | ||
| node_id.clone(), | ||
| streamkit_api::Node { kind: kind.clone(), params: params.clone(), state: None }, | ||
|
|
@@ -1224,6 +1229,33 @@ async fn handle_apply_batch( | |
| }); | ||
| } | ||
|
|
||
| // Pre-validate duplicate node_ids against the pipeline model. | ||
| // Simulate the batch's Add/Remove sequence so that Remove→Add for | ||
| // the same ID within the batch is allowed, but duplicate Adds | ||
| // (without intervening Remove) are rejected before any mutation. | ||
| { | ||
| let pipeline = session.pipeline.lock().await; | ||
| let mut live_ids: std::collections::HashSet<&str> = | ||
| pipeline.nodes.keys().map(String::as_str).collect(); | ||
| for op in &operations { | ||
| match op { | ||
| streamkit_api::BatchOperation::AddNode { node_id, .. } => { | ||
| if !live_ids.insert(node_id.as_str()) { | ||
| return Some(ResponsePayload::Error { | ||
| message: format!( | ||
| "Batch rejected: node '{node_id}' already exists in the pipeline" | ||
| ), | ||
| }); | ||
| } | ||
| }, | ||
| streamkit_api::BatchOperation::RemoveNode { node_id } => { | ||
| live_ids.remove(node_id.as_str()); | ||
| }, | ||
| _ => {}, | ||
| } | ||
| } | ||
| } // Pipeline lock released after pre-validation | ||
|
|
||
| // Validate permissions for all operations | ||
| for op in &operations { | ||
| if let streamkit_api::BatchOperation::AddNode { kind, params, .. } = op { | ||
|
Comment on lines
1259
to
1261
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Missing oneshot and plugin allowlist checks in batch operations The (Refers to lines 1259-1320) Was this helpful? React with 👍 or 👎 to provide feedback. Debug
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged — the missing oneshot and plugin allowlist checks in the batch path are a pre-existing issue, not introduced by this PR. The batch validation loop at lines 1259-1320 was unchanged; I only added the duplicate node_id pre-check. Fixing the missing oneshot/allowlist checks in the batch path would be a good follow-up but is out of scope for this PR. |
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.