Keep a FOS report readable after its task is gone

packages/web/commons/schema.php

@@ -4062,3 +4062,101 @@ function () {
     . "SET `logType` = 'state' "
     . "WHERE `logType` = '' OR `logType` IS NULL",
 );
+// 283
+$this->schema[] = array(
+    // A report keeps enough identity to be read after its task is gone.
+    // Ported from 1.6 schema 341 (#1236).
+    //
+    // taskLog stores no host and no task type of its own, and reaches both
+    // through `tasks`. Nothing deletes taskLog rows -- but Host::destroy()
+    // calls TaskManager->destroy() and taskLog is in no cascade at all, so
+    // deleting a host destroys its tasks and leaves the reports behind with
+    // nothing to join to, losing the host name at the same moment the host
+    // row that could supply it goes.
+    //
+    // Host name is the first thing anyone searches a failure by, and this
+    // branch has no Task Management log pane, so the REST API is the only
+    // reader there is -- it hands back a report whose taskID points at
+    // nothing and no way at all to learn which machine it came from. The
+    // point of GH-1206 is that a failure message is findable later instead
+    // of arriving as a phone photo of a wrapped console, and a foreign key
+    // to a routinely-deleted row cannot deliver that.
+    //
+    // Blocking deletion of a task that has reports was the alternative. It
+    // inverts the dependency -- a diagnostic artifact would then constrain
+    // operational cleanup -- and to be consistent it would have to block
+    // HOST deletion too, since that is the path that actually removes tasks.
+    //
+    // The state a row records is NOT copied: taskLog already stores
+    // taskStateID itself, so that lookup survives the task.
+    //
+    // Written only by the FOS report endpoint. Every other row in this table
+    // is a state transition written by TaskingElement::taskLog() on every
+    // transition; they are meaningless without their task anyway, and making
+    // that path do three extra lookups buys nothing. Same reasoning that
+    // gave logText no value on a state row in step 280.
+    //
+    // Two column shapes on purpose, and they follow what the writer can
+    // actually produce. FOGController::save() omits an unset OPTIONAL column
+    // whose key ends in "id" -- so logHostID gets its DEFAULT of NULL -- but
+    // for every other key an unset value is written as '', never NULL (the
+    // trap step 282 had to repair for logType). Declaring logHostName NOT
+    // NULL DEFAULT '' says what the ORM will really store rather than
+    // describing a NULL the writer cannot produce.
+    //
+    // A closure rather than a bare ALTER for the same reason step 280 is:
+    // ADD COLUMN has no IF NOT EXISTS below MariaDB 10.0.2/MySQL 8.0.29, so
+    // a re-run has to converge on its own rather than error.
+    function () {
+        $have = self::$DB->query(
+            "SELECT `COLUMN_NAME` AS `c` FROM `information_schema`.`COLUMNS` "
+            . "WHERE `TABLE_SCHEMA` = DATABASE() AND `TABLE_NAME` = 'taskLog' "
+            . "AND `COLUMN_NAME` IN "
+            . "('logHostID','logHostName','logTaskTypeName')"
+        )->fetch(\PDO::FETCH_ASSOC, 'fetch_all')->get();
+        $cols = array();
+        foreach ((array)$have as $row) {
+            if (isset($row['c'])) {
+                $cols[] = $row['c'];
+            }
+        }
+        $adds = array();
+        if (!in_array('logHostID', $cols)) {
+            $adds[] = "ADD `logHostID` INT(11) NULL DEFAULT NULL";
+        }
+        if (!in_array('logHostName', $cols)) {
+            // varchar(16) matches hosts.hostName, which is capped at the
+            // NetBIOS limit and cannot outgrow this copy.
+            $adds[] = "ADD `logHostName` VARCHAR(16) NOT NULL DEFAULT ''";
+        }
+        if (!in_array('logTaskTypeName', $cols)) {
+            // varchar(30) matches taskTypes.ttName.
+            $adds[] = "ADD `logTaskTypeName` VARCHAR(30) NOT NULL DEFAULT ''";
+        }
+        if (count($adds) > 0) {
+            self::$DB->query(
+                "ALTER TABLE `taskLog` " . implode(', ', $adds)
+            );
+        }
+
+        // Backfill the reports whose task is still there, so the history is
+        // not split between rows that know their host and rows that do not.
+        // Restricted to report rows and to rows not already filled, so a
+        // re-run is a no-op and a later hand-correction is not overwritten.
+        self::$DB->query(
+            "UPDATE `taskLog` "
+            . "JOIN `tasks` ON `tasks`.`taskID` = `taskLog`.`taskID` "
+            . "LEFT JOIN `hosts` "
+            . "ON `hosts`.`hostID` = `tasks`.`taskHostID` "
+            . "LEFT JOIN `taskTypes` "
+            . "ON `taskTypes`.`ttID` = `tasks`.`taskTypeID` "
+            . "SET `taskLog`.`logHostID` = `tasks`.`taskHostID`, "
+            . "`taskLog`.`logHostName` = COALESCE(`hosts`.`hostName`, ''), "
+            . "`taskLog`.`logTaskTypeName` = COALESCE(`taskTypes`.`ttName`, '') "
+            . "WHERE `taskLog`.`logType` <> 'state' "
+            . "AND `taskLog`.`logHostID` IS NULL"
+        );
+
+        return true;
+    },
+);

packages/web/lib/fog/system.class.php

