Fix: improve detection of failed migrations when using separate proce…

…sses and the child process dies without traces
kaliop-uk · Oct 17, 2016 · f3fe503 · f3fe503
1 parent 1bd064d
commit f3fe503
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 13 deletions.
diff --git a/API/StorageHandlerInterface.php b/API/StorageHandlerInterface.php
@@ -24,6 +24,7 @@ public function loadMigration($migrationName);
 
     /**
      * Creates and stores a new migration (leaving it in TODO status)
+     *
      * @param MigrationDefinition $migrationDefinition
      * @return mixed
      * @throws \Exception If the migration exists already
@@ -32,6 +33,7 @@ public function addMigration(MigrationDefinition $migrationDefinition);
 
     /**
      * Starts a migration (creates and stores it, in STARTED status)
+     *
      * @param MigrationDefinition $migrationDefinition
      * @return Migration
      * @throws \Exception If the migration was already executed, skipped or executing
@@ -40,6 +42,7 @@ public function startMigration(MigrationDefinition $migrationDefinition);
 
     /**
      * Ends a migration (updates it)
+     *
      * @param Migration $migration
      * @param bool $force When true, the migration will be updated even if it was not in 'started' status
      * @throws \Exception If the migration was not started (unless $force=true)
@@ -48,12 +51,14 @@ public function endMigration(Migration $migration, $force=false);
 
     /**
      * Removes a migration from storage (regardless of its status)
+     *
      * @param Migration $migration
      */
     public function deleteMigration(Migration $migration);
 
     /**
      * Skips a migration (upserts it, in SKIPPED status)
+     *
      * @param MigrationDefinition $migrationDefinition
      * @return Migration
      * @throws \Exception If the migration was already executed, skipped or executing

diff --git a/Command/MigrateCommand.php b/Command/MigrateCommand.php
@@ -80,17 +80,17 @@ protected function execute(InputInterface $input, OutputInterface $output)
             $this->setVerbosity(self::VERBOSITY_CHILD);
         }
 
-        $migrationsService = $this->getMigrationService();
+        $migrationService = $this->getMigrationService();
 
         $paths = $input->getOption('path');
-        $migrationDefinitions = $migrationsService->getMigrationsDefinitions($paths);
-        $migrations = $migrationsService->getMigrations();
+        $migrationDefinitions = $migrationService->getMigrationsDefinitions($paths);
+        $migrations = $migrationService->getMigrations();
 
         // filter away all migrations except 'to do' ones
         $toExecute = array();
         foreach($migrationDefinitions as $name => $migrationDefinition) {
             if (!isset($migrations[$name]) || (($migration = $migrations[$name]) && $migration->status == Migration::STATUS_TODO)) {
-                $toExecute[$name] = $migrationsService->parseMigrationDefinition($migrationDefinition);
+                $toExecute[$name] = $migrationService->parseMigrationDefinition($migrationDefinition);
             }
         }
 
@@ -99,10 +99,10 @@ protected function execute(InputInterface $input, OutputInterface $output)
         if (empty($paths)) {
             foreach ($migrations as $migration) {
                 if ($migration->status == Migration::STATUS_TODO && !isset($toExecute[$migration->name])) {
-                    $migrationDefinitions = $migrationsService->getMigrationsDefinitions(array($migration->path));
+                    $migrationDefinitions = $migrationService->getMigrationsDefinitions(array($migration->path));
                     if (count($migrationDefinitions)) {
                         $migrationDefinition = reset($migrationDefinitions);
-                        $toExecute[$migration->name] = $migrationsService->parseMigrationDefinition($migrationDefinition);
+                        $toExecute[$migration->name] = $migrationService->parseMigrationDefinition($migrationDefinition);
                     } else {
                         // q: shall we raise a warning here ?
                     }
@@ -182,6 +182,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
             }
         }
 
+        /** @var MigrationDefinition $migrationDefinition */
         foreach($toExecute as $name => $migrationDefinition) {
 
             // let's skip migrations that we know are invalid - user was warned and he decided to proceed anyway
@@ -209,20 +210,46 @@ function ($type, $buffer) {
                     }
                 );
 
