microduck/microduck
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #80 from pollen-robotics/apply-repairs-skew-rather-than-reporting-inert

Browse Source 
apply stops being inert when a daemon is not running what is installed
This commit is contained in:
Pierre Rouanet 2026-08-11 17:57:37 +02:00 committed by GitHub
commit 827f539819
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 289 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
docs/design/restart-order.md
View File

@ -98,6 +98,14 @@ Any failure from step 11 onward rolls back: swap `current` back, re-run `on_appl
previous release, confirm the boot counter, journal it. Steps 1–10 fail with the old release still
live and nothing to undo.
**Step 2 can also end it, and not always without restarting anything.** If the manifest names the
release already installed, the operation stops there and reports `already_current` — nothing is
downloaded and nothing is swapped. Before replying it does §5's reading over that release's units and
names in `stale` any that are running something else, and those are scheduled exactly as steps 16–18
schedule the deferred pair. So `apply` on an up-to-date robot is inert, and `apply` on a robot whose
daemons disagree with what is installed is not. `select` on the release a board already has active does
the same, for the same reason.
### Step 11 in detail — `hooks/postinstall`
The hook ships inside the signed artifact and runs with the release directory as its cwd. In order:
@ -145,8 +153,13 @@ Four details that are load-bearing:
restarting its own parent. A `systemd-run` timer lives outside it.
- **The update lock is dropped first** (step 16, before the spawn). A fork duplicates every open
descriptor, so spawning while holding the lock hands a copy to the child.
- **Only on `Applied`.** A rollback leaves the resident `updaterd` already matching `current`, so
restarting there would be churn with nothing to fix.
- **On `Applied`, and on `already_current` with stale units** — `engine.rs::restarts_owed` decides
which, and it is the only thing that decides it. The first owes the fixed pair; the second owes
whatever §2 found running the wrong release, which is usually `updaterd` itself. A rollback owes
nothing: it leaves the resident `updaterd` already matching `current`, so restarting there would be
churn with nothing to fix.
- **Every unit goes through the timer**, including ones an update would restart in place at step 12. A
second, immediate path for `configd` and `robotd` would buy five seconds and cost a mechanism.
- **Failures are logged, never returned.** An update that succeeded is not reported as failed because
a restart could not be scheduled; the cost of that is a daemon on an old binary until the next boot.
@ -302,8 +315,7 @@ Three deliberate non-actions:
- **`updaterd` never restarts itself here.** A successor that disagreed about which release is active
would restart, disagree again, and loop — in the one process that owns recovery, so nothing would be
left to break the cycle. It is safe to only report: the process has just started, so anything stale
about it was decided before this code ran. This is the one skew that still needs a reboot or a
manual `systemctl restart updaterd`.
about it was decided before this code ran. It is repaired from the other side instead — see below.
- **A stopped unit is left stopped.** No identity file means either stopped or too old to publish, and
both read the same here. Starting it would override, on every `updaterd` start, whoever stopped it.
- **A daemon that published nothing is not treated as stale.** Restarting a robot's daemons for being
@ -315,6 +327,24 @@ release left *ahead* of `current` by an explicit rollback, not only one left beh
It runs on every `updaterd` start, so at boot it is a no-op: everything started from the same symlink.
The cost is one file read per unit.
### The same reading, from `apply`
`reconcile::stale_units` is `verdict_for` over the same identity files with the acting left out, and
`apply` and `select` call it on their already-current paths (§2). It exists because of the exception
above: a stale `updaterd` is the one skew this module refuses to repair, and the only thing that ever
looks at it afterwards is a person running `apply` because a daemon seems wrong. That used to answer
`already_current` and schedule nothing, which read as confirmation that there was nothing to fix.
Two properties keep it from being the loop §5 guards against. It fires on a request rather than on
every start, and it schedules through `systemd-run` rather than driving `systemctl` from inside the
process being restarted. It reports as well as acts: the units are named in the `already_current`
outcome, so a client — `robotctl`, or the app — sees which daemons were not running the release the
robot has installed.
Both stale verdicts count as stale here (`Restarted` and `ReportedOnly`); the difference between them
is about who may perform the restart, not about what is wrong. Everything else the table says is
unchanged: a stopped unit and a daemon that published nothing are still not stale.
## 6. First install
`scripts/install.sh` on a bare board, in order (`main`):
@ -349,7 +379,7 @@ Two version numbers are legitimately different at once, and which pair it is dec
|---|---|
| `updaterd` or `btd` behind the installed release, for a few seconds after an update | expected — the deferred restart has not fired yet |
| `btd`, `robotd`, `configd` or `padd` disagreeing with `current` at all, persistently | the restart did not take effect *and* §5 did not fix it — so either `updaterd` has not restarted since, or its restart failed (journal, at `error`) |
| `updaterd` behind it, persistently | the deferred restart never landed, and §5 will not fix this one. `systemctl restart updaterd`, then read the journal for why |
| `updaterd` behind it, persistently | the deferred restart never landed, and §5 will not fix this one. `robotctl update apply daemon` repairs it — it reports `already_current` with `updaterd` in `stale` and schedules the restart. `systemctl restart updaterd` is the same fix by hand; either way the journal has why it did not land |
| any daemon reporting `build unknown (old)` | it predates the identity mechanism, so §5 leaves it alone. One update makes it answerable |
Ask the robot rather than inferring:

32
docs/project/install-path-gap.md
View File

@ -15,7 +15,8 @@ their own document because anyone investigating one will arrive believing it is
case 1's `serde(default)` discipline landed, case 2's handshake proposal was decided *against* with
its reasoning written down, and the "units outlive the release that installed them" consequence is
now a refusal rather than emergent behaviour. What is still open is the *testing* gap the title
names — no test installs a real artifact — plus three smaller decisions marked below.
names — no test installs a real artifact — plus the two smaller items marked below, both of which
are that same gap in a specific form.
One change since the first draft moves enough of this document to call out here: `updaterd` and
`btd` are now restarted a few seconds *after* the update replies (`RESTART_AFTER_REPLYING`), and the
@ -116,16 +117,25 @@ with what its component has active, and anything stale is restarted. Since `upda
five seconds after every applied update, that check runs seconds after every `apply`, so the state
this section is about now heals itself with `Engine::apply` unchanged.
What that leaves is narrower than either option as written. `Engine::apply` still returns
`AlreadyCurrent` on the installed version alone, and `reconcile` deliberately does not repair one
skew: `updaterd`'s own, because a self-restart loop in the process that owns recovery is the one
failure with no way out. Those two meet in a single case — a stale `updaterd`, an operator reaching
for `apply`, `already_current`, no restart scheduled, and nothing to fix it but a hand-run `systemctl
restart updaterd`. **Still open**, as one narrow change to `apply` rather than as two options; a
`--force` flag is what is left if `apply` is not to repair skew by itself. If it is added, its guard
needs its own reasoning: `install --force` refuses while `robotd` answers because it disables the
health gate, and `apply` keeps that gate, so copying the guard by symmetry would disable the flag
exactly when a robot is up and skewed.
What that left was narrower than either option as written, and it is now closed too. `reconcile`
deliberately does not repair one skew — `updaterd`'s own, because a self-restart loop in the process
that owns recovery is the one failure with no way out — and that met an `apply` returning
`AlreadyCurrent` on the installed version alone in a single case: a stale `updaterd`, an operator
reaching for `apply`, `already_current`, no restart scheduled, and nothing to fix it but a hand-run
`systemctl restart updaterd`.
**Fixed** (`engine.rs::restarts_owed`, `reconcile::stale_units`). `apply` and `select` read the same
identity files on their already-current paths, name the units running something else in the
`already_current` outcome, and schedule those restarts the way an update schedules its deferred pair.
It is not the loop the startup check guards against: it fires once, on a request, and through
`systemd-run` rather than from inside the process being restarted.
**And `apply --force` is dropped rather than deferred.** Its whole job was to re-run the restart on an
already-current release; that is now what `apply` does when — and only when — something is actually
stale. A flag would add a second way to ask for it, gated by a question that took a paragraph to get
right: `install --force` refuses while `robotd` answers because it disables the health gate, `apply`
keeps that gate, so copying the guard by symmetry would have disabled the flag exactly when a robot is
up and skewed. Nothing needs to answer that now.
**Fixed, and this paragraph used to say otherwise.** `btd` is excluded from the *in-flight* restart
because restarting it drops the BLE connection carrying the update's own progress stream — that part

