feat(history): record the first registered provider as the default

docs/develop/rest-api/history_api.md

@@ -110,7 +110,7 @@ Returns an array of path strings.
 
 The History API supports the registration of multiple history provider plugins.
 
-The first plugin registered is set as the _default_ provider and all requests will be directed to it.
+The first plugin to register becomes the _default_ provider and all requests are directed to it. When settings name no default, and the settings the server started with are safe to save, it records that first provider — so the choice holds across restarts and a plugin installed later does not take it over. A default already named in settings is never replaced, and a server started with `--data` or sample data records nothing, keeping the first provider for that run only. Change the default in the Admin UI under Apps & Plugins -> Configuration.
 
 Requests can be directed to a specific provider by using the `provider` parameter in the request with the _id_ of the provider plugin.
 

src/api/history/index.ts

@@ -66,6 +66,15 @@ export class HistoryApiHttpRegistry {
    * the HISTORYPROVIDERS serverevent. */
   private unavailableGraceExpired = false
   private unavailableGraceTimer: ReturnType<typeof setTimeout> | null = null
+  /** True once a failed recording has been reported for this run. */
+  private recordFailureReported = false
+  /** The provider chosen as default whose write has not landed yet.
+   * Held across later registrations so a failed write is retried by the
+   * next one: without it the arrival of a second provider ends the
+   * attempts, and the default reverts to registration order on every
+   * start. Cleared when that provider unregisters, since recording a
+   * provider that has gone would be worse than the fallback. */
+  private pendingDefaultProviderId?: string
   proxy: HistoryApi
 
   /** The configured provider when it is registered, otherwise the first
@@ -85,7 +94,15 @@ export class HistoryApiHttpRegistry {
     private app: HistoryApplication,
     private unavailableGraceMs: number = UNAVAILABLE_GRACE_MS
   ) {
-    this.configuredProviderId = app.config.settings.historyApi?.defaultProvider
+    // No supported path writes an empty id — the POST route rejects a
+    // falsy one — so an empty value comes from a hand-edited file and
+    // means nothing was chosen. Reading it as absent keeps every
+    // consumer agreeing: defaultProviderId and the notification already
+    // test truthiness, while the grace window and the state event tested
+    // for undefined, so an empty id armed the window and the Admin UI
+    // reported a default named "" as unavailable.
+    this.configuredProviderId =
+      app.config.settings.historyApi?.defaultProvider || undefined
     this.proxy = {
       getValues: (query: ValuesRequest): Promise<ValuesResponse> => {
         return this.defaultProvider().getValues(query)
@@ -128,6 +145,7 @@ export class HistoryApiHttpRegistry {
       this.unavailableGraceExpired = false
       this.notifyConfiguredAvailable()
     }
+    this.recordDefaultIfUnconfigured(pluginId)
     this.emitProvidersState()
     debug(
       `Registered history api provider ${pluginId},`,
@@ -136,11 +154,66 @@ export class HistoryApiHttpRegistry {
     )
   }
 
+  /** Persist the first provider to register when settings name none.
+   *
+   * Unrecorded, the default is resolved from registration order on
+   * every start, and `startPlugins` does not await `start()`: a plugin
+   * that registers once its database answers loses the slot to one that
+   * registers from `start()` itself. Recording it makes the first
+   * outcome the lasting one, so a provider installed later cannot take
+   * the default from the one already serving.
+   */
+  private recordDefaultIfUnconfigured(pluginId: string): void {
+    if (this.configuredProviderId !== undefined) {
+      return
+    }
+    if (!this.app.config.safeToPersistSettings) {
+      debug.enabled &&
+        debug(
+          `Not recording ${pluginId}: settings are not the configured state`
+        )
+      return
+    }
+    if (this.pendingDefaultProviderId === undefined) {
+      // Only the first provider claims the slot; later ones just give a
+      // failed write another chance at the one already claimed.
+      if (this.historyProviders.size !== 1) {
+        return
+      }
+      this.pendingDefaultProviderId = pluginId
+    }
+    const candidate = this.pendingDefaultProviderId
+    this.saveConfiguredProvider(candidate, (err?: Error) => {
+      if (!err) {
+        this.pendingDefaultProviderId = undefined
+        return
+      }
+      // A plugin that reconnects in a loop registers each time, and
+      // console.error reaches the 100-entry log ring the admin UI
+      // subscribes to. Reporting every attempt would push out the
+      // plugin errors that explain the loop, so the retry stays and
+      // only the first report does.
+      if (!this.recordFailureReported) {
+        this.recordFailureReported = true
+        console.error(
+          `Failed to record ${candidate} as the default history provider:`,
+          err.message
+        )
+      } else {
+        debug.enabled &&
+          debug(`Failed to record ${candidate} again: ${err.message}`)
+      }
+    })
+  }
+
   unregisterHistoryApiProvider(pluginId: string): void {
     if (!pluginId || !this.historyProviders.has(pluginId)) {
       return
     }
     this.historyProviders.delete(pluginId)
+    if (pluginId === this.pendingDefaultProviderId) {
+      this.pendingDefaultProviderId = undefined
+    }
     if (pluginId === this.configuredProviderId) {
       this.armUnavailableGrace()
     }

src/api/history/openApi.ts

@@ -356,7 +356,7 @@ historyApiDoc.paths = {
                   configured: {
                     type: 'string',
                     description:
-                      'Provider identifier persisted in server settings. May differ from `id` when the configured provider is not currently registered.'
+                      'Provider identifier persisted in server settings, set either by a client through this API or by the server itself, which records the first provider to register when no default is configured and its settings are safe to save. May differ from `id` when the configured provider is not currently registered, and is absent on a server that has recorded nothing.'
                   }
                 },
                 example: { id: 'signalk-to-influxdb2' }

src/config/config.ts

@@ -77,6 +77,26 @@ export interface Config {
   baseDeltaEditor: DeltaEditor
   hasOldDefaults: boolean
   overrideTimestampWithNow: boolean
+  /** True while `settings` represents what the user configured, so it
+   * may be saved back to their file. Cleared in two cases: `--data` and
+   * the sample data flags change the data connections that are in
+   * effect for this run, and a settings file that exists but cannot be
+   * parsed falls back to empty settings. `--data` disables every
+   * configured provider and adds a FileStream for the log; the sample
+   * flags add a FileStream provider beside the configured ones.
+   * Writers that persist settings without a user asking must check
+   * this.
+   *
+   * A missing settings file does not clear it: there is nothing to
+   * overwrite, and a fresh install should still be able to save.
+   *
+   * Neither do the environment variables that set a single key
+   * (TRUST_PROXY, SSLPORT, WSCOMPRESSION) or --securityenabled. Those
+   * configure a container for the life of the installation, so
+   * blocking on them would stop automatic writes from ever succeeding
+   * there. They can still reach the file through any write, which is a
+   * wider problem than this flag solves. */
+  safeToPersistSettings: boolean
   security: boolean
   settings: {
     useBaseDeltas?: boolean
@@ -171,7 +191,9 @@ export interface Config {
        * Applied whenever the provider is registered, so the default does
        * not depend on plugin load order. When the configured provider is
        * not registered (e.g. plugin disabled), the first registered
-       * provider serves as fallback. */
+       * provider serves as fallback. Set through the History API, or by
+       * the server itself when a provider registers while this key is
+       * absent and safeToPersistSettings is set. */
       defaultProvider?: string
     }
     notifications?: {
@@ -203,6 +225,7 @@ export function load(app: ConfigApp) {
 
   config.appPath = config.appPath || path.normalize(__dirname + '/../../')
   debug('appPath:' + config.appPath)
+  config.safeToPersistSettings = true
 
   try {
     config.name = packageJson.name
@@ -231,6 +254,12 @@ export function load(app: ConfigApp) {
   if (_.isObject(app.config.settings)) {
     debug('Using settings from constructor call, not reading defaults')
     disableWriteSettings = true
+    // The settings came from the caller, not from the user's file, so
+    // there is nothing they may be saved back over. writeSettingsFile
+    // already refuses; clearing this stops the writers that keep their
+    // own files, such as the sourceRef migration's channel labels, from
+    // persisting half of a change whose other half is refused.
+    config.safeToPersistSettings = false
     if (config.defaults) {
       convertOldDefaultsToDeltas(app.config.baseDeltaEditor, config.defaults)
     }
@@ -296,6 +325,7 @@ export function load(app: ConfigApp) {
       ],
       enabled: true
     })
+    app.config.safeToPersistSettings = false
   }
 
   if (app.argv['sample-n2k-data']) {
@@ -318,6 +348,7 @@ export function load(app: ConfigApp) {
       ],
       enabled: true
     })
+    app.config.safeToPersistSettings = false
   }
 
   if (app.argv.data) {
@@ -356,6 +387,7 @@ export function load(app: ConfigApp) {
       ],
       enabled: true
     })
+    app.config.safeToPersistSettings = false
   }
 
   if (app.argv['override-timestamps']) {
@@ -590,6 +622,9 @@ function readSettingsFile(app: ConfigApp) {
       app.config.settings = {
         pipedProviders: []
       }
+      // The file is still there and still the user's, so nothing may
+      // save these empty settings over it while they are unreadable.
+      app.config.safeToPersistSettings = false
     }
   }
   if (_.isUndefined(app.config.settings.pipedProviders)) {

src/sourceref-migration.ts

@@ -11,6 +11,8 @@ const LABELS_FILENAME = 'n2k-channel-labels.json'
 interface MigrationApp {
   config: {
     configPath: string
+    /** See Config.safeToPersistSettings. */
+    safeToPersistSettings: boolean
     settings: {
       priorityOverrides?: Record<
         string,
@@ -97,7 +99,12 @@ export function migrateSourceRef(
   }
   const settings = app.config.settings
   let settingsChanged = false
+  let labelsChanged = false
   const migrated = new Set<string>()
+  // Both files this function writes are covered: persisting one half of
+  // a migration would leave the channel labels keyed by newRef while
+  // settings still name oldRef.
+  const mayPersist = app.config.safeToPersistSettings
 
   // 1. priorityOverrides (path-level) — dedupe per path if newRef already present
   if (settings.priorityOverrides) {
@@ -197,18 +204,21 @@ export function migrateSourceRef(
       }
     }
     if (labelUpdates.length > 0) {
+      labelsChanged = true
       for (const [oldKey, newKey, value] of labelUpdates) {
         delete labels[oldKey]
         if (!(newKey in labels)) {
           labels[newKey] = value
         }
       }
-      // Sync write keeps callers (including the migration tests) able to
-      // observe the new file shape immediately on return. Channel labels
-      // files are tiny (one entry per N2K instance) so the blocking cost
-      // is negligible at startup.
-      fs.writeFileSync(labelsPath, JSON.stringify(labels, null, 2))
-      migrated.add('channelLabels')
+      if (mayPersist) {
+        // Sync write keeps callers (including the migration tests) able to
+        // observe the new file shape immediately on return. Channel labels
+        // files are tiny (one entry per N2K instance) so the blocking cost
+        // is negligible at startup.
+        fs.writeFileSync(labelsPath, JSON.stringify(labels, null, 2))
+        migrated.add('channelLabels')
+      }
     }
   } catch (e: unknown) {
     if ((e as NodeJS.ErrnoException).code !== 'ENOENT') {
@@ -259,7 +269,22 @@ export function migrateSourceRef(
     )
   }
 
-  if (settingsChanged) {
+  if ((settingsChanged || labelsChanged) && !mayPersist) {
+    // Saving would put this run's replaced data connections, or empty
+    // settings standing in for a file that would not parse, into the
+    // user's settings.json. The rewrites still apply in memory, so the
+    // running server is consistent with itself, but the rename is not
+    // deferred: migration runs from the sourceRefChanged event, and a
+    // later start of a device already at its new address raises no such
+    // event. The refs are logged so the rename can be made by hand.
+    // Channel labels count as a change of their own: a device whose ref
+    // appears only there still needs the hand rename, and nothing else
+    // would report it.
+    console.log(
+      `sourceRef migration ${oldRef} -> ${newRef} not persisted: settings do not represent the configured state`
+    )
+    finalize()
+  } else if (settingsChanged) {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     writeSettingsFile(app as any, settings, (err: Error) => {
       if (err) {
@@ -273,7 +298,8 @@ export function migrateSourceRef(
       finalize()
     })
   } else {
-    // Nothing to persist (e.g. only channelLabels file was rewritten).
+    // Nothing left to persist: either nothing matched, or the channel
+    // labels file was the only match and has already been written.
     finalize()
   }
 }