From 3cb996b8148840b1346a7c7fc107b2a9bcdb80bd Mon Sep 17 00:00:00 2001
From: jakubs <jakubs>
Date: Wed, 16 May 2012 14:09:40 +0000
Subject: [PATCH] SP-45, BIS-21 - faulty path handler an recognize of the file
 cannot be deleted due to autorecovery - improve cleanup of registration and
 recovery process

SVN: 25285
---
 .../ch/systemsx/cisd/etlserver/ETLDaemon.java | 19 +++--
 .../ITopLevelDataSetRegistrator.java          |  4 +-
 .../etlserver/TransferredDataSetHandler.java  | 11 ++-
 .../api/v1/PutDataSetServerPluginHolder.java  |  5 ++
 ...tOmniscientTopLevelDataSetRegistrator.java | 29 +++++--
 .../DataSetStorageRecoveryManager.java        | 40 +++++++--
 .../IDataSetStorageRecoveryManager.java       |  7 ++
 .../etlserver/registrator/IRollbackStack.java |  2 +-
 .../JythonTopLevelDataSetHandler.java         | 20 +++--
 .../impl/DataSetRegistrationTransaction.java  | 19 +++--
 .../v2/JythonTopLevelDataSetHandlerV2.java    | 11 +++
 .../DataSetStorageRollbackerTest.java         |  8 ++
 .../JythonDropboxRecoveryTest.java            | 83 +++++++++++++++----
 .../JythonTopLevelDataSetRegistratorTest.java | 38 +++++++--
 .../DataSetRegistrationTransactionTest.java   |  8 ++
 15 files changed, 245 insertions(+), 59 deletions(-)

diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/ETLDaemon.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/ETLDaemon.java
index ac33d5450cb..772ef2f24cd 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/ETLDaemon.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/ETLDaemon.java
@@ -43,6 +43,7 @@ import ch.systemsx.cisd.common.exceptions.HighLevelException;
 import ch.systemsx.cisd.common.exceptions.Status;
 import ch.systemsx.cisd.common.filesystem.DirectoryScanningTimerTask;
 import ch.systemsx.cisd.common.filesystem.DirectoryScanningTimerTask.IScannedStore;
+import ch.systemsx.cisd.common.filesystem.FaultyPathDirectoryScanningHandler.IFaultyPathDirectoryScanningHandlerDelegate;
 import ch.systemsx.cisd.common.filesystem.FaultyPathDirectoryScanningHandler;
 import ch.systemsx.cisd.common.filesystem.FileUtilities;
 import ch.systemsx.cisd.common.filesystem.IDirectoryScanningHandler;
@@ -325,7 +326,8 @@ public final class ETLDaemon
                         dataSourceQueryService, notifySuccessfulRegistration);
         final HighwaterMarkDirectoryScanningHandler directoryScanningHandler =
                 createDirectoryScanningHandler(pathHandler, highwaterMarkWatcher,
-                        incomingDataDirectory, threadParameters.reprocessFaultyDatasets());
+                        incomingDataDirectory, threadParameters.reprocessFaultyDatasets(),
+                        pathHandler);
         FileFilter fileFilter =
                 createFileFilter(incomingDataDirectory, threadParameters.useIsFinishedMarkerFile(),
                         parameters);
@@ -551,25 +553,28 @@ public final class ETLDaemon
 
     private final static HighwaterMarkDirectoryScanningHandler createDirectoryScanningHandler(
             final IStopSignaler stopSignaler, final HighwaterMarkWatcher highwaterMarkWatcher,
-            final File incomingDataDirectory, boolean reprocessFaultyDatasets)
+            final File incomingDataDirectory, boolean reprocessFaultyDatasets,
+            IFaultyPathDirectoryScanningHandlerDelegate faultyPathHandlerDelegate)
     {
         final IDirectoryScanningHandler faultyPathHandler =
                 createFaultyPathHandler(stopSignaler, incomingDataDirectory,
-                        reprocessFaultyDatasets);
+                        reprocessFaultyDatasets, faultyPathHandlerDelegate);
         return new HighwaterMarkDirectoryScanningHandler(faultyPathHandler, highwaterMarkWatcher,
                 incomingDataDirectory);
     }
 
     private static IDirectoryScanningHandler createFaultyPathHandler(
             final IStopSignaler stopSignaler, final File incomingDataDirectory,
-            boolean reprocessFaultyDatasets)
+            boolean reprocessFaultyDatasets,
+            IFaultyPathDirectoryScanningHandlerDelegate faultyPathHandlerDelegate)
     {
         if (reprocessFaultyDatasets)
         {
             return createDummyFaultyPathHandler();
         } else
         {
-            return new FaultyPathDirectoryScanningHandler(incomingDataDirectory, stopSignaler);
+            return new FaultyPathDirectoryScanningHandler(incomingDataDirectory, stopSignaler,
+                    faultyPathHandlerDelegate);
         }
     }
 