11
docs/robot/cheatsheet-dev.md
View File

@ -62,12 +62,15 @@ filter's only opt-in, it applies to the one command, and it leaves nothing switc
after it replies** — the first cannot restart itself mid-update, and the second may be carrying the
reply. So a `btd` fix is live a few seconds later, with no manual step. Reconnect and it is there.
- **If one of those two restarts does not happen, the next `updaterd` start fixes it.** Except
`updaterd` itself, which reports the disagreement rather than restarting itself — that one is a
`systemctl restart updaterd` by hand.
`updaterd` itself, which reports the disagreement rather than restarting itself. Run the apply again
for that one: it answers `already_current`, names the daemon that is not running it, and schedules
the restart. `sudo systemctl restart updaterd` does the same by hand.
- **A board running an `updaterd` older than 0.4.0 has none of that** and keeps both on the old binary
until you restart them. One update fixes it, and only the update after that behaves.
- **`robotctl update apply` reports `already_current` and does nothing** if you try to reinstall the
same version, so it is not the command to reach for when a fix looks absent.
- **`robotctl update apply` reports `already_current` and installs nothing** if you ask for the version
a board already has — but it is no longer inert. It checks which daemons are running that release and
restarts the ones that are not, naming them in `stale`. So it *is* the command to reach for when a
fix looks absent: either it fixes it, or `stale` is empty and the fix was never in that release.
The symptom is a fix that is definitely installed and definitely not working. Ask which release each
daemon is running:

