refactor: Make unfail methods return an outcome enum

Author: evaporei

core/src/subgraph/instance_manager.rs

@@ -625,7 +625,8 @@ where
                         // There's no point in calling it if we have no current or parent block
                         // pointers, because there would be: no block to revert to or to search
                         // errors from (first execution).
-                        ctx.inputs
+                        let _outcome = ctx
+                            .inputs
                             .store
                             .unfail_deterministic_error(&current_ptr, &parent_ptr)?;
                     }
@@ -652,7 +653,8 @@ where
                     if should_try_unfail_non_deterministic {
                         // If the deployment head advanced, we can unfail
                         // the non-deterministic error (if there's any).
-                        ctx.inputs
+                        let _outcome = ctx
+                            .inputs
                             .store
                             .unfail_non_deterministic_error(&block_ptr)?;
 

graph/src/components/store.rs

@@ -749,6 +749,12 @@ pub enum EntityOperation {
     Remove { key: EntityKey },
 }
 
+#[derive(Debug, PartialEq)]
+pub enum UnfailOutcome {
+    Noop,
+    Unfailed,
+}
+
 #[derive(Error, Debug)]
 pub enum StoreError {
     #[error("store error: {0}")]
@@ -1022,11 +1028,14 @@ pub trait WritableStore: Send + Sync + 'static {
         &self,
         current_ptr: &BlockPtr,
         parent_ptr: &BlockPtr,
-    ) -> Result<(), StoreError>;
+    ) -> Result<UnfailOutcome, StoreError>;
 
     /// If a non-deterministic error happened and the current deployment head is past the error
     /// block range, this function unfails the subgraph and deletes the error.
-    fn unfail_non_deterministic_error(&self, current_ptr: &BlockPtr) -> Result<(), StoreError>;
+    fn unfail_non_deterministic_error(
+        &self,
+        current_ptr: &BlockPtr,
+    ) -> Result<UnfailOutcome, StoreError>;
 
     /// Set subgraph status to failed with the given error as the cause.
     async fn fail_subgraph(&self, error: SubgraphError) -> Result<(), StoreError>;
@@ -1208,11 +1217,15 @@ impl WritableStore for MockStore {
         unimplemented!()
     }
 
