fix(xo-server): sort subtasks even for failed task

packages/xo-server/package.json

@@ -26,7 +26,7 @@
     "xo-xcppool-ssh": "dist/ssh-cli.mjs"
   },
   "engines": {
-    "node": ">=14.18"
+    "node": ">=22.3"
   },
   "dependencies": {
     "@iarna/toml": "^2.2.1",

packages/xo-server/src/_handleBackupLog.mjs

@@ -1,6 +1,6 @@
 import humanFormat from 'human-format'
 import ms from 'ms'
-import { computeStatusAndSortSubtasks } from './xo-mixins/backups-ng-logs.mjs'
+import { consolidateTaskStatusAndResult } from './xo-mixins/backups-ng-logs.mjs'
 import { createLogger } from '@xen-orchestra/log'
 
 const { warn } = createLogger('xo:server:handleBackupLog')
@@ -42,7 +42,7 @@ export function forwardResult(log) {
 // it records logs generated by backup task logs
 export const handleBackupLog = (taskLog, event, { app, jobName } = {}) => {
   if (event.type === 'end') {
-    taskLog.status = computeStatusAndSortSubtasks(taskLog)
+    taskLog.status = consolidateTaskStatusAndResult(taskLog)
   }
 
   // sending data to Nagios

packages/xo-server/src/xo-mixins/_backups-ng-logs.test.mjs

@@ -0,0 +1,139 @@
+import assert from 'assert/strict'
+import test from 'node:test'
+import { serializeError } from '@vates/task'
+
+import { consolidateTaskStatusAndResult } from './backups-ng-logs.mjs'
+
+const { describe, it } = test
+
+describe('consolidateTaskStatusAndResult', function () {
+  it('returns the task status unchanged when it has no subtasks', () => {
+    const task = { status: 'success' }
+    assert.equal(consolidateTaskStatusAndResult(task), 'success')
+  })
+
+  it('returns "failure" without inspecting subtasks when the task itself already failed', () => {
+    const task = {
+      status: 'failure',
+      result: new Error('own failure'),
+      tasks: [{ status: 'success' }],
+    }
+    assert.equal(consolidateTaskStatusAndResult(task), 'failure')
+    // untouched: the task already carries its own result
+    assert.equal(task.result.message, 'own failure')
+  })
+
+  it('turns a failure into "skipped" when the error matches a known skip reason', () => {
+    const task = { status: 'failure', result: new Error('no disks found') }
+    assert.equal(consolidateTaskStatusAndResult(task), 'skipped')
+  })
+
+  it("propagates a single failing subtask's serialized result to the parent, and still sorts subtasks", () => {
+    const error = new Error('BodyTimeoutError')
+    const task = {
+      status: 'success',
+      tasks: [
+        { status: 'success', start: 2, end: 20 },
+        { status: 'failure', result: error, start: 1, end: 10 },
+      ],
+    }
+    assert.equal(consolidateTaskStatusAndResult(task), 'failure')
+    // already serialized to a plain object: downstream serializers that don't special-case
+    // Error/AggregateError (e.g. a plain JSON.stringify) would otherwise silently drop it
+    assert.equal(task.result.message, 'BodyTimeoutError')
+    assert.equal(task.result.name, 'Error')
+    assert.deepEqual(
+      task.tasks.map(({ start }) => start),
+      [1, 2]
+    )
+  })
+
+  it("bundles multiple failing subtasks' results into a serialized AggregateError", () => {
+    const error1 = new Error('remote A timed out')
+    const error2 = new Error('remote B timed out')
+    const task = {
+      status: 'success',
+      tasks: [{ status: 'failure', result: error1 }, { status: 'success' }, { status: 'failure', result: error2 }],
+    }
+    assert.equal(consolidateTaskStatusAndResult(task), 'failure')
+    assert.equal(task.result.name, 'AggregateError')
+    assert.deepEqual(task.result.errors, [serializeError(error1), serializeError(error2)])
+  })
+
+  it("does not overwrite the parent's own result when subtasks also fail", () => {
+    const ownError = new Error('own failure')
+    const task = {
+      status: 'success',
+      result: ownError,
+      tasks: [{ status: 'failure', result: new Error('subtask failure') }],
+    }
+    assert.equal(consolidateTaskStatusAndResult(task), 'failure')
+    assert.equal(task.result, ownError)
+  })
+
+  it('ignores a failing subtask superseded by a successful retry with the same name/id', () => {
+    const task = {
+      status: 'success',
+      warnings: [{ data: { isRetry: true } }],
+      tasks: [
+        { status: 'failure', result: new Error('first try failed'), properties: { name: 'export', id: 'sr:1' } },
+        { status: 'success', properties: { name: 'export', id: 'sr:1' } },
+      ],
+    }
+    assert.equal(consolidateTaskStatusAndResult(task), 'success')
+    assert.equal(task.result, undefined)
+  })
+
+  it('still fails if the retried subtask does not match name/id of any successful sibling', () => {
+    const task = {
+      status: 'success',
+      warnings: [{ data: { isRetry: true } }],
+      tasks: [
+        { status: 'failure', result: new Error('export failed'), properties: { name: 'export', id: 'sr:1' } },
+        { status: 'success', properties: { name: 'export', id: 'sr:2' } },
+      ],
+    }
+    assert.equal(consolidateTaskStatusAndResult(task), 'failure')
+  })
+
+  it('reports "skipped" when a subtask was skipped and none failed', () => {
+    const task = {
+      status: 'success',
+      tasks: [{ status: 'success' }, { status: 'skipped' }],
+    }
+    assert.equal(consolidateTaskStatusAndResult(task), 'skipped')
+  })
+
+  it('sorts subtasks by end time, finished before unfinished', () => {
+    const task = {
+      status: 'success',
+      tasks: [
+        { status: 'success', start: 3, end: 30 },
+        { status: 'pending', start: 1 },
+        { status: 'success', start: 2, end: 10 },
+      ],
+    }
+    consolidateTaskStatusAndResult(task)
+    assert.deepEqual(
+      task.tasks.map(({ start }) => start),
+      [2, 3, 1]
+    )
+  })
+
+  it('still sorts subtasks when the task itself already failed', () => {
+    const task = {
+      status: 'failure',
+      result: new Error('own failure'),
+      tasks: [
+        { status: 'success', start: 3, end: 30 },
+        { status: 'pending', start: 1 },
+        { status: 'success', start: 2, end: 10 },
+      ],
+    }
+    consolidateTaskStatusAndResult(task)
+    assert.deepEqual(
+      task.tasks.map(({ start }) => start),
+      [2, 3, 1]
+    )
+  })
+})

packages/xo-server/src/xo-mixins/backups-ng-logs.mjs

@@ -3,6 +3,7 @@ import iteratee from 'lodash/iteratee.js'
 import ms from 'ms'
 import sortedIndexBy from 'lodash/sortedIndexBy.js'
 import { noSuchObject } from 'xo-common/api-errors.js'
+import { serializeError } from '@vates/task'
 
 import { debounceWithKey } from '../_pDebounceWithKey.mjs'
 
@@ -25,27 +26,43 @@ const isSupersededByRetry = (subtask, siblings) =>
       other.properties?.id === subtask.properties?.id
   )
 
-export const computeStatusAndSortSubtasks = task => {
+export const consolidateTaskStatusAndResult = task => {
   let status = getStatus(task.result, task.status)
 
-  if (status === 'failure' || task.tasks === undefined) {
+  if (task.tasks === undefined) {
     return status
   }
 
-  // A retried subtask leaves a warning at the VM task level.
-  // If the initial try fails but a retry succeeds, the failure must not affect the parent task status.
-  const wasRetried = task.warnings?.some(({ data }) => data?.isRetry === true) ?? false
+  if (status !== 'failure') {
+    // A retried subtask leaves a warning at the VM task level.
+    // If the initial try fails but a retry succeeds, the failure must not affect the parent task status.
+    const wasRetried = task.warnings?.some(({ data }) => data?.isRetry === true) ?? false
 
-  for (let i = 0, n = task.tasks.length; i < n; ++i) {
-    const subtask = task.tasks[i]
-    if (subtask.status === 'failure') {
-      if (wasRetried && isSupersededByRetry(subtask, task.tasks)) {
+    const failedResults = []
+    for (let i = 0, n = task.tasks.length; i < n; ++i) {
+      const subtask = task.tasks[i]
+      if (subtask.status === 'failure') {
+        if (wasRetried && isSupersededByRetry(subtask, task.tasks)) {
+          continue
+        }
+        failedResults.push(subtask.result)
         continue
       }
-      return 'failure'
+      if (subtask.status === 'skipped') {
+        status = subtask.status
+      }
     }
-    if (subtask.status === 'skipped') {
-      status = subtask.status
+
+    if (failedResults.length > 0) {
+      // the task's own result may already carry an error; only fall back to the subtasks' when it doesn't
+      task.result =
+        task.result ??
+        serializeError(
+          failedResults.length === 1
+            ? failedResults[0]
+            : new AggregateError(failedResults, `Task failed with multiple errors`)
+        )
+      status = 'failure'
     }
   }
 
@@ -173,8 +190,8 @@ export default {
             log.end = time
             log.result = data.error
             log.status = statusFromError(data.error)
-            // computeStatusAndSortSubtasks reads log.status
-            log.status = computeStatusAndSortSubtasks(log)
+            // consolidateTaskStatusAndResult reads log.status
+            log.status = consolidateTaskStatusAndResult(log)
           }
         } else if (event === 'job.backupTaskStart') {
           // happens once, only for backups using XO Tasks
@@ -215,8 +232,8 @@ export default {
             log.end = time
             log.result = data.result
             log.status = data.status
-            // computeStatusAndSortSubtasks reads log.status
-            log.status = computeStatusAndSortSubtasks(log)
+            // consolidateTaskStatusAndResult reads log.status
+            log.status = consolidateTaskStatusAndResult(log)
           }
         } else if (event === 'task.warning') {
           const parent = started[data.taskId]
@@ -255,8 +272,8 @@ export default {
             log.end = time
             log.result = data.error
             log.status = statusFromError(data.error)
-            // computeStatusAndSortSubtasks reads log.status
-            log.status = computeStatusAndSortSubtasks(log)
+            // consolidateTaskStatusAndResult reads log.status
+            log.status = consolidateTaskStatusAndResult(log)
           }
         }
       }