17
duck-ipc-proto/src/lib.rs
View File

@ -1166,11 +1166,22 @@ pub enum ApplyResult {
},
AlreadyCurrent {
version: semver::Version,
/// Units running something other than `version`, which this outcome has scheduled a restart
/// for. Empty in the ordinary case, where nothing was installed because nothing needed to be.
///
/// It earns a field rather than only a log line because "already current" is otherwise
/// indistinguishable from "already current, and a daemon is not running it" — the state that
/// made a recovery command look like a confirmation that there was nothing to recover.
///
/// `default`, so an older `updaterd`'s reply still parses: it reports no stale units because
/// it did not look, which reads the same as finding none. `ApplyResult` does not
/// `deny_unknown_fields`, so an older client ignores the field rather than failing to decode
/// the outcome of an update it just performed.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
stale: Vec<String>,
},
/// Everything verified; stopped before the swap because `dry_run` was set.
DryRunPassed {
candidate: semver::Version,
},
DryRunPassed { candidate: semver::Version },
/// Applied, failed its gate, reverted. The robot is on `reverted_to`.
RolledBack {
attempted: semver::Version,

186
updater/src/engine.rs
View File

@ -364,7 +364,7 @@ impl Engine {
// parallel, copies of *other* engines' locks too. It surfaced as unrelated operations
// failing with `Busy`. Nothing below this point touches the store.
drop(lock);
schedule_restarts_if_applied(self.deferred_restarts, &outcome).await;
schedule_restarts_if_needed(self.deferred_restarts, &outcome).await;
outcome
}
@ -413,8 +413,15 @@ impl Engine {
}
if Some(&manifest.version) == installed.as_ref() {
// Correct, and for years the whole answer. It is the wrong *question* in one case: the
// release is installed and a daemon is serving from a different one. That is what an
// operator reaching for `apply` is usually trying to fix, and answering "already current"
// told them there was nothing to fix. The units are named here and restarted after the
// reply — see `restarts_owed`.
let stale = self.stale_units(&manifest.version, cfg, store);
return Ok(ApplyResult::AlreadyCurrent {
version: manifest.version,
stale,
});
}
@ -908,8 +915,11 @@ impl Engine {
let current = store.current()?;
if current.as_ref() == Some(version) {
// Same repair as `apply`'s, for the same reason: selecting the release a board already has
// active is the other command an operator reaches for when a daemon looks wrong.
return Ok(ApplyResult::AlreadyCurrent {
version: version.clone(),
stale: self.stale_units(version, &cfg, &store),
});
}
@ -919,7 +929,7 @@ impl Engine {
.transition_to(component, &cfg, &store, version, current)
.await;
drop(lock);
schedule_restarts_if_applied(self.deferred_restarts, &outcome).await;
schedule_restarts_if_needed(self.deferred_restarts, &outcome).await;
outcome
}
@ -1121,6 +1131,22 @@ impl Engine {
/// [`MAX_BOOT_ATTEMPTS`] boots, and delete staging leftovers. This is the path
/// that catches a release which doesn't start at all — the in-process health
/// gate can't, because it died with it.
/// The units of one component that are not running the release named, and so are owed a restart.
///
/// The same reading [`Self::reconcile_running_units`] does at startup, for one component and
/// without acting: the acting happens after the reply is on the wire, because the units this most
/// often names are the two that cannot be restarted before it. Called on the already-current
/// paths only, which is where the answer changes what the operation reports.
fn stale_units(
&self,
version: &semver::Version,
cfg: &ComponentConfig,
store: &Store,
) -> Vec<String> {
let units = units_shipped(&store.release_dir(version), &configured_units(cfg));
crate::reconcile::stale_units(version, &units)
}
/// Check that the restarts an update scheduled actually happened, and fix what did not.
///
/// Runs at startup, beside [`Self::recover_on_start`], because that is the first moment the
@ -1144,13 +1170,12 @@ impl Engine {
continue;
};
let configured = match &self.config.components[name].on_apply {
ApplyAction::Restart { units } => units.clone(),
_ => Vec::new(),
};
// The *shipped* set rather than the restart set: `updaterd` and `btd` are excluded from
// an update's own restarts, which makes them the two this check exists for.
let units = units_shipped(&store.release_dir(&active), &configured);
let units = units_shipped(
&store.release_dir(&active),
&configured_units(&self.config.components[name]),
);
if units.is_empty() {
continue;
}
@ -1775,20 +1800,55 @@ async fn self_test_updaterd(release_dir: &Path) -> Result<(), Error> {
Err(Error::SelfTest(format!("{}: {reason}", output.status)))
}
/// Schedule the deferred restarts when an operation actually moved to a new release.
/// Which units an outcome leaves owing a restart.
///
/// Only on `Applied`. A rollback leaves the resident `updaterd` already matching `current` — it was
/// never restarted, so it is still the binary belonging to the release being returned to — and
/// restarting there would be churn with nothing to fix.
async fn schedule_restarts_if_applied(enabled: bool, outcome: &Result<ApplyResult, Error>) {
/// Two cases, and they are one act for two reasons.
///
/// **`Applied`** owes the pair a running update cannot restart in place: itself, and the transport the
/// reply may be travelling over.
///
/// **`AlreadyCurrent` with stale units** owes exactly those. Nothing was installed because nothing
/// needed to be, and a daemon is still running something else — the state
/// [`crate::reconcile`] repairs at every start, except for the one unit it refuses to repair. A stale
/// `updaterd` will not restart itself from its own startup path, so the only thing that ever looks at
/// it is an operator running `apply`, who until now got `already_current`, no restart, and a robot
/// still on the old binary. Repairing it here is not the loop the startup check guards against: it
/// fires once, on a request, rather than on every start.
///
/// A rollback owes nothing, and that is not an omission: it leaves the resident `updaterd` already
/// matching `current` — it was never restarted, so it is still the binary belonging to the release
/// being returned to — and restarting there would be churn with nothing to fix.
///
/// Pure, and separated from the scheduling below for the reason `reconcile::verdict_for` is: which
/// outcomes owe what is the part that can be wrong, and arranging each of them on a board costs an
/// afternoon apiece.
fn restarts_owed(outcome: &Result<ApplyResult, Error>) -> Vec<&str> {
match outcome {
Ok(ApplyResult::Applied { .. }) => RESTART_AFTER_REPLYING.to_vec(),
Ok(ApplyResult::AlreadyCurrent { stale, .. }) => stale.iter().map(String::as_str).collect(),
_ => Vec::new(),
}
}
/// Schedule what [`restarts_owed`] says the outcome owes.
async fn schedule_restarts_if_needed(enabled: bool, outcome: &Result<ApplyResult, Error>) {
if !enabled {
// A test binary. See `Engine::without_deferred_restarts` for why this is about forking.
tracing::debug!("deferred restarts suppressed");
return;
}
if matches!(outcome, Ok(ApplyResult::Applied { .. })) {
schedule_deferred_restarts(SYSTEMD_RUN).await;
let units = restarts_owed(outcome);
if units.is_empty() {
return;
}
if let Ok(ApplyResult::AlreadyCurrent { version, .. }) = outcome {
tracing::warn!(
%version,
units = units.join(","),
"this release is already installed, and these are not running it; restarting them"
);
}
schedule_deferred_restarts(SYSTEMD_RUN, &units).await;
}
/// The program that schedules the deferred restarts.
@ -1811,8 +1871,15 @@ const SYSTEMD_RUN: &str = "systemd-run";
/// update that succeeded must not be reported as failed because a restart could not be scheduled.
/// Swallowing them is affordable because they are not the last word: [`crate::reconcile`] checks at
/// the next start that each unit is on the active release and restarts what is not.
async fn schedule_deferred_restarts(systemd_run: &str) {
for unit in RESTART_AFTER_REPLYING {
///
/// The units are a parameter because two callers owe different ones — see [`restarts_owed`] — and
/// because every unit that reaches here goes through the transient timer, including the ones an update
/// restarts in place. A second, immediate path for `robotd` and `configd` would buy five seconds and
/// cost a mechanism. Nor does this need [`restart_one`]'s absent-unit handling: a unit is named here
/// either because the release ships it or because it published an identity, and a process that
/// published one is running.
async fn schedule_deferred_restarts(systemd_run: &str, units: &[&str]) {
for unit in units.iter().copied() {
let mut command = tokio::process::Command::new(systemd_run);
command
.arg(format!("--on-active={DEFERRED_RESTART_DELAY}"))
@ -1863,9 +1930,11 @@ const NEVER_RESTART: [&str; 2] = ["updaterd", "btd"];
/// `on_apply`'s list lives in the operator's `/etc/robot/updater.toml`, and `install.sh` preserves
/// that file — so a board provisioned before a daemon existed keeps a list that does not mention it,
/// and every release swaps that daemon's binary while leaving the old process running. The update
/// reports success, the daemon answers on stale code, and `apply` then says `already_current` and
/// does nothing. Four correct fixes were diagnosed as broken that way in one afternoon; see
/// `docs/project/install-path-gap.md` §4.
/// reports success, the daemon answers on stale code, and `apply` — as it then was — said
/// `already_current` and did nothing, so the obvious recovery command confirmed there was nothing to
/// recover. Four correct fixes were diagnosed as broken that way in one afternoon; see
/// `docs/project/install-path-gap.md` §4. Both halves are closed now: this function is the first, and
/// [`restarts_owed`] is the second.
///
/// A release already states which units it provides — it ships them in `systemd/` — so it can say
/// which to restart. Same realisation that made `hooks/postinstall` the right place to *install*
@ -1877,6 +1946,17 @@ const NEVER_RESTART: [&str; 2] = ["updaterd", "btd"];
///
/// An unreadable or absent `systemd/` directory yields the configured list unchanged. Older releases
/// predate the directory, and a rollback to one must still work.
/// The units a board's config names for this component, if it names any.
///
/// Shared by the three callers that need it rather than matched inline at each, so a component whose
/// `on_apply` is not a restart cannot be read as naming units in one place and not another.
fn configured_units(cfg: &ComponentConfig) -> Vec<String> {
match &cfg.on_apply {
ApplyAction::Restart { units } => units.clone(),
_ => Vec::new(),
}
}
fn units_shipped(release_dir: &Path, configured: &[String]) -> Vec<String> {
let mut units: Vec<String> = Vec::new();
@ -2242,7 +2322,7 @@ esac
let dir = tempfile::tempdir().unwrap();
let systemd_run = stub_recorder(dir.path(), "systemd-run");
schedule_deferred_restarts(systemd_run.to_str().unwrap()).await;
schedule_deferred_restarts(systemd_run.to_str().unwrap(), &RESTART_AFTER_REPLYING).await;
let log = calls(dir.path());
let lines: Vec<&str> = log.lines().collect();
@ -2272,6 +2352,66 @@ esac
assert!(log.contains("restart btd"), "{log}");
}
/// A stale unit reported by an already-current apply is scheduled by name, and only it.
///
/// The pair an `Applied` owes is fixed; this list is not, so the two cases cannot share one
/// assertion. `configd` here rather than `updaterd` for a reason that is not arbitrary: it proves
/// the scheduler is driven by the outcome's list and not by `RESTART_AFTER_REPLYING`, which is the
/// mistake this generalisation makes possible.
#[tokio::test]
async fn the_stale_units_are_the_ones_scheduled() {
let dir = tempfile::tempdir().unwrap();
let systemd_run = stub_recorder(dir.path(), "systemd-run");
schedule_deferred_restarts(systemd_run.to_str().unwrap(), &["configd"]).await;
let log = calls(dir.path());
assert!(log.ends_with("-- systemctl restart configd\n"), "{log}");
assert_eq!(log.lines().count(), 1, "{log}");
}
fn already_current(stale: &[&str]) -> Result<ApplyResult, Error> {
Ok(ApplyResult::AlreadyCurrent {
version: semver::Version::parse("0.4.0").expect("a test version"),
stale: stale.iter().map(|u| (*u).to_owned()).collect(),
})
}
/// What each outcome owes, which is the part a board cannot be made to demonstrate.
///
/// The third case is the one this exists for, and it is the whole reason `apply` stopped being
/// inert: a stale `updaterd` is the skew `reconcile` refuses to repair, so an operator running
/// `apply` is the only thing that ever reaches it.
#[test]
fn which_outcomes_owe_a_restart() {
let applied = Ok(ApplyResult::Applied {
from: None,
to: semver::Version::parse("0.4.0").expect("a test version"),
});
assert_eq!(restarts_owed(&applied), RESTART_AFTER_REPLYING.to_vec());
// Nothing installed, nothing skewed: the ordinary already-current, and it must stay silent.
// Restarting a healthy robot's daemons because someone asked for a release it already has
// would be a worse command than the inert one.
assert!(restarts_owed(&already_current(&[])).is_empty());
assert_eq!(restarts_owed(&already_current(&["updaterd"])), ["updaterd"]);
assert_eq!(
restarts_owed(&already_current(&["configd", "updaterd"])),
["configd", "updaterd"]
);
// A rollback leaves the resident `updaterd` already matching `current`, so there is nothing
// to fix and a restart would be churn.
let rolled_back = Ok(ApplyResult::RolledBack {
attempted: semver::Version::parse("0.5.0").expect("a test version"),
reverted_to: None,
reason: "the gate".to_owned(),
});
assert!(restarts_owed(&rolled_back).is_empty());
assert!(restarts_owed(&Err(Error::Busy)).is_empty());
}
/// Scheduling that fails must not propagate. The update is already committed and journalled by
/// this point, so an unschedulable restart cannot be allowed to report a good update as failed —
/// `reconcile` picks it up at the next start instead.
@ -2279,7 +2419,11 @@ esac
async fn a_scheduler_that_cannot_be_run_is_not_an_error() {
let dir = tempfile::tempdir().unwrap();
// Nothing at this path, so the spawn fails outright.
schedule_deferred_restarts(dir.path().join("absent").to_str().unwrap()).await;
schedule_deferred_restarts(
dir.path().join("absent").to_str().unwrap(),
&RESTART_AFTER_REPLYING,
)
.await;
}
/// One invocation per unit, which is the fix. `systemctl restart a b` fails as a whole when

4
updater/src/main.rs
View File

@ -475,7 +475,9 @@ async fn install(
}
// Unreachable given the store guard above, but reporting it as success would be
// wrong if that guard ever moves.
Ok(ApplyResult::AlreadyCurrent { version }) => {
// `stale` is ignored rather than reported: this is a bootstrap install, before anything is
// serving, so there is no running daemon for it to disagree with.
Ok(ApplyResult::AlreadyCurrent { version, .. }) => {
tracing::warn!(component, version = %version, "already current");
ExitCode::SUCCESS
}

45
updater/src/reconcile.rs
View File

@ -38,6 +38,12 @@
//! disagreed about what "current" is, and a loop in the process that owns recovery is the one
//! failure with no way out. It is reported and left alone, which is safe: it has just started, so
//! anything stale about it was decided before this code ran.
//!
//! That exception used to be the end of the story, and it left one skew nothing repaired. It is now
//! reached from the other side: [`stale_units`] is the same reading without the acting, and
//! `Engine::apply` calls it when a release turns out to be already installed — so an operator running
//! `apply` on a robot that looks wrong schedules the restart this module refuses to perform. Once,
//! on a request, which is not the loop guarded against above.
use duck_ipc_proto as proto;
@ -87,6 +93,30 @@ pub fn verdict_for(
}
}
/// Which of these units are not running `expected`, without touching any of them.
///
/// The observing half of [`check`], for the one caller that must act differently. `Engine::apply`
/// reaching an already-current release is the only way a stale `updaterd` gets looked at — this
/// module will not restart it, deliberately — and it cannot drive `systemctl` the way [`check`] does:
/// restarting `updaterd` or `btd` from inside a running update kills the process doing the restarting
/// or drops the transport carrying the reply. So it reads here and schedules there.
///
/// [`verdict_for`] rather than a second comparison, so what counts as stale stays in one place: a
/// stopped unit does not, and neither does one whose build predates the identity file.
///
/// `is_self` is false and both stale verdicts are accepted, because whether a unit may be restarted
/// is the caller's question and not this one's. Every stale unit is named.
pub fn stale_units(expected: &semver::Version, units: &[String]) -> Vec<String> {
units
.iter()
.filter(|unit| {
let verdict = verdict_for(running_release(unit).as_ref(), expected, false);
matches!(verdict, Verdict::Restarted | Verdict::ReportedOnly)
})
.cloned()
.collect()
}
/// The release each unit is running, checked against what its component has active.
///
/// `systemctl` is a parameter for the same reason it is one in `engine`: a test hands it a stub. It
@ -307,7 +337,13 @@ mod tests {
.map(str::to_owned)
.to_vec();
let findings = check(systemctl.to_str().unwrap(), &active, "updaterd", units).await;
let findings = check(
systemctl.to_str().unwrap(),
&active,
"updaterd",
units.clone(),
)
.await;
assert_eq!(verdict_of(&findings, "configd"), Verdict::Restarted);
assert_eq!(verdict_of(&findings, "robotd"), Verdict::Current);
@ -324,5 +360,12 @@ mod tests {
assert!(!calls.contains("restart robotd"), "{calls}");
assert!(!calls.contains("restart padd"), "{calls}");
assert!(!calls.contains("restart updaterd"), "{calls}");
// And the observing half, over the same files, because it has to agree with the above on what
// stale means while disagreeing on what to do about it. `updaterd` is in this list precisely
// where it is absent from the calls: the caller schedules that restart instead of running it,
// and a `stale_units` that hid it would leave the one skew nothing else repairs unreported.
let stale = stale_units(&active, &units);
assert_eq!(stale, ["btd", "configd", "updaterd"], "{stale:?}");
}
}