@@ -623,7 +628,9 @@ public final class ETLDaemon
         TimingParameters.setDefault(parameters.getTimingParameters());
         if (QueueingPathRemoverService.isRunning() == false)
         {
-            QueueingPathRemoverService.start(DssPropertyParametersUtil.getStoreRootDir(parameters.getProperties()), shredderQueueFile);
+            QueueingPathRemoverService.start(
+                    DssPropertyParametersUtil.getStoreRootDir(parameters.getProperties()),
+                    shredderQueueFile);
         }
         if (QueueingDataSetStatusUpdaterService.isRunning() == false)
         {
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/ITopLevelDataSetRegistrator.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/ITopLevelDataSetRegistrator.java
index faa8d26d797..c93de2bd194 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/ITopLevelDataSetRegistrator.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/ITopLevelDataSetRegistrator.java
@@ -20,6 +20,7 @@ import java.io.File;
 import java.util.concurrent.locks.Lock;
 
 import ch.systemsx.cisd.common.filesystem.IPathHandler;
+import ch.systemsx.cisd.common.filesystem.FaultyPathDirectoryScanningHandler.IFaultyPathDirectoryScanningHandlerDelegate;
 import ch.systemsx.cisd.common.utilities.ISelfTestable;
 import ch.systemsx.cisd.openbis.dss.generic.shared.dto.DataSetInformation;
 
@@ -28,7 +29,8 @@ import ch.systemsx.cisd.openbis.dss.generic.shared.dto.DataSetInformation;
  * 
  * @author Chandrasekhar Ramakrishnan
  */
-public interface ITopLevelDataSetRegistrator extends IPathHandler, ISelfTestable
+public interface ITopLevelDataSetRegistrator extends IPathHandler, ISelfTestable,
+        IFaultyPathDirectoryScanningHandlerDelegate
 {
     /**
      * A lock used to synchronize shutting down the processing thread of the top-level data set
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/TransferredDataSetHandler.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/TransferredDataSetHandler.java
index 0a8cedb44bb..a3a45ba1fb9 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/TransferredDataSetHandler.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/TransferredDataSetHandler.java
@@ -347,7 +347,8 @@ public final class TransferredDataSetHandler extends AbstractTopLevelDataSetRegi
             File incomingDataSetFile, final DataSetInformation dsInfo,
             DataSetRegistrationAlgorithm.IDataSetInApplicationServerRegistrator registrator)
     {
-        IDelegatedActionWithResult<Boolean> cleanAftrewardsAction = new AbstractDelegatedActionWithResult<Boolean>(true);
+        IDelegatedActionWithResult<Boolean> cleanAftrewardsAction =
+                new AbstractDelegatedActionWithResult<Boolean>(true);
         if (registrator != null)
         {
             return new OverridingRegistrationHelper(this, incomingDataSetFile, getGlobalState()
@@ -546,4 +547,12 @@ public final class TransferredDataSetHandler extends AbstractTopLevelDataSetRegi
     {
         return getGlobalState().getStoreRootDir();
     }
+
+    /**
+     * Any path can be added to faulty paths as far as we are concerned.
+     */
+    public boolean shouldNotAddToFaultyPathsOrNull(File storeItem)
+    {
+        return false;
+    }
 }
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/api/v1/PutDataSetServerPluginHolder.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/api/v1/PutDataSetServerPluginHolder.java
index f3ab7d1b475..3922881f6ed 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/api/v1/PutDataSetServerPluginHolder.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/api/v1/PutDataSetServerPluginHolder.java
@@ -86,4 +86,9 @@ public class PutDataSetServerPluginHolder extends AbstractTopLevelDataSetRegistr
         throw new NotImplementedException();
     }
 
