mirror of
https://github.com/Start9Labs/start-os.git
synced 2026-03-26 02:11:53 +00:00
fix: RunAction task re-evaluation compared against partial input, not full config
Bug: After running an action (e.g. bitcoin's autoconfig), update_tasks was
called with the submitted form input — which for task-triggered actions is
filtered to only the task's fields (e.g. {zmqEnabled: true}). Other services'
tasks targeting the same action were then compared against this partial via
is_partial_of, so any task wanting a field NOT in the submission (e.g.
{blocknotify: "curl..."}) would incorrectly become active, even though the
full config still satisfied it.
This caused a cycling bug: running LND's autoconfig (zmqEnabled) would
activate Datum's task (blocknotify), and vice versa, despite the merge
correctly preserving both values in the config.
Fix: After running an action, fetch the full current config via
get_action_input (same as create_task and recheck_tasks already do) and
compare tasks against that.
The one-liner fix would have been to add a get_action_input call in the
RunAction handler. Instead, we extracted eval_action_tasks on
ServiceActorSeed — a single method that both RunAction and recheck_tasks
now call — because the duplication between these two sites is exactly how
this bug happened: recheck_tasks fetched the full config, RunAction didn't,
and they silently diverged.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -9,7 +9,7 @@ use crate::db::model::package::{
|
|||||||
};
|
};
|
||||||
use crate::prelude::*;
|
use crate::prelude::*;
|
||||||
use crate::rpc_continuations::Guid;
|
use crate::rpc_continuations::Guid;
|
||||||
use crate::service::{ProcedureName, Service, ServiceActor};
|
use crate::service::{ProcedureName, Service, ServiceActor, ServiceActorSeed};
|
||||||
use crate::util::actor::background::BackgroundJobQueue;
|
use crate::util::actor::background::BackgroundJobQueue;
|
||||||
use crate::util::actor::{ConflictBuilder, Handler};
|
use crate::util::actor::{ConflictBuilder, Handler};
|
||||||
use crate::util::serde::is_partial_of;
|
use crate::util::serde::is_partial_of;
|
||||||
@@ -131,6 +131,51 @@ pub fn update_tasks(
|
|||||||
critical_activated
|
critical_activated
|
||||||
}
|
}
|
||||||
|
|
||||||
|
impl ServiceActorSeed {
|
||||||
|
/// Fetch the current action input and re-evaluate all tasks targeting this action.
|
||||||
|
///
|
||||||
|
/// This is the single source of truth for task re-evaluation. Used both after
|
||||||
|
/// running an action and during service init (recheck_tasks).
|
||||||
|
pub(super) async fn eval_action_tasks(
|
||||||
|
&self,
|
||||||
|
action_id: &ActionId,
|
||||||
|
was_run: bool,
|
||||||
|
) -> Result<(), Error> {
|
||||||
|
let package_id = &self.id;
|
||||||
|
let current_input = self
|
||||||
|
.persistent_container
|
||||||
|
.execute::<Option<ActionInput>>(
|
||||||
|
Guid::new(),
|
||||||
|
ProcedureName::GetActionInput(action_id.clone()),
|
||||||
|
json!({ "prefill": Value::Null }),
|
||||||
|
Some(Duration::from_secs(30)),
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
.log_err()
|
||||||
|
.ok()
|
||||||
|
.flatten()
|
||||||
|
.and_then(|ai| ai.value);
|
||||||
|
let Some(input) = current_input else {
|
||||||
|
return Ok(());
|
||||||
|
};
|
||||||
|
self.ctx
|
||||||
|
.db
|
||||||
|
.mutate(|db| {
|
||||||
|
for (_, pde) in db.as_public_mut().as_package_data_mut().as_entries_mut()? {
|
||||||
|
if pde.as_tasks_mut().mutate(|tasks| {
|
||||||
|
Ok(update_tasks(tasks, package_id, action_id, &input, was_run))
|
||||||
|
})? {
|
||||||
|
pde.as_status_info_mut().stop()?;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Ok(())
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
.result?;
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
pub(super) struct RunAction {
|
pub(super) struct RunAction {
|
||||||
action_id: ActionId,
|
action_id: ActionId,
|
||||||
input: Value,
|
input: Value,
|
||||||
@@ -203,22 +248,7 @@ impl Handler<RunAction> for ServiceActor {
|
|||||||
)
|
)
|
||||||
.await
|
.await
|
||||||
.with_kind(ErrorKind::Action)?;
|
.with_kind(ErrorKind::Action)?;
|
||||||
let package_id = package_id.clone();
|
self.0.eval_action_tasks(action_id, true).await?;
|
||||||
self.0
|
|
||||||
.ctx
|
|
||||||
.db
|
|
||||||
.mutate(|db| {
|
|
||||||
for (_, pde) in db.as_public_mut().as_package_data_mut().as_entries_mut()? {
|
|
||||||
if pde.as_tasks_mut().mutate(|tasks| {
|
|
||||||
Ok(update_tasks(tasks, &package_id, action_id, &input, true))
|
|
||||||
})? {
|
|
||||||
pde.as_status_info_mut().stop()?;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
Ok(())
|
|
||||||
})
|
|
||||||
.await
|
|
||||||
.result?;
|
|
||||||
Ok(result)
|
Ok(result)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -39,7 +39,6 @@ use crate::lxc::ContainerId;
|
|||||||
use crate::prelude::*;
|
use crate::prelude::*;
|
||||||
use crate::rpc_continuations::{Guid, RpcContinuation};
|
use crate::rpc_continuations::{Guid, RpcContinuation};
|
||||||
use crate::s9pk::S9pk;
|
use crate::s9pk::S9pk;
|
||||||
use crate::service::action::update_tasks;
|
|
||||||
use crate::service::rpc::{ExitParams, InitKind};
|
use crate::service::rpc::{ExitParams, InitKind};
|
||||||
use crate::service::service_map::InstallProgressHandles;
|
use crate::service::service_map::InstallProgressHandles;
|
||||||
use crate::service::uninstall::cleanup;
|
use crate::service::uninstall::cleanup;
|
||||||
@@ -238,8 +237,7 @@ impl Service {
|
|||||||
async fn recheck_tasks(&self) -> Result<(), Error> {
|
async fn recheck_tasks(&self) -> Result<(), Error> {
|
||||||
let service_id = &self.seed.id;
|
let service_id = &self.seed.id;
|
||||||
let peek = self.seed.ctx.db.peek().await;
|
let peek = self.seed.ctx.db.peek().await;
|
||||||
let mut action_input: BTreeMap<ActionId, Value> = BTreeMap::new();
|
let action_ids: BTreeSet<_> = peek
|
||||||
let tasks: BTreeSet<_> = peek
|
|
||||||
.as_public()
|
.as_public()
|
||||||
.as_package_data()
|
.as_package_data()
|
||||||
.as_entries()?
|
.as_entries()?
|
||||||
@@ -266,29 +264,16 @@ impl Service {
|
|||||||
.flatten_ok()
|
.flatten_ok()
|
||||||
.map(|a| a.and_then(|a| a))
|
.map(|a| a.and_then(|a| a))
|
||||||
.try_collect()?;
|
.try_collect()?;
|
||||||
let procedure_id = Guid::new();
|
drop(peek);
|
||||||
for action_id in tasks {
|
for action_id in action_ids {
|
||||||
if let Some(input) = self
|
self.seed.eval_action_tasks(&action_id, false).await?;
|
||||||
.get_action_input(procedure_id.clone(), action_id.clone(), Value::Null)
|
|
||||||
.await
|
|
||||||
.log_err()
|
|
||||||
.flatten()
|
|
||||||
.and_then(|i| i.value)
|
|
||||||
{
|
|
||||||
action_input.insert(action_id, input);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
// Defensive sweep: stop any package that still has an active critical task.
|
||||||
|
// This catches tasks that were already active before this init cycle.
|
||||||
self.seed
|
self.seed
|
||||||
.ctx
|
.ctx
|
||||||
.db
|
.db
|
||||||
.mutate(|db| {
|
.mutate(|db| {
|
||||||
for (action_id, input) in &action_input {
|
|
||||||
for (_, pde) in db.as_public_mut().as_package_data_mut().as_entries_mut()? {
|
|
||||||
pde.as_tasks_mut().mutate(|tasks| {
|
|
||||||
Ok(update_tasks(tasks, service_id, action_id, input, false))
|
|
||||||
})?;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
for (_, pde) in db.as_public_mut().as_package_data_mut().as_entries_mut()? {
|
for (_, pde) in db.as_public_mut().as_package_data_mut().as_entries_mut()? {
|
||||||
if pde
|
if pde
|
||||||
.as_tasks()
|
.as_tasks()
|
||||||
|
|||||||
Reference in New Issue
Block a user