Skip to main content

max / alloy

Put a floor under the recovery, and stop rejecting bare references Two defects in the three commits before this, both found by auditing them. The recovery could skip the step that answers for the disk. It is a plan like any other, so a command in it that fails takes the rest of its queue along, and the wreck was in that queue. Of the stages it runs, five are plain commands with no routing to the wreck, the mount among them. A filesystem too damaged to mount would therefore end the run with the trap still on the disk, which is the case the wreck exists for and the one it was most likely to miss: a disk in bad shape is exactly what makes a repair fail. So the answer of last resort no longer sits inside the recovery. It sits behind it, as Sequence::or_else, where nothing the recovery does can drop it, and it holds for a command that cannot even be spawned. What goes there has to assume nothing about the disk, because it runs precisely when nothing about the disk could be established. update_reference_parses split the reference on its first colon to drop the transport, which is right for what ostree writes and wrong for a bare `quay.io/alloy/alloy:43`: that became `43` and was then rejected for naming no tag. A valid reference refused. A transport never contains a slash and a registry reference always does before its tag, which is what tells the two apart. The comment claimed this behaviour already; now the code has it.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-06 17:34 UTC
Signed with PGP, not checked
Commit: dd7aca4b398b246141e7dffd7e159b31629069f7
Parent: 46412f3
2 files changed, +175 insertions, -9 deletions
M crates/alloy/src/install.rs +25 -5
@@ -1489,11 +1489,15 @@
1489 1489 .trim();
1490 1490
1491 1491 // ostree writes `<transport>:<image>`, and the half a registry has to
1492 - // understand is the second. A reference with no transport at all is left
1493 - // whole rather than guessed at.
1494 - let image = reference
1495 - .split_once(':')
1496 - .map_or(reference, |(_transport, image)| image);
1492 + // understand is the second. A transport never contains a slash and a
1493 + // registry reference always does before its tag, which is what tells
1494 + // `ostree-unverified-registry:quay.io/alloy/alloy:43` apart from a bare
1495 + // `quay.io/alloy/alloy:43`. Splitting on the first colon regardless turned
1496 + // the bare form into `43` and then rejected it for naming no tag.
1497 + let image = match reference.split_once(':') {
1498 + Some((transport, image)) if !transport.contains('/') => image,
1499 + _ => reference,
1500 + };
1497 1501
1498 1502 if image.starts_with('/') {
1499 1503 return Err(format!(
@@ -4225,6 +4229,13 @@
4225 4229 self.passphrase.value(),
4226 4230 self.recovery.as_deref().unwrap_or_default(),
4227 4231 ))
4232 + // The floor: whatever the recovery could not establish, an
4233 + // unreadable disk is not an install. See [`Sequence::or_else`].
4234 + .or_else(wreck_plan(
4235 + self.answers.disk.as_deref().unwrap_or_default(),
4236 + None,
4237 + "the disk could not be checked",
4238 + ))
4228 4239 .on_commit_failure(recover_plan(
4229 4240 self.answers.disk.as_deref().unwrap_or_default(),
4230 4241 &self.choices(
@@ -7146,6 +7157,15 @@
7146 7157 assert!(message.contains("path"), "{message}");
7147 7158 }
7148 7159
7160 + // A bare registry reference has no transport to strip. Splitting on the
7161 + // first colon regardless turned this into "43" and then rejected it for
7162 + // naming no tag, which is a valid reference refused.
7163 + #[test]
7164 + fn an_origin_with_no_transport_prefix_parses() {
7165 + let origin = "container-image-reference=quay.io/alloy/alloy:43\n";
7166 + assert!(update_reference_parses(origin).is_ok());
7167 + }
7168 +
7149 7169 #[test]
7150 7170 fn an_origin_with_no_tag_is_rejected() {
7151 7171 let origin = "container-image-reference=ostree-unverified-registry:quay.io/alloy/alloy\n";
M crates/alloy/src/run.rs +150 -4
@@ -369,6 +369,18 @@
369 369 /// until the duty owed to the disk is discharged, and the outcome the user
370 370 /// reads should describe both.
371 371 failure: Option<String>,
372 + /// What to run when the recovery itself cannot go on.
373 + ///
374 + /// The recovery is a plan like any other: its commands can fail, and one
375 + /// that fails takes the rest of the queue with it. Without this, the step
376 + /// that answers for the disk is the step most easily skipped, because the
377 + /// disk being in bad shape is exactly what makes the repair fail. A
378 + /// filesystem too damaged to mount would end the run with the trap still on
379 + /// it.
380 + ///
381 + /// So the answer of last resort does not sit inside the recovery. It sits
382 + /// behind it, where nothing the recovery does can drop it.
383 + last_resort: Option<Vec<Stage>>,
372 384 }
373 385
374 386 impl Sequence {
@@ -390,6 +402,7 @@
390 402 checked_after_commit: false,
391 403 recovery: None,
392 404 failure: None,
405 + last_resort: None,
393 406 }
394 407 }
395 408
@@ -408,6 +421,17 @@
408 421 self
409 422 }
410 423
424 + /// Stages to run if the recovery cannot finish.
425 + ///
426 + /// The floor under [`on_commit_failure`](Self::on_commit_failure). What
427 + /// goes here has to be the answer that needs nothing to be true of the
428 + /// disk, because it runs precisely when nothing about the disk could be
429 + /// established.
430 + pub(crate) fn or_else(mut self, stages: Vec<Stage>) -> Self {
431 + self.last_resort = (!stages.is_empty()).then_some(stages);
432 + self
433 + }
434 +
411 435 /// Whether the run is discharging its duty to a committed disk rather than
412 436 /// still installing.
413 437 pub(crate) fn recovering(&self) -> bool {
@@ -634,11 +658,20 @@
634 658 self.queue.clear();
635 659 self.current = None;
636 660
637 - // The recovery itself failing. The original failure is what the run is
638 - // about; this says the duty owed to the disk was not discharged, which
639 - // is the sentence that tells a user their machine needs attention
640 - // rather than another try.
661 + // Failing while already answering for the disk.
641 662 if let Some(original) = self.failure.take() {
663 + // One more thing to try, and it is the one that assumes nothing.
664 + if let Some(last_resort) = self.last_resort.take() {
665 + push_line(
666 + &mut self.output,
667 + format!("{message}. Falling back to clearing the disk outright"),
668 + );
669 + self.failure = Some(original);
670 + self.queue = last_resort.into();
671 + return;
672 + }
673 + // Nothing left. This is the sentence that tells a user their
674 + // machine needs attention rather than another try.
642 675 self.outcome = Some(Err(format!(
643 676 "{original}; the disk could not be put right either: {message}"
644 677 )));
@@ -1428,6 +1461,119 @@
1428 1461
1429 1462 // A repair step's exit status is not the state of the disk, so it must not
1430 1463 // stop the check that reads the state of the disk.
1464 +
1465 + // The hole the audit found. The recovery is a plan, so a command in it that
1466 + // fails takes the rest of the queue with it -- including the step that
1467 + // answers for the disk. And the disk being in bad shape is exactly what
1468 + // makes a repair fail, so the answer was most likely to be skipped in the
1469 + // case it exists for.
1470 + #[test]
1471 + fn a_recovery_that_cannot_run_still_reaches_the_last_resort() {
1472 + let mut log = CommandLog::new();
1473 + let mut sequence = Sequence::new(vec![
1474 + Stage::Commit(Invocation::new("true")),
1475 + Stage::Run(Invocation::new("false")),
1476 + ])
1477 + .on_commit_failure(vec![
1478 + // Stands in for the mount: a plain command, not a resolver, and
1479 + // the thing a damaged filesystem refuses.
1480 + Stage::Run(Invocation::new("false")),
1481 + Stage::Run(Invocation::new("echo").arg("checked the disk")),
1482 + ])
1483 + .or_else(vec![Stage::Run(
1484 + Invocation::new("echo").arg("cleared the disk"),
1485 + )]);
1486 +
1487 + drive(&mut sequence, &mut log);
1488 +
1489 + assert!(
1490 + sequence.output().iter().any(|l| l == "cleared the disk"),
1491 + "the disk was left as it was: {:?}",
1492 + sequence.output()
1493 + );
1494 + assert!(sequence.outcome().expect("stopped").is_err());
1495 + }
1496 +
1497 + // A recovery that finishes needs no floor under it.
1498 + #[test]
1499 + fn a_recovery_that_completes_does_not_reach_the_last_resort() {
1500 + let mut log = CommandLog::new();
1501 + let mut sequence = Sequence::new(vec![
1502 + Stage::Commit(Invocation::new("true")),
1503 + Stage::Run(Invocation::new("false")),
1504 + ])
1505 + .on_commit_failure(vec![Stage::Run(Invocation::new("echo").arg("put right"))])
1506 + .or_else(vec![Stage::Run(
1507 + Invocation::new("echo").arg("must not run"),
1508 + )]);
1509 +
1510 + drive(&mut sequence, &mut log);
1511 +
1512 + assert!(!sequence.output().iter().any(|l| l.contains("must not run")));
1513 + }
1514 +
1515 + // A command that cannot be spawned at all is still a recovery that did not
1516 + // finish, so the floor holds there too.
1517 + #[test]
1518 + fn a_recovery_that_cannot_spawn_reaches_the_last_resort() {
1519 + let mut log = CommandLog::new();
1520 + let mut sequence = Sequence::new(vec![
1521 + Stage::Commit(Invocation::new("true")),
1522 + Stage::Run(Invocation::new("false")),
1523 + ])
1524 + .on_commit_failure(vec![Stage::Run(Invocation::new("alloy-no-such-command"))])
1525 + .or_else(vec![Stage::Run(
1526 + Invocation::new("echo").arg("cleared the disk"),
1527 + )]);
1528 +
1529 + drive(&mut sequence, &mut log);
1530 +
1531 + assert!(sequence.output().iter().any(|l| l == "cleared the disk"));
1532 + }
1533 +
1534 + // The reported failure is still the install's. The floor is the last thing
1535 + // tried, not the thing being reported.
1536 + #[test]
1537 + fn the_last_resort_reports_the_failure_that_led_to_it() {
1538 + let mut log = CommandLog::new();
1539 + let mut sequence = Sequence::new(vec![
1540 + Stage::Commit(Invocation::new("true")),
1541 + Stage::Resolve {
1542 + invocation: Invocation::new("echo").arg("x"),
1543 + then: Box::new(|_| Err("the account was never created".into())),
1544 + },
1545 + ])
1546 + .on_commit_failure(vec![Stage::Run(Invocation::new("false"))])
1547 + .or_else(vec![Stage::Run(Invocation::new("true"))]);
1548 +
1549 + drive(&mut sequence, &mut log);
1550 +
1551 + let Some(Err(message)) = sequence.outcome() else {
1552 + panic!("the sequence did not fail");
1553 + };
1554 + assert_eq!(message, "the account was never created");
1555 + }
1556 +
1557 + // Nothing left to try: both the recovery and the floor failed, which is the
1558 + // sentence that means the machine needs attention rather than another try.
1559 + #[test]
1560 + fn a_failed_last_resort_says_the_disk_could_not_be_put_right() {
1561 + let mut log = CommandLog::new();
1562 + let mut sequence = Sequence::new(vec![
1563 + Stage::Commit(Invocation::new("true")),
1564 + Stage::Run(Invocation::new("false")),
1565 + ])
1566 + .on_commit_failure(vec![Stage::Run(Invocation::new("false"))])
1567 + .or_else(vec![Stage::Run(Invocation::new("false"))]);
1568 +
1569 + drive(&mut sequence, &mut log);
1570 +
1571 + let Some(Err(message)) = sequence.outcome() else {
1572 + panic!("the sequence did not fail");
1573 + };
1574 + assert!(message.contains("could not be put right"), "{message}");
1575 + }
1576 +
1431 1577 #[test]
1432 1578 fn an_attempt_that_fails_does_not_stop_the_check_behind_it() {
1433 1579 let mut log = CommandLog::new();