make sure to close and destroy the database before removing files from disk

lib/core/src/Cardano/DB/Sqlite.hs

@@ -44,6 +44,8 @@ import Cardano.Wallet.Logging
     ( logTrace )
 import Control.Concurrent.MVar
     ( newMVar, withMVar )
+import Control.Exception
+    ( throwIO )
 import Control.Monad
     ( mapM_ )
 import Control.Monad.Catch
@@ -129,7 +131,14 @@ handleConstraint e = handleJust select handler . fmap Right
 destroyDBLayer :: SqliteContext -> IO ()
 destroyDBLayer (SqliteContext {getSqlBackend, trace, dbFile}) = do
     logDebug trace (MsgClosing dbFile)
-    close' getSqlBackend
+    tryClose
+  where
+    tryClose = close' getSqlBackend
+        `catch` handleBusy
+
+    handleBusy e@(SqliteException name _ _) = case name of
+        Sqlite.ErrorBusy -> tryClose
+        _ -> throwIO e
 
 {-------------------------------------------------------------------------------
                            Internal / Database Setup

lib/core/src/Cardano/Wallet/Api/Server.hs

@@ -481,9 +481,9 @@ deleteWallet
     -> ApiT WalletId
     -> Handler NoContent
 deleteWallet ctx (ApiT wid) = do
-    liftHandler $ withWorkerCtx ctx wid throwE $ \wrk -> W.deleteWallet wrk wid
-    liftIO $ Registry.remove re wid
+    liftHandler $ withWorkerCtx @_ @s @k ctx wid throwE $ \_ -> pure ()
     liftIO $ (df ^. #removeDatabase) wid
+    liftIO $ Registry.remove re wid
     return NoContent
   where
     re = ctx ^. workerRegistry @s @k

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

@@ -23,7 +23,7 @@
 
 module Cardano.Wallet.DB.Sqlite
     ( newDBLayer
-    , mkDBFactory
+    , newDBFactory
     , findDatabases
     , withDBLayer
 
@@ -91,6 +91,8 @@ import Cardano.Wallet.Primitive.AddressDiscovery
     ( IsOurs (..) )
 import Control.Arrow
     ( (***) )
+import Control.Concurrent.MVar
+    ( newEmptyMVar, putMVar, readMVar )
 import Control.DeepSeq
     ( NFData )
 import Control.Exception
@@ -190,17 +192,17 @@ withDBLayer
        -- ^ Logging object
     -> Maybe FilePath
        -- ^ Path to database directory, or Nothing for in-memory database
-    -> (DBLayer IO s k -> IO a)
+    -> ((SqliteContext, DBLayer IO s k) -> IO a)
        -- ^ Action to run.
     -> IO a
-withDBLayer logConfig trace mDatabaseDir action =
-    bracket before after (action . snd)
+withDBLayer logConfig trace mDatabaseDir =
+    bracket before after
   where
     before = newDBLayer logConfig trace mDatabaseDir
     after = destroyDBLayer . fst
 
 -- | Instantiate a 'DBFactory' from a given directory
-mkDBFactory
+newDBFactory
     :: forall s k.
         ( IsOurs s W.Address
         , IsOurs s W.ChimericAccount
@@ -216,33 +218,38 @@ mkDBFactory
        -- ^ Logging object
     -> Maybe FilePath
        -- ^ Path to database directory, or Nothing for in-memory database
-    -> DBFactory IO s k
-mkDBFactory cfg tr mDatabaseDir = case mDatabaseDir of
-    Nothing -> DBFactory
-        { withDatabase = \_ ->
-            withDBLayer cfg tracerDB Nothing
-        , removeDatabase = \_ ->
-            pure ()
-        }
-    Just databaseDir -> DBFactory
-        { withDatabase = \wid ->
-            withDBLayer cfg tracerDB (Just $ databaseFile wid)
-        , removeDatabase = \wid -> do
-            let files =
-                    [ databaseFile wid
-                    , databaseFile wid <> "-wal"
-                    , databaseFile wid <> "-shm"
-                    ]
-            mapM_ removePathForcibly files
-        }
+    -> IO (DBFactory IO s k)
+newDBFactory cfg tr mDatabaseDir = do
+    mvar <- newEmptyMVar
+    case mDatabaseDir of
+        Nothing -> pure DBFactory
+            { withDatabase = \_ action ->
+                withDBLayer cfg tracerDB Nothing (action . snd)
+            , removeDatabase = \_ ->
+                pure ()
+            }
+        Just databaseDir -> pure DBFactory
+            { withDatabase = \wid action ->
+                withDBLayer cfg tracerDB (Just $ databaseFile wid) $ \(ctx,db) -> do
+                    putMVar mvar ctx
+                    action db
+            , removeDatabase = \wid -> do
+                let files =
+                        [ databaseFile wid
+                        , databaseFile wid <> "-wal"
+                        , databaseFile wid <> "-shm"
+                        ]
+                readMVar mvar >>= destroyDBLayer
+                mapM_ removePathForcibly files
+            }
+          where
+            databaseFilePrefix = keyTypeDescriptor $ Proxy @k
+            databaseFile wid =
+                databaseDir </>
+                databaseFilePrefix <> "." <>
+                T.unpack (toText wid) <> ".sqlite"
       where
-        databaseFilePrefix = keyTypeDescriptor $ Proxy @k
-        databaseFile wid =
-            databaseDir </>
-            databaseFilePrefix <> "." <>
-            T.unpack (toText wid) <> ".sqlite"
-  where
-    tracerDB = appendName "database" tr
+        tracerDB = appendName "database" tr
 
 -- | Return all wallet databases that match the specified key type within the
 --   specified directory.

lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs

@@ -51,13 +51,18 @@ import Cardano.Crypto.Wallet
 import Cardano.DB.Sqlite
     ( SqliteContext, destroyDBLayer )
 import Cardano.Wallet.DB
-    ( DBLayer (..), ErrNoSuchWallet (..), PrimaryKey (..), cleanDB )
+    ( DBFactory (..)
+    , DBLayer (..)
+    , ErrNoSuchWallet (..)
+    , PrimaryKey (..)
+    , cleanDB
+    )
 import Cardano.Wallet.DB.Arbitrary
     ( KeyValPairs (..) )
 import Cardano.Wallet.DB.Properties
     ( properties, withDB )
 import Cardano.Wallet.DB.Sqlite
-    ( PersistState, newDBLayer, withDBLayer )
+    ( PersistState, newDBFactory, newDBLayer, withDBLayer )
 import Cardano.Wallet.DB.StateMachine
     ( prop_parallel, prop_sequential )
 import Cardano.Wallet.DummyTarget.Primitive.Types
@@ -106,12 +111,14 @@ import Cardano.Wallet.Primitive.Types
     )
 import Cardano.Wallet.Unsafe
     ( unsafeRunExceptT )
+import Control.Concurrent
+    ( forkIO )
 import Control.DeepSeq
     ( NFData )
 import Control.Exception
     ( throwIO )
 import Control.Monad
-    ( forM_, replicateM_, unless )
+    ( forM_, forever, replicateM_, unless )
 import Control.Monad.IO.Class
     ( liftIO )
 import Control.Monad.Trans.Except
@@ -137,13 +144,13 @@ import Data.Time.Clock
 import GHC.Conc
     ( TVar, newTVarIO, readTVarIO, writeTVar )
 import System.Directory
-    ( doesFileExist, removeFile )
+    ( doesFileExist, listDirectory, removeFile )
 import System.IO
     ( hClose )
 import System.IO.Error
     ( isUserError )
 import System.IO.Temp
-    ( emptySystemTempFile, withSystemTempFile )
+    ( emptySystemTempFile, withSystemTempDirectory, withSystemTempFile )
 import System.IO.Unsafe
     ( unsafePerformIO )
 import System.Random
@@ -174,6 +181,7 @@ import qualified Cardano.BM.Data.AggregatedKind as CM
 import qualified Cardano.BM.Data.Backend as CM
 import qualified Cardano.BM.Data.SubTrace as CM
 import qualified Cardano.Wallet.Primitive.AddressDerivation.Shelley as Seq
+import qualified Data.ByteString.Char8 as B8
 import qualified Data.HashMap.Strict as HM
 import qualified Data.List as L
 import qualified Data.Set as Set
@@ -343,6 +351,23 @@ fileModeSpec =  do
                 (ctx, _) <- newDBLayer' @(SeqState 'Testnet ShelleyKey) db
                 destroyDBLayer ctx
 
+    describe "DBFactory" $ do
+        it "withDatabase *> removeDatabase works and remove files" $ do
+            withSystemTempDirectory "DBFactory" $ \dir -> do
+                cfg <- defaultConfigTesting
+                DBFactory{..} <- newDBFactory cfg nullTracer (Just dir)
+
+                -- NOTE
+                -- Start a concurrent worker which makes action on the DB in
+                -- parallel to simulate activity.
+                _ <- forkIO $ withDatabase testWid $ \(DBLayer{..} :: TestDBSeq) -> do
+                    forever $ do
+                        cp <- atomically $ readCheckpoint $ PrimaryKey testWid
+                        B8.putStrLn (B8.pack $ show cp)
+
+                removeDatabase testWid
+                listDirectory dir `shouldReturn` mempty
+
     describe "Sqlite database file" $ do
         let writeSomething DBLayer{..} = do
                 atomically $ unsafeRunExceptT $
@@ -511,7 +536,7 @@ withTestDBFile action expectations = do
     withSystemTempFile "spec.db" $ \fp handle -> do
         hClose handle
         removeFile fp
-        withDBLayer logConfig trace (Just fp) action
+        withDBLayer logConfig trace (Just fp) (action . snd)
         expectations fp
 
 inMemoryDBLayer

lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs

@@ -51,8 +51,6 @@ import Cardano.Wallet.Api.Types
     ( DecodeAddress, EncodeAddress )
 import Cardano.Wallet.DaedalusIPC
     ( daedalusIPC )
-import Cardano.Wallet.DB
-    ( DBFactory )
 import Cardano.Wallet.DB.Sqlite
     ( PersistState )
 import Cardano.Wallet.Jormungandr.Compatibility
@@ -236,8 +234,9 @@ serveWallet
     apiLayer tracer tl nl = do
         let (block0, bp) = staticBlockchainParameters nl
         wallets <- maybe (pure []) (Sqlite.findDatabases @k trText) databaseDir
+        db <- Sqlite.newDBFactory cfg trText databaseDir
         Server.newApiLayer
-            tracer (toWLBlock block0, bp, sTolerance) nl' tl dbFactory wallets
+            tracer (toWLBlock block0, bp, sTolerance) nl' tl db wallets
       where
         nl' = toWLBlock <$> nl
 
@@ -256,19 +255,6 @@ serveWallet
         onExit = defaultWorkerAfter tr'
         trStakePool = contramap (fmap LogStakePoolLayerMsg) trRoot
 
-    dbFactory
-        :: forall s k.
-            ( IsOurs s Address
-            , IsOurs s ChimericAccount
-            , NFData s
-            , Show s
-            , PersistState s
-            , PersistPrivateKey (k 'RootK)
-            , WalletKey k
-            )
-        => DBFactory IO s k
-    dbFactory = Sqlite.mkDBFactory cfg trText databaseDir
-
     handleNetworkStartupError :: ErrStartup -> IO ExitCode
     handleNetworkStartupError = \case
         ErrStartupGetBlockchainParameters e -> case e of