+    public boolean shouldNotAddToFaultyPathsOrNull(File storeItem)
+    {
+        throw new NotImplementedException();
+    }
+
 }
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/AbstractOmniscientTopLevelDataSetRegistrator.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/AbstractOmniscientTopLevelDataSetRegistrator.java
index 14ee954b79a..8fc4b2ec2a0 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/AbstractOmniscientTopLevelDataSetRegistrator.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/AbstractOmniscientTopLevelDataSetRegistrator.java
@@ -310,7 +310,7 @@ public abstract class AbstractOmniscientTopLevelDataSetRegistrator<T extends Dat
         return state.getGlobalState().getStorageRecoveryManager().isRecoveryFile(incoming);
     }
 
-    private boolean hasRecoveryMarkerFile(File incoming)
+    protected boolean hasRecoveryMarkerFile(File incoming)
     {
         return new File(incoming.getAbsolutePath()
                 + IDataSetStorageRecoveryManager.PROCESSING_MARKER).exists();
@@ -362,10 +362,12 @@ public abstract class AbstractOmniscientTopLevelDataSetRegistrator<T extends Dat
                 {
                     public Boolean execute(boolean didOperationSucceed)
                     {
-                        boolean markerDeleteSucceeded =
-                                state.getMarkerFileUtility().deleteAndLogIsFinishedMarkerFile(
-                                        incomingDataSetFileOrIsFinishedFile);
-                        return markerDeleteSucceeded;
+                        if (hasRecoveryMarkerFile(incomingDataSetFile))
+                        {
+                            return true;
+                        }
+                        return state.getMarkerFileUtility().deleteAndLogIsFinishedMarkerFile(
+                                incomingDataSetFileOrIsFinishedFile);
                     }
                 };
         } else
