fix(swap): never release the fallback swap during a failed rollback; release 0.3.0 [P0]
The rollback claimed an invariant in its own comment - restore the original, confirm it,
and only then release staging - and did not enforce it. original_back was computed and then
ignored: the staging swapoff ran unconditionally.
The failure path that mattered: staging is active, the original is already off, reactivating
the original fails (swapon can fail under the memory pressure that made the resize
necessary), and the rollback then switched off the staging swap - the last thing carrying
load. Recovering from a failed resize left the host with no swap at all, which is worse
than the failure being recovered from.
original_back now gates the release. If the original cannot be brought back the rollback
returns early with the staging swap still active, and the error carries the two commands
that finish the job by hand. An extra temporary swap is always the better end state than
none. The staging file is also only unlinked once swapoff has actually succeeded - an
active swapfile cannot be removed, and attempting it produced an error that explained
nothing.
Asserting this needs a seam, because the invariant is a sequence of calls rather than a
value: a small SwapCommands trait covers swapon/swapoff, with the real implementation
behind it. Three tests: the original cannot be restored and swapoff is never called with
the message naming the preserved fallback; the original comes back and staging is released
in that order; nothing was taken down so nothing is touched.
Also from the same audit:
- StaleOwnership advice was wrong. It suggested deleting the ownership record, which turns
the file Foreign - refused just as firmly. The message now distinguishes the two cases: a
leftover .previous next to the swapfile is the signature of a replacement killed between
installing the new file and recording its identity, so the error says nothing is lost,
points at the intact previous swapfile, and gives the exact commands for going back or
keeping the new file. Full automatic crash recovery is deliberately not added here - it
needs a transition journal, and that belongs with the Debian acceptance work.
- The acceptance checklist still described the PID-file lock ("re-acquires by the absence
of /proc/<pid>"), which stopped being true when locking moved to flock. The kernel
releases it on process exit, and the leftover file means nothing.
- shellcheck was installed by --prepare-build-env but never run. It now runs in
prepare_build_gate: the script installs systemd units and writes to /usr/sbin, so it is
part of the release surface.
- The skip message for systemd-analyze verify now says what the check actually is - a
post-install gate, expected to be skipped on a clean builder - rather than reading like a
build gate that silently did not run.
One audit item did not reproduce: there is no duplicated -j in antitorrent_drop_counter,
the flag appears once.
Version raised to 0.3.0 in traffic_core/Cargo.toml, with the packaging examples in README
and INSTALLATION updated to match. The reboot scheduler keeps its own 0.2.0.
cargo fmt --check, clippy -D warnings, cargo test --locked --workspace --release: 191 + 4
passed. build.sh not syntax-checked here - no bash on this host; the shellcheck gate now
covers it on the builder.
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
[package]
|
||||
name = "awg2-harness"
|
||||
version = "0.1.6"
|
||||
version = "0.3.0"
|
||||
edition = "2024"
|
||||
|
||||
[dependencies]
|
||||
|
||||
+253
-23
@@ -772,12 +772,7 @@ fn ensure_desired_swapfile(desired: &DesiredSwap, active: &[ActiveSwap]) -> anyh
|
||||
refusing to touch it. Remove it manually or point swap.path elsewhere",
|
||||
p.display()
|
||||
),
|
||||
SwapfileOwnership::StaleOwnership => anyhow::bail!(
|
||||
"{} is not the file awg2-harness created - it was replaced since. Refusing to \
|
||||
touch it. Remove it manually, or delete {SWAP_OWNERSHIP_PATH} to let \
|
||||
awg2-harness adopt a freshly created swapfile",
|
||||
p.display()
|
||||
),
|
||||
SwapfileOwnership::StaleOwnership => anyhow::bail!("{}", stale_ownership_help(p)?),
|
||||
_ => {}
|
||||
}
|
||||
|
||||
@@ -829,6 +824,43 @@ fn ensure_desired_swapfile(desired: &DesiredSwap, active: &[ActiveSwap]) -> anyh
|
||||
}
|
||||
}
|
||||
|
||||
/// Explains a stale ownership record, distinguishing the two ways it happens.
|
||||
///
|
||||
/// A leftover `.previous` next to the swapfile is the signature of a replacement that was
|
||||
/// killed between installing the new file and recording its identity: the path holds the
|
||||
/// new inode while the record still describes the old one. That is recoverable by hand and
|
||||
/// deserves the exact commands, not the generic message.
|
||||
///
|
||||
/// The generic advice must also not say "delete the ownership record" on its own - without
|
||||
/// a record the file becomes `Foreign`, which this refuses just as firmly.
|
||||
fn stale_ownership_help(p: &Path) -> anyhow::Result<String> {
|
||||
let previous = suffixed_path(p, PREVIOUS_SUFFIX)?;
|
||||
if previous.exists() {
|
||||
return Ok(format!(
|
||||
"{} does not match the recorded swapfile, and {} is present: a swap replacement \
|
||||
was interrupted after the new file was installed but before its identity was \
|
||||
recorded. Nothing is lost - the previous swapfile is still at {}. To go back: \
|
||||
`mv {} {}` then remove {}. To keep the new file instead: remove {} and {}, \
|
||||
then re-run ensure-swap so it is created and recorded from scratch",
|
||||
p.display(),
|
||||
previous.display(),
|
||||
previous.display(),
|
||||
previous.display(),
|
||||
p.display(),
|
||||
SWAP_OWNERSHIP_PATH,
|
||||
previous.display(),
|
||||
SWAP_OWNERSHIP_PATH,
|
||||
));
|
||||
}
|
||||
Ok(format!(
|
||||
"{} is not the file awg2-harness created - it was replaced since. Refusing to touch \
|
||||
it. Move it aside or delete it, and remove {}, then re-run ensure-swap: a swapfile \
|
||||
with no ownership record is treated as foreign and is never reused",
|
||||
p.display(),
|
||||
SWAP_OWNERSHIP_PATH
|
||||
))
|
||||
}
|
||||
|
||||
/// Re-activates the managed swap at a new priority, restoring the old one on failure.
|
||||
///
|
||||
/// `swapoff` followed by `swapon` is two steps, and the second can fail - a bad priority,
|
||||
@@ -999,6 +1031,39 @@ fn rollback_replacement(
|
||||
previous: &Path,
|
||||
state: &ReplacementState,
|
||||
cause: anyhow::Error,
|
||||
) -> anyhow::Error {
|
||||
rollback_replacement_with(&SystemSwap, desired, staging, previous, state, cause)
|
||||
}
|
||||
|
||||
/// The two kernel operations the rollback depends on.
|
||||
///
|
||||
/// Behind a trait so the recovery order can be tested. The invariant that matters -
|
||||
/// never release the staging swap while the original is not back - is only observable as a
|
||||
/// sequence of calls, so asserting it needs a seam rather than another pure helper.
|
||||
trait SwapCommands {
|
||||
fn swapon(&self, path: &Path, priority: i32) -> anyhow::Result<()>;
|
||||
fn swapoff(&self, path: &Path) -> anyhow::Result<()>;
|
||||
}
|
||||
|
||||
struct SystemSwap;
|
||||
|
||||
impl SwapCommands for SystemSwap {
|
||||
fn swapon(&self, path: &Path, priority: i32) -> anyhow::Result<()> {
|
||||
swapon_at(path, priority)
|
||||
}
|
||||
|
||||
fn swapoff(&self, path: &Path) -> anyhow::Result<()> {
|
||||
run("swapoff", &[path.to_string_lossy().as_ref()])
|
||||
}
|
||||
}
|
||||
|
||||
fn rollback_replacement_with(
|
||||
ops: &dyn SwapCommands,
|
||||
desired: &DesiredSwap,
|
||||
staging: &Path,
|
||||
previous: &Path,
|
||||
state: &ReplacementState,
|
||||
cause: anyhow::Error,
|
||||
) -> anyhow::Error {
|
||||
let p = &desired.path;
|
||||
let mut notes: Vec<String> = Vec::new();
|
||||
@@ -1026,7 +1091,7 @@ fn rollback_replacement(
|
||||
// Reactivate it if we took it down, at the priority it had before.
|
||||
let mut original_back = state.original_active;
|
||||
if !state.original_active {
|
||||
match swapon_at(p, state.previous_priority) {
|
||||
match ops.swapon(p, state.previous_priority) {
|
||||
Ok(()) => original_back = true,
|
||||
Err(e) => notes.push(format!(
|
||||
"could not reactivate {} at priority {}: {e}",
|
||||
@@ -1036,32 +1101,53 @@ fn rollback_replacement(
|
||||
}
|
||||
}
|
||||
|
||||
// Only now release the staging swap, so the two are never both down.
|
||||
// The staging swap is the only thing still carrying load at this point. Releasing it
|
||||
// before the original is confirmed back would turn a failed resize into a host with no
|
||||
// swap at all - the rollback would be doing more damage than the failure it recovers
|
||||
// from. An extra temporary swap is always the better end state.
|
||||
if !original_back {
|
||||
let detail = format_notes(¬es);
|
||||
return anyhow::anyhow!(
|
||||
"{cause}; the previous swap could not be restored{detail}; leaving the staging \
|
||||
swap at {} active so the host is not left without swap. Once memory pressure \
|
||||
allows, run `swapon -p {} {}` and then `swapoff {}`",
|
||||
staging.display(),
|
||||
state.previous_priority,
|
||||
p.display(),
|
||||
staging.display()
|
||||
);
|
||||
}
|
||||
|
||||
// Original confirmed back: now the staging swap can go. The file is only unlinked once
|
||||
// swapoff has actually succeeded - removing an active swapfile is not possible, and
|
||||
// trying leaves an error that says nothing useful.
|
||||
if state.staging_active {
|
||||
match run("swapoff", &[staging.to_string_lossy().as_ref()]) {
|
||||
Ok(()) => {}
|
||||
match ops.swapoff(staging) {
|
||||
Ok(()) => {
|
||||
if let Err(e) = remove_if_present(staging) {
|
||||
notes.push(format!("inactive staging file left behind: {e}"));
|
||||
}
|
||||
}
|
||||
Err(e) => notes.push(format!(
|
||||
"staging swap {} is still active: {e}",
|
||||
"previous swap is restored, but the staging swap {} is still active: {e}",
|
||||
staging.display()
|
||||
)),
|
||||
}
|
||||
}
|
||||
if let Err(e) = remove_if_present(staging) {
|
||||
notes.push(format!("staging file left behind: {e}"));
|
||||
} else if let Err(e) = remove_if_present(staging) {
|
||||
notes.push(format!("inactive staging file left behind: {e}"));
|
||||
}
|
||||
|
||||
let detail = if notes.is_empty() {
|
||||
anyhow::anyhow!(
|
||||
"{cause}; rolled back to the previous swapfile{}",
|
||||
format_notes(¬es)
|
||||
)
|
||||
}
|
||||
|
||||
fn format_notes(notes: &[String]) -> String {
|
||||
if notes.is_empty() {
|
||||
String::new()
|
||||
} else {
|
||||
format!(" ({})", notes.join("; "))
|
||||
};
|
||||
if original_back {
|
||||
anyhow::anyhow!("{cause}; rolled back to the previous swapfile{detail}")
|
||||
} else {
|
||||
anyhow::anyhow!(
|
||||
"{cause}; rollback could not restore the previous swap{detail}. The host may \
|
||||
currently have no swap"
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1648,6 +1734,150 @@ mod tests {
|
||||
assert!(!state.new_installed);
|
||||
}
|
||||
|
||||
/// Records the swap calls a rollback makes, so their order and absence can be asserted.
|
||||
struct FakeSwap {
|
||||
swapon_fails: bool,
|
||||
calls: std::sync::Mutex<Vec<String>>,
|
||||
}
|
||||
|
||||
impl FakeSwap {
|
||||
fn new(swapon_fails: bool) -> Self {
|
||||
Self {
|
||||
swapon_fails,
|
||||
calls: std::sync::Mutex::new(Vec::new()),
|
||||
}
|
||||
}
|
||||
fn calls(&self) -> Vec<String> {
|
||||
self.calls.lock().expect("lock").clone()
|
||||
}
|
||||
}
|
||||
|
||||
impl SwapCommands for FakeSwap {
|
||||
fn swapon(&self, path: &Path, priority: i32) -> anyhow::Result<()> {
|
||||
self.calls
|
||||
.lock()
|
||||
.expect("lock")
|
||||
.push(format!("swapon {} -p {priority}", path.display()));
|
||||
if self.swapon_fails {
|
||||
anyhow::bail!("cannot allocate swap");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
fn swapoff(&self, path: &Path) -> anyhow::Result<()> {
|
||||
self.calls
|
||||
.lock()
|
||||
.expect("lock")
|
||||
.push(format!("swapoff {}", path.display()));
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
fn rollback_fixture() -> (DesiredSwap, PathBuf, PathBuf) {
|
||||
let desired = DesiredSwap {
|
||||
path: PathBuf::from("/swapfile"),
|
||||
size_bytes: 4096,
|
||||
priority: 5,
|
||||
swappiness: 1,
|
||||
};
|
||||
(
|
||||
desired,
|
||||
PathBuf::from("/swapfile.awg2-harness.new"),
|
||||
PathBuf::from("/swapfile.awg2-harness.previous"),
|
||||
)
|
||||
}
|
||||
|
||||
/// The invariant that matters most: if the original cannot be brought back, the staging
|
||||
/// swap is the only thing carrying load, so releasing it would turn a failed resize
|
||||
/// into a host with no swap at all. An extra temporary swap is always the better end
|
||||
/// state.
|
||||
#[test]
|
||||
fn rollback_keeps_staging_active_when_the_original_cannot_be_restored() {
|
||||
let (desired, staging, previous) = rollback_fixture();
|
||||
let ops = FakeSwap::new(true);
|
||||
let state = ReplacementState {
|
||||
staging_active: true,
|
||||
original_active: false,
|
||||
original_moved: false,
|
||||
new_installed: false,
|
||||
previous_priority: 1,
|
||||
};
|
||||
|
||||
let err = rollback_replacement_with(
|
||||
&ops,
|
||||
&desired,
|
||||
&staging,
|
||||
&previous,
|
||||
&state,
|
||||
anyhow::anyhow!("resize failed"),
|
||||
);
|
||||
|
||||
let calls = ops.calls();
|
||||
assert_eq!(
|
||||
calls,
|
||||
vec!["swapon /swapfile -p 1"],
|
||||
"the staging swap must not be released"
|
||||
);
|
||||
assert!(!calls.iter().any(|c| c.starts_with("swapoff")));
|
||||
let msg = err.to_string();
|
||||
assert!(msg.contains("leaving the staging swap"));
|
||||
assert!(msg.contains("so the host is not left without swap"));
|
||||
}
|
||||
|
||||
/// With the original confirmed back, the staging swap is released - and at the previous
|
||||
/// priority, not the one being applied, which may be what failed.
|
||||
#[test]
|
||||
fn rollback_releases_staging_only_after_the_original_is_back() {
|
||||
let (desired, staging, previous) = rollback_fixture();
|
||||
let ops = FakeSwap::new(false);
|
||||
let state = ReplacementState {
|
||||
staging_active: true,
|
||||
original_active: false,
|
||||
original_moved: false,
|
||||
new_installed: false,
|
||||
previous_priority: 1,
|
||||
};
|
||||
|
||||
let err = rollback_replacement_with(
|
||||
&ops,
|
||||
&desired,
|
||||
&staging,
|
||||
&previous,
|
||||
&state,
|
||||
anyhow::anyhow!("resize failed"),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
ops.calls(),
|
||||
vec![
|
||||
"swapon /swapfile -p 1",
|
||||
"swapoff /swapfile.awg2-harness.new",
|
||||
],
|
||||
"restore first, release second"
|
||||
);
|
||||
assert!(
|
||||
err.to_string()
|
||||
.contains("rolled back to the previous swapfile")
|
||||
);
|
||||
}
|
||||
|
||||
/// Nothing was taken down yet, so there is nothing to restore and nothing to release.
|
||||
#[test]
|
||||
fn rollback_is_quiet_when_the_original_was_never_deactivated() {
|
||||
let (desired, staging, previous) = rollback_fixture();
|
||||
let ops = FakeSwap::new(false);
|
||||
let state = ReplacementState::default();
|
||||
|
||||
let _ = rollback_replacement_with(
|
||||
&ops,
|
||||
&desired,
|
||||
&staging,
|
||||
&previous,
|
||||
&state,
|
||||
anyhow::anyhow!("resize failed"),
|
||||
);
|
||||
assert!(ops.calls().is_empty());
|
||||
}
|
||||
|
||||
/// A leftover from a killed run that is still active cannot be unlinked. Removing it
|
||||
/// blindly fails every reconcile, so it is reported with the command that fixes it.
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user