fix(xo-server): auto reconnection infinite loop when pool is already connectd (#10355)

* don't rearm after a fatal error before connection setting changed ( does not affect the normal route on non fatal error like newtork issue)
* serialize error correctly in database to ensure the "avoid a database write per auto-reconnect" corectly trigger
a lot of more cleanup to ensure that the connection is really dropped even if the host changed its id
* fix a mishnalding in the grace period before marking a pool disconnected . this will keep the ui in sync AND not redownload all the xapi object for a transient issue
* fix an error transmitted as raw, so the user was only seeing {}

CHANGELOG.unreleased.md

@@ -22,6 +22,8 @@
 - [Web-core] Fix "console offline" illustration sparks color (PR [#10309](https://github.com/vatesfr/xen-orchestra/pull/10309))
 - [Web-core] Fix 404 illustration color (PR [#10325](https://github.com/vatesfr/xen-orchestra/pull/10325))
 - [Backup-archive] No longer create a `cache.json.gz` file on immutable/S3 remote during cleanup, which could not be deleted afterwards and stayed billed forever (PR [#10243](https://github.com/vatesfr/xen-orchestra/pull/10243))
+- [Servers] Fix endless connection attempts to a pool which is already connected through another server entry (PR [#10355](https://github.com/vatesfr/xen-orchestra/pull/10355))
+- [Servers] fix a mishandling in the grace period before marking a pool disconnected, this will keep the ui in sync AND not redownload all the xapi object for a transient issue (PR [#10355](https://github.com/vatesfr/xen-orchestra/pull/10355))
 
 ### Packages to release
 
@@ -43,5 +45,6 @@
 - @xen-orchestra/backups patch
 - @xen-orchestra/web minor
 - @xen-orchestra/web-core minor
+- xo-server patch
 
 <!--packages-end-->

packages/xo-server/src/models/server.test.mjs

@@ -0,0 +1,54 @@
+import assert from 'node:assert/strict'
+import { describe, it } from 'node:test'
+
+import { serializeError } from '../utils.mjs'
+import { Servers } from './server.mjs'
+
+// Reproduces what a record goes through on its way to the database and back:
+// `Collection#_add` copies it with JSON *before* the model serializes it, then
+// redis stores and returns JSON
+const throughDatabase = record => {
+  const copy = JSON.parse(JSON.stringify(record))
+  Servers.prototype._serialize(copy)
+
+  const stored = JSON.parse(JSON.stringify(copy))
+  Servers.prototype._unserialize(stored)
+  return stored
+}
+
+const makeServer = error => ({ id: 'server-1', enabled: true, error, host: '192.0.2.1' })
+
+describe('Servers', function () {
+  describe('error', function () {
+    it('does not survive the copy made by the collection when it is a raw Error', function () {
+      // hence `serializeError` in `_connectXenServer`: `message`, `name` and
+      // `stack` are not enumerable, JSON copies them away
+      const { error } = throughDatabase(makeServer(new Error('this pool is already connected')))
+
+      assert.equal(error?.message, undefined)
+    })
+
+    it('keeps its message and code once serialized by the caller', function () {
+      const cause = new Error('SESSION_AUTHENTICATION_FAILED(, )')
+      cause.code = 'SESSION_AUTHENTICATION_FAILED'
+
+      const { error } = throughDatabase(makeServer(serializeError(cause)))
+
+      assert.equal(error.message, 'SESSION_AUTHENTICATION_FAILED(, )')
+      assert.equal(error.code, 'SESSION_AUTHENTICATION_FAILED')
+      assert.equal(error.name, 'Error')
+    })
+
+    it('compares equal to a new occurrence of the same error, so a failing server is not rewritten', function () {
+      // `_connectXenServer` skips the database write – and therefore any
+      // reconnection attempt triggered by it – when the error did not change
+      const previousError = throughDatabase(
+        makeServer(serializeError(new Error('this pool is already connected')))
+      ).error
+      const currentError = serializeError(new Error('this pool is already connected'))
+
+      assert.equal(previousError.code, currentError.code)
+      assert.equal(previousError.message, currentError.message)
+    })
+  })
+})

packages/xo-server/src/xo-mixins/_xen-servers.test.mjs

@@ -0,0 +1,161 @@
+import assert from 'node:assert/strict'
+import { describe, it } from 'node:test'
+import { configure } from '@xen-orchestra/log/configure'
+
+import XenServers from './xen-servers.mjs'
+
+configure({ level: 'FATAL', transport: () => {} })
+
+// Minimal XoApp mock: the constructor only registers hooks and watches a config
+// duration, none of which are fired by these tests
+const createMockApp = () => ({
+  config: { watchDuration: () => {} },
+  hooks: { on: () => {} },
+})
+
+const SERVER = { id: 'server-1', enabled: true, host: '192.0.2.1', password: 'secret', username: 'root' }
+
+// `poolId` defaults to the pool the connection was opened with, `$id` is the
+// one it currently reports (they differ after a pool UUID change)
+const createXapi = ({ $id = 'pool-1' } = {}) => ({
+  disconnect: async () => {},
+  getObjectByRef: () => ({ uuid: 'host-1' }),
+  pool: { $id, master: 'OpaqueRef:master', uuid: $id },
+})
+
+// `_servers` is normally created on the `core started` hook, and
+// `_autoReconnectXenServer` would start a real reconnection loop
+const createXenServers = (...servers) => {
+  const xenServers = new XenServers(createMockApp(), { safeMode: true })
+
+  const stored = { __proto__: null }
+  const serversToInitialize = servers.length === 0 ? [SERVER] : servers
+
+  for (const server of serversToInitialize) {
+    stored[server.id] = { ...server }
+  }
+  const updates = []
+  const reconnected = []
+
+  xenServers._servers = {
+    first: async id => (stored[id] === undefined ? undefined : { ...stored[id] }),
+    update: async model => {
+      updates.push({ ...model })
+      Object.assign(stored[model.id], model)
+    },
+  }
+  xenServers._autoReconnectXenServer = id => reconnected.push(id)
+
+  return { reconnected, updates, xenServers }
+}
+
+describe('updateXenServer', function () {
+  it('does not arm the auto-reconnect loop on the internal error bookkeeping write', async function () {
+    const { reconnected, updates, xenServers } = createXenServers()
+
+    // `_connectXenServer` writes the error of every failed attempt: re-arming
+    // the loop here restarts one which just stopped on a permanent error
+    await xenServers.updateXenServer('server-1', { error: new Error('this pool is already connected') })
+
+    assert.equal(updates.length, 1, 'the error must still be persisted')
+    assert.deepEqual(reconnected, [])
+  })
+
+  it('does not arm the auto-reconnect loop when the error is cleared', async function () {
+    const { reconnected, xenServers } = createXenServers({ ...SERVER, error: { message: 'boom' } })
+
+    await xenServers.updateXenServer('server-1', { error: null })
+
+    assert.deepEqual(reconnected, [])
+  })
+
+  it('does not arm the auto-reconnect loop on an unrelated property', async function () {
+    const { reconnected, updates, xenServers } = createXenServers()
+
+    await xenServers.updateXenServer('server-1', { label: 'new label' })
+
+    assert.equal(updates.length, 1)
+    assert.deepEqual(reconnected, [])
+  })
+
+  it('arms the auto-reconnect loop when the server is enabled', async function () {
+    const { reconnected, xenServers } = createXenServers({ ...SERVER, enabled: false })
+
+    await xenServers.updateXenServer('server-1', { enabled: true })
+
+    assert.deepEqual(reconnected, ['server-1'])
+  })
+
+  it('arms the auto-reconnect loop when the credentials or the address change', async function () {
+    for (const properties of [{ host: '192.0.2.2' }, { password: 'new secret' }, { username: 'admin' }]) {
+      const { reconnected, xenServers } = createXenServers()
+
+      await xenServers.updateXenServer('server-1', properties)
+
+      assert.deepEqual(reconnected, ['server-1'], `expected a reconnection for ${JSON.stringify(properties)}`)
+    }
+  })
+
+  it('does not arm the auto-reconnect loop while the server is already connecting', async function () {
+    const { reconnected, xenServers } = createXenServers({ ...SERVER, enabled: false })
+    xenServers._connectingXenServers.add('server-1')
+
+    await xenServers.updateXenServer('server-1', { enabled: true })
+
+    assert.deepEqual(reconnected, [])
+  })
+})
+
+describe('disconnectXenServer', function () {
+  const connect = (xenServers, { poolId = 'pool-1', serverId = 'server-1' } = {}) => {
+    xenServers._xapis[serverId] = createXapi({ $id: poolId })
+    xenServers._serverIdsByPool[poolId] = serverId
+  }
+
+  it('forgets the pool of the server', async function () {
+    const { xenServers } = createXenServers()
+    connect(xenServers)
+
+    await xenServers.disconnectXenServer('server-1')
+
+    assert.deepEqual({ ...xenServers._serverIdsByPool }, {})
+    assert.equal(xenServers._xapis['server-1'], undefined)
+  })
+
+  it('forgets the pool of the server even after a pool UUID change', async function () {
+    const { xenServers } = createXenServers()
+    connect(xenServers, { poolId: 'pool-1' })
+    // `_onXenAdd` re-registers the server under the new identifier
+    delete xenServers._serverIdsByPool['pool-1']
+    xenServers._serverIdsByPool['pool-2'] = 'server-1'
+
+    await xenServers.disconnectXenServer('server-1')
+
+    assert.deepEqual({ ...xenServers._serverIdsByPool }, {})
+  })
+
+  it('does not drop the connection of the server holding the pool', async function () {
+    // a second entry registered on an already connected pool stays `enabled`
+    // but disconnected, with a `PoolAlreadyConnected` error: disconnecting or
+    // deleting it must not release the pool of the server which owns it
+    const { xenServers } = createXenServers(SERVER, { ...SERVER, id: 'server-2' })
+    connect(xenServers)
+    const xapi = xenServers._xapis['server-1']
+
+    await xenServers.disconnectXenServer('server-2')
+
+    assert.deepEqual({ ...xenServers._serverIdsByPool }, { 'pool-1': 'server-1' })
+    assert.equal(xenServers._xapis['server-1'], xapi)
+    assert.equal(xenServers._getXenServerStatus('server-1'), 'connected')
+  })
+
+  it('leaves the pools of the other servers alone', async function () {
+    const { xenServers } = createXenServers()
+    connect(xenServers)
+    connect(xenServers, { poolId: 'pool-2', serverId: 'server-2' })
+
+    await xenServers.disconnectXenServer('server-1')
+
+    assert.deepEqual({ ...xenServers._serverIdsByPool }, { 'pool-2': 'server-2' })
+  })
+})

packages/xo-server/src/xo-mixins/xen-servers.mjs

@@ -1,5 +1,4 @@
 import assert from 'assert'
-import findKey from 'lodash/findKey.js'
 import pick from 'lodash/pick.js'
 import { asyncEach } from '@vates/async-each'
 import { BaseError } from 'make-error'
@@ -25,7 +24,7 @@ import { getRpuTracesConfig, openRpuTrace } from '../_rpuObservability.mjs'
 import xapiObjectToXo from '../xapi-object-to-xo.mjs'
 import XapiStats from '../xapi-stats.mjs'
 import { autoReconnect } from '../_xenServerAutoReconnect.mjs'
-import { camelToSnakeCase, forEach, isEmpty, popProperty } from '../utils.mjs'
+import { camelToSnakeCase, forEach, isEmpty, popProperty, serializeError } from '../utils.mjs'
 import { Servers } from '../models/server.mjs'
 
 // ===================================================================
@@ -235,7 +234,19 @@ export default class XenServers {
 
     // an enabled server must not stay disconnected without retries, e.g. after
     // the user fixed its credentials or address
-    if (server.enabled && !this._connectingXenServers.has(id) && this._getXenServerStatus(id) === 'disconnected') {
+    //
+    // only an update which can actually change the outcome of a connection may
+    // (re-)arm the loop: `_connectXenServer` writes `{ error }` on every failed
+    // attempt, and that bookkeeping write must not restart a loop which just
+    // stopped on a permanent error (e.g. PoolAlreadyConnected), which would
+    // retry forever, at full speed since each loop restarts its own backoff
+    const canFixConnection = properties.enabled === true || connectionIdentityChanged
+    if (
+      canFixConnection &&
+      server.enabled &&
+      !this._connectingXenServers.has(id) &&
+      this._getXenServerStatus(id) === 'disconnected'
+    ) {
       this._autoReconnectXenServer(id)
     }
   }
@@ -275,8 +286,7 @@ export default class XenServers {
     forEach(newXapiObjects, function handleObject(xapiObject, xapiId) {
       // handle pool UUID change
       if (xapiObject.$type === 'pool' && serverIdsByPool[xapiObject.$id] === undefined) {
-        const obsoletePoolId = findKey(serverIdsByPool, serverId => serverId === conId)
-        delete serverIdsByPool[obsoletePoolId]
+        self._forgetXenServerPool(conId)
         serverIdsByPool[xapiObject.$id] = conId
       }
 
@@ -609,28 +619,31 @@ export default class XenServers {
 
       this.updateXenServer(id, { error: null })::ignoreErrors()
 
-      xapi.once('eventFetchingError', function eventFetchingErrorListener() {
+      const onEventFetchingError = () => {
         const timeout = setTimeout(() => {
           xapi.xo.uninstall()
 
           // switch server status from connected to connecting
-          delete serverIdsByPool[poolId]
+          this._forgetXenServerPool(server.id)
         }, this._xapiMarkDisconnectedDelay)
         xapi.once('eventFetchingSuccess', () => {
-          xapi.once('eventFetchingError', eventFetchingErrorListener)
+          xapi.once('eventFetchingError', onEventFetchingError)
           if (serverIdsByPool[poolId] === undefined) {
+            // the pool may now be known under another identifier, `install()`
+            // replays the pool object which registers it under the right one
             serverIdsByPool[poolId] = server.id
             xapi.xo.install()
           } else {
             clearTimeout(timeout)
           }
         })
-      })
+      }
+      xapi.once('eventFetchingError', onEventFetchingError)
 
       xapi.once('disconnected', () => {
         xapi.xo.uninstall()
         delete this._xapis[server.id]
-        delete this._serverIdsByPool[poolId]
+        this._forgetXenServerPool(server.id)
         this._app.emit('server:disconnected', { server, xapi })
 
         // deliberate disconnections set `enabled` to false beforehand, in
@@ -642,10 +655,12 @@ export default class XenServers {
       delete this._xapis[server.id]
       xapi.disconnect()::ignoreErrors()
 
+      const serializedError = serializeError(error)
+
       // avoid a database write per auto-reconnect attempt when the error did not change
       const previousError = server.error
-      if (previousError?.code !== error?.code || previousError?.message !== error?.message) {
-        this.updateXenServer(id, { error })::ignoreErrors()
+      if (previousError?.code !== serializedError.code || previousError?.message !== serializedError.message) {
+        this.updateXenServer(id, { error: serializedError })::ignoreErrors()
       }
 
       // permanent errors: retrying is pointless, do not start the loop
@@ -657,6 +672,21 @@ export default class XenServers {
     }
   }
 
+  // Removes every pool this server is registered as the connection of.
+  //
+  // The pool is looked up by server instead of by identifier because a pool
+  // UUID can change during the life of a connection (see `_onXenAdd`): a
+  // mapping left behind would make every subsequent connection to that pool
+  // fail with `PoolAlreadyConnected`, including the ones of this very server.
+  _forgetXenServerPool(serverId) {
+    const serverIdsByPool = this._serverIdsByPool
+    for (const poolId of Object.keys(serverIdsByPool)) {
+      if (serverIdsByPool[poolId] === serverId) {
+        delete serverIdsByPool[poolId]
+      }
+    }
+  }
+
   async disconnectXenServer(id) {
     // throw no such object if the server does not exist
     const server = await this.getXenServer(id)
@@ -678,11 +708,7 @@ export default class XenServers {
     const xapi = this._xapis[id]
     delete this._xapis[id]
 
-    const serverIdsByPool = this._serverIdsByPool
-    const poolId = findKey(serverIdsByPool, _ => _ === xapi)
-    if (poolId !== undefined) {
-      delete serverIdsByPool[id]
-    }
+    this._forgetXenServerPool(id)
 
     return xapi?.disconnect()
   }
@@ -719,7 +745,7 @@ export default class XenServers {
       lastEventFetchedTimestamp !== undefined &&
       Date.now() > lastEventFetchedTimestamp + this._xapiMarkDisconnectedDelay
     ) {
-      server.error = xapis[server.id].watchEventsError
+      server.error = serializeError(xapis[server.id].watchEventsError)
     }
     server.status = this._getXenServerStatus(server.id)
     if (server.status === 'connected') {