@@ -487,7 +489,7 @@ public abstract class AbstractOmniscientTopLevelDataSetRegistrator<T extends Dat
 
     private void handleRecovery(final File recoveryMarkerFile)
     {
-        DataSetStoragePrecommitRecoveryState<T> recoveryState =
+        final DataSetStoragePrecommitRecoveryState<T> recoveryState =
                 state.getGlobalState().getStorageRecoveryManager()
                         .extractPrecommittedCheckpoint(recoveryMarkerFile);
 
@@ -513,8 +515,19 @@ public abstract class AbstractOmniscientTopLevelDataSetRegistrator<T extends Dat
                     {
                         public Boolean execute(boolean didOperationSucceed)
                         {
-                            recoveryMarkerFile.delete();
-                            recoveryFile.delete();
+                            if (didOperationSucceed)
+                            {
+                                File incomingMarkerFile =
+                                        MarkerFileUtility.getMarkerFileFromIncoming(recoveryState
+                                                .getIncomingDataSetFile().getRealIncomingFile());
+                                if (incomingMarkerFile.exists())
+                                {
+                                    incomingMarkerFile.delete();
+                                }
+
+                                recoveryMarkerFile.delete();
+                                recoveryFile.delete();
+                            }
                             return true;
                         }
                     };
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/DataSetStorageRecoveryManager.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/DataSetStorageRecoveryManager.java
index 128c2843092..2bf89ae0cc3 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/DataSetStorageRecoveryManager.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/DataSetStorageRecoveryManager.java
@@ -18,7 +18,12 @@ package ch.systemsx.cisd.etlserver.registrator;
 
 import java.io.File;
 
+import org.apache.log4j.Logger;
+
+import ch.systemsx.cisd.common.exceptions.UserFailureException;
 import ch.systemsx.cisd.common.filesystem.FileUtilities;
+import ch.systemsx.cisd.common.logging.LogCategory;
+import ch.systemsx.cisd.common.logging.LogFactory;
 import ch.systemsx.cisd.openbis.dss.generic.shared.dto.DataSetInformation;
 
 /**
@@ -28,6 +33,9 @@ import ch.systemsx.cisd.openbis.dss.generic.shared.dto.DataSetInformation;
  */
 public class DataSetStorageRecoveryManager implements IDataSetStorageRecoveryManager
 {
+    static final Logger operationLog = LogFactory.getLogger(LogCategory.OPERATION,
+            DataSetStorageRecoveryManager.class);
+
     private File dropboxRecoveryStateDir;
 
     public <T extends DataSetInformation> void checkpointPrecommittedState(
@@ -42,12 +50,20 @@ public class DataSetStorageRecoveryManager implements IDataSetStorageRecoveryMan
                         runner.getDssRegistrationLogger(), runner.getRollbackStack(), incoming);
 
         runner.getRollbackStack().setLockedState(true);
-        
+
         FileUtilities.writeToFile(serializedFile, recoveryState);
 
         File processingMarkerFile = getProcessingMarkerFile(runner);
         FileUtilities.writeToFile(processingMarkerFile, serializedFile.getAbsolutePath());
 
+        operationLog.info("Store precommit recovery checkpoint with markerfile "
+                + processingMarkerFile);
+    }
+
+    public <T extends DataSetInformation> void removeCheckpoint(
+            DataSetStorageAlgorithmRunner<T> runner)
+    {
+        cleanup(runner);
     }
 
     private <T extends DataSetInformation> File getProcessingMarkerFile(
@@ -72,29 +88,40 @@ public class DataSetStorageRecoveryManager implements IDataSetStorageRecoveryMan
         String recoveryFilePath = FileUtilities.loadToString(markerFile).trim();
         return new File(recoveryFilePath);
     }
-    
+
     @SuppressWarnings("unchecked")
     public <T extends DataSetInformation> DataSetStoragePrecommitRecoveryState<T> extractPrecommittedCheckpoint(
             File markerFile)
     {
         File recoveryFile = getRecoveryFileFromMarker(markerFile);
-        return FileUtilities.loadToObject(recoveryFile,
-                DataSetStoragePrecommitRecoveryState.class);
+        return FileUtilities.loadToObject(recoveryFile, DataSetStoragePrecommitRecoveryState.class);
     }
 
     public <T extends DataSetInformation> void registrationCompleted(
             DataSetStorageAlgorithmRunner<T> runner)
     {
+        cleanup(runner);
+    }
+
+    public <T extends DataSetInformation> void cleanup(DataSetStorageAlgorithmRunner<T> runner)
+    {
+        File markerFile = getProcessingMarkerFile(runner);
+        File recoveryState = getSerializedFile(runner);
+        
+        operationLog.info("Cleanup recovery with marker file "+ markerFile);
+        
         runner.getRollbackStack().setLockedState(false);
         // Cleanup the state we have accumulated
-        File markerFile = getProcessingMarkerFile(runner);
         FileUtilities.delete(markerFile);
-        File recoveryState = getSerializedFile(runner);
         FileUtilities.delete(recoveryState);
     }
 
     public boolean canRecoverFromError(Throwable ex)
     {
+        if (ex instanceof UserFailureException)
+        {
+            return false;
+        }
         return true;
     }
 
@@ -107,4 +134,5 @@ public class DataSetStorageRecoveryManager implements IDataSetStorageRecoveryMan
     {
         this.dropboxRecoveryStateDir = dropboxRecoveryStateDir;
     }
+
 }
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/IDataSetStorageRecoveryManager.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/IDataSetStorageRecoveryManager.java
index 44d2fe43185..425813a15c4 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/IDataSetStorageRecoveryManager.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/IDataSetStorageRecoveryManager.java
@@ -38,6 +38,13 @@ public interface IDataSetStorageRecoveryManager
     <T extends DataSetInformation> void checkpointPrecommittedState(
             DataSetStorageAlgorithmRunner<T> runner);
 
+    /**
+     * Remove recovery checkpoint.
+     */
+    <T extends DataSetInformation> void removeCheckpoint(
+            DataSetStorageAlgorithmRunner<T> runner);
+
+    
     /**
      * Note that registration has completed.
      */
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/IRollbackStack.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/IRollbackStack.java
index 819f12e70b7..01a04ddd773 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/IRollbackStack.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/IRollbackStack.java
@@ -32,7 +32,7 @@ public interface IRollbackStack
     
     /**
      * Sets the locked state of this rollback stack. Changing this state to true results in creating
-     * or deleting the marker file.
+     * or deleting the marker file. If already in a desired state - does nothing.
      */
     public void setLockedState(boolean lockedState);
     
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/JythonTopLevelDataSetHandler.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/JythonTopLevelDataSetHandler.java
index 2d064acb568..e4c47cd9499 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/JythonTopLevelDataSetHandler.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/JythonTopLevelDataSetHandler.java
@@ -304,8 +304,7 @@ public class JythonTopLevelDataSetHandler<T extends DataSetInformation> extends
         invokeDidEncounterSecondaryTransactionErrorsFunction(service, transaction, secondaryErrors);
     }
 
