[ee] Debounce node

[diegoimbert]

No description provided.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	3ec83d4
Status:	✅  Deploy successful!
Preview URL:	https://ad6936c3.windmill.pages.dev
Branch Preview URL:	https://branch-name-debounce-nodes.windmill.pages.dev

View logs

[diegoimbert]

/updatesqlx

[windmill-internal-app]

Starting sqlx update...

View workflow run

[windmill-internal-app]

❌ SQLx update failed. Please check the workflow logs for details.

[diegoimbert]

/updatesqlx

[windmill-internal-app]

Starting sqlx update...

View workflow run

[windmill-internal-app]

❌ SQLx update failed. Please check the workflow logs for details.

[diegoimbert]

/updatesqlx

[windmill-internal-app]

Starting sqlx update...

View workflow run

[windmill-internal-app]

❌ SQLx update failed. Please check the workflow logs for details.

[diegoimbert]

/ai review this pr

[claude]

Claude finished @diegoimbert's task in 2m 42s —— View job

---

Review of PR #8324: [ee] Debounce node

• Gather context and read changed files
• Review backend changes (Rust)
• Review frontend changes (Svelte)
• Review OpenAPI spec changes
• Review tests
• Post final review

---

Overview

This PR adds per-node (step-level) debouncing to flow modules, extending the existing flow-level debouncing feature. It adds a debouncing field to FlowModule, a new FlowModuleDebounce.svelte component for the UI, and backend logic in worker_flow.rs to call maybe_debounce_flow_node from the EE crate when a step is pushed.

Overall the feature is well-structured, the tests are thorough, and the code reuses existing patterns (e.g., DebouncingSettings, DebounceLimit component). Below are findings by area:

---

🔴 Potential Issues

1. Debounce runs inside parallel job loop without guard

backend/windmill-worker/src/worker_flow.rs ~L3685-3708

The debouncing code runs inside the for (i, payload_tag) in job_payloads.into_iter().enumerate() loop (line 3394). When job_payloads is ContinuePayload::ParallelJobs (parallel for-loops or branch-all), this debounce logic will execute for each parallel sub-job. The comment says "only for single-job steps, not parallel loops/branches" but there's no actual guard to enforce this.

This could cause unexpected behavior — multiple parallel sub-jobs of the same step would debounce against each other, canceling sibling jobs within the same flow run.

Suggestion: Add a guard like if len == 1 { ... } or check !matches!(next_status, NextStatus::AllFlowJobs { .. }) before applying debouncing:

#[cfg(feature = "private")]
if len == 1 {
    if let Some(ref debouncing) = module.debouncing {
        // ... debounce logic
    }
}

Fix this →

2. traverse_modules error silently swallowed in guard

backend/windmill-api-flows/src/flows.rs ~L1614-1616

if let Err(e) = check_result {
    tracing::warn!("Failed to traverse flow modules for debounce guard: {e}");
}

If traverse_modules fails (e.g., a module has invalid value JSON), the error is logged as a warning but the flow is still allowed to be saved. This means a flow with node-level debouncing could pass the guard even if the traversal couldn't fully inspect all modules. Consider returning the error instead:

check_result?;

3. Version guard reuses MIN_VERSION_SUPPORTS_DEBOUNCING_V2 for a new feature

backend/windmill-api-flows/src/flows.rs L1620

Node-level debouncing is a new capability, but the version guard checks MIN_VERSION_SUPPORTS_DEBOUNCING_V2 (version 1.597.0). If this feature ships in a version after 1.597.0, workers at exactly 1.597.0 would pass the guard but wouldn't have the maybe_debounce_flow_node code. Consider whether a new MIN_VERSION_SUPPORTS_NODE_DEBOUNCING constant is needed, or whether 1.597.0 is indeed the correct minimum.

---

🟡 Suggestions

4. traverse_modules could short-circuit

backend/windmill-api-flows/src/flows.rs ~L1607-1613

The traversal visits all modules even after finding the first one with debouncing. You could return early using Err as a control-flow mechanism or use traverse_modules with a mutable flag that breaks early:

let check_result = FlowModule::traverse_modules(&flow_value.modules, &mut |m| {
    if m.debouncing.as_ref().is_some_and(|d| d.debounce_delay_s.is_some_and(|s| s > 0)) {
        anyhow::bail!("has_node_debouncing");  // early exit
    }
    Ok(())
});
let has_node_debouncing = check_result.is_err();

This is minor since flows won't have thousands of modules, but it's a cleaner pattern.

5. Frontend: dual $effect pattern could cause stale writes

frontend/src/lib/components/flows/content/FlowModuleDebounce.svelte L24-47

The component has two $effect blocks — one to read from flowModule into local state (L24-31), and one to write local state back to flowModule (L35-47). This bidirectional sync pattern can be fragile with Svelte 5's fine-grained reactivity. If the write-back effect triggers before the read effect re-syncs after a module switch, stale values from the previous module could be written to the new module.

The flowModule.id dependency tracking on L26 helps mitigate this, but the ordering of effects isn't guaranteed. Consider using $derived for the read path or untrack in the write-back to be safer.

6. OpenAPI: debouncing property not marked as EE-only

openflow.openapi.yaml L329-351

The new debouncing property on FlowModule doesn't have any description indicating it's an enterprise feature. The backend code is gated behind #[cfg(feature = "private")], but the schema doesn't reflect this. Consider adding a note in the description like other EE features.

---

✅ What looks good

