CORDA-3329 Exceptions thrown in raw vault observers can cause critica…

…l issues (corda#5816)

Observers registered on NodeVaultService#rawUpdates, if they throw an exception when called from serviceHub#recordTransactions and if this exception is not handled by the flow hospital, then this leads to the transaction not being recorded in the local vault. This could get the ledger in an out of sync state.

In the specific case this happens within FinalityFlow#notariseAndRecord this leads to the transaction being notarized but not recorded in the local vault nor broadcasted in any counter party. The -failed to be recorded locally- transaction and its output states are not visible to any vault, and its input states not able to consumed by a new transaction, since they are recorded as consumed within the Notary. In this specific case we need not loose, by any means, the current transaction.

We will handle all cases by catching all exceptions thrown from serviceHub#recordTransactions, wrapping them with a HospitalizeFlowException and throwing it instead. The flow will get to the hospital for observation to be retried from previous checkpoint on next node restart.

detekt-baseline.xml

@@ -173,6 +173,7 @@
     <ID>ComplexMethod:NodeNamedCache.kt$DefaultNamedCacheFactory$open protected fun &lt;K, V&gt; configuredForNamed(caffeine: Caffeine&lt;K, V&gt;, name: String): Caffeine&lt;K, V&gt;</ID>
     <ID>ComplexMethod:NodeVaultService.kt$NodeVaultService$@Throws(VaultQueryException::class) private fun &lt;T : ContractState&gt; _queryBy(criteria: QueryCriteria, paging_: PageSpecification, sorting: Sort, contractStateType: Class&lt;out T&gt;, skipPagingChecks: Boolean): Vault.Page&lt;T&gt;</ID>
     <ID>ComplexMethod:NodeVaultService.kt$NodeVaultService$private fun makeUpdates(batch: Iterable&lt;CoreTransaction&gt;, statesToRecord: StatesToRecord, previouslySeen: Boolean): List&lt;Vault.Update&lt;ContractState&gt;&gt;</ID>
+    <ID>ComplexMethod:NodeVaultService.kt$NodeVaultService$private fun processAndNotify(updates: List&lt;Vault.Update&lt;ContractState&gt;&gt;)</ID>
     <ID>ComplexMethod:ObjectDiffer.kt$ObjectDiffer$fun diff(a: Any?, b: Any?): DiffTree?</ID>
     <ID>ComplexMethod:Obligation.kt$Obligation$override fun verify(tx: LedgerTransaction)</ID>
     <ID>ComplexMethod:QuasarInstrumentationHook.kt$QuasarInstrumentationHookAgent.Companion$@JvmStatic fun premain(argumentsString: String?, instrumentation: Instrumentation)</ID>
@@ -196,6 +197,7 @@
     <ID>ComplexMethod:TlsDiffAlgorithmsTest.kt$TlsDiffAlgorithmsTest$@Test fun testClientServerTlsExchange()</ID>
     <ID>ComplexMethod:TlsDiffProtocolsTest.kt$TlsDiffProtocolsTest$@Test fun testClientServerTlsExchange()</ID>
     <ID>ComplexMethod:TransactionUtils.kt$ fun createComponentGroups(inputs: List&lt;StateRef&gt;, outputs: List&lt;TransactionState&lt;ContractState&gt;&gt;, commands: List&lt;Command&lt;*&gt;&gt;, attachments: List&lt;SecureHash&gt;, notary: Party?, timeWindow: TimeWindow?, references: List&lt;StateRef&gt;, networkParametersHash: SecureHash?): List&lt;ComponentGroup&gt;</ID>
+    <ID>ComplexMethod:TransitionExecutorImpl.kt$TransitionExecutorImpl$@Suppress("NestedBlockDepth", "ReturnCount") @Suspendable override fun executeTransition( fiber: FlowFiber, previousState: StateMachineState, event: Event, transition: TransitionResult, actionExecutor: ActionExecutor ): Pair&lt;FlowContinuation, StateMachineState&gt;</ID>
     <ID>ComplexMethod:TypeModellingFingerPrinter.kt$FingerPrintingState$// For a type we haven't seen before, determine the correct path depending on the type of type it is. private fun fingerprintNewType(type: LocalTypeInformation)</ID>
     <ID>ComplexMethod:UniversalContract.kt$UniversalContract$override fun verify(tx: LedgerTransaction)</ID>
     <ID>ComplexMethod:Util.kt$fun &lt;T&gt; debugCompare(perLeft: Perceivable&lt;T&gt;, perRight: Perceivable&lt;T&gt;)</ID>
@@ -3975,6 +3977,7 @@
     <ID>TooGenericExceptionCaught:DriverDSLImpl.kt$exception: Throwable</ID>
     <ID>TooGenericExceptionCaught:DriverTests.kt$DriverTests$e: Exception</ID>
     <ID>TooGenericExceptionCaught:ErrorCodeLoggingTests.kt$e: Exception</ID>
+    <ID>TooGenericExceptionCaught:ErrorHandling.kt$ErrorHandling.CheckpointAfterErrorFlow$t: Throwable</ID>
     <ID>TooGenericExceptionCaught:EventProcessor.kt$EventProcessor$ex: Exception</ID>
     <ID>TooGenericExceptionCaught:Eventually.kt$e: Exception</ID>
     <ID>TooGenericExceptionCaught:Expect.kt$exception: Exception</ID>
@@ -4109,6 +4112,7 @@
     <ID>TooGenericExceptionThrown:CrossCashTest.kt$throw Exception( "Generated payment of ${request.amount} from $issuer, " + "however they only have $issuerQuantity!" )</ID>
     <ID>TooGenericExceptionThrown:CrossCashTest.kt$throw Exception( "Generated payment of ${request.amount} from ${node.mainIdentity}, " + "however there is no cash from $issuer!" )</ID>
     <ID>TooGenericExceptionThrown:CrossCashTest.kt$throw Exception( "Generated payment of ${request.amount} from ${node.mainIdentity}, " + "however they only have $senderQuantity!" )</ID>
+    <ID>TooGenericExceptionThrown:DbListenerService.kt$DbListenerService$throw Exception("Mother of all exceptions")</ID>
     <ID>TooGenericExceptionThrown:FlowAsyncOperationTests.kt$FlowAsyncOperationTests.ErroredExecute$throw Exception()</ID>
     <ID>TooGenericExceptionThrown:FlowFrameworkTests.kt$FlowFrameworkTests$throw Exception("Error")</ID>
     <ID>TooGenericExceptionThrown:Generator.kt$Generator$throw Exception("Failed to generate", error)</ID>

node-api/src/main/kotlin/net/corda/nodeapi/internal/persistence/CordaPersistence.kt

@@ -4,6 +4,7 @@ import co.paralleluniverse.strands.Strand
 import com.zaxxer.hikari.HikariDataSource
 import com.zaxxer.hikari.pool.HikariPool
 import com.zaxxer.hikari.util.ConcurrentBag
+import net.corda.core.flows.HospitalizeFlowException
 import net.corda.core.internal.NamedCacheFactory
 import net.corda.core.schemas.MappedSchema
 import net.corda.core.utilities.contextLogger
@@ -100,7 +101,7 @@ class CordaPersistence(
         attributeConverters: Collection<AttributeConverter<*, *>> = emptySet(),
         customClassLoader: ClassLoader? = null,
         val closeConnection: Boolean = true,
-        val errorHandler: (t: Throwable) -> Unit = {}
+        val errorHandler: DatabaseTransaction.(e: Exception) -> Unit = {}
 ) : Closeable {
     companion object {
         private val log = contextLogger()
@@ -191,17 +192,17 @@ class CordaPersistence(
     }
 
     fun createSession(): Connection {
+        // We need to set the database for the current [Thread] or [Fiber] here as some tests share threads across databases.
+        _contextDatabase.set(this)
+        val transaction = contextTransaction
         try {
-            // We need to set the database for the current [Thread] or [Fiber] here as some tests share threads across databases.
-            _contextDatabase.set(this)
-            currentDBSession().flush()
-            return contextTransaction.connection
-        } catch (sqlException: SQLException) {
-            errorHandler(sqlException)
-            throw sqlException
-        } catch (persistenceException: PersistenceException) {
-            errorHandler(persistenceException)
-            throw persistenceException
+            transaction.session.flush()
+            return transaction.connection
+        } catch (e: Exception) {
+            if (e is SQLException || e is PersistenceException) {
+                transaction.errorHandler(e)
+            }
+            throw e
         }
     }
 
@@ -230,18 +231,22 @@ class CordaPersistence(
                         recoverAnyNestedSQLException: Boolean, statement: DatabaseTransaction.() -> T): T {
         _contextDatabase.set(this)
         val outer = contextTransactionOrNull
-        try {
-            return if (outer != null) {
+        return if (outer != null) {
+            // we only need to handle errors coming out of inner transactions because,
+            // a. whenever this code is being executed within the flow state machine, a top level transaction should have
+            // previously been created by the flow state machine in ActionExecutorImpl#executeCreateTransaction
+            // b. exceptions coming out from top level transactions are already being handled in CordaPersistence#inTopLevelTransaction
+            // i.e. roll back and close the transaction
+            try {
                 outer.statement()
-            } else {
-                inTopLevelTransaction(isolationLevel, recoverableFailureTolerance, recoverAnyNestedSQLException, statement)
+            } catch (e: Exception) {
+                if (e is SQLException || e is PersistenceException || e is HospitalizeFlowException) {
+                    outer.errorHandler(e)
+                }
+                throw e
             }
-        } catch (sqlException: SQLException) {
-            errorHandler(sqlException)
-            throw sqlException
-        } catch (persistenceException: PersistenceException) {
-            errorHandler(persistenceException)
-            throw persistenceException
+        } else {
+            inTopLevelTransaction(isolationLevel, recoverableFailureTolerance, recoverAnyNestedSQLException, statement)
         }
     }
 

node-api/src/main/kotlin/net/corda/nodeapi/internal/persistence/DatabaseTransaction.kt

@@ -1,7 +1,7 @@
 package net.corda.nodeapi.internal.persistence
 
 import co.paralleluniverse.strands.Strand
-import org.hibernate.BaseSessionEventListener
+import net.corda.core.CordaRuntimeException
 import org.hibernate.Session
 import org.hibernate.Transaction
 import rx.subjects.PublishSubject
@@ -51,7 +51,27 @@ class DatabaseTransaction(
     private var committed = false
     private var closed = false
 
+    /**
+     * Holds the first exception thrown from a series of statements executed in the same [DatabaseTransaction].
+     * The purpose of this property is to make sure this exception cannot be suppressed in user code.
+     * The exception will be thrown on the next [commit]. It is used only inside a flow state machine execution.
+     */
+    private var firstExceptionInDatabaseTransaction: Exception? = null
+
+    fun setException(e: Exception) {
+        if (firstExceptionInDatabaseTransaction == null) {
+            firstExceptionInDatabaseTransaction = e
+        }
+    }
+
+    private fun clearException() {
+        firstExceptionInDatabaseTransaction = null
+    }
+
     fun commit() {
+        firstExceptionInDatabaseTransaction?.let {
+            throw DatabaseTransactionException(it)
+        }
         if (sessionDelegate.isInitialized()) {
             hibernateTransaction.commit()
         }
@@ -66,16 +86,18 @@ class DatabaseTransaction(
         if (!connection.isClosed) {
             connection.rollback()
         }
+        clearException()
     }
 
     fun close() {
         if (sessionDelegate.isInitialized() && session.isOpen) {
             session.close()
         }
-
         if (database.closeConnection) {
             connection.close()
         }
+        clearException()
+
         contextTransactionOrNull = outerTransaction
         if (outerTransaction == null) {
             synchronized(this) {
@@ -99,3 +121,7 @@ class DatabaseTransaction(
     }
 }
 
+/**
+ * Wrapper exception, for any exception registered as [DatabaseTransaction.firstExceptionInDatabaseTransaction].
+ */
+class DatabaseTransactionException(override val cause: Throwable): CordaRuntimeException(cause.message, cause)