Remove catch_unwind wrapper around the rust query planner

.changesets/maint_lrlna_remove_catch_unwind.md

@@ -0,0 +1,15 @@
+
+### Remove catch_unwind wrapper around the native query planner ([PR #6397](https://github.com/apollographql/router/pull/6397))
+
+As part of internal maintenance of the query planner, we are removing the
+catch_unwind wrapper around the native query planner. This wrapper was used as
+an extra safeguard for potential panics the native planner could produce. The
+native query planner no longer has any code paths that could panic. We have also
+not witnessed a panic in the last four months, having processed 560 million real
+user operations through the native planner. 
+
+This maintenance work also removes backtrace capture for federation errors which
+was used for debugging and is no longer necessary as we have the confidence in
+the native planner's implementation.
+
+By [@lrlna](https://github.com/lrlna) in https://github.com/apollographql/router/pull/6397

apollo-federation/src/error/mod.rs

@@ -1,4 +1,3 @@
-use std::backtrace::Backtrace;
 use std::cmp::Ordering;
 use std::fmt::Display;
 use std::fmt::Formatter;
@@ -523,8 +522,8 @@ pub struct MultipleFederationErrors {
 impl MultipleFederationErrors {
     pub fn push(&mut self, error: FederationError) {
         match error {
-            FederationError::SingleFederationError { inner, .. } => {
-                self.errors.push(inner);
+            FederationError::SingleFederationError(error) => {
+                self.errors.push(error);
             }
             FederationError::MultipleFederationErrors(errors) => {
                 self.errors.extend(errors.errors);
@@ -591,22 +590,14 @@ impl Display for AggregateFederationError {
     }
 }
 
-/// Work around thiserror, which when an error field has a type named `Backtrace`
-/// "helpfully" implements `Error::provides` even though that API is not stable yet:
-/// <https://github.com/rust-lang/rust/issues/99301>
-type ThiserrorTrustMeThisIsTotallyNotABacktrace = Backtrace;
-
 // PORT_NOTE: Often times, JS functions would either throw/return a GraphQLError, return a vector
 // of GraphQLErrors, or take a vector of GraphQLErrors and group them together under an
 // AggregateGraphQLError which itself would have a specific error message and code, and throw that.
 // We represent all these cases with an enum, and delegate to the members.
 #[derive(thiserror::Error)]
 pub enum FederationError {
-    #[error("{inner}")]
-    SingleFederationError {
-        inner: SingleFederationError,
-        trace: ThiserrorTrustMeThisIsTotallyNotABacktrace,
-    },
+    #[error(transparent)]
+    SingleFederationError(#[from] SingleFederationError),
     #[error(transparent)]
     MultipleFederationErrors(#[from] MultipleFederationErrors),
     #[error(transparent)]
@@ -616,22 +607,13 @@ pub enum FederationError {
 impl std::fmt::Debug for FederationError {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         match self {
-            Self::SingleFederationError { inner, trace } => write!(f, "{inner}\n{trace}"),
+            Self::SingleFederationError(inner) => std::fmt::Debug::fmt(inner, f),
             Self::MultipleFederationErrors(inner) => std::fmt::Debug::fmt(inner, f),
             Self::AggregateFederationError(inner) => std::fmt::Debug::fmt(inner, f),
         }
     }
 }
 
-impl From<SingleFederationError> for FederationError {
-    fn from(inner: SingleFederationError) -> Self {
-        Self::SingleFederationError {
-            inner,
-            trace: Backtrace::capture(),
-        }
-    }
-}
-
 impl From<DiagnosticList> for FederationError {
     fn from(value: DiagnosticList) -> Self {
         SingleFederationError::from(value).into()

apollo-federation/src/operation/rebase.rs

@@ -150,10 +150,9 @@ impl FederationError {
     fn is_rebase_error(&self) -> bool {
         matches!(
             self,
-            crate::error::FederationError::SingleFederationError {
-                inner: crate::error::SingleFederationError::InternalRebaseError(_),
-                ..
-            }
+            crate::error::FederationError::SingleFederationError(
+                crate::error::SingleFederationError::InternalRebaseError(_)
+            )
         )
     }
 }

apollo-federation/src/query_plan/fetch_dependency_graph.rs

@@ -32,6 +32,7 @@ use crate::bail;
 use crate::display_helpers::DisplayOption;
 use crate::error::FederationError;
 use crate::error::SingleFederationError;
+use crate::internal_error;
 use crate::link::graphql_definition::DeferDirectiveArguments;
 use crate::operation::ArgumentList;
 use crate::operation::ContainmentOptions;
@@ -1718,12 +1719,12 @@ impl FetchDependencyGraph {
                 children.push(child_index);
             } else {
                 let Some(child_defer_ref) = &child.defer_ref else {
-                    panic!(
+                    return Err(internal_error!(
                         "{} has defer_ref `{}`, so its child {} cannot have a top-level defer_ref.",
                         node.display(node_index),
                         DisplayOption(node.defer_ref.as_ref()),
                         child.display(child_index),
-                    );
+                    ));
                 };
 
                 if !node.selection_set.selection_set.selections.is_empty() {
@@ -2676,10 +2677,9 @@ impl FetchDependencyGraphNode {
         let operation =
             operation_compression.compress(&self.subgraph_name, subgraph_schema, operation)?;
         let operation_document = operation.try_into().map_err(|err| match err {
-            FederationError::SingleFederationError {
-                inner: SingleFederationError::InvalidGraphQL { diagnostics },
-                ..
-            } => FederationError::internal(format!(
+            FederationError::SingleFederationError(SingleFederationError::InvalidGraphQL {
+                diagnostics,
+            }) => FederationError::internal(format!(
                 "Query planning produced an invalid subgraph operation.\n{diagnostics}"
             )),
             _ => err,
@@ -3402,7 +3402,9 @@ impl DeferTracking {
 
         if let Some(parent_ref) = &defer_context.current_defer_ref {
             let Some(parent_info) = self.deferred.get_mut(parent_ref) else {
-                panic!("Cannot find info for parent {parent_ref} or {label}");
+                return Err(internal_error!(
+                    "Cannot find info for parent {parent_ref} or {label}"
+                ));
             };
 
             parent_info.deferred.insert(label.clone());

apollo-federation/src/query_plan/query_planner.rs

@@ -16,6 +16,7 @@ use super::fetch_dependency_graph::FetchIdGenerator;
 use super::ConditionNode;
 use crate::error::FederationError;
 use crate::error::SingleFederationError;
+use crate::internal_error;
 use crate::operation::normalize_operation;
 use crate::operation::NamedFragments;
 use crate::operation::NormalizedDefer;
@@ -443,10 +444,10 @@ impl QueryPlanner {
             .root_kinds_to_nodes()?
             .get(&normalized_operation.root_kind)
         else {
-            panic!(
+            return Err(internal_error!(
                 "Shouldn't have a {0} operation if the subgraphs don't have a {0} root",
                 normalized_operation.root_kind
-            );
+            ));
         };
 
         let operation_compression = if self.config.generate_query_fragments {

apollo-federation/src/sources/connect/json_selection/apply_to.rs

@@ -92,6 +92,8 @@ impl ApplyToError {
         }))
     }
 
+    // This macro is useful for tests, but it absolutely should never be used with
+    // dynamic input at runtime, since it panics for any input that's not JSON.
     #[cfg(test)]
     fn from_json(json: &JSON) -> Self {
         if let JSON::Object(error) = json {

apollo-router/src/executable.rs

@@ -1,6 +1,5 @@
 //! Main entry point for CLI command to start server.
 
-use std::cell::Cell;
 use std::env;
 use std::fmt::Debug;
 use std::net::SocketAddr;
@@ -751,20 +750,10 @@ fn setup_panic_handler() {
         } else {
             tracing::error!("{}", e)
         }
-        if !USING_CATCH_UNWIND.get() {
-            // Once we've panic'ed the behaviour of the router is non-deterministic
-            // We've logged out the panic details. Terminate with an error code
-            std::process::exit(1);
-        }
+        std::process::exit(1);
     }));
 }
 
-// TODO: once the Rust query planner does not use `todo!()` anymore,
-// remove this and the use of `catch_unwind` to call it.
-thread_local! {
-    pub(crate) static USING_CATCH_UNWIND: Cell<bool> = const { Cell::new(false) };
-}
-
 static COPIED: AtomicBool = AtomicBool::new(false);
 
 fn copy_args_to_env() {

apollo-router/src/query_planner/bridge_query_planner.rs

@@ -183,10 +183,7 @@ impl PlannerMode {
         let result = QueryPlanner::new(schema.federation_supergraph(), config);
 
         match &result {
-            Err(FederationError::SingleFederationError {
-                inner: error,
-                trace: _,
-            }) => match error {
+            Err(FederationError::SingleFederationError(error)) => match error {
                 SingleFederationError::UnsupportedFederationVersion { .. } => {
                     metric_rust_qp_init(Some(UNSUPPORTED_FED1));
                 }

apollo-router/src/query_planner/dual_query_planner.rs

@@ -15,6 +15,7 @@ use apollo_compiler::validation::Valid;
 use apollo_compiler::ExecutableDocument;
 use apollo_compiler::Name;
 use apollo_compiler::Node;
+use apollo_federation::error::FederationError;
 use apollo_federation::query_plan::query_planner::QueryPlanOptions;
 use apollo_federation::query_plan::query_planner::QueryPlanner;
 use apollo_federation::query_plan::QueryPlan;
@@ -24,7 +25,6 @@ use super::fetch::SubgraphOperation;
 use super::subscription::SubscriptionNode;
 use super::FlattenNode;
 use crate::error::format_bridge_errors;
-use crate::executable::USING_CATCH_UNWIND;
 use crate::query_planner::bridge_query_planner::metric_query_planning_plan_duration;
 use crate::query_planner::bridge_query_planner::JS_QP_MODE;
 use crate::query_planner::bridge_query_planner::RUST_QP_MODE;
@@ -80,47 +80,23 @@ impl BothModeComparisonJob {
     }
 
     fn execute(self) {
-        // TODO: once the Rust query planner does not use `todo!()` anymore,
-        // remove `USING_CATCH_UNWIND` and this use of `catch_unwind`.
-        let rust_result = std::panic::catch_unwind(|| {
-            let name = self
-                .operation_name
-                .clone()
-                .map(Name::try_from)
-                .transpose()?;
-            USING_CATCH_UNWIND.set(true);
-
-            let start = Instant::now();
-
-            // No question mark operator or macro from here …
-            let result =
+        let start = Instant::now();
+
+        let rust_result = self
+            .operation_name
+            .as_deref()
+            .map(|n| Name::new(n).map_err(FederationError::from))
+            .transpose()
+            .and_then(|operation| {
                 self.rust_planner
-                    .build_query_plan(&self.document, name, self.plan_options);
-
-            let elapsed = start.elapsed().as_secs_f64();
-            metric_query_planning_plan_duration(RUST_QP_MODE, elapsed);
-
-            metric_query_planning_plan_both_comparison_duration(RUST_QP_MODE, elapsed);
-            metric_query_planning_plan_both_comparison_duration(JS_QP_MODE, self.js_duration);
-
-            // … to here, so the thread can only eiher reach here or panic.
-            // We unset USING_CATCH_UNWIND in both cases.
-            USING_CATCH_UNWIND.set(false);
-            result
-        })
-        .unwrap_or_else(|panic| {
-            USING_CATCH_UNWIND.set(false);
-            Err(apollo_federation::error::FederationError::internal(
-                format!(
-                    "query planner panicked: {}",
-                    panic
-                        .downcast_ref::<String>()
-                        .map(|s| s.as_str())
-                        .or_else(|| panic.downcast_ref::<&str>().copied())
-                        .unwrap_or_default()
-                ),
-            ))
-        });
+                    .build_query_plan(&self.document, operation, self.plan_options)
+            });
+
+        let elapsed = start.elapsed().as_secs_f64();
+        metric_query_planning_plan_duration(RUST_QP_MODE, elapsed);
+
+        metric_query_planning_plan_both_comparison_duration(RUST_QP_MODE, elapsed);
+        metric_query_planning_plan_both_comparison_duration(JS_QP_MODE, self.js_duration);
 
         let name = self.operation_name.as_deref();
         let operation_desc = if let Ok(operation) = self.document.operations.get(name) {