From 4dd38feefb65d130fc5c2364ae42cc582ad100cc Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 1 May 2026 17:46:35 +0200 Subject: [PATCH] fix(forks): include Suspended in conflict gate, use parent_workspace_id for fork detection Three fixes from the Claude review on PR #8976: - Suspended mode still attaches the listener (it just pauses auto-run of queued jobs); two suspended fork+parent listeners would still split Kafka events / share a PG slot. Gate set_trigger_mode on `mode != Disabled` instead of `mode == Enabled` so Suspended also surfaces the warning. - workspaces_export.rs::fork_*_ignore_keys keyed off the wm-fork-* prefix while set_trigger_mode and set_schedule_enabled key off parent_workspace_id. Switch the export filter to query parent_workspace_id once at the top of tarball_workspace and pass is_fork through. The column is the contract; the prefix is a creation-time naming convention that could in principle drift. - TriggerModeToggle's suspend-dropdown action reassigned the non-bindable `triggerMode` prop instead of the local `innerTriggerMode` mirror, leaking inconsistent state if the dispatch was cancelled. Now writes to innerTriggerMode like the Toggle's on:change handler does. --- ...0d65ffa5bfc5f10e8bd2a3162894c93283259.json | 22 ++++++++++++++++ backend/windmill-api/src/workspaces_export.rs | 26 ++++++++++++++----- backend/windmill-trigger/src/handler.rs | 18 +++++++------ docs/fork-triggers.md | 14 +++++++--- .../triggers/TriggerModeToggle.svelte | 7 ++++- 5 files changed, 67 insertions(+), 20 deletions(-) create mode 100644 backend/.sqlx/query-f57ff370f6775602d2c200d78650d65ffa5bfc5f10e8bd2a3162894c93283259.json diff --git a/backend/.sqlx/query-f57ff370f6775602d2c200d78650d65ffa5bfc5f10e8bd2a3162894c93283259.json b/backend/.sqlx/query-f57ff370f6775602d2c200d78650d65ffa5bfc5f10e8bd2a3162894c93283259.json new file mode 100644 index 0000000000..93f93b921d --- /dev/null +++ b/backend/.sqlx/query-f57ff370f6775602d2c200d78650d65ffa5bfc5f10e8bd2a3162894c93283259.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "?column?", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "f57ff370f6775602d2c200d78650d65ffa5bfc5f10e8bd2a3162894c93283259" +} diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index 886f83d9fc..c41397387f 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -45,7 +45,6 @@ use windmill_common::scripts::ScriptRunnableSettingsHandle; use windmill_common::utils::require_admin; use windmill_common::variables::decrypt; use windmill_common::worker::WINDMILL_DIR; -use windmill_common::workspaces::WM_FORK_PREFIX; use windmill_common::{ db::UserDB, error::{to_anyhow, Error, Result}, @@ -124,16 +123,16 @@ pub fn is_none_or_false(val: &Option) -> bool { /// source workspace is a fork. Stripping these keys avoids propagating /// fork-local operational state (enabled flag, runtime listener identifiers) /// back to the parent workspace through the git-sync round-trip. -fn fork_trigger_ignore_keys(w_id: &str) -> Option> { - if w_id.starts_with(WM_FORK_PREFIX) { +fn fork_trigger_ignore_keys(is_fork: bool) -> Option> { + if is_fork { Some(vec!["mode", "enabled"]) } else { None } } -fn fork_schedule_ignore_keys(w_id: &str) -> Option> { - if w_id.starts_with(WM_FORK_PREFIX) { +fn fork_schedule_ignore_keys(is_fork: bool) -> Option> { + if is_fork { Some(vec!["enabled"]) } else { None @@ -436,6 +435,19 @@ pub(crate) async fn tarball_workspace( let mut tx = user_db.begin(&authed).await?; + // Source-of-truth check for fork-ness: the workspace's parent_workspace_id + // column. The wm-fork-* prefix is a creation-time naming convention that + // could in principle drift (rename, manual SQL); the column is the + // contract that matches what the conflict-warning gates read. + let is_fork: bool = sqlx::query_scalar!( + "SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1", + &w_id + ) + .fetch_optional(&mut *tx) + .await? + .flatten() + .unwrap_or(false); + let tmp_dir = TempDir::new_in(&*WINDMILL_DIR)?; let name = match archive_type.as_deref() { @@ -703,7 +715,7 @@ pub(crate) async fn tarball_workspace( .fetch_all(&mut *tx) .await?; - let schedule_ignore_keys = fork_schedule_ignore_keys(&w_id); + let schedule_ignore_keys = fork_schedule_ignore_keys(is_fork); for schedule in schedules { let app_str = &to_string_without_metadata(&schedule, false, schedule_ignore_keys.clone()) @@ -734,7 +746,7 @@ pub(crate) async fn tarball_workspace( feature = "private" ) ))] - let trigger_ignore_keys = fork_trigger_ignore_keys(&w_id); + let trigger_ignore_keys = fork_trigger_ignore_keys(is_fork); #[cfg(feature = "http_trigger")] { diff --git a/backend/windmill-trigger/src/handler.rs b/backend/windmill-trigger/src/handler.rs index 3b06951808..855961cf5e 100644 --- a/backend/windmill-trigger/src/handler.rs +++ b/backend/windmill-trigger/src/handler.rs @@ -820,14 +820,16 @@ async fn set_trigger_mode( let mut tx = user_db.begin(&authed).await?; - // Block enabling a trigger in a fork when the parent has the same path - // (regardless of parent's mode), unless the caller passes force=true. - // Until Phase 3 ships namespacing of listener identifiers, the cloned - // upstream identifier is shared with the parent — two enabled listeners - // would split or steal events; one enabled vs. one disabled can still - // destructively claim shared state. Skipped for kinds where the upstream - // identifier is already workspace-scoped at runtime (HTTP, Email). - if T::FORK_CONFLICT_ON_ENABLE && payload.mode == TriggerMode::Enabled && !payload.force { + // Block transitioning a trigger in a fork to any mode that attaches a + // listener (Enabled or Suspended) when the parent has the same path, + // unless the caller passes force=true. Suspended still keeps the + // listener attached — it just stops auto-running queued jobs — so a + // suspended fork would still split Kafka events / share a PG slot + // with the parent. The cloned upstream identifier is shared by + // construction; the risk is independent of the parent's current mode. + // Skipped for kinds where the upstream identifier is already + // workspace-scoped at runtime (HTTP, Email). + if T::FORK_CONFLICT_ON_ENABLE && payload.mode != TriggerMode::Disabled && !payload.force { if let Some(parent_id) = parent_has_trigger(&mut *tx, T::TABLE_NAME, &workspace_id, path).await? { diff --git a/docs/fork-triggers.md b/docs/fork-triggers.md index 6f5fe3e629..393fa5c78d 100644 --- a/docs/fork-triggers.md +++ b/docs/fork-triggers.md @@ -70,10 +70,16 @@ update path is naturally safe. ## Conflict warning on enable -The `set_*_trigger_mode` and `set_schedule_enabled` endpoints check whether -the parent workspace has a row at the same trigger path — **regardless of -the parent's current `mode`/`enabled`**. If so, they reject the request with -an error string of the shape: +The `set_*_trigger_mode` endpoint fires the warning whenever a fork transitions +to a mode that *attaches a listener* — `Enabled` or `Suspended`. Suspended is +not "off": the listener still attaches and consumes events; only the auto-run +of queued jobs is paused. Two suspended forks would still split Kafka events +or share a Postgres slot with the parent. `Disabled` is the only mode that +fully detaches. + +The check fires whenever the parent workspace has a row at the same trigger +path — **regardless of the parent's current `mode`/`enabled`**. If so, the +endpoint rejects the request with an error string of the shape: ``` fork-conflict:: diff --git a/frontend/src/lib/components/triggers/TriggerModeToggle.svelte b/frontend/src/lib/components/triggers/TriggerModeToggle.svelte index afb873a83b..3cb0d2f559 100644 --- a/frontend/src/lib/components/triggers/TriggerModeToggle.svelte +++ b/frontend/src/lib/components/triggers/TriggerModeToggle.svelte @@ -113,7 +113,12 @@ displayName: 'Suspend job execution', icon: Pause, action: () => { - triggerMode = 'suspended' + // Optimistically flip the local mirror, not the + // non-bindable `triggerMode` prop. The parent will + // echo the new mode back via $effect on success; + // on cancel, our on:change reset path snaps it + // back to the prop value. + innerTriggerMode = 'suspended' onToggleMode?.('suspended') }, tooltip: