chore(coverage): Fix missing coverage from API integration tests

Three changes:
1. We neglected to forward stderr from Rosenpass subprocess two
   in the API setup integration test (driveby fix)
2. Added rudimentary signal handling for program termination
   to rosenpass, specifically for the coverage reporting
3. Apparently std::process::Child::kill() sends SIGKILL and not
   SIGTERM, so our nice new signal handler was never used.
   Switched to a rustix based child reaper.

(2) and (3) where necessary because llvm-cov does not produce coverage
when a subprocess terminates due to a default signal handler.
This commit is contained in:
Karolin Varner
2024-12-09 22:43:06 +01:00
parent 4ea1c76b81
commit 737781c8bc
5 changed files with 97 additions and 10 deletions

11
Cargo.lock generated
View File

@@ -1850,6 +1850,7 @@ dependencies = [
"rustix",
"serde",
"serial_test",
"signal-hook",
"stacker",
"static_assertions",
"tempfile",
@@ -2178,6 +2179,16 @@ version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64"
[[package]]
name = "signal-hook"
version = "0.3.17"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8621587d4798caf8eb44879d42e56b9a93ea5dcd315a6487c357130095b62801"
dependencies = [
"libc",
"signal-hook-registry",
]
[[package]]
name = "signal-hook-registry"
version = "1.4.2"

View File

@@ -74,6 +74,7 @@ hex = { version = "0.4.3" }
heck = { version = "0.5.0" }
libc = { version = "0.2" }
uds = { git = "https://github.com/rosenpass/uds" }
signal-hook = "0.3.17"
#Dev dependencies
serial_test = "3.2.0"
@@ -89,4 +90,4 @@ procspawn = { version = "1.0.1", features = ["test-support"] }
#Broker dependencies (might need cleanup or changes)
wireguard-uapi = { version = "3.0.0", features = ["xplatform"] }
command-fds = "0.2.3"
rustix = { version = "0.38.41", features = ["net", "fs"] }
rustix = { version = "0.38.41", features = ["net", "fs", "process"] }

View File

@@ -62,6 +62,7 @@ heck = { workspace = true, optional = true }
command-fds = { workspace = true, optional = true }
rustix = { workspace = true, optional = true }
uds = { workspace = true, optional = true, features = ["mio_1xx"] }
signal-hook = { workspace = true, optional = true }
[build-dependencies]
anyhow = { workspace = true }
@@ -87,5 +88,6 @@ experiment_api = [
"rosenpass-util/experiment_file_descriptor_passing",
"rosenpass-wireguard-broker/experiment_api",
]
internal_signal_handling_for_coverage_reports = ["signal-hook"]
internal_testing = []
internal_bin_gen_ipc_msg_types = ["hex", "heck"]

View File

@@ -5,6 +5,7 @@ use clap::Parser;
use clap_mangen::roff::{roman, Roff};
use log::error;
use rosenpass::cli::CliArgs;
use rosenpass_util::functional::run;
use std::process::exit;
/// Printing custom man sections when generating the man page
@@ -77,13 +78,40 @@ pub fn main() {
// error!("error dummy");
}
let broker_interface = args.get_broker_interface();
match args.run(broker_interface, None) {
Ok(_) => {}
Err(e) => {
error!("{e:?}");
exit(1);
let res = run(|| {
#[cfg(feature = "internal_signal_handling_for_coverage_reports")]
let term_signal = terminate::TerminateRequested::new()?;
let broker_interface = args.get_broker_interface();
let err = match args.run(broker_interface, None) {
Ok(()) => return Ok(()),
Err(err) => err,
};
// This is very very hacky and just used for coverage measurement
#[cfg(feature = "internal_signal_handling_for_coverage_reports")]
{
let terminated_by_signal = err
.downcast_ref::<std::io::Error>()
.filter(|e| e.kind() == std::io::ErrorKind::Interrupted)
.filter(|_| term_signal.value())
.is_some();
if terminated_by_signal {
log::warn!(
"\
Terminated by signal; this signal handler is correct during coverage testing \
but should be otherwise disabled"
);
return Ok(());
}
}
Err(err)
});
if let Err(e) = res {
error!("{e:?}");
exit(1);
}
}
@@ -110,3 +138,47 @@ Peischl, Stephan Ajuvo, and Lisa Schmidt.";
/// Custom main page section: Bugs.
static BUGS_MAN: &str = r"
The bugs are tracked at https://github.com/rosenpass/rosenpass/issues.";
/// These signal handlers are used exclusively used during coverage testing
/// to ensure that the llvm-cov can produce reports during integration tests
/// with multiple processes where subprocesses are terminated via kill(2).
///
/// llvm-cov does not support producing coverage reports when the process exits
/// through a signal, so this is necessary.
///
/// The functionality of exiting gracefully upon reception of a terminating signal
/// is desired for the production variant of Rosenpass, but we should make sure
/// to use a higher quality implementation; in particular, we should use signalfd(2).
///
#[cfg(feature = "internal_signal_handling_for_coverage_reports")]
mod terminate {
use signal_hook::flag::register as sig_register;
use std::sync::{
atomic::{AtomicBool, Ordering},
Arc,
};
/// Automatically register a signal handler for common termination signals;
/// whether one of these signals was issued can be polled using [Self::value].
///
/// The signal handler is not removed when this struct goes out of scope.
pub struct TerminateRequested {
value: Arc<AtomicBool>,
}
impl TerminateRequested {
/// Register signal handlers watching for common termination signals
pub fn new() -> anyhow::Result<Self> {
let value = Arc::new(AtomicBool::new(false));
for sig in signal_hook::consts::TERM_SIGNALS.iter().copied() {
sig_register(sig, Arc::clone(&value))?;
}
Ok(Self { value })
}
/// Check whether a termination signal has been set
pub fn value(&self) -> bool {
self.value.load(Ordering::Relaxed)
}
}
}

View File

@@ -33,8 +33,10 @@ struct KillChild(std::process::Child);
impl Drop for KillChild {
fn drop(&mut self) {
self.0.kill().discard_result();
self.0.wait().discard_result()
use rustix::process::{kill_process, Pid, Signal::Term};
let pid = Pid::from_child(&self.0);
rustix::process::kill_process(pid, Term).discard_result();
self.0.wait().discard_result();
}
}
@@ -153,7 +155,6 @@ fn api_integration_api_setup() -> anyhow::Result<()> {
peer_b.config_file_path.to_str().context("")?,
])
.stdin(Stdio::null())
.stderr(Stdio::null())
.stdout(Stdio::piped())
.spawn()?,
);