-                if (!$process->isSuccessful()) {
-                    $err = $process->getErrorOutput();
+                try {
+
+                    if (!$process->isSuccessful()) {
+                        throw new \Exception($process->getErrorOutput());
+                    }
+
+                    // There are cases where the separate process dies halfway but does not return a non-zero code.
+                    // That's why we should double-check here if the migration is still tagged as 'started'...
+                    /** @var Migration $migration */
+                    $migration = $migrationService->getMigration($migrationDefinition->name);
+
+                    if (!$migration) {
+                        // q: shall we add the migration to the db as failed? In doubt, we let it become a ghost, disappeared without a trace...
+                        throw new \Exception("After the separate process charged to execute the migration finished, the migration can not be found in the database any more.");
+                    } else if ($migration->status == Migration::STATUS_STARTED) {
+                        $errorMsg = "The separate process charged to execute the migration left it in 'started' state. Most likely it died halfway through execution.";
+                        $migrationService->endMigration(New Migration(
+                            $migration->name,
+                            $migration->md5,
+                            $migration->path,
+                            $migration->executionDate,
+                            Migration::STATUS_FAILED,
+                            ($migration->executionError != '' ? ($errorMsg . ' ' . $migration->executionError) : $errorMsg)
+                        ));
+                        throw new \Exception($errorMsg);
+                    }
+
+                } catch(\Exception $e) {
                     if ($input->getOption('ignore-failures')) {
-                        $output->writeln("\n<error>Migration failed! Reason: " . $err . "</error>\n");
+                        $output->writeln("\n<error>Migration failed! Reason: " . $e->getMessage() . "</error>\n");
                         continue;
                     }
-                    $output->writeln("\n<error>Migration aborted! Reason: " . $err . "</error>");
+                    $output->writeln("\n<error>Migration aborted! Reason: " . $e->getMessage() . "</error>");
                     return 1;
                 }
 
             } else {
 
                 try {
-                    $migrationsService->executeMigration(
+                    $migrationService->executeMigration(
                         $migrationDefinition,
                         !$input->getOption('no-transactions'),
                         $input->getOption('default-language')

diff --git a/Core/MigrationService.php b/Core/MigrationService.php
@@ -86,7 +86,7 @@ public function getMigrationsDefinitions(array $paths = array())
     }
 
     /**
-     * Return the list of all the migrations which where executed or attempted so far
+     * Returns the list of all the migrations which where executed or attempted so far
      *
      * @return \Kaliop\eZMigrationBundle\API\Collection\MigrationCollection
      */
@@ -130,6 +130,16 @@ public function skipMigration(MigrationDefinition $migrationDefinition)
         return $this->storageHandler->skipMigration($migrationDefinition);
     }
 
+    /**
+     * Not be called by external users for normal use cases, you should use executeMigration() instead
+     *
+     * @param Migration $migration
+     */
+    public function endMigration(Migration $migration)
+    {
+        return $this->storageHandler->endMigration($migration);
+    }
+
     /**
      * Parses a migration definition, return a parsed definition.
      * If there is a parsing error, the definition status will be updated accordingly

diff --git a/Core/StorageHandler/Database.php b/Core/StorageHandler/Database.php
@@ -204,6 +204,7 @@ public function endMigration(Migration $migration, $force=false)
 
     /**
      * Removes a Migration from the table - regardless of its state!
+     *
      * @param Migration $migration
      */
     public function deleteMigration(Migration $migration)
@@ -214,7 +215,7 @@ public function deleteMigration(Migration $migration)
     }
 
     /**
-     * Stops a migration by storing it in the db. Migration status can not be 'started'
+     * Skips a migration by storing it in the db. Migration status can not be 'started'
      *
      * @param MigrationDefinition $migrationDefinition
      * @return Migration

diff --git a/WHATSNEW.md b/WHATSNEW.md
@@ -1,3 +1,9 @@
+Version 2.4.2
+=============
+
+* Fix: improve detection of failed migrations when using separate processes and the child process dies without traces
+
+
 Version 2.4.1
 =============