fix(xen-api): update undici to prevent timeout error (#10282)

CHANGELOG.unreleased.md

@@ -29,6 +29,7 @@
 - [Rolling pool update/reboot] VMs are brought back to the host they were running on more reliably, and a VM that cannot be moved back no longer fails the whole operation (PR [#10295](https://github.com/vatesfr/xen-orchestra/pull/10295))
 - [XO server] Fix current_operations format on host and pool objects (PR [#10283](https://github.com/vatesfr/xen-orchestra/pull/10283))
 - [Web-core] Fix "no data" illustration stars color (PR [#10327](https://github.com/vatesfr/xen-orchestra/pull/10327))
+- [Backup] Fix `uncaught exception AssertionError: assert(!this.paused)` in the logs, when a host closes a transfer while XO is writing to a slower destination (PR [#10282](https://github.com/vatesfr/xen-orchestra/pull/10282))
 
 ### Packages to release
 
@@ -50,6 +51,7 @@
 - @xen-orchestra/backups patch
 - @xen-orchestra/web minor
 - @xen-orchestra/web-core minor
+- xen-api major
 - xo-server minor
 - xo-web minor
 

packages/xen-api/index.mjs

@@ -6,7 +6,7 @@ import ms from 'ms'
 import map from 'lodash/map.js'
 import noop from 'lodash/noop.js'
 import Obfuscate from '@vates/obfuscate'
-import { Agent, ProxyAgent, request } from 'undici'
+import { Agent, interceptors, ProxyAgent, request } from 'undici'
 import { coalesceCalls } from '@vates/coalesce-calls'
 import { Collection } from 'xo-collection'
 import { compose } from '@vates/compose'
@@ -241,9 +241,13 @@ export class Xapi extends EventEmitter {
     const { httpProxy } = opts
     this._allowUnauthorized = opts.allowUnauthorized
     const dispatcherOpts = {
+      // XAPI speaks HTTP/1.1 only; undici >= 8 offers h2 during ALPN by default,
+      // which buys us nothing here and changes how the TLS handshake looks to
+      // XAPI and to whatever sits in front of it (stunnel, reverse proxies).
+      allowH2: false,
+
       bodyTimeout: this._httpInactivityTimeout,
       headersTimeout: this._httpInactivityTimeout,
-      maxRedirections: 3,
     }
     const tlsOpts = {
       minVersion: 'TLSv1',
@@ -272,6 +276,20 @@ export class Xapi extends EventEmitter {
         connect: tlsOpts,
       })
     }
+
+    // Redirects used to be handled by the dispatcher's `maxRedirections` option,
+    // which undici >= 8 silently ignores: it must now be an explicit
+    // interceptor. This matters when talking to a slave, which answers a 302 to
+    // the pool master. Calls that handle redirections themselves (`getResource`)
+    // keep opting out with a per-request `maxRedirections: 0`.
+    //
+    // Warning: since undici 7, this interceptor follows the fetch spec and
+    // rewrites a redirected POST into a bodyless GET, which would strip the
+    // payload of an RPC call. Harmless for now because XAPI only redirects
+    // GETs (`/export`, `/export_raw_vdi`, `/vm_rrd`) and PUTs (`/import`,
+    // `/import_raw_vdi`, `/messages`, `/rrd_put`), and answers a
+    // `HOST_IS_SLAVE` error instead of a redirection on the RPC endpoints
+    this._undiciDispatcher = this._undiciDispatcher.compose(interceptors.redirect({ maxRedirections: 3 }))
     this._setUrl(url)
 
     // Addresses of known pool members, used to fail over to a surviving host

packages/xen-api/package.json

@@ -28,7 +28,7 @@
     "xen-api": "./cli.mjs"
   },
   "engines": {
-    "node": ">=18"
+    "node": ">=22.19"
   },
   "dependencies": {
     "@vates/coalesce-calls": "^0.1.0",
@@ -51,7 +51,7 @@
     "proxy-agent": "^6.3.1",
     "promise-toolbox": "^0.21.0",
     "pw": "0.0.4",
-    "undici": "^6.2.1",
+    "undici": "^8.10.0",
     "xo-collection": "^0.6.1",
     "xo-common": "^0.11.0"
   },
@@ -72,6 +72,6 @@
   "scripts": {
     "plot": "gnuplot -p memory-test.gnu",
     "postversion": "npm publish",
-    "test": "tap --allow-incomplete-coverage"
+    "test": "tap --allow-incomplete-coverage *.spec.mjs && node --test"
   }
 }

packages/xen-api/redirections.test.mjs

@@ -0,0 +1,78 @@
+import { after, before, test } from 'node:test'
+import { createServer } from 'node:http'
+import { request } from 'undici'
+import { strict as assert } from 'node:assert'
+
+import { Xapi } from './index.mjs'
+
+const MAX_REDIRECTIONS = 3
+
+function listen(requestListener) {
+  const server = createServer(requestListener)
+  return new Promise(resolve => {
+    server.listen(0, '127.0.0.1', () => resolve(server))
+  })
+}
+
+function url(server, pathname = '/') {
+  const { address, port } = server.address()
+  return `http://${address}:${port}${pathname}`
+}
+
+let dispatcher, master, slave
+
+before(async () => {
+  // the pool master: answers the resource, and a chain of redirections used to
+  // check that the limit is enforced
+  master = await listen((req, res) => {
+    const hop = /^\/hop\/(\d+)$/.exec(req.url)
+    if (hop !== null) {
+      res.writeHead(302, { location: `/hop/${Number(hop[1]) + 1}` })
+      return res.end()
+    }
+
+    res.writeHead(200)
+    res.end('master')
+  })
+
+  // a slave: redirects everything to the master
+  slave = await listen((req, res) => {
+    res.writeHead(302, { location: new URL(req.url, url(master)).href })
+    res.end()
+  })
+
+  // the dispatcher `getResource` uses, built by the constructor: no connection
+  // is opened and no XAPI is contacted
+  dispatcher = new Xapi({ url: 'https://user:password@127.0.0.1', watchEvents: false })._undiciDispatcher
+})
+
+after(async () => {
+  master.close()
+  slave.close()
+  await dispatcher.close().catch(() => {})
+})
+
+test('a 302 from a slave is followed to the pool master', async () => {
+  const response = await request(url(slave, '/export'), { dispatcher })
+
+  assert.equal(response.statusCode, 200)
+  assert.equal(await response.body.text(), 'master')
+})
+
+test('`maxRedirections: 0` opts out, as `getResource` does', async () => {
+  const response = await request(url(slave, '/export'), { dispatcher, maxRedirections: 0 })
+
+  assert.equal(response.statusCode, 302)
+  assert.equal(response.headers.location, url(master, '/export'))
+  await response.body.dump()
+})
+
+test('no more than `maxRedirections` are followed', async () => {
+  const response = await request(url(master, '/hop/1'), { dispatcher })
+
+  // the last redirection is handed back to the caller instead of being
+  // followed, and no error is thrown
+  assert.equal(response.statusCode, 302)
+  assert.equal(response.headers.location, `/hop/${1 + MAX_REDIRECTIONS + 1}`)
+  await response.body.dump()
+})

packages/xen-api/undiciBackpressure.test.mjs

@@ -0,0 +1,99 @@
+import { createServer } from 'node:net'
+import { pipeline } from 'node:stream/promises'
+import { request } from 'undici'
+import { strict as assert } from 'node:assert'
+import { test } from 'node:test'
+import { Writable } from 'node:stream'
+
+import { Xapi } from './index.mjs'
+
+// Regression test for https://github.com/nodejs/undici/issues/5360
+//
+// XAPI answers exports and `/pool/xmldbdump` with an EOF-delimited body:
+// `Connection: close`, no `content-length`, no chunked encoding. When the
+// consumer is slower than the host (a backup written to a remote repository),
+// undici's HTTP/1 parser pauses on backpressure. If the peer then sends FIN
+// while the parser is paused, `Parser.finish()` used to `assert(!this.paused)`
+// from inside the socket `'end'` handler, where nothing can catch it: the
+// process got an uncaught `AssertionError` and the transfer never settled — all
+// the bytes were delivered but neither `end` nor `error` was ever emitted.
+//
+// Fixed in undici 8.10.0, which drains the still-buffered data on `finish()`
+// instead of asserting. This test guards the version floor: it fails on
+// undici 6.x and 7.x.
+
+const BODY_SIZE = 1024 * 1024
+
+const TEST_TIMEOUT = 10e3
+
+// answers any request with an EOF-delimited body, then closes
+function createXapiLikeServer() {
+  const body = Buffer.alloc(BODY_SIZE, 0x61)
+
+  const server = createServer(socket => {
+    socket.on('error', () => {})
+    socket.once('data', () => {
+      socket.write('HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n')
+      socket.write(body)
+      socket.end()
+    })
+  })
+
+  return new Promise(resolve => {
+    server.listen(0, '127.0.0.1', () => resolve(server))
+  })
+}
+
+// consumes the body slower than the server sends it, which is what makes the
+// parser pause
+function createSlowSink(counter, delay = 10) {
+  return new Writable({
+    highWaterMark: 1,
+    write(chunk, _encoding, callback) {
+      counter.size += chunk.length
+      setTimeout(callback, delay)
+    },
+  })
+}
+
+test('a host closing an export while the consumer is behind', async t => {
+  const server = await createXapiLikeServer()
+  t.after(() => server.close())
+
+  // the dispatcher `getResource` uses, built by the constructor: no connection
+  // is opened and no XAPI is contacted
+  const xapi = new Xapi({ url: 'https://user:password@127.0.0.1', watchEvents: false })
+  const dispatcher = xapi._undiciDispatcher
+
+  // `close()` waits for the in-flight request, which is exactly what never
+  // completes when the bug strikes
+  t.after(() => dispatcher.destroy())
+
+  const { address, port } = server.address()
+  const response = await request(`http://${address}:${port}/export`, {
+    dispatcher,
+
+    // as in `getResource`, which follows redirections itself
+    maxRedirections: 0,
+  })
+
+  const counter = { size: 0 }
+
+  // the timer is kept referenced: when the bug strikes, the transfer is the
+  // only thing left on the event loop and it never settles
+  let timer
+  const timeout = new Promise((resolve, reject) => {
+    timer = setTimeout(() => reject(new Error('the transfer never settled')), TEST_TIMEOUT)
+  })
+
+  try {
+    await Promise.race([pipeline(response.body, createSlowSink(counter)), timeout])
+  } finally {
+    clearTimeout(timer)
+  }
+
+  // the uncaught `AssertionError` thrown from the socket `'end'` handler is not
+  // asserted on: it escapes to the process, where the test runner reports it and
+  // fails this test on its own
+  assert.equal(counter.size, BODY_SIZE, 'the whole body should be delivered')
+})

yarn.lock

@@ -19220,6 +19220,11 @@ undici@^7.8.0:
   resolved "https://registry.yarnpkg.com/undici/-/undici-7.27.2.tgz#f8fae968ee68377cfc61713d9cd152773716804f"
   integrity sha512-uZsKNuzQxDMUY6M3pIMvy5tvlGmtq8XJ2oLAkfRKGNu+1VQAIvLy2xIVG5ATZl5wDXl/tddByAWCizRbOme+TA==
 
+undici@^8.10.0:
+  version "8.10.0"
+  resolved "https://registry.yarnpkg.com/undici/-/undici-8.10.0.tgz#67ed7c4087f0f40fba7bef3a46f2be80572f2473"
+  integrity sha512-HvltHd7avK13QIw/oLe4qoOLyoVSoafqJ2jYOrtMRBkbYT31eiBQ8O0ehRKZiEZCMEyLFQNIADpgCWC5fALvYQ==
+
 unicode-canonical-property-names-ecmascript@^2.0.0:
   version "2.0.1"
   resolved "https://registry.yarnpkg.com/unicode-canonical-property-names-ecmascript/-/unicode-canonical-property-names-ecmascript-2.0.1.tgz#cb3173fe47ca743e228216e4a3ddc4c84d628cc2"