-    
-    //getters for v2 hook functions required for auto-recovery
+    // getters for v2 hook functions required for auto-recovery
     public PyFunction tryGetPostRegistrationFunction(DataSetRegistrationService<T> service)
     {
         PythonInterpreter interpreter = getInterpreterFromService(service);
@@ -313,7 +312,7 @@ public class JythonTopLevelDataSetHandler<T extends DataSetInformation> extends
                 tryJythonFunction(interpreter, JythonHookFunction.POST_REGISTRATION_FUNCTION_NAME);
         return function;
     }
-    
+
     public PyFunction tryGetPostStorageFunction(DataSetRegistrationService<T> service)
     {
         PythonInterpreter interpreter = getInterpreterFromService(service);
@@ -330,7 +329,7 @@ public class JythonTopLevelDataSetHandler<T extends DataSetInformation> extends
                         JythonHookFunction.ROLLBACK_PRE_REGISTRATION_FUNCTION_NAME);
         return function;
     }
-    
+
     /**
      * If true than the old methods of jython hook functions will also be used (as a fallbacks in
      * case of the new methods or missing, or normally)
@@ -380,9 +379,9 @@ public class JythonTopLevelDataSetHandler<T extends DataSetInformation> extends
             DataSetRegistrationTransaction<T> transaction)
     {
         PythonInterpreter interpreter = getInterpreterFromService(service);
-        
+
         PyFunction function = tryGetPostStorageFunction(service);
-        
+
         if (null != function)
         {
             invokeTransactionFunctionWithContext(function, service, transaction);
@@ -421,7 +420,6 @@ public class JythonTopLevelDataSetHandler<T extends DataSetInformation> extends
         }
     }
 
-
     private void invokeDidEncounterSecondaryTransactionErrorsFunction(
             DataSetRegistrationService<T> service, DataSetRegistrationTransaction<T> transaction,
             List<SecondaryTransactionFailure> secondaryErrors)
@@ -600,4 +598,12 @@ public class JythonTopLevelDataSetHandler<T extends DataSetInformation> extends
         return super.asSerializableException(throwable);
     }
 
+    /**
+     * V1 registration framework -- any file can go into faulty paths.
+     */
+    public boolean shouldNotAddToFaultyPathsOrNull(File storeItem)
+    {
+        return false;
+    }
+
 }
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/api/v1/impl/DataSetRegistrationTransaction.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/api/v1/impl/DataSetRegistrationTransaction.java
index c2240a884df..845fab10e0d 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/api/v1/impl/DataSetRegistrationTransaction.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/api/v1/impl/DataSetRegistrationTransaction.java
@@ -446,18 +446,25 @@ public class DataSetRegistrationTransaction<T extends DataSetInformation> implem
     public void didRollbackStorageAlgorithmRunner(DataSetStorageAlgorithmRunner<T> algorithm,
             Throwable ex, ErrorType errorType)
     {
+
         boolean useAutoRecovery = autoRecoverySettings == AutoRecoverySettings.USE_AUTO_RECOVERY;
-        useAutoRecovery =
-                useAutoRecovery
-                        && errorType == ErrorType.OPENBIS_REGISTRATION_FAILURE
-                        && registrationService.getRegistratorContext().getGlobalState()
-                                .getStorageRecoveryManager().canRecoverFromError(ex);
-        if (useAutoRecovery)
+
+        IDataSetStorageRecoveryManager storageRecoveryManager =
+                registrationService.getRegistratorContext().getGlobalState()
+                        .getStorageRecoveryManager();
+        boolean canRecover =
+                useAutoRecovery && errorType == ErrorType.OPENBIS_REGISTRATION_FAILURE
+                        && storageRecoveryManager.canRecoverFromError(ex);
+        if (useAutoRecovery && canRecover)
         {
             registrationService.registerNonFatalError(ex);
             state = new RecoveryPendingTransactionState<T>(getStateAsLiveState());
         } else
         {
+            if (useAutoRecovery)
+            {
+                storageRecoveryManager.removeCheckpoint(algorithm);
+            }
             rollback();
             registrationService.didRollbackTransaction(this, algorithm, ex, errorType);
         }
diff --git a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/api/v2/JythonTopLevelDataSetHandlerV2.java b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/api/v2/JythonTopLevelDataSetHandlerV2.java
index cc818aca303..60d395a9184 100644
--- a/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/api/v2/JythonTopLevelDataSetHandlerV2.java
+++ b/datastore_server/source/java/ch/systemsx/cisd/etlserver/registrator/api/v2/JythonTopLevelDataSetHandlerV2.java
@@ -96,4 +96,15 @@ public class JythonTopLevelDataSetHandlerV2<T extends DataSetInformation> extend
     {
         return false;
     }
+
+    /**
+     * V2 registration framework -- do not put files that are scheduled for recovery into the faulty
+     * paths.
+     */
+    @Override
+    public boolean shouldNotAddToFaultyPathsOrNull(File file)
+    {
+        // If there is a recovery marker file, do not add the file to faulty paths.
+        return hasRecoveryMarkerFile(file);
+    }
 }
