From 9fd86aa523f6d0c67d084a299d47684a8084f5f4 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Tue, 4 Feb 2025 14:49:13 +0000 Subject: [PATCH] ROVER-311 Stop `rover dev` if the Router binary crashes (#2382) At the moment (in Rover 0.26.3) if the Router fails to startup because of bad configuration `rover dev` doesn't respond to that event. It will sit there and still attempt to update the hot reloadable config, despite the fact that this won't have an impact as the binary is not monitoring the config once it has crashed. This moves us to a much better situation where the error is printed to the user, and then `rover dev` shuts down, so they can retry it once they've fixed the config in question. This also includes a change to re-instate some logging that was previously removed. --- src/command/dev/do_dev.rs | 21 +++++++ src/command/dev/router/binary.rs | 95 ++++++++++++++++++++++++-------- 2 files changed, 92 insertions(+), 24 deletions(-) diff --git a/src/command/dev/do_dev.rs b/src/command/dev/do_dev.rs index 1f41915e3..e89cb7f0e 100644 --- a/src/command/dev/do_dev.rs +++ b/src/command/dev/do_dev.rs @@ -13,6 +13,7 @@ use semver::Version; use tower::ServiceExt; use super::version_upgrade_message::VersionUpgradeMessage; +use crate::command::dev::router::binary::RunRouterBinaryError; use crate::command::dev::router::config::{RouterAddress, RouterHost, RouterPort}; use crate::command::dev::router::hot_reload::HotReloadConfigOverrides; use crate::command::dev::router::run::RunRouter; @@ -278,6 +279,26 @@ impl Dev { eprintln!("{}", router_log); } } + Err(RunRouterBinaryError::BinaryExited(res)) => { + match res { + Ok(status) => { + match status.code() { + None => { + eprintln!("Router process terminal by signal"); + } + Some(code) => { + eprintln!("Router process exited with status code: {code}"); + } + } + + } + Err(err) => { + tracing::error!("Router process exited without status code. Error: {err}") + } + } + eprintln!("\nRouter binary exited, stopping `rover dev` processes..."); + break; + } Err(err) => { tracing::error!("{:?}", err); } diff --git a/src/command/dev/router/binary.rs b/src/command/dev/router/binary.rs index 667abd1f0..8313ab8ac 100644 --- a/src/command/dev/router/binary.rs +++ b/src/command/dev/router/binary.rs @@ -1,12 +1,15 @@ use std::collections::HashMap; -use std::fmt; -use std::process::Stdio; +use std::fmt::Formatter; +use std::net::{AddrParseError, SocketAddr}; +use std::process::{ExitStatus, Stdio}; +use std::{fmt, io}; use buildstructor::Builder; use camino::Utf8PathBuf; use futures::TryFutureExt; use houston::Credential; -use rover_std::Style; +use regex::Regex; +use rover_std::{infoln, Style}; use semver::Version; use tap::TapFallible; use tokio::io::{AsyncBufReadExt, BufReader}; @@ -16,6 +19,7 @@ use tower::{Service, ServiceExt}; use super::config::remote::RemoteRouterConfig; use super::hot_reload::HotReloadError; +use crate::command::dev::router::config::{RouterAddress, RouterHost, RouterPort}; use crate::subtask::SubtaskHandleUnit; use crate::utils::effect::exec::{ExecCommandConfig, ExecCommandOutput}; use crate::RoverError; @@ -39,11 +43,34 @@ fn should_select_log_message(log_message: &str) -> bool { .is_empty() } +fn produce_special_message(raw_message: &str) { + let starting_message_regex = Regex::new(r"^.*\s+.*://(.*:[0-9]+).*\s+.*").unwrap(); + + let contents = match starting_message_regex.captures(raw_message) { + None => raw_message.to_string(), + Some(captures) => { + let socket_address: Option> = + captures.get(1).map(|m| m.as_str().parse()); + match socket_address { + Some(Ok(socket_addr)) => { + let router_address = RouterAddress::new( + Some(RouterHost::CliOption(socket_addr.ip())), + Some(RouterPort::CliOption(socket_addr.port())), + ) + .pretty_string(); + format!("Your supergraph is running! head to {router_address} to query your supergraph") + } + _ => raw_message.to_string(), + } + } + }; + infoln!("{}", contents) +} + impl fmt::Display for RouterLog { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { let warn_prefix = Style::WarningPrefix.paint("WARN:"); let error_prefix = Style::ErrorPrefix.paint("ERROR:"); - let info_prefix = Style::InfoPrefix.paint("INFO:"); let unknown_prefix = Style::ErrorPrefix.paint("UNKNOWN:"); match self { Self::Stdout(stdout) => { @@ -60,12 +87,10 @@ impl fmt::Display for RouterLog { .unwrap_or(stdout); match level { - "INFO" => { - if should_select_log_message(message) { - write!(f, "{} {}", info_prefix, &message)? - } - tracing::info!(%message) + "INFO" if should_select_log_message(message) => { + produce_special_message(message); } + "INFO" => tracing::info!(%message), "DEBUG" => tracing::debug!(%message), "TRACE" => tracing::trace!(%message), "WARN" => write!(f, "{} {}", warn_prefix, &message)?, @@ -112,6 +137,8 @@ pub enum RunRouterBinaryError { }, #[error("Failed to expand config: {}.", .err)] Expansion { err: RoverError }, + #[error("Router Binary exited")] + BinaryExited(io::Result), } impl From for RunRouterBinaryError { @@ -261,24 +288,27 @@ where } match child.stderr.take() { Some(stderr) => { - tokio::task::spawn(async move { - let mut lines = BufReader::new(stderr).lines(); - while let Ok(Some(line)) = - lines.next_line().await.tap_err(|err| { - tracing::error!( - "Error reading from router stderr: {:?}", - err - ) - }) - { - let _ = sender - .send(Ok(RouterLog::Stderr(line))) - .tap_err(|err| { + tokio::task::spawn({ + let sender = sender.clone(); + async move { + let mut lines = BufReader::new(stderr).lines(); + while let Ok(Some(line)) = + lines.next_line().await.tap_err(|err| { tracing::error!( + "Error reading from router stderr: {:?}", + err + ) + }) + { + let _ = sender + .send(Ok(RouterLog::Stderr(line))) + .tap_err(|err| { + tracing::error!( "Failed to send router stderr message. {:?}", err ) - }); + }); + } } }); } @@ -291,6 +321,23 @@ where }); } } + // Spawn a task that just sits listening to the Router binary, and if it + // exits, fire an error to say so, such that we can stop Rover Dev + // running if this happens. + tokio::spawn({ + async move { + let res = child.wait().await; + let _ = sender + .send(Err(RunRouterBinaryError::BinaryExited(res))) + .tap_err(|err| { + tracing::error!( + "Failed to send router stderr message. {:?}", + err + ) + }); + } + }) + .await }) .await; }