Merge branch 'dev' into i1g/fix_coprocessor_data_null

Author: IvanGoncharov

.changesets/fix_caroline_compute_pool_metrics.md

@@ -0,0 +1,19 @@
+### Add compute pool metrics ([PR #7184](https://github.com/apollographql/router/pull/7184))
+
+The compute job pool is used within the router for compute intensive jobs that should not block the Tokio worker threads.
+When this pool becomes saturated it is difficult for users to see why so that they can take action.
+This change adds new metrics to help users understand how long jobs are waiting to be processed.  
+
+New metrics:
+- `apollo.router.compute_jobs.queue_is_full` - A counter of requests rejected because the queue was full.
+- `apollo.router.compute_jobs.duration` - A histogram of time spent in the compute pipeline by the job, including the queue and query planning.
+  - `job.type`: (`QueryPlanning`, `QueryParsing`, `Introspection`)
+  - `job.outcome`: (`ExecutedOk`, `ExecutedError`, `ChannelError`, `RejectedQueueFull`, `Abandoned`)
+- `apollo.router.compute_jobs.queue.wait.duration` - A histogram of time spent in the compute queue by the job.
+  - `job.type`: (`QueryPlanning`, `QueryParsing`, `Introspection`)
+- `apollo.router.compute_jobs.execution.duration` - A histogram of time spent to execute job (excludes time spent in the queue).
+  - `job.type`: (`QueryPlanning`, `QueryParsing`, `Introspection`)
+- `apollo.router.compute_jobs.active_jobs` - A gauge of the number of compute jobs being processed in parallel.
+  - `job.type`: (`QueryPlanning`, `QueryParsing`, `Introspection`)
+
+By [@carodewig](https://github.com/carodewig) in https://github.com/apollographql/router/pull/7184

.changesets/fix_renee_bail_invalid_query_plans.md

@@ -0,0 +1,7 @@
+### Fix crash when an invalid query plan is generated ([PR #7214](https://github.com/apollographql/router/pull/7214))
+
+When an invalid query plan is generated, the router could panic and crash.
+This could happen if there are gaps in the GraphQL validation implementation.
+Now, even if there are unresolved gaps, the router will handle it gracefully and reject the request.
+
+By [@goto-bus-stop](https://github.com/goto-bus-stop) in https://github.com/apollographql/router/pull/7214

.config/mise/config.toml

@@ -12,3 +12,4 @@ rust = "1.85.0"
 "cargo:graphql_client_cli" = "0.14.0"
 "cargo:cargo-llvm-cov" = "0.6.10"
 "cargo:cargo-fuzz" = "0.12.0"
+"cargo:typos-cli" = "1.31.1"

Cargo.lock

@@ -1351,7 +1351,7 @@ dependencies = [
  "bitflags 2.6.0",
  "cexpr",
  "clang-sys",
- "itertools 0.10.5",
+ "itertools 0.11.0",
  "lazy_static",
  "lazycell",
  "log",
@@ -1946,9 +1946,9 @@ dependencies = [
 
 [[package]]
 name = "crossbeam-channel"
-version = "0.5.14"
+version = "0.5.15"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "06ba6d68e24814cb8de6bb986db8222d3a027d15872cabc0d18817bc3c0e4471"
+checksum = "82b8f8f868b36967f9606790d1903570de9ceaf870a7bf9fbbd3016d636a2cb2"
 dependencies = [
  "crossbeam-utils",
 ]
@@ -2382,7 +2382,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "33d852cb9b869c2a9b3df2f71a3074817f01e1844f839a144f5fcef059a4eb5d"
 dependencies = [
  "libc",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -3993,7 +3993,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "fc2f4eb4bc735547cfed7c0a4922cbd04a4655978c09b54f1f7b228750664c34"
 dependencies = [
  "cfg-if",
- "windows-targets 0.48.5",
+ "windows-targets 0.52.6",
 ]
 
 [[package]]
@@ -5149,7 +5149,7 @@ version = "0.13.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "d0f3e5beed80eb580c68e2c600937ac2c4eedabdfd5ef1e5b7ea4f3fba84497b"
 dependencies = [
- "heck 0.4.1",
+ "heck 0.5.0",
  "itertools 0.13.0",
  "log",
  "multimap 0.10.0",
@@ -5170,7 +5170,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "81bddcdb20abf9501610992b6759a4c888aef7d1a7247ef75e2404275ac24af1"
 dependencies = [
  "anyhow",
- "itertools 0.10.5",
+ "itertools 0.11.0",
  "proc-macro2",
  "quote",
  "syn 2.0.90",
@@ -5291,7 +5291,7 @@ dependencies = [
  "once_cell",
  "socket2",
  "tracing",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -5912,7 +5912,7 @@ dependencies = [
  "errno",
  "libc",
  "linux-raw-sys 0.4.14",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -5925,7 +5925,7 @@ dependencies = [
  "errno",
  "libc",
  "linux-raw-sys 0.9.3",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -6474,7 +6474,7 @@ dependencies = [
  "cfg-if",
  "libc",
  "psm",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -6666,7 +6666,7 @@ dependencies = [
  "getrandom 0.3.1",
  "once_cell",
  "rustix 1.0.3",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -7739,7 +7739,7 @@ version = "0.1.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb"
 dependencies = [
- "windows-sys 0.48.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]

DEVELOPMENT.md

@@ -106,7 +106,7 @@ cargo build --profile release-dhat --features dhat-heap
 
 This will create a router in `./target/release-dhat`, which can be run with:
 ```shell
-cargo run --profile release-dhat --feautures dhat-heap -- -s ./apollo-router/testing_schema.graphql -c router.yaml
+cargo run --profile release-dhat --features dhat-heap -- -s ./apollo-router/testing_schema.graphql -c router.yaml
 ```
 
 When you run your binary, on termination you will get `dhat-heap.json` and/or `dhat-ad-hoc.json` files which can

RELEASE_CHECKLIST.md

@@ -283,7 +283,7 @@ Start following the steps below to start a release PR.  The process is **not ful
     git push --set-upstream "${APOLLO_ROUTER_RELEASE_GIT_ORIGIN}" "prep-${APOLLO_ROUTER_RELEASE_VERSION}"
     ```
 
-15. Programatically create a small temporary file called `this_release.md` with the changelog details of _precisely this release_ from the `CHANGELOG.md`:
+15. Programmatically create a small temporary file called `this_release.md` with the changelog details of _precisely this release_ from the `CHANGELOG.md`:
 
     > Note: This file could totally be created by the `xtask` if we merely decide convention for it and whether we want it checked in or not.  It will be used again later in process and, in theory, by CI.  Definitely not suggesting this should live on as regex.
 

apollo-federation/src/link/link_spec_definition.rs

@@ -156,7 +156,7 @@ impl LinkSpecDefinition {
         //   (in practice, our own code can actually handle this as it does not strongly rely on
         //   that "it should be the first" rule, but that would set a bad example).
         // 2. earlier versions (pre-#1875) were always putting that directive on the definition,
-        //   and we wanted to avoid suprising users by changing that for not reason.
+        //   and we wanted to avoid surprising users by changing that for not reason.
         //
         // So instead, we put the directive on the schema definition unless some extensions exists
         // but no definition does (that is, no non-extension elements are populated).

apollo-federation/src/operation/mod.rs

@@ -77,7 +77,7 @@ static NEXT_ID: atomic::AtomicUsize = atomic::AtomicUsize::new(1);
 ///
 /// NOTE: This ID does not ensure that IDs are unique because its internal counter resets on
 /// startup. It currently implements `Serialize` for debugging purposes. It should not implement
-/// `Deserialize`, and, more specfically, it should not be used for caching until uniqueness is
+/// `Deserialize`, and, more specifically, it should not be used for caching until uniqueness is
 /// provided (i.e. the inner type is a `Uuid` or the like).
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, serde::Serialize)]
 pub(crate) struct SelectionId(usize);
@@ -1324,7 +1324,7 @@ impl SelectionSet {
     /// so we can efficiently generate query plans. In order to prevent the query planner from spending time
     /// exploring those useless __typename options, we "remove" the unnecessary __typename selections from the
     /// operation. Since we need to ensure that the __typename field will still need to be queried, we "tag"
-    /// one of the "sibling" selections (using "attachement") to remember that __typename needs to be added
+    /// one of the "sibling" selections (using "attachment") to remember that __typename needs to be added
     /// back eventually. The core query planning algorithm will ignore that tag, and because __typename has been
     /// otherwise removed, we'll save any related work. As we build the final query plan, we'll check back for
     /// those "tags" and add back the __typename selections. As this only happen after the query planning
@@ -1698,7 +1698,7 @@ impl SelectionSet {
     ///
     /// The final selections are optional. If `path` ends on a leaf field, then no followup
     /// selections would make sense.
-    /// When final selections are provided, unecessary fragments will be automatically removed
+    /// When final selections are provided, unnecessary fragments will be automatically removed
     /// at the junction between the path and those final selections.
     ///
     /// For instance, suppose that we have:
@@ -2144,12 +2144,12 @@ fn compute_aliases_for_non_merging_fields(
                         };
                     }
                 } else {
-                    // We need to alias the new occurence.
+                    // We need to alias the new occurrence.
                     let alias = gen_alias_name(response_name, &seen_response_names);
 
                     // Given how we generate aliases, it's is very unlikely that the generated alias will conflict with any of the other response name
                     // at the level, but it's theoretically possible. By adding the alias to the seen names, we ensure that in the remote change that
-                    // this ever happen, we'll avoid the conflict by giving another alias to the followup occurence.
+                    // this ever happen, we'll avoid the conflict by giving another alias to the followup occurrence.
                     let selections = match field.selection_set.as_ref() {
                         Some(s) => {
                             let mut p = path.clone();

apollo-federation/src/query_graph/graph_path.rs

@@ -257,7 +257,7 @@ pub(crate) struct SubgraphEnteringEdgeInfo {
 ///
 /// NOTE: This ID does not ensure that IDs are unique because its internal counter resets on
 /// startup. It currently implements `Serialize` for debugging purposes. It should not implement
-/// `Deserialize`, and, more specfically, it should not be used for caching until uniqueness is
+/// `Deserialize`, and, more specifically, it should not be used for caching until uniqueness is
 /// provided (i.e. the inner type is a `Uuid` or the like).
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, serde::Serialize)]
 pub(crate) struct OverrideId(usize);

apollo-federation/src/query_plan/fetch_dependency_graph.rs

@@ -822,7 +822,7 @@ impl FetchDependencyGraph {
         // 1. is for the same subgraph
         // 2. has the same merge_at
         // 3. is for the same entity type (we don't reuse nodes for different entities just yet,
-        //    as this can create unecessary dependencies that gets in the way of some optimizations;
+        //    as this can create unnecessary dependencies that gets in the way of some optimizations;
         //    the final optimizations in `reduceAndOptimize` will however later merge nodes
         //    on the same subgraph and mergeAt when possible).
         // 4. is not part of our conditions or our conditions ancestors
@@ -940,7 +940,7 @@ impl FetchDependencyGraph {
             return true;
         }
 
-        // No risk of inifite loop as the graph is acyclic:
+        // No risk of infinite loop as the graph is acyclic:
         let mut to_check = haystack.clone();
         while let Some(next) = to_check.pop() {
             for parent in self.parents_of(next) {
@@ -3851,7 +3851,7 @@ fn compute_nodes_for_op_path_element<'a>(
         // If the operation contains other directives or a non-trivial type condition,
         // we need to preserve it and so we add operation.
         // Otherwise, we just skip it as a minor optimization (it makes the subgraph query
-        // slighly smaller and on complex queries, it might also deduplicate similar selections).
+        // slightly smaller and on complex queries, it might also deduplicate similar selections).
         return Ok(ComputeNodesStackItem {
             tree: &child.tree,
             node_id: stack_item.node_id,

apollo-federation/src/query_plan/fetch_dependency_graph_processor.rs

@@ -302,7 +302,7 @@ impl FetchDependencyGraphProcessor<Option<PlanNode>, DeferredDeferBlock>
                 // Note that currently `ConditionNode` only works for variables
                 // (`ConditionNode.condition` is expected to be a variable name and nothing else).
                 // We could change that, but really, why have a trivial `ConditionNode`
-                // when we can optimise things righ away.
+                // when we can optimise things right away.
                 condition.then_some(value)
             }
             Conditions::Variables(variables) => {

apollo-federation/src/query_plan/generate.rs

@@ -125,7 +125,7 @@ where
     // all plans are guaranteed to be more costly than `initial` anyway.
     // Note that save for `initial`,
     // we always compute `partialCost` as the pros of exiting some branches early are large enough
-    // that it outweight computing some costs unecessarily from time to time.
+    // that it outweigh computing some costs unnecessarily from time to time.
     let mut stack = VecDeque::new();
     stack.push_back(Partial {
         partial_plan: initial,

apollo-federation/src/query_plan/mod.rs

@@ -247,3 +247,17 @@ pub enum QueryPathElement {
     Field { response_key: Name },
     InlineFragment { type_condition: Name },
 }
+
+impl PlanNode {
+    /// Returns the kind of plan node this is as a human-readable string. Exact output not guaranteed.
+    fn node_kind(&self) -> &'static str {
+        match self {
+            Self::Fetch(_) => "Fetch",
+            Self::Sequence(_) => "Sequence",
+            Self::Parallel(_) => "Parallel",
+            Self::Flatten(_) => "Flatten",
+            Self::Defer(_) => "Defer",
+            Self::Condition(_) => "Condition",
+        }
+    }
+}

apollo-federation/src/query_plan/query_planner.rs

@@ -493,10 +493,11 @@ impl QueryPlanner {
             ),
             Some(PlanNode::Sequence(root_node)) if is_subscription => {
                 let Some((primary, rest)) = root_node.nodes.split_first() else {
-                    unreachable!("Sequence must have at least one node");
+                    // TODO(@goto-bus-stop): We could probably guarantee this in the type system
+                    bail!("Invalid query plan: Sequence must have at least one node");
                 };
                 let PlanNode::Fetch(primary) = primary.clone() else {
-                    unreachable!("Primary node of a subscription is not a Fetch");
+                    bail!("Invalid query plan: Primary node of a subscription is not a Fetch");
                 };
                 let rest = PlanNode::Sequence(SequenceNode {
                     nodes: rest.to_vec(),
@@ -509,9 +510,10 @@ impl QueryPlanner {
                 ))
             }
             Some(node) if is_subscription => {
-                unreachable!(
-                    "Unexpected top level PlanNode: '{node:?}' when processing subscription"
-                )
+                bail!(
+                    "Invalid query plan for subscription: unexpected {} at root",
+                    node.node_kind()
+                );
             }
             Some(PlanNode::Fetch(inner)) => Some(TopLevelPlanNode::Fetch(inner)),
             Some(PlanNode::Sequence(inner)) => Some(TopLevelPlanNode::Sequence(inner)),
@@ -628,7 +630,7 @@ fn compute_root_serial_dependency_graph(
             // PORT_NOTE: It is unclear if they correct thing to do here is get the next ID, use
             // the current ID that is inside the fetch dep graph's ID generator, or to use the
             // starting ID. Because this method ensure uniqueness between IDs, this approach was
-            // taken; however, it could be the case that this causes unforseen issues.
+            // taken; however, it could be the case that this causes unforeseen issues.
             digest.push(std::mem::replace(
                 &mut fetch_dependency_graph,
                 new_dep_graph,

apollo-federation/src/query_plan/query_planning_traversal.rs

@@ -640,7 +640,7 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> {
         // To guarantee that the selection is fully local from the provided vertex/type, we must have:
         // - no edge crossing subgraphs from that vertex.
         // - the type must be compositeType (mostly just ensuring the selection make sense).
-        // - everything in the selection must be avaiable in the type (which `rebaseOn` essentially validates).
+        // - everything in the selection must be available in the type (which `rebaseOn` essentially validates).
         // - the selection must not "type-cast" into any abstract type that has inconsistent runtimes acrosse subgraphs. The reason for the
         //   later condition is that `selection` is originally a supergraph selection, but that we're looking to apply "as-is" to a subgraph.
         //   But suppose it has a `... on I` where `I` is an interface. Then it's possible that `I` includes "more" types in the supergraph
@@ -913,11 +913,11 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> {
     }
 
     /// Look at how many plans we'd have to generate and if it's "too much"
-    /// reduce it to something manageable by arbitrarilly throwing out options.
+    /// reduce it to something manageable by arbitrarily throwing out options.
     /// This effectively means that when a query has too many options,
     /// we give up on always finding the "best" query plan in favor of an "ok" query plan.
     ///
-    /// TODO: currently, when we need to reduce options, we do so somewhat arbitrarilly.
+    /// TODO: currently, when we need to reduce options, we do so somewhat arbitrarily.
     /// More precisely, we reduce the branches with the most options first
     /// and then drop the last option of the branch,
     /// repeating until we have a reasonable number of plans to consider.
@@ -1338,7 +1338,7 @@ fn test_prune_and_reorder_first_branch() {
         assert_eq!(branches, expected)
     }
     // Either the first branch had strictly more options than the second,
-    // so it is still at its correct potition after removing one option…
+    // so it is still at its correct position after removing one option…
     assert(
         &["abcdE", "fgh", "ijk", "lmn", "op"],
         &["abcd", "fgh", "ijk", "lmn", "op"],

apollo-federation/src/query_plan/serializable_document.rs

@@ -11,7 +11,7 @@ use serde::Serialize;
 ///
 /// The relevant schema is required to parse from string but not available during deserialization,
 /// so this contains a dual (either or both) “string” or “parsed” representation.
-/// Accessing the latter is fallible, and requires an explicit initilization step to provide the schema.
+/// Accessing the latter is fallible, and requires an explicit initialization step to provide the schema.
 #[derive(Clone)]
 pub struct SerializableDocument {
     serialized: String,