From 2d720ffe9d3607f5cdadab225cb949c756ad6740 Mon Sep 17 00:00:00 2001
From: Theo Butler <theodusbutler@gmail.com>
Date: Fri, 17 Dec 2021 11:29:32 -0500
Subject: [PATCH] Use fresh context for each indexer (#97)

Say we have some query that has no block requirement, so making the query deterministic will set a block hash based on the latest block for that indexer. If we need to try another indexer, then the context will be carried over and the next indexer may be queried with a block that is greater than its indexing status. This will result in unnecessary retries and penalties for many indexers that happen to be selected after the first for any query.

See also: https://github.com/graphprotocol/agora/pull/31
---
 Cargo.toml                |  2 +-
 src/query_engine/mod.rs   | 18 ++++++++++++++++++
 src/query_engine/tests.rs |  4 +++-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index d286a14d..fdcf6a57 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -12,7 +12,7 @@ actix-http = "=3.0.0-beta.14"
 actix-web = "=4.0.0-beta.13"
 async-trait = "0.1"
 bs58 = "0.4"
-cost-model = {git = "ssh://git@github.com/graphprotocol/agora.git", rev = "48bf329"}
+cost-model = {git = "ssh://git@github.com/graphprotocol/agora.git", rev = "c7c9876"}
 eventuals = "0.6"
 futures = "0.3"
 futures-util = "0.3"
diff --git a/src/query_engine/mod.rs b/src/query_engine/mod.rs
index 175a738b..f6fe51b0 100644
--- a/src/query_engine/mod.rs
+++ b/src/query_engine/mod.rs
@@ -297,6 +297,24 @@ impl<I: IndexerInterface + Clone + Send + 'static> QueryEngine<I> {
                 &[&deployment_ipfs],
                 |hist| hist.start_timer(),
             );
+
+            // Since we modify the context in-place, we need to reset the context to the state of
+            // the original client query. This to avoid the following scenario:
+            // 1. A client query has no block requirements set for some top-level operation
+            // 2. The first indexer is selected, with some indexing status at block number `n`
+            // 3. The query is made deterministic by setting the block requirement to the hash of
+            //    block `n`
+            // 4. Some condition requires us to retry this query on another indexer with an indexing
+            //    status at a block less than `n`
+            // 5. The same context is re-used, including the block requirement set to the hash of
+            //    block `n`
+            // 6. The indexer is seen as being behind and is unnecessarily penalized
+            //
+            // TODO: Avoid the additional cloning of the entire AST here, especially in the case
+            // where retries are necessary. Only the top-level operation arguments need to be reset
+            // to the state of the client query.
+            let mut context = context.clone();
+
             let selection_result = self
                 .indexers
                 .select_indexer(
diff --git a/src/query_engine/tests.rs b/src/query_engine/tests.rs
index 7324ac26..59dc3798 100644
--- a/src/query_engine/tests.rs
+++ b/src/query_engine/tests.rs
@@ -565,7 +565,9 @@ impl IndexerInterface for TopologyIndexer {
             let hash = capture.get(1).unwrap().as_str().parse::<Bytes32>().unwrap();
             let number = blocks.iter().position(|block| block.hash == hash).unwrap();
             if number > indexer.block(blocks.len()) {
-                todo!("block ahead of indexer")
+                return Err(
+                    "Failed to decode `block.hash` value: `no block with that hash found`".into(),
+                );
             }
         }
         Ok(IndexerResponse {