-    fn unfail_deterministic_error(&self, _: &BlockPtr, _: &BlockPtr) -> Result<(), StoreError> {
+    fn unfail_deterministic_error(
+        &self,
+        _: &BlockPtr,
+        _: &BlockPtr,
+    ) -> Result<UnfailOutcome, StoreError> {
         unimplemented!()
     }
 
-    fn unfail_non_deterministic_error(&self, _: &BlockPtr) -> Result<(), StoreError> {
+    fn unfail_non_deterministic_error(&self, _: &BlockPtr) -> Result<UnfailOutcome, StoreError> {
         unimplemented!()
     }
 

graph/src/lib.rs

@@ -123,8 +123,8 @@ pub mod prelude {
         EntityChangeOperation, EntityCollection, EntityFilter, EntityKey, EntityLink,
         EntityModification, EntityOperation, EntityOrder, EntityQuery, EntityRange, EntityWindow,
         EthereumCallCache, ParentLink, PoolWaitStats, QueryStore, QueryStoreManager, StoreError,
-        StoreEvent, StoreEventStream, StoreEventStreamBox, SubgraphStore, WindowAttribute,
-        BLOCK_NUMBER_MAX, SUBSCRIPTION_THROTTLE_INTERVAL,
+        StoreEvent, StoreEventStream, StoreEventStreamBox, SubgraphStore, UnfailOutcome,
+        WindowAttribute, BLOCK_NUMBER_MAX, SUBSCRIPTION_THROTTLE_INTERVAL,
     };
     pub use crate::components::subgraph::{
         BlockState, DataSourceTemplateInfo, HostMetrics, RuntimeHost, RuntimeHostBuilder,

store/postgres/src/deployment_store.rs

@@ -31,7 +31,7 @@ use graph::prelude::{
     anyhow, debug, info, lazy_static, o, warn, web3, ApiSchema, AttributeNames, BlockNumber,
     BlockPtr, CheapClone, DeploymentHash, DeploymentState, Entity, EntityKey, EntityModification,
     EntityQuery, Error, Logger, QueryExecutionError, Schema, StopwatchMetrics, StoreError,
-    StoreEvent, Value, BLOCK_NUMBER_MAX,
+    StoreEvent, UnfailOutcome, Value, BLOCK_NUMBER_MAX,
 };
 use graph_graphql::prelude::api_schema;
 use web3::types::Address;
@@ -1185,7 +1185,7 @@ impl DeploymentStore {
         site: Arc<Site>,
         current_ptr: &BlockPtr,
         parent_ptr: &BlockPtr,
-    ) -> Result<(), StoreError> {
+    ) -> Result<UnfailOutcome, StoreError> {
         let conn = &self.get_conn()?;
         let deployment_id = &site.deployment;
 
@@ -1194,12 +1194,12 @@ impl DeploymentStore {
             let subgraph_error = match detail::fatal_error(conn, deployment_id)? {
                 Some(fatal_error) => fatal_error,
                 // If the subgraph is not failed then there is nothing to do.
-                None => return Ok(()),
+                None => return Ok(UnfailOutcome::Noop),
             };
 
             // Confidence check
             if !subgraph_error.deterministic {
-                return Ok(()); // Nothing to do
+                return Ok(UnfailOutcome::Noop); // Nothing to do
             }
 
             use deployment::SubgraphHealth::*;
@@ -1231,6 +1231,8 @@ impl DeploymentStore {
 
                     // Unfail the deployment.
                     deployment::update_deployment_status(conn, deployment_id, prev_health, None)?;
+
+                    Ok(UnfailOutcome::Unfailed)
                 }
                 // Found error, but not for deployment head, we don't need to
                 // revert the block operations.
@@ -1244,6 +1246,8 @@ impl DeploymentStore {
                         "error_block_hash" => format!("0x{}", hex::encode(&hash_bytes)),
                         "deployment_head" => format!("{}", current_ptr.hash),
                     );
+
+                    Ok(UnfailOutcome::Noop)
                 }
                 // Same as branch above, if you find this warning in the logs,
                 // something is wrong, this shouldn't happen.
@@ -1252,10 +1256,10 @@ impl DeploymentStore {
                         "subgraph_id" => deployment_id,
                         "error_id" => &subgraph_error.id,
                     );
-                }
-            };
 
-            Ok(())
+                    Ok(UnfailOutcome::Noop)
+                }
+            }
         })
     }
 
@@ -1273,7 +1277,7 @@ impl DeploymentStore {
         &self,
         site: Arc<Site>,
         current_ptr: &BlockPtr,
-    ) -> Result<(), StoreError> {
+    ) -> Result<UnfailOutcome, StoreError> {
         let conn = &self.get_conn()?;
         let deployment_id = &site.deployment;
 
@@ -1282,12 +1286,12 @@ impl DeploymentStore {
             let subgraph_error = match detail::fatal_error(conn, deployment_id)? {
                 Some(fatal_error) => fatal_error,
                 // If the subgraph is not failed then there is nothing to do.
-                None => return Ok(()),
+                None => return Ok(UnfailOutcome::Noop),
             };
 
             // Confidence check
             if subgraph_error.deterministic {
-                return Ok(()); // Nothing to do
+                return Ok(UnfailOutcome::Noop); // Nothing to do
             }
 
             match subgraph_error.block_range {
@@ -1314,7 +1318,7 @@ impl DeploymentStore {
                         // Delete the fatal error.
                         deployment::delete_error(conn, &subgraph_error.id)?;
 
-                        Ok(())
+                        Ok(UnfailOutcome::Unfailed)
                     }
                 // NOOP, the deployment head is still before where non-deterministic error happened.
                 block_range => {
@@ -1328,7 +1332,7 @@ impl DeploymentStore {
                         "error_block_hash" => subgraph_error.block_hash.as_ref().map(|hash| format!("0x{}", hex::encode(hash))),
                     );
 
-                    Ok(())
+                    Ok(UnfailOutcome::Noop)
                 }
             }
         })

