diff --git a/backend/windmill-api-schedule/src/lib.rs b/backend/windmill-api-schedule/src/lib.rs index e2d495eaad..29fa215800 100644 --- a/backend/windmill-api-schedule/src/lib.rs +++ b/backend/windmill-api-schedule/src/lib.rs @@ -242,23 +242,6 @@ async fn create_schedule( let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?; - // A git-sync/merge/create write into a fork never sets operational state: - // force `enabled = false` so a cloned / synced / merged / UI-created schedule - // can't fire alongside the parent's. The fork owner re-enables locally via - // `setenabled`. Schedule analog of the trigger rule in - // `windmill-trigger::handler::workspace_is_fork`; the read half (parent-value - // substitution on fork export) lives in `workspaces_export.rs`. Read fork-ness - // on the non-RLS `db` pool (like the other two sites) so the determination is - // complete regardless of the caller's folder perms. - let target_is_fork: bool = sqlx::query_scalar!( - "SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1", - w_id - ) - .fetch_optional(&db) - .await? - .flatten() - .unwrap_or(false); - // Check schedule for error ScheduleType::from_str(&ns.schedule, ns.cron_version.as_deref(), true)?; @@ -358,12 +341,7 @@ async fn create_schedule( // flows (CLI merge, UI merge, `wmill push` of a fork tarball) — which // either send the source's actual flag (create case) or omit `enabled` // entirely (update case, where `EditSchedule` lacks the field). - // A write into a fork always lands disabled regardless of the request. - if target_is_fork { - false - } else { - ns.enabled.unwrap_or(true) - }, + ns.enabled.unwrap_or(true), resolved_email, resolved_permissioned_as, ns.on_failure, @@ -435,7 +413,7 @@ async fn create_schedule( .await?; } - if !target_is_fork && ns.enabled.unwrap_or(true) { + if ns.enabled.unwrap_or(true) { tx = push_scheduled_job(&db, tx, &schedule, Some(&authed.clone().into()), None).await? } tx.commit().await?; diff --git a/backend/windmill-trigger/src/handler.rs b/backend/windmill-trigger/src/handler.rs index 3591c88931..82380d1afb 100644 --- a/backend/windmill-trigger/src/handler.rs +++ b/backend/windmill-trigger/src/handler.rs @@ -34,16 +34,15 @@ use windmill_git_sync::handle_deployment_metadata; /// True when the workspace is a fork (`parent_workspace_id IS NOT NULL`). /// -/// Operational state (`mode`) belongs to the parent workspace: a git-sync / -/// merge / clone / UI-create write into a fork must never set it. On create we -/// force `disabled` so a fork trigger can't compete with the parent's listener; -/// on update we preserve the fork's existing value. `setmode` is the intended -/// explicit mutator of a fork's mode (and carries its own conflict warning) — -/// runtime error handling may still auto-disable an errored trigger, which is -/// orthogonal to this rule. This is the write half whose read half lives in -/// `workspaces_export.rs` (parent-value substitution on fork export), and it is -/// the single authority shared by both the git-sync round-trip and the in-app -/// compare-workspaces merge. +/// On *update* into a fork, preserve the fork's existing `mode` instead of +/// writing the incoming value. This is the necessary complement to the export +/// read-half (`workspaces_export.rs`): that substitutes the *parent's* `mode` +/// into a fork's synced file, so a write-back of that file into the fork +/// (`wmill sync push`, compare-workspaces "Update current") would otherwise +/// flip the fork's trigger to the parent's (often enabled) state and re-create +/// the listener conflict. `setmode` remains the explicit mutator of a fork's +/// mode (with its own conflict warning); create deliberately does *not* force a +/// state, so a fork-only trigger lands in whatever state its creator chose. async fn workspace_is_fork(db: &DB, workspace_id: &str) -> Result { let is_fork: Option = sqlx::query_scalar("SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1") @@ -461,13 +460,6 @@ async fn create_trigger( let mut tx = user_db.begin(&authed).await?; - // Writing into a fork never sets operational state: force `disabled` so a - // cloned / synced / merged / UI-created trigger can't compete with the - // parent's listener. The fork owner re-enables locally via `setmode`. - if workspace_is_fork(&db, &workspace_id).await? { - new_trigger.base.set_mode(TriggerMode::Disabled); - } - let new_path = new_trigger.base.path.clone(); let labels = new_trigger.base.labels.clone(); diff --git a/docs/fork-triggers.md b/docs/fork-triggers.md index 308d21f1be..c3a7e892e9 100644 --- a/docs/fork-triggers.md +++ b/docs/fork-triggers.md @@ -84,16 +84,24 @@ parent had also edited it. Substituting the parent's value makes the fork's file byte-identical to the parent on that field, so the merge has nothing to resolve. -**Write half — fork writes never set operational state.** A write into a fork -(git-sync push, merge deploy, clone, or a plain UI create) must not set the -state, otherwise pulling the substituted parent value straight back into the -fork would re-enable it. So `create_trigger`/`create_schedule` force `disabled` -for a fork target, and `update_trigger` preserves the fork's existing `mode` -(`workspace_is_fork` in `windmill-trigger/src/handler.rs`; schedule `enabled` is -naturally preserved because `EditSchedule` has no `enabled` field). The same -handlers serve both merge paths, so the two can't diverge. The fork owner -re-enables locally via `setmode`/`setenabled` (which carry the conflict -warning below). +**Write half — an *update* into a fork preserves its existing state.** The +read-half writes the *parent's* (often enabled) value into a fork's synced +file. If that file is then written back into the fork — `wmill sync push`, +compare-workspaces "Update current", or any bidirectional sync — a naive update +would flip the fork's trigger to the parent's state and re-create the listener +conflict. So `update_trigger` preserves the fork's existing `mode` for a fork +target (`workspace_is_fork` in `windmill-trigger/src/handler.rs`); schedule +`enabled` is naturally preserved because `EditSchedule` has no `enabled` field. + +**Create is *not* forced.** A create into a fork keeps the caller's chosen +state, so creating a trigger/schedule in the fork UI works normally and a +fork-only row lands enabled if the user wants it. A fork-only row has no parent +counterpart, so it can't share an upstream identifier and can't conflict. (Rows +*cloned* from the parent at fork creation still start disabled — that's forced +in the SQL clone, `clone_triggers_and_schedules`, not in the create handler.) +The fork owner toggles state via `setmode`/`setenabled` (which carry the +conflict warning below) — those endpoints are unchanged, so the UI's enable/ +disable controls behave the same in a fork as anywhere else. For a non-fork target the incoming value is applied as given — so a fork→parent merge of an existing trigger writes the parent's own (substituted) value (a @@ -199,11 +207,13 @@ is owned by the parent* above) — the two paths share the backend `create`/ For a fork target the backend preserves it regardless (`workspace_is_fork`); for a parent target the `is_mode_unspecified()` safeguard does. Schedules also rely on `EditSchedule` lacking the `enabled` field. -- **Create**: the source's `mode`/`enabled` is passed through. Into a **parent** - there's no row to preserve, so a fork-only trigger/schedule lands with the - state the fork creator chose (omitting the flag defaults to `enabled`: - `BaseTriggerData::mode()` → `Enabled`, schedule insert → `true`). Into a - **fork** the backend forces `disabled` — a fork write never enables anything. +- **Create**: the source's `mode`/`enabled` is passed through (into a fork or a + parent alike) — there's no row to preserve, so the trigger/schedule lands with + the state the source chose (omitting the flag defaults to `enabled`: + `BaseTriggerData::mode()` → `Enabled`, schedule insert → `true`). Create is not + force-disabled for forks: a fork-only row has no parent counterpart and can't + conflict, and rows cloned from the parent already start disabled via the SQL + clone at fork creation. ## Future work — runtime listener suffix