scheduled prompts: add/remove targets on edit (#474 fast-follow)
This commit is contained in:
parent
376b37fed7
commit
99bf4d635d
6 changed files with 216 additions and 3 deletions
|
|
@ -1509,6 +1509,17 @@ struct EditScheduleForm {
|
|||
interval_seconds: Option<Option<u64>>,
|
||||
#[serde(default)]
|
||||
next_fire_at_unix: Option<i64>,
|
||||
/// New targets to add. Replace-on-conflict: re-adding a
|
||||
/// previously-cancelled target drops the tombstone and the
|
||||
/// target starts fresh (operator intent on re-add = "this
|
||||
/// target is active again, fresh start").
|
||||
#[serde(default)]
|
||||
targets_add: Option<Vec<String>>,
|
||||
/// Targets to cancel. Same path as `cancel_targets`:
|
||||
/// tombstones preserve audit and the parent schedule
|
||||
/// auto-cancels when no active targets remain.
|
||||
#[serde(default)]
|
||||
targets_remove: Option<Vec<String>>,
|
||||
}
|
||||
|
||||
/// serde adaptor: turns missing-key into `None`, explicit-null
|
||||
|
|
@ -1543,6 +1554,8 @@ async fn patch_schedule(
|
|||
description: form.description,
|
||||
interval_seconds: form.interval_seconds,
|
||||
next_fire_at_unix: form.next_fire_at_unix,
|
||||
targets_add: form.targets_add,
|
||||
targets_remove: form.targets_remove,
|
||||
};
|
||||
if let Err(e) = state.coord.scheduled_prompts.update(id, patch) {
|
||||
return error_response(&format!("edit schedule {id}: {e:#}"));
|
||||
|
|
|
|||
|
|
@ -356,6 +356,8 @@ async fn dispatch(req: &ManagerRequest, coord: &Arc<Coordinator>) -> ManagerResp
|
|||
description,
|
||||
interval_seconds,
|
||||
next_fire_at_unix,
|
||||
targets_add,
|
||||
targets_remove,
|
||||
} => handle_edit_schedule(
|
||||
coord,
|
||||
hive_sh4re::MANAGER_AGENT,
|
||||
|
|
@ -364,6 +366,8 @@ async fn dispatch(req: &ManagerRequest, coord: &Arc<Coordinator>) -> ManagerResp
|
|||
description.clone(),
|
||||
*interval_seconds,
|
||||
*next_fire_at_unix,
|
||||
targets_add.clone(),
|
||||
targets_remove.clone(),
|
||||
),
|
||||
ManagerRequest::ListSchedules => match coord.scheduled_prompts.list() {
|
||||
Ok(schedules) => ManagerResponse::Schedules {
|
||||
|
|
@ -895,6 +899,8 @@ fn handle_edit_schedule(
|
|||
description: Option<Option<String>>,
|
||||
interval_seconds: Option<Option<u64>>,
|
||||
next_fire_at_unix: Option<i64>,
|
||||
targets_add: Option<Vec<String>>,
|
||||
targets_remove: Option<Vec<String>>,
|
||||
) -> ManagerResponse {
|
||||
let schedule = match coord.scheduled_prompts.get(schedule_id) {
|
||||
Ok(Some(s)) => s,
|
||||
|
|
@ -922,6 +928,8 @@ fn handle_edit_schedule(
|
|||
description,
|
||||
interval_seconds,
|
||||
next_fire_at_unix,
|
||||
targets_add,
|
||||
targets_remove,
|
||||
};
|
||||
match coord.scheduled_prompts.update(schedule_id, patch) {
|
||||
Ok(()) => ManagerResponse::Ok,
|
||||
|
|
|
|||
|
|
@ -153,12 +153,24 @@ pub struct NewSchedule {
|
|||
/// `Some(Some(n))` = "set to n seconds." Same encoding as JSON
|
||||
/// "missing key" vs "explicit null" — the dashboard surface
|
||||
/// preserves the distinction.
|
||||
///
|
||||
/// Target add/remove (#474 fast-follow): `targets_add` / `targets_remove`
|
||||
/// run inside the same transaction as the scalar field updates so
|
||||
/// "save my changes" is atomic. Remove delegates to the same
|
||||
/// cancel-targets path used by `cancel_targets` (tombstoning, preserves
|
||||
/// audit, auto-cancels parent when no active targets remain). Add uses
|
||||
/// `INSERT OR REPLACE` so re-adding a previously-cancelled target
|
||||
/// drops the tombstone and starts fresh (operator intent on re-add =
|
||||
/// "this target is active again"; prior history was already visible
|
||||
/// at cancel time).
|
||||
#[derive(Debug, Clone, Default)]
|
||||
pub struct UpdateSchedule {
|
||||
pub body: Option<String>,
|
||||
pub description: Option<Option<String>>,
|
||||
pub interval_seconds: Option<Option<u64>>,
|
||||
pub next_fire_at_unix: Option<i64>,
|
||||
pub targets_add: Option<Vec<String>>,
|
||||
pub targets_remove: Option<Vec<String>>,
|
||||
}
|
||||
|
||||
pub struct ScheduledPrompts {
|
||||
|
|
@ -404,6 +416,59 @@ impl ScheduledPrompts {
|
|||
params![next, id],
|
||||
)?;
|
||||
}
|
||||
// Target removals first, then adds. Order matters: an
|
||||
// operator who sends `{add: ["alice"], remove: ["alice"]}`
|
||||
// probably typo'd, but treating add-after-remove as the
|
||||
// surviving intent means a clean re-arm rather than an
|
||||
// orphaned tombstone. Plus removing-then-adding is the
|
||||
// natural way to "restart history" on one target without
|
||||
// a separate flow.
|
||||
let now = now_unix();
|
||||
if let Some(remove) = patch.targets_remove.as_deref() {
|
||||
for target in remove {
|
||||
tx.execute(
|
||||
"UPDATE scheduled_prompt_targets
|
||||
SET cancelled_at_unix = COALESCE(cancelled_at_unix, ?1)
|
||||
WHERE schedule_id = ?2 AND target = ?3",
|
||||
params![now, id, target],
|
||||
)?;
|
||||
}
|
||||
}
|
||||
if let Some(add) = patch.targets_add.as_deref() {
|
||||
for target in add {
|
||||
// INSERT OR REPLACE drops any prior row (cancelled
|
||||
// tombstone or otherwise) and inserts a fresh one
|
||||
// with empty history. Operator intent on re-add =
|
||||
// "this target is active again, fresh start."
|
||||
tx.execute(
|
||||
"INSERT OR REPLACE INTO scheduled_prompt_targets
|
||||
(schedule_id, target, cancelled_at_unix,
|
||||
last_fired_at_unix, last_result)
|
||||
VALUES (?1, ?2, NULL, NULL, NULL)",
|
||||
params![id, target],
|
||||
)?;
|
||||
}
|
||||
}
|
||||
// If the remove set drained the last active target (and
|
||||
// the add set didn't bring any back), auto-cancel the
|
||||
// parent so the worker stops scanning it. Mirrors
|
||||
// `cancel_targets`'s behaviour for the same edge case.
|
||||
if patch.targets_remove.is_some() || patch.targets_add.is_some() {
|
||||
let active_targets: i64 = tx.query_row(
|
||||
"SELECT COUNT(*) FROM scheduled_prompt_targets
|
||||
WHERE schedule_id = ?1 AND cancelled_at_unix IS NULL",
|
||||
params![id],
|
||||
|row| row.get(0),
|
||||
)?;
|
||||
if active_targets == 0 {
|
||||
tx.execute(
|
||||
"UPDATE scheduled_prompts
|
||||
SET cancelled_at_unix = COALESCE(cancelled_at_unix, ?1)
|
||||
WHERE id = ?2",
|
||||
params![now, id],
|
||||
)?;
|
||||
}
|
||||
}
|
||||
tx.commit()?;
|
||||
Ok(())
|
||||
}
|
||||
|
|
@ -846,6 +911,115 @@ mod tests {
|
|||
assert!(s.description.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_adds_new_targets() {
|
||||
let (_dir, db) = open();
|
||||
let id = submit_one_shot(&db, 100, &["alice"]);
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
targets_add: Some(vec!["bob".into(), "carol".into()]),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("update");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
let names: Vec<_> = s.targets.iter().map(|t| t.target.as_str()).collect();
|
||||
assert!(names.contains(&"alice"));
|
||||
assert!(names.contains(&"bob"));
|
||||
assert!(names.contains(&"carol"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_removes_targets_via_cancel_tombstone() {
|
||||
let (_dir, db) = open();
|
||||
let id = submit_one_shot(&db, 100, &["alice", "bob"]);
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
targets_remove: Some(vec!["alice".into()]),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("update");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
let alice = s.targets.iter().find(|t| t.target == "alice").unwrap();
|
||||
let bob = s.targets.iter().find(|t| t.target == "bob").unwrap();
|
||||
// alice tombstoned, bob still active, parent still active.
|
||||
assert!(alice.cancelled_at_unix.is_some());
|
||||
assert!(bob.cancelled_at_unix.is_none());
|
||||
assert!(s.cancelled_at_unix.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_removing_last_target_auto_cancels_parent() {
|
||||
let (_dir, db) = open();
|
||||
let id = submit_one_shot(&db, 100, &["alice"]);
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
targets_remove: Some(vec!["alice".into()]),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("update");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
assert!(s.cancelled_at_unix.is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_re_add_resurrects_cancelled_target_fresh() {
|
||||
let (_dir, db) = open();
|
||||
let id = submit_one_shot(&db, 100, &["alice", "bob"]);
|
||||
// Record some history on alice, then cancel her.
|
||||
db.record_target_result(id, "alice", 50, "ok").expect("record");
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
targets_remove: Some(vec!["alice".into()]),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("cancel alice");
|
||||
// Re-add alice — should drop the tombstone + prior history.
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
targets_add: Some(vec!["alice".into()]),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("re-add alice");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
let alice = s.targets.iter().find(|t| t.target == "alice").unwrap();
|
||||
assert!(alice.cancelled_at_unix.is_none(), "fresh row, no tombstone");
|
||||
assert!(alice.last_fired_at_unix.is_none(), "history reset");
|
||||
assert!(alice.last_result.is_none(), "history reset");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_add_and_remove_in_one_transaction() {
|
||||
let (_dir, db) = open();
|
||||
let id = submit_one_shot(&db, 100, &["alice"]);
|
||||
db.update(
|
||||
id,
|
||||
UpdateSchedule {
|
||||
targets_remove: Some(vec!["alice".into()]),
|
||||
targets_add: Some(vec!["bob".into()]),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("update");
|
||||
let s = db.get(id).expect("get").expect("present");
|
||||
let alice = s.targets.iter().find(|t| t.target == "alice").unwrap();
|
||||
let bob = s.targets.iter().find(|t| t.target == "bob").unwrap();
|
||||
assert!(alice.cancelled_at_unix.is_some());
|
||||
assert!(bob.cancelled_at_unix.is_none());
|
||||
// Bob arrived AFTER alice was tombstoned in the same tx —
|
||||
// active-target count > 0, so parent stays alive.
|
||||
assert!(s.cancelled_at_unix.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_source_round_trips() {
|
||||
let (_dir, db) = open();
|
||||
|
|
|
|||
Loading…
Reference in a new issue