diff --git a/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/DataSetStorageRollbackerTest.java b/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/DataSetStorageRollbackerTest.java
index 7470373e040..d832d8840cd 100644
--- a/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/DataSetStorageRollbackerTest.java
+++ b/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/DataSetStorageRollbackerTest.java
@@ -150,6 +150,14 @@ public class DataSetStorageRollbackerTest extends AbstractFileSystemTestCase
         {
 
         }
+
+        /**
+         * V1 Rollbacker test -- any file can go into faulty paths.
+         */
+        public boolean shouldNotAddToFaultyPathsOrNull(File storeItem)
+        {
+            return false;
+        }
     }
 
     private void setUpHomeDataBaseExpectations()
diff --git a/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/JythonDropboxRecoveryTest.java b/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/JythonDropboxRecoveryTest.java
index c40a05c6547..f890bb52604 100644
--- a/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/JythonDropboxRecoveryTest.java
+++ b/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/JythonDropboxRecoveryTest.java
@@ -29,6 +29,8 @@ import org.jmock.Expectations;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
+import ch.systemsx.cisd.common.exceptions.EnvironmentFailureException;
+import ch.systemsx.cisd.common.exceptions.UserFailureException;
 import ch.systemsx.cisd.common.filesystem.FileUtilities;
 import ch.systemsx.cisd.common.test.RecordingMatcher;
 import ch.systemsx.cisd.openbis.generic.shared.basic.dto.Experiment;
@@ -57,7 +59,11 @@ public class JythonDropboxRecoveryTest extends AbstractJythonDataSetHandlerTest
         LinkedList<RecoveryTestCase> testCases = new LinkedList<RecoveryTestCase>();
         RecoveryTestCase testCase;
 
-        testCase = new RecoveryTestCase("Basic recovery testcase");
+        testCase = new RecoveryTestCase("basic recovery succeeded");
+        testCases.add(testCase);
+
+        testCase = new RecoveryTestCase("basic unrecoverable");
+        testCase.canRecoverFromError = false;
         testCases.add(testCase);
 
         // result value
@@ -100,15 +106,7 @@ public class JythonDropboxRecoveryTest extends AbstractJythonDataSetHandlerTest
          * If true than this registration has been succesfull. Which means that the recovery should
          * continue registration rather rollback.
          */
