Skip to content

fix: ensure to remove tasks with errors from used deployments #548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jhonasiv merged 5 commits into from
Mar 13, 2026
Merged

fix: ensure to remove tasks with errors from used deployments #548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
71d9ba1
fix: ensure to remove tasks with errors from used deployments
jhonasiv Mar 10, 2026
06e3f55
fix: take used deployments directly from the plan
jhonasiv Mar 11, 2026
8fcd2c5
fix: fill deployed task for selected deployments
jhonasiv Mar 12, 2026
2f02ec7
fix: add propagate deployed task when merging a TaskContext
jhonasiv Mar 12, 2026
36225ce
chore: resolve rubocop and heisenbug on test order
jhonasiv Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions lib/syskit/network_generation/engine.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ def initialize(
event_logger: event_logger,
resolution_control: resolution_control
)
@used_deployments = {}
@required_instances = {}
end

Expand Down Expand Up @@ -128,13 +127,12 @@ def compute_deployed_network(
resolution_control: @resolution_control
)

used_deployments, = deployer.deploy(
deployer.deploy(
error_handler: error_handler, validate: validate_deployed_network,
lazy: lazy_deploy
)
@used_deployments = @used_deployments.merge(used_deployments)
resolution_errors = error_handler.process_failures(
required_instances, cleanup_failed_tasks: true
resolution_errors = process_failures(
error_handler, required_instances, cleanup_failed_tasks: true
)
# Sanity check that the plan was properly cleaned up
SystemNetworkDeployer.verify_all_tasks_deployed(
Expand Down Expand Up @@ -174,7 +172,7 @@ def apply_deployed_network_to_plan
@deployment_tasks, @deployed_tasks =
log_timepoint_group "finalize_deployed_tasks" do
adaptation = RuntimeNetworkAdaptation.new(
work_plan, @used_deployments,
work_plan,
merge_solver: @merge_solver,
event_logger: @event_logger,
resolution_control: @resolution_control
Expand Down Expand Up @@ -394,7 +392,7 @@ def compute_system_network(
instance_requirements
)

_, @used_deployments = system_network_generator.resolve_system_network(
system_network_generator.resolve_system_network(
garbage_collect: garbage_collect,
validate_abstract_network: validate_abstract_network,
validate_generated_network: validate_generated_network,
Expand All @@ -406,7 +404,8 @@ def compute_system_network(
toplevel_tasks_to_requirements =
system_network_generator.toplevel_tasks_to_requirements

resolution_errors = error_handler.process_failures(
resolution_errors = process_failures(
error_handler,
required_instances,
cleanup_failed_tasks: cleanup_resolution_errors
)
Expand All @@ -423,6 +422,16 @@ def compute_system_network(
[required_instances, resolution_errors, toplevel_tasks_to_requirements]
end

def process_failures(error_handler, required_instances, cleanup_failed_tasks:)
resolution_errors = error_handler.process_failures(required_instances)
return resolution_errors unless cleanup_failed_tasks

error_handler.cleanup_resolution_errors(
resolution_errors, required_instances, work_plan
)
resolution_errors
end

# Computes the system network, that is the network that fullfills
# a list of requirements
#
Expand Down
21 changes: 10 additions & 11 deletions lib/syskit/network_generation/resolution_error_handler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,11 @@ def failures_from_exception(tasks, plan, merge_solver, exception)
end
end

def process_failures(required_instances, cleanup_failed_tasks:)
def process_failures(required_instances)
requirement_tasks = required_instances.keys
toplevel_tasks = required_instances.values

resolution_errors = @resolution_failures.flat_map do |failure|
@resolution_failures.flat_map do |failure|
failed_task = failure.failed_task
indexes = find_index_of_toplevel_tasks_depending_on(
failed_task, toplevel_tasks, failure.plan, failure.merge_solver
Expand All @@ -165,14 +165,6 @@ def process_failures(required_instances, cleanup_failed_tasks:)
failure.to_resolution_errors(instance)
end
end

if cleanup_failed_tasks
cleanup_resolution_errors(
resolution_errors, required_instances, @plan
)
end

resolution_errors
end

# Cleanup the requirement tasks and toplevel tasks that encountered resolution
Expand All @@ -188,19 +180,24 @@ def cleanup_resolution_errors(
requirement_task = error.planning_task
required_instances.delete requirement_task
end
return if resolution_errors.empty?
return [] if resolution_errors.empty?

NetworkGeneration.debug "cleanup up after error resolution"
protected_tasks = required_instances.values.map do |v|
@merge_solver.replacement_for(v)
end

removed_tasks = []
work_plan
.static_garbage_collect(protected_roots: protected_tasks) do |obj|
NetworkGeneration.debug { " removing #{obj}" }
# Remove tasks that are not useful anymore
@plan.remove_task(obj)
removed_tasks << obj
end
@resolution_failures.clear

removed_tasks
end
end

Expand All @@ -215,6 +212,8 @@ def register_resolution_failures_from_exception(_tasks, exception)
def process_failures(*)
[]
end

def cleanup_resolution_errors(*); end
end
end
end
9 changes: 8 additions & 1 deletion lib/syskit/network_generation/runtime_network_adaptation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class RuntimeNetworkAdaptation
# @param [{Component=>DeploymentGroup::DeployedTask}] used_deployments the
# mapping from task instances to the deployment used for it
def initialize(
work_plan, used_deployments,
work_plan,
merge_solver:,
event_logger: work_plan.event_logger,
resolution_control: Async::Control.new
Expand All @@ -24,6 +24,7 @@ def initialize(
@resolution_control = resolution_control
@merge_solver = merge_solver

used_deployments = find_used_deployments(work_plan)
tasks_per_configured_deployments =
used_deployments.each_with_object({}) do |(task, deployed_task), h|
(h[deployed_task.configured_deployment] ||= []) << task
Expand All @@ -36,6 +37,12 @@ def initialize(
end
end

def find_used_deployments(work_plan)
work_plan.find_local_tasks(TaskContext).each_with_object({}) do |t, h|
h[t] = t.deployed_task
end
end

def apply
result = finalize_deployed_tasks
sever_old_plan_from_new_plan
Expand Down
7 changes: 7 additions & 0 deletions lib/syskit/network_generation/system_network_deployer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ def apply_selected_deployments(selected_deployments, deployment_tasks = {})
# subnet. This is not the goal here.
merge_solver.apply_merge_group(task => deployed_task)
used_deployments[deployed_task] = deployed_task_m
deployed_task.deployed_task = deployed_task_m

used_deployments.merge!(
apply_selected_deployments_discover_schedulers(
Expand All @@ -261,6 +262,7 @@ def apply_selected_deployments_discover_schedulers(
recursive = apply_selected_deployments_discover_schedulers(
scheduler_task, configured_deployment
)
scheduler_task.deployed_task = scheduler_deployed_task
{ scheduler_task => scheduler_deployed_task }.merge(recursive)
end

Expand All @@ -279,6 +281,10 @@ def lazy_apply_selected_deployments(selected_deployments)
task.orogen_model = sel.orogen_model
task.orogen_model.master
end
selected_deployments.each do |task, sel|
task.deployed_task = sel
end

return selected_deployments if with_master.empty?

used_deployments = selected_deployments.dup
Expand All @@ -294,6 +300,7 @@ def lazy_apply_selected_deployments(selected_deployments)
scheduler_deployed_task = Models::DeploymentGroup::DeployedTask.new(
deployment, scheduler_name
)
scheduler_task.deployed_task = task.deployed_task
used_deployments[scheduler_task] = scheduler_deployed_task
end
used_deployments
Expand Down
7 changes: 2 additions & 5 deletions lib/syskit/network_generation/system_network_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,6 @@ def resolve_system_network(
validate_deployed_network: true
)
deployment_tasks = {}
@used_deployments = {}
early_deploy(deployment_tasks)

merge_solver.merge_identical_tasks
Expand Down Expand Up @@ -324,15 +323,13 @@ def resolve_system_network(
)
interruption_point("syskit-netgen:validation")

@used_deployments.transform_keys! { @merge_solver.replacement_for(_1) }
[@toplevel_tasks, @used_deployments]
@toplevel_tasks
end

def early_deploy(deployment_tasks)
return unless early_deploy?

used_deployments, = deploy(deployment_tasks)
@used_deployments.merge!(used_deployments)
deploy(deployment_tasks)
interruption_point "syskit-netgen:early-deploy"
end

Expand Down
6 changes: 6 additions & 0 deletions lib/syskit/task_context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class TaskContext < Component
# The last state before we went to orogen_state
attr_reader :last_orogen_state

attr_accessor :deployed_task

# @api private
#
# Initialize the communication with the remote task
Expand Down Expand Up @@ -326,6 +328,10 @@ def merge(merged_task)
if merged_task.orocos_task && !orocos_task
self.orocos_task = merged_task.orocos_task
end

if merged_task.deployed_task && !deployed_task
self.deployed_task = merged_task.deployed_task
end
nil
end

Expand Down
58 changes: 57 additions & 1 deletion test/network_generation/test_engine.rb
View file
Open in desktop
Copy link

@wvmcastro wvmcastro Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a few months from now knowing what these tests are actually testing will be a puzzle. I think it is worth documenting what the are testing and how. Moreover, if it wouldn't be too painful to write, I would like to see a test that checks @used_deployments before and after error resolution and asserts that the desired deployments were removed

Copy link
Contributor Author

@jhonasiv jhonasiv Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they do test a typical behavior from #resolve. The issue before is that we assumed #resolve didnt need any tests due to being tested in higher level elsewhere, but this behavior actually had to be tested since it is specific to this set of parameters. I dont think they are specific to the errors we found, this code path is something that should be directly tested anyways.

Your suggestion is one that is very specific to this bug though, I dont agree that the unit tests should be this granular on this part of syskit.

Copy link
Member

@doudou doudou Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer the system test, not depending on the internal implementation (which we are fixing actually)

Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ def deploy_dev_and_bus
assert_master_slave_pattern_correct
end

it "deploys slave tasks when the deploymentc exists" do
it "deploys slave tasks when the deployment exists" do
deployment_task = @configured_deployment.new(plan: plan)

syskit_deploy(
Expand Down Expand Up @@ -693,6 +693,62 @@ def assert_master_slave_pattern_correct(deployment_task: nil)
assert_equal [t2.planning_task], required_instances.keys
end
end

describe "#resolve" do
it "handles deployment errors during final deployment resolution " \
"gracefully" do
task2_m = Syskit::TaskContext.new_submodel
task2_m.provides @srv_m, as: "srv"

t1 = @cmp_m.use("test" => @task_m.new(arg: 2),
"other" => task2_m.new).as_plan
plan.add(t1)

errors = syskit_engine.resolve(
requirement_tasks: [t1.planning_task],
capture_errors_during_network_resolution: true,
default_deployment_group: default_deployment_group,
cleanup_resolution_errors: true
)
assert_equal 1, errors.size
assert_kind_of MissingDeployment,
errors.first.original_exception
end

it "handles deployment errors during network adaption with bad new " \
"task" do
t1 = @task_m.with_arguments(arg: 1).as_plan
plan.add(t1)

t1 = t1.as_service
syskit_engine.resolve(
requirement_tasks: [t1.planning_task],
default_deployment_group: default_deployment_group,
capture_errors_during_network_resolution: true,
early_deploy: true,
cleanup_resolution_errors: true
)

new_engine = Syskit::NetworkGeneration::Engine.new(plan)
t2 = @task_m.with_arguments(arg: 2).as_plan
plan.add(t2)

errors = new_engine.resolve(
requirement_tasks: [t1.planning_task, t2.planning_task],
default_deployment_group: default_deployment_group,
capture_errors_during_network_resolution: true,
early_deploy: true,
cleanup_resolution_errors: true
)
# Both of the tasks will emit a ConflictingDeploymentAllocation
# with one another
assert_equal 2, errors.size
errors.each do |e|
assert_kind_of ConflictingDeploymentAllocation,
e.original_exception
end
end
end
end

describe "when scheduling tasks for reconfiguration" do
Expand Down
Loading