Refactor [github] token persistence, again (#1906)

Instead of saving tokens on a timer, save them when they change. Use EventEmitter to keep the components loosely coupled.

This is easier to reason about, much easier to test, and better supports adapting to backends which support atomic operations.

This replaces json-autosave, which was a bit difficult to read and also hard to test, with fsos, the lower-level utility it’s built on.

Ref: #1848

lib/github-auth.js

@@ -1,5 +1,6 @@
 'use strict'
 
+const { EventEmitter } = require('events')
 const crypto = require('crypto')
 const log = require('./log')
 const secretIsValid = require('./sys/secret-is-valid')
@@ -15,6 +16,8 @@ let githubUserTokens = []
 const reqRemaining = new Map() // From token to requests remaining.
 const reqReset = new Map() // From token to timestamp.
 
+const emitter = new EventEmitter()
+
 // Personal tokens allow access to GitHub private repositories.
 // You can manage your personal GitHub token at
 // <https://github.com/settings/tokens>.
@@ -110,6 +113,7 @@ function setRoutes(server) {
       }, 10000)
     }
     addGithubToken(data.token)
+    emitter.emit('token-added', data.token)
     end('Thanks!')
   })
 }
@@ -312,6 +316,7 @@ function githubRequest(request, url, query, cb) {
       if (res.statusCode === 401) {
         // Unauthorized.
         rmGithubToken(githubToken)
+        emitter.emit('token-removed', githubToken)
       } else {
         const remaining = +res.headers['x-ratelimit-remaining']
         // reset is in UTC epoch seconds.
@@ -331,6 +336,8 @@ module.exports = {
   setRoutes,
   serializeDebugInfo,
   addGithubToken,
+  rmGithubToken,
   getAllTokenIds,
   removeAllTokens,
+  emitter,
 }

lib/token-persistence.js

@@ -1,43 +1,55 @@
 'use strict'
 
-const autosave = require('json-autosave')
+const fsos = require('fsos')
 const githubAuth = require('./github-auth')
+const log = require('./log')
 
 // This is currently bound to the legacy github auth code. That will be
 // replaced with a dependency-injected token provider.
 class TokenPersistence {
   constructor({ path }) {
     this.path = path
-    this.save = undefined
+
+    this.noteTokenAdded = this.noteTokenAdded.bind(this)
+    this.noteTokenRemoved = this.noteTokenRemoved.bind(this)
   }
 
   async initialize() {
-    // This code is a bit difficult to understand, in part because it's
-    // working against the interface of `json-autosave` which wants to own the
-    // data structure.
-    const save = await autosave(this.path, { data: [] })
-    this.save = save
+    let contents
+    try {
+      contents = await fsos.get(this.path)
+    } catch (e) {
+      if (e.code === 'ENOENT') {
+        contents = '[]'
+      } else {
+        throw e
+      }
+    }
 
-    save.data.forEach(tokenString => {
+    const tokens = JSON.parse(contents)
+    tokens.forEach(tokenString => {
       githubAuth.addGithubToken(tokenString)
     })
-
-    // Override the autosave handler to refresh the token data before
-    // saving.
-    save.autosave = () => {
-      save.data = githubAuth.getAllTokenIds()
-      return save.save()
-    }
-    // Put the change in autosave handler into effect.
-    save.stop()
-    save.start()
   }
 
-  async stop() {
-    if (this.save) {
-      this.save.stop()
-      await this.save.autosave()
-      this.save = undefined
+  async save() {
+    const tokens = githubAuth.getAllTokenIds()
+    await fsos.set(this.path, JSON.stringify(tokens))
+  }
+
+  async noteTokenAdded(token) {
+    try {
+      await this.save()
+    } catch (e) {
+      log.error(e)
+    }
+  }
+
+  async noteTokenRemoved(token) {
+    try {
+      await this.save()
+    } catch (e) {
+      log.error(e)
     }
   }
 }

lib/token-persistence.spec.js

@@ -3,22 +3,11 @@
 const fs = require('fs')
 const tmp = require('tmp')
 const readFile = require('fs-readfile-promise')
-const sinon = require('sinon')
-const { sleep } = require('wait-promise')
 const { expect } = require('chai')
 const TokenPersistence = require('./token-persistence')
 const githubAuth = require('./github-auth')
 
 describe('Token persistence', function() {
-  // Fake timers must be set up before any timers are scheduled.
-  let clock
-  beforeEach(function() {
-    clock = sinon.useFakeTimers()
-  })
-  afterEach(function() {
-    clock.restore()
-  })
-
   beforeEach(githubAuth.removeAllTokens)
   afterEach(githubAuth.removeAllTokens)
 
@@ -27,67 +16,63 @@ describe('Token persistence', function() {
     path = tmp.tmpNameSync()
     persistence = new TokenPersistence({ path })
   })
-  afterEach(function() {
-    if (persistence) {
-      persistence.stop()
-      persistence = null
-    }
-  })
 
   context('when the file does not exist', function() {
-    it('creates it with an empty array', async function() {
+    it('does nothing', async function() {
       await persistence.initialize()
-      const json = JSON.parse(await readFile(path))
+      expect(githubAuth.getAllTokenIds()).to.deep.equal([])
+    })
 
-      expect(json).to.deep.equal([])
+    it('saving creates an empty file', async function() {
+      await persistence.initialize()
+
+      await persistence.save()
+
+      const json = JSON.parse(await readFile(path))
+      expect(json).to.deep.deep.equal([])
     })
   })
 
   context('when the file exists', function() {
-    it('loads the contents', async function() {
-      const initialTokens = ['a', 'b', 'c'].map(char => char.repeat(40))
+    const initialTokens = ['a', 'b', 'c'].map(char => char.repeat(40))
+
+    beforeEach(async function() {
       fs.writeFileSync(path, JSON.stringify(initialTokens))
+    })
 
+    it('loads the contents', async function() {
       await persistence.initialize()
-
       expect(githubAuth.getAllTokenIds()).to.deep.equal(initialTokens)
     })
-  })
 
-  context('when shutting down', function() {
-    it('writes added tokens to the file', async function() {
-      const initialTokens = ['a', 'b', 'c'].map(char => char.repeat(40))
-      fs.writeFileSync(path, JSON.stringify(initialTokens))
+    context('when tokens are added', function() {
+      it('writes them to the file', async function() {
+        const newToken = 'e'.repeat(40)
+        const expected = initialTokens.slice()
+        expected.push(newToken)
 
-      const newToken = 'e'.repeat(40)
-      const expected = initialTokens.slice()
-      expected.push(newToken)
+        await persistence.initialize()
+        githubAuth.addGithubToken(newToken)
+        await persistence.noteTokenAdded(newToken)
 
-      await persistence.initialize()
-      githubAuth.addGithubToken(newToken)
-      await persistence.stop()
-
-      const json = JSON.parse(await readFile(path))
-      expect(json).to.deep.equal(expected)
+        const json = JSON.parse(await readFile(path))
+        expect(json).to.deep.equal(expected)
+      })
     })
-  })
 
-  context('time has elapsed', function() {
-    it('writes added tokens to the file', async function() {
-      const addedTokens = ['d', 'e'].map(char => char.repeat(40))
+    context('when tokens are removed', function() {
+      it('writes them to the file', async function() {
+        const expected = Array.from(initialTokens)
+        const toRemove = expected.pop()
 
-      await persistence.initialize()
+        await persistence.initialize()
 
-      addedTokens.forEach(githubAuth.addGithubToken)
+        githubAuth.rmGithubToken(toRemove)
+        await persistence.noteTokenRemoved(toRemove)
 
-      // Fake time passing to trigger autosaving, then give the save a brief
-      // moment of real time to complete before reading.
-      clock.tick(5000)
-      clock.restore()
-      await sleep(200)
-
-      const json = JSON.parse(await readFile(path))
-      expect(json).to.deep.equal(addedTokens)
+        const json = JSON.parse(await readFile(path))
+        expect(json).to.deep.equal(expected)
+      })
     })
   })
 })

package-lock.json

@@ -7041,14 +7041,6 @@
         "yargs": "^11.0.0"
       }
     },
-    "json-autosave": {
-      "version": "1.1.2",
-      "resolved": "https://registry.npmjs.org/json-autosave/-/json-autosave-1.1.2.tgz",
-      "integrity": "sha1-v8TJsXeRn0vpV3iWf5HfGKQRo3E=",
-      "requires": {
-        "fsos": "~1.1.0"
-      }
-    },
     "json-loader": {
       "version": "0.5.7",
       "resolved": "https://registry.npmjs.org/json-loader/-/json-loader-0.5.7.tgz",

package.json

@@ -28,12 +28,12 @@
     "dot": "~1.1.2",
     "emojic": "^1.1.14",
     "escape-string-regexp": "^1.0.5",
+    "fsos": "^1.1.3",
     "glob": "^7.1.1",
     "gm": "^1.23.0",
     "is-css-color": "^1.0.0",
     "joi": "^13.3.0",
     "js-yaml": "^3.11.0",
-    "json-autosave": "~1.1.2",
     "jsonpath": "~1.0.0",
     "lodash.countby": "^4.6.0",
     "lodash.mapkeys": "^4.6.0",

services/github/github-constellation.js

@@ -39,10 +39,12 @@ class GithubConstellation {
     try {
       await this.persistence.initialize()
     } catch (e) {
-      // TODO Send to sentry.
-      console.error(e)
+      log.error(e)
     }
 
+    githubAuth.emitter.on('token-added', this.persistence.noteTokenAdded)
+    githubAuth.emitter.on('token-removed', this.persistence.noteTokenRemoved)
+
     setAdminRoutes(server)
 
     if (serverSecrets && serverSecrets.gh_client_id) {
@@ -56,8 +58,14 @@ class GithubConstellation {
       this.debugInterval = undefined
     }
 
-    await this.persistence.stop()
-    this.persistence = undefined
+    githubAuth.emitter.removeListener(
+      'token-added',
+      this.persistence.noteTokenAdded
+    )
+    githubAuth.emitter.removeListener(
+      'token-removed',
+      this.persistence.noteTokenRemoved
+    )
   }
 }