store/postgres/src/subgraph_store.rs

@@ -24,7 +24,7 @@ use graph::{
         anyhow, futures03::future::join_all, lazy_static, o, web3::types::Address, ApiSchema,
         BlockPtr, DeploymentHash, Entity, EntityKey, EntityModification, Error, Logger, NodeId,
         Schema, StopwatchMetrics, StoreError, SubgraphName, SubgraphStore as SubgraphStoreTrait,
-        SubgraphVersionSwitchingMode,
+        SubgraphVersionSwitchingMode, UnfailOutcome,
     },
     slog::{error, warn},
     util::{backoff::ExponentialBackoff, timed_cache::TimedCache},
@@ -1192,14 +1192,17 @@ impl WritableStoreTrait for WritableStore {
         &self,
         current_ptr: &BlockPtr,
         parent_ptr: &BlockPtr,
-    ) -> Result<(), StoreError> {
+    ) -> Result<UnfailOutcome, StoreError> {
         self.retry("unfail_deterministic_error", || {
             self.writable
                 .unfail_deterministic_error(self.site.clone(), current_ptr, parent_ptr)
         })
     }
 
-    fn unfail_non_deterministic_error(&self, current_ptr: &BlockPtr) -> Result<(), StoreError> {
+    fn unfail_non_deterministic_error(
+        &self,
+        current_ptr: &BlockPtr,
+    ) -> Result<UnfailOutcome, StoreError> {
         self.retry("unfail_non_deterministic_error", || {
             self.writable
                 .unfail_non_deterministic_error(self.site.clone(), current_ptr)

store/postgres/tests/subgraph.rs

@@ -13,6 +13,7 @@ use graph::{
     prelude::SubgraphManifest,
     prelude::SubgraphName,
     prelude::SubgraphVersionSwitchingMode,
+    prelude::UnfailOutcome,
     prelude::{futures03, StoreEvent},
     prelude::{CheapClone, DeploymentHash, NodeId, SubgraphStore as _},
     semver::Version,
@@ -630,11 +631,12 @@ fn fail_unfail_deterministic_error() {
         assert_eq!(Some(1), vi.latest_ethereum_block_number);
 
         // Unfail the subgraph.
-        writable
+        let outcome = writable
             .unfail_deterministic_error(&BLOCKS[1], &BLOCKS[0])
             .unwrap();
 
         // We don't have fatal errors anymore and the block got reverted.
+        assert_eq!(outcome, UnfailOutcome::Unfailed);
         assert!(!query_store.has_non_fatal_errors(None).await.unwrap());
         let vi = get_version_info(&store, NAME);
         assert_eq!(&*NAME, vi.deployment_id.as_str());
@@ -702,11 +704,12 @@ fn fail_unfail_deterministic_error_noop() {
             .expect("can get writable");
 
         // Run unfail with no errors results in NOOP.
-        writable
+        let outcome = writable
             .unfail_deterministic_error(&BLOCKS[1], &BLOCKS[0])
             .unwrap();
 
         // Nothing to unfail, state continues the same.
+        assert_eq!(outcome, UnfailOutcome::Noop);
         assert_eq!(count(), 0);
         let vi = get_version_info(&store, NAME);
         assert_eq!(&*NAME, vi.deployment_id.as_str());
@@ -732,12 +735,13 @@ fn fail_unfail_deterministic_error_noop() {
         assert_eq!(Some(1), vi.latest_ethereum_block_number);
 
         // Running unfail_deterministic_error against a NON-deterministic error will do nothing.
-        writable
+        let outcome = writable
             .unfail_deterministic_error(&BLOCKS[1], &BLOCKS[0])
             .unwrap();
 
         // State continues the same, nothing happened.
         // Neither the block got reverted or error deleted.
+        assert_eq!(outcome, UnfailOutcome::Noop);
         assert_eq!(count(), 1);
         let vi = get_version_info(&store, NAME);
         assert_eq!(&*NAME, vi.deployment_id.as_str());
@@ -757,12 +761,13 @@ fn fail_unfail_deterministic_error_noop() {
 
         // Running unfail_deterministic_error won't do anything,
         // the hashes won't match and there's nothing to revert.
-        writable
+        let outcome = writable
             .unfail_deterministic_error(&BLOCKS[1], &BLOCKS[0])
             .unwrap();
 
         // State continues the same.
         // Neither the block got reverted or error deleted.
+        assert_eq!(outcome, UnfailOutcome::Noop);
         assert_eq!(count(), 2);
         let vi = get_version_info(&store, NAME);
         assert_eq!(&*NAME, vi.deployment_id.as_str());
@@ -848,9 +853,10 @@ fn fail_unfail_non_deterministic_error() {
         assert_eq!(Some(1), vi.latest_ethereum_block_number);
 
         // Unfail the subgraph and delete the fatal error.
-        writable.unfail_non_deterministic_error(&BLOCKS[1]).unwrap();
+        let outcome = writable.unfail_non_deterministic_error(&BLOCKS[1]).unwrap();
 
         // We don't have fatal errors anymore and the subgraph is healthy.
+        assert_eq!(outcome, UnfailOutcome::Unfailed);
         assert_eq!(count(), 0);
         let vi = get_version_info(&store, NAME);
         assert_eq!(&*NAME, vi.deployment_id.as_str());
@@ -918,9 +924,10 @@ fn fail_unfail_non_deterministic_error_noop() {
             .expect("can get writable");
 
         // Running unfail without any errors will do nothing.
-        writable.unfail_non_deterministic_error(&BLOCKS[1]).unwrap();
+        let outcome = writable.unfail_non_deterministic_error(&BLOCKS[1]).unwrap();
 
         // State continues the same, nothing happened.
+        assert_eq!(outcome, UnfailOutcome::Noop);
         assert_eq!(count(), 0);
         let vi = get_version_info(&store, NAME);
         assert_eq!(&*NAME, vi.deployment_id.as_str());
@@ -946,9 +953,10 @@ fn fail_unfail_non_deterministic_error_noop() {
         assert_eq!(Some(1), vi.latest_ethereum_block_number);
 
         // Running unfail_non_deterministic_error will be NOOP, the error is deterministic.
-        writable.unfail_non_deterministic_error(&BLOCKS[1]).unwrap();
+        let outcome = writable.unfail_non_deterministic_error(&BLOCKS[1]).unwrap();
 
         // Nothing happeened, state continues the same.
+        assert_eq!(outcome, UnfailOutcome::Noop);
         assert_eq!(count(), 1);
         let vi = get_version_info(&store, NAME);
         assert_eq!(&*NAME, vi.deployment_id.as_str());
@@ -967,9 +975,10 @@ fn fail_unfail_non_deterministic_error_noop() {
         writable.fail_subgraph(error).await.unwrap();
 
         // Since the block range of the block won't match the deployment head, this will be NOOP.
-        writable.unfail_non_deterministic_error(&BLOCKS[1]).unwrap();
+        let outcome = writable.unfail_non_deterministic_error(&BLOCKS[1]).unwrap();
 
         // State continues the same besides a new error added to the database.
+        assert_eq!(outcome, UnfailOutcome::Noop);
         assert_eq!(count(), 2);
         let vi = get_version_info(&store, NAME);
         assert_eq!(&*NAME, vi.deployment_id.as_str());