Split out of review feedback on #849 so that PR stays scoped to the LXC state-aware lifecycle slice.
Problem
The signal watchdog's DestroyContainer rollback removes the firewall before destroying the container, so a failed destroy leaves a one-shot container running with no egress filtering.
rollback_plan (src/backends/lxc/common/src/signal_cleanup.rs:100-105):
SignalRollback::DestroyContainer => {
if owns_firewall {
plan.push(RollbackStep::RemoveFirewall);
}
plan.push(RollbackStep::DestroyContainer);
}execute_rollback aborts the plan only on a failed StopContainer:
if !ok && step == RollbackStep::StopContainer {
return;
}So if RemoveFirewall succeeds and lxc-destroy -f then fails, the watchdog exits having unfiltered a container that is still up. The NetworkOnly plan deliberately avoids exactly this by stopping first.
The doc comment on execute_rollback currently asserts the state is unreachable -- "In a DestroyContainer plan nothing precedes RemoveFirewall, and DestroyContainer is last, so neither can be reached with a container left running behind it." That holds only if destroy always succeeds. It is the one step whose failure is not gated.
Why it is not fixed in #849
Reordering to destroy-then-remove inverts the rationale currently documented above rollback_plan, which chose the present order to remove host state "while the name is still unambiguous, before the container is gone." Changing it means re-deciding which hazard wins -- a leaked chain versus an unfiltered live container -- and updating the ordering contract, its comment, and the tests that pin the plan. That is a fail-behavior change to the signal path, wider than the lifecycle slice #849 lands.
Suggested fix
Push DestroyContainer first and RemoveFirewall only after it succeeds, gating on destroy the way the stop path gates on stop, so the rollback fails closed. Prefer leaking the chain over exposing a running container, matching the trade state_aware.rs stop already makes.
Found by Copilot review on #849 (comment 3780192957).
Split out of review feedback on #849 so that PR stays scoped to the LXC state-aware lifecycle slice.
Problem
The signal watchdog's
DestroyContainerrollback removes the firewall before destroying the container, so a failed destroy leaves a one-shot container running with no egress filtering.rollback_plan(src/backends/lxc/common/src/signal_cleanup.rs:100-105):execute_rollbackaborts the plan only on a failedStopContainer:So if
RemoveFirewallsucceeds andlxc-destroy -fthen fails, the watchdog exits having unfiltered a container that is still up. TheNetworkOnlyplan deliberately avoids exactly this by stopping first.The doc comment on
execute_rollbackcurrently asserts the state is unreachable -- "In aDestroyContainerplan nothing precedesRemoveFirewall, andDestroyContaineris last, so neither can be reached with a container left running behind it." That holds only if destroy always succeeds. It is the one step whose failure is not gated.Why it is not fixed in #849
Reordering to destroy-then-remove inverts the rationale currently documented above
rollback_plan, which chose the present order to remove host state "while the name is still unambiguous, before the container is gone." Changing it means re-deciding which hazard wins -- a leaked chain versus an unfiltered live container -- and updating the ordering contract, its comment, and the tests that pin the plan. That is a fail-behavior change to the signal path, wider than the lifecycle slice #849 lands.Suggested fix
Push
DestroyContainerfirst andRemoveFirewallonly after it succeeds, gating on destroy the way the stop path gates on stop, so the rollback fails closed. Prefer leaking the chain over exposing a running container, matching the tradestate_aware.rsstopalready makes.Found by Copilot review on #849 (comment 3780192957).