@@ -54,7 +54,7 @@ public function __construct()
     {
         self::_versionCompare();
         define('FOG_VERSION', '1.5.10.2308');
-        define('FOG_SCHEMA', 282);
+        define('FOG_SCHEMA', 283);
         define('FOG_BCACHE_VER', 143);
         define('FOG_CLIENT_VERSION', '0.13.0');
         // GH-959: iPXE lives in FOGProject/fog-ipxe and its binaries arrive as

packages/web/lib/fog/tasklog.class.php

@@ -30,6 +30,20 @@ class TaskLog extends FOGController
     /**
      * The task log fields and common names.
      *
+     * hostID/hostName/taskTypeName are a copy of who the report was about,
+     * kept because the row outlives what it points at. taskLog reaches host
+     * and task type through `tasks`; nothing deletes taskLog rows, but
+     * Host::destroy() destroys the host's tasks and taskLog is in no cascade
+     * -- so deleting a host leaves the reports with nothing to join to,
+     * losing the host name at the same moment the host row that could supply
+     * it goes. This branch has no Task Management log pane, so the REST API
+     * is the only reader, and it could not recover the host at all. Schema
+     * 283 stores it here.
+     *
+     * Only the FOS report endpoint fills them. A state row leaves them
+     * empty: it is an annotation on a task and means nothing without it,
+     * and TaskingElement::taskLog() runs on every transition.
+     *
      * @var array
      */
     protected $databaseFields = array(
@@ -41,6 +55,9 @@ class TaskLog extends FOGController
         'createdBy' => 'createdBy',
         'type' => 'logType',
         'text' => 'logText',
+        'hostID' => 'logHostID',
+        'hostName' => 'logHostName',
+        'taskTypeName' => 'logTaskTypeName',
     );
     /**
      * A row recording a state transition, which is what every row was

packages/web/lib/reg-task/taskerror.class.php

@@ -281,12 +281,21 @@ private static function _markFailed($Task)
      */
     private static function _logRow($Task, $type, $text)
     {
+        // Host and task type are copied onto the row, not left to the join.
+        // Host::destroy() destroys the host's tasks, taskLog is in no
+        // cascade, so the report outlives everything it points at -- and by
+        // the time the join fails the host row is gone too, which makes this
+        // the last moment the name can be recorded at all. See TaskLog's
+        // $databaseFields and schema step 283.
         self::getClass('TaskLog')
             ->set('taskID', $Task->get('id'))
             ->set('stateID', $Task->get('stateID'))
             ->set('createdBy', 'fos')
             ->set('type', $type)
             ->set('text', $text)
+            ->set('hostID', self::$Host->get('id'))
+            ->set('hostName', self::$Host->get('name'))
+            ->set('taskTypeName', $Task->getTaskTypeText())
             ->save();
     }
     /**

tests/task-error-report.test.php

@@ -339,13 +339,58 @@ class TaskLog
 // ----------------------------------------------------------- the plumbing
 
 $model = file_get_contents($web . '/lib/fog/tasklog.class.php');
-foreach (array('type' => 'logType', 'text' => 'logText') as $key => $column) {
+$columns = array(
+    'type' => 'logType',
+    'text' => 'logText',
+    'hostID' => 'logHostID',
+    'hostName' => 'logHostName',
+    'taskTypeName' => 'logTaskTypeName',
+);
+foreach ($columns as $key => $column) {
     if (false === strpos($model, "'$key' => '$column'")) {
         $fails[] = "TaskLog does not map $key onto $column, so the endpoint's"
             . ' report is dropped on save';
     }
 }
 
+// ---------------------------------------------------------- the retention
+
+// The row outlives what it points at. Host::destroy() destroys the host's
+// tasks and taskLog is in no cascade, so a report whose host is deleted keeps
+// its text and loses the name it would be searched by -- and this branch has
+// no log pane, so the REST API is the only reader and cannot recover it from
+// anywhere. Ported from 1.6 (GH-1236); the reader half has nothing to port.
+foreach (array(
+    'hostID' => "set('hostID', self::\$Host->get('id'))",
+    'hostName' => "set('hostName', self::\$Host->get('name'))",
+    'taskTypeName' => "set('taskTypeName', \$Task->getTaskTypeText())",
+) as $field => $needle) {
+    if (false === strpos($src, $needle)) {
+        $fails[] = "the endpoint does not store $field on the report, so the"
+            . ' row loses it the moment its task is deleted';
+    }
+}
+// The columns have to exist before anything can be written to them, and a
+// declared field that has no column takes the whole INSERT down -- which is
+// silent here, because save() swallows it and _logRow ignores the return.
+if (!preg_match(
+    "#ADD `logHostID` INT\(11\).*?ADD `logHostName` VARCHAR\(16\)"
+    . ".*?ADD `logTaskTypeName` VARCHAR\(30\)#s",
+    $schema
+)) {
+    $fails[] = 'no schema step adds the report identity columns, so every'
+        . ' report insert fails with 1054 and is dropped without a trace';
+}
+// Existing reports whose task survived must not be left as the only rows
+// that cannot answer the question.
+if (!preg_match(
+    "#UPDATE `taskLog` .*?SET `taskLog`\.`logHostID`#s",
+    $schema
+)) {
+    $fails[] = 'the schema step adds the columns but does not backfill the'
+        . ' reports whose task is still there';
+}
+
 // The log directory has to appear in all three lists or the Log Viewer fails
 // in two different ways -- no entry, or an entry answering "Invalid Folder".
 $subdir = constant('TaskError::LOG_SUBDIR');