mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-18 16:02:10 +00:00
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.
This commit is contained in:
+22
@@ -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"
|
||||
}
|
||||
@@ -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>) -> 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<Vec<&'static str>> {
|
||||
if w_id.starts_with(WM_FORK_PREFIX) {
|
||||
fn fork_trigger_ignore_keys(is_fork: bool) -> Option<Vec<&'static str>> {
|
||||
if is_fork {
|
||||
Some(vec!["mode", "enabled"])
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
fn fork_schedule_ignore_keys(w_id: &str) -> Option<Vec<&'static str>> {
|
||||
if w_id.starts_with(WM_FORK_PREFIX) {
|
||||
fn fork_schedule_ignore_keys(is_fork: bool) -> Option<Vec<&'static str>> {
|
||||
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")]
|
||||
{
|
||||
|
||||
@@ -820,14 +820,16 @@ async fn set_trigger_mode<T: TriggerCrud>(
|
||||
|
||||
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?
|
||||
{
|
||||
|
||||
+10
-4
@@ -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:<kind>:<parent_workspace_id>
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user