-        protected boolean registrationSuccesfull = true;
-
-        /**
-         * True if the registration should throw exception to the top level. With this setting the
-         * handler is said to throw all exception to the top level, so that we can catch them. To
-         * check how the system reacts to error's itself (like rollback mechanism) this should be
-         * set to false.
-         */
-        protected boolean shouldThrowExceptionDuringRegistration = true;
+        protected boolean registrationSuccessful = true;
 
         private RecoveryTestCase(String title)
         {
@@ -147,13 +145,32 @@ public class JythonDropboxRecoveryTest extends AbstractJythonDataSetHandlerTest
 
         // run the actual code
         handler.handle(markerFile);
+
         if (testCase.canRecoverFromError)
         {
-            File recoveryMarkerFile = getCreatedRecoveryMarkerFile();
+            File recoveryMarkerFile = assertRecoveryMarkerFile();
+            assertOriginalMarkerFileExists();
 
             handler.handle(recoveryMarkerFile);
+
+            //if failure happened here then don't expect recovery / marker files to be deleted
+            
+            if (testCase.registrationSuccessful )
+            {
+                //assert the item is in store and everything
+            }
+            else 
+            {
+                //nothing is is store, all is cleared
+            }
+            
+            assertNoOriginalMarkerFileExists();
+            assertNoRecoveryMarkerFile();
         } else
         {
+            assertNoOriginalMarkerFileExists();
+            assertNoRecoveryMarkerFile();
+            // assert there is no recovery file
             // rolllback requirementes
         }
 
@@ -166,6 +183,33 @@ public class JythonDropboxRecoveryTest extends AbstractJythonDataSetHandlerTest
 
     }
 
+    private void assertOriginalMarkerFileExists()
+    {
+        assertTrue(
+                "The original registration marker file should not be deleted when entering recovery mode",
+                markerFile.exists());
+    }
+
+    private void assertNoOriginalMarkerFileExists()
+    {
+        assertFalse(
+                "The original registration marker file should be deleted",
+                markerFile.exists());
+    }
+
+    private File assertRecoveryMarkerFile()
+    {
+        File file = getCreatedRecoveryMarkerFile();
+        assertTrue("The recovery marker file does not exist! " + file, file.exists());
+        return file;
+    }
+
+    private void assertNoRecoveryMarkerFile()
+    {
+        File file = getCreatedRecoveryMarkerFile();
+        assertTrue("The recovery marker file should not exist! " + file, false == file.exists());
+    }
+
     private File getCreatedRecoveryMarkerFile()
     {
         File originalIncoming =
@@ -173,8 +217,6 @@ public class JythonDropboxRecoveryTest extends AbstractJythonDataSetHandlerTest
         File recoveryMarkerFile =
                 new File(originalIncoming.getAbsolutePath()
                         + IDataSetStorageRecoveryManager.PROCESSING_MARKER);
-        assertTrue("The recovery marker file does not exist! " + recoveryMarkerFile,
-                recoveryMarkerFile.exists());
         return recoveryMarkerFile;
     }
 
