feat: catch and log electron-store.set errors (#2547)

* feat: add safe-store-set function and fix tests * feat: use safe-store-set * fix: confirm safeStoreSet callback is function before calling * test: mock store doesn't run migrations * fix: safe-store-set awaits onSuccessFn call * fix: safeStoreSet is now store.safeSet * chore: remove debugging code * chore: change log namespace for safeSet * chore: remove old import * chore: try to fix failing CI due to store migrations
ipfs · Aug 9, 2023 · 536710e · 536710e
1 parent 417875d
commit 536710e
Show file tree

Hide file tree

Showing 19 changed files with 207 additions and 80 deletions.
diff --git a/src/automatic-gc.js b/src/automatic-gc.js
@@ -24,9 +24,14 @@ function disable () {
   }
 }
 
+/**
+ *
+ * @param {string[]} newFlags
+ */
 function applyConfig (newFlags) {
-  store.set('ipfsConfig.flags', newFlags)
-  ipcMain.emit(ipcMainEvents.IPFS_CONFIG_CHANGED) // trigger node restart
+  store.safeSet('ipfsConfig.flags', newFlags, () => {
+    ipcMain.emit(ipcMainEvents.IPFS_CONFIG_CHANGED) // trigger node restart
+  })
 }
 
 module.exports = async function () {

diff --git a/src/common/config-keys.js b/src/common/config-keys.js
@@ -1,6 +1,6 @@
 /**
  * Configuration key constants for the features in IPFS Desktop.
- *
+ * @constant
  * @type {Record<import('../types').CONFIG_KEYS, string>}
  */
 const CONFIG_KEYS = {

diff --git a/src/common/logger.js b/src/common/logger.js
@@ -58,7 +58,7 @@ logger.info(`[meta] logs can be found on ${logsPath}`)
 
 /**
  *
- * @param {AnalyticsTimeOptions & import('countly-sdk-nodejs').CountlyAddEventOptions} args
+ * @param {AnalyticsTimeOptions & Omit<import('countly-sdk-nodejs').CountlyAddEventOptions, 'key'>} opts
  *
  * @returns {void} void
  */
@@ -79,11 +79,11 @@ module.exports = Object.freeze({
   /**
    *
    * @param {string} msg
-   * @param {AnalyticsTimeOptions} opts
+   * @param {AnalyticsTimeOptions & Omit<import('countly-sdk-nodejs').CountlyAddEventOptions, 'key'>} opts
    *
    * @returns {AnalyticsTimeReturnValue} AnalyticsTimeReturnValue
    */
-  start: (msg, opts = {}) => {
+  start: (msg, opts) => {
     const start = performance.now()
     logger.info(`${msg} STARTED`)
 
@@ -107,19 +107,35 @@ module.exports = Object.freeze({
   /**
    *
    * @param {string} msg
-   * @param {AnalyticsTimeOptions & import('countly-sdk-nodejs').CountlyAddEventOptions} opts
+   * @param {AnalyticsTimeOptions & Omit<import('countly-sdk-nodejs').CountlyAddEventOptions, 'key'>} [opts]
    *
    * @returns {void} void
    */
-  info: (msg, opts = {}) => {
-    addAnalyticsEvent({ count: 1, ...opts })
+  info: (msg, opts) => {
+    if (opts) {
+      addAnalyticsEvent({ count: 1, ...opts })
+    }
 
     logger.info(msg)
   },
 
-  error: (err) => {
-    Countly.log_error(err)
-    logger.error(err)
+  /**
+   *
+   * @param {Error|string} errMsg
+   * @param {Error|unknown} [error]
+   */
+  error: (errMsg, error) => {
+    if (errMsg instanceof Error) {
+      Countly.log_error(errMsg)
+      logger.error(errMsg)
+    } else if (error != null && error instanceof Error) {
+      // errorMessage is not an instance of an error, but error is
+      Countly.log_error(error)
+      logger.error(errMsg, error)
+    } else {
+      Countly.log_error(new Error(errMsg))
+      logger.error(errMsg, error)
+    }
   },
 
   warn: (msg, meta) => {
@@ -131,5 +147,9 @@ module.exports = Object.freeze({
   },
 
   logsPath,
-  addAnalyticsEvent
+  addAnalyticsEvent,
+  /**
+   * For when you want to log something without potentially emitting it to countly
+   */
+  fileLogger: logger
 })
diff --git a/src/common/store.js b/src/common/store.js
@@ -1,22 +1,30 @@
-const electron = require('electron')
+const { app } = require('electron')
 const Store = require('electron-store')
 
+const logger = require('./logger')
+
+const { fileLogger } = logger
+
+/**
+ * @type {import('./types').DesktopPersistentStore}
+ */
 const defaults = {
   ipfsConfig: {
-    type: 'go',
     path: '',
     flags: [
       '--agent-version-suffix=desktop',
       '--migrate',
       '--enable-gc'
     ]
   },
-  language: (electron.app || electron.remote.app).getLocale(),
-  experiments: {}
+  language: app.getLocale(),
+  experiments: {},
+  binaryPath: ''
 }
 
 const migrations = {
   '>=0.11.0': store => {
+    fileLogger.info('Running migration: >=0.11.0')
     store.delete('version')
 
     const flags = store.get('ipfsConfig.flags', [])
@@ -26,6 +34,7 @@ const migrations = {
     }
   },
   '>0.13.2': store => {
+    fileLogger.info('Running migration: >0.13.2')
     const flags = store.get('ipfsConfig.flags', [])
     const automaticGC = store.get('automaticGC', false)
     // ensure checkbox follows cli flag config
@@ -34,6 +43,7 @@ const migrations = {
     }
   },
   '>=0.17.0': store => {
+    fileLogger.info('Running migration: >=0.17.0')
     let flags = store.get('ipfsConfig.flags', [])
 
     // make sure version suffix is always present and normalized
@@ -53,6 +63,7 @@ const migrations = {
     }
   },
   '>=0.20.6': store => {
+    fileLogger.info('Running migration: >=0.20.6')
     let flags = store.get('ipfsConfig.flags', [])
 
     // use default instead of hard-coded dhtclient
@@ -64,7 +75,38 @@ const migrations = {
   }
 }
 
-const store = new Store({
+/**
+ * @extends {Store<import('./types').DesktopPersistentStore>}
+ */
+class StoreWrapper extends Store {
+  constructor (options) {
+    super(options)
+
+    /**
+     * @template {unknown} R
+     * @param {string} key
+     * @param {unknown} value
+     * @param {() => Promise<R>|R|void} [onSuccessFn]
+     * @returns {Promise<R|void>}
+     */
+    this.safeSet = async function (key, value, onSuccessFn) {
+      try {
+        this.set(key, value)
+        if (typeof onSuccessFn === 'function') {
+          try {
+            return await onSuccessFn()
+          } catch (err) {
+            logger.error(`[store.safeSet] Error calling onSuccessFn for '${key}'`, /** @type {Error} */(err))
+          }
+        }
+      } catch (err) {
+        logger.error(`[store.safeSet] Could not set store key '${key}' to '${value}'`, /** @type {Error} */(err))
+      }
+    }
+  }
+}
+
+const store = new StoreWrapper({
   defaults,
   migrations
 })

diff --git a/src/common/types.ts b/src/common/types.ts
@@ -0,0 +1,12 @@
+export interface DesktopPersistentStore_IpfsdConfig {
+  path: string,
+  flags: string[]
+}
+
+export interface DesktopPersistentStore {
+
+  ipfsConfig: DesktopPersistentStore_IpfsdConfig,
+  language: string,
+  experiments: Record<string, boolean>,
+  binaryPath?: string
+}
diff --git a/src/custom-ipfs-binary.js b/src/custom-ipfs-binary.js
@@ -40,23 +40,23 @@ async function setCustomBinary () {
       return
     }
 
-    store.set(SETTINGS_KEY, filePaths[0])
+    store.safeSet(SETTINGS_KEY, filePaths[0], () => {
+      opt = showDialog({
+        showDock: false,
+        title: i18n.t('setCustomIpfsBinarySuccess.title'),
+        message: i18n.t('setCustomIpfsBinarySuccess.message', { path: filePaths[0] }),
+        buttons: [
+          i18n.t('restart'),
+          i18n.t('close')
+        ]
+      })
 
-    opt = showDialog({
-      showDock: false,
-      title: i18n.t('setCustomIpfsBinarySuccess.title'),
-      message: i18n.t('setCustomIpfsBinarySuccess.message', { path: filePaths[0] }),
-      buttons: [
-        i18n.t('restart'),
-        i18n.t('close')
-      ]
-    })
+      logger.info(`[custom binary] updated to ${filePaths[0]}`)
 
-    logger.info(`[custom binary] updated to ${filePaths[0]}`)
-
-    if (opt === 0) {
-      restartIpfs()
-    }
+      if (opt === 0) {
+        restartIpfs()
+      }
+    })
   })
 }
 

diff --git a/src/daemon/config.js b/src/daemon/config.js
@@ -234,13 +234,13 @@ function migrateConfig (ipfsd) {
   if (changed) {
     try {
       writeConfigFile(ipfsd, config)
-      store.set(REVISION_KEY, REVISION)
+      store.safeSet(REVISION_KEY, REVISION)
     } catch (err) {
       logger.error(`[daemon] migrateConfig: error writing config file: ${err.message || err}`)
       return
     }
   }
-  store.set(REVISION_KEY, REVISION)
+  store.safeSet(REVISION_KEY, REVISION)
 }
 
 /**

diff --git a/src/daemon/index.js b/src/daemon/index.js
@@ -64,7 +64,7 @@ async function setupDaemon () {
     // not set.
     if (!config.path || typeof config.path !== 'string') {
       config.path = ipfsd.path
-      store.set('ipfsConfig', config)
+      store.safeSet('ipfsConfig', config)
     }
 
     log.end()

diff --git a/src/enable-namesys-pubsub.js b/src/enable-namesys-pubsub.js
@@ -25,12 +25,13 @@ function disable () {
 }
 
 function applyConfig (newFlags) {
-  store.set('ipfsConfig.flags', newFlags)
-  ipcMain.emit(ipcMainEvents.IPFS_CONFIG_CHANGED) // trigger node restart
+  store.safeSet('ipfsConfig.flags', newFlags, () => {
+    ipcMain.emit(ipcMainEvents.IPFS_CONFIG_CHANGED) // trigger node restart
+  })
 }
 
 module.exports = async function () {
-  const activate = ({ newValue, oldValue }) => {
+  const activate = ({ newValue, oldValue = null }) => {
     if (newValue === oldValue) return
 
     try {

diff --git a/src/enable-pubsub.js b/src/enable-pubsub.js
@@ -25,12 +25,13 @@ function disable () {
 }
 
 function applyConfig (newFlags) {
-  store.set('ipfsConfig.flags', newFlags)
-  ipcMain.emit(ipcMainEvents.IPFS_CONFIG_CHANGED) // trigger node restart
+  store.safeSet('ipfsConfig.flags', newFlags, () => {
+    ipcMain.emit(ipcMainEvents.IPFS_CONFIG_CHANGED) // trigger node restart
+  })
 }
 
 module.exports = async function () {
-  const activate = ({ newValue, oldValue }) => {
+  const activate = ({ newValue, oldValue = null }) => {
     if (newValue === oldValue) return
 
     try {

diff --git a/src/i18n.js b/src/i18n.js
@@ -32,7 +32,7 @@ module.exports = async function () {
       return
     }
 
-    store.set('language', lang)
+    store.safeSet('language', lang)
 
     await i18n.changeLanguage(lang)
     ipcMain.emit(ipcMainEvents.LANG_UPDATED, lang)

diff --git a/src/move-repository-location.js b/src/move-repository-location.js
@@ -69,23 +69,25 @@ module.exports = function ({ stopIpfs, startIpfs }) {
       await fs.move(currDir, newDir)
       logger.info(`[move repository] moved from ${currDir} to ${newDir}`)
     } catch (err) {
-      logger.error(`[move repository] ${err.toString()}`)
+      logger.error(`[move repository] error moving from '${currDir}' to '${newDir}'`, err)
       return recoverableErrorDialog(err, {
         title: i18n.t('moveRepositoryFailed.title'),
         message: i18n.t('moveRepositoryFailed.message', { currDir, newDir })
       })
     }
 
     config.path = newDir
-    store.set('ipfsConfig', config)
-    logger.info('[move repository] configuration updated', { withAnalytics: analyticsKeys.MOVE_REPOSITORY })
 
-    showDialog({
-      title: i18n.t('moveRepositorySuccessDialog.title'),
-      message: i18n.t('moveRepositorySuccessDialog.message', { location: newDir }),
-      showDock: false
-    })
+    await store.safeSet('ipfsConfig', config, async () => {
+      logger.info('[move repository] configuration updated', { withAnalytics: analyticsKeys.MOVE_REPOSITORY })
+
+      showDialog({
+        title: i18n.t('moveRepositorySuccessDialog.title'),
+        message: i18n.t('moveRepositorySuccessDialog.message', { location: newDir }),
+        showDock: false
+      })
 
-    await startIpfs()
+      await startIpfs()
+    })
   })
 }
diff --git a/src/tray.js b/src/tray.js
@@ -405,8 +405,7 @@ module.exports = async function () {
   await setupMenu()
 
   createToggler(CONFIG_KEYS.MONOCHROME_TRAY_ICON, async ({ newValue }) => {
-    store.set(CONFIG_KEYS.MONOCHROME_TRAY_ICON, newValue)
-    return true
+    return store.safeSet(CONFIG_KEYS.MONOCHROME_TRAY_ICON, newValue, () => true)
   })
 
   ctx.setProp('tray', tray)

diff --git a/src/utils/create-toggler.js b/src/utils/create-toggler.js
@@ -9,10 +9,10 @@ module.exports = function (settingsOption, activate) {
     const newValue = !oldValue
 
     if (await activate({ newValue, oldValue, feedback: true })) {
-      store.set(settingsOption, newValue)
-
-      const action = newValue ? 'enabled' : 'disabled'
-      logger.info(`[${settingsOption}] ${action}`)
+      store.safeSet(settingsOption, newValue, () => {
+        const action = newValue ? 'enabled' : 'disabled'
+        logger.info(`[${settingsOption}] ${action}`)
+      })
     }
 
     // We always emit the event so any handlers for it can act upon