• Tests are comprehensive: 4 new test cases covering first job, second cancellation, default key generation, and counter tracking — good coverage of the core debouncing scenarios.
• Reuses existing DebouncingSettings struct and DebounceLimit UI component — clean reuse of existing patterns.
• OpenAPI type fix: Changing debounce_delay_s, max_total_debouncing_time, and max_total_debounces_amount from number to integer is a good correctness improvement.
• Guard function refactored from nested if-else to early returns — improves readability.
• All existing tests updated with the new debouncing: None field.

---

⚠️ Note on SQLx

The SQLx update has failed 3 times in this PR. The new .sqlx/ query files are committed but may need regeneration against the latest schema. This should be resolved before merge.

---

[claude]

Claude finished @diegoimbert's task in 5m 1s —— View job

---

Review of PR #8324: [ee] Debounce node

• Gather context and read changed files
• Review backend changes (Rust)
• Review frontend changes (Svelte)
• Review OpenAPI spec changes
• Review tests
• Post final review with reproduction instructions

---

Overview

This PR adds per-node (step-level) debouncing for flow modules, extending the existing flow-level debouncing feature. It includes:

• A debouncing: Option<DebouncingSettings> field on FlowModule (reusing the existing struct)
• Backend EE logic in worker_flow.rs calling maybe_debounce_flow_node
• A new FlowModuleDebounce.svelte component
• Version guard (MIN_VERSION_SUPPORTS_NODE_DEBOUNCING = 1.658.0)
• 4 new integration tests for flow node debouncing
• OpenAPI schema updates marked as EE only

The feature is well-structured and reuses existing patterns cleanly.

---

Findings

1. Debounce runs inside parallel job loop — intentional?

backend/windmill-worker/src/worker_flow.rs:3685-3708

The debounce code runs inside the for (i, payload_tag) in job_payloads.into_iter().enumerate() loop (L3394). When job_payloads is ContinuePayload::ParallelJobs, debouncing will fire for each parallel sub-job.

I see this was addressed and then intentionally reverted (commits 6f25470 → 9e0e459). The comment on L3685 says "only for single-job steps, not parallel loops/branches" but there's no guard enforcing this. If the intent is that debouncing shouldn't apply to parallel steps, the guard should exist. If the revert was intentional because the debounce key/module.id will naturally differ for each parallel branch, a comment clarifying this would help.

2. traverse_modules error silently swallowed

backend/windmill-api-flows/src/flows.rs:1618-1620

if let Err(e) = check_result {
    tracing::warn!("Failed to traverse flow modules for debounce guard: {e}");
}

If traversal fails (e.g., malformed module value JSON), the guard silently passes and the flow is saved. A flow with node-level debouncing could bypass the version check if traversal errors out before reaching the debounced module. Consider propagating the error:

check_result?;

Or at minimum, if the traversal fails AND node debouncing was detected before the failure, still reject.

Fix this →

3. Frontend: bidirectional $effect sync could cause stale writes on module switch

frontend/src/lib/components/flows/content/FlowModuleDebounce.svelte:24-47

Two $effect blocks — one reads from flowModule into local state (L24-31), one writes local state back to flowModule (L35-47). When switching between modules, the write-back effect could fire with stale values from the previous module before the read effect re-syncs. The flowModule.id tracking on L26 helps, but effect ordering is not guaranteed in Svelte 5.

This is a low-severity concern since the user would need to switch modules rapidly, but it's worth noting. A possible mitigation: use untrack in the write-back effect for the initial sync, or debounce the write-back.

4. traverse_modules doesn't short-circuit

backend/windmill-api-flows/src/flows.rs:1609-1617

Minor: the traversal visits all modules even after finding the first debounced one. Since flows are typically small this doesn't matter, but you could short-circuit by returning Err once found:

let has_node_debouncing = FlowModule::traverse_modules(&flow_value.modules, &mut |m| {
    if m.debouncing.as_ref().is_some_and(|d| d.debounce_delay_s.is_some_and(|s| s > 0)) {
        anyhow::bail!("has_node_debouncing");
    }
    Ok(())
}).is_err();

---

What looks good

• Tests are comprehensive: 4 new flow-node-specific tests covering first job, second cancellation, default key generation, and counter tracking.
• Clean reuse of DebouncingSettings struct and DebounceLimit UI component — the EE license gate on DebounceLimit inputs carries over naturally to the new component.
• Proper version guard with MIN_VERSION_SUPPORTS_NODE_DEBOUNCING at 1.658.0.
• OpenAPI marked as EE only (description: "Debounce configuration for this step (EE only)").
• Type fix: debounce_delay_s, max_total_debouncing_time, max_total_debounces_amount correctly changed from number to integer in the OpenAPI spec.

---

SQLx note

The SQLx update has failed 3 times. The .sqlx/ query files are committed but may need regeneration. This should be resolved before merge.

---

Reproduction instructions

To test node-level debouncing, navigate to the Flow Editor for any flow (create a new one or edit existing). Click on any step/node in the flow, then open the Advanced tab. A new Debounce tab should appear alongside Sleep, Suspend, Mock, etc. Click it to reveal the debounce configuration panel with fields for delay (seconds), custom debounce key, args to accumulate, max total debouncing time, and max debounces amount. These inputs should be disabled without an enterprise license. Configure a debounce delay (e.g., 5 seconds), save the flow, then trigger it twice rapidly — the first flow run's debounced step should be canceled in favor of the second run's step.

---