@@ -210,7 +252,7 @@ public class JythonDropboxRecoveryTest extends AbstractJythonDataSetHandlerTest
             {
                 checkRegistrationSucceeded();
 
-                if (testCase.registrationSuccesfull)
+                if (testCase.registrationSuccessful)
                 {
                     setStorageConfirmed();
                 }
@@ -241,15 +283,24 @@ public class JythonDropboxRecoveryTest extends AbstractJythonDataSetHandlerTest
         {
             one(openBisService).performEntityOperations(with(atomicatOperationDetails));
 
-            Exception e = new IllegalArgumentException("Failure in atomicOperationDetails");
+            Exception e;
+            if (testCase.canRecoverFromError)
+            {
+                e = new EnvironmentFailureException("Potentially recoverable failure in registration");
+            } else
+            {
+                e = new UserFailureException("Unrecoverable failure in registration");
+            }
+
             will(throwException(e));
 
         }
 
+        @SuppressWarnings({ "rawtypes", "unchecked" })
         protected void checkRegistrationSucceeded()
         {
             one(openBisService).listDataSetsByCode(Arrays.asList(DATA_SET_CODE));
-            if (testCase.registrationSuccesfull)
+            if (testCase.registrationSuccessful)
             {
                 // with the current implemntation returning the non-empty list should be enough
                 List<ExternalData> externalDatas = (List) Arrays.asList(new Object());
diff --git a/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/JythonTopLevelDataSetRegistratorTest.java b/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/JythonTopLevelDataSetRegistratorTest.java
index ba2f614b48a..42be78c44e3 100644
--- a/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/JythonTopLevelDataSetRegistratorTest.java
+++ b/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/JythonTopLevelDataSetRegistratorTest.java
@@ -319,8 +319,8 @@ public class JythonTopLevelDataSetRegistratorTest extends AbstractJythonDataSetH
         return resultsList;
     }
 
-    //INFO: testCase parameters
-    
+    // INFO: testCase parameters
+
     /**
      * Parameters for the single run of the testSimpleTransaction
      * 
@@ -428,7 +428,7 @@ public class JythonTopLevelDataSetRegistratorTest extends AbstractJythonDataSetH
         }
     }
 
-    //INFO: test simple transaction
+    // INFO: test simple transaction
     @Test(dataProvider = "simpleTransactionTestCaseProvider")
     public void testSimpleTransaction(final TestCaseParameters testCase)
     {
@@ -515,11 +515,11 @@ public class JythonTopLevelDataSetRegistratorTest extends AbstractJythonDataSetH
         context.assertIsSatisfied();
     }
 
-    public Expectations getSimpleTransactionExpectations(
-            final TestCaseParameters testCase,
+    public Expectations getSimpleTransactionExpectations(final TestCaseParameters testCase,
             final RecordingMatcher<AtomicEntityOperationDetails> atomicatOperationDetails)
     {
-        final Experiment experiment = new ExperimentBuilder().identifier(EXPERIMENT_IDENTIFIER).getExperiment();
+        final Experiment experiment =
+                new ExperimentBuilder().identifier(EXPERIMENT_IDENTIFIER).getExperiment();
         return new Expectations()
             {
                 {
@@ -530,6 +530,13 @@ public class JythonTopLevelDataSetRegistratorTest extends AbstractJythonDataSetH
                 {
                     checkIfRecoveryFile();
 
+                    if (testCase.failurePoint != null
+                            && testCase.failurePoint
+                                    .compareTo(TestCaseParameters.FailurePoint.DURING_OPENBIS_REGISTRATION) < 0)
+                    {
+                        cleanRecoveryCheckpoint(false);
+                    }
+
                     if (testCase.failurePoint == TestCaseParameters.FailurePoint.AT_THE_BEGINNING)
                     {
                         return;
@@ -567,6 +574,7 @@ public class JythonTopLevelDataSetRegistratorTest extends AbstractJythonDataSetH
 
                     if (testCase.failurePoint == TestCaseParameters.FailurePoint.DURING_OPENBIS_REGISTRATION)
                     {
+                        cleanRecoveryCheckpoint(true);
                         return;
                     }
 
@@ -584,6 +592,23 @@ public class JythonTopLevelDataSetRegistratorTest extends AbstractJythonDataSetH
                     will(returnValue(false));
                 }
 
+                @SuppressWarnings("unchecked")
+                private void cleanRecoveryCheckpoint(boolean required)
+                {
+                    if (testCase.shouldUseAutoRecovery)
+                    {
+                        if (required)
+                        {
+                            one(storageRecoveryManager).removeCheckpoint(
+                                    with(any(DataSetStorageAlgorithmRunner.class)));
+                        } else
+                        {
+                            allowing(storageRecoveryManager).removeCheckpoint(
+                                    with(any(DataSetStorageAlgorithmRunner.class)));
+                        }
+                    }
+                }
+
                 protected void setStorageConfirmed()
                 {
                     one(openBisService).setStorageConfirmed(DATA_SET_CODE);
@@ -817,7 +842,6 @@ public class JythonTopLevelDataSetRegistratorTest extends AbstractJythonDataSetH
         }
     }
 
-
     @Test
     public void testTwoSimpleDataSets()
     {
diff --git a/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/api/v1/impl/DataSetRegistrationTransactionTest.java b/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/api/v1/impl/DataSetRegistrationTransactionTest.java
index 81228580837..311fe1f79fe 100644
--- a/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/api/v1/impl/DataSetRegistrationTransactionTest.java
+++ b/datastore_server/sourceTest/java/ch/systemsx/cisd/etlserver/registrator/api/v1/impl/DataSetRegistrationTransactionTest.java
@@ -710,6 +710,14 @@ public class DataSetRegistrationTransactionTest extends AbstractFileSystemTestCa
         {
             return new DataSet<DataSetInformation>(registrationDetails, stagingFile, openBisService);
         }
+
+        /**
+         * V1 test -- any file can go into faulty paths.
+         */
+        public boolean shouldNotAddToFaultyPathsOrNull(File storeItem)
+        {
+            return false;
+        }
     }
 
     private void checkContentsOfFile(File dst)
-- 
GitLab